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

[Alexander Graf <agraf@suse.de>]

> Am 12.08.2016 um 00:38 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi Alex,
> 
>> On 11 August 2016 at 05:33, Alexander Graf <agraf@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@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?

You mean like in v2 which Stephen nacked? :)

> 
>> 
>> 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...

The problem is that we need the gpio device to determine whether the pin is muxed. There is no temporal control that I could see that would allow me to be in a place where the gpio device exists, the serial device does not exist, and where I could then not spawn the serial device based on board logic.


Alex