[U-Boot] [PATCH v2] serial: bcm283x_mu: Detect disabled serial device
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(®s->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(®s->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(®s->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; };
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.
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
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 :).
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 wayto 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
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 wayto 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
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 wayto 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
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
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
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
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
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
participants (3)
-
Alexander Graf -
Simon Glass -
Stephen Warren