serial: ns16550: Fix serial output on Tegra186

PATCH New Thierry Reding 4 participants 7 messages Sep 30, 2016 – Oct 3, 2016

lore

Share & tools

lore

Thierry Reding

author

2016-09-30 08:46 UTC #0 lore raw

PATCH 1/1 serial: ns16550: Fix serial output on Tegra186

From: Thierry Reding <[email protected]>

For Tegra186 there are currently no UART clocks wired up in device tree. This exposes a regression introduced in commit 50fce1d5d874 ("serial:
ns16550: Support clocks via phandle"), which causes the p2771-0000-500 board (and probably any Tegra186-based board as well) to fail to boot.

The reason is that if no clocks property exists, then clk_get_by_index() returns -ENOENT (via fdtdec_parse_phandle_with_args()) rather than -ENODEV as the above-mentioned commit expects.

Fix this by checking for the right error code.

Reported-by Alexandre Courbot <[email protected]>

Signed-off-by Thierry Reding <[email protected]>

---
 drivers/serial/ns16550.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/serial/ns16550.c +1 -1

index 765499dab646..9c36dbe2a566 100644
@@ -408,7 +408,7 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev)
 		err = clk_get_rate(&clk);
 		if (!IS_ERR_VALUE(err))
 			plat->clock = err;
-	} else if (err != -ENODEV && err != -ENOSYS) {
+	} else if (err != -ENOENT && err != -ENOSYS) {
 		debug("ns16550 failed to get clock\n");
 		return err;
 	}

Signature · 1 line

2.10.0

Alexandre Courbot

2016-09-30 08:53 UTC #1 lore raw

PATCH 1/1 serial: ns16550: Fix serial output on Tegra186

Quoted 7 lines · Thierry Reding

