diff mbox series

[U-Boot,1/2] i2c: mvtwsi: disable i2c slave on Armada 38x

Message ID 8795abbec31a71ce30ae3b149c22e0e6b7d66414.1527569133.git.baruch@tkos.co.il
State Changes Requested
Delegated to: Heiko Schocher
Headers show
Series [U-Boot,1/2] i2c: mvtwsi: disable i2c slave on Armada 38x | expand

Commit Message

Baruch Siach May 29, 2018, 4:45 a.m. UTC 
Equivalent code that disables the hidden i2c0 slave already exists in
the Turris Omnia platform specific code. But this hidden i2c0 slave that
interferes the i2c bus is not board specific. Armada 38x SoCs and at
least some Kirkwood variants are affected as well. Add code to disable
this slave to the i2c bus driver to make it work on all affected
hardware.

Use the bind callback because we want this to always run at boot,
regardless of whether U-Boot uses the i2c bus.

Cc: Rabeeh Khoury <rabeeh@solid-run.com>
Cc: Chris Packham <judge.packham@gmail.com>
Reviewed-by: Stefan Roese <sr@denx.de>
Reviewed-by: Heiko Schocher <hs@denx.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v2:
  * Use clrbits_le32 (Stefan Roese)

  * Apply to Kirkwood (Chris Packham)

  * Add review tags from Stefan and Heiko
---
 drivers/i2c/mvtwsi.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

Comments

Stefan Roese May 29, 2018, 6:11 a.m. UTC | #1 
On 29.05.2018 06:45, Baruch Siach wrote:
> Equivalent code that disables the hidden i2c0 slave already exists in
> the Turris Omnia platform specific code. But this hidden i2c0 slave that
> interferes the i2c bus is not board specific. Armada 38x SoCs and at
> least some Kirkwood variants are affected as well. Add code to disable
> this slave to the i2c bus driver to make it work on all affected
> hardware.
> 
> Use the bind callback because we want this to always run at boot,
> regardless of whether U-Boot uses the i2c bus.
> 
> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> Cc: Chris Packham <judge.packham@gmail.com>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Heiko Schocher <hs@denx.de>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> v2:
>    * Use clrbits_le32 (Stefan Roese)
> 
>    * Apply to Kirkwood (Chris Packham)
> 
>    * Add review tags from Stefan and Heiko

Please add the patch revision also to the patch subject next time:

[PATCH 1/2 v2] i2c: mvtwsi: disable i2c slave on Armada 38x

One more comment below...

