[PATCH 0/5] Reduce size of a few boards

[PATCH 0/5] Reduce size of a few boards

[Simon Glass]

This series is intended to be applied before with EFI clean-up series
from Takahiro-san. It reduces the size of a few boards by removing
features. It also allows the partition driver to be dropped in SPL,
thus eliminating the SPL size gains.

The helper-function patch must be applied after the EFI series, but
the others can come before.


Simon Glass (5):
  disk: Add an option for partitions in SPL
  tbs2910: Disable ext4 write
  rcar3_salvator-x: Drop EFI_LOADER
  phycore-rk3288: Avoid enabling partition support in SPL
  disk: Use a helper function to reduce duplication

 configs/phycore-rk3288_defconfig   |  2 +-
 configs/rcar3_salvator-x_defconfig |  2 +-
 configs/tbs2910_defconfig          |  1 -
 disk/Kconfig                       | 24 +++++++++++++---
 disk/Makefile                      |  6 ++--
 disk/disk-uclass.c                 | 46 ++++++++++++++++--------------
 drivers/block/blk-uclass.c         |  2 +-
 7 files changed, 51 insertions(+), 32 deletions(-)

-- 
2.35.1.723.g4982287a31-goog

[PATCH 1/5] disk: Add an option for partitions in SPL

[Simon Glass]

In some cases we do not want to enable partition support in SPL. Add an
option to allow this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 disk/Kconfig               | 24 ++++++++++++++++++++----
 disk/Makefile              |  6 +++---
 drivers/block/blk-uclass.c |  2 +-
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/disk/Kconfig b/disk/Kconfig
index cee16a80bc2..13700322e97 100644
--- a/disk/Kconfig
+++ b/disk/Kconfig
@@ -4,10 +4,6 @@ menu "Partition Types"
 config PARTITIONS
 	bool "Enable Partition Labels (disklabels) support"
 	default y
-	select SPL_SPRINTF if SPL
-	select TPL_SPRINTF if TPL
-	select SPL_STRTO if SPL
-	select TPL_STRTO if TPL
 	help
 	  Partition Labels (disklabels) Supported:
 	  Zero or more of the following:
@@ -23,6 +19,26 @@ config PARTITIONS
 	  you must configure support for at least one non-MTD partition type
 	  as well.
 
+config SPL_PARTITIONS
+	bool "Enable Partition Labels (disklabels) support in SPL"
+	default y if PARTITIONS
+	select SPL_SPRINTF
+	select SPL_STRTO
+	help
+	  Enable this for base partition support in SPL. The required
+	  partition table types shold be enabled separately. This add a
+	  small amount of size to SPL, typically 500 bytes.
+
+config TPL_PARTITIONS
+	bool "Enable Partition Labels (disklabels) support in TPL"
+	default y if PARTITIONS
+	select TPL_SPRINTF
+	select TPL_STRTO
+	help
+	  Enable this for base partition support in SPL. The required
+	  partition table types shold be enabled separately. This add a
+	  small amount of size to SPL, typically 500 bytes.
+
 config MAC_PARTITION
 	bool "Enable Apple's MacOS partition table"
 	depends on PARTITIONS
diff --git a/disk/Makefile b/disk/Makefile
index ec37b74f5f4..ffd7b07f867 100644
--- a/disk/Makefile
+++ b/disk/Makefile
@@ -5,9 +5,9 @@
 
 #ccflags-y += -DET_DEBUG -DDEBUG
 
-obj-$(CONFIG_PARTITIONS)	+= part.o
-ifdef CONFIG_$(SPL_)BLK
-obj-$(CONFIG_PARTITIONS) 	+= disk-uclass.o
+obj-$(CONFIG_$(SPL_TPL_)PARTITIONS)	+= part.o
+ifdef CONFIG_$(SPL_TPL_)BLK
+obj-$(CONFIG_$(SPL_TPL_)PARTITIONS)	+= disk-uclass.o
 endif
 obj-$(CONFIG_$(SPL_)MAC_PARTITION)   += part_mac.o
 obj-$(CONFIG_$(SPL_)DOS_PARTITION)   += part_dos.o
diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 58dc74e71f1..bcd18ed38b2 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -712,7 +712,7 @@ int blk_unbind_all(int if_type)
 
 static int blk_post_probe(struct udevice *dev)
 {
-	if (IS_ENABLED(CONFIG_PARTITIONS) &&
+	if (CONFIG_IS_ENABLED(PARTITIONS) &&
 	    IS_ENABLED(CONFIG_HAVE_BLOCK_DEVICE)) {
 		struct blk_desc *desc = dev_get_uclass_plat(dev);
 
-- 
2.35.1.723.g4982287a31-goog

Re: [PATCH 1/5] disk: Add an option for partitions in SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

On Fri, Mar 11, 2022 at 12:10:01PM -0700, Simon Glass wrote:

> In some cases we do not want to enable partition support in SPL. Add an
> option to allow this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  disk/Kconfig               | 24 ++++++++++++++++++++----
>  disk/Makefile              |  6 +++---
>  drivers/block/blk-uclass.c |  2 +-
>  3 files changed, 24 insertions(+), 8 deletions(-)

I'm not sure this makes sense?  I thought when I looked in to this last
the only place where we had partition code being linked and not
discarded in the case of SPL and no devices that would have partitions
on them was one xilinx platform.  How do we get to bringing in partition
code and not having something that uses it?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/5] disk: Add an option for partitions in SPL

[Simon Glass]

Hi Tom,

On Mon, 14 Mar 2022 at 06:49, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Mar 11, 2022 at 12:10:01PM -0700, Simon Glass wrote:
>
> > In some cases we do not want to enable partition support in SPL. Add an
> > option to allow this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  disk/Kconfig               | 24 ++++++++++++++++++++----
> >  disk/Makefile              |  6 +++---
> >  drivers/block/blk-uclass.c |  2 +-
> >  3 files changed, 24 insertions(+), 8 deletions(-)
>
> I'm not sure this makes sense?  I thought when I looked in to this last
> the only place where we had partition code being linked and not
> discarded in the case of SPL and no devices that would have partitions
> on them was one xilinx platform.  How do we get to bringing in partition
> code and not having something that uses it?

The problem is that drivers are not discarded and Takahiro's series
adds a driver for partitions.

So yes, we were able to get away with this before, but cannot now.

Regards,
Simon

Re: [PATCH 1/5] disk: Add an option for partitions in SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1530 bytes --]

On Mon, Mar 14, 2022 at 12:24:41PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 14 Mar 2022 at 06:49, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Mar 11, 2022 at 12:10:01PM -0700, Simon Glass wrote:
> >
> > > In some cases we do not want to enable partition support in SPL. Add an
> > > option to allow this.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > >  disk/Kconfig               | 24 ++++++++++++++++++++----
> > >  disk/Makefile              |  6 +++---
> > >  drivers/block/blk-uclass.c |  2 +-
> > >  3 files changed, 24 insertions(+), 8 deletions(-)
> >
> > I'm not sure this makes sense?  I thought when I looked in to this last
> > the only place where we had partition code being linked and not
> > discarded in the case of SPL and no devices that would have partitions
> > on them was one xilinx platform.  How do we get to bringing in partition
> > code and not having something that uses it?
> 
> The problem is that drivers are not discarded and Takahiro's series
> adds a driver for partitions.
> 
> So yes, we were able to get away with this before, but cannot now.

Things which aren't needed / used need to be discarded.  So we might
have use cases for this, yes (I was thinking after I sent that OK, yes,
imx probably tends to not need partition support since we read at raw
offsets, outside from maybe falcon mode using targets).  But we
shouldn't generally be now pulling in drivers that aren't functionally
used.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/5] disk: Add an option for partitions in SPL

[Simon Glass]

Hi Tom,

On Mon, 14 Mar 2022 at 12:31, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Mar 14, 2022 at 12:24:41PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 14 Mar 2022 at 06:49, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Mar 11, 2022 at 12:10:01PM -0700, Simon Glass wrote:
> > >
> > > > In some cases we do not want to enable partition support in SPL. Add an
> > > > option to allow this.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > >  disk/Kconfig               | 24 ++++++++++++++++++++----
> > > >  disk/Makefile              |  6 +++---
> > > >  drivers/block/blk-uclass.c |  2 +-
> > > >  3 files changed, 24 insertions(+), 8 deletions(-)
> > >
> > > I'm not sure this makes sense?  I thought when I looked in to this last
> > > the only place where we had partition code being linked and not
> > > discarded in the case of SPL and no devices that would have partitions
> > > on them was one xilinx platform.  How do we get to bringing in partition
> > > code and not having something that uses it?
> >
> > The problem is that drivers are not discarded and Takahiro's series
> > adds a driver for partitions.
> >
> > So yes, we were able to get away with this before, but cannot now.
>
> Things which aren't needed / used need to be discarded.  So we might
> have use cases for this, yes (I was thinking after I sent that OK, yes,
> imx probably tends to not need partition support since we read at raw
> offsets, outside from maybe falcon mode using targets).  But we
> shouldn't generally be now pulling in drivers that aren't functionally
> used.

