[PATCH v3 1/2] dt-bindings: gpio: 74x164: Introduce the 'registers-default' property

[PATCH v3 1/2] dt-bindings: gpio: 74x164: Introduce the 'registers-default' property

[Fabio Estevam]

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(+)

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>;
 };
-- 
2.17.1

[PATCH v3 2/2] gpio: 74x164: Introduce the 'registers-default' property

[Fabio Estevam]

From: Sandor Yu <sandor.yu@nxp.com>

On the imx7d-sdb board, there is one output pin of the 74x164 that
controls all peripherals power supply (PERI_3V).

This pin should be at high voltage level when the 74x164 is probed,
otherwise the modules dependent on PERI_3V3 will not be powered.

Add a new optional property called 'registers-default' that allows
describing the default output value for each shift register.

Signed-off-by: Sandor Yu <sandor.yu@nxp.com>
Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
Changes since v2:
-Add Sandor's Signed-off-by tag.

 drivers/gpio/gpio-74x164.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpio-74x164.c b/drivers/gpio/gpio-74x164.c
index 05637d585152..a11637518774 100644
--- a/drivers/gpio/gpio-74x164.c
+++ b/drivers/gpio/gpio-74x164.c
@@ -141,6 +141,9 @@ static int gen_74x164_probe(struct spi_device *spi)
 	chip->registers = nregs;
 	chip->gpio_chip.ngpio = GEN_74X164_NUMBER_GPIOS * chip->registers;
 
+	device_property_read_u8_array(&spi->dev, "registers-default",
+				      chip->buffer, chip->registers);
+
 	chip->gpio_chip.can_sleep = true;
 	chip->gpio_chip.parent = &spi->dev;
 	chip->gpio_chip.owner = THIS_MODULE;
-- 
2.17.1

Re: [PATCH v3 2/2] gpio: 74x164: Introduce the 'registers-default' property

[Linus Walleij]

On Wed, Dec 30, 2020 at 10:49 PM Fabio Estevam <festevam@gmail.com> wrote:

> From: Sandor Yu <sandor.yu@nxp.com>
>
> On the imx7d-sdb board, there is one output pin of the 74x164 that
> controls all peripherals power supply (PERI_3V).
>
> This pin should be at high voltage level when the 74x164 is probed,
> otherwise the modules dependent on PERI_3V3 will not be powered.

This seems like a usecase for a fixed GPIO regulator.
Documentation/devicetree/bindings/regulator/fixed-regulator.yaml
drivers/regulator/fixed.c

> Add a new optional property called 'registers-default' that allows
> describing the default output value for each shift register.
>
> Signed-off-by: Sandor Yu <sandor.yu@nxp.com>
> Signed-off-by: Fabio Estevam <festevam@gmail.com>
> ---
> Changes since v2:
> -Add Sandor's Signed-off-by tag.

Again while poking some defaults into some registers does indeed
solve the problem, what we really want is to assign initial values
to GPIO lines for cases where there are no consumers.

If there are actually no device drivers on the GPIO line you can
use GPIO hogs.
Documentation/devicetree/bindings/gpio/gpio.txt

Otherwise e.g. properly obtain reset or GPIO regulator etc.

Deferred probe will sort things out,

If you just need the line to go to a certain level for the short
period between initializing the GPIO driver and the proper device
driver taking control of the line, we need initial GPIO line
values, which was discussed in my previous mail.

Yours,
Linus Walleij

Re: [PATCH v3 1/2] dt-bindings: gpio: 74x164: Introduce the 'registers-default' property

[Linus Walleij]

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

Re: [PATCH v3 1/2] dt-bindings: gpio: 74x164: Introduce the 'registers-default' property

[Fabio Estevam]

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

Re: [PATCH v3 1/2] dt-bindings: gpio: 74x164: Introduce the 'registers-default' property

[Linus Walleij]

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

Re: [PATCH v3 1/2] dt-bindings: gpio: 74x164: Introduce the 'registers-default' property

[Andrew Lunn]

> 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

Re: [PATCH v3 1/2] dt-bindings: gpio: 74x164: Introduce the 'registers-default' property

[Andrew Lunn]

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

Re: [PATCH v3 1/2] dt-bindings: gpio: 74x164: Introduce the 'registers-default' property

[Fabio Estevam]

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!