diff mbox series

[U-Boot] board: rpi4: fix instantiating PL011 driver

Message ID alpine.OSX.2.21.1909270137030.94829@polar.dapoz.ca
State New
Delegated to: Matthias Brugger
Headers show
Series [U-Boot] board: rpi4: fix instantiating PL011 driver | expand

Commit Message

Mark Dapoz Sept. 27, 2019, 5:39 a.m. UTC
The bcm283x PL011 serial driver is hard coded to allow connections only
for UART0.  This prevents using any of the other UARTs as the U-boot
console.

The bcm283x serial driver is updated to allow connections to any of the
PL011 devices.  The initialization logic has been updated as follows:

     - a GPIO pinlist is extracted from the device tree for the device
     - each pin is compared against a function assignment table
     - if the pin is found in the table and the currently assigned GPIO
       function doesn't match, the device is unavailable
     - if the function matches the table entry or the pin is not listed
       the table, the device is available

The function assignment table contains entries for UART0 to ensure it
is multiplexed to GPIO pins 14 and 15.

This logic allows GPIO 14 and 15 to be multiplexed between the miniUART
and the PL011 UART by the board's loader.

An error in the default clock rate has also been fixed.  If the device
tree entry for the serial device doesn't specify a "clocks" property,
the default frequency of 1Hz was used.  The default has been changed
to 48MHz.

The configuration for the Raspberry Pi 4 has been updated to include
the PL011 driver.

To select an alternate UART as the U-boot console, the stdout-path can
be used as follows:

       chosen {
              stdout-path = "/soc/serial@7e201a00:115200";
              }

Signed-off-by: Mark Dapoz <Mark.Dapoz@windriver.com>
---

  configs/rpi_4_32b_defconfig           |   1 +
  configs/rpi_4_defconfig               |   1 +
  drivers/serial/serial_bcm283x_pl011.c | 150 ++++++++++++++++++++++++++++++----
  3 files changed, 136 insertions(+), 16 deletions(-)

Comments

Matthias Brugger Nov. 8, 2019, 5:23 p.m. UTC | #1
On 27/09/2019 07:39, Mark Dapoz wrote:
> The bcm283x PL011 serial driver is hard coded to allow connections only
> for UART0.  This prevents using any of the other UARTs as the U-boot
> console.
> 
> The bcm283x serial driver is updated to allow connections to any of the
> PL011 devices.  The initialization logic has been updated as follows:
> 
>     - a GPIO pinlist is extracted from the device tree for the device
>     - each pin is compared against a function assignment table
>     - if the pin is found in the table and the currently assigned GPIO
>       function doesn't match, the device is unavailable
>     - if the function matches the table entry or the pin is not listed
>       the table, the device is available
> 
> The function assignment table contains entries for UART0 to ensure it
> is multiplexed to GPIO pins 14 and 15.
> 

Which is what we already check right now, correct?

> This logic allows GPIO 14 and 15 to be multiplexed between the miniUART
> and the PL011 UART by the board's loader.
> 

Maybe I'm too tired, but I don't see where this is done. I can just see that you
changed the function to check if PL011 uses GPIO15 as ALT0 to a different
function which checks the same. Can you please explain.

> An error in the default clock rate has also been fixed.  If the device
> tree entry for the serial device doesn't specify a "clocks" property,
> the default frequency of 1Hz was used.  The default has been changed
> to 48MHz.

If doesn't that mean that we don't have a valid DTS?
If this still makes sense then please split this out into a separate patch.

Regards,
Matthias