We don't have a way of detecting whether a driver is used, with driver
model, so the only option is Kconfig. Does that make sense? Please let
me know if there is any disconnect here.

Regards,
Simon

Re: [PATCH 1/5] disk: Add an option for partitions in SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

On Mon, Mar 14, 2022 at 01:21:07PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 14 Mar 2022 at 12:31, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Mar 14, 2022 at 12:24:41PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 14 Mar 2022 at 06:49, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Fri, Mar 11, 2022 at 12:10:01PM -0700, Simon Glass wrote:
> > > >
> > > > > In some cases we do not want to enable partition support in SPL. Add an
> > > > > option to allow this.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >
> > > > >  disk/Kconfig               | 24 ++++++++++++++++++++----
> > > > >  disk/Makefile              |  6 +++---
> > > > >  drivers/block/blk-uclass.c |  2 +-
> > > > >  3 files changed, 24 insertions(+), 8 deletions(-)
> > > >
> > > > I'm not sure this makes sense?  I thought when I looked in to this last
> > > > the only place where we had partition code being linked and not
> > > > discarded in the case of SPL and no devices that would have partitions
> > > > on them was one xilinx platform.  How do we get to bringing in partition
> > > > code and not having something that uses it?
> > >
> > > The problem is that drivers are not discarded and Takahiro's series
> > > adds a driver for partitions.
> > >
> > > So yes, we were able to get away with this before, but cannot now.
> >
> > Things which aren't needed / used need to be discarded.  So we might
> > have use cases for this, yes (I was thinking after I sent that OK, yes,
> > imx probably tends to not need partition support since we read at raw
> > offsets, outside from maybe falcon mode using targets).  But we
> > shouldn't generally be now pulling in drivers that aren't functionally
> > used.
> 
> We don't have a way of detecting whether a driver is used, with driver
> model, so the only option is Kconfig. Does that make sense? Please let
> me know if there is any disconnect here.

Yes, it's possible that we have platforms enabling functionality they
don't _need_, but unlikely (aside from the we don't need partition
reading, we do raw offsets from block device only platforms).  So there
shouldn't be a whole bunch of stuff now being pulled in to SPL that
wasn't before and doesn't need investigation / explanation.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/5] disk: Add an option for partitions in SPL

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

On Fri, Mar 11, 2022 at 12:10:01PM -0700, Simon Glass wrote:

> In some cases we do not want to enable partition support in SPL. Add an
> option to allow this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

With a slight tweak (as disk/disk-uclass.c isn't in yet), applied to
u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/5] disk: Add an option for partitions in SPL

[AKASHI Takahiro]

Hi Simon,

On Fri, Mar 11, 2022 at 12:10:01PM -0700, Simon Glass wrote:
> In some cases we do not want to enable partition support in SPL. Add an
> option to allow this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  disk/Kconfig               | 24 ++++++++++++++++++++----
>  disk/Makefile              |  6 +++---
>  drivers/block/blk-uclass.c |  2 +-
>  3 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/disk/Kconfig b/disk/Kconfig
> index cee16a80bc2..13700322e97 100644
> --- a/disk/Kconfig
> +++ b/disk/Kconfig
> @@ -4,10 +4,6 @@ menu "Partition Types"
>  config PARTITIONS
>  	bool "Enable Partition Labels (disklabels) support"
>  	default y
> -	select SPL_SPRINTF if SPL
> -	select TPL_SPRINTF if TPL
> -	select SPL_STRTO if SPL
> -	select TPL_STRTO if TPL
>  	help
>  	  Partition Labels (disklabels) Supported:
>  	  Zero or more of the following:
> @@ -23,6 +19,26 @@ config PARTITIONS
>  	  you must configure support for at least one non-MTD partition type
>  	  as well.
>  
> +config SPL_PARTITIONS
> +	bool "Enable Partition Labels (disklabels) support in SPL"
> +	default y if PARTITIONS
> +	select SPL_SPRINTF
> +	select SPL_STRTO
> +	help
> +	  Enable this for base partition support in SPL. The required
> +	  partition table types shold be enabled separately. This add a
> +	  small amount of size to SPL, typically 500 bytes.

