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

[Simon Glass]

Hi Alex,

On 12 August 2016 at 15:03, Alexander Graf <agraf at suse.de> wrote:
>
>> On 12 Aug 2016, at 22:07, Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Alex,
>>
>> On 12 August 2016 at 12:38, Alexander Graf <agraf at suse.de> wrote:
>>>
>>>
>>> On 12.08.16 19:21, Simon Glass wrote:
>>>> Hi Alex,
>>>>
>>>> On 11 August 2016 at 23:27, Alexander Graf <agraf at suse.de> wrote:
>>>>>
>>>>>
>>>>>> Am 12.08.2016 um 00:38 schrieb Simon Glass <sjg at chromium.org>:
>>>>>>
>>>>>> 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?
>>>>>
>>>>> You mean like in v2 which Stephen nacked? :)
>>>>
>>>> Yes :-(
>>>>
>>>> Well you can put what you like in the board code, and if this is only
>>>> on the rpi, then it makes sense.
>>>>
>>>> Really though, this is a pinctrl thing, so if there were a pinctrl
>>>> driver you could just use it. The GPIO driver should not deal with pin
>>>> muxing.
>>>
>>> It's the same IP block on the RPi :).
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>
>>>> Can you use board_early_init_f() ?
>>>
>>> How? I guess we would need to
>>>
>>> a) Create the GPIO device
>>> b) Ask the GPIO device whether the pin is muxed correctly
>>> c) Create serial device based on outcome of b
>>>
>>> Is that possible?
>>
>> Well you were asking for a place where you could check a GPIO before
>> the serial driver is started. In board_early_init_f() you can check
>> the GPIO and basically do what you are doing now.
>
> Did the GPIO device get spawned at that point already?

It will already be bound, so that the device exists. But it is only
probed when it is used. So if you use it here, it will be probed.

Regards,
Simon