Message ID | 20201230214918.17133-1-festevam@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v3,1/2] dt-bindings: gpio: 74x164: Introduce the 'registers-default' property | expand |
On Wed, Dec 30, 2020 at 10:49 PM Fabio Estevam <festevam@gmail.com> wrote: > There are cases where a certain default output value in the 74x164 > output is needed. > > For example: the imx6ul-evk board has the Ethernet PHY reset controlled > by the 74x164 chip. > > After enabling the OE pin, the output pins of the 74x164 chip go to > zero by default, which makes the Ethernet PHY not to be detected. So should the ethernet PHY not just have some reset-gpios that it obtain and de-assert as part of probing? For example drivers/net/phy/mdio_bus.c has this: /* de-assert bus level PHY GPIO reset */ gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW); Other drivers can do the same. Deferred probe should ascertain that this GPIO driver gets loaded before the ethernet phy driver. > Add a new optional property called 'registers-default' that allows > describing the default output value for each shift register. > > Signed-off-by: Fabio Estevam <festevam@gmail.com> (...) > Optional properties: > - enable-gpios: GPIO connected to the OE (Output Enable) pin. > +- registers-default: An array of 8-bit values describing the default output > +value of each shift registers. What this does is to set up several of the GPIO lines to default values, right? I think this is a hack, there have again and again been proposed to have a way to set up initial values of GPIO lines. I think we need to fix that instead. I am sorry that initial values just stalemate all the time. I imagine that codewise it should just be some lines in of_gpiochip_scan_gpios() in gpiolib-of.c (which can later be made into generic device properties if ACPI needs this too). The format of the OF bindings is what people have been discussing for too long, whether gpio-initial-values, etc. I would just implement what the DT people want and will ACK so it solves your problem. I will try to look up references to earlier discussions on this. Yours, Linus Walleij
Hi Linus, On Tue, Jan 5, 2021 at 12:35 PM Linus Walleij <linus.walleij@linaro.org> wrote: > So should the ethernet PHY not just have some reset-gpios > that it obtain and de-assert as part of probing? > > For example drivers/net/phy/mdio_bus.c has this: > > /* de-assert bus level PHY GPIO reset */ > gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW); > > Other drivers can do the same. > > Deferred probe should ascertain that this GPIO driver gets > loaded before the ethernet phy driver. Thanks for your review. I investigated this further and it seems to me that the issue is in drivers/net/phy/mdio_bus.c, so I am adding some netdev folks. The driver drivers/net/phy/mdio_bus.c and the yaml description says that it only supports one bus level PHY GPIO reset via reset-gpios property. On the imx6ul-evk, there are two KSZ8081 PHYs in the same mdio bus, so this is how I described it in dts: https://pastebin.com/raw/xLjYUHdN but the 'reset-gpios' properties are not found in this case. I think we need to let drivers/net/phy/mdio_bus.c to search for 'reset-gpios' also inside the mdio children. My understanding is that adding phy-reset-gpios under each fec node is deprecated. Any suggestions? Thanks, Fabio Estevam
On Thu, Jan 7, 2021 at 1:16 PM Fabio Estevam <festevam@gmail.com> wrote: > The driver drivers/net/phy/mdio_bus.c and the yaml description says > that it only supports one bus level PHY GPIO reset via reset-gpios > property. I suppose it's this binding: Documentation/devicetree/bindings/net/ethernet-phy.yaml > On the imx6ul-evk, there are two KSZ8081 PHYs in the same mdio bus, so > this is how I described it in dts: > > https://pastebin.com/raw/xLjYUHdN > > but the 'reset-gpios' properties are not found in this case. I think > we need to let drivers/net/phy/mdio_bus.c to search for 'reset-gpios' > also inside the mdio children. This driver gives me headache. The bindings say that it should populate devices from the compatible of the subnodes with names like "ethernet-phy-id0141.0e90" and stuff like that. Indeed, but I don't understand why the phy in this example does not have a compatible string? There is some hackery going on to probe the driver from the bus level since commit 46abc02175b3c246dd5141d878f565a8725060c9 "phylib: give mdio buses a device tree presence" But as far as I read the code this driver should be probing devices inidividually for ther reset-gpios, I think the problem is maybe that no proper devices (mdiodev) are added for these devices, does mdiobus_register_device() even get called for them? Does of_mdiobus_register_phy() get called? If this is the problem I think you need to add compatible for your phy devices and make sure there is some code to probe them as well? of_mdiobus_register() must be called by the MDIO bus master/host driver. This is what traverses the children with for_each_available_child_of_node(). Does your host driver properly call this function? Sorry if I sound a bit confused. Yours, Linus Walleij
On Thu, Jan 07, 2021 at 09:16:31AM -0300, Fabio Estevam wrote: > Hi Linus, > > On Tue, Jan 5, 2021 at 12:35 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > So should the ethernet PHY not just have some reset-gpios > > that it obtain and de-assert as part of probing? > > > > For example drivers/net/phy/mdio_bus.c has this: > > > > /* de-assert bus level PHY GPIO reset */ > > gpiod = devm_gpiod_get_optional(&bus->dev, "reset", GPIOD_OUT_LOW); > > > > Other drivers can do the same. > > > > Deferred probe should ascertain that this GPIO driver gets > > loaded before the ethernet phy driver. > > Thanks for your review. I investigated this further and it seems to me > that the issue is in drivers/net/phy/mdio_bus.c, so I am adding some > netdev folks. > > The driver drivers/net/phy/mdio_bus.c and the yaml description says > that it only supports one bus level PHY GPIO reset via reset-gpios > property. > > On the imx6ul-evk, there are two KSZ8081 PHYs in the same mdio bus, so > this is how I described it in dts: There are two different GPIO supported. There is the bus GPIO you have found, which is intended to reset all devices on the MDIO bus. And there is a per device GPIO reset and reset controller. However, in order to use these, you need to be able to 'discover' the PHY, potentially when the device is held in reset. Some devices will respond to MDIO while held in reset, some don't. If you PHYs don't you need to add a compatible of the form ethernet-phy-id[a-f0-9]{4}\\.[a-f0-9]{4}$ with the PHY ID. The PHY will then be probed, independent of if it can be found on the bus or not, and that probing will enable the GPIO. Andrew
> Indeed, but I don't understand why the phy in this example does not > have a compatible string? Normally, PHYs can be enumerated on the bus, in the same way USB and PCI devices can be enumerated. The device itself then tells you what it is campatible with, in the same way PCI and USB does. Having a compatible string is then detrimental, because people get it wrong, device tree says one thing, the device itself says something else, which do we trust? There are also boards where different series of have populated with different pin compatible PHYs. Enumeration just works, having a compatible would be a problem. But, there are some PHYs which you cannot enumerate, e.g they do not respond to MDIO while held in reset, or the silicon is buggy and has an invalid ID. In those cases, a compatible should be listed with the correct PHY ID. Andrew
Hi Andrew, On Thu, Jan 7, 2021 at 11:25 AM Andrew Lunn <andrew@lunn.ch> wrote: > There are two different GPIO supported. There is the bus GPIO you have > found, which is intended to reset all devices on the MDIO bus. > > And there is a per device GPIO reset and reset controller. However, in > order to use these, you need to be able to 'discover' the PHY, > potentially when the device is held in reset. Some devices will > respond to MDIO while held in reset, some don't. If you PHYs don't you > need to add a compatible of the form > ethernet-phy-id[a-f0-9]{4}\\.[a-f0-9]{4}$ with the PHY ID. The PHY > will then be probed, independent of if it can be found on the bus or > not, and that probing will enable the GPIO. It works after following your suggestion :-) I will submit the dts patches shortly. Thanks a lot for your help, really appreciate it!
diff --git a/Documentation/devicetree/bindings/gpio/gpio-74x164.txt b/Documentation/devicetree/bindings/gpio/gpio-74x164.txt index 2a97553d8d76..bf8f45896018 100644 --- a/Documentation/devicetree/bindings/gpio/gpio-74x164.txt +++ b/Documentation/devicetree/bindings/gpio/gpio-74x164.txt @@ -14,6 +14,8 @@ Required properties: Optional properties: - enable-gpios: GPIO connected to the OE (Output Enable) pin. +- registers-default: An array of 8-bit values describing the default output +value of each shift registers. Example: @@ -24,4 +26,5 @@ gpio5: gpio5@0 { #gpio-cells = <2>; registers-number = <4>; spi-max-frequency = <100000>; + registers-default = /bits/ 8 <0x57 0xF0 0xFF 0xF0>; };
There are cases where a certain default output value in the 74x164 output is needed. For example: the imx6ul-evk board has the Ethernet PHY reset controlled by the 74x164 chip. After enabling the OE pin, the output pins of the 74x164 chip go to zero by default, which makes the Ethernet PHY not to be detected. Add a new optional property called 'registers-default' that allows describing the default output value for each shift register. Signed-off-by: Fabio Estevam <festevam@gmail.com> --- Changes since v2: - None Documentation/devicetree/bindings/gpio/gpio-74x164.txt | 3 +++ 1 file changed, 3 insertions(+)