> 
> The configuration for the Raspberry Pi 4 has been updated to include
> the PL011 driver.
> 
> To select an alternate UART as the U-boot console, the stdout-path can
> be used as follows:
> 
>       chosen {
>              stdout-path = "/soc/serial@7e201a00:115200";
>              }
> 
> Signed-off-by: Mark Dapoz <Mark.Dapoz@windriver.com>
> ---
> 
>  configs/rpi_4_32b_defconfig           |   1 +
>  configs/rpi_4_defconfig               |   1 +
>  drivers/serial/serial_bcm283x_pl011.c | 150 ++++++++++++++++++++++++++++++----
>  3 files changed, 136 insertions(+), 16 deletions(-)
> 
> diff --git a/configs/rpi_4_32b_defconfig b/configs/rpi_4_32b_defconfig
> index dc69690..cd66b0b 100644
> --- a/configs/rpi_4_32b_defconfig
> +++ b/configs/rpi_4_32b_defconfig
> @@ -31,3 +31,4 @@ CONFIG_SYS_WHITE_ON_BLACK=y
>  CONFIG_CONSOLE_SCROLL_LINES=10
>  CONFIG_PHYS_TO_BUS=y
>  CONFIG_OF_LIBFDT_OVERLAY=y
> +CONFIG_BCM283X_PL011_SERIAL=y
> diff --git a/configs/rpi_4_defconfig b/configs/rpi_4_defconfig
> index 2d63197..9f862c4 100644
> --- a/configs/rpi_4_defconfig
> +++ b/configs/rpi_4_defconfig
> @@ -31,3 +31,4 @@ CONFIG_SYS_WHITE_ON_BLACK=y
>  CONFIG_CONSOLE_SCROLL_LINES=10
>  CONFIG_PHYS_TO_BUS=y
>  CONFIG_OF_LIBFDT_OVERLAY=y
> +CONFIG_BCM283X_PL011_SERIAL=y
> diff --git a/drivers/serial/serial_bcm283x_pl011.c
> b/drivers/serial/serial_bcm283x_pl011.c
> index 2527bb8..75b7ed5 100644
> --- a/drivers/serial/serial_bcm283x_pl011.c
> +++ b/drivers/serial/serial_bcm283x_pl011.c
> @@ -11,24 +11,142 @@
>  #include <serial.h>
>  #include "serial_pl01x_internal.h"
> 
> +#define    UART0_GPIO_RX    15
> +#define    UART0_GPIO_TX    14
> +
> +/* required pin vs function list for the PL011 UARTs */
> +
> +static const struct pin_func_map {
> +    int pin;    /* GPIO pin number */
> +    int function;    /* GPIO function */
> +} uart_pin_functions[] = {
> +    { .pin = UART0_GPIO_TX, .function = BCM2835_GPIO_ALT0 }, /* TXD0 */
> +    { .pin = UART0_GPIO_RX, .function = BCM2835_GPIO_ALT0 }, /* RXD0 */
> +};
> +
> +static const int num_uart_pin_functions =
> +         sizeof(uart_pin_functions) / sizeof(struct pin_func_map);
> +
> +/*
> + * Check if the pin/function assignment is valid
> + *
> + * The pin/function assignment is validated against the PL011 function
> + * table to determine if the current configuration is valid for a
> + * PL011 UART.
> + *
> + * @return true if the assignment is valid, false if not
> + */
> +static bool bcm383x_check_gpio_mapping(int pin, int function)
> +{
> +    int i;
> +
> +    /* iterate through the list checking the pin assignment */
> +
> +    for (i = 0; i < num_uart_pin_functions; i++) {
> +        if (uart_pin_functions[i].pin == pin) {
> +            /* pin assignment matches, check for a valid function */
> +
> +            if (uart_pin_functions[i].function == function)
> +                return true;
> +            else
> +                return false;
> +        }
> +    }
> +
> +    /* no pin match found, the mapping is unrestricted */
> +
> +    return true;
> +}
> +
>  /*
> - * Check if this serial device is muxed
> + * Check if this serial device is available and muxed correctly
>   *
> - * The serial device will only work properly if it has been muxed to the serial
> - * pins by firmware. Check whether that happened here.
> + * The uart0 serial device will only work properly if it has been muxed to
> + * the serial pins by firmware. Check if the device is uart0 and if it is
> + * is the muxing correct.
>   *
> - * @return true if serial device is muxed, false if not
> + * @return true if serial device is usable, false if not
>   */
> -static bool bcm283x_is_serial_muxed(void)
> +static bool bcm383x_check_serial_availability(struct udevice *dev)
>  {
> -    int serial_gpio = 15;
> -    struct udevice *dev;
> +    const void *fdt = gd->fdt_blob;
> +    const fdt32_t *cell;
> +    struct udevice *pin_dev;
> +    u32 phandle;
> +    int pin_func;
> +    int pin;
> +    int offset = 0;
> +    int size;
> +    int len;
> +    int i;
> +
> +    /* get the offset for this device's pinctrl-0 phandle */
> +
> +    cell = fdt_getprop(fdt, dev_of_offset(dev), "pinctrl-0", &size);
> +    if (!cell)
> +        return false;
> +
> +    /* find the offset of the device's pinctrl-0 node */
> +
> +    while (size) {
> +        phandle = fdt32_to_cpu(*cell++);
> +        size -= sizeof(*cell);
> +
> +        offset = fdt_node_offset_by_phandle(fdt, phandle);
> +        if (offset < 0) {
> +            /* bad offset, not found */
> 
> -    if (uclass_first_device(UCLASS_PINCTRL, &dev) || !dev)
> +            return false;
> +        }
> +    }
> +
> +    /* check to make sure we found the pinctrl-0 node */
> +
> +    if (offset == 0)
>          return false;
> 
> -    if (pinctrl_get_gpio_mux(dev, 0, serial_gpio) != BCM2835_GPIO_ALT0)
> +    /* get the bcrm,pins property */
> +
> +    cell = fdt_getprop(fdt, offset, "brcm,pins", &len);
> +
> +    if (!cell) {
> +        /* no pin property, signal as unavailable */
> +
>          return false;
> +    }
> +
> +    /* find the pinctrl device */
> +
> +    if (uclass_first_device(UCLASS_PINCTRL, &pin_dev) || !pin_dev)
> +        return false;
> +
> +    /* if the pin list is empty, assume this is uart0 */
> +
> +    if (len < sizeof(int)) {
> +        /* check that the Tx GPIO pin is muxed for the PL011 */
> +
> +        pin_func = pinctrl_get_gpio_mux(pin_dev, 0, UART0_GPIO_TX);
> +        return bcm383x_check_gpio_mapping(UART0_GPIO_TX, pin_func);
> +    }
> +
> +    /* check the pin list for a valid PL011 mapping */
> +
> +    for (i = 0; i < len / sizeof(int); i++) {
> +        /* get the function assigned to this pin */
> +
> +        pin = fdt32_to_cpu(cell[i]);
> +        pin_func = pinctrl_get_gpio_mux(pin_dev, 0, pin);
> +
> +        /* check the pin/function assignment */
> +
> +        if (bcm383x_check_gpio_mapping(pin, pin_func) == false) {
> +            /* any failed pin checks makes the device unavailable */
> +
> +            return false;
> +        }
> +    }
> +
> +    /* all checks pass, indicate device is available */
> 
>      return true;
>  }
> @@ -38,19 +156,19 @@ static int bcm283x_pl011_serial_ofdata_to_platdata(struct
> udevice *dev)
>      struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
>      int ret;
> 
> -    /* Don't spawn the device if it's not muxed */
> -    if (!bcm283x_is_serial_muxed())
> +    /* don't spawn the device if it's not usable */
> +
> +    if (bcm383x_check_serial_availability(dev) == false)
>          return -ENODEV;
> 
>      ret = pl01x_serial_ofdata_to_platdata(dev);
>      if (ret)
>          return ret;
> 
> -    /*
> -     * TODO: Reinitialization doesn't always work for now, just skip
> -     *       init always - we know we're already initialized
> -     */
> -    plat->skip_init = true;
> +    /* default clock to 48MHz if the device tree didn't specificy a freq */
> +
> +    if (plat->clock == 1)
> +        plat->clock = 48000000;
> 
>      return 0;
>  }
Mark Dapoz Nov. 18, 2019, 6:46 a.m. UTC | #2
> On 19-Nov-8, at 12:23 PM, Matthias Brugger <matthias.bgg@gmail.com> wrote:
> 
> On 27/09/2019 07:39, Mark Dapoz wrote:
>> The bcm283x PL011 serial driver is hard coded to allow connections only
>> for UART0.  This prevents using any of the other UARTs as the U-boot
>> console.
>> 
>> The bcm283x serial driver is updated to allow connections to any of the
>> PL011 devices.  The initialization logic has been updated as follows:
>> 
>>     - a GPIO pinlist is extracted from the device tree for the device
>>     - each pin is compared against a function assignment table
>>     - if the pin is found in the table and the currently assigned GPIO
>>       function doesn't match, the device is unavailable
>>     - if the function matches the table entry or the pin is not listed
>>       the table, the device is available
>> 
>> The function assignment table contains entries for UART0 to ensure it
>> is multiplexed to GPIO pins 14 and 15.
>> 
> 
> Which is what we already check right now, correct?

