netdev - Re: DSA mv88e6xxx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 3 Feb 2023 08:07:52 +0000
From:   "Valek, Andrej" <andrej.valek@...mens.com>
To:     "andrew@...n.ch" <andrew@...n.ch>
CC:     "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: DSA mv88e6xxx_probe

Hello again,

On Thu, 2023-02-02 at 19:10 +0100, Andrew Lunn wrote:
> > > > > chip->reset = devm_gpiod_get_optional(dev, "reset",
> > > > > GPIOD_OUT_LOW);
> > > > > if (IS_ERR(chip->reset))
> > > > >         goto out;
> > > > > 
> > > > > if (chip->reset)
> > > > >         usleep_range(1000, 2000);
> > > > 
> > > > So it should wait, but for what?
> > > 
> > > The current code is designed to take a switch held in reset out of
> > > reset. It does not perform an actual reset.
> > > 
> > How does it then work? I see just a "devm_gpiod_get_optional" which
> > just assign an pointer to "chip->reset" and then
> > " if (chip->reset) usleep_range(1000, 2000);" which just waits for
> > "something" ? Where is the "reset" took out? I don't see any gpio set
> > to 0.
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/consumer.h#L49
> 
>         GPIOD_OUT_LOW   = GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT,
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L4051
> 
>         /* Process flags */
>         if (dflags & GPIOD_FLAGS_BIT_DIR_OUT)
>                 ret = gpiod_direction_output(desc,
>                                 !!(dflags & GPIOD_FLAGS_BIT_DIR_VAL));
>         else
>                 ret = gpiod_direction_input(desc);
> 
Ok, not it makes much more sense. So it's automatically set output to OFF. In my case to HIGH.
> > > If you need a real reset, you probably need to call
> > > mv88e6xxx_hardware_reset(chip), not usleep().
> > > 
> > > However, a reset can be a slow operation, specially if the EEPROM is
> > > full of stuff. So we want to avoid two resets if possible.
> > > 
> > > The MDIO bus itself has DT descriptions for a GPIO reset. See
> > > Documentation/devicetree/bindings/net/mdio.yaml
> > This looks promising. So I have to just move the "reset-gpios" DTB
> > entry from switch to mdio section. But which driver handles it,
> > drivers/net/phy/mdio_bus.c,
> 
> Yes.
> 
> > > mdio {
> > >         #address-cells = <1>;
> > >         #size-cells = 0>;
> > while here is no compatible part... .
> 
> It does not need a compatible, because it is part of the FEC, and the
> FEC has a compatible. Remember this is device tree, sometimes you need
> to go up the tree towards the root to find the actual device with a
> compatible.
> 
>     Andrew
I tried put the "reset-gpios" and "reset-delay-us" into multiple mdio locations, but nothing has been working. DTB looks like that:

> &fec1 {
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&pinctrl_fec1>;
> 	phy-mode = "rgmii-id";
> 	tx-internal-delay-ps = <2000>;
> 	rx-internal-delay-ps = <2000>;
> 	slaves = <1>;			// use only one emac if
> 	status = "okay";
> 	mac-address = [ 00 00 00 00 00 00 ]; // Filled in by U-Boot
>
> 	// #### 3. try ####
> 	//phy-reset-gpios = <&lsio_gpio0 13 GPIO_ACTIVE_LOW>;
> 	//reset-delay-us = <10000>;
>
> 	fixed-link {
> 		speed = <1000>;
> 		full-duplex;
> 	};
>
> 	mdio {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
>
> 		// 1. try
> 		reset-gpios = <&lsio_gpio0 13 GPIO_ACTIVE_LOW>;
> 		reset-delay-us = <10000>;
>
> 		// MV88E6321 Switch
> 		switch: switch@10 {
> 			compatible = "marvell,mv88e6321","marvell,mv88e6085";
>
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			dsa,member = <0 0>;
>
> 			reg = <0x10>;
> 			interrupt-controller;
> 			#interrupt-cells = <2>;
>
> 			// this is the location, where driver is handling the reset
> 			//reset-gpios = <&lsio_gpio0 13 GPIO_ACTIVE_LOW>;
>
> 			ports {
> 				#address-cells = <1>;
> 				#size-cells = <0>;
>
> 				port0: ports@0 {
> 					reg = <0>;
> 					label = "wan0";
> 					phy-handle = <&switch1phy0>;
> 				// other ports part...
> 			};
>
> 			mdio {
> 				#address-cells = <1>;
> 				#size-cells = <0>;
>
> 				// #### 2. try ####
> 				//reset-gpios = <&lsio_gpio0 13 GPIO_ACTIVE_LOW>;
> 				//reset-delay-us = <10000>;
>
>
> 				switch1phy0: switch1phy0@0 {
> 					reg = <0>;
> 					select-class-a;
> 				};
> 				// ...
> 			};
> 		};
> 	};
>};

And if I look at the decompiled DTB, there is no physical address for MDIO bus.
> mdio {
> 		#address-cells = <0x01>;
> 		#size-cells = <0x00>;
> 		reset-gpios = <0x8c 0x0d 0x01>;
> 		reset-delay-us = <0x2710>;
>
>		switch@10 {
> 			compatible = "marvell,mv88e6321\0marvell,mv88e6085";
> 			#address-cells = <0x01>;
> 			#size-cells = <0x00>;
> 			dsa,member = <0x00 0x00>;

So how to verify it?

Thank you,
Andrej