| Message ID | 20200329160443.11518-4-dariobin@libero.it |
|---|---|
| State | Accepted |
| Commit | 59006608d65b242b19176f1a1fdeeb99391654d2 |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | Support reading an u32 from a multi-value property | expand |
On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote: > > Now reading a 32 bit value from a device-tree property can be expressed > as reading the first element of an array with a single value. > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > > --- > > drivers/core/of_access.c | 16 +--------------- > drivers/core/ofnode.c | 23 ++--------------------- > 2 files changed, 3 insertions(+), 36 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org> Can you please check the code-size delta in SPL on a suitable board?
> Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto: > > > On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote: > > > > Now reading a 32 bit value from a device-tree property can be expressed > > as reading the first element of an array with a single value. > > > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > > > > --- > > > > drivers/core/of_access.c | 16 +--------------- > > drivers/core/ofnode.c | 23 ++--------------------- > > 2 files changed, 3 insertions(+), 36 deletions(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> > > Can you please check the code-size delta in SPL on a suitable board? I have a black beaglebone available (am335x_evm_defconfig). u-boot-spl.map generated without applying the refactoring patch .... .text.ofnode_read_u32 0x0000000000000000 0x2e drivers/built-in.o .text.ofnode_read_u32_index 0x0000000000000000 0x38 drivers/built-in.o .text.ofnode_read_u32_default 0x0000000000000000 0x12 drivers/built-in.o u-boot-spl.map genarated with the refactoring patch applied .... .text.ofnode_read_u32_index 0x0000000000000000 0x38 drivers/built-in.o .text.ofnode_read_u32 0x0000000000000000 0x8 drivers/built-in.o .text.ofnode_read_u32_default 0x0000000000000000 0x14 drivers/built-in.o I hope I have correctly answered what you asked me. Otherwise I am available to perform the right checks on your indications. Thanks and regards, Dario Binacchi
Hi Dario, On Wed, 1 Apr 2020 at 13:34, <dariobin@libero.it> wrote: > > > > Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto: > > > > > > On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote: > > > > > > Now reading a 32 bit value from a device-tree property can be expressed > > > as reading the first element of an array with a single value. > > > > > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > > > > > > --- > > > > > > drivers/core/of_access.c | 16 +--------------- > > > drivers/core/ofnode.c | 23 ++--------------------- > > > 2 files changed, 3 insertions(+), 36 deletions(-) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > Can you please check the code-size delta in SPL on a suitable board? > > I have a black beaglebone available (am335x_evm_defconfig). > > u-boot-spl.map generated without applying the refactoring patch > .... > .text.ofnode_read_u32 > 0x0000000000000000 0x2e drivers/built-in.o > .text.ofnode_read_u32_index > 0x0000000000000000 0x38 drivers/built-in.o > .text.ofnode_read_u32_default > 0x0000000000000000 0x12 drivers/built-in.o > > u-boot-spl.map genarated with the refactoring patch applied > .... > .text.ofnode_read_u32_index > 0x0000000000000000 0x38 drivers/built-in.o > .text.ofnode_read_u32 > 0x0000000000000000 0x8 drivers/built-in.o > .text.ofnode_read_u32_default > 0x0000000000000000 0x14 drivers/built-in.o Possibly, but a better test is to build your branch with the patch in: buildman -b <branch> <board> Then check the size: buildman -b <branch> -sS or function detail: buildman -b <branch> -sSB That will give us a true picture for SPL. It will show incremental size increase with your patch. See buildman docs for more info. Regards, Simon
> Il 2 aprile 2020 alle 20.54 Simon Glass <sjg@chromium.org> ha scritto: > > > Hi Dario, > > On Wed, 1 Apr 2020 at 13:34, <dariobin@libero.it> wrote: > > > > > > > Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto: > > > > > > > > > On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote: > > > > > > > > Now reading a 32 bit value from a device-tree property can be expressed > > > > as reading the first element of an array with a single value. > > > > > > > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > > > > > > > > --- > > > > > > > > drivers/core/of_access.c | 16 +--------------- > > > > drivers/core/ofnode.c | 23 ++--------------------- > > > > 2 files changed, 3 insertions(+), 36 deletions(-) > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > Can you please check the code-size delta in SPL on a suitable board? > > > > I have a black beaglebone available (am335x_evm_defconfig). > > > > u-boot-spl.map generated without applying the refactoring patch > > .... > > .text.ofnode_read_u32 > > 0x0000000000000000 0x2e drivers/built-in.o > > .text.ofnode_read_u32_index > > 0x0000000000000000 0x38 drivers/built-in.o > > .text.ofnode_read_u32_default > > 0x0000000000000000 0x12 drivers/built-in.o > > > > u-boot-spl.map genarated with the refactoring patch applied > > .... > > .text.ofnode_read_u32_index > > 0x0000000000000000 0x38 drivers/built-in.o > > .text.ofnode_read_u32 > > 0x0000000000000000 0x8 drivers/built-in.o > > .text.ofnode_read_u32_default > > 0x0000000000000000 0x14 drivers/built-in.o > > Possibly, but a better test is to build your branch with the patch in: > > buildman -b <branch> <board> > > Then check the size: > > buildman -b <branch> -sS > > or function detail: > > buildman -b <branch> -sSB > > That will give us a true picture for SPL. It will show incremental > size increase with your patch. Hi Simon, this is the buildman response: ... 03: dm: core: support reading a single indexed u32 value 04: dm: core: refactor functions reading an u32 from dt arm: (for 4/708 boards) all +11.5 bss -10.0 text +21.5 am335x_hs_evm : all +24 text +24 u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) function old new delta ofnode_read_u32_index - 62 +62 ofnode_read_u32_default 18 20 +2 ofnode_read_u32 46 8 -38 am335x_hs_evm_uart: all +24 text +24 u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) function old new delta ofnode_read_u32_index - 62 +62 ofnode_read_u32_default 18 20 +2 ofnode_read_u32 46 8 -38 am335x_evm : bss -24 text +24 u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) function old new delta ofnode_read_u32_index - 62 +62 ofnode_read_u32_default 18 20 +2 ofnode_read_u32 46 8 -38 am335x_boneblack_vboot: all -2 bss -16 text +14 u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) function old new delta ofnode_read_u32_index - 62 +62 ofnode_read_u32_default 18 20 +2 ofnode_read_u32 46 8 -38 Regards, Dario > > See buildman docs for more info. > > Regards, > Simon
Hi Dario, On Sat, 4 Apr 2020 at 06:49, <dariobin@libero.it> wrote: > > > Il 2 aprile 2020 alle 20.54 Simon Glass <sjg@chromium.org> ha scritto: > > > > > > Hi Dario, > > > > On Wed, 1 Apr 2020 at 13:34, <dariobin@libero.it> wrote: > > > > > > > > > > Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto: > > > > > > > > > > > > On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote: > > > > > > > > > > Now reading a 32 bit value from a device-tree property can be expressed > > > > > as reading the first element of an array with a single value. > > > > > > > > > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > > > > > > > > > > --- > > > > > > > > > > drivers/core/of_access.c | 16 +--------------- > > > > > drivers/core/ofnode.c | 23 ++--------------------- > > > > > 2 files changed, 3 insertions(+), 36 deletions(-) > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > Can you please check the code-size delta in SPL on a suitable board? > > > > > > I have a black beaglebone available (am335x_evm_defconfig). > > > > > > u-boot-spl.map generated without applying the refactoring patch > > > .... > > > .text.ofnode_read_u32 > > > 0x0000000000000000 0x2e drivers/built-in.o > > > .text.ofnode_read_u32_index > > > 0x0000000000000000 0x38 drivers/built-in.o > > > .text.ofnode_read_u32_default > > > 0x0000000000000000 0x12 drivers/built-in.o > > > > > > u-boot-spl.map genarated with the refactoring patch applied > > > .... > > > .text.ofnode_read_u32_index > > > 0x0000000000000000 0x38 drivers/built-in.o > > > .text.ofnode_read_u32 > > > 0x0000000000000000 0x8 drivers/built-in.o > > > .text.ofnode_read_u32_default > > > 0x0000000000000000 0x14 drivers/built-in.o > > > > Possibly, but a better test is to build your branch with the patch in: > > > > buildman -b <branch> <board> > > > > Then check the size: > > > > buildman -b <branch> -sS > > > > or function detail: > > > > buildman -b <branch> -sSB > > > > That will give us a true picture for SPL. It will show incremental > > size increase with your patch. > > Hi Simon, > this is the buildman response: > ... > 03: dm: core: support reading a single indexed u32 value > 04: dm: core: refactor functions reading an u32 from dt > arm: (for 4/708 boards) all +11.5 bss -10.0 text +21.5 > am335x_hs_evm : all +24 text +24 > u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) > function old new delta > ofnode_read_u32_index - 62 +62 > ofnode_read_u32_default 18 20 +2 > ofnode_read_u32 46 8 -38 > am335x_hs_evm_uart: all +24 text +24 > u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) > function old new delta > ofnode_read_u32_index - 62 +62 > ofnode_read_u32_default 18 20 +2 > ofnode_read_u32 46 8 -38 > am335x_evm : bss -24 text +24 > u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) > function old new delta > ofnode_read_u32_index - 62 +62 > ofnode_read_u32_default 18 20 +2 > ofnode_read_u32 46 8 -38 > am335x_boneblack_vboot: all -2 bss -16 text +14 > u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) > function old new delta > ofnode_read_u32_index - 62 +62 > ofnode_read_u32_default 18 20 +2 > ofnode_read_u32 46 8 -38 This does not actually look like SPL though, only U-Boot. I hope that means that SPL doesn't increase in size. Anyway for U-Boot proper it looks like it adds about 24 bytes of code. I think it is worth it. Regards, Simon
Hi Dario, On Sat, 4 Apr 2020 at 06:49, <dariobin@libero.it> wrote: > > > Il 2 aprile 2020 alle 20.54 Simon Glass <sjg@chromium.org> ha scritto: > > > > > > Hi Dario, > > > > On Wed, 1 Apr 2020 at 13:34, <dariobin@libero.it> wrote: > > > > > > > > > > Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto: > > > > > > > > > > > > On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote: > > > > > > > > > > Now reading a 32 bit value from a device-tree property can be expressed > > > > > as reading the first element of an array with a single value. > > > > > > > > > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > > > > > > > > > > --- > > > > > > > > > > drivers/core/of_access.c | 16 +--------------- > > > > > drivers/core/ofnode.c | 23 ++--------------------- > > > > > 2 files changed, 3 insertions(+), 36 deletions(-) > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > Can you please check the code-size delta in SPL on a suitable board? > > > > > > I have a black beaglebone available (am335x_evm_defconfig). > > > > > > u-boot-spl.map generated without applying the refactoring patch > > > .... > > > .text.ofnode_read_u32 > > > 0x0000000000000000 0x2e drivers/built-in.o > > > .text.ofnode_read_u32_index > > > 0x0000000000000000 0x38 drivers/built-in.o > > > .text.ofnode_read_u32_default > > > 0x0000000000000000 0x12 drivers/built-in.o > > > > > > u-boot-spl.map genarated with the refactoring patch applied > > > .... > > > .text.ofnode_read_u32_index > > > 0x0000000000000000 0x38 drivers/built-in.o > > > .text.ofnode_read_u32 > > > 0x0000000000000000 0x8 drivers/built-in.o > > > .text.ofnode_read_u32_default > > > 0x0000000000000000 0x14 drivers/built-in.o > > > > Possibly, but a better test is to build your branch with the patch in: > > > > buildman -b <branch> <board> > > > > Then check the size: > > > > buildman -b <branch> -sS > > > > or function detail: > > > > buildman -b <branch> -sSB > > > > That will give us a true picture for SPL. It will show incremental > > size increase with your patch. > > Hi Simon, > this is the buildman response: > ... > 03: dm: core: support reading a single indexed u32 value > 04: dm: core: refactor functions reading an u32 from dt > arm: (for 4/708 boards) all +11.5 bss -10.0 text +21.5 > am335x_hs_evm : all +24 text +24 > u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) > function old new delta > ofnode_read_u32_index - 62 +62 > ofnode_read_u32_default 18 20 +2 > ofnode_read_u32 46 8 -38 > am335x_hs_evm_uart: all +24 text +24 > u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) > function old new delta > ofnode_read_u32_index - 62 +62 > ofnode_read_u32_default 18 20 +2 > ofnode_read_u32 46 8 -38 > am335x_evm : bss -24 text +24 > u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) > function old new delta > ofnode_read_u32_index - 62 +62 > ofnode_read_u32_default 18 20 +2 > ofnode_read_u32 46 8 -38 > am335x_boneblack_vboot: all -2 bss -16 text +14 > u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26) > function old new delta > ofnode_read_u32_index - 62 +62 > ofnode_read_u32_default 18 20 +2 > ofnode_read_u32 46 8 -38 This does not actually look like SPL though, only U-Boot. I hope that means that SPL doesn't increase in size. Anyway for U-Boot proper it looks like it adds about 24 bytes of code. I think it is worth it. Regards, Simon Applied to u-boot-dm/next, thanks!
diff --git a/drivers/core/of_access.c b/drivers/core/of_access.c index 55e4a38fc5..d116d106d9 100644 --- a/drivers/core/of_access.c +++ b/drivers/core/of_access.c @@ -449,21 +449,7 @@ static void *of_find_property_value_of_size(const struct device_node *np, int of_read_u32(const struct device_node *np, const char *propname, u32 *outp) { - const __be32 *val; - - debug("%s: %s: ", __func__, propname); - if (!np) - return -EINVAL; - val = of_find_property_value_of_size(np, propname, sizeof(*outp)); - if (IS_ERR(val)) { - debug("(not found)\n"); - return PTR_ERR(val); - } - - *outp = be32_to_cpup(val); - debug("%#x (%d)\n", *outp, *outp); - - return 0; + return of_read_u32_index(np, propname, 0, outp); } int of_read_u32_array(const struct device_node *np, const char *propname, diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index 5bc3b02996..b0be7cbe19 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -18,32 +18,13 @@ int ofnode_read_u32(ofnode node, const char *propname, u32 *outp) { - assert(ofnode_valid(node)); - debug("%s: %s: ", __func__, propname); - - if (ofnode_is_np(node)) { - return of_read_u32(ofnode_to_np(node), propname, outp); - } else { - const fdt32_t *cell; - int len; - - cell = fdt_getprop(gd->fdt_blob, ofnode_to_offset(node), - propname, &len); - if (!cell || len < sizeof(int)) { - debug("(not found)\n"); - return -EINVAL; - } - *outp = fdt32_to_cpu(cell[0]); - } - debug("%#x (%d)\n", *outp, *outp); - - return 0; + return ofnode_read_u32_index(node, propname, 0, outp); } u32 ofnode_read_u32_default(ofnode node, const char *propname, u32 def) { assert(ofnode_valid(node)); - ofnode_read_u32(node, propname, &def); + ofnode_read_u32_index(node, propname, 0, &def); return def; }
Now reading a 32 bit value from a device-tree property can be expressed as reading the first element of an array with a single value. Signed-off-by: Dario Binacchi <dariobin@libero.it> --- drivers/core/of_access.c | 16 +--------------- drivers/core/ofnode.c | 23 ++--------------------- 2 files changed, 3 insertions(+), 36 deletions(-)