[PATCH 1/2] ARM: dts: imx6dl-yapp4: Fix the QCA switch register address

[PATCH 1/2] ARM: dts: imx6dl-yapp4: Fix the QCA switch register address

[Michal Vokáč]

The switch address in the node name is in hex while the address in the reg
property is decimal which is wrong. Fix that and write the reg address
as a hexadecimal number.

Fixes: 15b43e497ffd ("ARM: dts: imx6dl-yapp4: Use correct pseudo PHY address for the switch")
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi b/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi
index cfb0fc924b42..5763f8253d51 100644
--- a/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi
+++ b/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi
@@ -143,7 +143,7 @@
 
 		switch@10 {
 			compatible = "qca,qca8334";
-			reg = <10>;
+			reg = <0x10>;
 			reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
 
 			switch_ports: ports {
-- 
2.1.4

[PATCH 2/2] ARM: dts: imx6dl-yapp4: Move the internal switch PHYs under the switch node

[Michal Vokáč]

We identified that the PHYs actually do not work since commit 7da7b84fee58
("ARM: dts: imx6dl-yapp4: Move phy reset into switch node") as
a coincidence of several circumstances.

The reset signal is kept asserted by a pull-down resistor on the board
unless it is deasserted by GPIO from the SoC. This is to keep the switch
dead until it is configured properly by the kernel and user space.

Prior to the referenced commit the switch was reset by the FEC driver
and the reset GPIO was actively deasserted. The mdio-bus was scanned
and the attached switch and its PHYs were found and configured.

With the referenced commit the switch is reset by the qca8k driver.
Because of another bug in the qca8k driver, functionality of the reset
pin depends on its pre-kernel configuration. See commit c44fc98f0a8f
("net: dsa: qca8k: fix illegal usage of GPIO")

The problem did not appear until we removed support for the switch
and configuration of its reset pin from the bootloader.

To fix that, properly describe the internal mdio-bus configuration of
the qca8334 switch. The PHYs are internal to the switch and sit on its
internal mdio-bus.

Fixes: 7da7b84fee58 ("ARM: dts: imx6dl-yapp4: Move phy reset into switch node")
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi | 23 ++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi b/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi
index 5763f8253d51..eec1f9092572 100644
--- a/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi
+++ b/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi
@@ -133,14 +133,6 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		phy_port2: phy@1 {
-			reg = <1>;
-		};
-
-		phy_port3: phy@2 {
-			reg = <2>;
-		};
-
 		switch@10 {
 			compatible = "qca,qca8334";
 			reg = <0x10>;
@@ -165,15 +157,30 @@
 				eth2: port@2 {
 					reg = <2>;
 					label = "eth2";
+					phy-mode = "internal";
 					phy-handle = <&phy_port2>;
 				};
 
 				eth1: port@3 {
 					reg = <3>;
 					label = "eth1";
+					phy-mode = "internal";
 					phy-handle = <&phy_port3>;
 				};
 			};
+
+			mdio {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				phy_port2: ethernet-phy@1 {
+					reg = <1>;
+				};
+
+				phy_port3: ethernet-phy@2 {
+					reg = <2>;
+				};
+			};
 		};
 	};
 };
-- 
2.1.4

Re: [PATCH 1/2] ARM: dts: imx6dl-yapp4: Fix the QCA switch register address

[Andrew Lunn]

On Mon, Feb 12, 2024 at 04:23:41PM +0100, Michal Vokáč wrote:
> The switch address in the node name is in hex while the address in the reg
> property is decimal which is wrong. Fix that and write the reg address
> as a hexadecimal number.

This feels the wrong way around. The reg value is used by the kernel,
where as the node name is not. If the reg value was wrong, the switch
would not be found. If this file was tested, why did somebody not
notice the switch was missing?

Do you have the hardware? Can you confirm is really does not work
without this patch? Was 15b43e497ffd never actually tested?

Thanks
	Andrew