Yes, the existing behaviour is maintained.

> 
>> This logic allows GPIO 14 and 15 to be multiplexed between the miniUART
>> and the PL011 UART by the board's loader.
>> 
> 
> Maybe I'm too tired, but I don't see where this is done. I can just see that you
> changed the function to check if PL011 uses GPIO15 as ALT0 to a different
> function which checks the same. Can you please explain.

The pin list for GPIO14 and GPIO15 is checked if they are using ALT0 before
indicating that PL011 is available since that’s the only PL011 device that is
multiplexed with another serial port.  All other PL011 devices are assumed to be
available and the function returns true.   The DTS will determine which PL011’s
are enabled by setting the “status” property.

> 
>> An error in the default clock rate has also been fixed.  If the device
>> tree entry for the serial device doesn't specify a "clocks" property,
>> the default frequency of 1Hz was used.  The default has been changed
>> to 48MHz.
> 
> If doesn't that mean that we don't have a valid DTS?
> If this still makes sense then please split this out into a separate patch.

The generic PL011 driver in drivers/serial/serial_pl01x.c is looking for the
“clock” property but the DTS for the Pi has a “clocks” property instead.
When the driver can’t find the “clock” property, it uses a default value of 1Hz.
The PL011 driver wrapper for the Pi just changes it to 48MHz.

