[U-Boot] [PATCH] serial: add BCM283x mini UART driver

[Simon Glass]

Hi Stephen,

On 9 April 2016 at 21:45, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 04/09/2016 12:34 PM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 16 March 2016 at 21:46, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>
>>> The RPi3 typically uses the regular UART for high-speed communication
>>> with
>>> the Bluetooth device, leaving us the mini UART to use for the serial
>>> console. Add support for this UART so we can use it.
>>>
>>> Signed-off-by: Stephen Warren <swarren at wwwdotorg.org>
>>> ---
>>> (This will be a dependency for the RPi 3 patches, so it'd be good if it
>>> could make it into mainline pretty quickly if acceptable.)
>>
>>
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>
>> Not sure if this went in already. But see comment below.
>
>
> It has, but I can send a fix.
>
>>> diff --git a/drivers/serial/serial_bcm283x_mu.c
>>> b/drivers/serial/serial_bcm283x_mu.c
>
>
>>> +static int bcm283x_mu_serial_setbrg(struct udevice *dev, int baudrate)
>>> +{
>>> +       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
>>> +       struct bcm283x_mu_regs *regs = priv->regs;
>>> +       /* FIXME: Get this from plat data later */
>>> +       u32 clock_rate = 250000000;
>>
>>
>> Or device tree?
>
>
> Well even if DT were used on this platform, the code right here would get
> the clock rate from platform data. Now, whether the platform data came from
> a board file or was parsed from DT is another matter.
>
>>> +static int bcm283x_mu_serial_pending(struct udevice *dev, bool input)
>>> +{
>>> +       struct bcm283x_mu_priv *priv = dev_get_priv(dev);
>>> +       struct bcm283x_mu_regs *regs = priv->regs;
>>> +       unsigned int lsr = readl(&regs->lsr);
>>> +
>>> +       if (input) {
>>> +               WATCHDOG_RESET();
>>> +               return lsr & BCM283X_MU_LSR_RX_READY;
>>> +       } else {
>>> +               return !(lsr & BCM283X_MU_LSR_TX_IDLE);
>>
>>
>> These look like flags - be care to return 1 if there is an unknown
>> number of characters, rather than (e.g. 4). The latter might cause the
>> uclass to expect 4 characters to be present.
>>
>> Suggest putting ? 1 : 0 or ? 0 : 1 on the end.
>
>
> Luckily BCM283X_MU_LSR_RX_READY is BIT(0) so there's no practical issue. A
> !! would make this more obvious at this point in the code though. Do you
> think that warrants a patch? The ! on the second return also ensure the
> correct return value.

I think it is worth a patch, in case perhaps someone else copies that
code later. Agreed not a high priority though.

Regards,
Simon