[PATCH u-boot-marvell] arm64: dts: armada-3720-espressobin: fix COMPHY nodes

[PATCH u-boot-marvell] arm64: dts: armada-3720-espressobin: fix COMPHY nodes

[Marek Behún]

This commit fixes initialization of COMPHY on EspressoBin.

Commit 22f418935be4 ("phy: marvell: a3700: Use comphy_mux on Armada
37xx.") introduced usage of comphy_mux on Armada 37xx comphy driver.
The lanes are defined in comphy_a3700.c as described in functional
specification, that is:
  lane 0 is SGMII1 or USB3
  lane 1 is PCIe or SGMII0
  lane 2 is SATA or USB3

But the DTS for EspressoBin configures PCIe on lane 0 and USB3 on
lane 1, which is wrong in the sense of the specification and doesn't
work with the comphy_mux code, which is 2 years now (the aardvark driver
causes synchronous abort in U-Boot).

It worked till the above mentioned commit, because the code for powering
up PCIe PHY doesn't work with lane number at all, and the code for
powering up USB3 PHY works differently only if USB3 is on lane 2, ie.
the check goes like:
  if (lane == 2)
    something
  else
    something else
so it does not differentiate between lanes 0 and 1.

In the future I shall post patches that remove the comphy_a3700 driver
and add comphy driver which uses calls to ATF, like Linux' driver does.
This will have the advantage of same DTS bindings as Linux', but till
this is done, we need this patch.

Signed-off-by: Marek Beh?n <marek.behun@nic.cz>
Tested-by: Pali Roh?r <pali@kernel.org>
Cc: Stefan Roese <sr@denx.de>
---
 arch/arm/dts/armada-3720-espressobin.dts | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/dts/armada-3720-espressobin.dts b/arch/arm/dts/armada-3720-espressobin.dts
index 84e2c2adba..50381e979e 100644
--- a/arch/arm/dts/armada-3720-espressobin.dts
+++ b/arch/arm/dts/armada-3720-espressobin.dts
@@ -72,13 +72,13 @@
 &comphy {
 	max-lanes = <3>;
 	phy0 {
-		phy-type = <PHY_TYPE_PEX0>;
-		phy-speed = <PHY_SPEED_2_5G>;
+		phy-type = <PHY_TYPE_USB3_HOST0>;
+		phy-speed = <PHY_SPEED_5G>;
 	};
 
 	phy1 {
-		phy-type = <PHY_TYPE_USB3_HOST0>;
-		phy-speed = <PHY_SPEED_5G>;
+		phy-type = <PHY_TYPE_PEX0>;
+		phy-speed = <PHY_SPEED_2_5G>;
 	};
 
 	phy2 {
-- 
2.26.2

[EXT] [PATCH u-boot-marvell] arm64: dts: armada-3720-espressobin: fix COMPHY nodes

[Kostya Porotchkin]

> -----Original Message-----
> From: Marek Beh?n <marek.behun@nic.cz>
> Sent: Wednesday, August 19, 2020 12:57
> To: u-boot at lists.denx.de
> Cc: Kostya Porotchkin <kostap@marvell.com>; Marek Beh?n
> <marek.behun@nic.cz>; Pali Roh?r <pali@kernel.org>; Stefan Roese
> <sr@denx.de>
> Subject: [EXT] [PATCH u-boot-marvell] arm64: dts: armada-3720-espressobin:
> fix COMPHY nodes
> 
> External Email
> 
> ----------------------------------------------------------------------
> This commit fixes initialization of COMPHY on EspressoBin.
> 
> Commit 22f418935be4 ("phy: marvell: a3700: Use comphy_mux on Armada
> 37xx.") introduced usage of comphy_mux on Armada 37xx comphy driver.
> The lanes are defined in comphy_a3700.c as described in functional
> specification, that is:
>   lane 0 is SGMII1 or USB3
>   lane 1 is PCIe or SGMII0
>   lane 2 is SATA or USB3
> 
> But the DTS for EspressoBin configures PCIe on lane 0 and USB3 on lane 1,
> which is wrong in the sense of the specification and doesn't work with the
> comphy_mux code, which is 2 years now (the aardvark driver causes
> synchronous abort in U-Boot).
> 
> It worked till the above mentioned commit, because the code for powering
> up PCIe PHY doesn't work with lane number at all, and the code for powering
> up USB3 PHY works differently only if USB3 is on lane 2, ie.
> the check goes like:
>   if (lane == 2)
>     something
>   else
>     something else
> so it does not differentiate between lanes 0 and 1.
> 
> In the future I shall post patches that remove the comphy_a3700 driver and
> add comphy driver which uses calls to ATF, like Linux' driver does.
> This will have the advantage of same DTS bindings as Linux', but till this is
> done, we need this patch.
> 
> Signed-off-by: Marek Beh?n <marek.behun@nic.cz>
> Tested-by: Pali Roh?r <pali@kernel.org>
> Cc: Stefan Roese <sr@denx.de>
> ---
>  arch/arm/dts/armada-3720-espressobin.dts | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/dts/armada-3720-espressobin.dts
> b/arch/arm/dts/armada-3720-espressobin.dts
> index 84e2c2adba..50381e979e 100644
> --- a/arch/arm/dts/armada-3720-espressobin.dts
> +++ b/arch/arm/dts/armada-3720-espressobin.dts
> @@ -72,13 +72,13 @@
>  &comphy {
>  	max-lanes = <3>;
>  	phy0 {
> -		phy-type = <PHY_TYPE_PEX0>;
> -		phy-speed = <PHY_SPEED_2_5G>;
> +		phy-type = <PHY_TYPE_USB3_HOST0>;
> +		phy-speed = <PHY_SPEED_5G>;
>  	};
> 
>  	phy1 {
> -		phy-type = <PHY_TYPE_USB3_HOST0>;
> -		phy-speed = <PHY_SPEED_5G>;
> +		phy-type = <PHY_TYPE_PEX0>;
> +		phy-speed = <PHY_SPEED_2_5G>;
>  	};
> 
>  	phy2 {
> --
> 2.26.2

Reviewed by Konstantin Porotchkin <kostap@marvell.com>

[PATCH u-boot-marvell] arm64: dts: armada-3720-espressobin: fix COMPHY nodes

[Stefan Roese]

On 19.08.20 11:57, Marek Beh?n wrote:
> This commit fixes initialization of COMPHY on EspressoBin.
> 
> Commit 22f418935be4 ("phy: marvell: a3700: Use comphy_mux on Armada
> 37xx.") introduced usage of comphy_mux on Armada 37xx comphy driver.
> The lanes are defined in comphy_a3700.c as described in functional
> specification, that is:
>    lane 0 is SGMII1 or USB3
>    lane 1 is PCIe or SGMII0
>    lane 2 is SATA or USB3
> 
> But the DTS for EspressoBin configures PCIe on lane 0 and USB3 on
> lane 1, which is wrong in the sense of the specification and doesn't
> work with the comphy_mux code, which is 2 years now (the aardvark driver
> causes synchronous abort in U-Boot).
> 
> It worked till the above mentioned commit, because the code for powering
> up PCIe PHY doesn't work with lane number at all, and the code for
> powering up USB3 PHY works differently only if USB3 is on lane 2, ie.
> the check goes like:
>    if (lane == 2)
>      something
>    else
>      something else
> so it does not differentiate between lanes 0 and 1.
> 
> In the future I shall post patches that remove the comphy_a3700 driver
> and add comphy driver which uses calls to ATF, like Linux' driver does.
> This will have the advantage of same DTS bindings as Linux',

That's good. Thanks.

> but till
> this is done, we need this patch.
> 
> Signed-off-by: Marek Beh?n <marek.behun@nic.cz>
> Tested-by: Pali Roh?r <pali@kernel.org>
> Cc: Stefan Roese <sr@denx.de>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   arch/arm/dts/armada-3720-espressobin.dts | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/dts/armada-3720-espressobin.dts b/arch/arm/dts/armada-3720-espressobin.dts
> index 84e2c2adba..50381e979e 100644
> --- a/arch/arm/dts/armada-3720-espressobin.dts
> +++ b/arch/arm/dts/armada-3720-espressobin.dts
> @@ -72,13 +72,13 @@
>   &comphy {
>   	max-lanes = <3>;
>   	phy0 {
> -		phy-type = <PHY_TYPE_PEX0>;
> -		phy-speed = <PHY_SPEED_2_5G>;
> +		phy-type = <PHY_TYPE_USB3_HOST0>;
> +		phy-speed = <PHY_SPEED_5G>;
>   	};
>   
>   	phy1 {
> -		phy-type = <PHY_TYPE_USB3_HOST0>;
> -		phy-speed = <PHY_SPEED_5G>;
> +		phy-type = <PHY_TYPE_PEX0>;
> +		phy-speed = <PHY_SPEED_2_5G>;
>   	};
>   
>   	phy2 {
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[PATCH u-boot-marvell] arm64: dts: armada-3720-espressobin: fix COMPHY nodes

[Andre Heider]

On 19/08/2020 11:57, Marek Beh?n wrote:
> This commit fixes initialization of COMPHY on EspressoBin.
> 
> Commit 22f418935be4 ("phy: marvell: a3700: Use comphy_mux on Armada
> 37xx.") introduced usage of comphy_mux on Armada 37xx comphy driver.
> The lanes are defined in comphy_a3700.c as described in functional
> specification, that is:
>    lane 0 is SGMII1 or USB3
>    lane 1 is PCIe or SGMII0
>    lane 2 is SATA or USB3
> 
> But the DTS for EspressoBin configures PCIe on lane 0 and USB3 on
> lane 1, which is wrong in the sense of the specification and doesn't
> work with the comphy_mux code, which is 2 years now (the aardvark driver
> causes synchronous abort in U-Boot).
> 
> It worked till the above mentioned commit, because the code for powering
> up PCIe PHY doesn't work with lane number at all, and the code for
> powering up USB3 PHY works differently only if USB3 is on lane 2, ie.
> the check goes like:
>    if (lane == 2)
>      something
>    else
>      something else
> so it does not differentiate between lanes 0 and 1.
> 
> In the future I shall post patches that remove the comphy_a3700 driver
> and add comphy driver which uses calls to ATF, like Linux' driver does.
> This will have the advantage of same DTS bindings as Linux', but till
> this is done, we need this patch.
> 
> Signed-off-by: Marek Beh?n <marek.behun@nic.cz>
> Tested-by: Pali Roh?r <pali@kernel.org>
> Cc: Stefan Roese <sr@denx.de>

Tested-by: Andre Heider <a.heider@gmail.com>

Nice, thank you!

I remember getting these data aborts more than a year ago...

Andre

[PATCH u-boot-marvell] arm64: dts: armada-3720-espressobin: fix COMPHY nodes

[Andre Heider]

Hi Marek,

On 19/08/2020 11:57, Marek Beh?n wrote:
> This commit fixes initialization of COMPHY on EspressoBin.
> 
> Commit 22f418935be4 ("phy: marvell: a3700: Use comphy_mux on Armada
> 37xx.") introduced usage of comphy_mux on Armada 37xx comphy driver.
> The lanes are defined in comphy_a3700.c as described in functional
> specification, that is:
>    lane 0 is SGMII1 or USB3
>    lane 1 is PCIe or SGMII0
>    lane 2 is SATA or USB3
> 
> But the DTS for EspressoBin configures PCIe on lane 0 and USB3 on
> lane 1, which is wrong in the sense of the specification and doesn't
> work with the comphy_mux code, which is 2 years now (the aardvark driver
> causes synchronous abort in U-Boot).
> 
> It worked till the above mentioned commit, because the code for powering
> up PCIe PHY doesn't work with lane number at all, and the code for
> powering up USB3 PHY works differently only if USB3 is on lane 2, ie.
> the check goes like:
>    if (lane == 2)
>      something
>    else
>      something else
> so it does not differentiate between lanes 0 and 1.
> 
> In the future I shall post patches that remove the comphy_a3700 driver
> and add comphy driver which uses calls to ATF, like Linux' driver does.
> This will have the advantage of same DTS bindings as Linux', but till
> this is done, we need this patch.
> 
> Signed-off-by: Marek Beh?n <marek.behun@nic.cz>
> Tested-by: Pali Roh?r <pali@kernel.org>
> Cc: Stefan Roese <sr@denx.de>

now that I have a working mainline firmware, I think I have a related 
problem: my sata ssd doesn't get detected, I just get this:

TIM-1.0
WTMI-devel-18.12.1-
WTMI: system early-init
CPU VDD voltage default value: 1.108V
NOTICE:  Booting Trusted Firmware
NOTICE:  BL1: v2.3(): (Marvell-devel-18.12.0)
NOTICE:  BL1: Built : 06:12:46, Aug 26 2020
NOTICE:  BL1: Booting BL2
NOTICE:  BL2: v2.3(): (Marvell-devel-18.12.0)
NOTICE:  BL2: Built : 06:12:46, Aug 26 2020
NOTICE:  BL1: Booting BL31
NOTICE:  BL31: v2.3(): (Marvell-devel-18.12.0)
NOTICE:  BL31: Built : 06:12:46

U-Boot 2020.07 (Aug 26 2020 - 06:12:46 +0000)

DRAM:  1 GiB
Comphy-0: USB3_HOST0    5 Gbps
Comphy-1: PEX0          2.5 Gbps
Comphy-2: SATA0         5 Gbps
SATA link 0 timeout.

Any idea what's missing here? Do you think this may also be comphy related?

Please cc: me on your comphy/atf patches, I'd be willing to test those!

Thanks,
Andre

[PATCH u-boot-marvell] arm64: dts: armada-3720-espressobin: fix COMPHY nodes

[Andre Heider]

On 28/08/2020 10:18, Andre Heider wrote:
> Hi Marek,
> 
> On 19/08/2020 11:57, Marek Beh?n wrote:
>> This commit fixes initialization of COMPHY on EspressoBin.
>>
>> Commit 22f418935be4 ("phy: marvell: a3700: Use comphy_mux on Armada
>> 37xx.") introduced usage of comphy_mux on Armada 37xx comphy driver.
>> The lanes are defined in comphy_a3700.c as described in functional
>> specification, that is:
>> ?? lane 0 is SGMII1 or USB3
>> ?? lane 1 is PCIe or SGMII0
>> ?? lane 2 is SATA or USB3
>>
>> But the DTS for EspressoBin configures PCIe on lane 0 and USB3 on
>> lane 1, which is wrong in the sense of the specification and doesn't
>> work with the comphy_mux code, which is 2 years now (the aardvark driver
>> causes synchronous abort in U-Boot).
>>
>> It worked till the above mentioned commit, because the code for powering
>> up PCIe PHY doesn't work with lane number at all, and the code for
>> powering up USB3 PHY works differently only if USB3 is on lane 2, ie.
>> the check goes like:
>> ?? if (lane == 2)
>> ???? something
>> ?? else
>> ???? something else
>> so it does not differentiate between lanes 0 and 1.
>>
>> In the future I shall post patches that remove the comphy_a3700 driver
>> and add comphy driver which uses calls to ATF, like Linux' driver does.
>> This will have the advantage of same DTS bindings as Linux', but till
>> this is done, we need this patch.
>>
>> Signed-off-by: Marek Beh?n <marek.behun@nic.cz>
>> Tested-by: Pali Roh?r <pali@kernel.org>
>> Cc: Stefan Roese <sr@denx.de>
> 
> now that I have a working mainline firmware, I think I have a related 
> problem: my sata ssd doesn't get detected, I just get this:
> 
> TIM-1.0
> WTMI-devel-18.12.1-
> WTMI: system early-init
> CPU VDD voltage default value: 1.108V
> NOTICE:? Booting Trusted Firmware
> NOTICE:? BL1: v2.3(): (Marvell-devel-18.12.0)
> NOTICE:? BL1: Built : 06:12:46, Aug 26 2020
> NOTICE:? BL1: Booting BL2
> NOTICE:? BL2: v2.3(): (Marvell-devel-18.12.0)
> NOTICE:? BL2: Built : 06:12:46, Aug 26 2020
> NOTICE:? BL1: Booting BL31
> NOTICE:? BL31: v2.3(): (Marvell-devel-18.12.0)
> NOTICE:? BL31: Built : 06:12:46
> 
> U-Boot 2020.07 (Aug 26 2020 - 06:12:46 +0000)
> 
> DRAM:? 1 GiB
> Comphy-0: USB3_HOST0??? 5 Gbps
> Comphy-1: PEX0????????? 2.5 Gbps
> Comphy-2: SATA0???????? 5 Gbps
> SATA link 0 timeout.
> 
> Any idea what's missing here? Do you think this may also be comphy related?
> 
> Please cc: me on your comphy/atf patches, I'd be willing to test those!

Nevermind, Pali figured it out:
https://patchwork.ozlabs.org/project/uboot/patch/20200828145629.540954-1-a.heider at gmail.com/

Regards,
Andre

[PATCH u-boot-marvell] arm64: dts: armada-3720-espressobin: fix COMPHY nodes

[Andre Heider]

Hi Marek,

On 19/08/2020 11:57, Marek Beh?n wrote:
> This commit fixes initialization of COMPHY on EspressoBin.
> 
> Commit 22f418935be4 ("phy: marvell: a3700: Use comphy_mux on Armada
> 37xx.") introduced usage of comphy_mux on Armada 37xx comphy driver.
> The lanes are defined in comphy_a3700.c as described in functional
> specification, that is:
>    lane 0 is SGMII1 or USB3
>    lane 1 is PCIe or SGMII0
>    lane 2 is SATA or USB3
> 
> But the DTS for EspressoBin configures PCIe on lane 0 and USB3 on
> lane 1, which is wrong in the sense of the specification and doesn't
> work with the comphy_mux code, which is 2 years now (the aardvark driver
> causes synchronous abort in U-Boot).

I think armada-3720-db.dts requires the same fix?

Regards,
Andre

[PATCH u-boot-marvell] arm64: dts: armada-3720-espressobin: fix COMPHY nodes

[Marek Behun]

On Sun, 30 Aug 2020 08:38:32 +0200
Andre Heider <a.heider@gmail.com> wrote:

> Hi Marek,
> 
> On 19/08/2020 11:57, Marek Beh?n wrote:
> > This commit fixes initialization of COMPHY on EspressoBin.
> > 
> > Commit 22f418935be4 ("phy: marvell: a3700: Use comphy_mux on Armada
> > 37xx.") introduced usage of comphy_mux on Armada 37xx comphy driver.
> > The lanes are defined in comphy_a3700.c as described in functional
> > specification, that is:
> >    lane 0 is SGMII1 or USB3
> >    lane 1 is PCIe or SGMII0
> >    lane 2 is SATA or USB3
> > 
> > But the DTS for EspressoBin configures PCIe on lane 0 and USB3 on
> > lane 1, which is wrong in the sense of the specification and doesn't
> > work with the comphy_mux code, which is 2 years now (the aardvark driver
> > causes synchronous abort in U-Boot).  
> 
> I think armada-3720-db.dts requires the same fix?
> 
> Regards,
> Andre

You are right. Sigh :( ....

[PATCH u-boot-marvell] arm64: dts: armada-3720-espressobin: fix COMPHY nodes

[Stefan Roese]

On 19.08.20 11:57, Marek Beh?n wrote:
> This commit fixes initialization of COMPHY on EspressoBin.
> 
> Commit 22f418935be4 ("phy: marvell: a3700: Use comphy_mux on Armada
> 37xx.") introduced usage of comphy_mux on Armada 37xx comphy driver.
> The lanes are defined in comphy_a3700.c as described in functional
> specification, that is:
>    lane 0 is SGMII1 or USB3
>    lane 1 is PCIe or SGMII0
>    lane 2 is SATA or USB3
> 
> But the DTS for EspressoBin configures PCIe on lane 0 and USB3 on
> lane 1, which is wrong in the sense of the specification and doesn't
> work with the comphy_mux code, which is 2 years now (the aardvark driver
> causes synchronous abort in U-Boot).
> 
> It worked till the above mentioned commit, because the code for powering
> up PCIe PHY doesn't work with lane number at all, and the code for
> powering up USB3 PHY works differently only if USB3 is on lane 2, ie.
> the check goes like:
>    if (lane == 2)
>      something
>    else
>      something else
> so it does not differentiate between lanes 0 and 1.
> 
> In the future I shall post patches that remove the comphy_a3700 driver
> and add comphy driver which uses calls to ATF, like Linux' driver does.
> This will have the advantage of same DTS bindings as Linux', but till
> this is done, we need this patch.
> 
> Signed-off-by: Marek Beh?n <marek.behun@nic.cz>
> Tested-by: Pali Roh?r <pali@kernel.org>
> Cc: Stefan Roese <sr@denx.de>

Applied to u-boot-marvell/master

Thanks,
Stefan

> ---
>   arch/arm/dts/armada-3720-espressobin.dts | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/dts/armada-3720-espressobin.dts b/arch/arm/dts/armada-3720-espressobin.dts
> index 84e2c2adba..50381e979e 100644
> --- a/arch/arm/dts/armada-3720-espressobin.dts
> +++ b/arch/arm/dts/armada-3720-espressobin.dts
> @@ -72,13 +72,13 @@
>   &comphy {
>   	max-lanes = <3>;
>   	phy0 {
> -		phy-type = <PHY_TYPE_PEX0>;
> -		phy-speed = <PHY_SPEED_2_5G>;
> +		phy-type = <PHY_TYPE_USB3_HOST0>;
> +		phy-speed = <PHY_SPEED_5G>;
>   	};
>   
>   	phy1 {
> -		phy-type = <PHY_TYPE_USB3_HOST0>;
> -		phy-speed = <PHY_SPEED_5G>;
> +		phy-type = <PHY_TYPE_PEX0>;
> +		phy-speed = <PHY_SPEED_2_5G>;
>   	};
>   
>   	phy2 {
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de