-mark
diff mbox series

Patch

diff --git a/configs/rpi_4_32b_defconfig b/configs/rpi_4_32b_defconfig
index dc69690..cd66b0b 100644
--- a/configs/rpi_4_32b_defconfig
+++ b/configs/rpi_4_32b_defconfig
@@ -31,3 +31,4 @@  CONFIG_SYS_WHITE_ON_BLACK=y
  CONFIG_CONSOLE_SCROLL_LINES=10
  CONFIG_PHYS_TO_BUS=y
  CONFIG_OF_LIBFDT_OVERLAY=y
+CONFIG_BCM283X_PL011_SERIAL=y
diff --git a/configs/rpi_4_defconfig b/configs/rpi_4_defconfig
index 2d63197..9f862c4 100644
--- a/configs/rpi_4_defconfig
+++ b/configs/rpi_4_defconfig
@@ -31,3 +31,4 @@  CONFIG_SYS_WHITE_ON_BLACK=y
  CONFIG_CONSOLE_SCROLL_LINES=10
  CONFIG_PHYS_TO_BUS=y
  CONFIG_OF_LIBFDT_OVERLAY=y
+CONFIG_BCM283X_PL011_SERIAL=y
diff --git a/drivers/serial/serial_bcm283x_pl011.c b/drivers/serial/serial_bcm283x_pl011.c
index 2527bb8..75b7ed5 100644
--- a/drivers/serial/serial_bcm283x_pl011.c
+++ b/drivers/serial/serial_bcm283x_pl011.c
@@ -11,24 +11,142 @@ 
  #include <serial.h>
  #include "serial_pl01x_internal.h"

+#define	UART0_GPIO_RX	15
+#define	UART0_GPIO_TX	14
+
+/* required pin vs function list for the PL011 UARTs */
+
+static const struct pin_func_map {
+	int pin;	/* GPIO pin number */
+	int function;	/* GPIO function */
+} uart_pin_functions[] = {
+	{ .pin = UART0_GPIO_TX, .function = BCM2835_GPIO_ALT0 }, /* TXD0 */
+	{ .pin = UART0_GPIO_RX, .function = BCM2835_GPIO_ALT0 }, /* RXD0 */
+};
+
+static const int num_uart_pin_functions =
+		 sizeof(uart_pin_functions) / sizeof(struct pin_func_map);
+
+/*
+ * Check if the pin/function assignment is valid
+ *
+ * The pin/function assignment is validated against the PL011 function
+ * table to determine if the current configuration is valid for a
+ * PL011 UART.
+ *
+ * @return true if the assignment is valid, false if not
+ */
+static bool bcm383x_check_gpio_mapping(int pin, int function)
+{
+	int i;
+
+	/* iterate through the list checking the pin assignment */
+
+	for (i = 0; i < num_uart_pin_functions; i++) {
+		if (uart_pin_functions[i].pin == pin) {
+			/* pin assignment matches, check for a valid function */
+
+			if (uart_pin_functions[i].function == function)
+				return true;
+			else
+				return false;
+		}
+	}
+
+	/* no pin match found, the mapping is unrestricted */
+
+	return true;
+}
+
  /*
- * Check if this serial device is muxed
+ * Check if this serial device is available and muxed correctly
   *
- * The serial device will only work properly if it has been muxed to the serial
- * pins by firmware. Check whether that happened here.
+ * The uart0 serial device will only work properly if it has been muxed to
+ * the serial pins by firmware. Check if the device is uart0 and if it is
+ * is the muxing correct.
   *
- * @return true if serial device is muxed, false if not
+ * @return true if serial device is usable, false if not
   */
