[PATCH 0/4] sunxi: Fixes for DM I2C drivers

[PATCH 0/4] sunxi: Fixes for DM I2C drivers

[Samuel Holland]

A while back, the sunxi-specific P2WI and RSB drivers were converted to
support DM_I2C. Now they are used as non-DM drivers in SPL, and DM
drivers in U-Boot proper.

However, the DM version of the code did not fully initialize either the
controller or the connected chips. So the DM driver would only work if
the non-DM version had previously been used in SPL.

With these bug fixes and the pinctrl series, the drivers now work on
SoCs like A64 and H6, which have a PMIC but do not set it up in SPL.


Samuel Holland (4):
  i2c: sun6i_p2wi: Initialize chips in .child_pre_probe
  i2c: sun6i_p2wi: Add support for DM clocks and resets
  i2c: sun8i_rsb: Initialize chips in .child_pre_probe
  i2c: sun8i_rsb: Add support for DM clocks and resets

 drivers/i2c/sun6i_p2wi.c | 15 ++++++++++++++-
 drivers/i2c/sun8i_rsb.c  | 15 ++++++++++++++-
 2 files changed, 28 insertions(+), 2 deletions(-)

-- 
2.33.1

[PATCH 1/4] i2c: sun6i_p2wi: Initialize chips in .child_pre_probe

[Samuel Holland]

Chips attached to the P2WI bus require an initialization command before
they can be used. (Specifically, this switches the chip from I2C mode
to P2WI mode.) The driver does this in its .probe_chip hook, under the
assumption that .probe_chip is called during child probe. This is not
the case; .probe_chip is only called by dm_i2c_probe, which is intended
for use by board-level code, not for chips with OF nodes.

