diff mbox series

arm64: tegra: Fix HSUART for Smaug

Message ID 20230714101018.10617-1-diogo.ivo@tecnico.ulisboa.pt
State Accepted
Headers show
Series arm64: tegra: Fix HSUART for Smaug | expand

Commit Message

Diogo Ivo July 14, 2023, 10:10 a.m. UTC
After commit 71de0a054d0e ("arm64: tegra: Drop serial clock-names and
reset-names") was applied, the HSUART failed to probe and the following
error is seen:

 serial-tegra 70006300.serial: Couldn't get the reset
 serial-tegra: probe of 70006300.serial failed with error -2

Commit 71de0a054d0e ("arm64: tegra: Drop serial clock-names and
reset-names") is correct because the "reset-names" property is not
needed for 8250 UARTs. However, the "reset-names" is required for the
HSUART and should have been populated as part of commit a63c0cd83720c
("arm64: dts: tegra: smaug: Add Bluetooth node") that enabled the HSUART
for the Pixel C. Fix this by populating the "reset-names" property for
the HSUART on the Pixel C.

Fixes: a63c0cd83720 ("arm64: dts: tegra: smaug: Add Bluetooth node")
Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
---
 arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 1 +
 1 file changed, 1 insertion(+)

Comments

Jon Hunter July 14, 2023, 10:21 a.m. UTC | #1
On 14/07/2023 11:10, Diogo Ivo wrote:
> After commit 71de0a054d0e ("arm64: tegra: Drop serial clock-names and
> reset-names") was applied, the HSUART failed to probe and the following
> error is seen:
> 
>   serial-tegra 70006300.serial: Couldn't get the reset
>   serial-tegra: probe of 70006300.serial failed with error -2
> 
> Commit 71de0a054d0e ("arm64: tegra: Drop serial clock-names and
> reset-names") is correct because the "reset-names" property is not
> needed for 8250 UARTs. However, the "reset-names" is required for the
> HSUART and should have been populated as part of commit a63c0cd83720c
> ("arm64: dts: tegra: smaug: Add Bluetooth node") that enabled the HSUART
> for the Pixel C. Fix this by populating the "reset-names" property for
> the HSUART on the Pixel C.
> 
> Fixes: a63c0cd83720 ("arm64: dts: tegra: smaug: Add Bluetooth node")
> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> ---
>   arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> index 2c608d645642..bcb533cc002c 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> @@ -1364,6 +1364,7 @@ serial@70006000 {
>   
>   	uartd: serial@70006300 {
>   		compatible = "nvidia,tegra30-hsuart";
> +		reset-names = "serial";
>   		status = "okay";
>   
>   		bluetooth {


Thanks!

Reviewed-by: Jon Hunter <jonathanh@nvidia.com>

Jon
Thierry Reding July 14, 2023, 2:41 p.m. UTC | #2
On Fri, Jul 14, 2023 at 11:10:17AM +0100, Diogo Ivo wrote:
> After commit 71de0a054d0e ("arm64: tegra: Drop serial clock-names and
> reset-names") was applied, the HSUART failed to probe and the following
> error is seen:
> 
>  serial-tegra 70006300.serial: Couldn't get the reset
>  serial-tegra: probe of 70006300.serial failed with error -2
> 
> Commit 71de0a054d0e ("arm64: tegra: Drop serial clock-names and
> reset-names") is correct because the "reset-names" property is not
> needed for 8250 UARTs. However, the "reset-names" is required for the
> HSUART and should have been populated as part of commit a63c0cd83720c
> ("arm64: dts: tegra: smaug: Add Bluetooth node") that enabled the HSUART
> for the Pixel C. Fix this by populating the "reset-names" property for
> the HSUART on the Pixel C.
> 
> Fixes: a63c0cd83720 ("arm64: dts: tegra: smaug: Add Bluetooth node")
> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> ---
>  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> index 2c608d645642..bcb533cc002c 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> @@ -1364,6 +1364,7 @@ serial@70006000 {
>  
>  	uartd: serial@70006300 {
>  		compatible = "nvidia,tegra30-hsuart";
> +		reset-names = "serial";
>  		status = "okay";
>  
>  		bluetooth {

FWIW, we need to do this for a whole bunch of devices. I've got a local
patch for all the cases that allows schema validation. I'll pull in your
patch and then rebase mine on top and send it out.

Thanks,
Thierry
Thierry Reding July 14, 2023, 2:52 p.m. UTC | #3
From: Thierry Reding <treding@nvidia.com>


On Fri, 14 Jul 2023 11:10:17 +0100, Diogo Ivo wrote:
> After commit 71de0a054d0e ("arm64: tegra: Drop serial clock-names and
> reset-names") was applied, the HSUART failed to probe and the following
> error is seen:
> 
>  serial-tegra 70006300.serial: Couldn't get the reset
>  serial-tegra: probe of 70006300.serial failed with error -2
> 
> [...]

Applied, thanks!

[1/1] arm64: tegra: Fix HSUART for Smaug
      commit: 590bfe51838f6345a6a3288507661dc9b7208464

Best regards,
Krzysztof Kozlowski July 17, 2023, 8:03 a.m. UTC | #4
On 14/07/2023 12:10, Diogo Ivo wrote:
> After commit 71de0a054d0e ("arm64: tegra: Drop serial clock-names and
> reset-names") was applied, the HSUART failed to probe and the following
> error is seen:
> 
>  serial-tegra 70006300.serial: Couldn't get the reset
>  serial-tegra: probe of 70006300.serial failed with error -2
> 
> Commit 71de0a054d0e ("arm64: tegra: Drop serial clock-names and
> reset-names") is correct because the "reset-names" property is not
> needed for 8250 UARTs. However, the "reset-names" is required for the
> HSUART and should have been populated as part of commit a63c0cd83720c
> ("arm64: dts: tegra: smaug: Add Bluetooth node") that enabled the HSUART
> for the Pixel C. Fix this by populating the "reset-names" property for
> the HSUART on the Pixel C.
> 
> Fixes: a63c0cd83720 ("arm64: dts: tegra: smaug: Add Bluetooth node")
> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> ---
>  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> index 2c608d645642..bcb533cc002c 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> @@ -1364,6 +1364,7 @@ serial@70006000 {
>  
>  	uartd: serial@70006300 {
>  		compatible = "nvidia,tegra30-hsuart";
> +		reset-names = "serial";

Why reset-names is board specific? This makes little sense. If you need
reset-names, then it is part of SoC.

Best regards,
Krzysztof
Thierry Reding July 18, 2023, 7:59 a.m. UTC | #5
On Mon, Jul 17, 2023 at 10:03:19AM +0200, Krzysztof Kozlowski wrote:
> On 14/07/2023 12:10, Diogo Ivo wrote:
> > After commit 71de0a054d0e ("arm64: tegra: Drop serial clock-names and
> > reset-names") was applied, the HSUART failed to probe and the following
> > error is seen:
> > 
> >  serial-tegra 70006300.serial: Couldn't get the reset
> >  serial-tegra: probe of 70006300.serial failed with error -2
> > 
> > Commit 71de0a054d0e ("arm64: tegra: Drop serial clock-names and
> > reset-names") is correct because the "reset-names" property is not
> > needed for 8250 UARTs. However, the "reset-names" is required for the
> > HSUART and should have been populated as part of commit a63c0cd83720c
> > ("arm64: dts: tegra: smaug: Add Bluetooth node") that enabled the HSUART
> > for the Pixel C. Fix this by populating the "reset-names" property for
> > the HSUART on the Pixel C.
> > 
> > Fixes: a63c0cd83720 ("arm64: dts: tegra: smaug: Add Bluetooth node")
> > Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> > ---
> >  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > index 2c608d645642..bcb533cc002c 100644
> > --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > @@ -1364,6 +1364,7 @@ serial@70006000 {
> >  
> >  	uartd: serial@70006300 {
> >  		compatible = "nvidia,tegra30-hsuart";
> > +		reset-names = "serial";
> 
> Why reset-names is board specific? This makes little sense. If you need
> reset-names, then it is part of SoC.

As the commit message explains, this is because we have two
"conflicting" device tree bindings for these devices. One is a standard
UART, typically used when the device is a debug serial console and a
more specialized high-speed UART which then requires additional
properties.

Effectively this means that whenever we override the standard UART with
the high-speed UART we need to extend the device tree with those
additional properties. We actually have similar issues with dmas and
dma-names, which are only valid for the HS UART and I have a couple of
patches[0][1] to clean that all up. It's a bit weird because we
basically have a patch that removes reset-names (for standard UART) and
then we have another patch that adds the reset-names (for HS UART), but
I don't see a way around it given the bindings that we have.

Thierry

[0]: http://patchwork.ozlabs.org/project/linux-tegra/list/?series=364269
[1]: http://patchwork.ozlabs.org/project/linux-tegra/list/?series=364270
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
index 2c608d645642..bcb533cc002c 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
@@ -1364,6 +1364,7 @@  serial@70006000 {
 
 	uartd: serial@70006300 {
 		compatible = "nvidia,tegra30-hsuart";
+		reset-names = "serial";
 		status = "okay";
 
 		bluetooth {