| Message ID | 20191108130123.6839-33-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | QUICC Engine support on ARM and ARM64 | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (85c5b0984ebb104ec7a0a853ec1e63c19f500313) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 94 lines checked |
On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > - if (*iprop) > - qe_port->port.uartclk = *iprop; > + if (val) > + qe_port->port.uartclk = val; > else { > /* > * Older versions of U-Boot do not initialize the brg-frequency > * property, so in this case we assume the BRG frequency is > * half the QE bus frequency. > */ This bug in older U-Boots is definitely PowerPC-specific, so could you change this so that it reports an error on ARM if brg-frequency is zero, and displays a warning on PowerPC?
On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > + if (of_property_read_u32(np, "cell-index", &val) && > + of_property_read_u32(np, "device-id", &val)) { I know that this is technically correct, but it's obfuscated IMHO. 'val' is set correctly only when of_property_read_u32(...) is "false", which is doubly-weird because of_property_read_u32(...) doesn't actually return a boolean. I would rather you break this into two if-statements like the original code.
On 15/11/2019 05.25, Timur Tabi wrote: > On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: >> >> + if (of_property_read_u32(np, "cell-index", &val) && >> + of_property_read_u32(np, "device-id", &val)) { > > I know that this is technically correct, but it's obfuscated IMHO. > 'val' is set correctly only when of_property_read_u32(...) is "false", > which is doubly-weird because of_property_read_u32(...) doesn't > actually return a boolean. > > I would rather you break this into two if-statements like the original code. > Sure, I can do that.
On 14/11/2019 14.57, Timur Tabi wrote: > On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: >> >> - if (*iprop) >> - qe_port->port.uartclk = *iprop; >> + if (val) >> + qe_port->port.uartclk = val; >> else { >> /* >> * Older versions of U-Boot do not initialize the brg-frequency >> * property, so in this case we assume the BRG frequency is >> * half the QE bus frequency. >> */ > > This bug in older U-Boots is definitely PowerPC-specific, so could you > change this so that it reports an error on ARM if brg-frequency is > zero, and displays a warning on PowerPC? > That would be a separate patch, this patch is only concerned with eliminating the implicit assumption of the host being big-endian. And there's already been some pushback to adding arch-specific ifdefs (which I agree with, but as I responded there see as the lesser evil), so unless there's a very good reason to add that complexity, I'd rather not. Rasmus
On 11/15/19 2:01 AM, Rasmus Villemoes wrote: > That would be a separate patch, this patch is only concerned with > eliminating the implicit assumption of the host being big-endian. And > there's already been some pushback to adding arch-specific ifdefs (which > I agree with, but as I responded there see as the lesser evil), so > unless there's a very good reason to add that complexity, I'd rather not. We don't want to encourage people to introduce device trees that don't have the brg-frequency property in them.
On Fri, 2019-11-15 at 08:35 -0600, Timur Tabi wrote: > On 11/15/19 2:01 AM, Rasmus Villemoes wrote: > > That would be a separate patch, this patch is only concerned with > > eliminating the implicit assumption of the host being big-endian. And > > there's already been some pushback to adding arch-specific ifdefs (which > > I agree with, but as I responded there see as the lesser evil), so > > unless there's a very good reason to add that complexity, I'd rather not. > > We don't want to encourage people to introduce device trees that don't > have the brg-frequency property in them. Yeah, workarounds like this should be as targeted as possible. If we knew the specific chips/boards on which U-Boot has this problem, then limiting it to those would have been even better (e.g. fix up the device tree from the platform code), but at this point containing the damage to PPC seems like the most reasonable approach. It's not relevant to this specific patch, but it is relevant to a patchset expanding the set of platforms on which this code builds. -Scott
diff --git a/drivers/tty/serial/ucc_uart.c b/drivers/tty/serial/ucc_uart.c index 313697842e24..f5ea84928a3b 100644 --- a/drivers/tty/serial/ucc_uart.c +++ b/drivers/tty/serial/ucc_uart.c @@ -1256,10 +1256,10 @@ static int soft_uart_init(struct platform_device *ofdev) static int ucc_uart_probe(struct platform_device *ofdev) { struct device_node *np = ofdev->dev.of_node; - const unsigned int *iprop; /* Integer OF properties */ const char *sprop; /* String OF properties */ struct uart_qe_port *qe_port = NULL; struct resource res; + u32 val; int ret; /* @@ -1290,23 +1290,19 @@ static int ucc_uart_probe(struct platform_device *ofdev) /* Get the UCC number (device ID) */ /* UCCs are numbered 1-7 */ - iprop = of_get_property(np, "cell-index", NULL); - if (!iprop) { - iprop = of_get_property(np, "device-id", NULL); - if (!iprop) { - dev_err(&ofdev->dev, "UCC is unspecified in " - "device tree\n"); - ret = -EINVAL; - goto out_free; - } + if (of_property_read_u32(np, "cell-index", &val) && + of_property_read_u32(np, "device-id", &val)) { + dev_err(&ofdev->dev, "UCC is unspecified in device tree\n"); + ret = -EINVAL; + goto out_free; } - if ((*iprop < 1) || (*iprop > UCC_MAX_NUM)) { - dev_err(&ofdev->dev, "no support for UCC%u\n", *iprop); + if (val < 1 || val > UCC_MAX_NUM) { + dev_err(&ofdev->dev, "no support for UCC%u\n", val); ret = -ENODEV; goto out_free; } - qe_port->ucc_num = *iprop - 1; + qe_port->ucc_num = val - 1; /* * In the future, we should not require the BRG to be specified in the @@ -1350,13 +1346,12 @@ static int ucc_uart_probe(struct platform_device *ofdev) } /* Get the port number, numbered 0-3 */ - iprop = of_get_property(np, "port-number", NULL); - if (!iprop) { + if (of_property_read_u32(np, "port-number", &val)) { dev_err(&ofdev->dev, "missing port-number in device tree\n"); ret = -EINVAL; goto out_free; } - qe_port->port.line = *iprop; + qe_port->port.line = val; if (qe_port->port.line >= UCC_MAX_UART) { dev_err(&ofdev->dev, "port-number must be 0-%u\n", UCC_MAX_UART - 1); @@ -1386,31 +1381,29 @@ static int ucc_uart_probe(struct platform_device *ofdev) } } - iprop = of_get_property(np, "brg-frequency", NULL); - if (!iprop) { + if (of_property_read_u32(np, "brg-frequency", &val)) { dev_err(&ofdev->dev, "missing brg-frequency in device tree\n"); ret = -EINVAL; goto out_np; } - if (*iprop) - qe_port->port.uartclk = *iprop; + if (val) + qe_port->port.uartclk = val; else { /* * Older versions of U-Boot do not initialize the brg-frequency * property, so in this case we assume the BRG frequency is * half the QE bus frequency. */ - iprop = of_get_property(np, "bus-frequency", NULL); - if (!iprop) { + if (of_property_read_u32(np, "bus-frequency", &val)) { dev_err(&ofdev->dev, "missing QE bus-frequency in device tree\n"); ret = -EINVAL; goto out_np; } - if (*iprop) - qe_port->port.uartclk = *iprop / 2; + if (val) + qe_port->port.uartclk = val / 2; else { dev_err(&ofdev->dev, "invalid QE bus-frequency in device tree\n");
For this to work correctly on little-endian hosts, don't access the device-tree properties directly in native endianness, but use the of_property_read_u32() helper. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- drivers/tty/serial/ucc_uart.c | 41 +++++++++++++++-------------------- 1 file changed, 17 insertions(+), 24 deletions(-)