Re: [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
On Sat, May 21, 2016 at 06:50:37PM +0200, Daniel Schwierzeck wrote: > are you going to send a v4 of this patch? I can also do this if you > like. Then I could apply all other v3 patches of this series. Hi Daniel, I'm happy with either of those options. If you'd prefer me to submit a v4 let me know, but I'd be happy with you replacing patch 3/5. Thanks, Paul ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
Hi Paul, Am 18.05.2016 um 16:52 schrieb Simon Glass: >>> >>> Are you sure systems rely on using I/O ports with map_physmem? The only >>> other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones, >>> in include/configs/x86-common.h, and so far as I can tell they don't use >>> device model which suggests this code has simply been untested before. I >>> don't see why you would use map_physmem on an I/O port address that is >>> then going to be passed to inb/outb & I think the code here is simply >>> wrong to do so. >> >> the current code looks wrong. serial_in_shift() is expanded to inb() >> in case of CONFIG_SYS_NS16550_PORT_MAPPED and to >> in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case >> a map_physmem() is required and should be done in serial_in_shift() >> itself or preferrably only once in >> ns16550_serial_ofdata_to_platdata(). >> >> I think the correct approach would be the following: > > This is better I think. But how about adding a device tree binding to > select I/O access? In principle each device might have its own > settings. Note that's what I worked towards last time I had a crack at this, but it just expanded into an attempt to tackle the mess that is ns16550.c & rather lost sight of the original goal of making Malta work. https://patchwork.ozlabs.org/patch/575643/ https://patchwork.ozlabs.org/patch/577194/ >>> >>> Yes it is tricky. What do you think about the suggestions above? >> >> for now we should only fix the broken port-based I/O. Additional DT >> bindings could be added later. I'd like to merge the DM support on MIPS >> Malta in this merge window ;) > > In that case your patch of moving it to ofdata_to_platdata() looks OK to me. are you going to send a v4 of this patch? I can also do this if you like. Then I could apply all other v3 patches of this series. -- - Daniel signature.asc Description: OpenPGP digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
Hi Daniel, On 18 May 2016 at 04:04, Daniel Schwierzeckwrote: > > > Am 17.05.2016 um 18:00 schrieb Simon Glass: >> Hi Paul, >> >> On 17 May 2016 at 09:58, Paul Burton wrote: >>> On Tue, May 17, 2016 at 09:54:21AM -0600, Simon Glass wrote: diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index 28da9dd..e58e6aa 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value) unsigned char *addr; offset *= 1 << plat->reg_shift; +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED + addr = (unsigned char *)plat->base + offset; +#else addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; +#endif >>> >>> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs >>> to be another parameter? Possibly a flag. But with driver-model we >>> need to be able to support both options in the core code. >> >> Hi Simon, >> >> Are you sure systems rely on using I/O ports with map_physmem? The only >> other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones, >> in include/configs/x86-common.h, and so far as I can tell they don't use >> device model which suggests this code has simply been untested before. I >> don't see why you would use map_physmem on an I/O port address that is >> then going to be passed to inb/outb & I think the code here is simply >> wrong to do so. > > the current code looks wrong. serial_in_shift() is expanded to inb() > in case of CONFIG_SYS_NS16550_PORT_MAPPED and to > in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case > a map_physmem() is required and should be done in serial_in_shift() > itself or preferrably only once in > ns16550_serial_ofdata_to_platdata(). > > I think the correct approach would be the following: This is better I think. But how about adding a device tree binding to select I/O access? In principle each device might have its own settings. >>> >>> Note that's what I worked towards last time I had a crack at this, but >>> it just expanded into an attempt to tackle the mess that is ns16550.c & >>> rather lost sight of the original goal of making Malta work. >>> >>> https://patchwork.ozlabs.org/patch/575643/ >>> https://patchwork.ozlabs.org/patch/577194/ >> >> Yes it is tricky. What do you think about the suggestions above? > > for now we should only fix the broken port-based I/O. Additional DT > bindings could be added later. I'd like to merge the DM support on MIPS > Malta in this merge window ;) In that case your patch of moving it to ofdata_to_platdata() looks OK to me. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
Am 17.05.2016 um 18:00 schrieb Simon Glass: > Hi Paul, > > On 17 May 2016 at 09:58, Paul Burtonwrote: >> On Tue, May 17, 2016 at 09:54:21AM -0600, Simon Glass wrote: >>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c >>> index 28da9dd..e58e6aa 100644 >>> --- a/drivers/serial/ns16550.c >>> +++ b/drivers/serial/ns16550.c >>> @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int >>> offset, int value) >>> unsigned char *addr; >>> >>> offset *= 1 << plat->reg_shift; >>> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED >>> + addr = (unsigned char *)plat->base + offset; >>> +#else >>> addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; >>> +#endif >> >> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs >> to be another parameter? Possibly a flag. But with driver-model we >> need to be able to support both options in the core code. > > Hi Simon, > > Are you sure systems rely on using I/O ports with map_physmem? The only > other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones, > in include/configs/x86-common.h, and so far as I can tell they don't use > device model which suggests this code has simply been untested before. I > don't see why you would use map_physmem on an I/O port address that is > then going to be passed to inb/outb & I think the code here is simply > wrong to do so. the current code looks wrong. serial_in_shift() is expanded to inb() in case of CONFIG_SYS_NS16550_PORT_MAPPED and to in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case a map_physmem() is required and should be done in serial_in_shift() itself or preferrably only once in ns16550_serial_ofdata_to_platdata(). I think the correct approach would be the following: >>> >>> This is better I think. But how about adding a device tree binding to >>> select I/O access? In principle each device might have its own >>> settings. >> >> Note that's what I worked towards last time I had a crack at this, but >> it just expanded into an attempt to tackle the mess that is ns16550.c & >> rather lost sight of the original goal of making Malta work. >> >> https://patchwork.ozlabs.org/patch/575643/ >> https://patchwork.ozlabs.org/patch/577194/ > > Yes it is tricky. What do you think about the suggestions above? for now we should only fix the broken port-based I/O. Additional DT bindings could be added later. I'd like to merge the DM support on MIPS Malta in this merge window ;) -- - Daniel signature.asc Description: OpenPGP digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
Hi Paul, On 17 May 2016 at 09:58, Paul Burtonwrote: > On Tue, May 17, 2016 at 09:54:21AM -0600, Simon Glass wrote: >> > >> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c >> > >> > index 28da9dd..e58e6aa 100644 >> > >> > --- a/drivers/serial/ns16550.c >> > >> > +++ b/drivers/serial/ns16550.c >> > >> > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int >> > >> > offset, int value) >> > >> > unsigned char *addr; >> > >> > >> > >> > offset *= 1 << plat->reg_shift; >> > >> > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED >> > >> > + addr = (unsigned char *)plat->base + offset; >> > >> > +#else >> > >> > addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; >> > >> > +#endif >> > >> >> > >> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs >> > >> to be another parameter? Possibly a flag. But with driver-model we >> > >> need to be able to support both options in the core code. >> > > >> > > Hi Simon, >> > > >> > > Are you sure systems rely on using I/O ports with map_physmem? The only >> > > other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones, >> > > in include/configs/x86-common.h, and so far as I can tell they don't use >> > > device model which suggests this code has simply been untested before. I >> > > don't see why you would use map_physmem on an I/O port address that is >> > > then going to be passed to inb/outb & I think the code here is simply >> > > wrong to do so. >> > >> > the current code looks wrong. serial_in_shift() is expanded to inb() >> > in case of CONFIG_SYS_NS16550_PORT_MAPPED and to >> > in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case >> > a map_physmem() is required and should be done in serial_in_shift() >> > itself or preferrably only once in >> > ns16550_serial_ofdata_to_platdata(). >> > >> > I think the correct approach would be the following: >> >> This is better I think. But how about adding a device tree binding to >> select I/O access? In principle each device might have its own >> settings. > > Note that's what I worked towards last time I had a crack at this, but > it just expanded into an attempt to tackle the mess that is ns16550.c & > rather lost sight of the original goal of making Malta work. > > https://patchwork.ozlabs.org/patch/575643/ > https://patchwork.ozlabs.org/patch/577194/ Yes it is tricky. What do you think about the suggestions above? Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
On Tue, May 17, 2016 at 09:54:21AM -0600, Simon Glass wrote: > > >> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c > > >> > index 28da9dd..e58e6aa 100644 > > >> > --- a/drivers/serial/ns16550.c > > >> > +++ b/drivers/serial/ns16550.c > > >> > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int > > >> > offset, int value) > > >> > unsigned char *addr; > > >> > > > >> > offset *= 1 << plat->reg_shift; > > >> > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED > > >> > + addr = (unsigned char *)plat->base + offset; > > >> > +#else > > >> > addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; > > >> > +#endif > > >> > > >> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs > > >> to be another parameter? Possibly a flag. But with driver-model we > > >> need to be able to support both options in the core code. > > > > > > Hi Simon, > > > > > > Are you sure systems rely on using I/O ports with map_physmem? The only > > > other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones, > > > in include/configs/x86-common.h, and so far as I can tell they don't use > > > device model which suggests this code has simply been untested before. I > > > don't see why you would use map_physmem on an I/O port address that is > > > then going to be passed to inb/outb & I think the code here is simply > > > wrong to do so. > > > > the current code looks wrong. serial_in_shift() is expanded to inb() > > in case of CONFIG_SYS_NS16550_PORT_MAPPED and to > > in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case > > a map_physmem() is required and should be done in serial_in_shift() > > itself or preferrably only once in > > ns16550_serial_ofdata_to_platdata(). > > > > I think the correct approach would be the following: > > This is better I think. But how about adding a device tree binding to > select I/O access? In principle each device might have its own > settings. Note that's what I worked towards last time I had a crack at this, but it just expanded into an attempt to tackle the mess that is ns16550.c & rather lost sight of the original goal of making Malta work. https://patchwork.ozlabs.org/patch/575643/ https://patchwork.ozlabs.org/patch/577194/ Thanks, Paul ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
Hi Daniel, On 17 May 2016 at 09:51, Daniel Schwierzeckwrote: > > 2016-05-17 14:48 GMT+02:00 Paul Burton : > > On Tue, May 17, 2016 at 06:11:22AM -0600, Simon Glass wrote: > >> Hi Paul, > >> > >> On 16 May 2016 at 11:44, Paul Burton wrote: > >> > If the UART is to be accessed using I/O port accessors (inb & outb) then > >> > using map_physmem doesn't make sense, since it operates in a different > >> > memory space. Remove the call to map_physmem when > >> > CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses > >> > to not be mangled by the incorrect mapping. > >> > > >> > Signed-off-by: Paul Burton > >> > --- > >> > > >> > Changes in v2: > >> > - New patch, part of a simplified approach tackling only a single Malta > >> > UART. > >> > > >> > drivers/serial/ns16550.c | 8 > >> > 1 file changed, 8 insertions(+) > >> > > >> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c > >> > index 28da9dd..e58e6aa 100644 > >> > --- a/drivers/serial/ns16550.c > >> > +++ b/drivers/serial/ns16550.c > >> > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int > >> > offset, int value) > >> > unsigned char *addr; > >> > > >> > offset *= 1 << plat->reg_shift; > >> > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED > >> > + addr = (unsigned char *)plat->base + offset; > >> > +#else > >> > addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; > >> > +#endif > >> > >> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs > >> to be another parameter? Possibly a flag. But with driver-model we > >> need to be able to support both options in the core code. > > > > Hi Simon, > > > > Are you sure systems rely on using I/O ports with map_physmem? The only > > other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones, > > in include/configs/x86-common.h, and so far as I can tell they don't use > > device model which suggests this code has simply been untested before. I > > don't see why you would use map_physmem on an I/O port address that is > > then going to be passed to inb/outb & I think the code here is simply > > wrong to do so. > > the current code looks wrong. serial_in_shift() is expanded to inb() > in case of CONFIG_SYS_NS16550_PORT_MAPPED and to > in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case > a map_physmem() is required and should be done in serial_in_shift() > itself or preferrably only once in > ns16550_serial_ofdata_to_platdata(). > > I think the correct approach would be the following: This is better I think. But how about adding a device tree binding to select I/O access? In principle each device might have its own settings. > > --- a/drivers/serial/ns16550.c > +++ b/drivers/serial/ns16550.c > @@ -100,7 +100,8 @@ static void ns16550_writeb(NS16550_t port, int > offset, int value) > unsigned char *addr; > > offset *= 1 << plat->reg_shift; > - addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; > + addr = plat->base + offset; > + > /* > * As far as we know it doesn't make sense to support selection of > * these options at run-time, so use the existing CONFIG options. > @@ -114,7 +115,7 @@ static int ns16550_readb(NS16550_t port, int offset) > unsigned char *addr; > > offset *= 1 << plat->reg_shift; > - addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; > + addr = plat->base + offset; > > return serial_in_shift(addr + plat->reg_offset, plat->reg_shift); > } > @@ -400,7 +401,12 @@ int ns16550_serial_ofdata_to_platdata(struct udevice > *dev) > if (addr == FDT_ADDR_T_NONE) > return -EINVAL; > > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED > plat->base = addr; > +#else > + plat->base = map_physmem(addr, 0, MAP_NOCACHE); > +#endif > + > plat->reg_offset = fdtdec_get_int(gd->fdt_blob, dev->of_offset, > "reg-offset", 0); > plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset, > > -- > - Daniel Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
2016-05-17 14:48 GMT+02:00 Paul Burton: > On Tue, May 17, 2016 at 06:11:22AM -0600, Simon Glass wrote: >> Hi Paul, >> >> On 16 May 2016 at 11:44, Paul Burton wrote: >> > If the UART is to be accessed using I/O port accessors (inb & outb) then >> > using map_physmem doesn't make sense, since it operates in a different >> > memory space. Remove the call to map_physmem when >> > CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses >> > to not be mangled by the incorrect mapping. >> > >> > Signed-off-by: Paul Burton >> > --- >> > >> > Changes in v2: >> > - New patch, part of a simplified approach tackling only a single Malta >> > UART. >> > >> > drivers/serial/ns16550.c | 8 >> > 1 file changed, 8 insertions(+) >> > >> > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c >> > index 28da9dd..e58e6aa 100644 >> > --- a/drivers/serial/ns16550.c >> > +++ b/drivers/serial/ns16550.c >> > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int >> > offset, int value) >> > unsigned char *addr; >> > >> > offset *= 1 << plat->reg_shift; >> > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED >> > + addr = (unsigned char *)plat->base + offset; >> > +#else >> > addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; >> > +#endif >> >> Please don't add CONFIG #ifdefs in these functions. Perhaps it needs >> to be another parameter? Possibly a flag. But with driver-model we >> need to be able to support both options in the core code. > > Hi Simon, > > Are you sure systems rely on using I/O ports with map_physmem? The only > other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones, > in include/configs/x86-common.h, and so far as I can tell they don't use > device model which suggests this code has simply been untested before. I > don't see why you would use map_physmem on an I/O port address that is > then going to be passed to inb/outb & I think the code here is simply > wrong to do so. the current code looks wrong. serial_in_shift() is expanded to inb() in case of CONFIG_SYS_NS16550_PORT_MAPPED and to in_le32()/in_be32()/readl()/readb() otherwise. Only in the latter case a map_physmem() is required and should be done in serial_in_shift() itself or preferrably only once in ns16550_serial_ofdata_to_platdata(). I think the correct approach would be the following: --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -100,7 +100,8 @@ static void ns16550_writeb(NS16550_t port, int offset, int value) unsigned char *addr; offset *= 1 << plat->reg_shift; - addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; + addr = plat->base + offset; + /* * As far as we know it doesn't make sense to support selection of * these options at run-time, so use the existing CONFIG options. @@ -114,7 +115,7 @@ static int ns16550_readb(NS16550_t port, int offset) unsigned char *addr; offset *= 1 << plat->reg_shift; - addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; + addr = plat->base + offset; return serial_in_shift(addr + plat->reg_offset, plat->reg_shift); } @@ -400,7 +401,12 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev) if (addr == FDT_ADDR_T_NONE) return -EINVAL; +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED plat->base = addr; +#else + plat->base = map_physmem(addr, 0, MAP_NOCACHE); +#endif + plat->reg_offset = fdtdec_get_int(gd->fdt_blob, dev->of_offset, "reg-offset", 0); plat->reg_shift = fdtdec_get_int(gd->fdt_blob, dev->of_offset, -- - Daniel ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
On Tue, May 17, 2016 at 06:11:22AM -0600, Simon Glass wrote: > Hi Paul, > > On 16 May 2016 at 11:44, Paul Burtonwrote: > > If the UART is to be accessed using I/O port accessors (inb & outb) then > > using map_physmem doesn't make sense, since it operates in a different > > memory space. Remove the call to map_physmem when > > CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses > > to not be mangled by the incorrect mapping. > > > > Signed-off-by: Paul Burton > > --- > > > > Changes in v2: > > - New patch, part of a simplified approach tackling only a single Malta > > UART. > > > > drivers/serial/ns16550.c | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c > > index 28da9dd..e58e6aa 100644 > > --- a/drivers/serial/ns16550.c > > +++ b/drivers/serial/ns16550.c > > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, > > int value) > > unsigned char *addr; > > > > offset *= 1 << plat->reg_shift; > > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED > > + addr = (unsigned char *)plat->base + offset; > > +#else > > addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; > > +#endif > > Please don't add CONFIG #ifdefs in these functions. Perhaps it needs > to be another parameter? Possibly a flag. But with driver-model we > need to be able to support both options in the core code. Hi Simon, Are you sure systems rely on using I/O ports with map_physmem? The only other systems that define CONFIG_SYS_NS16550_PORT_MAPPED are x86 ones, in include/configs/x86-common.h, and so far as I can tell they don't use device model which suggests this code has simply been untested before. I don't see why you would use map_physmem on an I/O port address that is then going to be passed to inb/outb & I think the code here is simply wrong to do so. > This driver is a real mess. I'm hoping we can simplify it once we have > everything on driver model. Agreed, that would be wonderful. Thanks, Paul ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
Hi Paul, On 16 May 2016 at 11:44, Paul Burtonwrote: > If the UART is to be accessed using I/O port accessors (inb & outb) then > using map_physmem doesn't make sense, since it operates in a different > memory space. Remove the call to map_physmem when > CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses > to not be mangled by the incorrect mapping. > > Signed-off-by: Paul Burton > --- > > Changes in v2: > - New patch, part of a simplified approach tackling only a single Malta UART. > > drivers/serial/ns16550.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c > index 28da9dd..e58e6aa 100644 > --- a/drivers/serial/ns16550.c > +++ b/drivers/serial/ns16550.c > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, > int value) > unsigned char *addr; > > offset *= 1 << plat->reg_shift; > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED > + addr = (unsigned char *)plat->base + offset; > +#else > addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; > +#endif Please don't add CONFIG #ifdefs in these functions. Perhaps it needs to be another parameter? Possibly a flag. But with driver-model we need to be able to support both options in the core code. This driver is a real mess. I'm hoping we can simplify it once we have everything on driver model. > /* > * As far as we know it doesn't make sense to support selection of > * these options at run-time, so use the existing CONFIG options. > @@ -114,7 +118,11 @@ static int ns16550_readb(NS16550_t port, int offset) > unsigned char *addr; > > offset *= 1 << plat->reg_shift; > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED > + addr = (unsigned char *)plat->base + offset; > +#else > addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; > +#endif > > return serial_in_shift(addr + plat->reg_offset, plat->reg_shift); > } > -- > 2.8.2 > Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
Am 16.05.2016 um 19:44 schrieb Paul Burton: > If the UART is to be accessed using I/O port accessors (inb & outb) then > using map_physmem doesn't make sense, since it operates in a different > memory space. Remove the call to map_physmem when > CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses > to not be mangled by the incorrect mapping. > > Signed-off-by: Paul BurtonReviewed-by: Daniel Schwierzeck Tested-by: Daniel Schwierzeck > --- > > Changes in v2: > - New patch, part of a simplified approach tackling only a single Malta UART. > > drivers/serial/ns16550.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c > index 28da9dd..e58e6aa 100644 > --- a/drivers/serial/ns16550.c > +++ b/drivers/serial/ns16550.c > @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, > int value) > unsigned char *addr; > > offset *= 1 << plat->reg_shift; > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED > + addr = (unsigned char *)plat->base + offset; > +#else > addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; > +#endif > /* >* As far as we know it doesn't make sense to support selection of >* these options at run-time, so use the existing CONFIG options. > @@ -114,7 +118,11 @@ static int ns16550_readb(NS16550_t port, int offset) > unsigned char *addr; > > offset *= 1 << plat->reg_shift; > +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED > + addr = (unsigned char *)plat->base + offset; > +#else > addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; > +#endif > > return serial_in_shift(addr + plat->reg_offset, plat->reg_shift); > } > -- - Daniel signature.asc Description: OpenPGP digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports
If the UART is to be accessed using I/O port accessors (inb & outb) then using map_physmem doesn't make sense, since it operates in a different memory space. Remove the call to map_physmem when CONFIG_SYS_NS16550_PORT_MAPPED is defined, allowing I/O port addresses to not be mangled by the incorrect mapping. Signed-off-by: Paul Burton--- Changes in v2: - New patch, part of a simplified approach tackling only a single Malta UART. drivers/serial/ns16550.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index 28da9dd..e58e6aa 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -100,7 +100,11 @@ static void ns16550_writeb(NS16550_t port, int offset, int value) unsigned char *addr; offset *= 1 << plat->reg_shift; +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED + addr = (unsigned char *)plat->base + offset; +#else addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; +#endif /* * As far as we know it doesn't make sense to support selection of * these options at run-time, so use the existing CONFIG options. @@ -114,7 +118,11 @@ static int ns16550_readb(NS16550_t port, int offset) unsigned char *addr; offset *= 1 << plat->reg_shift; +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED + addr = (unsigned char *)plat->base + offset; +#else addr = map_physmem(plat->base, 0, MAP_NOCACHE) + offset; +#endif return serial_in_shift(addr + plat->reg_offset, plat->reg_shift); } -- 2.8.2 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot