| Message ID | 20220614132132.22329-1-stefan.herbrechtsmeier-oss@weidmueller.com |
|---|---|
| State | Accepted |
| Commit | b471bdc47b2acabe0b5fcb0fb40a0b1c0602d3b3 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/2] dm: core: Add functions to read 8/16-bit integers | expand |
On 6/14/22 15:21, Stefan Herbrechtsmeier wrote: > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > Add functions to read 8/16-bit integers like the existing functions for > 32/64-bit to simplify read of 8/16-bit integers from device tree > properties. > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> Reviewed-by: Marek Vasut <marex@denx.de>
Hi Stefan, On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > Add functions to read 8/16-bit integers like the existing functions for > 32/64-bit to simplify read of 8/16-bit integers from device tree > properties. > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > --- > > arch/sandbox/dts/test.dts | 2 ++ > drivers/core/of_access.c | 38 +++++++++++++++++++++++ > drivers/core/ofnode.c | 62 +++++++++++++++++++++++++++++++++++++ > drivers/core/read.c | 21 +++++++++++++ > include/dm/of_access.h | 32 +++++++++++++++++++ > include/dm/ofnode.h | 40 ++++++++++++++++++++++++ > include/dm/read.h | 65 +++++++++++++++++++++++++++++++++++++++ > test/dm/test-fdt.c | 19 ++++++++++++ > 8 files changed, 279 insertions(+) This looks good but is very expensive in terms of code size. Can you update your u8 and u16 functions to reuse the existing u32 function and just convert the value? Regards, Simon
Hi Simon, Am 12.07.2022 um 12:58 schrieb Simon Glass: > Hi Stefan, > > On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier > <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >> >> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >> >> Add functions to read 8/16-bit integers like the existing functions for >> 32/64-bit to simplify read of 8/16-bit integers from device tree >> properties. >> >> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >> --- >> >> arch/sandbox/dts/test.dts | 2 ++ >> drivers/core/of_access.c | 38 +++++++++++++++++++++++ >> drivers/core/ofnode.c | 62 +++++++++++++++++++++++++++++++++++++ >> drivers/core/read.c | 21 +++++++++++++ >> include/dm/of_access.h | 32 +++++++++++++++++++ >> include/dm/ofnode.h | 40 ++++++++++++++++++++++++ >> include/dm/read.h | 65 +++++++++++++++++++++++++++++++++++++++ >> test/dm/test-fdt.c | 19 ++++++++++++ >> 8 files changed, 279 insertions(+) > > This looks good but is very expensive in terms of code size. Can you > update your u8 and u16 functions to reuse the existing u32 function > and just convert the value? The u32 function requires a 32 bit value inside the device tree because it checks the size and maybe swap the bytes. The u8 and u16 function requires only a 8 and 16 bit value inside the device tree. Regards Stefan
Hi Stefan, On Tue, 12 Jul 2022 at 12:31, Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > > Hi Simon, > > Am 12.07.2022 um 12:58 schrieb Simon Glass: > > Hi Stefan, > > > > On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier > > <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > >> > >> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > >> > >> Add functions to read 8/16-bit integers like the existing functions for > >> 32/64-bit to simplify read of 8/16-bit integers from device tree > >> properties. > >> > >> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > >> --- > >> > >> arch/sandbox/dts/test.dts | 2 ++ > >> drivers/core/of_access.c | 38 +++++++++++++++++++++++ > >> drivers/core/ofnode.c | 62 +++++++++++++++++++++++++++++++++++++ > >> drivers/core/read.c | 21 +++++++++++++ > >> include/dm/of_access.h | 32 +++++++++++++++++++ > >> include/dm/ofnode.h | 40 ++++++++++++++++++++++++ > >> include/dm/read.h | 65 +++++++++++++++++++++++++++++++++++++++ > >> test/dm/test-fdt.c | 19 ++++++++++++ > >> 8 files changed, 279 insertions(+) > > > > This looks good but is very expensive in terms of code size. Can you > > update your u8 and u16 functions to reuse the existing u32 function > > and just convert the value? > > The u32 function requires a 32 bit value inside the device tree because > it checks the size and maybe swap the bytes. > > The u8 and u16 function requires only a 8 and 16 bit value inside the > device tree. Yes that's true. What is the use case for these functions? Reviewed-by: Simon Glass <sjg@chromium.org> Regards, Simon
Hi Simon, Am 13.07.2022 um 17:28 schrieb Simon Glass: > Hi Stefan, > > On Tue, 12 Jul 2022 at 12:31, Stefan Herbrechtsmeier > <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >> >> Hi Simon, >> >> Am 12.07.2022 um 12:58 schrieb Simon Glass: >>> Hi Stefan, >>> >>> On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier >>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>>> >>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >>>> >>>> Add functions to read 8/16-bit integers like the existing functions for >>>> 32/64-bit to simplify read of 8/16-bit integers from device tree >>>> properties. >>>> >>>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >>>> --- >>>> >>>> arch/sandbox/dts/test.dts | 2 ++ >>>> drivers/core/of_access.c | 38 +++++++++++++++++++++++ >>>> drivers/core/ofnode.c | 62 +++++++++++++++++++++++++++++++++++++ >>>> drivers/core/read.c | 21 +++++++++++++ >>>> include/dm/of_access.h | 32 +++++++++++++++++++ >>>> include/dm/ofnode.h | 40 ++++++++++++++++++++++++ >>>> include/dm/read.h | 65 +++++++++++++++++++++++++++++++++++++++ >>>> test/dm/test-fdt.c | 19 ++++++++++++ >>>> 8 files changed, 279 insertions(+) >>> >>> This looks good but is very expensive in terms of code size. Can you >>> update your u8 and u16 functions to reuse the existing u32 function >>> and just convert the value? >> >> The u32 function requires a 32 bit value inside the device tree because >> it checks the size and maybe swap the bytes. >> >> The u8 and u16 function requires only a 8 and 16 bit value inside the >> device tree. > > Yes that's true. What is the use case for these functions? The usb251xb driver requires this functions [1]. The usb251xb device tree binding [2] defines the ids as 16 bit values and the Linux driver use 8 bit for an unspecified property. Without this changes the driver doesn't satisfy the specification and is incompatible to the Linux driver. [1] https://lists.denx.de/pipermail/u-boot/2022-June/486424.html [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/usb/usb251xb.txt Regards Stefan
Hi Stefan, On Wed, 13 Jul 2022 at 10:08, Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > > Hi Simon, > > Am 13.07.2022 um 17:28 schrieb Simon Glass: > > Hi Stefan, > > > > On Tue, 12 Jul 2022 at 12:31, Stefan Herbrechtsmeier > > <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > >> > >> Hi Simon, > >> > >> Am 12.07.2022 um 12:58 schrieb Simon Glass: > >>> Hi Stefan, > >>> > >>> On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier > >>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > >>>> > >>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > >>>> > >>>> Add functions to read 8/16-bit integers like the existing functions for > >>>> 32/64-bit to simplify read of 8/16-bit integers from device tree > >>>> properties. > >>>> > >>>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > >>>> --- > >>>> > >>>> arch/sandbox/dts/test.dts | 2 ++ > >>>> drivers/core/of_access.c | 38 +++++++++++++++++++++++ > >>>> drivers/core/ofnode.c | 62 +++++++++++++++++++++++++++++++++++++ > >>>> drivers/core/read.c | 21 +++++++++++++ > >>>> include/dm/of_access.h | 32 +++++++++++++++++++ > >>>> include/dm/ofnode.h | 40 ++++++++++++++++++++++++ > >>>> include/dm/read.h | 65 +++++++++++++++++++++++++++++++++++++++ > >>>> test/dm/test-fdt.c | 19 ++++++++++++ > >>>> 8 files changed, 279 insertions(+) > >>> > >>> This looks good but is very expensive in terms of code size. Can you > >>> update your u8 and u16 functions to reuse the existing u32 function > >>> and just convert the value? > >> > >> The u32 function requires a 32 bit value inside the device tree because > >> it checks the size and maybe swap the bytes. > >> > >> The u8 and u16 function requires only a 8 and 16 bit value inside the > >> device tree. > > > > Yes that's true. What is the use case for these functions? > > The usb251xb driver requires this functions [1]. The usb251xb device > tree binding [2] defines the ids as 16 bit values and the Linux driver > use 8 bit for an unspecified property. Without this changes the driver > doesn't satisfy the specification and is incompatible to the Linux driver. I wonder if that binding is a bit ambiguous. From what I have seen we normally use a single cell for int values, partly so that fdtdump works and partly because the format doesn't allow using any less space anyway. IMO that binding should use a whole cell for the byte and u16 values. Then we can use the u32() function and do a mask. Regards, Simon > > [1] https://lists.denx.de/pipermail/u-boot/2022-June/486424.html > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/usb/usb251xb.txt > > Regards > Stefan
Hi Simon, Am 14.07.2022 um 12:22 schrieb Simon Glass: > Hi Stefan, > > On Wed, 13 Jul 2022 at 10:08, Stefan Herbrechtsmeier > <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >> >> Hi Simon, >> >> Am 13.07.2022 um 17:28 schrieb Simon Glass: >>> Hi Stefan, >>> >>> On Tue, 12 Jul 2022 at 12:31, Stefan Herbrechtsmeier >>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>>> >>>> Hi Simon, >>>> >>>> Am 12.07.2022 um 12:58 schrieb Simon Glass: >>>>> Hi Stefan, >>>>> >>>>> On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier >>>>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>>>>> >>>>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >>>>>> >>>>>> Add functions to read 8/16-bit integers like the existing functions for >>>>>> 32/64-bit to simplify read of 8/16-bit integers from device tree >>>>>> properties. >>>>>> >>>>>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >>>>>> --- >>>>>> >>>>>> arch/sandbox/dts/test.dts | 2 ++ >>>>>> drivers/core/of_access.c | 38 +++++++++++++++++++++++ >>>>>> drivers/core/ofnode.c | 62 +++++++++++++++++++++++++++++++++++++ >>>>>> drivers/core/read.c | 21 +++++++++++++ >>>>>> include/dm/of_access.h | 32 +++++++++++++++++++ >>>>>> include/dm/ofnode.h | 40 ++++++++++++++++++++++++ >>>>>> include/dm/read.h | 65 +++++++++++++++++++++++++++++++++++++++ >>>>>> test/dm/test-fdt.c | 19 ++++++++++++ >>>>>> 8 files changed, 279 insertions(+) >>>>> >>>>> This looks good but is very expensive in terms of code size. Can you >>>>> update your u8 and u16 functions to reuse the existing u32 function >>>>> and just convert the value? >>>> >>>> The u32 function requires a 32 bit value inside the device tree because >>>> it checks the size and maybe swap the bytes. >>>> >>>> The u8 and u16 function requires only a 8 and 16 bit value inside the >>>> device tree. >>> >>> Yes that's true. What is the use case for these functions? >> >> The usb251xb driver requires this functions [1]. The usb251xb device >> tree binding [2] defines the ids as 16 bit values and the Linux driver >> use 8 bit for an unspecified property. Without this changes the driver >> doesn't satisfy the specification and is incompatible to the Linux driver. > > I wonder if that binding is a bit ambiguous. From what I have seen we > normally use a single cell for int values, partly so that fdtdump > works and partly because the format doesn't allow using any less space > anyway. > > IMO that binding should use a whole cell for the byte and u16 values. How should we go on? The specification is 5 years old. I can ignore the specification and remove the "/bits/ 16" from my device tree source. > Then we can use the u32() function and do a mask. The driver cast away the unseeded bits. >> >> [1] https://lists.denx.de/pipermail/u-boot/2022-June/486424.html >> [2] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/usb/usb251xb.txt Stefan Regards Stefan
On 7/14/22 12:37, Stefan Herbrechtsmeier wrote: > Hi Simon, > > Am 14.07.2022 um 12:22 schrieb Simon Glass: >> Hi Stefan, >> >> On Wed, 13 Jul 2022 at 10:08, Stefan Herbrechtsmeier >> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>> >>> Hi Simon, >>> >>> Am 13.07.2022 um 17:28 schrieb Simon Glass: >>>> Hi Stefan, >>>> >>>> On Tue, 12 Jul 2022 at 12:31, Stefan Herbrechtsmeier >>>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>>>> >>>>> Hi Simon, >>>>> >>>>> Am 12.07.2022 um 12:58 schrieb Simon Glass: >>>>>> Hi Stefan, >>>>>> >>>>>> On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier >>>>>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>>>>>> >>>>>>> From: Stefan Herbrechtsmeier >>>>>>> <stefan.herbrechtsmeier@weidmueller.com> >>>>>>> >>>>>>> Add functions to read 8/16-bit integers like the existing >>>>>>> functions for >>>>>>> 32/64-bit to simplify read of 8/16-bit integers from device tree >>>>>>> properties. >>>>>>> >>>>>>> Signed-off-by: Stefan Herbrechtsmeier >>>>>>> <stefan.herbrechtsmeier@weidmueller.com> >>>>>>> --- >>>>>>> >>>>>>> arch/sandbox/dts/test.dts | 2 ++ >>>>>>> drivers/core/of_access.c | 38 +++++++++++++++++++++++ >>>>>>> drivers/core/ofnode.c | 62 >>>>>>> +++++++++++++++++++++++++++++++++++++ >>>>>>> drivers/core/read.c | 21 +++++++++++++ >>>>>>> include/dm/of_access.h | 32 +++++++++++++++++++ >>>>>>> include/dm/ofnode.h | 40 ++++++++++++++++++++++++ >>>>>>> include/dm/read.h | 65 >>>>>>> +++++++++++++++++++++++++++++++++++++++ >>>>>>> test/dm/test-fdt.c | 19 ++++++++++++ >>>>>>> 8 files changed, 279 insertions(+) >>>>>> >>>>>> This looks good but is very expensive in terms of code size. Can you >>>>>> update your u8 and u16 functions to reuse the existing u32 function >>>>>> and just convert the value? >>>>> >>>>> The u32 function requires a 32 bit value inside the device tree >>>>> because >>>>> it checks the size and maybe swap the bytes. >>>>> >>>>> The u8 and u16 function requires only a 8 and 16 bit value inside the >>>>> device tree. >>>> >>>> Yes that's true. What is the use case for these functions? >>> >>> The usb251xb driver requires this functions [1]. The usb251xb device >>> tree binding [2] defines the ids as 16 bit values and the Linux driver >>> use 8 bit for an unspecified property. Without this changes the driver >>> doesn't satisfy the specification and is incompatible to the Linux >>> driver. >> >> I wonder if that binding is a bit ambiguous. From what I have seen we >> normally use a single cell for int values, partly so that fdtdump >> works and partly because the format doesn't allow using any less space >> anyway. >> >> IMO that binding should use a whole cell for the byte and u16 values. > > How should we go on? The specification is 5 years old. I can ignore the > specification and remove the "/bits/ 16" from my device tree source. We will not change the binding due to a deficiency of U-Boot. For the 8 bit field: The length field is set to 1 byte. 3 zero bytes are following for alignment. We tend to check the length field. We could reduce the number functions if on most levels of function indirection we would pass a size field. E.g. replace of_read_u*_array() by macros all invoking the same function with different values (1, 2, 4, 8) of the element size parameter. But that rework can be done after this patch series. Anyway on most boards the linker will eliminate the new functions as unused. Best regards Heinrich > >> Then we can use the u32() function and do a mask. > > The driver cast away the unseeded bits. > >>> >>> [1] https://lists.denx.de/pipermail/u-boot/2022-June/486424.html >>> [2] >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/usb/usb251xb.txt > Stefan > > Regards > Stefan
Hi Stefan, On Thu, 14 Jul 2022 at 04:37, Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > > Hi Simon, > > Am 14.07.2022 um 12:22 schrieb Simon Glass: > > Hi Stefan, > > > > On Wed, 13 Jul 2022 at 10:08, Stefan Herbrechtsmeier > > <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > >> > >> Hi Simon, > >> > >> Am 13.07.2022 um 17:28 schrieb Simon Glass: > >>> Hi Stefan, > >>> > >>> On Tue, 12 Jul 2022 at 12:31, Stefan Herbrechtsmeier > >>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > >>>> > >>>> Hi Simon, > >>>> > >>>> Am 12.07.2022 um 12:58 schrieb Simon Glass: > >>>>> Hi Stefan, > >>>>> > >>>>> On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier > >>>>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > >>>>>> > >>>>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > >>>>>> > >>>>>> Add functions to read 8/16-bit integers like the existing functions for > >>>>>> 32/64-bit to simplify read of 8/16-bit integers from device tree > >>>>>> properties. > >>>>>> > >>>>>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > >>>>>> --- > >>>>>> > >>>>>> arch/sandbox/dts/test.dts | 2 ++ > >>>>>> drivers/core/of_access.c | 38 +++++++++++++++++++++++ > >>>>>> drivers/core/ofnode.c | 62 +++++++++++++++++++++++++++++++++++++ > >>>>>> drivers/core/read.c | 21 +++++++++++++ > >>>>>> include/dm/of_access.h | 32 +++++++++++++++++++ > >>>>>> include/dm/ofnode.h | 40 ++++++++++++++++++++++++ > >>>>>> include/dm/read.h | 65 +++++++++++++++++++++++++++++++++++++++ > >>>>>> test/dm/test-fdt.c | 19 ++++++++++++ > >>>>>> 8 files changed, 279 insertions(+) > >>>>> > >>>>> This looks good but is very expensive in terms of code size. Can you > >>>>> update your u8 and u16 functions to reuse the existing u32 function > >>>>> and just convert the value? > >>>> > >>>> The u32 function requires a 32 bit value inside the device tree because > >>>> it checks the size and maybe swap the bytes. > >>>> > >>>> The u8 and u16 function requires only a 8 and 16 bit value inside the > >>>> device tree. > >>> > >>> Yes that's true. What is the use case for these functions? > >> > >> The usb251xb driver requires this functions [1]. The usb251xb device > >> tree binding [2] defines the ids as 16 bit values and the Linux driver > >> use 8 bit for an unspecified property. Without this changes the driver > >> doesn't satisfy the specification and is incompatible to the Linux driver. > > > > I wonder if that binding is a bit ambiguous. From what I have seen we > > normally use a single cell for int values, partly so that fdtdump > > works and partly because the format doesn't allow using any less space > > anyway. > > > > IMO that binding should use a whole cell for the byte and u16 values. > > How should we go on? The specification is 5 years old. I can ignore the > specification and remove the "/bits/ 16" from my device tree source. I believe either would work, since reading a u32 from the device tree and masking it would achieve the same result. The problem with the current u32 function is that it requires 4 bytes. IMO the binding is odd, but I don't think you can just change it. > > > Then we can use the u32() function and do a mask. > > The driver cast away the unseeded bits. OK. Anyway as Heinrich says below, most boards won't use these functions. I hope that other drivers don't start using u8 and u16 values when a u32 will do. Reviewed-by: Simon Glass <sjg@chromium.org> > > >> > >> [1] https://lists.denx.de/pipermail/u-boot/2022-June/486424.html > >> [2] > >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/usb/usb251xb.txt > Stefan > Regards, Simon
Hi, Am 14.07.2022 um 16:51 schrieb Simon Glass: > Hi Stefan, > > On Thu, 14 Jul 2022 at 04:37, Stefan Herbrechtsmeier > <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >> >> Hi Simon, >> >> Am 14.07.2022 um 12:22 schrieb Simon Glass: >>> Hi Stefan, >>> >>> On Wed, 13 Jul 2022 at 10:08, Stefan Herbrechtsmeier >>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>>> >>>> Hi Simon, >>>> >>>> Am 13.07.2022 um 17:28 schrieb Simon Glass: >>>>> Hi Stefan, >>>>> >>>>> On Tue, 12 Jul 2022 at 12:31, Stefan Herbrechtsmeier >>>>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>>>>> >>>>>> Hi Simon, >>>>>> >>>>>> Am 12.07.2022 um 12:58 schrieb Simon Glass: >>>>>>> Hi Stefan, >>>>>>> >>>>>>> On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier >>>>>>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>>>>>>> >>>>>>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >>>>>>>> >>>>>>>> Add functions to read 8/16-bit integers like the existing functions for >>>>>>>> 32/64-bit to simplify read of 8/16-bit integers from device tree >>>>>>>> properties. >>>>>>>> >>>>>>>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >>>>>>>> --- >>>>>>>> >>>>>>>> arch/sandbox/dts/test.dts | 2 ++ >>>>>>>> drivers/core/of_access.c | 38 +++++++++++++++++++++++ >>>>>>>> drivers/core/ofnode.c | 62 +++++++++++++++++++++++++++++++++++++ >>>>>>>> drivers/core/read.c | 21 +++++++++++++ >>>>>>>> include/dm/of_access.h | 32 +++++++++++++++++++ >>>>>>>> include/dm/ofnode.h | 40 ++++++++++++++++++++++++ >>>>>>>> include/dm/read.h | 65 +++++++++++++++++++++++++++++++++++++++ >>>>>>>> test/dm/test-fdt.c | 19 ++++++++++++ >>>>>>>> 8 files changed, 279 insertions(+) >>>>>>> >>>>>>> This looks good but is very expensive in terms of code size. Can you >>>>>>> update your u8 and u16 functions to reuse the existing u32 function >>>>>>> and just convert the value? >>>>>> >>>>>> The u32 function requires a 32 bit value inside the device tree because >>>>>> it checks the size and maybe swap the bytes. >>>>>> >>>>>> The u8 and u16 function requires only a 8 and 16 bit value inside the >>>>>> device tree. >>>>> >>>>> Yes that's true. What is the use case for these functions? >>>> >>>> The usb251xb driver requires this functions [1]. The usb251xb device >>>> tree binding [2] defines the ids as 16 bit values and the Linux driver >>>> use 8 bit for an unspecified property. Without this changes the driver >>>> doesn't satisfy the specification and is incompatible to the Linux driver. >>> >>> I wonder if that binding is a bit ambiguous. From what I have seen we >>> normally use a single cell for int values, partly so that fdtdump >>> works and partly because the format doesn't allow using any less space >>> anyway. >>> >>> IMO that binding should use a whole cell for the byte and u16 values. >> >> How should we go on? The specification is 5 years old. I can ignore the >> specification and remove the "/bits/ 16" from my device tree source. > > I believe either would work, since reading a u32 from the device tree > and masking it would achieve the same result. The problem with the > current u32 function is that it requires 4 bytes. > > IMO the binding is odd, but I don't think you can just change it. > >> >>> Then we can use the u32() function and do a mask. >> >> The driver cast away the unseeded bits. > > OK. > > Anyway as Heinrich says below, most boards won't use these functions. > I hope that other drivers don't start using u8 and u16 values when a > u32 will do. > > Reviewed-by: Simon Glass <sjg@chromium.org> > >> >>>> >>>> [1] https://lists.denx.de/pipermail/u-boot/2022-June/486424.html >>>> [2] >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/usb/usb251xb.txt >> Stefan >> > > Regards, > Simon Does this series need a rework to be applied? Regards Stefan
Hi Heinrich, Am 14.07.2022 um 14:58 schrieb Heinrich Schuchardt: > On 7/14/22 12:37, Stefan Herbrechtsmeier wrote: >> Hi Simon, >> >> Am 14.07.2022 um 12:22 schrieb Simon Glass: >>> Hi Stefan, >>> >>> On Wed, 13 Jul 2022 at 10:08, Stefan Herbrechtsmeier >>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>>> >>>> Hi Simon, >>>> >>>> Am 13.07.2022 um 17:28 schrieb Simon Glass: >>>>> Hi Stefan, >>>>> >>>>> On Tue, 12 Jul 2022 at 12:31, Stefan Herbrechtsmeier >>>>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>>>>> >>>>>> Hi Simon, >>>>>> >>>>>> Am 12.07.2022 um 12:58 schrieb Simon Glass: >>>>>>> Hi Stefan, >>>>>>> >>>>>>> On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier >>>>>>> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >>>>>>>> >>>>>>>> From: Stefan Herbrechtsmeier >>>>>>>> <stefan.herbrechtsmeier@weidmueller.com> >>>>>>>> >>>>>>>> Add functions to read 8/16-bit integers like the existing >>>>>>>> functions for >>>>>>>> 32/64-bit to simplify read of 8/16-bit integers from device tree >>>>>>>> properties. >>>>>>>> >>>>>>>> Signed-off-by: Stefan Herbrechtsmeier >>>>>>>> <stefan.herbrechtsmeier@weidmueller.com> >>>>>>>> --- >>>>>>>> >>>>>>>> arch/sandbox/dts/test.dts | 2 ++ >>>>>>>> drivers/core/of_access.c | 38 +++++++++++++++++++++++ >>>>>>>> drivers/core/ofnode.c | 62 >>>>>>>> +++++++++++++++++++++++++++++++++++++ >>>>>>>> drivers/core/read.c | 21 +++++++++++++ >>>>>>>> include/dm/of_access.h | 32 +++++++++++++++++++ >>>>>>>> include/dm/ofnode.h | 40 ++++++++++++++++++++++++ >>>>>>>> include/dm/read.h | 65 >>>>>>>> +++++++++++++++++++++++++++++++++++++++ >>>>>>>> test/dm/test-fdt.c | 19 ++++++++++++ >>>>>>>> 8 files changed, 279 insertions(+) >>>>>>> >>>>>>> This looks good but is very expensive in terms of code size. Can you >>>>>>> update your u8 and u16 functions to reuse the existing u32 function >>>>>>> and just convert the value? >>>>>> >>>>>> The u32 function requires a 32 bit value inside the device tree >>>>>> because >>>>>> it checks the size and maybe swap the bytes. >>>>>> >>>>>> The u8 and u16 function requires only a 8 and 16 bit value inside the >>>>>> device tree. >>>>> >>>>> Yes that's true. What is the use case for these functions? >>>> >>>> The usb251xb driver requires this functions [1]. The usb251xb device >>>> tree binding [2] defines the ids as 16 bit values and the Linux driver >>>> use 8 bit for an unspecified property. Without this changes the driver >>>> doesn't satisfy the specification and is incompatible to the Linux >>>> driver. >>> >>> I wonder if that binding is a bit ambiguous. From what I have seen we >>> normally use a single cell for int values, partly so that fdtdump >>> works and partly because the format doesn't allow using any less space >>> anyway. >>> >>> IMO that binding should use a whole cell for the byte and u16 values. >> >> How should we go on? The specification is 5 years old. I can ignore the >> specification and remove the "/bits/ 16" from my device tree source. > > We will not change the binding due to a deficiency of U-Boot. > > For the 8 bit field: > > The length field is set to 1 byte. > 3 zero bytes are following for alignment. > > We tend to check the length field. > > We could reduce the number functions if on most levels of function > indirection we would pass a size field. > > E.g. replace of_read_u*_array() by macros all invoking the same function > with different values (1, 2, 4, 8) of the element size parameter. This doesn't help because you have to adapt the byte order and on most levels the functions are simple wrappers. The rework isn't easy because we have a combination of wrappers and duplicate implementations. Thereby the ofnode functions are wrappers and implementations at the same time: dev_read_u32 -> ofnode_read_u32 -> ofnode_read_u32_index - if ofnode_is_np -> of_read_u32_index -> of_find_property_value_of_size -> be32_to_cpup - else -> fdt_getprop -> fdt32_to_cpu We need to remove the second implementations (of_read_) to reduce the size. Regards Stefan
On Tue, Jun 14, 2022 at 03:21:30PM +0200, Stefan Herbrechtsmeier wrote: > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > Add functions to read 8/16-bit integers like the existing functions for > 32/64-bit to simplify read of 8/16-bit integers from device tree > properties. > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > Reviewed-by: Marek Vasut <marex@denx.de> > Reviewed-by: Simon Glass <sjg@chromium.org> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/next, thanks!
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 8f93775ff4..035f5edfa2 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -226,6 +226,8 @@ test5-gpios = <&gpio_a 19>; bool-value; + int8-value = /bits/ 8 <0x12>; + int16-value = /bits/ 16 <0x1234>; int-value = <1234>; uint-value = <(-1234)>; int64-value = /bits/ 64 <0x1111222233334444>; diff --git a/drivers/core/of_access.c b/drivers/core/of_access.c index c20b19cb50..0efcde3ba5 100644 --- a/drivers/core/of_access.c +++ b/drivers/core/of_access.c @@ -482,6 +482,44 @@ static void *of_find_property_value_of_size(const struct device_node *np, return prop->value; } +int of_read_u8(const struct device_node *np, const char *propname, u8 *outp) +{ + const u8 *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 = *val; + debug("%#x (%d)\n", *outp, *outp); + + return 0; +} + +int of_read_u16(const struct device_node *np, const char *propname, u16 *outp) +{ + const __be16 *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 = be16_to_cpup(val); + debug("%#x (%d)\n", *outp, *outp); + + return 0; +} + int of_read_u32(const struct device_node *np, const char *propname, u32 *outp) { return of_read_u32_index(np, propname, 0, outp); diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index a59832ebbf..ce411b7c35 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -31,6 +31,68 @@ bool ofnode_name_eq(ofnode node, const char *name) return (strlen(name) == len) && !strncmp(node_name, name, len); } +int ofnode_read_u8(ofnode node, const char *propname, u8 *outp) +{ + const u8 *cell; + int len; + + assert(ofnode_valid(node)); + debug("%s: %s: ", __func__, propname); + + if (ofnode_is_np(node)) + return of_read_u8(ofnode_to_np(node), propname, outp); + + cell = fdt_getprop(gd->fdt_blob, ofnode_to_offset(node), propname, + &len); + if (!cell || len < sizeof(*cell)) { + debug("(not found)\n"); + return -EINVAL; + } + *outp = *cell; + debug("%#x (%d)\n", *outp, *outp); + + return 0; +} + +u8 ofnode_read_u8_default(ofnode node, const char *propname, u8 def) +{ + assert(ofnode_valid(node)); + ofnode_read_u8(node, propname, &def); + + return def; +} + +int ofnode_read_u16(ofnode node, const char *propname, u16 *outp) +{ + const fdt16_t *cell; + int len; + + assert(ofnode_valid(node)); + debug("%s: %s: ", __func__, propname); + + if (ofnode_is_np(node)) + return of_read_u16(ofnode_to_np(node), propname, outp); + + cell = fdt_getprop(gd->fdt_blob, ofnode_to_offset(node), propname, + &len); + if (!cell || len < sizeof(*cell)) { + debug("(not found)\n"); + return -EINVAL; + } + *outp = be16_to_cpup(cell); + debug("%#x (%d)\n", *outp, *outp); + + return 0; +} + +u16 ofnode_read_u16_default(ofnode node, const char *propname, u16 def) +{ + assert(ofnode_valid(node)); + ofnode_read_u16(node, propname, &def); + + return def; +} + int ofnode_read_u32(ofnode node, const char *propname, u32 *outp) { return ofnode_read_u32_index(node, propname, 0, outp); diff --git a/drivers/core/read.c b/drivers/core/read.c index c73508d276..07ab8ab41c 100644 --- a/drivers/core/read.c +++ b/drivers/core/read.c @@ -13,6 +13,27 @@ #include <asm/io.h> #include <linux/ioport.h> +int dev_read_u8(const struct udevice *dev, const char *propname, u8 *outp) +{ + return ofnode_read_u8(dev_ofnode(dev), propname, outp); +} + +u8 dev_read_u8_default(const struct udevice *dev, const char *propname, u8 def) +{ + return ofnode_read_u8_default(dev_ofnode(dev), propname, def); +} + +int dev_read_u16(const struct udevice *dev, const char *propname, u16 *outp) +{ + return ofnode_read_u16(dev_ofnode(dev), propname, outp); +} + +u16 dev_read_u16_default(const struct udevice *dev, const char *propname, + u16 def) +{ + return ofnode_read_u16_default(dev_ofnode(dev), propname, def); +} + int dev_read_u32(const struct udevice *dev, const char *propname, u32 *outp) { return ofnode_read_u32(dev_ofnode(dev), propname, outp); diff --git a/include/dm/of_access.h b/include/dm/of_access.h index ec6e6e2c7c..ec9603bc58 100644 --- a/include/dm/of_access.h +++ b/include/dm/of_access.h @@ -258,6 +258,38 @@ struct device_node *of_find_node_by_prop_value(struct device_node *from, */ struct device_node *of_find_node_by_phandle(phandle handle); +/** + * of_read_u8() - Find and read a 8-bit integer from a property + * + * Search for a property in a device node and read a 8-bit value from + * it. + * + * @np: device node from which the property value is to be read. + * @propname: name of the property to be searched. + * @outp: pointer to return value, modified only if return value is 0. + * + * Return: 0 on success, -EINVAL if the property does not exist, + * -ENODATA if property does not have a value, and -EOVERFLOW if the + * property data isn't large enough. + */ +int of_read_u8(const struct device_node *np, const char *propname, u8 *outp); + +/** + * of_read_u16() - Find and read a 16-bit integer from a property + * + * Search for a property in a device node and read a 16-bit value from + * it. + * + * @np: device node from which the property value is to be read. + * @propname: name of the property to be searched. + * @outp: pointer to return value, modified only if return value is 0. + * + * Return: 0 on success, -EINVAL if the property does not exist, + * -ENODATA if property does not have a value, and -EOVERFLOW if the + * property data isn't large enough. + */ +int of_read_u16(const struct device_node *np, const char *propname, u16 *outp); + /** * of_read_u32() - Find and read a 32-bit integer from a property * diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h index 2c4d72d77f..5a41affba1 100644 --- a/include/dm/ofnode.h +++ b/include/dm/ofnode.h @@ -245,6 +245,46 @@ static inline ofnode ofnode_root(void) */ bool ofnode_name_eq(ofnode node, const char *name); +/** + * ofnode_read_u8() - Read a 8-bit integer from a property + * + * @node: valid node reference to read property from + * @propname: name of the property to read from + * @outp: place to put value (if found) + * Return: 0 if OK, -ve on error + */ +int ofnode_read_u8(ofnode node, const char *propname, u8 *outp); + +/** + * ofnode_read_u8_default() - Read a 8-bit integer from a property + * + * @node: valid node reference to read property from + * @propname: name of the property to read from + * @def: default value to return if the property has no value + * Return: property value, or @def if not found + */ +u8 ofnode_read_u8_default(ofnode node, const char *propname, u8 def); + +/** + * ofnode_read_u16() - Read a 16-bit integer from a property + * + * @node: valid node reference to read property from + * @propname: name of the property to read from + * @outp: place to put value (if found) + * Return: 0 if OK, -ve on error + */ +int ofnode_read_u16(ofnode node, const char *propname, u16 *outp); + +/** + * ofnode_read_u16_default() - Read a 16-bit integer from a property + * + * @node: valid node reference to read property from + * @propname: name of the property to read from + * @def: default value to return if the property has no value + * Return: property value, or @def if not found + */ +u16 ofnode_read_u16_default(ofnode node, const char *propname, u16 def); + /** * ofnode_read_u32() - Read a 32-bit integer from a property * diff --git a/include/dm/read.h b/include/dm/read.h index 1b54b69acf..122b9cd15b 100644 --- a/include/dm/read.h +++ b/include/dm/read.h @@ -31,6 +31,47 @@ static inline const struct device_node *dev_np(const struct udevice *dev) #endif #if !defined(CONFIG_DM_DEV_READ_INLINE) || CONFIG_IS_ENABLED(OF_PLATDATA) +/** + * dev_read_u8() - read a 8-bit integer from a device's DT property + * + * @dev: device to read DT property from + * @propname: name of the property to read from + * @outp: place to put value (if found) + * Return: 0 if OK, -ve on error + */ +int dev_read_u8(const struct udevice *dev, const char *propname, u8 *outp); + +/** + * dev_read_u8_default() - read a 8-bit integer from a device's DT property + * + * @dev: device to read DT property from + * @propname: name of the property to read from + * @def: default value to return if the property has no value + * Return: property value, or @def if not found + */ +u8 dev_read_u8_default(const struct udevice *dev, const char *propname, u8 def); + +/** + * dev_read_u16() - read a 16-bit integer from a device's DT property + * + * @dev: device to read DT property from + * @propname: name of the property to read from + * @outp: place to put value (if found) + * Return: 0 if OK, -ve on error + */ +int dev_read_u16(const struct udevice *dev, const char *propname, u16 *outp); + +/** + * dev_read_u16_default() - read a 16-bit integer from a device's DT property + * + * @dev: device to read DT property from + * @propname: name of the property to read from + * @def: default value to return if the property has no value + * Return: property value, or @def if not found + */ +u16 dev_read_u16_default(const struct udevice *dev, const char *propname, + u16 def); + /** * dev_read_u32() - read a 32-bit integer from a device's DT property * @@ -772,6 +813,30 @@ phy_interface_t dev_read_phy_mode(const struct udevice *dev); #else /* CONFIG_DM_DEV_READ_INLINE is enabled */ #include <asm/global_data.h> +static inline int dev_read_u8(const struct udevice *dev, + const char *propname, u8 *outp) +{ + return ofnode_read_u8(dev_ofnode(dev), propname, outp); +} + +static inline int dev_read_u8_default(const struct udevice *dev, + const char *propname, u8 def) +{ + return ofnode_read_u8_default(dev_ofnode(dev), propname, def); +} + +static inline int dev_read_u16(const struct udevice *dev, + const char *propname, u16 *outp) +{ + return ofnode_read_u16(dev_ofnode(dev), propname, outp); +} + +static inline int dev_read_u16_default(const struct udevice *dev, + const char *propname, u16 def) +{ + return ofnode_read_u16_default(dev_ofnode(dev), propname, def); +} + static inline int dev_read_u32(const struct udevice *dev, const char *propname, u32 *outp) { diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c index e1de066226..41385293df 100644 --- a/test/dm/test-fdt.c +++ b/test/dm/test-fdt.c @@ -868,6 +868,8 @@ DM_TEST(dm_test_first_child, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); static int dm_test_read_int(struct unit_test_state *uts) { struct udevice *dev; + u8 val8; + u16 val16; u32 val32; s32 sval; uint val; @@ -875,6 +877,23 @@ static int dm_test_read_int(struct unit_test_state *uts) ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev)); ut_asserteq_str("a-test", dev->name); + + ut_assertok(dev_read_u8(dev, "int8-value", &val8)); + ut_asserteq(0x12, val8); + + ut_asserteq(-EINVAL, dev_read_u8(dev, "missing", &val8)); + ut_asserteq(6, dev_read_u8_default(dev, "missing", 6)); + + ut_asserteq(0x12, dev_read_u8_default(dev, "int8-value", 6)); + + ut_assertok(dev_read_u16(dev, "int16-value", &val16)); + ut_asserteq(0x1234, val16); + + ut_asserteq(-EINVAL, dev_read_u16(dev, "missing", &val16)); + ut_asserteq(6, dev_read_u16_default(dev, "missing", 6)); + + ut_asserteq(0x1234, dev_read_u16_default(dev, "int16-value", 6)); + ut_assertok(dev_read_u32(dev, "int-value", &val32)); ut_asserteq(1234, val32);