| Message ID | 20170929125238.26226-31-mario.six@gdsys.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Wolfgang Denk |
| Headers | show |
| Series | [U-Boot,001/080] mpc8308rdb: Fix style violation | expand |
Hi Mario, On 29 September 2017 at 06:51, Mario Six <mario.six@gdsys.cc> wrote: > The dev_read_addr function does not do any bus translations, and just > returns the raw address read from the device tree, which makes the > driver not work on systems that need bus translations to get the actual > memory address of the device's register space. Aside from any current functionality, what is the correct thing for dev_read_addr() to do? I worry that the two parts (live/flat tree) might do different things. In any case, we should not compound the problem if dev_read_addr() is wrong. > > Since the dev_read_addr function is widely used, we refrain from > modifying it, and instead read the raw address from the device tree, and > apply the bus translations using the recently introduced > dev_translate_address function. > > Signed-off-by: Mario Six <mario.six@gdsys.cc> > --- > drivers/serial/ns16550.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) REgards, Simon
(adding Philipp since he converted lots of drivers to livetree recently) On Mon, Oct 9, 2017 at 6:48 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Mario, > > On 29 September 2017 at 06:51, Mario Six <mario.six@gdsys.cc> wrote: >> The dev_read_addr function does not do any bus translations, and just >> returns the raw address read from the device tree, which makes the >> driver not work on systems that need bus translations to get the actual >> memory address of the device's register space. > > Aside from any current functionality, what is the correct thing for > dev_read_addr() to do? I worry that the two parts (live/flat tree) > might do different things. > I took a closer look, and indeed, the two parts of dev_read_addr behave differently: dev_read_addr calls dev_read_addr_index, which calls either ofnode_get_addr_index when live tree is active, or devfdt_get_addr_index when it's not. devfdt_get_addr_index applies bus translations, but ofnode_get_addr_index returns the untranslated address using of_read_number (the else part doesn't run, since we have an active live tree if ofnode_get_addr_index was called). We could fix this by running of_translate_address on the value returned by of_read_number, so that dev_get_addr would then always return a translated address. But what I think is strange is that most live tree conversion patches (e.g. a9d3037 ("usb: dwc2: convert to livetree") or 4aac33f ("dm: mmc: fsl_esdhc: Update to support livetree")) simply replaced devfdt_get_addr (which does apply bus translations) with dev_read_addr (which does not apply bus translations in the live tree case). Shouldn't the converted driver have failed in the live tree case? Or were all drivers converted until now not depending on any bus translations? > In any case, we should not compound the problem if dev_read_addr() is wrong. > >> >> Since the dev_read_addr function is widely used, we refrain from >> modifying it, and instead read the raw address from the device tree, and >> apply the bus translations using the recently introduced >> dev_translate_address function. >> >> Signed-off-by: Mario Six <mario.six@gdsys.cc> >> --- >> drivers/serial/ns16550.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) > > REgards, > Simon Best regards, Mario
> On 9 Oct 2017, at 14:45, Mario Six <mario.six@gdsys.cc> wrote: > > (adding Philipp since he converted lots of drivers to livetree recently) > > On Mon, Oct 9, 2017 at 6:48 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Mario, >> >> On 29 September 2017 at 06:51, Mario Six <mario.six@gdsys.cc> wrote: >>> The dev_read_addr function does not do any bus translations, and just >>> returns the raw address read from the device tree, which makes the >>> driver not work on systems that need bus translations to get the actual >>> memory address of the device's register space. >> >> Aside from any current functionality, what is the correct thing for >> dev_read_addr() to do? I worry that the two parts (live/flat tree) >> might do different things. >> > > I took a closer look, and indeed, the two parts of dev_read_addr behave > differently: dev_read_addr calls dev_read_addr_index, which calls either > ofnode_get_addr_index when live tree is active, or devfdt_get_addr_index when > it's not. devfdt_get_addr_index applies bus translations, but > ofnode_get_addr_index returns the untranslated address using of_read_number > (the else part doesn't run, since we have an active live tree if > ofnode_get_addr_index was called). Good point. I would expect the livetree case to use translated addresses during runtime, as dev_read_addr_index calls either ofnode_get_addr_index or devfdt_get_addr_index. In other words: are we missing an address translation from ofnode_get_addr_index or should the address retrieved via ofnode_get_addr_index have been translated by earlier processing? > > We could fix this by running of_translate_address on the value returned by > of_read_number, so that dev_get_addr would then always return a translated > address. > > But what I think is strange is that most live tree conversion patches (e.g. > a9d3037 ("usb: dwc2: convert to livetree") or 4aac33f ("dm: mmc: fsl_esdhc: > Update to support livetree")) simply replaced devfdt_get_addr (which does apply > bus translations) with dev_read_addr (which does not apply bus translations in > the live tree case). Shouldn't the converted driver have failed in the live > tree case? Or were all drivers converted until now not depending on any bus > translations? > >> In any case, we should not compound the problem if dev_read_addr() is wrong. >> >>> >>> Since the dev_read_addr function is widely used, we refrain from >>> modifying it, and instead read the raw address from the device tree, and >>> apply the bus translations using the recently introduced >>> dev_translate_address function. >>> >>> Signed-off-by: Mario Six <mario.six@gdsys.cc> >>> --- >>> drivers/serial/ns16550.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> REgards, >> Simon > > Best regards, > > Mario
Hi, On 9 October 2017 at 06:55, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: > >> On 9 Oct 2017, at 14:45, Mario Six <mario.six@gdsys.cc> wrote: >> >> (adding Philipp since he converted lots of drivers to livetree recently) >> >> On Mon, Oct 9, 2017 at 6:48 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Mario, >>> >>> On 29 September 2017 at 06:51, Mario Six <mario.six@gdsys.cc> wrote: >>>> The dev_read_addr function does not do any bus translations, and just >>>> returns the raw address read from the device tree, which makes the >>>> driver not work on systems that need bus translations to get the actual >>>> memory address of the device's register space. >>> >>> Aside from any current functionality, what is the correct thing for >>> dev_read_addr() to do? I worry that the two parts (live/flat tree) >>> might do different things. >>> >> >> I took a closer look, and indeed, the two parts of dev_read_addr behave >> differently: dev_read_addr calls dev_read_addr_index, which calls either >> ofnode_get_addr_index when live tree is active, or devfdt_get_addr_index when >> it's not. devfdt_get_addr_index applies bus translations, but >> ofnode_get_addr_index returns the untranslated address using of_read_number >> (the else part doesn't run, since we have an active live tree if >> ofnode_get_addr_index was called). > > Good point. I would expect the livetree case to use translated addresses > during runtime, as dev_read_addr_index calls either ofnode_get_addr_index > or devfdt_get_addr_index. > > In other words: are we missing an address translation from ofnode_get_addr_index > or should the address retrieved via ofnode_get_addr_index have been > translated by earlier processing? Thank you both. Yes it seems like the right answer is to add the missing translation in. There is a CONFIG_OF_TRANSLATE which enables this. I think most boards don't use it, which is probably why there are no problems without it, but I think it is becoming more common as boards become more complex. > >> >> We could fix this by running of_translate_address on the value returned by >> of_read_number, so that dev_get_addr would then always return a translated >> address. >> >> But what I think is strange is that most live tree conversion patches (e.g. >> a9d3037 ("usb: dwc2: convert to livetree") or 4aac33f ("dm: mmc: fsl_esdhc: >> Update to support livetree")) simply replaced devfdt_get_addr (which does apply >> bus translations) with dev_read_addr (which does not apply bus translations in >> the live tree case). Shouldn't the converted driver have failed in the live >> tree case? Or were all drivers converted until now not depending on any bus >> translations? >> >>> In any case, we should not compound the problem if dev_read_addr() is wrong. >>> >>>> >>>> Since the dev_read_addr function is widely used, we refrain from >>>> modifying it, and instead read the raw address from the device tree, and >>>> apply the bus translations using the recently introduced >>>> dev_translate_address function. >>>> >>>> Signed-off-by: Mario Six <mario.six@gdsys.cc> >>>> --- >>>> drivers/serial/ns16550.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> REgards, >>> Simon >> >> Best regards, >> >> Mario > Regards, Simon
Hi, On Mon, Oct 9, 2017 at 4:09 PM, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On 9 October 2017 at 06:55, Dr. Philipp Tomsich > <philipp.tomsich@theobroma-systems.com> wrote: >> >>> On 9 Oct 2017, at 14:45, Mario Six <mario.six@gdsys.cc> wrote: >>> >>> (adding Philipp since he converted lots of drivers to livetree recently) >>> >>> On Mon, Oct 9, 2017 at 6:48 AM, Simon Glass <sjg@chromium.org> wrote: >>>> Hi Mario, >>>> >>>> On 29 September 2017 at 06:51, Mario Six <mario.six@gdsys.cc> wrote: >>>>> The dev_read_addr function does not do any bus translations, and just >>>>> returns the raw address read from the device tree, which makes the >>>>> driver not work on systems that need bus translations to get the actual >>>>> memory address of the device's register space. >>>> >>>> Aside from any current functionality, what is the correct thing for >>>> dev_read_addr() to do? I worry that the two parts (live/flat tree) >>>> might do different things. >>>> >>> >>> I took a closer look, and indeed, the two parts of dev_read_addr behave >>> differently: dev_read_addr calls dev_read_addr_index, which calls either >>> ofnode_get_addr_index when live tree is active, or devfdt_get_addr_index when >>> it's not. devfdt_get_addr_index applies bus translations, but >>> ofnode_get_addr_index returns the untranslated address using of_read_number >>> (the else part doesn't run, since we have an active live tree if >>> ofnode_get_addr_index was called). >> >> Good point. I would expect the livetree case to use translated addresses >> during runtime, as dev_read_addr_index calls either ofnode_get_addr_index >> or devfdt_get_addr_index. >> >> In other words: are we missing an address translation from ofnode_get_addr_index >> or should the address retrieved via ofnode_get_addr_index have been >> translated by earlier processing? > > Thank you both. Yes it seems like the right answer is to add the > missing translation in. There is a CONFIG_OF_TRANSLATE which enables > this. I think most boards don't use it, which is probably why there > are no problems without it, but I think it is becoming more common as > boards become more complex. > OK, I'll send a separate patch this week. Thanks for taking a look! >> >>> >>> We could fix this by running of_translate_address on the value returned by >>> of_read_number, so that dev_get_addr would then always return a translated >>> address. >>> >>> But what I think is strange is that most live tree conversion patches (e.g. >>> a9d3037 ("usb: dwc2: convert to livetree") or 4aac33f ("dm: mmc: fsl_esdhc: >>> Update to support livetree")) simply replaced devfdt_get_addr (which does apply >>> bus translations) with dev_read_addr (which does not apply bus translations in >>> the live tree case). Shouldn't the converted driver have failed in the live >>> tree case? Or were all drivers converted until now not depending on any bus >>> translations? >>> >>>> In any case, we should not compound the problem if dev_read_addr() is wrong. >>>> >>>>> >>>>> Since the dev_read_addr function is widely used, we refrain from >>>>> modifying it, and instead read the raw address from the device tree, and >>>>> apply the bus translations using the recently introduced >>>>> dev_translate_address function. >>>>> >>>>> Signed-off-by: Mario Six <mario.six@gdsys.cc> >>>>> --- >>>>> drivers/serial/ns16550.c | 5 +++-- >>>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> REgards, >>>> Simon >>> >>> Best regards, >>> >>> Mario >> > > Regards, > Simon > Best regards, Mario
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index 6f9ce689cf..15d55dfd24 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -389,12 +389,13 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev) { struct ns16550_platdata *plat = dev->platdata; const u32 port_type = dev_get_driver_data(dev); - fdt_addr_t addr; + fdt32_t addr; struct clk clk; int err; /* try Processor Local Bus device first */ - addr = dev_read_addr(dev); + addr = dev_read_u32_default(dev, "reg", 0); + addr = dev_translate_address(dev, &addr); #if defined(CONFIG_PCI) && defined(CONFIG_DM_PCI) if (addr == FDT_ADDR_T_NONE) { /* then try pci device */
The dev_read_addr function does not do any bus translations, and just returns the raw address read from the device tree, which makes the driver not work on systems that need bus translations to get the actual memory address of the device's register space. Since the dev_read_addr function is widely used, we refrain from modifying it, and instead read the raw address from the device tree, and apply the bus translations using the recently introduced dev_translate_address function. Signed-off-by: Mario Six <mario.six@gdsys.cc> --- drivers/serial/ns16550.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)