Despite of you aim here, this option doesn't work.
As I mentioned in
  https://lists.denx.de/pipermail/u-boot/2022-April/481258.html
phycore-rk3288_defconfig, for instance, compiles disk/part.o
(and disk/disk-uclass.o) in SPL image.

Basically, I think that CONFIG_(SPL_)PARTITIONS should not be
user-selectable. Instead, it should be implicitly selected
by any of CONFIG_(SPL_)XXX_PARTITION's.

-Takahiro Akashi

> +
> +config TPL_PARTITIONS
> +	bool "Enable Partition Labels (disklabels) support in TPL"
> +	default y if PARTITIONS
> +	select TPL_SPRINTF
> +	select TPL_STRTO
> +	help
> +	  Enable this for base partition support in SPL. The required
> +	  partition table types shold be enabled separately. This add a
> +	  small amount of size to SPL, typically 500 bytes.
> +
>  config MAC_PARTITION
>  	bool "Enable Apple's MacOS partition table"
>  	depends on PARTITIONS
> diff --git a/disk/Makefile b/disk/Makefile
> index ec37b74f5f4..ffd7b07f867 100644
> --- a/disk/Makefile
> +++ b/disk/Makefile
> @@ -5,9 +5,9 @@
>  
>  #ccflags-y += -DET_DEBUG -DDEBUG
>  
> -obj-$(CONFIG_PARTITIONS)	+= part.o
> -ifdef CONFIG_$(SPL_)BLK
> -obj-$(CONFIG_PARTITIONS) 	+= disk-uclass.o
> +obj-$(CONFIG_$(SPL_TPL_)PARTITIONS)	+= part.o
> +ifdef CONFIG_$(SPL_TPL_)BLK
> +obj-$(CONFIG_$(SPL_TPL_)PARTITIONS)	+= disk-uclass.o
>  endif
>  obj-$(CONFIG_$(SPL_)MAC_PARTITION)   += part_mac.o
>  obj-$(CONFIG_$(SPL_)DOS_PARTITION)   += part_dos.o
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index 58dc74e71f1..bcd18ed38b2 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -712,7 +712,7 @@ int blk_unbind_all(int if_type)
>  
>  static int blk_post_probe(struct udevice *dev)
>  {
> -	if (IS_ENABLED(CONFIG_PARTITIONS) &&
> +	if (CONFIG_IS_ENABLED(PARTITIONS) &&
>  	    IS_ENABLED(CONFIG_HAVE_BLOCK_DEVICE)) {
>  		struct blk_desc *desc = dev_get_uclass_plat(dev);
>  
> -- 
> 2.35.1.723.g4982287a31-goog
> 

[PATCH 2/5] tbs2910: Disable ext4 write

[Simon Glass]

This board is right up against the size limit. Drop ext4 writing to give
it some more space.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 configs/tbs2910_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configs/tbs2910_defconfig b/configs/tbs2910_defconfig
index e1278f2e70a..9819df55f05 100644
--- a/configs/tbs2910_defconfig
+++ b/configs/tbs2910_defconfig
@@ -50,7 +50,6 @@ CONFIG_CMD_TIME=y
 CONFIG_CMD_SYSBOOT=y
 CONFIG_CMD_EXT2=y
 CONFIG_CMD_EXT4=y
-CONFIG_CMD_EXT4_WRITE=y
 CONFIG_CMD_FAT=y
 CONFIG_CMD_FS_GENERIC=y
 CONFIG_EFI_PARTITION=y
-- 
2.35.1.723.g4982287a31-goog

Re: [PATCH 2/5] tbs2910: Disable ext4 write

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 392 bytes --]

On Fri, Mar 11, 2022 at 12:10:02PM -0700, Simon Glass wrote:

> This board is right up against the size limit. Drop ext4 writing to give
> it some more space.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

I would really like to see what LTO does here instead of removing
functionality.  I'm sure it shrinks things enough to fit, but it does
need run time testing.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/5] tbs2910: Disable ext4 write

[Simon Glass]

Hi Tom,

On Fri, 11 Mar 2022 at 12:58, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Mar 11, 2022 at 12:10:02PM -0700, Simon Glass wrote:
>
> > This board is right up against the size limit. Drop ext4 writing to give
> > it some more space.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
>
> I would really like to see what LTO does here instead of removing
> functionality.  I'm sure it shrinks things enough to fit, but it does
> need run time testing.

Well let's see if the maintainer responds. But this is blocking the
EFI series which we really need to get in it. It can always be cleaned
up later.

Regards,
Simon

Re: [PATCH 2/5] tbs2910: Disable ext4 write

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

On Fri, Mar 11, 2022 at 07:24:41PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 11 Mar 2022 at 12:58, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Mar 11, 2022 at 12:10:02PM -0700, Simon Glass wrote:
> >
> > > This board is right up against the size limit. Drop ext4 writing to give
> > > it some more space.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> >
> > I would really like to see what LTO does here instead of removing
> > functionality.  I'm sure it shrinks things enough to fit, but it does
> > need run time testing.
> 
> Well let's see if the maintainer responds. But this is blocking the
> EFI series which we really need to get in it. It can always be cleaned
> up later.

Erm, Soeren has been quite responsive.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/5] tbs2910: Disable ext4 write

