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

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

[Alexander Graf]

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.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

I'm currently on the road and verified that the non-uart case works properly
with this patch and that the uart case doesn't do anything stupid, but please
double check it before applying :).

---
 board/raspberrypi/rpi/rpi.c                  |  1 +
 drivers/serial/serial_bcm283x_mu.c           | 33 ++++++++++++++++++++++++++++
 include/dm/platform_data/serial_bcm283x_mu.h |  1 +
 3 files changed, 35 insertions(+)

diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
index 4c8253d..9c52c91 100644
--- a/board/raspberrypi/rpi/rpi.c
+++ b/board/raspberrypi/rpi/rpi.c
@@ -53,6 +53,7 @@ U_BOOT_DEVICE(bcm2835_serials) = {
 #else
 static const struct bcm283x_mu_serial_platdata serial_platdata = {
 	.base = 0x3f215040,
+	.gpio = 0x3f200000,
 	.clock = 250000000,
 	.skip_init = true,
 };
diff --git a/drivers/serial/serial_bcm283x_mu.c b/drivers/serial/serial_bcm283x_mu.c
index 7357bbf..02d5f39 100644
--- a/drivers/serial/serial_bcm283x_mu.c
+++ b/drivers/serial/serial_bcm283x_mu.c
@@ -39,6 +39,20 @@ struct bcm283x_mu_regs {
 	u32 baud;
 };
 
+struct bcm283x_gpio_regs {
+	u32 gpfsel0;
+	u32 gpfsel1;
+	u32 gpfsel2;
+	u32 gpfsel3;
+	u32 gpfsel4;
+	u32 gpfsel5;
+	/* ... */
+};
+
+#define BCM283X_GPIO_GPFSEL1_F15_SHIFT	15
+#define BCM283X_GPIO_ALTFUNC_MASK	0x7
+#define BCM283X_GPIO_ALTFUNC_5		0x2
+
 #define BCM283X_MU_LCR_DATA_SIZE_8	3
 
 #define BCM283X_MU_LSR_TX_IDLE		BIT(6)
@@ -48,6 +62,7 @@ struct bcm283x_mu_regs {
 
 struct bcm283x_mu_priv {
 	struct bcm283x_mu_regs *regs;
+	bool disabled;
 };
 
 static int bcm283x_mu_serial_setbrg(struct udevice *dev, int baudrate)
@@ -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;
 }
 
@@ -84,6 +108,9 @@ static int bcm283x_mu_serial_getc(struct udevice *dev)
 	struct bcm283x_mu_regs *regs = priv->regs;
 	u32 data;
 
+	if (priv->disabled)
+		return -EAGAIN;
+
 	/* Wait until there is data in the FIFO */
 	if (!(readl(&regs->lsr) & BCM283X_MU_LSR_RX_READY))
 		return -EAGAIN;
@@ -98,6 +125,9 @@ static int bcm283x_mu_serial_putc(struct udevice *dev, const char data)
 	struct bcm283x_mu_priv *priv = dev_get_priv(dev);
 	struct bcm283x_mu_regs *regs = priv->regs;
 
+	if (priv->disabled)
+		return 0;
+
 	/* Wait until there is space in the FIFO */
 	if (!(readl(&regs->lsr) & BCM283X_MU_LSR_TX_EMPTY))
 		return -EAGAIN;
@@ -114,6 +144,9 @@ static int bcm283x_mu_serial_pending(struct udevice *dev, bool input)
 	struct bcm283x_mu_regs *regs = priv->regs;
 	unsigned int lsr = readl(&regs->lsr);
 
+	if (priv->disabled)
+		return 0;
+
 	if (input) {
 		WATCHDOG_RESET();
 		return (lsr & BCM283X_MU_LSR_RX_READY) ? 1 : 0;
diff --git a/include/dm/platform_data/serial_bcm283x_mu.h b/include/dm/platform_data/serial_bcm283x_mu.h
index 57ae6ad..b2e32d3 100644
--- a/include/dm/platform_data/serial_bcm283x_mu.h
+++ b/include/dm/platform_data/serial_bcm283x_mu.h
@@ -17,6 +17,7 @@
  */
 struct bcm283x_mu_serial_platdata {
 	unsigned long base;
+	unsigned long gpio;
 	unsigned int clock;
 	bool skip_init;
 };
-- 
2.6.6

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

[Stephen Warren]

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.

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.

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

[Alexander Graf]

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

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


Alex

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

[Stephen Warren]

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

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

[Alexander Graf]

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.

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.


Alex

[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@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?

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

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

[Alexander Graf]

> 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

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

[Simon Glass]

Hi Alex,

On 11 August 2016 at 23:27, Alexander Graf <agraf@suse.de> wrote:
>
>
>> 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? :)

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.

>
>>
>>>
>>> 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() ?

Regards,
Simon

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

[Alexander Graf]

On 12.08.16 19:21, Simon Glass wrote:
> Hi Alex,
> 
> On 11 August 2016 at 23:27, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>>> 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? :)
> 
> 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?


Alex

[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@suse.de> wrote:
>
>
> On 12.08.16 19:21, Simon Glass wrote:
>> Hi Alex,
>>
>> On 11 August 2016 at 23:27, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>> 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? :)
>>
>> 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

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

[Alexander Graf]

> On 12 Aug 2016, at 22:07, Simon Glass <sjg@chromium.org> wrote:
> 
> Hi Alex,
> 
> On 12 August 2016 at 12:38, Alexander Graf <agraf@suse.de> wrote:
>> 
>> 
>> On 12.08.16 19:21, Simon Glass wrote:
>>> Hi Alex,
>>> 
>>> On 11 August 2016 at 23:27, Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>> 
>>>>> 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? :)
>>> 
>>> 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] [PATCH v2] serial: bcm283x_mu: Detect disabled serial device

[Simon Glass]

Hi Alex,

On 12 August 2016 at 15:03, Alexander Graf <agraf@suse.de> wrote:
>
>> On 12 Aug 2016, at 22:07, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Alex,
>>
>> On 12 August 2016 at 12:38, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 12.08.16 19:21, Simon Glass wrote:
>>>> Hi Alex,
>>>>
>>>> On 11 August 2016 at 23:27, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>>> 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? :)
>>>>
>>>> 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