[U-Boot] [PATCH v2 3/5] dm: ns16550: Don't map_physmem for I/O ports

[Daniel Schwierzeck]

Am 17.05.2016 um 18:00 schrieb Simon Glass:
> Hi Paul,
> 
> On 17 May 2016 at 09:58, Paul Burton <paul.burton at 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 ;)

-- 
- Daniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160518/71425fa1/attachment.sig>