[Simon Glass]

Hi Tom,

On Fri, 11 Mar 2022 at 21:04, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Mar 11, 2022 at 07:24:41PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 11 Mar 2022 at 12:58, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Mar 11, 2022 at 12:10:02PM -0700, Simon Glass wrote:
> > >
> > > > This board is right up against the size limit. Drop ext4 writing to give
> > > > it some more space.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > >
> > > I would really like to see what LTO does here instead of removing
> > > functionality.  I'm sure it shrinks things enough to fit, but it does
> > > need run time testing.
> >
> > Well let's see if the maintainer responds. But this is blocking the
> > EFI series which we really need to get in it. It can always be cleaned
> > up later.
>
> Erm, Soeren has been quite responsive.

OK, good. It even looks like our emails crossed.

Regards,
Simon

Re: [PATCH 2/5] tbs2910: Disable ext4 write

[Soeren Moch]

On 11.03.22 20:10, Simon Glass wrote:
> This board is right up against the size limit. Drop ext4 writing to give
> it some more space.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
NAK.

At least for now.
We should not randomly disable user visible functionality and not
without investigating other possible options.

Regards,
Soeren
> ---
>
>   configs/tbs2910_defconfig | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/configs/tbs2910_defconfig b/configs/tbs2910_defconfig
> index e1278f2e70a..9819df55f05 100644
> --- a/configs/tbs2910_defconfig
> +++ b/configs/tbs2910_defconfig
> @@ -50,7 +50,6 @@ CONFIG_CMD_TIME=y
>   CONFIG_CMD_SYSBOOT=y
>   CONFIG_CMD_EXT2=y
>   CONFIG_CMD_EXT4=y
> -CONFIG_CMD_EXT4_WRITE=y
>   CONFIG_CMD_FAT=y
>   CONFIG_CMD_FS_GENERIC=y
>   CONFIG_EFI_PARTITION=y

Re: [PATCH 2/5] tbs2910: Disable ext4 write

[Simon Glass]

Hi Soeren,

On Fri, 11 Mar 2022 at 15:57, Soeren Moch <smoch@web.de> wrote:
>
>
>
> On 11.03.22 20:10, Simon Glass wrote:
> > This board is right up against the size limit. Drop ext4 writing to give
> > it some more space.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> NAK.
>
> At least for now.
> We should not randomly disable user visible functionality and not
> without investigating other possible options.

OK please go ahead and send a suitable patch.

Regards,
Simon


> > ---
> >
> >   configs/tbs2910_defconfig | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/configs/tbs2910_defconfig b/configs/tbs2910_defconfig
> > index e1278f2e70a..9819df55f05 100644
> > --- a/configs/tbs2910_defconfig
> > +++ b/configs/tbs2910_defconfig
> > @@ -50,7 +50,6 @@ CONFIG_CMD_TIME=y
> >   CONFIG_CMD_SYSBOOT=y
> >   CONFIG_CMD_EXT2=y
> >   CONFIG_CMD_EXT4=y
> > -CONFIG_CMD_EXT4_WRITE=y
> >   CONFIG_CMD_FAT=y
> >   CONFIG_CMD_FS_GENERIC=y
> >   CONFIG_EFI_PARTITION=y
>

[PATCH 3/5] rcar3_salvator-x: Drop EFI_LOADER

[Simon Glass]