Since this initialization command must be run before a child chip can be
used, do it before probing each child.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/i2c/sun6i_p2wi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/sun6i_p2wi.c b/drivers/i2c/sun6i_p2wi.c
index c9e1b3fcd5..da7f540509 100644
--- a/drivers/i2c/sun6i_p2wi.c
+++ b/drivers/i2c/sun6i_p2wi.c
@@ -191,11 +191,12 @@ static int sun6i_p2wi_probe(struct udevice *bus)
 static int sun6i_p2wi_child_pre_probe(struct udevice *child)
 {
 	struct dm_i2c_chip *chip = dev_get_parent_plat(child);
+	struct udevice *bus = child->parent;
 
 	/* Ensure each transfer is for a single register. */
 	chip->flags |= DM_I2C_CHIP_RD_ADDRESS | DM_I2C_CHIP_WR_ADDRESS;
 
-	return 0;
+	return sun6i_p2wi_probe_chip(bus, chip->chip_addr, 0);
 }
 
 static const struct dm_i2c_ops sun6i_p2wi_ops = {
-- 
2.33.1

Re: [PATCH 1/4] i2c: sun6i_p2wi: Initialize chips in .child_pre_probe

[Andre Przywara]

On Thu, 17 Mar 2022 23:52:33 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi,

> Chips attached to the P2WI bus require an initialization command before
> they can be used. (Specifically, this switches the chip from I2C mode
> to P2WI mode.) The driver does this in its .probe_chip hook, under the
> assumption that .probe_chip is called during child probe. This is not
> the case; .probe_chip is only called by dm_i2c_probe, which is intended
> for use by board-level code, not for chips with OF nodes.

Indeed, I see that code path only triggered explicitly by board specific
code, none of those places being compiled for sunxi.

So without looking too deep or having the ability to test this, it looks
alright to me anyway.

Acked-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> Since this initialization command must be run before a child chip can be
> used, do it before probing each child.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/i2c/sun6i_p2wi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/sun6i_p2wi.c b/drivers/i2c/sun6i_p2wi.c
> index c9e1b3fcd5..da7f540509 100644
> --- a/drivers/i2c/sun6i_p2wi.c
> +++ b/drivers/i2c/sun6i_p2wi.c
> @@ -191,11 +191,12 @@ static int sun6i_p2wi_probe(struct udevice *bus)
>  static int sun6i_p2wi_child_pre_probe(struct udevice *child)
>  {
>  	struct dm_i2c_chip *chip = dev_get_parent_plat(child);
> +	struct udevice *bus = child->parent;
>  
>  	/* Ensure each transfer is for a single register. */
>  	chip->flags |= DM_I2C_CHIP_RD_ADDRESS | DM_I2C_CHIP_WR_ADDRESS;
>  
> -	return 0;
> +	return sun6i_p2wi_probe_chip(bus, chip->chip_addr, 0);
>  }
>  
>  static const struct dm_i2c_ops sun6i_p2wi_ops = {

[PATCH 2/4] i2c: sun6i_p2wi: Add support for DM clocks and resets

[Samuel Holland]

Currently, clock/reset setup for this device is handled by a
platform-specific function and is intermixed with non-DM pinctrl
setup. Use the devicetree to get clocks/resets, which disentagles
it from the pinctrl setup in preparation for moving to DM_PINCTRL.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/i2c/sun6i_p2wi.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/i2c/sun6i_p2wi.c b/drivers/i2c/sun6i_p2wi.c
index da7f540509..087c682a50 100644
--- a/drivers/i2c/sun6i_p2wi.c
+++ b/drivers/i2c/sun6i_p2wi.c
@@ -14,10 +14,12 @@
  */
 
 #include <axp_pmic.h>
+#include <clk.h>
 #include <common.h>
 #include <dm.h>
 #include <errno.h>
 #include <i2c.h>
+#include <reset.h>
 #include <time.h>
 #include <asm/io.h>
 #include <asm/arch/cpu.h>
@@ -180,9 +182,19 @@ static int sun6i_p2wi_probe_chip(struct udevice *bus, uint chip_addr,
 static int sun6i_p2wi_probe(struct udevice *bus)
 {
 	struct sun6i_p2wi_priv *priv = dev_get_priv(bus);
+	struct reset_ctl *reset;
+	struct clk *clk;
 
 	priv->base = dev_read_addr_ptr(bus);
 
+	reset = devm_reset_control_get(bus, NULL);
+	if (!IS_ERR(reset))
+		reset_deassert(reset);
+
+	clk = devm_clk_get(bus, NULL);
+	if (!IS_ERR(clk))
+		clk_enable(clk);
+
 	sun6i_p2wi_init(priv->base);
 
 	return 0;
-- 
2.33.1

Re: [PATCH 2/4] i2c: sun6i_p2wi: Add support for DM clocks and resets

[Andre Przywara]

On Thu, 17 Mar 2022 23:52:34 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi,

> Currently, clock/reset setup for this device is handled by a
> platform-specific function and is intermixed with non-DM pinctrl
> setup. Use the devicetree to get clocks/resets, which disentagles
> it from the pinctrl setup in preparation for moving to DM_PINCTRL.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

No ability to test this, but it looks alright, and is robust against
failures. Also we support the required clocks and resets in our clock
driver, AFAICS. So:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> 
>  drivers/i2c/sun6i_p2wi.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/i2c/sun6i_p2wi.c b/drivers/i2c/sun6i_p2wi.c
> index da7f540509..087c682a50 100644
> --- a/drivers/i2c/sun6i_p2wi.c
> +++ b/drivers/i2c/sun6i_p2wi.c
> @@ -14,10 +14,12 @@
>   */
>  
>  #include <axp_pmic.h>
> +#include <clk.h>
>  #include <common.h>
>  #include <dm.h>
>  #include <errno.h>
>  #include <i2c.h>
> +#include <reset.h>
>  #include <time.h>
>  #include <asm/io.h>
>  #include <asm/arch/cpu.h>
> @@ -180,9 +182,19 @@ static int sun6i_p2wi_probe_chip(struct udevice *bus, uint chip_addr,
>  static int sun6i_p2wi_probe(struct udevice *bus)
>  {
>  	struct sun6i_p2wi_priv *priv = dev_get_priv(bus);
> +	struct reset_ctl *reset;
> +	struct clk *clk;
>  
>  	priv->base = dev_read_addr_ptr(bus);
>  
> +	reset = devm_reset_control_get(bus, NULL);
> +	if (!IS_ERR(reset))
> +		reset_deassert(reset);
> +
> +	clk = devm_clk_get(bus, NULL);
> +	if (!IS_ERR(clk))
> +		clk_enable(clk);
> +
>  	sun6i_p2wi_init(priv->base);
>  
>  	return 0;

[PATCH 3/4] i2c: sun8i_rsb: Initialize chips in .child_pre_probe

[Samuel Holland]

Chips attached to the RSB bus require an initialization command before
they can be used. (Specifically, this command programs the chip's
runtime address.) The driver does this in its .probe_chip hook, under
the assumption that .probe_chip is called during child probe. This is
not the case; .probe_chip is only called by dm_i2c_probe, which is
intended for use by board-level code, not for chips with OF nodes.

Since this initialization command must be run before a child chip can be
used, do it before probing each child.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/i2c/sun8i_rsb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/sun8i_rsb.c b/drivers/i2c/sun8i_rsb.c
index 716b245a00..0681e398f7 100644
--- a/drivers/i2c/sun8i_rsb.c
+++ b/drivers/i2c/sun8i_rsb.c
@@ -252,11 +252,12 @@ static int sun8i_rsb_probe(struct udevice *bus)
 static int sun8i_rsb_child_pre_probe(struct udevice *child)
 {
 	struct dm_i2c_chip *chip = dev_get_parent_plat(child);
+	struct udevice *bus = child->parent;
 
 	/* Ensure each transfer is for a single register. */
 	chip->flags |= DM_I2C_CHIP_RD_ADDRESS | DM_I2C_CHIP_WR_ADDRESS;
 
-	return 0;
+	return sun8i_rsb_probe_chip(bus, chip->chip_addr, 0);
 }
 
 static const struct dm_i2c_ops sun8i_rsb_ops = {
-- 
2.33.1

Re: [PATCH 3/4] i2c: sun8i_rsb: Initialize chips in .child_pre_probe

[Andre Przywara]

On Thu, 17 Mar 2022 23:52:35 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi,

> Chips attached to the RSB bus require an initialization command before
> they can be used. (Specifically, this command programs the chip's
> runtime address.) The driver does this in its .probe_chip hook, under
> the assumption that .probe_chip is called during child probe. This is
> not the case; .probe_chip is only called by dm_i2c_probe, which is
> intended for use by board-level code, not for chips with OF nodes.
> 
> Since this initialization command must be run before a child chip can be
> used, do it before probing each child.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Same as for patch 1/4:
currently that code path is not called by any sunxi code, so without
looking too deep or having the ability to test this, it looks alright to
me:

Acked-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> 
>  drivers/i2c/sun8i_rsb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/sun8i_rsb.c b/drivers/i2c/sun8i_rsb.c
> index 716b245a00..0681e398f7 100644
> --- a/drivers/i2c/sun8i_rsb.c
> +++ b/drivers/i2c/sun8i_rsb.c
> @@ -252,11 +252,12 @@ static int sun8i_rsb_probe(struct udevice *bus)
>  static int sun8i_rsb_child_pre_probe(struct udevice *child)
>  {
>  	struct dm_i2c_chip *chip = dev_get_parent_plat(child);
> +	struct udevice *bus = child->parent;
>  
>  	/* Ensure each transfer is for a single register. */
>  	chip->flags |= DM_I2C_CHIP_RD_ADDRESS | DM_I2C_CHIP_WR_ADDRESS;
>  
> -	return 0;
> +	return sun8i_rsb_probe_chip(bus, chip->chip_addr, 0);
>  }
>  
>  static const struct dm_i2c_ops sun8i_rsb_ops = {

[PATCH 4/4] i2c: sun8i_rsb: Add support for DM clocks and resets

[Samuel Holland]

Currently, clock/reset setup for this device is handled by a
platform-specific function and is intermixed with non-DM pinctrl
setup. Use the devicetree to get clocks/resets, which disentagles
it from the pinctrl setup in preparation for moving to DM_PINCTRL.

This also has the added benefit of picking the right clock/reset
bits for H6 and new SoCs that have a rearranged PRCM MMIO space.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/i2c/sun8i_rsb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/i2c/sun8i_rsb.c b/drivers/i2c/sun8i_rsb.c
index 0681e398f7..e3a44e6a1b 100644
--- a/drivers/i2c/sun8i_rsb.c
+++ b/drivers/i2c/sun8i_rsb.c
@@ -9,10 +9,12 @@
  */
 
 #include <axp_pmic.h>
+#include <clk.h>
 #include <common.h>
 #include <dm.h>
 #include <errno.h>
 #include <i2c.h>
+#include <reset.h>
 #include <time.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/gpio.h>
@@ -243,9 +245,19 @@ static int sun8i_rsb_probe_chip(struct udevice *bus, uint chip_addr,
 static int sun8i_rsb_probe(struct udevice *bus)
 {
 	struct sun8i_rsb_priv *priv = dev_get_priv(bus);
+	struct reset_ctl *reset;
+	struct clk *clk;
 
 	priv->base = dev_read_addr_ptr(bus);
 
+	reset = devm_reset_control_get(bus, NULL);
+	if (!IS_ERR(reset))
+		reset_deassert(reset);
+
+	clk = devm_clk_get(bus, NULL);
+	if (!IS_ERR(clk))
+		clk_enable(clk);
+
 	return sun8i_rsb_init(priv->base);
 }
 
-- 
2.33.1

Re: [PATCH 4/4] i2c: sun8i_rsb: Add support for DM clocks and resets

[Andre Przywara]

On Thu, 17 Mar 2022 23:52:36 -0500
Samuel Holland <samuel@sholland.org> wrote:

> Currently, clock/reset setup for this device is handled by a
> platform-specific function and is intermixed with non-DM pinctrl
> setup. Use the devicetree to get clocks/resets, which disentagles
> it from the pinctrl setup in preparation for moving to DM_PINCTRL.
> 
> This also has the added benefit of picking the right clock/reset
> bits for H6 and new SoCs that have a rearranged PRCM MMIO space.

So does this actually work right now with the A23/A33? They have specific
clocks for the RSB gates/resets, and I don't see support for that in the
tree right now? Or this is just fine because those clocks are treated
optional and their enabling is actually redundant, in practice?

Cheers,
Andre

> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/i2c/sun8i_rsb.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/i2c/sun8i_rsb.c b/drivers/i2c/sun8i_rsb.c
> index 0681e398f7..e3a44e6a1b 100644
> --- a/drivers/i2c/sun8i_rsb.c
> +++ b/drivers/i2c/sun8i_rsb.c
> @@ -9,10 +9,12 @@
>   */
>  
>  #include <axp_pmic.h>
> +#include <clk.h>
>  #include <common.h>
>  #include <dm.h>
>  #include <errno.h>
>  #include <i2c.h>
> +#include <reset.h>
>  #include <time.h>
>  #include <asm/arch/cpu.h>
>  #include <asm/arch/gpio.h>
> @@ -243,9 +245,19 @@ static int sun8i_rsb_probe_chip(struct udevice *bus, uint chip_addr,
>  static int sun8i_rsb_probe(struct udevice *bus)
>  {
>  	struct sun8i_rsb_priv *priv = dev_get_priv(bus);
> +	struct reset_ctl *reset;
> +	struct clk *clk;
>  
>  	priv->base = dev_read_addr_ptr(bus);
>  
> +	reset = devm_reset_control_get(bus, NULL);
> +	if (!IS_ERR(reset))
> +		reset_deassert(reset);
> +
> +	clk = devm_clk_get(bus, NULL);
> +	if (!IS_ERR(clk))
> +		clk_enable(clk);
> +
>  	return sun8i_rsb_init(priv->base);
>  }
>  

Re: [PATCH 4/4] i2c: sun8i_rsb: Add support for DM clocks and resets

[Samuel Holland]

Hi Andre,

On 4/4/22 12:30 PM, Andre Przywara wrote:
> On Thu, 17 Mar 2022 23:52:36 -0500
> Samuel Holland <samuel@sholland.org> wrote:
> 
>> Currently, clock/reset setup for this device is handled by a
>> platform-specific function and is intermixed with non-DM pinctrl
>> setup. Use the devicetree to get clocks/resets, which disentagles
>> it from the pinctrl setup in preparation for moving to DM_PINCTRL.
>>
>> This also has the added benefit of picking the right clock/reset
>> bits for H6 and new SoCs that have a rearranged PRCM MMIO space.
> 
> So does this actually work right now with the A23/A33? They have specific
> clocks for the RSB gates/resets, and I don't see support for that in the
> tree right now? Or this is just fine because those clocks are treated
> optional and their enabling is actually redundant, in practice?

Yes, A23/A33 work right now because the clocks are already enabled in SPL. So
this change does nothing (positive or negative) on those SoCs, because as you
note we are missing the A23/A33 DM clock driver.

For 64-bit SoCs, where RSB is not used in SPL, this change is not redundant.
With this series plus the pinctrl series, you should now have working RSB in
U-Boot proper on A64/H6/H616. I have been testing this with the `i2c` and `pmic`
commands.

Regards,
Samuel

> Cheers,
> Andre
> 
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>  drivers/i2c/sun8i_rsb.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/i2c/sun8i_rsb.c b/drivers/i2c/sun8i_rsb.c
>> index 0681e398f7..e3a44e6a1b 100644
>> --- a/drivers/i2c/sun8i_rsb.c
>> +++ b/drivers/i2c/sun8i_rsb.c
>> @@ -9,10 +9,12 @@
>>   */
>>  
>>  #include <axp_pmic.h>
>> +#include <clk.h>
>>  #include <common.h>
>>  #include <dm.h>
>>  #include <errno.h>
>>  #include <i2c.h>
>> +#include <reset.h>
>>  #include <time.h>
>>  #include <asm/arch/cpu.h>
>>  #include <asm/arch/gpio.h>
>> @@ -243,9 +245,19 @@ static int sun8i_rsb_probe_chip(struct udevice *bus, uint chip_addr,
>>  static int sun8i_rsb_probe(struct udevice *bus)
>>  {
>>  	struct sun8i_rsb_priv *priv = dev_get_priv(bus);
>> +	struct reset_ctl *reset;
>> +	struct clk *clk;
>>  
>>  	priv->base = dev_read_addr_ptr(bus);
>>  
>> +	reset = devm_reset_control_get(bus, NULL);
>> +	if (!IS_ERR(reset))
>> +		reset_deassert(reset);
>> +
>> +	clk = devm_clk_get(bus, NULL);
>> +	if (!IS_ERR(clk))
>> +		clk_enable(clk);
>> +
>>  	return sun8i_rsb_init(priv->base);
>>  }
>>  
> 

Re: [PATCH 0/4] sunxi: Fixes for DM I2C drivers

[Andre Przywara]

On Thu, 17 Mar 2022 23:52:32 -0500
Samuel Holland <samuel@sholland.org> wrote:

> A while back, the sunxi-specific P2WI and RSB drivers were converted to
> support DM_I2C. Now they are used as non-DM drivers in SPL, and DM
> drivers in U-Boot proper.
> 
> However, the DM version of the code did not fully initialize either the
> controller or the connected chips. So the DM driver would only work if
> the non-DM version had previously been used in SPL.
> 
> With these bug fixes and the pinctrl series, the drivers now work on
> SoCs like A64 and H6, which have a PMIC but do not set it up in SPL.
> 

All merged into sunxi/master, which already landed in mainline.

Thanks!
Andre
> 
> Samuel Holland (4):
>   i2c: sun6i_p2wi: Initialize chips in .child_pre_probe
>   i2c: sun6i_p2wi: Add support for DM clocks and resets
>   i2c: sun8i_rsb: Initialize chips in .child_pre_probe
>   i2c: sun8i_rsb: Add support for DM clocks and resets
> 
>  drivers/i2c/sun6i_p2wi.c | 15 ++++++++++++++-
>  drivers/i2c/sun8i_rsb.c  | 15 ++++++++++++++-
>  2 files changed, 28 insertions(+), 2 deletions(-)
>