[PATCH v5 0/2] i2c: mv64xxx: bus-reset-gpios

[PATCH v5 0/2] i2c: mv64xxx: bus-reset-gpios

[Chris Packham]

This series adds the ability to associate a gpio with an I2C bus so that
downstream devices can be brought out of reset when the host controller is
probed.

Chris Packham (2):
  dt-bindings: i2c: mv64xxx: add bus-reset-gpios property
  i2c: mv64xxx: add an optional bus-reset-gpios property

 .../bindings/i2c/marvell,mv64xxx-i2c.yaml        | 10 ++++++++++
 drivers/i2c/busses/i2c-mv64xxx.c                 | 16 ++++++++++++++++
 2 files changed, 26 insertions(+)

-- 
2.42.0

[PATCH v5 1/2] dt-bindings: i2c: mv64xxx: add bus-reset-gpios property

[Chris Packham]

Add bus-reset-gpios and bus-reset-duration-us properties to the
marvell,mv64xxx-i2c binding. These can be used to describe hardware
where a common reset GPIO is connected to all downstream devices on and
I2C bus. This reset will be asserted then released before the downstream
devices on the bus are probed.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

Notes:
    Changes in v5:
    - Rename reset-gpios and reset-duration-us to bus-reset-gpios and
      bus-reset-duration-us as requested by Wolfram
    Changes in v4:
    - Add r-by from Krzysztof
    Changes in v3:
    - Rename reset-delay-us to reset-duration-us to better reflect its
      purpose
    - Add default: for reset-duration-us
    - Add description: for reset-gpios
    Changes in v2:
    - Update commit message
    - Add reset-delay-us property

 .../devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml   | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
index 461d1c9ee3f7..b165d1c4f0b1 100644
--- a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
@@ -70,6 +70,16 @@ properties:
   resets:
     maxItems: 1
 
+  bus-reset-gpios:
+    description:
+      GPIO pin providing a common reset for all downstream devices. This GPIO
+      will be asserted then released before the downstream devices are probed.
+    maxItems: 1
+
+  bus-reset-duration-us:
+    description: Reset duration in us.
+    default: 1
+
   dmas:
     items:
       - description: RX DMA Channel
-- 
2.42.0

[PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios property

[Chris Packham]

Some hardware designs have a GPIO used to control the reset of all the
devices on and I2C bus. It's not possible for every child node to
declare a reset-gpios property as only the first device probed would be
able to successfully request it (the others will get -EBUSY). Represent
this kind of hardware design by associating the bus-reset-gpios with the
parent I2C bus. The reset line will be released prior to the child I2C
devices being probed.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v5:
    - Rename reset-gpios and reset-duration-us to bus-reset-gpios and
      bus-reset-duration-us as requested by Wolfram
    Changes in v4:
    - Add missing gpio/consumer.h
    - use fsleep() for enforcing reset-duration
    Changes in v3:
    - Rename reset-delay to reset-duration
    - Use reset-duration-us property to control the reset pulse rather than
      delaying after the reset
    Changes in v2:
    - Add a property to cover the length of delay after releasing the reset
      GPIO
    - Use dev_err_probe() when requesing the GPIO fails

 drivers/i2c/busses/i2c-mv64xxx.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index efd28bbecf61..6e2762d22e5a 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -13,6 +13,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/mv643xx_i2c.h>
@@ -160,6 +161,7 @@ struct mv64xxx_i2c_data {
 	bool			clk_n_base_0;
 	struct i2c_bus_recovery_info	rinfo;
 	bool			atomic;
+	struct gpio_desc	*reset_gpio;
 };
 
 static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
@@ -1036,6 +1038,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 	struct mv64xxx_i2c_data		*drv_data;
 	struct mv64xxx_i2c_pdata	*pdata = dev_get_platdata(&pd->dev);
 	struct resource *res;
+	u32	reset_duration;
 	int	rc;
 
 	if ((!pdata && !pd->dev.of_node))
@@ -1083,6 +1086,14 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 	if (drv_data->irq < 0)
 		return drv_data->irq;
 
+	drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "bus-reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(drv_data->reset_gpio))
+		return dev_err_probe(&pd->dev, PTR_ERR(drv_data->reset_gpio),
+				     "Cannot get reset gpio\n");
+	rc = device_property_read_u32(&pd->dev, "bus-reset-duration-us", &reset_duration);
+	if (rc)
+		reset_duration = 1;
+
 	if (pdata) {
 		drv_data->freq_m = pdata->freq_m;
 		drv_data->freq_n = pdata->freq_n;
@@ -1121,6 +1132,11 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 			goto exit_disable_pm;
 	}
 
+	if (drv_data->reset_gpio) {
+		fsleep(reset_duration);
+		gpiod_set_value_cansleep(drv_data->reset_gpio, 0);
+	}
+
 	rc = request_irq(drv_data->irq, mv64xxx_i2c_intr, 0,
 			 MV64XXX_I2C_CTLR_NAME, drv_data);
 	if (rc) {
-- 
2.42.0

Re: [PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios property

[Andi Shyti]

Hi Chris,

On Fri, Oct 27, 2023 at 04:31:04PM +1300, Chris Packham wrote:
> Some hardware designs have a GPIO used to control the reset of all the
> devices on and I2C bus. It's not possible for every child node to
> declare a reset-gpios property as only the first device probed would be
> able to successfully request it (the others will get -EBUSY). Represent
> this kind of hardware design by associating the bus-reset-gpios with the
> parent I2C bus. The reset line will be released prior to the child I2C
> devices being probed.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v5:
>     - Rename reset-gpios and reset-duration-us to bus-reset-gpios and
>       bus-reset-duration-us as requested by Wolfram

for such change you could have kept my r-b:

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Andi

Re: [PATCH v5 1/2] dt-bindings: i2c: mv64xxx: add bus-reset-gpios property

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

On Fri, Oct 27, 2023 at 04:31:03PM +1300, Chris Packham wrote:
> Add bus-reset-gpios and bus-reset-duration-us properties to the
> marvell,mv64xxx-i2c binding. These can be used to describe hardware
> where a common reset GPIO is connected to all downstream devices on and
> I2C bus. This reset will be asserted then released before the downstream
> devices on the bus are probed.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Krzysztof, are you fine with this change?

> ---
> 
> Notes:
>     Changes in v5:
>     - Rename reset-gpios and reset-duration-us to bus-reset-gpios and
>       bus-reset-duration-us as requested by Wolfram
>     Changes in v4:
>     - Add r-by from Krzysztof
>     Changes in v3:
>     - Rename reset-delay-us to reset-duration-us to better reflect its
>       purpose
>     - Add default: for reset-duration-us
>     - Add description: for reset-gpios
>     Changes in v2:
>     - Update commit message
>     - Add reset-delay-us property
> 
>  .../devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml   | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> index 461d1c9ee3f7..b165d1c4f0b1 100644
> --- a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> @@ -70,6 +70,16 @@ properties:
>    resets:
>      maxItems: 1
>  
> +  bus-reset-gpios:
> +    description:
> +      GPIO pin providing a common reset for all downstream devices. This GPIO
> +      will be asserted then released before the downstream devices are probed.
> +    maxItems: 1
> +
> +  bus-reset-duration-us:
> +    description: Reset duration in us.
> +    default: 1
> +
>    dmas:
>      items:
>        - description: RX DMA Channel
> -- 
> 2.42.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 1/2] dt-bindings: i2c: mv64xxx: add bus-reset-gpios property

[Krzysztof Kozlowski]

On 27/10/2023 11:09, Wolfram Sang wrote:
> On Fri, Oct 27, 2023 at 04:31:03PM +1300, Chris Packham wrote:
>> Add bus-reset-gpios and bus-reset-duration-us properties to the
>> marvell,mv64xxx-i2c binding. These can be used to describe hardware
>> where a common reset GPIO is connected to all downstream devices on and
>> I2C bus. This reset will be asserted then released before the downstream
>> devices on the bus are probed.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Krzysztof, are you fine with this change?

Actually no. NAK.

Not because of the naming, but because the new name triggered some new
paths in my brain which brought the point - this is old problem of power
sequencing of children.

I believe this must be solved in more generic way. First - generic for
all I2C devices. Second - generic also matching other buses/subsystems,
which have similar problem. We did it for USB (onboard USB), MMC
(unloved MMC power sequence) and now we are doing it for PCIe and few
others (Cc: Abel)

https://lpc.events/event/17/contributions/1507/

Current solution is heavily limited. What about regulators? What about
buses having 2 reset lines (still the same bus)? What about sequence?

Best regards,
Krzysztof

Re: [PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios property

[Krzysztof Kozlowski]

On 27/10/2023 05:31, Chris Packham wrote:
> Some hardware designs have a GPIO used to control the reset of all the
> devices on and I2C bus. It's not possible for every child node to
> declare a reset-gpios property as only the first device probed would be
> able to successfully request it (the others will get -EBUSY). Represent
> this kind of hardware design by associating the bus-reset-gpios with the
> parent I2C bus. The reset line will be released prior to the child I2C
> devices being probed.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v5:
>     - Rename reset-gpios and reset-duration-us to bus-reset-gpios and
>       bus-reset-duration-us as requested by Wolfram
>     Changes in v4:
>     - Add missing gpio/consumer.h
>     - use fsleep() for enforcing reset-duration
>     Changes in v3:
>     - Rename reset-delay to reset-duration
>     - Use reset-duration-us property to control the reset pulse rather than
>       delaying after the reset
>     Changes in v2:
>     - Add a property to cover the length of delay after releasing the reset
>       GPIO
>     - Use dev_err_probe() when requesing the GPIO fails
> 
>  drivers/i2c/busses/i2c-mv64xxx.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index efd28bbecf61..6e2762d22e5a 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -13,6 +13,7 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/spinlock.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/mv643xx_i2c.h>
> @@ -160,6 +161,7 @@ struct mv64xxx_i2c_data {
>  	bool			clk_n_base_0;
>  	struct i2c_bus_recovery_info	rinfo;
>  	bool			atomic;
> +	struct gpio_desc	*reset_gpio;
>  };
>  
>  static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
> @@ -1036,6 +1038,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>  	struct mv64xxx_i2c_data		*drv_data;
>  	struct mv64xxx_i2c_pdata	*pdata = dev_get_platdata(&pd->dev);
>  	struct resource *res;
> +	u32	reset_duration;
>  	int	rc;
>  
>  	if ((!pdata && !pd->dev.of_node))
> @@ -1083,6 +1086,14 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>  	if (drv_data->irq < 0)
>  		return drv_data->irq;
>  
> +	drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "bus-reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(drv_data->reset_gpio))
> +		return dev_err_probe(&pd->dev, PTR_ERR(drv_data->reset_gpio),
> +				     "Cannot get reset gpio\n");
> +	rc = device_property_read_u32(&pd->dev, "bus-reset-duration-us", &reset_duration);
> +	if (rc)
> +		reset_duration = 1;

No, this should be solved by core - for entire I2C at minimum. This is
not specific to this device.

Best regards,
Krzysztof

Re: [PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios property

[Krzysztof Kozlowski]

On 27/10/2023 13:27, Krzysztof Kozlowski wrote:
> On 27/10/2023 05:31, Chris Packham wrote:
>> Some hardware designs have a GPIO used to control the reset of all the
>> devices on and I2C bus. It's not possible for every child node to
>> declare a reset-gpios property as only the first device probed would be
>> able to successfully request it (the others will get -EBUSY). Represent

Cc: Mark,

Also this part is not true. If the bus is non-discoverable, then it is
possible to have reset-gpios in each probed device. You can share GPIOs,
so no problem with -EBUSY at all.

The problem is doing reset:
1. in proper moment for all devices
2. without affecting other devices when one unbinds/remove()

The (2) above is not solveable easy in kernel and we already had nice
talks about it just few days ago:
1. Apple case:
https://social.treehouse.systems/@marcan/111268780311634160

2. my WSA884x:
https://lore.kernel.org/alsa-devel/84f9f1c4-0627-4986-8160-b4ab99469b81@linaro.org/

Last,
I would like to apologize to you Chris. I understand that bringing such
feedback at v5 is not that good. I had plenty of time to say something
earlier, so this is not really professional from my side. I am sorry,
just my brain did not connect all these topics together.

I apologize.

Best regards,
Krzysztof

>> this kind of hardware design by associating the bus-reset-gpios with the
>> parent I2C bus. The reset line will be released prior to the child I2C
>> devices being probed.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>     Changes in v5:
>>     - Rename reset-gpios and reset-duration-us to bus-reset-gpios and
>>       bus-reset-duration-us as requested by Wolfram
>>     Changes in v4:
>>     - Add missing gpio/consumer.h
>>     - use fsleep() for enforcing reset-duration
>>     Changes in v3:
>>     - Rename reset-delay to reset-duration
>>     - Use reset-duration-us property to control the reset pulse rather than
>>       delaying after the reset
>>     Changes in v2:
>>     - Add a property to cover the length of delay after releasing the reset
>>       GPIO
>>     - Use dev_err_probe() when requesing the GPIO fails
>>
>>  drivers/i2c/busses/i2c-mv64xxx.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
>> index efd28bbecf61..6e2762d22e5a 100644
>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/module.h>
>>  #include <linux/spinlock.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/i2c.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/mv643xx_i2c.h>
>> @@ -160,6 +161,7 @@ struct mv64xxx_i2c_data {
>>  	bool			clk_n_base_0;
>>  	struct i2c_bus_recovery_info	rinfo;
>>  	bool			atomic;
>> +	struct gpio_desc	*reset_gpio;
>>  };
>>  
>>  static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>> @@ -1036,6 +1038,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>  	struct mv64xxx_i2c_data		*drv_data;
>>  	struct mv64xxx_i2c_pdata	*pdata = dev_get_platdata(&pd->dev);
>>  	struct resource *res;
>> +	u32	reset_duration;
>>  	int	rc;
>>  
>>  	if ((!pdata && !pd->dev.of_node))
>> @@ -1083,6 +1086,14 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>  	if (drv_data->irq < 0)
>>  		return drv_data->irq;
>>  
>> +	drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "bus-reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(drv_data->reset_gpio))
>> +		return dev_err_probe(&pd->dev, PTR_ERR(drv_data->reset_gpio),
>> +				     "Cannot get reset gpio\n");
>> +	rc = device_property_read_u32(&pd->dev, "bus-reset-duration-us", &reset_duration);
>> +	if (rc)
>> +		reset_duration = 1;
> 
> No, this should be solved by core - for entire I2C at minimum. This is
> not specific to this device.

Re: [PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios property

[Andi Shyti]

Hi Krzysztof,

On Fri, Oct 27, 2023 at 01:37:05PM +0200, Krzysztof Kozlowski wrote:
> On 27/10/2023 13:27, Krzysztof Kozlowski wrote:
> > On 27/10/2023 05:31, Chris Packham wrote:
> >> Some hardware designs have a GPIO used to control the reset of all the
> >> devices on and I2C bus. It's not possible for every child node to
> >> declare a reset-gpios property as only the first device probed would be
> >> able to successfully request it (the others will get -EBUSY). Represent
> 
> Cc: Mark,
> 
> Also this part is not true. If the bus is non-discoverable, then it is
> possible to have reset-gpios in each probed device. You can share GPIOs,
> so no problem with -EBUSY at all.
> 
> The problem is doing reset:
> 1. in proper moment for all devices
> 2. without affecting other devices when one unbinds/remove()

yes, I thought that we could get to this point, but I did not
object the patch as I didn't see an immediate better solution. I
would still be OK to merge it until we develop something better.

Let me mull this over and will be back to the topic.

Thanks, Krzysztof!
Andi

> The (2) above is not solveable easy in kernel and we already had nice
> talks about it just few days ago:
> 1. Apple case:
> https://social.treehouse.systems/@marcan/111268780311634160
> 
> 2. my WSA884x:
> https://lore.kernel.org/alsa-devel/84f9f1c4-0627-4986-8160-b4ab99469b81@linaro.org/
> 
> Last,
> I would like to apologize to you Chris. I understand that bringing such
> feedback at v5 is not that good. I had plenty of time to say something
> earlier, so this is not really professional from my side. I am sorry,
> just my brain did not connect all these topics together.
> 
> I apologize.
> 
> Best regards,
> Krzysztof

Re: [PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

On Fri, Oct 27, 2023 at 01:37:05PM +0200, Krzysztof Kozlowski wrote:

> The (2) above is not solveable easy in kernel and we already had nice
> talks about it just few days ago:
> 1. Apple case:
> https://social.treehouse.systems/@marcan/111268780311634160

Note that the Apple case is not a reset, it's a low power mode for the
device, though at the GPIO level the beheviour with tying together
multiple devices that need to coordinate their usage looks very similar.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 1/2] dt-bindings: i2c: mv64xxx: add bus-reset-gpios property

[Rob Herring]

On Fri, Oct 27, 2023 at 01:25:56PM +0200, Krzysztof Kozlowski wrote:
> On 27/10/2023 11:09, Wolfram Sang wrote:
> > On Fri, Oct 27, 2023 at 04:31:03PM +1300, Chris Packham wrote:
> >> Add bus-reset-gpios and bus-reset-duration-us properties to the
> >> marvell,mv64xxx-i2c binding. These can be used to describe hardware
> >> where a common reset GPIO is connected to all downstream devices on and
> >> I2C bus. This reset will be asserted then released before the downstream
> >> devices on the bus are probed.
> >>
> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > 
> > Krzysztof, are you fine with this change?
> 
> Actually no. NAK.
> 
> Not because of the naming, but because the new name triggered some new
> paths in my brain which brought the point - this is old problem of power
> sequencing of children.
> 
> I believe this must be solved in more generic way. First - generic for
> all I2C devices. Second - generic also matching other buses/subsystems,
> which have similar problem. We did it for USB (onboard USB), MMC
> (unloved MMC power sequence) and now we are doing it for PCIe and few
> others (Cc: Abel)

Unlike the others I2C doesn't expect to access the bus/device before 
devices probe, right?

> https://lpc.events/event/17/contributions/1507/

Oh, good!

> Current solution is heavily limited. What about regulators? What about
> buses having 2 reset lines (still the same bus)? What about sequence?

A more complicated case should be handled by the device's driver. If the 
GPIO reset was not shared we'd be handling it there too. I think what's 
needed is to solve the shared aspect. That's already done with reset 
subsys, so I think making 'reset-gpios' handled by it too is the way 
forward. That would handle the QCA WiFi/BT case I think.

I'm not sure waiting for that or something else to happen is worth 
holding up this simple case. It's not the only case of a common reset 
for a bus (MDIO).

Rob

Re: [PATCH v5 1/2] dt-bindings: i2c: mv64xxx: add bus-reset-gpios property

[Krzysztof Kozlowski]

On 27/10/2023 21:22, Rob Herring wrote:
> On Fri, Oct 27, 2023 at 01:25:56PM +0200, Krzysztof Kozlowski wrote:
>> On 27/10/2023 11:09, Wolfram Sang wrote:
>>> On Fri, Oct 27, 2023 at 04:31:03PM +1300, Chris Packham wrote:
>>>> Add bus-reset-gpios and bus-reset-duration-us properties to the
>>>> marvell,mv64xxx-i2c binding. These can be used to describe hardware
>>>> where a common reset GPIO is connected to all downstream devices on and
>>>> I2C bus. This reset will be asserted then released before the downstream
>>>> devices on the bus are probed.
>>>>
>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>
>>> Krzysztof, are you fine with this change?
>>
>> Actually no. NAK.
>>
>> Not because of the naming, but because the new name triggered some new
>> paths in my brain which brought the point - this is old problem of power
>> sequencing of children.
>>
>> I believe this must be solved in more generic way. First - generic for
>> all I2C devices. Second - generic also matching other buses/subsystems,
>> which have similar problem. We did it for USB (onboard USB), MMC
>> (unloved MMC power sequence) and now we are doing it for PCIe and few
>> others (Cc: Abel)
> 
> Unlike the others I2C doesn't expect to access the bus/device before 
> devices probe, right?
> 
>> https://lpc.events/event/17/contributions/1507/
> 
> Oh, good!
> 
>> Current solution is heavily limited. What about regulators? What about
>> buses having 2 reset lines (still the same bus)? What about sequence?
> 
> A more complicated case should be handled by the device's driver. If the 
> GPIO reset was not shared we'd be handling it there too. I think what's 
> needed is to solve the shared aspect. That's already done with reset 
> subsys, so I think making 'reset-gpios' handled by it too is the way 
> forward. That would handle the QCA WiFi/BT case I think.
> 
> I'm not sure waiting for that or something else to happen is worth 
> holding up this simple case. It's not the only case of a common reset 
> for a bus (MDIO).

I argue also that this bus-reset-gpios is not a property of this I2C
controller. IIUC, the I2C controller does not have a line to reset all
children. It's the children who have reset lines and it happens it is
shared. Just like my WSA884x case:
https://lore.kernel.org/alsa-devel/84f9f1c4-0627-4986-8160-b4ab99469b81@linaro.org/


Best regards,
Krzysztof

Re: [PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios property

[Chris Packham]

On 28/10/23 00:37, Krzysztof Kozlowski wrote:
> On 27/10/2023 13:27, Krzysztof Kozlowski wrote:
>> On 27/10/2023 05:31, Chris Packham wrote:
>>> Some hardware designs have a GPIO used to control the reset of all the
>>> devices on and I2C bus. It's not possible for every child node to
>>> declare a reset-gpios property as only the first device probed would be
>>> able to successfully request it (the others will get -EBUSY). Represent
> Cc: Mark,
>
> Also this part is not true. If the bus is non-discoverable, then it is
> possible to have reset-gpios in each probed device. You can share GPIOs,
> so no problem with -EBUSY at all.

Last time I checked you couldn't share GPIOs. If that's no longer the 
case then I can probably make what I need to happen work. It still 
creates an issue that I have multiple PCA954x muxes connected to a 
common reset GPIO so as each mux is probed the PCA954x driver will 
toggle the reset. That's probably OK as the PCA954x is sufficiently 
stateless that the extra resets won't do any harm but if it were a more 
complicated device then there would be issues.

Having some kind of ref-counted reset controller that is implemented 
with GPIOs is probably the better solution. I was kind of surprised that 
nothing existed like that in drivers/reset.

> The problem is doing reset:
> 1. in proper moment for all devices
> 2. without affecting other devices when one unbinds/remove()
>
> The (2) above is not solveable easy in kernel and we already had nice
> talks about it just few days ago:
> 1. Apple case:
> https://scanmail.trustwave.com/?c=20988&d=6qC75SLs-9PNM1ZHpLa6reGv82R6opEUmyI62vCytQ&u=https%3a%2f%2fsocial%2etreehouse%2esystems%2f%40marcan%2f111268780311634160
>
> 2. my WSA884x:
> https://scanmail.trustwave.com/?c=20988&d=6qC75SLs-9PNM1ZHpLa6reGv82R6opEUmyJk3q3j7g&u=https%3a%2f%2flore%2ekernel%2eorg%2falsa-devel%2f84f9f1c4-0627-4986-8160-b4ab99469b81%40linaro%2eorg%2f
Apologies for the mangled links (they're more secure now at least that's 
what our IS team have been sold).
> Last,
> I would like to apologize to you Chris. I understand that bringing such
> feedback at v5 is not that good. I had plenty of time to say something
> earlier, so this is not really professional from my side. I am sorry,
> just my brain did not connect all these topics together.
>
> I apologize.

Actually I kind of expected this feedback. I figured I could start with 
the driver that is currently causing me issues and once the dt-binding 
was considered good enough it might migrate to the i2c core.

>
> Best regards,
> Krzysztof
>
>>> this kind of hardware design by associating the bus-reset-gpios with the
>>> parent I2C bus. The reset line will be released prior to the child I2C
>>> devices being probed.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> ---
>>>
>>> Notes:
>>>      Changes in v5:
>>>      - Rename reset-gpios and reset-duration-us to bus-reset-gpios and
>>>        bus-reset-duration-us as requested by Wolfram
>>>      Changes in v4:
>>>      - Add missing gpio/consumer.h
>>>      - use fsleep() for enforcing reset-duration
>>>      Changes in v3:
>>>      - Rename reset-delay to reset-duration
>>>      - Use reset-duration-us property to control the reset pulse rather than
>>>        delaying after the reset
>>>      Changes in v2:
>>>      - Add a property to cover the length of delay after releasing the reset
>>>        GPIO
>>>      - Use dev_err_probe() when requesing the GPIO fails
>>>
>>>   drivers/i2c/busses/i2c-mv64xxx.c | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
>>> index efd28bbecf61..6e2762d22e5a 100644
>>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>>> @@ -13,6 +13,7 @@
>>>   #include <linux/slab.h>
>>>   #include <linux/module.h>
>>>   #include <linux/spinlock.h>
>>> +#include <linux/gpio/consumer.h>
>>>   #include <linux/i2c.h>
>>>   #include <linux/interrupt.h>
>>>   #include <linux/mv643xx_i2c.h>
>>> @@ -160,6 +161,7 @@ struct mv64xxx_i2c_data {
>>>   	bool			clk_n_base_0;
>>>   	struct i2c_bus_recovery_info	rinfo;
>>>   	bool			atomic;
>>> +	struct gpio_desc	*reset_gpio;
>>>   };
>>>   
>>>   static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>>> @@ -1036,6 +1038,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>>   	struct mv64xxx_i2c_data		*drv_data;
>>>   	struct mv64xxx_i2c_pdata	*pdata = dev_get_platdata(&pd->dev);
>>>   	struct resource *res;
>>> +	u32	reset_duration;
>>>   	int	rc;
>>>   
>>>   	if ((!pdata && !pd->dev.of_node))
>>> @@ -1083,6 +1086,14 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>>   	if (drv_data->irq < 0)
>>>   		return drv_data->irq;
>>>   
>>> +	drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "bus-reset", GPIOD_OUT_HIGH);
>>> +	if (IS_ERR(drv_data->reset_gpio))
>>> +		return dev_err_probe(&pd->dev, PTR_ERR(drv_data->reset_gpio),
>>> +				     "Cannot get reset gpio\n");
>>> +	rc = device_property_read_u32(&pd->dev, "bus-reset-duration-us", &reset_duration);
>>> +	if (rc)
>>> +		reset_duration = 1;
>> No, this should be solved by core - for entire I2C at minimum. This is
>> not specific to this device.

Re: [PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios property

[Chris Packham]

On 28/10/23 01:55, Andi Shyti wrote:
> Hi Krzysztof,
>
> On Fri, Oct 27, 2023 at 01:37:05PM +0200, Krzysztof Kozlowski wrote:
>> On 27/10/2023 13:27, Krzysztof Kozlowski wrote:
>>> On 27/10/2023 05:31, Chris Packham wrote:
>>>> Some hardware designs have a GPIO used to control the reset of all the
>>>> devices on and I2C bus. It's not possible for every child node to
>>>> declare a reset-gpios property as only the first device probed would be
>>>> able to successfully request it (the others will get -EBUSY). Represent
>> Cc: Mark,
>>
>> Also this part is not true. If the bus is non-discoverable, then it is
>> possible to have reset-gpios in each probed device. You can share GPIOs,
>> so no problem with -EBUSY at all.
>>
>> The problem is doing reset:
>> 1. in proper moment for all devices
>> 2. without affecting other devices when one unbinds/remove()
> yes, I thought that we could get to this point, but I did not
> object the patch as I didn't see an immediate better solution. I
> would still be OK to merge it until we develop something better.
>
> Let me mull this over and will be back to the topic.

If we're happy with plain GPIOs I can move what I've done so far to 
somewhere in the I2C core. I know we've got other hardware designs with 
different controllers that also have muxes connected to a common reset 
GPIO so I would have ended up moving this code to I2C core eventually.

If we're talking a proper reset driver implemented using GPIOs then that 
might be a bit of bigger task.

> Thanks, Krzysztof!
> Andi
>
>> The (2) above is not solveable easy in kernel and we already had nice
>> talks about it just few days ago:
>> 1. Apple case:
>> https://scanmail.trustwave.com/?c=20988&d=1LO75R2nre1LP3TyEWMYg1Is4Mz-YROPQ8JxsJqwkg&u=https%3a%2f%2fsocial%2etreehouse%2esystems%2f%40marcan%2f111268780311634160
>>
>> 2. my WSA884x:
>> https://scanmail.trustwave.com/?c=20988&d=1LO75R2nre1LP3TyEWMYg1Is4Mz-YROPQ8IvtMfhyQ&u=https%3a%2f%2flore%2ekernel%2eorg%2falsa-devel%2f84f9f1c4-0627-4986-8160-b4ab99469b81%40linaro%2eorg%2f
>>
>> Last,
>> I would like to apologize to you Chris. I understand that bringing such
>> feedback at v5 is not that good. I had plenty of time to say something
>> earlier, so this is not really professional from my side. I am sorry,
>> just my brain did not connect all these topics together.
>>
>> I apologize.
>>
>> Best regards,
>> Krzysztof

Re: [PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios property

[Krzysztof Kozlowski]

On 29/10/2023 21:48, Chris Packham wrote:
> 
> On 28/10/23 00:37, Krzysztof Kozlowski wrote:
>> On 27/10/2023 13:27, Krzysztof Kozlowski wrote:
>>> On 27/10/2023 05:31, Chris Packham wrote:
>>>> Some hardware designs have a GPIO used to control the reset of all the
>>>> devices on and I2C bus. It's not possible for every child node to
>>>> declare a reset-gpios property as only the first device probed would be
>>>> able to successfully request it (the others will get -EBUSY). Represent
>> Cc: Mark,
>>
>> Also this part is not true. If the bus is non-discoverable, then it is
>> possible to have reset-gpios in each probed device. You can share GPIOs,
>> so no problem with -EBUSY at all.
> 
> Last time I checked you couldn't share GPIOs. If that's no longer the 
> case then I can probably make what I need to happen work. It still 
> creates an issue that I have multiple PCA954x muxes connected to a 
> common reset GPIO so as each mux is probed the PCA954x driver will 
> toggle the reset. That's probably OK as the PCA954x is sufficiently 
> stateless that the extra resets won't do any harm but if it were a more 
> complicated device then there would be issues.

I know, but this is a broader problem, not really specific to this one
device. I also argue that your I2C controller does not actually have
this reset line.

> 
> Having some kind of ref-counted reset controller that is implemented 
> with GPIOs is probably the better solution. I was kind of surprised that 
> nothing existed like that in drivers/reset.

reset controller framework already supports this. The point is that GPIO
reset is not a reset controller, so in terms of bindings "resets"
property does not fit it.

> 
>> The problem is doing reset:
>> 1. in proper moment for all devices
>> 2. without affecting other devices when one unbinds/remove()
>>
>> The (2) above is not solveable easy in kernel and we already had nice
>> talks about it just few days ago:
>> 1. Apple case:
>> https://scanmail.trustwave.com/?c=20988&d=6qC75SLs-9PNM1ZHpLa6reGv82R6opEUmyI62vCytQ&u=https%3a%2f%2fsocial%2etreehouse%2esystems%2f%40marcan%2f111268780311634160
>>
>> 2. my WSA884x:
>> https://scanmail.trustwave.com/?c=20988&d=6qC75SLs-9PNM1ZHpLa6reGv82R6opEUmyJk3q3j7g&u=https%3a%2f%2flore%2ekernel%2eorg%2falsa-devel%2f84f9f1c4-0627-4986-8160-b4ab99469b81%40linaro%2eorg%2f
> Apologies for the mangled links (they're more secure now at least that's 
> what our IS team have been sold).
>> Last,
>> I would like to apologize to you Chris. I understand that bringing such
>> feedback at v5 is not that good. I had plenty of time to say something
>> earlier, so this is not really professional from my side. I am sorry,
>> just my brain did not connect all these topics together.
>>
>> I apologize.
> 
> Actually I kind of expected this feedback. I figured I could start with 
> the driver that is currently causing me issues and once the dt-binding 
> was considered good enough it might migrate to the i2c core.
> 
>>


Best regards,
Krzysztof

Re: [PATCH v5 2/2] i2c: mv64xxx: add an optional bus-reset-gpios property

[Chris Packham]

On 31/10/23 19:01, Krzysztof Kozlowski wrote:
> On 29/10/2023 21:48, Chris Packham wrote:
>> On 28/10/23 00:37, Krzysztof Kozlowski wrote:
>>> On 27/10/2023 13:27, Krzysztof Kozlowski wrote:
>>>> On 27/10/2023 05:31, Chris Packham wrote:
>>>>> Some hardware designs have a GPIO used to control the reset of all the
>>>>> devices on and I2C bus. It's not possible for every child node to
>>>>> declare a reset-gpios property as only the first device probed would be
>>>>> able to successfully request it (the others will get -EBUSY). Represent
>>> Cc: Mark,
>>>
>>> Also this part is not true. If the bus is non-discoverable, then it is
>>> possible to have reset-gpios in each probed device. You can share GPIOs,
>>> so no problem with -EBUSY at all.
>> Last time I checked you couldn't share GPIOs. If that's no longer the
>> case then I can probably make what I need to happen work. It still
>> creates an issue that I have multiple PCA954x muxes connected to a
>> common reset GPIO so as each mux is probed the PCA954x driver will
>> toggle the reset. That's probably OK as the PCA954x is sufficiently
>> stateless that the extra resets won't do any harm but if it were a more
>> complicated device then there would be issues.
> I know, but this is a broader problem, not really specific to this one
> device. I also argue that your I2C controller does not actually have
> this reset line.

Yes absolutely. Because the reset line is common to multiple pca954x 
muxes the only option I have (I think) is to associate the reset line 
with the controller. It happens to be true for my case that everything 
connected to that bus is affected by the reset line but I can completely 
see that there may be other designs where there are a mix of muxes and 
other devices on the root bus.

So associating the reset line with the I2C controller is a pragmatic 
solution (or an egregious hack depending on your point of view) that 
works with this kind of hardware design.

Another complete hack I've experimented with is adding the muxes defined 
with `status = "disabled";` in the dts and having a custom driver that 
requests the GPIO and manipulates the live device tree. It works but is 
quite a lot more code and will invariably break if I need to tweak the 
device tree.

>> Having some kind of ref-counted reset controller that is implemented
>> with GPIOs is probably the better solution. I was kind of surprised that
>> nothing existed like that in drivers/reset.
> reset controller framework already supports this. The point is that GPIO
> reset is not a reset controller, so in terms of bindings "resets"
> property does not fit it.

So I need some way of representing a GPIO line associated with multiple 
devices that must be requested and driven appropriately before the 
devices are probed. In lieu of anything else a "bus-reset-gpios" 
property recognized by the generic I2C framework kind of sounds like the 
best solution so far. Unless maybe there's some kind of pinctrl type 
thing that would already work.

>>> The problem is doing reset:
>>> 1. in proper moment for all devices
>>> 2. without affecting other devices when one unbinds/remove()
>>>
>>> The (2) above is not solveable easy in kernel and we already had nice
>>> talks about it just few days ago:
>>> 1. Apple case:
>>> https://scanmail.trustwave.com/?c=20988&d=tZjA5R77ysliRyWfDvgX9JnmLZr-TqhRWpYjsNO-5A&u=https%3a%2f%2fsocial%2etreehouse%2esystems%2f%40marcan%2f111268780311634160
>>>
>>> 2. my WSA884x:
>>> https://scanmail.trustwave.com/?c=20988&d=tZjA5R77ysliRyWfDvgX9JnmLZr-TqhRWpZ9tI7vvw&u=https%3a%2f%2flore%2ekernel%2eorg%2falsa-devel%2f84f9f1c4-0627-4986-8160-b4ab99469b81%40linaro%2eorg%2f
>> Apologies for the mangled links (they're more secure now at least that's
>> what our IS team have been sold).
>>> Last,
>>> I would like to apologize to you Chris. I understand that bringing such
>>> feedback at v5 is not that good. I had plenty of time to say something
>>> earlier, so this is not really professional from my side. I am sorry,
>>> just my brain did not connect all these topics together.
>>>
>>> I apologize.
>> Actually I kind of expected this feedback. I figured I could start with
>> the driver that is currently causing me issues and once the dt-binding
>> was considered good enough it might migrate to the i2c core.
>>
>
> Best regards,
> Krzysztof
>