This board is too close to the limit to enable this feature. Drop it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 configs/rcar3_salvator-x_defconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configs/rcar3_salvator-x_defconfig b/configs/rcar3_salvator-x_defconfig
index 5fb27d257af..402d56a1321 100644
--- a/configs/rcar3_salvator-x_defconfig
+++ b/configs/rcar3_salvator-x_defconfig
@@ -102,4 +102,4 @@ CONFIG_USB_EHCI_HCD=y
 CONFIG_USB_EHCI_GENERIC=y
 CONFIG_USB_STORAGE=y
 CONFIG_OF_LIBFDT_OVERLAY=y
-# CONFIG_EFI_UNICODE_CAPITALIZATION is not set
+# CONFIG_EFI_LOADER is not set
-- 
2.35.1.723.g4982287a31-goog

Re: [PATCH 3/5] rcar3_salvator-x: Drop EFI_LOADER

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

On Fri, Mar 11, 2022 at 12:10:03PM -0700, Simon Glass wrote:

> This board is too close to the limit to enable this feature. Drop it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  configs/rcar3_salvator-x_defconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configs/rcar3_salvator-x_defconfig b/configs/rcar3_salvator-x_defconfig
> index 5fb27d257af..402d56a1321 100644
> --- a/configs/rcar3_salvator-x_defconfig
> +++ b/configs/rcar3_salvator-x_defconfig
> @@ -102,4 +102,4 @@ CONFIG_USB_EHCI_HCD=y
>  CONFIG_USB_EHCI_GENERIC=y
>  CONFIG_USB_STORAGE=y
>  CONFIG_OF_LIBFDT_OVERLAY=y
> -# CONFIG_EFI_UNICODE_CAPITALIZATION is not set
> +# CONFIG_EFI_LOADER is not set

How close to the limit, but also how interested are the maintainers in
having UEFI boot?  This is likely another place where run-time checking
LTO is advised, but is likely fine as rzg2_beacon already enables it.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/5] rcar3_salvator-x: Drop EFI_LOADER

[Simon Glass]

Hi Tom,

On Fri, 11 Mar 2022 at 13:21, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Mar 11, 2022 at 12:10:03PM -0700, Simon Glass wrote:
>
> > This board is too close to the limit to enable this feature. Drop it.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  configs/rcar3_salvator-x_defconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/configs/rcar3_salvator-x_defconfig b/configs/rcar3_salvator-x_defconfig
> > index 5fb27d257af..402d56a1321 100644
> > --- a/configs/rcar3_salvator-x_defconfig
> > +++ b/configs/rcar3_salvator-x_defconfig
> > @@ -102,4 +102,4 @@ CONFIG_USB_EHCI_HCD=y
> >  CONFIG_USB_EHCI_GENERIC=y
> >  CONFIG_USB_STORAGE=y
> >  CONFIG_OF_LIBFDT_OVERLAY=y
> > -# CONFIG_EFI_UNICODE_CAPITALIZATION is not set
> > +# CONFIG_EFI_LOADER is not set
>
> How close to the limit, but also how interested are the maintainers in
> having UEFI boot?  This is likely another place where run-time checking
> LTO is advised, but is likely fine as rzg2_beacon already enables it.

About 2.5KB:

u-boot.img exceeds file size limit:
  limit:  0x100000 bytes
  actual: 0x1009b5 bytes
  excess: 0x9b5 bytes

The board is pretty big - looks like >100KB for the pinmux stuff.

Regards,
Simon

Re: [PATCH 3/5] rcar3_salvator-x: Drop EFI_LOADER

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1640 bytes --]

On Fri, Mar 11, 2022 at 07:24:42PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 11 Mar 2022 at 13:21, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Mar 11, 2022 at 12:10:03PM -0700, Simon Glass wrote:
> >
> > > This board is too close to the limit to enable this feature. Drop it.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > >  configs/rcar3_salvator-x_defconfig | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/configs/rcar3_salvator-x_defconfig b/configs/rcar3_salvator-x_defconfig
> > > index 5fb27d257af..402d56a1321 100644
> > > --- a/configs/rcar3_salvator-x_defconfig
> > > +++ b/configs/rcar3_salvator-x_defconfig
> > > @@ -102,4 +102,4 @@ CONFIG_USB_EHCI_HCD=y
> > >  CONFIG_USB_EHCI_GENERIC=y
> > >  CONFIG_USB_STORAGE=y
> > >  CONFIG_OF_LIBFDT_OVERLAY=y
> > > -# CONFIG_EFI_UNICODE_CAPITALIZATION is not set
> > > +# CONFIG_EFI_LOADER is not set
> >
> > How close to the limit, but also how interested are the maintainers in
> > having UEFI boot?  This is likely another place where run-time checking
> > LTO is advised, but is likely fine as rzg2_beacon already enables it.
> 
> About 2.5KB:
> 
> u-boot.img exceeds file size limit:
>   limit:  0x100000 bytes
>   actual: 0x1009b5 bytes
>   excess: 0x9b5 bytes
> 
> The board is pretty big - looks like >100KB for the pinmux stuff.

