[U-Boot] [PATCH v3 14/18] serial: 16550: allow the driver to support MediaTek serial

[Simon Glass]

Hi Weijie,

On 5 November 2018 at 19:48, Weijie Gao <weijie.gao at mediatek.com> wrote:
> On Mon, 2018-11-05 at 16:37 +0800, Ryder Lee wrote:
>> On Mon, 2018-11-05 at 10:20 +0800, Ryder Lee wrote:
>> > On Sat, 2018-11-03 at 00:09 -0600, Simon Glass wrote:
>> > > Hi Ryder,
>> > >
>> > > On 2 November 2018 at 09:15, Ryder Lee <ryder.lee at mediatek.com> wrote:
>> > > > This patch adds an extra operation in ns16550.c to suuport MediaTek
>> > > > SoCs as we have a highspeed register which influences the calcualtion
>> > > > of the divisor.
>> > > >
>> > > > Note that we don't support the baudrate greater than 115200 currently.
>> > > >
>> > > > Signed-off-by: Ryder Lee <ryder.lee at mediatek.com>
>> > > > Tested-by: Matthias Brugger <matthias.bgg at gmail.com>
>> > > > Reviewed-by: Simon Glass <sjg at chromium.org>
>> > > > ---
>> > > >  drivers/serial/ns16550.c | 10 ++++++++++
>> > > >  1 file changed, 10 insertions(+)
>> > >
>> > > Actually it seems to me that we should have a compatible string for
>> > > this and deal with this at run-time?
>> > >
>> > > Is that easy to do here?
>> > >
>> >
>> > How about this:
>> >
>> > uart0: serial at 11002000 {
>> >     compatible = "ns16550a";
>> >     ....
>> >     mediatek,highspeed = <0>;
>> >     ....
>> >
>> > Ryder
>> >
>>
>> Sorry, I didn't get it right. I will add a compatible string in
>> ns16550_serial_ids[] for MTK chips.
>>
>> Ryder
>>
>>
>
> Hi Simon,
>
> We have tried the compatible string, but it made the ns16550 driver more
> complicated.
>
> To use the compatible string we have to add a new field in
> ns16550_platdata, and change the flow of ns16550_serial_probe.
> Moreover, it's totally useless for debug uart.
>
> At present using a macro is the easiest way here.

It might be easier but it is not correct. We should not have
platform-specific #ifdefs in a common driver.

Here's what I think you should do:

1. Create your own separate driver which calls into the code of this one
2. Add a 'quirks' flag word to the platform data and refactor the code
to deal with your quirk, or add a separate field like you say
3. For the debug uart, put it in your separate driver, if you cannot
easily change _debug_uart_init(). I suppose we could have a
CONFIG_DEBUG_UART_QUIRKS flag wod?

Regards,
Simon