> 
> Fixes: 15b43e497ffd ("ARM: dts: imx6dl-yapp4: Use correct pseudo PHY address for the switch")
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
>  arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi b/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi
> index cfb0fc924b42..5763f8253d51 100644
> --- a/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi
> +++ b/arch/arm/boot/dts/nxp/imx/imx6dl-yapp4-common.dtsi
> @@ -143,7 +143,7 @@
>  
>  		switch@10 {
>  			compatible = "qca,qca8334";
> -			reg = <10>;
> +			reg = <0x10>;
>  			reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
>  
>  			switch_ports: ports {
> -- 
> 2.1.4
> 

Re: [PATCH 1/2] ARM: dts: imx6dl-yapp4: Fix the QCA switch register address

[Michal Vokáč]

On 12. 02. 24 17:08, Andrew Lunn wrote:
> On Mon, Feb 12, 2024 at 04:23:41PM +0100, Michal Vokáč wrote:
>> The switch address in the node name is in hex while the address in the reg
>> property is decimal which is wrong. Fix that and write the reg address
>> as a hexadecimal number.
> 
> This feels the wrong way around. The reg value is used by the kernel,
> where as the node name is not. If the reg value was wrong, the switch
> would not be found. If this file was tested, why did somebody not
> notice the switch was missing?
> 
> Do you have the hardware? Can you confirm is really does not work
> without this patch? Was 15b43e497ffd never actually tested?
Yes, I have bunch of these boards all around my desk - we manufacture
them. I am pretty sure I tested all the patches I have ever sent to
the mailing list regarding these boards.

The fact is that the switch actually works regardless of the reg value.
It worked prior to the 15b43e497ffd commit with address 0, it worked
later on with the reg value 10 and it works now with reg value 0x10.

While dealing with the broken reset gpio [1] I just noticed that in
the 15b43e497ffd commit I made a typo and the node name address value
and reg address value differ so I wanted to put that in order.

I barely remember that my motivation for creating the 15b43e497ffd
commit was that I saw similar change on the mailing list or in the git
log for the other boards using the QCA8K switch. So I "fixed" that
on our board as well.

Now when I was looking for some references I found that there is an
other board in mainline with similarly wrong setting:

arch/arm/boot/dts/qcom/qcom-ipq8064-rb3011.dts

                 switch1: switch@14 {
                         compatible = "qca,qca8337";

                         dsa,member = <1 0>;

                         pinctrl-0 = <&sw1_reset_pin>;
                         pinctrl-names = "default";

                         reset-gpios = <&qcom_pinmux 17 GPIO_ACTIVE_LOW>;
                         reg = <0x10>;

I admit that my understanding of the MDIO bus and addressing of
the connected external/internal devices is pretty limited. I have no
answer to why it works like that but as you brought up your questions
I would actually like to know as well.

Thank you!
Michal

+Cc: Jonathan McDowell
[1] https://patchwork.kernel.org/project/netdevbpf/patch/1706266175-3408-1-git-send-email-michal.vokac@ysoft.com/

Re: [PATCH 1/2] ARM: dts: imx6dl-yapp4: Fix the QCA switch register address

[Andrew Lunn]

On Tue, Feb 13, 2024 at 01:20:44PM +0100, Michal Vokáč wrote:
> On 12. 02. 24 17:08, Andrew Lunn wrote:
> > On Mon, Feb 12, 2024 at 04:23:41PM +0100, Michal Vokáč wrote:
> > > The switch address in the node name is in hex while the address in the reg
> > > property is decimal which is wrong. Fix that and write the reg address
> > > as a hexadecimal number.
> > 
> > This feels the wrong way around. The reg value is used by the kernel,
> > where as the node name is not. If the reg value was wrong, the switch
> > would not be found. If this file was tested, why did somebody not
> > notice the switch was missing?
> > 
> > Do you have the hardware? Can you confirm is really does not work
> > without this patch? Was 15b43e497ffd never actually tested?
> Yes, I have bunch of these boards all around my desk - we manufacture
> them. I am pretty sure I tested all the patches I have ever sent to
> the mailing list regarding these boards.
> 
> The fact is that the switch actually works regardless of the reg value.
> It worked prior to the 15b43e497ffd commit with address 0, it worked
> later on with the reg value 10 and it works now with reg value 0x10.

Ah, so that is the missing piece of information from the commit
message. That the reg value does not actually matter. Hence it is safe
to change it.

Please reword the commit message.

> I admit that my understanding of the MDIO bus and addressing of
> the connected external/internal devices is pretty limited. I have no
> answer to why it works like that but as you brought up your questions
> I would actually like to know as well.

My guess is, the switch assumes it has full access to all the
addresses on the bus. It probably uses a subset, but that subset is
hard coded. But the MDIO DT binding requires a valid reg value, so
something has to be used.

There are some devices which use a single address on the bus. The
mv88e6xxx can be strapped into such a mode, so you can have multiple
switches on the bus. The reg value is then used. But you can also
strap it so it takes over the whole bus, and uses #num_ports + 3
addresses on the bus, and those addresses are hard coded in the
silicon, so the reg value is ignored.

    Andrew

---
pw-bot: cr

Re: [PATCH 1/2] ARM: dts: imx6dl-yapp4: Fix the QCA switch register address

[Michal Vokáč]

On 13. 02. 24 14:10, Andrew Lunn wrote:
> On Tue, Feb 13, 2024 at 01:20:44PM +0100, Michal Vokáč wrote:
>> On 12. 02. 24 17:08, Andrew Lunn wrote:
>>> On Mon, Feb 12, 2024 at 04:23:41PM +0100, Michal Vokáč wrote:
>> The fact is that the switch actually works regardless of the reg value.
>> It worked prior to the 15b43e497ffd commit with address 0, it worked
>> later on with the reg value 10 and it works now with reg value 0x10.
> 
> Ah, so that is the missing piece of information from the commit
> message. That the reg value does not actually matter. Hence it is safe
> to change it.
> 
> Please reword the commit message.

OK, I will do so.

>> I admit that my understanding of the MDIO bus and addressing of
>> the connected external/internal devices is pretty limited. I have no
>> answer to why it works like that but as you brought up your questions
>> I would actually like to know as well.
> 
> My guess is, the switch assumes it has full access to all the
> addresses on the bus. It probably uses a subset, but that subset is
> hard coded. But the MDIO DT binding requires a valid reg value, so
> something has to be used.

That makes sense. The problem is that the MDIO access and addressing
of the QCA8K switch is not well documented in the datasheet.

> There are some devices which use a single address on the bus. The
> mv88e6xxx can be strapped into such a mode, so you can have multiple
> switches on the bus. The reg value is then used. But you can also
> strap it so it takes over the whole bus, and uses #num_ports + 3
> addresses on the bus, and those addresses are hard coded in the
> silicon, so the reg value is ignored.

Ah, yes I am actually aware of that feature on the mv88e6xxx. We use
it on newer board revisions. AFAIK the switch is by default strapped
to the single chip addressing mode by internal pull-ups.

Thank you very much for shedding some light to that topic!

Michal