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 <[email protected]>
---
drivers/tty/serial/ucc_uart.c | 41 +++++++++++++++--------------------
1 file changed, 17 insertions(+), 24 deletions(-)
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");
--
2.23.0
On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes
<[email protected]> 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
<[email protected]> 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
> <[email protected]> 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
> <[email protected]> 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