From: Thierry Reding <[email protected]>
For Tegra186 there are currently no UART clocks wired up in device tree. This exposes a regression introduced in commit 50fce1d5d874 ("serial:
ns16550: Support clocks via phandle"), which causes the p2771-0000-500 board (and probably any Tegra186-based board as well) to fail to boot.

Quoted 5 lines

The reason is that if no clocks property exists, then clk_get_by_index() returns -ENOENT (via fdtdec_parse_phandle_with_args()) rather than -ENODEV as the above-mentioned commit expects.
Fix this by checking for the right error code.

Tested-by Alexandre Courbot <[email protected]>

I sent a similar patch ~10 minutes before this one, but Thierry's commit message is clearer than mine (and his handling of -ENODEV probably more correct as well), so let's go with this version!

Paul Burton

2016-09-30 09:47 UTC #2 lore raw

PATCH 1/1 serial: ns16550: Fix serial output on Tegra186

Quoted 12 lines · On Friday, 30 September 2016 17:53:38 BST Alexandre Courbot wrote:

Quoted 11 lines · Thierry Reding
From: Thierry Reding <[email protected]>
For Tegra186 there are currently no UART clocks wired up in device tree. This exposes a regression introduced in commit 50fce1d5d874 ("serial:
ns16550: Support clocks via phandle"), which causes the p2771-0000-500 board (and probably any Tegra186-based board as well) to fail to boot.
The reason is that if no clocks property exists, then clk_get_by_index() returns -ENOENT (via fdtdec_parse_phandle_with_args()) rather than -ENODEV as the above-mentioned commit expects.

Quoted 7 lines

Quoted 1 line
Fix this by checking for the right error code.
Tested-by Alexandre Courbot <[email protected]>
I sent a similar patch ~10 minutes before this one, but Thierry's commit message is clearer than mine (and his handling of -ENODEV probably more correct as well), so let's go with this version!

Hi Thierry & Alexandre,

Apologies for the breakage!

If a DT contains a clock & the UART node references it by phandle then I believe clk_get_by_index() could return -ENODEV via uclass_get_device_by_of_offset & uclass_find_device_by_of_offset. So we probably need to handle both -ENOENT for the "no clocks property" case & - ENODEV for the "clocks property but no driver" case, as Alexandre's patch does?

Thanks,
Paul
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160930/43799725/attachment.sig>

Alexandre Courbot

2016-09-30 09:57 UTC #3 lore raw

PATCH 1/1 serial: ns16550: Fix serial output on Tegra186

Hi Paul,

Quoted 26 lines · Paul Burton

* PGP Signed by an unknown key
Quoted 12 lines · On Friday, 30 September 2016 17:53:38 BST Alexandre Courbot wrote:
Quoted 11 lines · Thierry Reding
From: Thierry Reding <[email protected]>
For Tegra186 there are currently no UART clocks wired up in device tree. This exposes a regression introduced in commit 50fce1d5d874 ("serial:
ns16550: Support clocks via phandle"), which causes the p2771-0000-500 board (and probably any Tegra186-based board as well) to fail to boot.
The reason is that if no clocks property exists, then clk_get_by_index() returns -ENOENT (via fdtdec_parse_phandle_with_args()) rather than -ENODEV as the above-mentioned commit expects.
Quoted 7 lines
Quoted 1 line
Fix this by checking for the right error code.
Tested-by Alexandre Courbot <[email protected]>
I sent a similar patch ~10 minutes before this one, but Thierry's commit message is clearer than mine (and his handling of -ENODEV probably more correct as well), so let's go with this version!
Hi Thierry & Alexandre,

Quoted 8 lines

Apologies for the breakage!
If a DT contains a clock & the UART node references it by phandle then I believe clk_get_by_index() could return -ENODEV via uclass_get_device_by_of_offset & uclass_find_device_by_of_offset. So we probably need to handle both -ENOENT for the "no clocks property" case & - ENODEV for the "clocks property but no driver" case, as Alexandre's patch does?

But if a DT contains a clock, shouldn't it simply take precedence over the legacy method? (not very familiar with this code so my comment may not make sense).

IOW, do we have a case where clk_get_by_index() returns -ENODEV and fdtdec_get_int() returns a valid clock?

Thierry Reding

author

2016-09-30 10:32 UTC #4 lore raw

PATCH 1/1 serial: ns16550: Fix serial output on Tegra186

In reply to Paul Burton

Quoted 24 lines · Paul Burton

Quoted 12 lines · On Friday, 30 September 2016 17:53:38 BST Alexandre Courbot wrote:
Quoted 11 lines · Thierry Reding
From: Thierry Reding <[email protected]>
For Tegra186 there are currently no UART clocks wired up in device tree. This exposes a regression introduced in commit 50fce1d5d874 ("serial:
ns16550: Support clocks via phandle"), which causes the p2771-0000-500 board (and probably any Tegra186-based board as well) to fail to boot.
The reason is that if no clocks property exists, then clk_get_by_index() returns -ENOENT (via fdtdec_parse_phandle_with_args()) rather than -ENODEV as the above-mentioned commit expects.
Quoted 7 lines
Quoted 1 line
Fix this by checking for the right error code.
Tested-by Alexandre Courbot <[email protected]>
I sent a similar patch ~10 minutes before this one, but Thierry's commit message is clearer than mine (and his handling of -ENODEV probably more correct as well), so let's go with this version!
Hi Thierry & Alexandre,

Quoted 8 lines

Apologies for the breakage!
If a DT contains a clock & the UART node references it by phandle then I believe clk_get_by_index() could return -ENODEV via uclass_get_device_by_of_offset & uclass_find_device_by_of_offset. So we probably need to handle both -ENOENT for the "no clocks property" case & - ENODEV for the "clocks property but no driver" case, as Alexandre's patch does?

Indeed. I thought I had checked all possible return values, but I've obviously been blind. Alex's patch looks more correct, though I'm beginning to wonder if there's a better way to achieve this than keep adding whatever error codes happen to be special cases.

How about we just leave out the else completely? At this point it seems like any failure to obtain a valid clock would best be dealt with by falling back to clock-frequency or the one specified in the config.

Are there even other errors besides -ENODEV, -ENOENT and -ENOSYS that we could get here?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160930/4a059803/attachment.sig>

Stephen Warren

2016-10-03 15:51 UTC #5 lore raw

PATCH 1/1 serial: ns16550: Fix serial output on Tegra186

In reply to Thierry Reding

Quoted 7 lines · Thierry Reding

From: Thierry Reding <[email protected]>
For Tegra186 there are currently no UART clocks wired up in device tree. This exposes a regression introduced in commit 50fce1d5d874 ("serial:
ns16550: Support clocks via phandle"), which causes the p2771-0000-500 board (and probably any Tegra186-based board as well) to fail to boot.

Quoted 5 lines

The reason is that if no clocks property exists, then clk_get_by_index() returns -ENOENT (via fdtdec_parse_phandle_with_args()) rather than -ENODEV as the above-mentioned commit expects.
Fix this by checking for the right error code.

Tested-by Stephen Warren <[email protected]>

Stephen Warren

2016-10-03 16:14 UTC #6 lore raw

PATCH 1/1 serial: ns16550: Fix serial output on Tegra186

Quoted 8 lines · Stephen Warren

Quoted 7 lines · Thierry Reding
From: Thierry Reding <[email protected]>
For Tegra186 there are currently no UART clocks wired up in device tree. This exposes a regression introduced in commit 50fce1d5d874 ("serial:
ns16550: Support clocks via phandle"), which causes the p2771-0000-500 board (and probably any Tegra186-based board as well) to fail to boot.

Quoted 7 lines

Quoted 5 lines
The reason is that if no clocks property exists, then clk_get_by_index() returns -ENOENT (via fdtdec_parse_phandle_with_args()) rather than -ENODEV as the above-mentioned commit expects.
Fix this by checking for the right error code.
Tested-by Stephen Warren <[email protected]>

Actually, I take that back. This works great for Tegra186 but breaks earlier Tegra SoCs.