Well, being an arm64 board, it probably wants to stay using standard
boot methods, so enable LTO rather than disable functionality is likely
the first path to take, but I'll let the board maintainers chime in.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 3/5] rcar3_salvator-x: Drop EFI_LOADER

[Simon Glass]

Hi Tom,

On Mon, 14 Mar 2022 at 06:54, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Mar 11, 2022 at 07:24:42PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 11 Mar 2022 at 13:21, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Fri, Mar 11, 2022 at 12:10:03PM -0700, Simon Glass wrote:
> > >
> > > > This board is too close to the limit to enable this feature. Drop it.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > >  configs/rcar3_salvator-x_defconfig | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/configs/rcar3_salvator-x_defconfig b/configs/rcar3_salvator-x_defconfig
> > > > index 5fb27d257af..402d56a1321 100644
> > > > --- a/configs/rcar3_salvator-x_defconfig
> > > > +++ b/configs/rcar3_salvator-x_defconfig
> > > > @@ -102,4 +102,4 @@ CONFIG_USB_EHCI_HCD=y
> > > >  CONFIG_USB_EHCI_GENERIC=y
> > > >  CONFIG_USB_STORAGE=y
> > > >  CONFIG_OF_LIBFDT_OVERLAY=y
> > > > -# CONFIG_EFI_UNICODE_CAPITALIZATION is not set
> > > > +# CONFIG_EFI_LOADER is not set
> > >
> > > How close to the limit, but also how interested are the maintainers in
> > > having UEFI boot?  This is likely another place where run-time checking
> > > LTO is advised, but is likely fine as rzg2_beacon already enables it.
> >
> > About 2.5KB:
> >
> > u-boot.img exceeds file size limit:
> >   limit:  0x100000 bytes
> >   actual: 0x1009b5 bytes
> >   excess: 0x9b5 bytes
> >
> > The board is pretty big - looks like >100KB for the pinmux stuff.
>
> Well, being an arm64 board, it probably wants to stay using standard
> boot methods, so enable LTO rather than disable functionality is likely
> the first path to take, but I'll let the board maintainers chime in.

OK I'll wait for the chime.

Regards,
Simon

[PATCH 4/5] phycore-rk3288: Avoid enabling partition support in SPL

[Simon Glass]

This is not needed or used, and adds code size. Drop it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 configs/phycore-rk3288_defconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configs/phycore-rk3288_defconfig b/configs/phycore-rk3288_defconfig
index 63aabfbe81b..a67be72f366 100644
--- a/configs/phycore-rk3288_defconfig
+++ b/configs/phycore-rk3288_defconfig
@@ -35,9 +35,9 @@ CONFIG_CMD_CACHE=y
 CONFIG_CMD_TIME=y
 CONFIG_CMD_PMIC=y
 CONFIG_CMD_REGULATOR=y
+# CONFIG_SPL_PARTITIONS is not set
 # CONFIG_SPL_DOS_PARTITION is not set
 # CONFIG_SPL_EFI_PARTITION is not set
-CONFIG_SPL_PARTITION_UUIDS=y
 CONFIG_SPL_OF_CONTROL=y
 CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
 CONFIG_ENV_IS_IN_MMC=y
-- 
2.35.1.723.g4982287a31-goog

Re: [PATCH 4/5] phycore-rk3288: Avoid enabling partition support in SPL

[Kever Yang]

