[U-Boot] [PATCH] serial: ns16550: Fix serial output on Tegra186

[U-Boot] [PATCH] serial: ns16550: Fix serial output on Tegra186

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

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 <acourbot@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/serial/ns16550.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 765499dab646..9c36dbe2a566 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -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;
 	}
-- 
2.10.0

[U-Boot] [PATCH] serial: ns16550: Fix serial output on Tegra186

[Alexandre Courbot]

On 09/30/2016 05:46 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> 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.

Tested-by: Alexandre Courbot <acourbot@nvidia.com>

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!

[U-Boot] [PATCH] serial: ns16550: Fix serial output on Tegra186

[Paul Burton]

On Friday, 30 September 2016 17:53:38 BST Alexandre Courbot wrote:
> On 09/30/2016 05:46 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > 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.
> 
> Tested-by: Alexandre Courbot <acourbot@nvidia.com>
> 
> 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>

[U-Boot] [PATCH] serial: ns16550: Fix serial output on Tegra186

[Alexandre Courbot]

Hi Paul,

On 09/30/2016 06:47 PM, Paul Burton wrote:
> * PGP Signed by an unknown key
> 
> On Friday, 30 September 2016 17:53:38 BST Alexandre Courbot wrote:
>> On 09/30/2016 05:46 PM, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> 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.
>>
>> Tested-by: Alexandre Courbot <acourbot@nvidia.com>
>>
>> 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?

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?

[U-Boot] [PATCH] serial: ns16550: Fix serial output on Tegra186

[Thierry Reding]

On Fri, Sep 30, 2016 at 10:47:38AM +0100, Paul Burton wrote:
> On Friday, 30 September 2016 17:53:38 BST Alexandre Courbot wrote:
> > On 09/30/2016 05:46 PM, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > 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.
> > 
> > Tested-by: Alexandre Courbot <acourbot@nvidia.com>
> > 
> > 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?

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>

[U-Boot] [PATCH] serial: ns16550: Fix serial output on Tegra186

[Stephen Warren]

On 09/30/2016 02:46 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> 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.

Tested-by: Stephen Warren <swarren@nvidia.com>

[U-Boot] [PATCH] serial: ns16550: Fix serial output on Tegra186

[Stephen Warren]

On 10/03/2016 09:51 AM, Stephen Warren wrote:
> On 09/30/2016 02:46 AM, Thierry Reding wrote:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> 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.
>
> Tested-by: Stephen Warren <swarren@nvidia.com>

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