| Message ID | 20160605174309.GE26814@localhost.localdomain |
|---|---|
| State | Deferred |
| Headers | show |
Hello, On 5 June 2016 at 19:43, Ladislav Michl <ladis@linux-mips.org> wrote: > Some CPUs contains boot ROM code capable reading first few blocks > (where SPL resides) of NAND flash and executing it. It is wise to > create separate partition here for SPL. As block size depends on > NAND chip used, we could either use worst case (biggest) partition > size or base its size on actual block size. This patch adds support > for the latter option. > There is similar problem on sunxi. Given this flash is non-removable and has many pins you are unlikely going to encounter its content on any system that was not loaded with u-boot. Still Linux can only produce fixed size mtdparts. You can prepend perfectly sized mtdparts for u-boot but until Linux is taught to follow that with its own partitions without gap you still need to use the worst case scenario for the start of the Linux partitions. On sunxi the range of supported block sizes and the size of the SPL+U-BOOT are not large so this is generally not worth the effort. If support for this is attempted Linux should be probably taught to get mtdparts in pages and eraseblocks and then the default mtdparts can be in those. If on the other hand you generate the fixed layout for Linux partitions on the fly and this patch changes how the Linux partitions are generated it can happen that the Linux partitions are at different place with different versions of u-boot giving potentially disastrous results. Thanks Miichal
On Sun, Jun 05, 2016 at 07:58:46PM +0200, Michal Suchanek wrote: > Hello, > > On 5 June 2016 at 19:43, Ladislav Michl <ladis@linux-mips.org> wrote: > > Some CPUs contains boot ROM code capable reading first few blocks > > (where SPL resides) of NAND flash and executing it. It is wise to > > create separate partition here for SPL. As block size depends on > > NAND chip used, we could either use worst case (biggest) partition > > size or base its size on actual block size. This patch adds support > > for the latter option. > > > > There is similar problem on sunxi. > > Given this flash is non-removable and has many pins you are unlikely > going to encounter its content on any system that was not loaded with > u-boot. > > Still Linux can only produce fixed size mtdparts. You can prepend > perfectly sized mtdparts for u-boot but until Linux is taught to > follow that with its own partitions without gap you still need to use > the worst case scenario for the start of the Linux partitions. I didn't get 'fixed size mtdparts' part. Linux supports cmdline partition layout passing since the dawn of mtd subsystem (I used it more than a decade ago for netstar board). Finally that's a reason mtdparts implementation in U-Boot is done this way. Both U-Boot and Linux (can) see the same partition layout as it is passed either via kernel cmdline or device tree blob. > On sunxi the range of supported block sizes and the size of the > SPL+U-BOOT are not large so this is generally not worth the effort. > > If support for this is attempted Linux should be probably taught to > get mtdparts in pages and eraseblocks and then the default mtdparts > can be in those. > > If on the other hand you generate the fixed layout for Linux > partitions on the fly and this patch changes how the Linux partitions > are generated it can happen that the Linux partitions are at different > place with different versions of u-boot giving potentially disastrous > results. Unless I'm missing something, partition layout is passed to the kernel from the bootloader. So if kernel ends up with a different layout than a bootloader, there's a bug somewhere and that should be fixed. Regards, ladis
On 5 June 2016 at 20:23, Ladislav Michl <ladis@linux-mips.org> wrote: > On Sun, Jun 05, 2016 at 07:58:46PM +0200, Michal Suchanek wrote: >> Hello, >> >> On 5 June 2016 at 19:43, Ladislav Michl <ladis@linux-mips.org> wrote: >> > Some CPUs contains boot ROM code capable reading first few blocks >> > (where SPL resides) of NAND flash and executing it. It is wise to >> > create separate partition here for SPL. As block size depends on >> > NAND chip used, we could either use worst case (biggest) partition >> > size or base its size on actual block size. This patch adds support >> > for the latter option. >> > >> >> There is similar problem on sunxi. >> >> Given this flash is non-removable and has many pins you are unlikely >> going to encounter its content on any system that was not loaded with >> u-boot. >> >> Still Linux can only produce fixed size mtdparts. You can prepend >> perfectly sized mtdparts for u-boot but until Linux is taught to >> follow that with its own partitions without gap you still need to use >> the worst case scenario for the start of the Linux partitions. > > I didn't get 'fixed size mtdparts' part. Linux supports cmdline > partition layout passing since the dawn of mtd subsystem (I used it more Sure. It does not, however, support size in anything but some kind of fixed size blocks. It cannot adjust the layout for different media page size or erase block size. It cannot specify that a partition follows after another or extends to the end of medium. What you specify on the commandline is in no way related to the actual medium you are partitioning. > than a decade ago for netstar board). Finally that's a reason mtdparts > implementation in U-Boot is done this way. Both U-Boot and Linux (can) > see the same partition layout as it is passed either via kernel cmdline > or device tree blob. > >> On sunxi the range of supported block sizes and the size of the >> SPL+U-BOOT are not large so this is generally not worth the effort. >> >> If support for this is attempted Linux should be probably taught to >> get mtdparts in pages and eraseblocks and then the default mtdparts >> can be in those. >> >> If on the other hand you generate the fixed layout for Linux >> partitions on the fly and this patch changes how the Linux partitions >> are generated it can happen that the Linux partitions are at different >> place with different versions of u-boot giving potentially disastrous >> results. > > Unless I'm missing something, partition layout is passed to the kernel > from the bootloader. So if kernel ends up with a different layout than > a bootloader, there's a bug somewhere and that should be fixed. That's not it. The problem is that if this patch changes the layout then building u-boot before this patch gives one layout and after this patch it gives another layout effectively changing the partitioning. All to save a few megabytes of a several gigiabyte medium. And if you say that people can always set the partitioning by hand then it completely defeats the purpose of fine-tuning the default in the first place. If on the other hand Linux got support for sizing partitions in nand pages or eraseblocks and a patch changed the mtdparts layout to use the new units then either both u-boot and Linux support the units or parsing the partition fails. So it's safe and flexible and more general and probably even less work on u-boot's part. Thanks Michal
On Mon, Jun 06, 2016 at 09:08:47AM +0200, Michal Suchanek wrote: > On 5 June 2016 at 20:23, Ladislav Michl <ladis@linux-mips.org> wrote: > > On Sun, Jun 05, 2016 at 07:58:46PM +0200, Michal Suchanek wrote: > >> There is similar problem on sunxi. > >> > >> Given this flash is non-removable and has many pins you are unlikely > >> going to encounter its content on any system that was not loaded with > >> u-boot. > >> > >> Still Linux can only produce fixed size mtdparts. You can prepend > >> perfectly sized mtdparts for u-boot but until Linux is taught to > >> follow that with its own partitions without gap you still need to use > >> the worst case scenario for the start of the Linux partitions. > > > > I didn't get 'fixed size mtdparts' part. Linux supports cmdline > > partition layout passing since the dawn of mtd subsystem (I used it more > > Sure. > > It does not, however, support size in anything but some kind of fixed > size blocks. > > It cannot adjust the layout for different media page size or erase > block size. It cannot specify that a partition follows after another > or extends to the end of medium. That's not actually needed, as we know both page size and erase block size. Therefore it is easy to make layout respect those values, which was actual motivation for this patch. The later is simply not true, as you can certainly create a partition that extends to the end of medium: mtdparts=omap2-nand.0:512k(xloader),1920k(uboot),128k(uboot-env),10m(boot),-(rootfs) Above is something that I want to avoid and replace with: mtdparts=omap2-nand.0:<4*size_of_block>(SPL),-(UBI) (actually this is going to be translated to DTB before booting kernel) More free eraseblock for UBI, the better... Boot ROM reads from first (4) bare sector(s) and everything else is stored in UBI volumes. This is currently the only reliable way to make a use of MLC NAND. > What you specify on the commandline is in no way related to the actual > medium you are partitioning. What I'm specifying on cmdline (DTB) is what I'm going to get. I has always worked this way. Well, some drivers of even board support files had partitioning hardcoded, but we are trying to get rid of that. [snip] > > Unless I'm missing something, partition layout is passed to the kernel > > from the bootloader. So if kernel ends up with a different layout than > > a bootloader, there's a bug somewhere and that should be fixed. > > That's not it. The problem is that if this patch changes the layout > then building u-boot before this patch gives one layout and after this > patch it gives another layout effectively changing the partitioning. > All to save a few megabytes of a several gigiabyte medium. And if you > say that people can always set the partitioning by hand then it > completely defeats the purpose of fine-tuning the default in the first > place. U-Boot never prevented people from shooting their own leg, therefore there's no 'feature' preventing them to change default layout. They are still free to do whatever they want it they find it useful. Also I'm not proposing to change layout for exiting boards, just proving way to do it. igep00x0 boards had buggy NAND support since begining, so while fixing that, let's do it right way. Note that this feature is completely optional. > If on the other hand Linux got support for sizing partitions in nand > pages or eraseblocks and a patch changed the mtdparts layout to use > the new units then either both u-boot and Linux support the units or > parsing the partition fails. So it's safe and flexible and more > general and probably even less work on u-boot's part. Okay, specifing size in therms of eraseblock would solve my problem and I have to admit I'm not aware such a thing exists. Any pointer to the patch introducing that change? Thank you, ladis
On 6 June 2016 at 09:48, Ladislav Michl <ladis@linux-mips.org> wrote: > On Mon, Jun 06, 2016 at 09:08:47AM +0200, Michal Suchanek wrote: >> On 5 June 2016 at 20:23, Ladislav Michl <ladis@linux-mips.org> wrote: >> > Unless I'm missing something, partition layout is passed to the kernel >> > from the bootloader. So if kernel ends up with a different layout than >> > a bootloader, there's a bug somewhere and that should be fixed. >> >> That's not it. The problem is that if this patch changes the layout >> then building u-boot before this patch gives one layout and after this >> patch it gives another layout effectively changing the partitioning. >> All to save a few megabytes of a several gigiabyte medium. And if you >> say that people can always set the partitioning by hand then it >> completely defeats the purpose of fine-tuning the default in the first >> place. > > U-Boot never prevented people from shooting their own leg, therefore > there's no 'feature' preventing them to change default layout. They > are still free to do whatever they want it they find it useful. > Also I'm not proposing to change layout for exiting boards, just > proving way to do it. igep00x0 boards had buggy NAND support since > begining, so while fixing that, let's do it right way. Note that > this feature is completely optional. > >> If on the other hand Linux got support for sizing partitions in nand >> pages or eraseblocks and a patch changed the mtdparts layout to use >> the new units then either both u-boot and Linux support the units or >> parsing the partition fails. So it's safe and flexible and more >> general and probably even less work on u-boot's part. > > Okay, specifing size in therms of eraseblock would solve my problem > and I have to admit I'm not aware such a thing exists. Any pointer to > the patch introducing that change? > I am not aware of any. it's on the list of nice to have things I will probably not get to, Anyway, It's imho the place to put this code so that Linux does not depend on u-boot for pre-chewing it's partition table to get the partitions right. Thanks Michal
On Mon, Jun 06, 2016 at 08:50:55PM +0200, Michal Suchanek wrote: > On 6 June 2016 at 09:48, Ladislav Michl <ladis@linux-mips.org> wrote: [snip] > > Okay, specifing size in therms of eraseblock would solve my problem > > and I have to admit I'm not aware such a thing exists. Any pointer to > > the patch introducing that change? > > > > I am not aware of any. it's on the list of nice to have things I will > probably not get to, > > Anyway, It's imho the place to put this code so that Linux does not > depend on u-boot for pre-chewing it's partition table to get the > partitions right. Linux already depends on U-Boot as U-Boot feeds Linux with MTD partitions. So this patch does not introduce any change in this regard. And whenever are paritions altered by user, saved to U-Boot environment or default layout provided according actual eraseblock size is just not important here. In the end both U-Boot and Linux are working with the same partition layout. This is the only thing that matters and this patch does not change that. It changes only this: instead of hardcoding default layout into string provided at compile time, there is now posibility to create that string runtime. That's all. And now... Your recomended solution is to change both U-Boot and Linux to understand partition layout based on indexes and sizes expressed in eraseblocks. While size of SPL could be nicely evaluated in terms of eraseblocks, it is a bit more complicated with kernel partition, because 10 eraseblocks says nothing about partition size, therefore you cannot say whenever kernel will fit that space without knowledge of eraseblock size. So you just reverted the problem and both U-Boot and Linux would end with more code to be maintained for compatibility. Best regards, ladis
On 6 June 2016 at 23:21, Ladislav Michl <ladis@linux-mips.org> wrote: > On Mon, Jun 06, 2016 at 08:50:55PM +0200, Michal Suchanek wrote: >> On 6 June 2016 at 09:48, Ladislav Michl <ladis@linux-mips.org> wrote: > [snip] >> > Okay, specifing size in therms of eraseblock would solve my problem >> > and I have to admit I'm not aware such a thing exists. Any pointer to >> > the patch introducing that change? >> > >> >> I am not aware of any. it's on the list of nice to have things I will >> probably not get to, >> >> Anyway, It's imho the place to put this code so that Linux does not >> depend on u-boot for pre-chewing it's partition table to get the >> partitions right. > > Linux already depends on U-Boot as U-Boot feeds Linux with MTD partitions. No. It depends on getting the partition layout on commandline or in DT. This can be something U-Boot generates, or something it passes on from its environment or something it does not touch at all. > So this patch does not introduce any change in this regard. And whenever > are paritions altered by user, saved to U-Boot environment or default > layout provided according actual eraseblock size is just not important > here. In the end both U-Boot and Linux are working with the same partition > layout. This is the only thing that matters and this patch does not change But for the layout to be correct you must build current u-boot with this patch and not use compiled-in command line, devicetree, older u-boot or different bootloader. > that. It changes only this: instead of hardcoding default layout into string > provided at compile time, there is now posibility to create that string > runtime. That's all. The only problem with that is that the code for generating the string is in u-boot and the string is needed in Linux so the logical thing is to move the code to Linux where it is needed. > > And now... Your recomended solution is to change both U-Boot and Linux > to understand partition layout based on indexes and sizes expressed > in eraseblocks. While size of SPL could be nicely evaluated in terms > of eraseblocks, it is a bit more complicated with kernel partition, > because 10 eraseblocks says nothing about partition size, therefore > you cannot say whenever kernel will fit that space without knowledge > of eraseblock size. That's why you need the possibility to start a partition at the end of preceding partition. That way you can have sizes of different partitions in different units. It's interesting that your firmware uses erase block size for the boot partition. The sunxi firmware loads the bootloader from fixed number of pages. > So you just reverted the problem and both U-Boot > and Linux would end with more code to be maintained for compatibility. Yes, if you want U-Boot and Linux to talk the same units you need both implementing the code to handle them. Still U-Boot will only need to pass around a string with partition layout and parse it in the cases it is accessing the partitions. Thanks Michal
On Tue, Jun 07, 2016 at 09:43:20AM +0200, Michal Suchanek wrote: > On 6 June 2016 at 23:21, Ladislav Michl <ladis@linux-mips.org> wrote: > > On Mon, Jun 06, 2016 at 08:50:55PM +0200, Michal Suchanek wrote: > > Linux already depends on U-Boot as U-Boot feeds Linux with MTD partitions. > > No. It depends on getting the partition layout on commandline or in > DT. This can be something U-Boot generates, or something it passes on > from its environment or something it does not touch at all. I really fail to understand how your wording conflicts with what I wrote above. Both cmdline and DT are provided by bootloader, so Linux depends on that. Unlike HDD for example where its layout is stored on media in the partition table. > > So this patch does not introduce any change in this regard. And whenever > > are paritions altered by user, saved to U-Boot environment or default > > layout provided according actual eraseblock size is just not important > > here. In the end both U-Boot and Linux are working with the same partition > > layout. This is the only thing that matters and this patch does not change > > But for the layout to be correct you must build current u-boot with > this patch and not use compiled-in command line, devicetree, older > u-boot or different bootloader. No, layout is correct as it is for all boards I have there. But I was told boards with different NAND chips were produced. Also I have to change paritioning anyway, as original solution does not work in long term and devices are failing in field. I'm not proposing to break any existing setup. Your solution does the contrary as it requires both recent U-Boot and recent kernel. Also, that function is only helper one. If you decide to use static defaults, I'm fine with that. But with above patch, every user of that board will start with sane and optimal defaults. This is the sole point. Also I'm pretty sure previous patch which enables mtdparts to use defaults implicitely will not break anything, otherwise someone would notice bugs present in mtdparts code. (Perhaps it's time to read UBI SPL patch serie and related discusion for a reason I started to work on that) > > that. It changes only this: instead of hardcoding default layout into string > > provided at compile time, there is now posibility to create that string > > runtime. That's all. > > The only problem with that is that the code for generating the string > is in u-boot and the string is needed in Linux so the logical thing is > to move the code to Linux where it is needed. No. The string is needed in both U-Boot _and_ Linux. I see no reason to change kernel as it is not byuig anything. > > And now... Your recomended solution is to change both U-Boot and Linux > > to understand partition layout based on indexes and sizes expressed > > in eraseblocks. While size of SPL could be nicely evaluated in terms > > of eraseblocks, it is a bit more complicated with kernel partition, > > because 10 eraseblocks says nothing about partition size, therefore > > you cannot say whenever kernel will fit that space without knowledge > > of eraseblock size. > > That's why you need the possibility to start a partition at the end of > preceding partition. That way you can have sizes of different > partitions in different units. That posibility is already there for ages. > It's interesting that your firmware uses erase block size for the boot > partition. The sunxi firmware loads the bootloader from fixed number > of pages. DM3730 tries to load from first eraseblock, if that fails, then second and finaly gives up at fourth (including). > > So you just reverted the problem and both U-Boot > > and Linux would end with more code to be maintained for compatibility. > > Yes, if you want U-Boot and Linux to talk the same units you need both > implementing the code to handle them. Still U-Boot will only need to > pass around a string with partition layout and parse it in the cases > it is accessing the partitions. And since U-Boot environment is usually stored in partition(s)... Also note that this change will break non-Linux systems and older Linux systems. And as you said, I'm doing that to save few megabytes on media, so now it seems to be too high price even to me. Best regards, ladis
diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c index 53074a1..71c7acb 100644 --- a/cmd/mtdparts.c +++ b/cmd/mtdparts.c @@ -109,17 +109,20 @@ DECLARE_GLOBAL_DATA_PTR; #define MTD_WRITEABLE_CMD 1 /* default values for mtdids and mtdparts variables */ -#if defined(MTDIDS_DEFAULT) -static const char *const mtdids_default = MTDIDS_DEFAULT; -#else -static const char *const mtdids_default = NULL; +#if !defined(MTDIDS_DEFAULT) +#define MTDIDS_DEFAULT NULL #endif - -#if defined(MTDPARTS_DEFAULT) -static const char *const mtdparts_default = MTDPARTS_DEFAULT; +#if !defined(MTDPARTS_DEFAULT) +#define MTDPARTS_DEFAULT NULL +#endif +#if defined(CONFIG_SYS_MTDPARTS_DEFAULT_RUNTIME) +#define MTD_DEFAULTS_READONLY +extern void board_mtdparts_default(const char **mtdids, const char **mtdparts); #else -static const char *const mtdparts_default = NULL; +#define MTD_DEFAULTS_READONLY const #endif +static const char *MTD_DEFAULTS_READONLY mtdids_default = MTDIDS_DEFAULT; +static const char *MTD_DEFAULTS_READONLY mtdparts_default = MTDPARTS_DEFAULT; /* copies of last seen 'mtdids', 'mtdparts' and 'partition' env variables */ #define MTDIDS_MAXLEN 128 @@ -1725,6 +1728,9 @@ int mtdparts_init(void) memset(last_ids, 0, MTDIDS_MAXLEN); memset(last_parts, 0, MTDPARTS_MAXLEN); memset(last_partition, 0, PARTITION_MAXLEN); +#if defined(CONFIG_SYS_MTDPARTS_DEFAULT_RUNTIME) + board_mtdparts_default(&mtdids_default, &mtdparts_default); +#endif use_defaults = 1; initialized = 1; }
Some CPUs contains boot ROM code capable reading first few blocks (where SPL resides) of NAND flash and executing it. It is wise to create separate partition here for SPL. As block size depends on NAND chip used, we could either use worst case (biggest) partition size or base its size on actual block size. This patch adds support for the latter option. Signed-off-by: Ladislav Michl <ladis@linux-mips.org> --- cmd/mtdparts.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)