[U-Boot] [PATCH v2] serial: bcm283x_mu: Detect disabled serial device

[Simon Glass]

Hi Alex,

On 11 August 2016 at 05:33, Alexander Graf <agraf at suse.de> wrote:
>
>
> On 09.08.16 06:28, Stephen Warren wrote:
>> On 08/04/2016 05:15 PM, Alexander Graf wrote:
>>>
>>>> On 04 Aug 2016, at 20:11, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>>
>>>> On 08/04/2016 01:11 AM, Alexander Graf wrote:
>>>>> On the raspberry pi, you can disable the serial port to gain dynamic
>>>>> frequency
>>>>> scaling which can get handy at times.
>>>>>
>>>>> However, in such a configuration the serial controller gets its rx
>>>>> queue filled
>>>>> up with zero bytes which then happily get transmitted on to whoever
>>>>> calls
>>>>> getc() today.
>>>>>
>>>>> This patch adds detection logic for that case by checking whether
>>>>> the RX pin is
>>>>> mapped to GPIO15 and disables the mini uart if it is not mapped
>>>>> properly.
>>>>>
>>>>> That way we can leave the driver enabled in the tree and can
>>>>> determine during
>>>>> runtime whether serial is usable or not, having a single binary that
>>>>> allows for
>>>>> uart and non-uart operation.
>>>>
>>>>> diff --git a/drivers/serial/serial_bcm283x_mu.c
>>>>> b/drivers/serial/serial_bcm283x_mu.c
>>>>
>>>>> @@ -72,9 +87,18 @@ static int bcm283x_mu_serial_probe(struct udevice
>>>>> *dev)
>>>>> {
>>>>>     struct bcm283x_mu_serial_platdata *plat = dev_get_platdata(dev);
>>>>>     struct bcm283x_mu_priv *priv = dev_get_priv(dev);
>>>>> +    struct bcm283x_gpio_regs *gpio = (struct bcm283x_gpio_regs
>>>>> *)plat->gpio;
>>>>>
>>>>>     priv->regs = (struct bcm283x_mu_regs *)plat->base;
>>>>>
>>>>> +    /*
>>>>> +     * The RPi3 disables the mini uart by default. The easiest way
>>>>> to find
>>>>> +     * out whether it is available is to check if the pin is muxed.
>>>>> +     */
>>>>> +    if (((readl(&gpio->gpfsel1) >> BCM283X_GPIO_GPFSEL1_F15_SHIFT) &
>>>>> +        BCM283X_GPIO_ALTFUNC_MASK) != BCM283X_GPIO_ALTFUNC_5)
>>>>> +        priv->disabled = true;
>>>>> +
>>>>>     return 0;
>>>>
>>>> Comment on the current implementation: Can't probe() return an error
>>>> if the device should be disabled? That would avoid the need to check
>>>> priv->disabled in all the other functions.
>>>
>>> I guess I should’ve put that in a comment somewhere. Unfortunately we
>>> can’t. If I just return an error on probe, U-Boot will panic because
>>> we tell it in a CONFIG define that we require a serial port (grep for
>>> CONFIG_REQUIRE_SERIAL_CONSOLE).
>>>
>>> We could maybe try to unset that define instead?
>>
>> Yes, assuming that U-Boot runs just fine with HDMI console only, I think
>> it's fine to unset CONFIG_REQUIRE_SERIAL_CONSOLE.
>>
>>>> Overall comment: I'd rather not put this logic into the UART driver
>>>> itself; it is system-specific rather than device-specific. I'd also
>>>> rather not have the UART driver touching GPIO registers; that's not
>>>> very modular, and could cause problems if the Pi is converted to use
>>>> DT to instantiate devices.
>>>>
>>>> Instead, can we put the logic into board/raspberrypi/rpi/rpi.c? I.e.
>>>> have some early function come along and enable/disable the
>>>> bcm2837_serials device object as appropriate? That way it isolates
>>>> the code to the Pi specifically, and not any other bcm283x board.
>>>> We'd want to wrap that code in #ifdef CONFIG_PL01X_SERIAL.
>>>
>>> We can do that if we can fail at probe time. If we absolutely must
>>> have a serial driver to work in the first place, that doesn’t work. I
>>> can try to poke at it, but it’ll be a few days I think :).
>
> So I couldn't find a sane way to fail probing based on something defined
> in the board file, reusing the existing gpio device.

Would it be possible to move this code into the serial driver?

>
> However, there's an easy alternative. We can make the console code just
> ignore our serial device if we set its pointer to NULL. That way we
> still have the device, but can contain all logic to disable usage of the
> mini uart to the board file.

I'm not very keen on that - feels like a hack.  What is stopping
Stephen's idea from working? I could perhaps help with dm plumbing is
that is the issue...

>
>
> Alex

Regards,
Simon