[U-Boot] [PATCH] serial: ns16550: Add support for the BayTrail internal HS UART

[Stefan Roese]

Hi Simon,

On 18.07.2016 13:24, Simon Glass wrote:
> On 18 July 2016 at 02:17, Stefan Roese <sr at denx.de> wrote:
>> Hi Bin,
>>
>>
>> On 14.07.2016 02:52, Bin Meng wrote:
>>>
>>> On Wed, Jul 13, 2016 at 2:03 PM, Stefan Roese <sr at denx.de> wrote:
>>>>
>>>> To support the BayTrail internal SIO HS UART, the internal UART clock
>>>> needs to get configured. This patch adds support for this clock
>>>> configuration which will be done, if a compatible DT node is found.
>>>>
>>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>> Cc: Simon Glass <sjg at chromium.org>
>>>> Cc: Bin Meng <bmeng.cn at gmail.com>
>>>> ---
>>>>    drivers/serial/ns16550.c | 26 ++++++++++++++++++++++++++
>>>>    1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>>> index c6cb3eb..12499ec 100644
>>>> --- a/drivers/serial/ns16550.c
>>>> +++ b/drivers/serial/ns16550.c
>>>> @@ -94,6 +94,13 @@ static inline int serial_in_shift(void *addr, int
>>>> shift)
>>>>    #define CONFIG_SYS_NS16550_CLK  0
>>>>    #endif
>>>>
>>>> +/* Intel BayTrail defines */
>>>> +#define BYT_PRV_CLK                    0x800
>>>> +#define BYT_PRV_CLK_EN                 (1 << 0)
>>>> +#define BYT_PRV_CLK_M_VAL_SHIFT                1
>>>> +#define BYT_PRV_CLK_N_VAL_SHIFT                16
>>>> +#define BYT_PRV_CLK_UPDATE             (1 << 31)
>>>> +
>>>>    static void ns16550_writeb(NS16550_t port, int offset, int value)
>>>>    {
>>>>           struct ns16550_platdata *plat = port->plat;
>>>> @@ -381,6 +388,25 @@ int ns16550_serial_ofdata_to_platdata(struct udevice
>>>> *dev)
>>>>                           return ret;
>>>>
>>>>                   addr = bar;
>>>> +
>>>> +               /* Setup BayTrail UART clock */
>>>> +               ret = fdt_node_check_compatible(gd->fdt_blob,
>>>> dev->of_offset,
>>>> +                                               "baytrail-hs-uart");
>>>> +               if (ret == 0) {
>>>> +                       u32 m, n, reg;
>>>> +
>>>> +                       /*
>>>> +                        * Configure the BayTrail UART clock for the
>>>> internal
>>>> +                        * HS UARTs (PCI devices) to 58982400 Hz
>>>> +                        */
>>>> +                       m = 0x2400;
>>>> +                       n = 0x3d09;
>>>> +                       reg = (m << BYT_PRV_CLK_M_VAL_SHIFT) |
>>>> +                               (n << BYT_PRV_CLK_N_VAL_SHIFT);
>>>> +                       writel(reg, bar + BYT_PRV_CLK);
>>>> +                       reg |= BYT_PRV_CLK_EN | BYT_PRV_CLK_UPDATE;
>>>> +                       writel(reg, bar + BYT_PRV_CLK);
>>>> +               }
>>>>           }
>>>>    #endif
>>>
>>>
>>> I prefer moving these clock setup codes to the SoC codes
>>> (arch/x86/cpu/baytrail/). Is that possible?
>>
>>
>> It is possible, but I'm not so sure if this is "better". First, I can't
>> find a file in arch/x86/cpu/baytrail/ that is really fitting for such
>> a UART clock setup. Sure, I could create a new one, but I'm not so sure
>> if its worth it. Also, in the Linux 8250 serial driver, the clock setup
>> is also done in this driver. It would be cleaner to move this clock
>> setup into a separate function in this file. But this would mean
>> additional #ifdef's for this function, which I'm hesitant to add.
>>
>> So what do you think? Can this code stay in this file? Or where
>> exactly do you want me to move it into?
>
> Perhaps arch_cpu_init_dm()? That's where ivybridge puts it. This
> function is called immediately after driver model is inited.

Thanks. Let me check if I can cook up a nicer patch version
shortly...

Thanks,
Stefan