| Message ID | 20170304124730.6008-1-vigneshr@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On Sat, Mar 04, 2017 at 06:17:30PM +0530, Vignesh R wrote: > SPI U-Boot image for K2 boards have now exceeded 512K partition > allocated to it and no longer fit the partitions defined in kernel DTS > file. Therefore, pass an updated MTD partition table from U-Boot as > kernel command line arguments to avoid kernel from accidentally > modifying boot loader image that has overflowed to next user partition. So pretty much everyone is now hitting the problem of "U-Boot" gets huge because we're including N DTB files in the binary in order to pick the correct one. We really need to start figuring out a real solution here that perhaps involves saying where a copy of the tree lives on hardware, and if not found falling back to a generic functional-enough tree. > To do is, introduce a common environment file for declaring SPI > partition so that each individual boards need not repeat the same. > Choose appropriate SPI bus from board config file and pass it as command > line argument to kernel. > > Signed-off-by: Vignesh R <vigneshr@ti.com> So, we want the mtdparts to be passed in via the device tree, not the command line directly. I'm also not super keen on anything that adds more to: > include/configs/k2e_evm.h | 5 +++++ > include/configs/k2g_evm.h | 3 +++ > include/configs/k2hk_evm.h | 4 ++++ > include/configs/k2l_evm.h | 4 ++++ > include/configs/ti_armv7_keystone2.h | 7 ++++++- Aside from #include <environment/ti/...h>. [snip] > diff --git a/include/environment/ti/spi.h b/include/environment/ti/spi.h > new file mode 100644 > index 000000000000..6fea9d61f071 > --- /dev/null > +++ b/include/environment/ti/spi.h > @@ -0,0 +1,15 @@ > +/* > + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com > + * > + * Environment variable definitions for SPI on TI boards. > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#ifndef __TI_SPI_H > +#define __TI_SPI_H > + > +#define SPI0_MTD_PARTS "spi0.0:1m(u-boot-spl)ro,-(misc);\0" > +#define SPI1_MTD_PARTS "spi1.0:1m(u-boot-spl)ro,-(misc);\0" OK, but if you look at all of the other SPI mtdparts on TI platforms this isn't enough and is very keystone specific. Also, curious, why aren't you making use of mtdids to give a more descriptive name here? Thanks!
Hi, On Saturday 04 March 2017 10:39 PM, Tom Rini wrote: > On Sat, Mar 04, 2017 at 06:17:30PM +0530, Vignesh R wrote: > >> SPI U-Boot image for K2 boards have now exceeded 512K partition >> allocated to it and no longer fit the partitions defined in kernel DTS >> file. Therefore, pass an updated MTD partition table from U-Boot as >> kernel command line arguments to avoid kernel from accidentally >> modifying boot loader image that has overflowed to next user partition. > > So pretty much everyone is now hitting the problem of "U-Boot" gets huge > because we're including N DTB files in the binary in order to pick the > correct one. We really need to start figuring out a real solution here > that perhaps involves saying where a copy of the tree lives on hardware, > and if not found falling back to a generic functional-enough tree. > Hmm.. The DTBs need to be stored on boot media(spi flash here) anyways and that will lead to altering SPI paritions >> To do is, introduce a common environment file for declaring SPI >> partition so that each individual boards need not repeat the same. >> Choose appropriate SPI bus from board config file and pass it as command >> line argument to kernel. >> >> Signed-off-by: Vignesh R <vigneshr@ti.com> > > So, we want the mtdparts to be passed in via the device tree, not the > command line directly. Passing mtdparts via DT creates DT backward compatibility issues and does not scale well if partitions size/layout needs to altered in future like this case. OTOH, cmdline args provide flexibility of not being static and is mostly transparent (as partitions appear in cmdline). Few recent discussions on lkml also point to use of non static way of passing mtd partition table with cmdline as one of the options: https://patchwork.kernel.org/patch/9499119/ https://patchwork.kernel.org/patch/9515551/ > I'm also not super keen on anything that adds more to: > >> include/configs/k2e_evm.h | 5 +++++ >> include/configs/k2g_evm.h | 3 +++ >> include/configs/k2hk_evm.h | 4 ++++ >> include/configs/k2l_evm.h | 4 ++++ >> include/configs/ti_armv7_keystone2.h | 7 ++++++- > The additions are mostly choosing the SPI instance to which nor flash is connected which differs wrt k2g(SPI1) and other k2 boards(SPI0). > Aside from #include <environment/ti/...h>. > > [snip] >> diff --git a/include/environment/ti/spi.h b/include/environment/ti/spi.h >> new file mode 100644 >> index 000000000000..6fea9d61f071 >> --- /dev/null >> +++ b/include/environment/ti/spi.h >> @@ -0,0 +1,15 @@ >> +/* >> + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com >> + * >> + * Environment variable definitions for SPI on TI boards. >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#ifndef __TI_SPI_H >> +#define __TI_SPI_H >> + >> +#define SPI0_MTD_PARTS "spi0.0:1m(u-boot-spl)ro,-(misc);\0" >> +#define SPI1_MTD_PARTS "spi1.0:1m(u-boot-spl)ro,-(misc);\0" > > OK, but if you look at all of the other SPI mtdparts on TI platforms > this isn't enough and is very keystone specific. Well, its mostly size specific. OMAP devices have bigger QSPI flashes a K2* devices have just 16MB flash and hence, this layout. I will rename it as KEYSTONE_SPIx_MTD_PART. But adding it here will enable partition table to be used across k2 board config files. > Also, curious, why aren't you making use of mtdids to give a more descriptive name here? AFAIK, mtdids are supported for nand flash, parallel nor and dataflash and not for SPI NOR devices supported under drivers/mtd/spi/spi_flash.c. They are not modelled as MTD devices AFAIU
On Mon, Mar 06, 2017 at 03:16:57PM +0530, Vignesh R wrote: > Hi, > > On Saturday 04 March 2017 10:39 PM, Tom Rini wrote: > > On Sat, Mar 04, 2017 at 06:17:30PM +0530, Vignesh R wrote: > > > >> SPI U-Boot image for K2 boards have now exceeded 512K partition > >> allocated to it and no longer fit the partitions defined in kernel DTS > >> file. Therefore, pass an updated MTD partition table from U-Boot as > >> kernel command line arguments to avoid kernel from accidentally > >> modifying boot loader image that has overflowed to next user partition. > > > > So pretty much everyone is now hitting the problem of "U-Boot" gets huge > > because we're including N DTB files in the binary in order to pick the > > correct one. We really need to start figuring out a real solution here > > that perhaps involves saying where a copy of the tree lives on hardware, > > and if not found falling back to a generic functional-enough tree. > > Hmm.. The DTBs need to be stored on boot media(spi flash here) anyways > and that will lead to altering SPI paritions Yes. This, FWIW, is exactly the use case at least some people describe as the point where the hardware is capable of shipping the device tree. > >> To do is, introduce a common environment file for declaring SPI > >> partition so that each individual boards need not repeat the same. > >> Choose appropriate SPI bus from board config file and pass it as command > >> line argument to kernel. > >> > >> Signed-off-by: Vignesh R <vigneshr@ti.com> > > > > So, we want the mtdparts to be passed in via the device tree, not the > > command line directly. To be clear, I was saying that fdt_fixup_mtdparts() should be used (which sadly isn't a common hook today, but that also gets to the next big problem below) to pass in whatever the partition layout is. I'm not a huge fan of dumping tons of data into the cmdline, but that's because I swear I've seen cases where that ends up being truncated. > Passing mtdparts via DT creates DT backward compatibility issues and > does not scale well if partitions size/layout needs to altered in future > like this case. OTOH, cmdline args provide flexibility of not being > static and is mostly transparent (as partitions appear in cmdline). Few > recent discussions on lkml also point to use of non static way of > passing mtd partition table with cmdline as one of the options: > https://patchwork.kernel.org/patch/9499119/ > https://patchwork.kernel.org/patch/9515551/ Adding in Tony since it seems like there's some confusion here perhaps. If you change the partition table of flash on a given device, you've changed the partition table on a given device, and users that had been using that previous table are SOL. Given that we (generally) do not have an MBR or GPT or anything on the front of flash, it doesn't matter if we have a "flag day" type change at the device tree file itself or now the environment has changed and we're passing in a different set of arguments somewhere. So if we're changing the partitioning of flash frequently, it doesn't matter where we're putting the definition of it, the problem is that we're doing it as it means we haven't put enough thought into the design to start with. Thanks! [snipped out the rest as yes, OK, thanks]
On Wednesday 08 March 2017 08:11 PM, Tom Rini wrote: > On Mon, Mar 06, 2017 at 03:16:57PM +0530, Vignesh R wrote: >> Hi, >> >> On Saturday 04 March 2017 10:39 PM, Tom Rini wrote: >>> On Sat, Mar 04, 2017 at 06:17:30PM +0530, Vignesh R wrote: >>> >>>> SPI U-Boot image for K2 boards have now exceeded 512K partition >>>> allocated to it and no longer fit the partitions defined in kernel DTS >>>> file. Therefore, pass an updated MTD partition table from U-Boot as >>>> kernel command line arguments to avoid kernel from accidentally >>>> modifying boot loader image that has overflowed to next user partition. >>> >>> So pretty much everyone is now hitting the problem of "U-Boot" gets huge >>> because we're including N DTB files in the binary in order to pick the >>> correct one. We really need to start figuring out a real solution here >>> that perhaps involves saying where a copy of the tree lives on hardware, >>> and if not found falling back to a generic functional-enough tree. >> >> Hmm.. The DTBs need to be stored on boot media(spi flash here) anyways >> and that will lead to altering SPI paritions > > Yes. This, FWIW, is exactly the use case at least some people describe > as the point where the hardware is capable of shipping the device tree. > >>>> To do is, introduce a common environment file for declaring SPI >>>> partition so that each individual boards need not repeat the same. >>>> Choose appropriate SPI bus from board config file and pass it as command >>>> line argument to kernel. >>>> >>>> Signed-off-by: Vignesh R <vigneshr@ti.com> >>> >>> So, we want the mtdparts to be passed in via the device tree, not the >>> command line directly. > > To be clear, I was saying that fdt_fixup_mtdparts() should be used > (which sadly isn't a common hook today, but that also gets to the next > big problem below) to pass in whatever the partition layout is. I'm not > a huge fan of dumping tons of data into the cmdline, but that's because > I swear I've seen cases where that ends up being truncated. > Fixing up partition table is not transparent to user IMO. Only user aware of U-Boot magic fdt_fixup_mtdparts() can figure out how partitions are being re organized. Also, currently mtdparts and fdt_fixup_mtdparts() don't seem to work too well for SPI NOR devices. SPI flash has to be probed (using "sf probe" at U-Boot prompt) at least once for it to be registered as MTD device so that, mtdparts starts recognizing NOR partitions (and thereby fixing mtd partition before jumping to kernel). If fdt_fixup_mtdparts() and mtdids are to be used without "sf probe" from u-boot, then a patch to make all spi flash devices automatically probe from board file needs to be added (similar to initr_nand()). >> Passing mtdparts via DT creates DT backward compatibility issues and >> does not scale well if partitions size/layout needs to altered in future >> like this case. OTOH, cmdline args provide flexibility of not being >> static and is mostly transparent (as partitions appear in cmdline). Few >> recent discussions on lkml also point to use of non static way of >> passing mtd partition table with cmdline as one of the options: >> https://patchwork.kernel.org/patch/9499119/ >> https://patchwork.kernel.org/patch/9515551/ > > Adding in Tony since it seems like there's some confusion here perhaps. > If you change the partition table of flash on a given device, you've > changed the partition table on a given device, and users that had been > using that previous table are SOL. Given that we (generally) do not > have an MBR or GPT or anything on the front of flash, it doesn't matter > if we have a "flag day" type change at the device tree file itself or > now the environment has changed and we're passing in a different set of > arguments somewhere. So if we're changing the partitioning of flash > frequently, it doesn't matter where we're putting the definition of it, > the problem is that we're doing it as it means we haven't put enough > thought into the design to start with. Thanks! > Yes, I agree that initial DT layout of 512K may not have been good design, but it would be good to have an agreeable way of fixing up MTD partitions when there is overflow. So, is fdt_fixup_mtdparts() preferred approach?
On Thu, Mar 09, 2017 at 03:28:02PM +0530, Vignesh R wrote: > > > On Wednesday 08 March 2017 08:11 PM, Tom Rini wrote: > > On Mon, Mar 06, 2017 at 03:16:57PM +0530, Vignesh R wrote: > >> Hi, > >> > >> On Saturday 04 March 2017 10:39 PM, Tom Rini wrote: > >>> On Sat, Mar 04, 2017 at 06:17:30PM +0530, Vignesh R wrote: > >>> > >>>> SPI U-Boot image for K2 boards have now exceeded 512K partition > >>>> allocated to it and no longer fit the partitions defined in kernel DTS > >>>> file. Therefore, pass an updated MTD partition table from U-Boot as > >>>> kernel command line arguments to avoid kernel from accidentally > >>>> modifying boot loader image that has overflowed to next user partition. > >>> > >>> So pretty much everyone is now hitting the problem of "U-Boot" gets huge > >>> because we're including N DTB files in the binary in order to pick the > >>> correct one. We really need to start figuring out a real solution here > >>> that perhaps involves saying where a copy of the tree lives on hardware, > >>> and if not found falling back to a generic functional-enough tree. > >> > >> Hmm.. The DTBs need to be stored on boot media(spi flash here) anyways > >> and that will lead to altering SPI paritions > > > > Yes. This, FWIW, is exactly the use case at least some people describe > > as the point where the hardware is capable of shipping the device tree. > > > >>>> To do is, introduce a common environment file for declaring SPI > >>>> partition so that each individual boards need not repeat the same. > >>>> Choose appropriate SPI bus from board config file and pass it as command > >>>> line argument to kernel. > >>>> > >>>> Signed-off-by: Vignesh R <vigneshr@ti.com> > >>> > >>> So, we want the mtdparts to be passed in via the device tree, not the > >>> command line directly. > > > > To be clear, I was saying that fdt_fixup_mtdparts() should be used > > (which sadly isn't a common hook today, but that also gets to the next > > big problem below) to pass in whatever the partition layout is. I'm not > > a huge fan of dumping tons of data into the cmdline, but that's because > > I swear I've seen cases where that ends up being truncated. > > > > Fixing up partition table is not transparent to user IMO. Only user > aware of U-Boot magic fdt_fixup_mtdparts() can figure out how partitions > are being re organized. > > Also, currently mtdparts and fdt_fixup_mtdparts() don't seem to work too > well for SPI NOR devices. SPI flash has to be probed (using "sf probe" > at U-Boot prompt) at least once for it to be registered as MTD device so > that, mtdparts starts recognizing NOR partitions (and thereby fixing mtd > partition before jumping to kernel). If fdt_fixup_mtdparts() and mtdids > are to be used without "sf probe" from u-boot, then a patch to make all > spi flash devices automatically probe from board file needs to be added > (similar to initr_nand()). > > >> Passing mtdparts via DT creates DT backward compatibility issues and > >> does not scale well if partitions size/layout needs to altered in future > >> like this case. OTOH, cmdline args provide flexibility of not being > >> static and is mostly transparent (as partitions appear in cmdline). Few > >> recent discussions on lkml also point to use of non static way of > >> passing mtd partition table with cmdline as one of the options: > >> https://patchwork.kernel.org/patch/9499119/ > >> https://patchwork.kernel.org/patch/9515551/ > > > > Adding in Tony since it seems like there's some confusion here perhaps. > > If you change the partition table of flash on a given device, you've > > changed the partition table on a given device, and users that had been > > using that previous table are SOL. Given that we (generally) do not > > have an MBR or GPT or anything on the front of flash, it doesn't matter > > if we have a "flag day" type change at the device tree file itself or > > now the environment has changed and we're passing in a different set of > > arguments somewhere. So if we're changing the partitioning of flash > > frequently, it doesn't matter where we're putting the definition of it, > > the problem is that we're doing it as it means we haven't put enough > > thought into the design to start with. Thanks! > > > > Yes, I agree that initial DT layout of 512K may not have been good > design, but it would be good to have an agreeable way of fixing up MTD > partitions when there is overflow. So, is fdt_fixup_mtdparts() preferred > approach? You make a good point about fdt_fixup_mtdparts() being non-trivial to have happen correctly in all cases above, so OK, lets put that aside. I'll also accept that previous best wisdom of not shoving tons of stuff into the cmdline, rather than passing it in the device tree, isn't correct anymore. But the big, un-tackled problem is that the old DT layout is failing because we're constantly increasing the number of full linux DTB files we're including in an image and thus increasing the size of our blob every time. We need to stop and think and maybe design things differently. Perhaps it's time for more platforms to have a spot on their storage where the DT is supposed to be, and we only use a fall back one that's included in U-Boot if it's not found? Franklin already posted a patch to have something kind-of similar be able to happen (which is to say, go from a generic DTB to the correct-for-the-HW one).
[...] On Friday 10 March 2017 11:32 PM, Tom Rini wrote: >> Yes, I agree that initial DT layout of 512K may not have been good >> design, but it would be good to have an agreeable way of fixing up MTD >> partitions when there is overflow. So, is fdt_fixup_mtdparts() preferred >> approach? > You make a good point about fdt_fixup_mtdparts() being non-trivial to > have happen correctly in all cases above, so OK, lets put that aside. > I'll also accept that previous best wisdom of not shoving tons of stuff > into the cmdline, rather than passing it in the device tree, isn't > correct anymore. > > But the big, un-tackled problem is that the old DT layout is failing > because we're constantly increasing the number of full linux DTB files > we're including in an image and thus increasing the size of our blob > every time. We need to stop and think and maybe design things > differently. Perhaps it's time for more platforms to have a spot on > their storage where the DT is supposed to be, and we only use a fall > back one that's included in U-Boot if it's not found? Franklin already > posted a patch to have something kind-of similar be able to happen > (which is to say, go from a generic DTB to the correct-for-the-HW one). I agree that DTB files are making u-boot image bulky. But it does not seem to be problem due to addition of DT alone. For example SPI boot image on K2 platform is two stage SPL+U-Boot combined into one single image u-boot-spi.gph which is about 555K. General boot image u-boot.bin is about 491K and u-boot-nodtb.bin is 432K. So even w/o dtbs SPI image may overflow and its because of new code/framework changes. There is similar issue with dra7xx where flash partition for SPL is running out due to addition of new code.
On Tue, Mar 14, 2017 at 05:11:06PM +0530, Vignesh R wrote: > [...] > > On Friday 10 March 2017 11:32 PM, Tom Rini wrote: > >> Yes, I agree that initial DT layout of 512K may not have been good > >> design, but it would be good to have an agreeable way of fixing up MTD > >> partitions when there is overflow. So, is fdt_fixup_mtdparts() preferred > >> approach? > > You make a good point about fdt_fixup_mtdparts() being non-trivial to > > have happen correctly in all cases above, so OK, lets put that aside. > > I'll also accept that previous best wisdom of not shoving tons of stuff > > into the cmdline, rather than passing it in the device tree, isn't > > correct anymore. > > > > But the big, un-tackled problem is that the old DT layout is failing > > because we're constantly increasing the number of full linux DTB files > > we're including in an image and thus increasing the size of our blob > > every time. We need to stop and think and maybe design things > > differently. Perhaps it's time for more platforms to have a spot on > > their storage where the DT is supposed to be, and we only use a fall > > back one that's included in U-Boot if it's not found? Franklin already > > posted a patch to have something kind-of similar be able to happen > > (which is to say, go from a generic DTB to the correct-for-the-HW one). > > I agree that DTB files are making u-boot image bulky. But it does not > seem to be problem due to addition of DT alone. For example SPI boot > image on K2 platform is two stage SPL+U-Boot combined into one single > image u-boot-spi.gph which is about 555K. General boot image u-boot.bin > is about 491K and u-boot-nodtb.bin is 432K. So even w/o dtbs SPI image > may overflow and its because of new code/framework changes. Which platform exactly? I don't see anything today that's quite that large. And can we not move towards the "normal" method of SPL loading the u-boot.img (or FIT) from? I guess the current architecture here is confusing me. Regardless, I still see the DT problem as the bigger one long term, and dra7xx shows that. And I agree we need to re-size how the flash is partitioned. > There is similar issue with dra7xx where flash partition for SPL is > running out due to addition of new code. The DRA flash partition is, and should be fine because we have the ROM-mandated limits and don't need to include U-Boot with the SPL image. The main U-Boot image however is growing and that is a DTB problem. The difference here between -nodtb and the .img (FIT) with all of the DTBs is over 300KiB. And that's mostly linear growth when compared with the single-DTB case.
On 3/16/2017 2:18 AM, Tom Rini wrote: > On Tue, Mar 14, 2017 at 05:11:06PM +0530, Vignesh R wrote: >> [...] >> >> On Friday 10 March 2017 11:32 PM, Tom Rini wrote: >>>> Yes, I agree that initial DT layout of 512K may not have been good >>>> design, but it would be good to have an agreeable way of fixing up MTD >>>> partitions when there is overflow. So, is fdt_fixup_mtdparts() preferred >>>> approach? >>> You make a good point about fdt_fixup_mtdparts() being non-trivial to >>> have happen correctly in all cases above, so OK, lets put that aside. >>> I'll also accept that previous best wisdom of not shoving tons of stuff >>> into the cmdline, rather than passing it in the device tree, isn't >>> correct anymore. >>> >>> But the big, un-tackled problem is that the old DT layout is failing >>> because we're constantly increasing the number of full linux DTB files >>> we're including in an image and thus increasing the size of our blob >>> every time. We need to stop and think and maybe design things >>> differently. Perhaps it's time for more platforms to have a spot on >>> their storage where the DT is supposed to be, and we only use a fall >>> back one that's included in U-Boot if it's not found? Franklin already >>> posted a patch to have something kind-of similar be able to happen >>> (which is to say, go from a generic DTB to the correct-for-the-HW one). >> I agree that DTB files are making u-boot image bulky. But it does not >> seem to be problem due to addition of DT alone. For example SPI boot >> image on K2 platform is two stage SPL+U-Boot combined into one single >> image u-boot-spi.gph which is about 555K. General boot image u-boot.bin >> is about 491K and u-boot-nodtb.bin is 432K. So even w/o dtbs SPI image >> may overflow and its because of new code/framework changes. > Which platform exactly? I don't see anything today that's quite that > large. Sorry, I had few debug prints enabled when I collected the stats. On vanila u-boot for k2hk here are the stats: u-boot-spi.gph 512K u-boot.bin 448K u-boot-nodtb.bin 420K Still at least for k2 platforms DTBs alone are not to be blamed for image size increase. > And can we not move towards the "normal" method of SPL loading > the u-boot.img (or FIT) from? I guess the current architecture here is > confusing me. This has been same for all k2 platforms. I guess we have single image so that user don't have to bother flashing multiple images for spi boot given the fact that all other boot modes have single image. > Regardless, I still see the DT problem as the bigger one long term, and > dra7xx shows that. And I agree we need to re-size how the flash is > partitioned. True. >> There is similar issue with dra7xx where flash partition for SPL is >> running out due to addition of new code. > The DRA flash partition is, and should be fine because we have the > ROM-mandated limits and don't need to include U-Boot with the SPL image. > The main U-Boot image however is growing and that is a DTB problem. The > difference here between -nodtb and the .img (FIT) with all of the DTBs > is over 300KiB. And that's mostly linear growth when compared with the > single-DTB case. I agree DTBs are adding to image size on DRA. But even SPL is growing and is bound to exceed its 64K limit[1]. [1] https://patchwork.kernel.org/patch/9515551/
On Sat, Mar 18, 2017 at 05:44:05PM +0530, Vignesh R wrote: > On 3/16/2017 2:18 AM, Tom Rini wrote: > > On Tue, Mar 14, 2017 at 05:11:06PM +0530, Vignesh R wrote: > >> [...] > >> > >> On Friday 10 March 2017 11:32 PM, Tom Rini wrote: > >>>> Yes, I agree that initial DT layout of 512K may not have been good > >>>> design, but it would be good to have an agreeable way of fixing up MTD > >>>> partitions when there is overflow. So, is fdt_fixup_mtdparts() preferred > >>>> approach? > >>> You make a good point about fdt_fixup_mtdparts() being non-trivial to > >>> have happen correctly in all cases above, so OK, lets put that aside. > >>> I'll also accept that previous best wisdom of not shoving tons of stuff > >>> into the cmdline, rather than passing it in the device tree, isn't > >>> correct anymore. > >>> > >>> But the big, un-tackled problem is that the old DT layout is failing > >>> because we're constantly increasing the number of full linux DTB files > >>> we're including in an image and thus increasing the size of our blob > >>> every time. We need to stop and think and maybe design things > >>> differently. Perhaps it's time for more platforms to have a spot on > >>> their storage where the DT is supposed to be, and we only use a fall > >>> back one that's included in U-Boot if it's not found? Franklin already > >>> posted a patch to have something kind-of similar be able to happen > >>> (which is to say, go from a generic DTB to the correct-for-the-HW one). > >> I agree that DTB files are making u-boot image bulky. But it does not > >> seem to be problem due to addition of DT alone. For example SPI boot > >> image on K2 platform is two stage SPL+U-Boot combined into one single > >> image u-boot-spi.gph which is about 555K. General boot image u-boot.bin > >> is about 491K and u-boot-nodtb.bin is 432K. So even w/o dtbs SPI image > >> may overflow and its because of new code/framework changes. > > Which platform exactly? I don't see anything today that's quite that > > large. > Sorry, I had few debug prints enabled when I collected the stats. On > vanila u-boot for k2hk here are the stats: > u-boot-spi.gph 512K > u-boot.bin 448K > u-boot-nodtb.bin 420K Yes, this is more in line with what I was seeing. > Still at least for k2 platforms DTBs alone are not to be blamed for > image size increase. Yes, features add size. And the desire to add more and more DTBs also increases the size. > > And can we not move towards the "normal" method of SPL loading > > the u-boot.img (or FIT) from? I guess the current architecture here is > > confusing me. > This has been same for all k2 platforms. I guess we have single image so > that user don't have to bother flashing multiple images for spi boot > given the fact that all other boot modes have single image. > > > Regardless, I still see the DT problem as the bigger one long term, and > > dra7xx shows that. And I agree we need to re-size how the flash is > > partitioned. > True. The next question is, given that Franklin is talking about being able to load the right DTB for any K2 platform basically, is the layout in https://patchwork.ozlabs.org/patch/736498/ really looking like it will be enough? > >> There is similar issue with dra7xx where flash partition for SPL is > >> running out due to addition of new code. > > The DRA flash partition is, and should be fine because we have the > > ROM-mandated limits and don't need to include U-Boot with the SPL image. > > The main U-Boot image however is growing and that is a DTB problem. The > > difference here between -nodtb and the .img (FIT) with all of the DTBs > > is over 300KiB. And that's mostly linear growth when compared with the > > single-DTB case. > I agree DTBs are adding to image size on DRA. But even SPL is growing > and is bound to exceed its 64K limit[1]. > > [1] https://patchwork.kernel.org/patch/9515551/ Yes, setting the initial size limit to 64KiB was a bad initial choice especially since we want to have more and more features in SPL too.
On Saturday 18 March 2017 08:04 PM, Tom Rini wrote: >>> And can we not move towards the "normal" method of SPL loading >>> the u-boot.img (or FIT) from? I guess the current architecture here is >>> confusing me. >> This has been same for all k2 platforms. I guess we have single image so >> that user don't have to bother flashing multiple images for spi boot >> given the fact that all other boot modes have single image. >> >>> Regardless, I still see the DT problem as the bigger one long term, and >>> dra7xx shows that. And I agree we need to re-size how the flash is >>> partitioned. >> True. > The next question is, given that Franklin is talking about being able to > load the right DTB for any K2 platform basically, is the layout in > https://patchwork.ozlabs.org/patch/736498/ really looking like it will > be enough? > I will leave Franklin to comment here. I dont think he has plans to do changes for all K2 platforms (I guess his plans are mostly limited to K2G)
On 03/20/2017 11:57 PM, Vignesh R wrote: > > > On Saturday 18 March 2017 08:04 PM, Tom Rini wrote: >>>> And can we not move towards the "normal" method of SPL loading >>>> the u-boot.img (or FIT) from? I guess the current architecture here is >>>> confusing me. >>> This has been same for all k2 platforms. I guess we have single image so >>> that user don't have to bother flashing multiple images for spi boot >>> given the fact that all other boot modes have single image. >>> >>>> Regardless, I still see the DT problem as the bigger one long term, and >>>> dra7xx shows that. And I agree we need to re-size how the flash is >>>> partitioned. >>> True. >> The next question is, given that Franklin is talking about being able to >> load the right DTB for any K2 platform basically, is the layout in >> https://patchwork.ozlabs.org/patch/736498/ really looking like it will >> be enough? >> > > I will leave Franklin to comment here. I dont think he has plans to do > changes for all K2 platforms (I guess his plans are mostly limited to K2G) I'm not sure if there is any real solution other than providing a generous amount of storage for U-boot in the flash memory. I don't think atleast within our TI SDK use cases we even use the misc partition. So I don't see a reason why we couldn't give U-boot's partition 3 or 4 MB of space. Also has there been any thoughts of compressing dtbs? These dtbs are relatively massive and compressed they are around 1/5 the size. Personally I'm not a fan of U-boot performing all these fix ups before passing things to the kernel. It forces so much coupling between bootloader versions and kernel. And things become more painful when changes in the kernel causes U-boot fix ups to break. >
On Tue, Mar 21, 2017 at 01:52:07PM -0500, Franklin S Cooper Jr wrote: > > > On 03/20/2017 11:57 PM, Vignesh R wrote: > > > > > > On Saturday 18 March 2017 08:04 PM, Tom Rini wrote: > >>>> And can we not move towards the "normal" method of SPL loading > >>>> the u-boot.img (or FIT) from? I guess the current architecture here is > >>>> confusing me. > >>> This has been same for all k2 platforms. I guess we have single image so > >>> that user don't have to bother flashing multiple images for spi boot > >>> given the fact that all other boot modes have single image. > >>> > >>>> Regardless, I still see the DT problem as the bigger one long term, and > >>>> dra7xx shows that. And I agree we need to re-size how the flash is > >>>> partitioned. > >>> True. > >> The next question is, given that Franklin is talking about being able to > >> load the right DTB for any K2 platform basically, is the layout in > >> https://patchwork.ozlabs.org/patch/736498/ really looking like it will > >> be enough? > >> > > > > I will leave Franklin to comment here. I dont think he has plans to do > > changes for all K2 platforms (I guess his plans are mostly limited to K2G) > > I'm not sure if there is any real solution other than providing a > generous amount of storage for U-boot in the flash memory. I don't think > atleast within our TI SDK use cases we even use the misc partition. So I > don't see a reason why we couldn't give U-boot's partition 3 or 4 MB of > space. OK. But what about a dedicated place where the right (base) board DTB could reside? > Also has there been any thoughts of compressing dtbs? These dtbs are > relatively massive and compressed they are around 1/5 the size. There has not yet been. My first thought is about decompression time, but maybe that won't matter. > > Personally I'm not a fan of U-boot performing all these fix ups before > passing things to the kernel. It forces so much coupling between > bootloader versions and kernel. And things become more painful when > changes in the kernel causes U-boot fix ups to break. To the final point, I cannot find the video from the device tree BoF at ELC, but as we talked about there, to some degree overlays/fixups will have to be done in Linux. But by the same token, a lot of overlay applications will be done prior to the kernel as well. When in doubt, this shouldn't be done in C, but in user replaceable/updatable scripts.
On 03/21/2017 07:25 PM, Tom Rini wrote: > On Tue, Mar 21, 2017 at 01:52:07PM -0500, Franklin S Cooper Jr wrote: >> >> >> On 03/20/2017 11:57 PM, Vignesh R wrote: >>> >>> >>> On Saturday 18 March 2017 08:04 PM, Tom Rini wrote: >>>>>> And can we not move towards the "normal" method of SPL loading >>>>>> the u-boot.img (or FIT) from? I guess the current architecture here is >>>>>> confusing me. >>>>> This has been same for all k2 platforms. I guess we have single image so >>>>> that user don't have to bother flashing multiple images for spi boot >>>>> given the fact that all other boot modes have single image. >>>>> >>>>>> Regardless, I still see the DT problem as the bigger one long term, and >>>>>> dra7xx shows that. And I agree we need to re-size how the flash is >>>>>> partitioned. >>>>> True. >>>> The next question is, given that Franklin is talking about being able to >>>> load the right DTB for any K2 platform basically, is the layout in >>>> https://patchwork.ozlabs.org/patch/736498/ really looking like it will >>>> be enough? >>>> >>> >>> I will leave Franklin to comment here. I dont think he has plans to do >>> changes for all K2 platforms (I guess his plans are mostly limited to K2G) >> >> I'm not sure if there is any real solution other than providing a >> generous amount of storage for U-boot in the flash memory. I don't think >> atleast within our TI SDK use cases we even use the misc partition. So I >> don't see a reason why we couldn't give U-boot's partition 3 or 4 MB of >> space. > > OK. But what about a dedicated place where the right (base) board DTB > could reside? If the concern is space atleast for TI evms I don't see the difference between carving out separate space just for dtbs vs having 1 large space for both U-boot and dtbs. Without the right dtb U-boot may not properly boot or have enough to boot a kernel. So if dtbs out grow their space we will need to increase it anyway. > >> Also has there been any thoughts of compressing dtbs? These dtbs are >> relatively massive and compressed they are around 1/5 the size. > > There has not yet been. My first thought is about decompression time, > but maybe that won't matter. So my thought would be that we will compress the dtbs individually and not the entire FIT image. This way U-boot can then read the FIT header uncompressed and then select and uncompress only the dtb it needs. > >> >> Personally I'm not a fan of U-boot performing all these fix ups before >> passing things to the kernel. It forces so much coupling between >> bootloader versions and kernel. And things become more painful when >> changes in the kernel causes U-boot fix ups to break. > > To the final point, I cannot find the video from the device tree BoF at > ELC, but as we talked about there, to some degree overlays/fixups will > have to be done in Linux. But by the same token, a lot of overlay > applications will be done prior to the kernel as well. When in doubt, > this shouldn't be done in C, but in user replaceable/updatable scripts. > I think prebuilt overlays that are loaded and applied in U-boot is the right way to go. Atleast then its not entwined within source code and is edited in a very human readable manner.
diff --git a/include/configs/k2e_evm.h b/include/configs/k2e_evm.h index 777f22540afb..3abe5c3fc821 100644 --- a/include/configs/k2e_evm.h +++ b/include/configs/k2e_evm.h @@ -10,6 +10,8 @@ #ifndef __CONFIG_K2E_EVM_H #define __CONFIG_K2E_EVM_H +#include <environment/ti/spi.h> + /* Platform type */ #define CONFIG_SOC_K2E @@ -30,6 +32,9 @@ /* SPL SPI Loader Configuration */ #define CONFIG_SPL_TEXT_BASE 0x0c100000 + +#define SPI_MTD_PARTS SPI0_MTD_PARTS + /* NAND Configuration */ #define CONFIG_SYS_NAND_PAGE_2K diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h index bd252312a20b..049c4dd6ec4b 100644 --- a/include/configs/k2g_evm.h +++ b/include/configs/k2g_evm.h @@ -10,6 +10,8 @@ #ifndef __CONFIG_K2G_EVM_H #define __CONFIG_K2G_EVM_H +#include <environment/ti/spi.h> + /* Platform type */ #define CONFIG_SOC_K2G @@ -76,4 +78,5 @@ #define CONFIG_BOUNCE_BUFFER #endif +#define SPI_MTD_PARTS SPI1_MTD_PARTS #endif /* __CONFIG_K2G_EVM_H */ diff --git a/include/configs/k2hk_evm.h b/include/configs/k2hk_evm.h index 4adb119b3066..d6e6171a6731 100644 --- a/include/configs/k2hk_evm.h +++ b/include/configs/k2hk_evm.h @@ -10,6 +10,8 @@ #ifndef __CONFIG_K2HK_EVM_H #define __CONFIG_K2HK_EVM_H +#include <environment/ti/spi.h> + /* Platform type */ #define CONFIG_SOC_K2HK @@ -30,6 +32,8 @@ /* SPL SPI Loader Configuration */ #define CONFIG_SPL_TEXT_BASE 0x0c200000 +#define SPI_MTD_PARTS SPI0_MTD_PARTS + /* NAND Configuration */ #define CONFIG_SYS_NAND_PAGE_2K diff --git a/include/configs/k2l_evm.h b/include/configs/k2l_evm.h index 9bdd56570be7..aaaff381665c 100644 --- a/include/configs/k2l_evm.h +++ b/include/configs/k2l_evm.h @@ -10,6 +10,8 @@ #ifndef __CONFIG_K2L_EVM_H #define __CONFIG_K2L_EVM_H +#include <environment/ti/spi.h> + /* Platform type */ #define CONFIG_SOC_K2L @@ -30,6 +32,8 @@ /* SPL SPI Loader Configuration */ #define CONFIG_SPL_TEXT_BASE 0x0c100000 +#define SPI_MTD_PARTS SPI0_MTD_PARTS + /* NAND Configuration */ #define CONFIG_SYS_NAND_PAGE_4K diff --git a/include/configs/ti_armv7_keystone2.h b/include/configs/ti_armv7_keystone2.h index 5d4ef58e5f1a..c48d7ba17a2d 100644 --- a/include/configs/ti_armv7_keystone2.h +++ b/include/configs/ti_armv7_keystone2.h @@ -213,6 +213,10 @@ /* EDMA3 */ #define CONFIG_TI_EDMA3 +#define KERNEL_MTD_PARTS \ + "mtdparts=" \ + SPI_MTD_PARTS + #define DEFAULT_FW_INITRAMFS_BOOT_ENV \ "name_fw_rd=k2-fw-initrd.cpio.gz\0" \ "set_rd_spec=setenv rd_spec ${rdaddr}:${filesize}\0" \ @@ -269,7 +273,8 @@ "sf write ${loadaddr} 0 ${filesize}\0" \ "burn_uboot_nand=nand erase 0 0x100000; " \ "nand write ${loadaddr} 0 ${filesize}\0" \ - "args_all=setenv bootargs console=ttyS0,115200n8 rootwait=1\0" \ + "args_all=setenv bootargs console=ttyS0,115200n8 rootwait=1 " \ + KERNEL_MTD_PARTS \ "args_net=setenv bootargs ${bootargs} rootfstype=nfs " \ "root=/dev/nfs rw nfsroot=${serverip}:${nfs_root}," \ "${nfs_options} ip=dhcp\0" \ diff --git a/include/environment/ti/spi.h b/include/environment/ti/spi.h new file mode 100644 index 000000000000..6fea9d61f071 --- /dev/null +++ b/include/environment/ti/spi.h @@ -0,0 +1,15 @@ +/* + * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com + * + * Environment variable definitions for SPI on TI boards. + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef __TI_SPI_H +#define __TI_SPI_H + +#define SPI0_MTD_PARTS "spi0.0:1m(u-boot-spl)ro,-(misc);\0" +#define SPI1_MTD_PARTS "spi1.0:1m(u-boot-spl)ro,-(misc);\0" + +#endif
SPI U-Boot image for K2 boards have now exceeded 512K partition allocated to it and no longer fit the partitions defined in kernel DTS file. Therefore, pass an updated MTD partition table from U-Boot as kernel command line arguments to avoid kernel from accidentally modifying boot loader image that has overflowed to next user partition. To do is, introduce a common environment file for declaring SPI partition so that each individual boards need not repeat the same. Choose appropriate SPI bus from board config file and pass it as command line argument to kernel. Signed-off-by: Vignesh R <vigneshr@ti.com> --- include/configs/k2e_evm.h | 5 +++++ include/configs/k2g_evm.h | 3 +++ include/configs/k2hk_evm.h | 4 ++++ include/configs/k2l_evm.h | 4 ++++ include/configs/ti_armv7_keystone2.h | 7 ++++++- include/environment/ti/spi.h | 15 +++++++++++++++ 6 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 include/environment/ti/spi.h