-static bool bcm283x_is_serial_muxed(void)
+static bool bcm383x_check_serial_availability(struct udevice *dev)
  {
-	int serial_gpio = 15;
-	struct udevice *dev;
+	const void *fdt = gd->fdt_blob;
+	const fdt32_t *cell;
+	struct udevice *pin_dev;
+	u32 phandle;
+	int pin_func;
+	int pin;
+	int offset = 0;
+	int size;
+	int len;
+	int i;
+
+	/* get the offset for this device's pinctrl-0 phandle */
+
+	cell = fdt_getprop(fdt, dev_of_offset(dev), "pinctrl-0", &size);
+	if (!cell)
+		return false;
+
+	/* find the offset of the device's pinctrl-0 node */
+
+	while (size) {
+		phandle = fdt32_to_cpu(*cell++);
+		size -= sizeof(*cell);
+
+		offset = fdt_node_offset_by_phandle(fdt, phandle);
+		if (offset < 0) {
+			/* bad offset, not found */

-	if (uclass_first_device(UCLASS_PINCTRL, &dev) || !dev)
+			return false;
+		}
+	}
+
+	/* check to make sure we found the pinctrl-0 node */
+
+	if (offset == 0)
  		return false;

-	if (pinctrl_get_gpio_mux(dev, 0, serial_gpio) != BCM2835_GPIO_ALT0)
+	/* get the bcrm,pins property */
+
+	cell = fdt_getprop(fdt, offset, "brcm,pins", &len);
+
+	if (!cell) {
+		/* no pin property, signal as unavailable */
+
  		return false;
+	}
+
+	/* find the pinctrl device */
+
+	if (uclass_first_device(UCLASS_PINCTRL, &pin_dev) || !pin_dev)
+		return false;
+
+	/* if the pin list is empty, assume this is uart0 */
+
+	if (len < sizeof(int)) {
+		/* check that the Tx GPIO pin is muxed for the PL011 */
+
+		pin_func = pinctrl_get_gpio_mux(pin_dev, 0, UART0_GPIO_TX);
+		return bcm383x_check_gpio_mapping(UART0_GPIO_TX, pin_func);
+	}
+
+	/* check the pin list for a valid PL011 mapping */
+
+	for (i = 0; i < len / sizeof(int); i++) {
+		/* get the function assigned to this pin */
+
+		pin = fdt32_to_cpu(cell[i]);
+		pin_func = pinctrl_get_gpio_mux(pin_dev, 0, pin);
+
+		/* check the pin/function assignment */
+
+		if (bcm383x_check_gpio_mapping(pin, pin_func) == false) {
+			/* any failed pin checks makes the device unavailable */
+
+			return false;
+		}
+	}
+
+	/* all checks pass, indicate device is available */

  	return true;
  }
@@ -38,19 +156,19 @@  static int bcm283x_pl011_serial_ofdata_to_platdata(struct udevice *dev)
  	struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
  	int ret;

-	/* Don't spawn the device if it's not muxed */
-	if (!bcm283x_is_serial_muxed())
+	/* don't spawn the device if it's not usable */
+
+	if (bcm383x_check_serial_availability(dev) == false)
  		return -ENODEV;

  	ret = pl01x_serial_ofdata_to_platdata(dev);
  	if (ret)
  		return ret;

-	/*
-	 * TODO: Reinitialization doesn't always work for now, just skip
-	 *       init always - we know we're already initialized
-	 */
-	plat->skip_init = true;
+	/* default clock to 48MHz if the device tree didn't specificy a freq */
+
+	if (plat->clock == 1)
+		plat->clock = 48000000;

  	return 0;
  }