Hi Daniel, On 18 May 2016 at 04:04, Daniel Schwierzeck <daniel.schwierz...@gmail.com> wrote: > > > Am 17.05.2016 um 18:00 schrieb Simon Glass: >> Hi Paul, >> >> On 17 May 2016 at 09:58, Paul Burton <paul.bur...@imgtec.com> 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
