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

[Simon Glass]

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.

The only question is how to enable/disable the serial driver. One
option is to add some platform data to the driver which has a
'disable' flag. Check that in the serial driver and return -EPERM from
the probe() method. I think the plumbing will work from there, but
have not tried it. As an example of this sort of hack, see
board_init() in gurnard.c (it is used in
atmel_fb_ofdata_to_platdata()).

Regards,
Simon