[PATCH net-next 0/2] net: pse-pd: tps23881: Reset GPIO support

[PATCH net-next 0/2] net: pse-pd: tps23881: Reset GPIO support

[Kyle Swenson]

On some boards, the TPS2388x's reset line (active low) is pulled low to
keep the chip in reset until the SoC pulls the device out of reset.
This series updates the device-tree binding for the tps23881 and then
adds support for the reset gpio handling in the tps23881 driver.

Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>

---
Kyle Swenson (2):
  dt-bindings: net: pse-pd: tps23881: add reset-gpios
  net: pse-pd: tps23881: Support reset-gpios

 .../devicetree/bindings/net/pse-pd/ti,tps23881.yaml |  3 +++
 drivers/net/pse-pd/tps23881.c                       | 13 ++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH net-next 1/2] dt-bindings: net: pse-pd: tps23881: add reset-gpios

[Kyle Swenson]

The TPS23881 has an active-low reset pin that can be connected to an
SoC.  Document this with the device-tree binding.

Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
---
 Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml b/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
index 6992d56832bf..d08abcb01211 100644
--- a/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
+++ b/Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml
@@ -21,10 +21,13 @@ properties:
     maxItems: 1
 
   '#pse-cells':
     const: 1
 
+  reset-gpios:
+    maxItems: 1
+
   channels:
     description: each set of 8 ports can be assigned to one physical
       channels or two for PoE4. This parameter describes the configuration
       of the ports conversion matrix that establishes relationship between
       the logical ports and the physical channels.
-- 
2.43.0

Re: [PATCH net-next 1/2] dt-bindings: net: pse-pd: tps23881: add reset-gpios

[Krzysztof Kozlowski]

On Mon, Aug 19, 2024 at 07:02:13PM +0000, Kyle Swenson wrote:
> The TPS23881 has an active-low reset pin that can be connected to an
> SoC.  Document this with the device-tree binding.
> 
> Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> ---
>  Documentation/devicetree/bindings/net/pse-pd/ti,tps23881.yaml | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH net-next 2/2] net: pse-pd: tps23881: support reset-gpios

[Kyle Swenson]

The TPS23880/1 has an active-low reset pin that some boards connect to
the SoC to control when the TPS23880 is pulled out of reset.

Add support for this via a reset-gpios property in the DTS.

Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
---
 drivers/net/pse-pd/tps23881.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index 2ea75686a319..837e1a2119ee 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -6,16 +6,16 @@
  */
 
 #include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pse-pd/pse.h>
-
 #define TPS23881_MAX_CHANS 8
 
 #define TPS23881_REG_PW_STATUS	0x10
 #define TPS23881_REG_OP_MODE	0x12
 #define TPS23881_OP_MODE_SEMIAUTO	0xaaaa
@@ -735,10 +735,11 @@ static int tps23881_flash_sram_fw(struct i2c_client *client)
 
 static int tps23881_i2c_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct tps23881_priv *priv;
+	struct gpio_desc *reset;
 	int ret;
 	u8 val;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		dev_err(dev, "i2c check functionality failed\n");