> ---
>   drivers/i2c/mvtwsi.c | 23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
> index f9822e56b894..c0d9a71627db 100644
> --- a/drivers/i2c/mvtwsi.c
> +++ b/drivers/i2c/mvtwsi.c
> @@ -11,6 +11,7 @@
>   #include <i2c.h>
>   #include <linux/errno.h>
>   #include <asm/io.h>
> +#include <linux/bitops.h>
>   #include <linux/compat.h>
>   #ifdef CONFIG_DM_I2C
>   #include <dm.h>
> @@ -70,8 +71,10 @@ struct  mvtwsi_registers {
>   		u32 baudrate;	/* When writing */
>   	};
>   	u32 xtnd_slave_addr;
> -	u32 reserved[2];
> +	u32 reserved0[2];
>   	u32 soft_reset;
> +	u32 reserved1[27];
> +	u32 debug;
>   };
>   
>   #endif
> @@ -795,6 +798,23 @@ static int mvtwsi_i2c_ofdata_to_platdata(struct udevice *bus)
>   	return 0;
>   }
>   
> +static void twsi_disable_i2c_slave(struct mvtwsi_registers *twsi)
> +{
> +	clrbits_le32(&twsi->debug, BIT(18));
> +}
> +
> +static int mvtwsi_i2c_bind(struct udevice *bus)
> +{
> +	struct mvtwsi_registers *twsi = devfdt_get_addr_ptr(bus);
> +
> +	/* Disable the hidden slave in i2c0 of these platforms */
> +	if ((IS_ENABLED(CONFIG_ARMADA_38X) || IS_ENABLED(CONFIG_KIRKWOOD))

We could better use the compatible check here:

	if (device_is_compatible(dev, "marvell,mv64xxx-i2c"))

> +			&& bus->req_seq == 0)
> +		twsi_disable_i2c_slave(twsi);
> +
> +	return 0;
> +}
> +
>   static int mvtwsi_i2c_probe(struct udevice *bus)
>   {
>   	struct mvtwsi_i2c_dev *dev = dev_get_priv(bus);
> @@ -850,6 +870,7 @@ U_BOOT_DRIVER(i2c_mvtwsi) = {
>   	.name = "i2c_mvtwsi",
>   	.id = UCLASS_I2C,
>   	.of_match = mvtwsi_i2c_ids,
> +	.bind = mvtwsi_i2c_bind,
>   	.probe = mvtwsi_i2c_probe,
>   	.ofdata_to_platdata = mvtwsi_i2c_ofdata_to_platdata,
>   	.priv_auto_alloc_size = sizeof(struct mvtwsi_i2c_dev),
> 

Thanks,
Stefan
Baruch Siach May 29, 2018, 5:55 p.m. UTC | #2 
Hi Stefan,

On Tue, May 29, 2018 at 08:11:55AM +0200, Stefan Roese wrote:
> On 29.05.2018 06:45, Baruch Siach wrote:
> > Equivalent code that disables the hidden i2c0 slave already exists in
> > the Turris Omnia platform specific code. But this hidden i2c0 slave that
> > interferes the i2c bus is not board specific. Armada 38x SoCs and at
> > least some Kirkwood variants are affected as well. Add code to disable
> > this slave to the i2c bus driver to make it work on all affected
> > hardware.
> > 
> > Use the bind callback because we want this to always run at boot,
> > regardless of whether U-Boot uses the i2c bus.
> > 
> > Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> > Cc: Chris Packham <judge.packham@gmail.com>
> > Reviewed-by: Stefan Roese <sr@denx.de>
> > Reviewed-by: Heiko Schocher <hs@denx.de>
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---

[snip]

> > +static int mvtwsi_i2c_bind(struct udevice *bus)
> > +{
> > +	struct mvtwsi_registers *twsi = devfdt_get_addr_ptr(bus);
> > +
> > +	/* Disable the hidden slave in i2c0 of these platforms */
> > +	if ((IS_ENABLED(CONFIG_ARMADA_38X) || IS_ENABLED(CONFIG_KIRKWOOD))
> 
> We could better use the compatible check here:
> 
> 	if (device_is_compatible(dev, "marvell,mv64xxx-i2c"))

This is not an equivalent check. marvell,mv64xxx-i2c covers other SoCs that 
might not be affected.

Furthermore, this makes a build time test into a run time one. This bloats the 
code for platforms like Allwinner that are unlikely to be affected.

What is the advantage of device_is_compatible()? Is it feasible to build a 
multi-platform U-Boot image?

> > +			&& bus->req_seq == 0)
> > +		twsi_disable_i2c_slave(twsi);
> > +
> > +	return 0;
> > +}

baruch
Chris Packham May 30, 2018, 4:15 a.m. UTC | #3 
On Wed, May 30, 2018 at 5:55 AM Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Stefan,

> On Tue, May 29, 2018 at 08:11:55AM +0200, Stefan Roese wrote:
> > On 29.05.2018 06:45, Baruch Siach wrote:
> > > Equivalent code that disables the hidden i2c0 slave already exists in
> > > the Turris Omnia platform specific code. But this hidden i2c0 slave
that
> > > interferes the i2c bus is not board specific. Armada 38x SoCs and at
> > > least some Kirkwood variants are affected as well. Add code to disable
> > > this slave to the i2c bus driver to make it work on all affected
> > > hardware.
> > >
> > > Use the bind callback because we want this to always run at boot,
> > > regardless of whether U-Boot uses the i2c bus.
> > >
> > > Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> > > Cc: Chris Packham <judge.packham@gmail.com>
> > > Reviewed-by: Stefan Roese <sr@denx.de>
> > > Reviewed-by: Heiko Schocher <hs@denx.de>
> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > ---

> [snip]

> > > +static int mvtwsi_i2c_bind(struct udevice *bus)
> > > +{
> > > +   struct mvtwsi_registers *twsi = devfdt_get_addr_ptr(bus);
> > > +
> > > +   /* Disable the hidden slave in i2c0 of these platforms */
> > > +   if ((IS_ENABLED(CONFIG_ARMADA_38X) || IS_ENABLED(CONFIG_KIRKWOOD))
> >
> > We could better use the compatible check here:
> >
> >       if (device_is_compatible(dev, "marvell,mv64xxx-i2c"))

> This is not an equivalent check. marvell,mv64xxx-i2c covers other SoCs
that
> might not be affected.

I agree with Baruch on this. The CONFIG_ARMADA_38X/CONFIG_KIRKWOOD singles
out platforms that we know have this feature and work. Using the compatible
string Is likely to flush out platforms that this hasn't been tested on
and/or where it doesn't work.

> Furthermore, this makes a build time test into a run time one. This
bloats the
> code for platforms like Allwinner that are unlikely to be affected.

> What is the advantage of device_is_compatible()? Is it feasible to build a
> multi-platform U-Boot image?

> > > +                   && bus->req_seq == 0)
> > > +           twsi_disable_i2c_slave(twsi);
> > > +
> > > +   return 0;
> > > +}

> baruch

> --
>       http://baruch.siach.name/blog/                  ~. .~   Tk Open
Systems

=}------------------------------------------------ooO--U--Ooo------------{=
>     - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
Chris Packham May 30, 2018, 4:35 a.m. UTC | #4 
On Tue, May 29, 2018 at 4:46 PM Baruch Siach <baruch@tkos.co.il> wrote:

> Equivalent code that disables the hidden i2c0 slave already exists in
> the Turris Omnia platform specific code. But this hidden i2c0 slave that
> interferes the i2c bus is not board specific. Armada 38x SoCs and at
> least some Kirkwood variants are affected as well. Add code to disable
> this slave to the i2c bus driver to make it work on all affected
> hardware.

> Use the bind callback because we want this to always run at boot,
> regardless of whether U-Boot uses the i2c bus.

> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> Cc: Chris Packham <judge.packham@gmail.com>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Heiko Schocher <hs@denx.de>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---

Tested-by: Chris Packham <judge.packham@gmail.com>
Stefan Roese May 30, 2018, 6:27 a.m. UTC | #5 
On 29.05.2018 19:55, Baruch Siach wrote:
> Hi Stefan,
> 
> On Tue, May 29, 2018 at 08:11:55AM +0200, Stefan Roese wrote:
>> On 29.05.2018 06:45, Baruch Siach wrote:
>>> Equivalent code that disables the hidden i2c0 slave already exists in
>>> the Turris Omnia platform specific code. But this hidden i2c0 slave that
>>> interferes the i2c bus is not board specific. Armada 38x SoCs and at
>>> least some Kirkwood variants are affected as well. Add code to disable
>>> this slave to the i2c bus driver to make it work on all affected
>>> hardware.
>>>
>>> Use the bind callback because we want this to always run at boot,
>>> regardless of whether U-Boot uses the i2c bus.
>>>
>>> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
>>> Cc: Chris Packham <judge.packham@gmail.com>
>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>> Reviewed-by: Heiko Schocher <hs@denx.de>
>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>> ---
> 
> [snip]
> 
>>> +static int mvtwsi_i2c_bind(struct udevice *bus)
>>> +{
>>> +	struct mvtwsi_registers *twsi = devfdt_get_addr_ptr(bus);
>>> +
>>> +	/* Disable the hidden slave in i2c0 of these platforms */
>>> +	if ((IS_ENABLED(CONFIG_ARMADA_38X) || IS_ENABLED(CONFIG_KIRKWOOD))
>>
>> We could better use the compatible check here:
>>
>> 	if (device_is_compatible(dev, "marvell,mv64xxx-i2c"))
> 
> This is not an equivalent check. marvell,mv64xxx-i2c covers other SoCs that
> might not be affected.

I only see Marvell Kirkwood and Armada XP / A38x boards using this
compatible property.

> Furthermore, this makes a build time test into a run time one. This bloats the
> code for platforms like Allwinner that are unlikely to be affected.

Yes, this is a disadvantage.

> What is the advantage of device_is_compatible()? Is it feasible to build a
> multi-platform U-Boot image?

The main advantage is, that multiple platforms can be supported
in one single image. This is how its done in Linux. I agree, that
this is not so common in U-Boot, e.g. to support Orion and Kirkwood
in one single U-Boot image. So lets keep it this way and apply
this patch version if nobody else objects.

Thanks,
Stefan
Heiko Schocher June 6, 2018, 10:52 a.m. UTC | #6 
Hello Baruch,

Am 29.05.2018 um 06:45 schrieb Baruch Siach:
> Equivalent code that disables the hidden i2c0 slave already exists in
> the Turris Omnia platform specific code. But this hidden i2c0 slave that
> interferes the i2c bus is not board specific. Armada 38x SoCs and at
> least some Kirkwood variants are affected as well. Add code to disable
> this slave to the i2c bus driver to make it work on all affected
> hardware.
> 
> Use the bind callback because we want this to always run at boot,
> regardless of whether U-Boot uses the i2c bus.
> 
> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> Cc: Chris Packham <judge.packham@gmail.com>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Heiko Schocher <hs@denx.de>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> v2:
>    * Use clrbits_le32 (Stefan Roese)
> 
>    * Apply to Kirkwood (Chris Packham)
> 
>    * Add review tags from Stefan and Heiko
> ---
>   drivers/i2c/mvtwsi.c | 23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)

Your patch breaks build for sun8i, see travis build:

https://travis-ci.org/hsdenx/u-boot-i2c/jobs/388655845

Please fix this issue, and send a v3, thanks!

bye,
Heiko
diff mbox series

Patch

diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
index f9822e56b894..c0d9a71627db 100644
--- a/drivers/i2c/mvtwsi.c
+++ b/drivers/i2c/mvtwsi.c
@@ -11,6 +11,7 @@ 
 #include <i2c.h>
 #include <linux/errno.h>
 #include <asm/io.h>
+#include <linux/bitops.h>
 #include <linux/compat.h>
 #ifdef CONFIG_DM_I2C
 #include <dm.h>
@@ -70,8 +71,10 @@  struct  mvtwsi_registers {
 		u32 baudrate;	/* When writing */
 	};
 	u32 xtnd_slave_addr;
-	u32 reserved[2];
+	u32 reserved0[2];
 	u32 soft_reset;
+	u32 reserved1[27];
+	u32 debug;
 };
 
 #endif
@@ -795,6 +798,23 @@  static int mvtwsi_i2c_ofdata_to_platdata(struct udevice *bus)
 	return 0;
 }
 
+static void twsi_disable_i2c_slave(struct mvtwsi_registers *twsi)
+{
+	clrbits_le32(&twsi->debug, BIT(18));
+}
+
+static int mvtwsi_i2c_bind(struct udevice *bus)
+{
+	struct mvtwsi_registers *twsi = devfdt_get_addr_ptr(bus);
+
+	/* Disable the hidden slave in i2c0 of these platforms */
+	if ((IS_ENABLED(CONFIG_ARMADA_38X) || IS_ENABLED(CONFIG_KIRKWOOD))
+			&& bus->req_seq == 0)
+		twsi_disable_i2c_slave(twsi);
+
+	return 0;
+}
+
 static int mvtwsi_i2c_probe(struct udevice *bus)
 {
 	struct mvtwsi_i2c_dev *dev = dev_get_priv(bus);
@@ -850,6 +870,7 @@  U_BOOT_DRIVER(i2c_mvtwsi) = {
 	.name = "i2c_mvtwsi",
 	.id = UCLASS_I2C,
 	.of_match = mvtwsi_i2c_ids,
+	.bind = mvtwsi_i2c_bind,
 	.probe = mvtwsi_i2c_probe,
 	.ofdata_to_platdata = mvtwsi_i2c_ofdata_to_platdata,
 	.priv_auto_alloc_size = sizeof(struct mvtwsi_i2c_dev),