ARM: keystone: Pass SPI MTD partition table via kernel command line

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 <[email protected]>

 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 new file +3 -0

new file mode 100644
index 000000000000..6fea9d61f071
@@ -0,0 +1,15 @@
+/*
+ * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com
+ *

Quoted 5 lines · Vignesh R

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.

Quoted 6 lines

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 <[email protected]>

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:

Quoted 5 lines

 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]

Quoted 14 lines

include/environment/ti/spi.h new file +8 -0

new file mode 100644
index 000000000000..6fea9d61f071
@@ -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+
+ */
+

include/environment/ti/spi.h line 13

Quoted 5 lines

include/environment/ti/spi.h new file +5 -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?

Hi,

Quoted 8 lines · Tom Rini

Quoted 5 lines · Vignesh R

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.

Quoted 5 lines

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

Quoted 9 lines

Quoted 6 lines

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 <[email protected]>

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/

Quoted 11 lines · Vignesh R

Hi,

Quoted 8 lines · Tom Rini

Quoted 5 lines · Vignesh R

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

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 <[email protected]>

Quoted 2 lines

Quoted 2 lines

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.

Quoted 8 lines

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!

Quoted 31 lines · Tom Rini

Quoted 11 lines · Vignesh R

Hi,

Quoted 8 lines · Tom Rini

Quoted 5 lines · Vignesh R

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

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 <[email protected]>

Quoted 9 lines

Quoted 2 lines

Quoted 2 lines

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()).

Quoted 9 lines

Quoted 8 lines

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/

Quoted 12 lines

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?

Quoted 4 lines

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).

[...]

Quoted 10 lines · Tom Rini

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 22 lines · Vignesh R

[...]

Quoted 19 lines · Tom Rini

Quoted 4 lines

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).

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 29 lines · Tom Rini

Quoted 28 lines · Vignesh R

[...]

Quoted 10 lines · Tom Rini

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines

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/

Quoted 7 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 3 lines

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?

Quoted 11 lines · Tom Rini

Quoted 11 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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)

Quoted 20 lines · Vignesh R

Quoted 16 lines · Tom Rini

Quoted 11 lines

Quoted 3 lines

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.

Quoted 3 lines

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?

Quoted 2 lines

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.

Quoted 26 lines · Franklin S Cooper Jr

Quoted 22 lines · Vignesh R

Quoted 7 lines · Tom Rini

Quoted 7 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 4 lines

Quoted 3 lines

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)

Quoted 5 lines

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?

Quoted 3 lines

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.

Quoted 6 lines

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.