[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <05f695cd76ffcc885e6ea70c58d0a07dbc48a341.camel@siemens.com>
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
Powered by blists - more mailing lists