> On 12 Aug 2016, at 22:07, Simon Glass <s...@chromium.org> wrote:
>
> Hi Alex,
>
> On 12 August 2016 at 12:38, Alexander Graf <ag...@suse.de> wrote:
>>
>>
>> On 12.08.16 19:21, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 11 August 2016 at 23:27, Alexander Graf <ag...@suse.de> wrote:
>>>>
>>>>
>>>>> Am 12.08.2016 um 00:38 schrieb Simon Glass <s...@chromium.org>:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>>> On 11 August 2016 at 05:33, Alexander Graf <ag...@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 <swar...@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?
Alex
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