On 2022/3/12 03:10, Simon Glass wrote:
> This is not needed or used, and adds code size. Drop it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>
>   configs/phycore-rk3288_defconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/configs/phycore-rk3288_defconfig b/configs/phycore-rk3288_defconfig
> index 63aabfbe81b..a67be72f366 100644
> --- a/configs/phycore-rk3288_defconfig
> +++ b/configs/phycore-rk3288_defconfig
> @@ -35,9 +35,9 @@ CONFIG_CMD_CACHE=y
>   CONFIG_CMD_TIME=y
>   CONFIG_CMD_PMIC=y
>   CONFIG_CMD_REGULATOR=y
> +# CONFIG_SPL_PARTITIONS is not set
>   # CONFIG_SPL_DOS_PARTITION is not set
>   # CONFIG_SPL_EFI_PARTITION is not set
> -CONFIG_SPL_PARTITION_UUIDS=y
>   CONFIG_SPL_OF_CONTROL=y
>   CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
>   CONFIG_ENV_IS_IN_MMC=y

[PATCH 5/5] disk: Use a helper function to reduce duplication

[Simon Glass]

Reduce the duplicated code slightly by using a helper function to handle
the common code.

This reduces the code size very slightly.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 disk/disk-uclass.c | 46 +++++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/disk/disk-uclass.c b/disk/disk-uclass.c
index 72ff62ebf58..7adfe6b2545 100644
--- a/disk/disk-uclass.c
+++ b/disk/disk-uclass.c
@@ -65,26 +65,38 @@ int part_create_block_devices(struct udevice *blk_dev)
 	return 0;
 }
 
+static int blk_part_setup(struct udevice *dev, lbaint_t *startp,
+			  lbaint_t blkcnt)
+{
+	struct disk_part *part;
+
+	part = dev_get_uclass_plat(dev);
+	if (*startp >= part->gpt_part_info.size)
+		return -E2BIG;
+
+	if (*startp + blkcnt > part->gpt_part_info.size)
+		blkcnt = part->gpt_part_info.size - *startp;
+	*startp += part->gpt_part_info.start;
+
+	return 0;
+}
+
 static ulong blk_part_read(struct udevice *dev, lbaint_t start,
 			   lbaint_t blkcnt, void *buffer)
 {
 	struct udevice *parent;
-	struct disk_part *part;
 	const struct blk_ops *ops;
+	int ret;
 
 	parent = dev_get_parent(dev);
 	ops = blk_get_ops(parent);
 	if (!ops->read)
 		return -ENOSYS;
 
-	part = dev_get_uclass_plat(dev);
-	if (start >= part->gpt_part_info.size)
+	ret = blk_part_setup(dev, &start, blkcnt);
+	if (ret)
 		return 0;
 
-	if ((start + blkcnt) > part->gpt_part_info.size)
-		blkcnt = part->gpt_part_info.size - start;
-	start += part->gpt_part_info.start;
-
 	return ops->read(parent, start, blkcnt, buffer);
 }
 
@@ -92,22 +104,18 @@ static ulong blk_part_write(struct udevice *dev, lbaint_t start,
 			    lbaint_t blkcnt, const void *buffer)
 {
 	struct udevice *parent;
-	struct disk_part *part;
 	const struct blk_ops *ops;
+	int ret;
 
 	parent = dev_get_parent(dev);
 	ops = blk_get_ops(parent);
 	if (!ops->write)
 		return -ENOSYS;
 
-	part = dev_get_uclass_plat(dev);
-	if (start >= part->gpt_part_info.size)
+	ret = blk_part_setup(dev, &start, blkcnt);
+	if (ret)
 		return 0;
 
-	if ((start + blkcnt) > part->gpt_part_info.size)
-		blkcnt = part->gpt_part_info.size - start;
-	start += part->gpt_part_info.start;
-
 	return ops->write(parent, start, blkcnt, buffer);
 }
 
@@ -115,22 +123,18 @@ static ulong blk_part_erase(struct udevice *dev, lbaint_t start,
 			    lbaint_t blkcnt)
 {
 	struct udevice *parent;
-	struct disk_part *part;
 	const struct blk_ops *ops;
+	int ret;
 
 	parent = dev_get_parent(dev);
 	ops = blk_get_ops(parent);
 	if (!ops->erase)
 		return -ENOSYS;
 
-	part = dev_get_uclass_plat(dev);
-	if (start >= part->gpt_part_info.size)
+	ret = blk_part_setup(dev, &start, blkcnt);
+	if (ret)
 		return 0;
 
-	if ((start + blkcnt) > part->gpt_part_info.size)
-		blkcnt = part->gpt_part_info.size - start;
-	start += part->gpt_part_info.start;
-
 	return ops->erase(parent, start, blkcnt);
 }
 
-- 
2.35.1.723.g4982287a31-goog