@@ -747,10 +748,20 @@ static int tps23881_i2c_probe(struct i2c_client *client)
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
+	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset))
+		return dev_err_probe(&client->dev, PTR_ERR(reset), "Failed to get reset GPIO\n");
+
+	if (reset) {
+		usleep_range(1000, 10000);
+		gpiod_set_value_cansleep(reset, 0); /* De-assert reset */
+		usleep_range(1000, 10000);
+	}
+
 	ret = i2c_smbus_read_byte_data(client, TPS23881_REG_DEVID);
 	if (ret < 0)
 		return ret;
 
 	if (FIELD_GET(TPS23881_REG_DEVID_MASK, ret) != TPS23881_DEVICE_ID) {
-- 
2.43.0

Re: [PATCH net-next 2/2] net: pse-pd: tps23881: support reset-gpios

[Oleksij Rempel]

Hi Kyle,

thank you for you patch.

On Mon, Aug 19, 2024 at 07:02:14PM +0000, Kyle Swenson wrote:
> The TPS23880/1 has an active-low reset pin that some boards connect to
> the SoC to control when the TPS23880 is pulled out of reset.
> 
> Add support for this via a reset-gpios property in the DTS.
> 
> Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> ---
>  drivers/net/pse-pd/tps23881.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
> index 2ea75686a319..837e1a2119ee 100644
> --- a/drivers/net/pse-pd/tps23881.c
> +++ b/drivers/net/pse-pd/tps23881.c
> @@ -6,16 +6,16 @@
>   */
>  
>  #include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/firmware.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pse-pd/pse.h>
> -

No need to remove space here.

>  #define TPS23881_MAX_CHANS 8
>  
>  #define TPS23881_REG_PW_STATUS	0x10
>  #define TPS23881_REG_OP_MODE	0x12
>  #define TPS23881_OP_MODE_SEMIAUTO	0xaaaa
> @@ -735,10 +735,11 @@ static int tps23881_flash_sram_fw(struct i2c_client *client)
>  
>  static int tps23881_i2c_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
>  	struct tps23881_priv *priv;
> +	struct gpio_desc *reset;
>  	int ret;
>  	u8 val;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>  		dev_err(dev, "i2c check functionality failed\n");
> @@ -747,10 +748,20 @@ static int tps23881_i2c_probe(struct i2c_client *client)
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		return dev_err_probe(&client->dev, PTR_ERR(reset), "Failed to get reset GPIO\n");
> +
> +	if (reset) {
> +		usleep_range(1000, 10000);
> +		gpiod_set_value_cansleep(reset, 0); /* De-assert reset */
> +		usleep_range(1000, 10000);

According to the datasheet, page 13:
https://www.ti.com/lit/ds/symlink/tps23880.pdf

Minimal reset time is 5 microseconds and the delay after power on reset should
be at least 20 milliseconds. Both sleep values should be corrected.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 2/2] net: pse-pd: tps23881: support reset-gpios

[Kyle Swenson]

Hi Oleksij,

On Tue, Aug 20, 2024 at 08:23:58AM +0200, Oleksij Rempel wrote:
> Hi Kyle,
> 
> thank you for you patch.
Thanks for the review!
> 
> On Mon, Aug 19, 2024 at 07:02:14PM +0000, Kyle Swenson wrote:
> > The TPS23880/1 has an active-low reset pin that some boards connect to
> > the SoC to control when the TPS23880 is pulled out of reset.
> > 
> > Add support for this via a reset-gpios property in the DTS.
> > 
> > Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> > ---
> >  drivers/net/pse-pd/tps23881.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
> > index 2ea75686a319..837e1a2119ee 100644
> > --- a/drivers/net/pse-pd/tps23881.c
> > +++ b/drivers/net/pse-pd/tps23881.c
> > @@ -6,16 +6,16 @@
> >   */
> >  
> >  #include <linux/bitfield.h>
> >  #include <linux/delay.h>
> >  #include <linux/firmware.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pse-pd/pse.h>
> > -
> 
> No need to remove space here.

Sorry about this, somehow I missed this gratuitous diff

> 
> >  #define TPS23881_MAX_CHANS 8
> >  
> >  #define TPS23881_REG_PW_STATUS	0x10
> >  #define TPS23881_REG_OP_MODE	0x12
> >  #define TPS23881_OP_MODE_SEMIAUTO	0xaaaa
> > @@ -735,10 +735,11 @@ static int tps23881_flash_sram_fw(struct i2c_client *client)
> >  
> >  static int tps23881_i2c_probe(struct i2c_client *client)
> >  {
> >  	struct device *dev = &client->dev;
> >  	struct tps23881_priv *priv;
> > +	struct gpio_desc *reset;
> >  	int ret;
> >  	u8 val;
> >  
> >  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> >  		dev_err(dev, "i2c check functionality failed\n");
> > @@ -747,10 +748,20 @@ static int tps23881_i2c_probe(struct i2c_client *client)
> >  
> >  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >  	if (!priv)
> >  		return -ENOMEM;
> >  
> > +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(reset))
> > +		return dev_err_probe(&client->dev, PTR_ERR(reset), "Failed to get reset GPIO\n");
> > +
> > +	if (reset) {
> > +		usleep_range(1000, 10000);
> > +		gpiod_set_value_cansleep(reset, 0); /* De-assert reset */
> > +		usleep_range(1000, 10000);
> 
> According to the datasheet, page 13:
> https://www.ti.com/lit/ds/symlink/tps23880.pdf
> 
> Minimal reset time is 5 microseconds and the delay after power on reset should
> be at least 20 milliseconds. Both sleep values should be corrected.

Sounds reasonable, I'll change the first delay to be closer to the 5us
minimum reset time.  I need to review the docs around delays to pick the
correct one for this case.

For the 2nd delay, I (now) see the 20ms you're referring to in the datasheet.

I was looking at the SRAM programming document
(https://www.ti.com/lit/pdf/SLVAE1) and it indicates we should delay the
SRAM and parity programming by at least 50ms after initial power on.

Should we guarantee we meet that 50ms requirement with the 2nd delay or
would you prefer I just meet the 20ms requirement in the datasheet?

> 
> Regards,
> Oleksij
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Thanks again for the review!

Cheers,
Kyle

Re: [PATCH net-next 2/2] net: pse-pd: tps23881: support reset-gpios

[Kyle Swenson]

On Tue, Aug 20, 2024 at 03:06:19PM -0600, Kyle Swenson wrote:
> Hi Oleksij,
> 
> On Tue, Aug 20, 2024 at 08:23:58AM +0200, Oleksij Rempel wrote:
> > Hi Kyle,
> > 
> > thank you for you patch.
> Thanks for the review!
> > 
> > On Mon, Aug 19, 2024 at 07:02:14PM +0000, Kyle Swenson wrote:
> > > The TPS23880/1 has an active-low reset pin that some boards connect to
> > > the SoC to control when the TPS23880 is pulled out of reset.
> > > 
> > > Add support for this via a reset-gpios property in the DTS.
> > > 
> > > Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> > > ---
> > >  drivers/net/pse-pd/tps23881.c | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
> > > index 2ea75686a319..837e1a2119ee 100644
> > > --- a/drivers/net/pse-pd/tps23881.c
> > > +++ b/drivers/net/pse-pd/tps23881.c
> > > @@ -6,16 +6,16 @@
> > >   */
> > >  
> > >  #include <linux/bitfield.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/firmware.h>
> > > +#include <linux/gpio/consumer.h>
> > >  #include <linux/i2c.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pse-pd/pse.h>
> > > -
> > 
> > No need to remove space here.
> 
> Sorry about this, somehow I missed this gratuitous diff
> 
> > 
> > >  #define TPS23881_MAX_CHANS 8
> > >  
> > >  #define TPS23881_REG_PW_STATUS	0x10
> > >  #define TPS23881_REG_OP_MODE	0x12
> > >  #define TPS23881_OP_MODE_SEMIAUTO	0xaaaa
> > > @@ -735,10 +735,11 @@ static int tps23881_flash_sram_fw(struct i2c_client *client)
> > >  
> > >  static int tps23881_i2c_probe(struct i2c_client *client)
> > >  {
> > >  	struct device *dev = &client->dev;
> > >  	struct tps23881_priv *priv;
> > > +	struct gpio_desc *reset;
> > >  	int ret;
> > >  	u8 val;
> > >  
> > >  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> > >  		dev_err(dev, "i2c check functionality failed\n");
> > > @@ -747,10 +748,20 @@ static int tps23881_i2c_probe(struct i2c_client *client)
> > >  
> > >  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > >  	if (!priv)
> > >  		return -ENOMEM;
> > >  
> > > +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(reset))
> > > +		return dev_err_probe(&client->dev, PTR_ERR(reset), "Failed to get reset GPIO\n");
> > > +
> > > +	if (reset) {
> > > +		usleep_range(1000, 10000);
> > > +		gpiod_set_value_cansleep(reset, 0); /* De-assert reset */
> > > +		usleep_range(1000, 10000);
> > 
> > According to the datasheet, page 13:
> > https://www.ti.com/lit/ds/symlink/tps23880.pdf
> > 
> > Minimal reset time is 5 microseconds and the delay after power on reset should
> > be at least 20 milliseconds. Both sleep values should be corrected.
> 
> Sounds reasonable, I'll change the first delay to be closer to the 5us
> minimum reset time.  I need to review the docs around delays to pick the
> correct one for this case.
> 
> For the 2nd delay, I (now) see the 20ms you're referring to in the datasheet.
> 
> I was looking at the SRAM programming document
> (https://www.ti.com/lit/pdf/SLVAE1) and it indicates we should delay the

Sorry, this is the wrong link.  Let me try again:

https://www.ti.com/lit/pdf/slvae12


> SRAM and parity programming by at least 50ms after initial power on.
> 
> Should we guarantee we meet that 50ms requirement with the 2nd delay or
> would you prefer I just meet the 20ms requirement in the datasheet?
> 
> > 
> > Regards,
> > Oleksij
> > -- 
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> Thanks again for the review!
> 
> Cheers,
> Kyle

Re: [PATCH net-next 2/2] net: pse-pd: tps23881: support reset-gpios

[Oleksij Rempel]

Hi Kyle,

On Tue, Aug 20, 2024 at 09:11:22PM +0000, Kyle Swenson wrote:
> On Tue, Aug 20, 2024 at 03:06:19PM -0600, Kyle Swenson wrote:
> > Hi Oleksij,
> > 
> > On Tue, Aug 20, 2024 at 08:23:58AM +0200, Oleksij Rempel wrote:
> > > Hi Kyle,
> > > 
> > > thank you for you patch.
> > Thanks for the review!
> > > 
> > > On Mon, Aug 19, 2024 at 07:02:14PM +0000, Kyle Swenson wrote:
> > > > The TPS23880/1 has an active-low reset pin that some boards connect to
> > > > the SoC to control when the TPS23880 is pulled out of reset.
> > > > 
> > > > Add support for this via a reset-gpios property in the DTS.
> > > > 
> > > > Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> > > > ---
> > > >  drivers/net/pse-pd/tps23881.c | 13 ++++++++++++-
> > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
> > > > index 2ea75686a319..837e1a2119ee 100644
> > > > --- a/drivers/net/pse-pd/tps23881.c
> > > > +++ b/drivers/net/pse-pd/tps23881.c
> > > > @@ -6,16 +6,16 @@
> > > >   */
> > > >  
> > > >  #include <linux/bitfield.h>
> > > >  #include <linux/delay.h>
> > > >  #include <linux/firmware.h>
> > > > +#include <linux/gpio/consumer.h>
> > > >  #include <linux/i2c.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/pse-pd/pse.h>
> > > > -
> > > 
> > > No need to remove space here.
> > 
> > Sorry about this, somehow I missed this gratuitous diff
> > 
> > > 
> > > >  #define TPS23881_MAX_CHANS 8
> > > >  
> > > >  #define TPS23881_REG_PW_STATUS	0x10
> > > >  #define TPS23881_REG_OP_MODE	0x12
> > > >  #define TPS23881_OP_MODE_SEMIAUTO	0xaaaa
> > > > @@ -735,10 +735,11 @@ static int tps23881_flash_sram_fw(struct i2c_client *client)
> > > >  
> > > >  static int tps23881_i2c_probe(struct i2c_client *client)
> > > >  {
> > > >  	struct device *dev = &client->dev;
> > > >  	struct tps23881_priv *priv;
> > > > +	struct gpio_desc *reset;
> > > >  	int ret;
> > > >  	u8 val;
> > > >  
> > > >  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> > > >  		dev_err(dev, "i2c check functionality failed\n");
> > > > @@ -747,10 +748,20 @@ static int tps23881_i2c_probe(struct i2c_client *client)
> > > >  
> > > >  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > >  	if (!priv)
> > > >  		return -ENOMEM;
> > > >  
> > > > +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > > +	if (IS_ERR(reset))
> > > > +		return dev_err_probe(&client->dev, PTR_ERR(reset), "Failed to get reset GPIO\n");
> > > > +
> > > > +	if (reset) {
> > > > +		usleep_range(1000, 10000);
> > > > +		gpiod_set_value_cansleep(reset, 0); /* De-assert reset */
> > > > +		usleep_range(1000, 10000);
> > > 
> > > According to the datasheet, page 13:
> > > https://www.ti.com/lit/ds/symlink/tps23880.pdf
> > > 
> > > Minimal reset time is 5 microseconds and the delay after power on reset should
> > > be at least 20 milliseconds. Both sleep values should be corrected.
> > 
> > Sounds reasonable, I'll change the first delay to be closer to the 5us
> > minimum reset time.  I need to review the docs around delays to pick the
> > correct one for this case.
> > 
> > For the 2nd delay, I (now) see the 20ms you're referring to in the datasheet.
> > 
> > I was looking at the SRAM programming document
> > (https://www.ti.com/lit/pdf/SLVAE1) and it indicates we should delay the
> 
> Sorry, this is the wrong link.  Let me try again:
> 
> https://www.ti.com/lit/pdf/slvae12
> 
> 
> > SRAM and parity programming by at least 50ms after initial power on.
> > 
> > Should we guarantee we meet that 50ms requirement with the 2nd delay or
> > would you prefer I just meet the 20ms requirement in the datasheet?

Ah, I see. The easiest way would be to wait 50ms, otherwise we will need
to do some more code to get this 50ms requirement.
Please add comments to the code describing the reason for one or another
delay. And, please use name and revision of documentation you used for this
choice.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |