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

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

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

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),
-- 
2.17.0

[U-Boot] [PATCH 2/2] mvebu: turris_omnia: add note about i2c slave disable

[Baruch Siach]

Code that disables the i2c slave is now in the mvtwsi i2c driver.
Platform must enable DM_I2C to use that code. Add a comment in the code
as a reminder for the planned DM_I2C migration of Turris Omnia.

Reviewed-by: Heiko Schocher <hs@denx.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v2: Add Reviewed-by from Heiko
---
 board/CZ.NIC/turris_omnia/turris_omnia.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
index da663cf1bb0c..160d30cd795a 100644
--- a/board/CZ.NIC/turris_omnia/turris_omnia.c
+++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
@@ -321,7 +321,11 @@ int board_early_init_f(void)
 	writel(OMNIA_GPP_OUT_ENA_LOW, MVEBU_GPIO0_BASE + 0x04);
 	writel(OMNIA_GPP_OUT_ENA_MID, MVEBU_GPIO1_BASE + 0x04);
 
-	/* Disable I2C debug mode blocking 0x64 I2C address */
+	/*
+	 * Disable I2C debug mode blocking 0x64 I2C address.
+	 * Note: that would be redundant once Turris Omnia migrates to DM_I2C,
+	 * because the mvtwsi driver includes equivalent code.
+	 */
 	i2c_debug_reg = readl(MVEBU_TWSI_BASE + MVTWSI_ARMADA_DEBUG_REG);
 	i2c_debug_reg &= ~(1<<18);
 	writel(i2c_debug_reg, MVEBU_TWSI_BASE + MVTWSI_ARMADA_DEBUG_REG);
-- 
2.17.0

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

[Stefan Roese]

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

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

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

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

[Chris Packham]

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 at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

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

[Chris Packham]

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>

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

[Stefan Roese]

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

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

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

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

[Heiko Schocher]

Hello Chris,

Am 29.05.2018 um 05:49 schrieb Chris Packham:
> On Tue, May 29, 2018 at 2:39 PM Baruch Siach <baruch@tkos.co.il> wrote:
> 
>> Hi Chris,
> 
>> On Tue, May 29, 2018 at 10:32:47AM +1200, Chris Packham wrote:
>>> Did you intend to omit the u-boot mailing list?
> 
>> No, sorry. Thanks for noticing. I'll resend with the change that Stefan
>> suggested and keep your Tested-by.
> 
>>> On Tue, May 29, 2018 at 3:11 AM 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. All Armada 38x SoCs are
>>>> affected. 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>
>>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>>
>>> Tested-by: Chris Packham <judge.packham@gmail.com>
> 
>> [snip]
> 
>>>
>>>> +       /* Disable the hidden slave on i2c0 of 38x */
>>>> +       if (IS_ENABLED(CONFIG_ARMADA_38X) && bus->req_seq == 0)
>>>
>>> I'll submit a patch to add IS_ENABLED(CONFIG_KIRKWOOD) when this hits
>>> u-boot.git.
> 
>> Have you hit this issue on Kirkwood? Which one?
> 
> Custom board with Kirkwood 88F6281_A0
> 
> Before
> => i2c probe Valid chip addresses: 48 50 51 52 53 64 70 71 72 73 74
> 
> After
> => i2c probe Valid chip addresses: 48 50 51 52 53 70 71 72 73 74

May you and Siach can add this immediately in v2 ?

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

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

[Chris Packham]

On Tue, May 29, 2018 at 3:56 PM Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Chris,

> On Tue, May 29, 2018 at 03:49:44PM +1200, Chris Packham wrote:
> > On Tue, May 29, 2018 at 2:39 PM Baruch Siach <baruch@tkos.co.il> wrote:
> > > On Tue, May 29, 2018 at 10:32:47AM +1200, Chris Packham wrote:
> > > > Did you intend to omit the u-boot mailing list?
> >
> > > No, sorry. Thanks for noticing. I'll resend with the change that
Stefan
> > > suggested and keep your Tested-by.
> >
> > > > On Tue, May 29, 2018 at 3:11 AM 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. All Armada 38x SoCs
are
> > > > > affected. 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>
> > > > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > >
> > > > Tested-by: Chris Packham <judge.packham@gmail.com>
> >
> > > [snip]
> >
> > > >
> > > > > +       /* Disable the hidden slave on i2c0 of 38x */
> > > > > +       if (IS_ENABLED(CONFIG_ARMADA_38X) && bus->req_seq == 0)
> > > >
> > > > I'll submit a patch to add IS_ENABLED(CONFIG_KIRKWOOD) when this
hits
> > > > u-boot.git.
> >
> > > Have you hit this issue on Kirkwood? Which one?
> >
> > Custom board with Kirkwood 88F6281_A0
> >
> > Before
> > => i2c probe Valid chip addresses: 48 50 51 52 53 64 70 71 72 73 74
> >
> > After
> > => i2c probe Valid chip addresses: 48 50 51 52 53 70 71 72 73 74

> Since I'm resending this patch anyway, would you mind if I add
> IS_ENABLED(CONFIG_KIRKWOOD)?


I've got a patch ready for that. I'll send it your way and you can either
add it to the series or just squash it into the v2 of this patch.

> baruch

> > > > > +               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 +873,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),
> > > > > --
> > > > > 2.17.0

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

=}------------------------------------------------ooO--U--Ooo------------{=
>     - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

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

[Baruch Siach]

Hi Chris,

On Tue, May 29, 2018 at 03:49:44PM +1200, Chris Packham wrote:
> On Tue, May 29, 2018 at 2:39 PM Baruch Siach <baruch@tkos.co.il> wrote:
> > On Tue, May 29, 2018 at 10:32:47AM +1200, Chris Packham wrote:
> > > Did you intend to omit the u-boot mailing list?
> 
> > No, sorry. Thanks for noticing. I'll resend with the change that Stefan
> > suggested and keep your Tested-by.
> 
> > > On Tue, May 29, 2018 at 3:11 AM 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. All Armada 38x SoCs are
> > > > affected. 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>
> > > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > >
> > > Tested-by: Chris Packham <judge.packham@gmail.com>
> 
> > [snip]
> 
> > >
> > > > +       /* Disable the hidden slave on i2c0 of 38x */
> > > > +       if (IS_ENABLED(CONFIG_ARMADA_38X) && bus->req_seq == 0)
> > >
> > > I'll submit a patch to add IS_ENABLED(CONFIG_KIRKWOOD) when this hits
> > > u-boot.git.
> 
> > Have you hit this issue on Kirkwood? Which one?
> 
> Custom board with Kirkwood 88F6281_A0
> 
> Before
> => i2c probe Valid chip addresses: 48 50 51 52 53 64 70 71 72 73 74
> 
> After
> => i2c probe Valid chip addresses: 48 50 51 52 53 70 71 72 73 74

Since I'm resending this patch anyway, would you mind if I add 
IS_ENABLED(CONFIG_KIRKWOOD)?

baruch

> > > > +               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 +873,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),
> > > > --
> > > > 2.17.0

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

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

[Chris Packham]

On Tue, May 29, 2018 at 2:39 PM Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Chris,

> On Tue, May 29, 2018 at 10:32:47AM +1200, Chris Packham wrote:
> > Did you intend to omit the u-boot mailing list?

> No, sorry. Thanks for noticing. I'll resend with the change that Stefan
> suggested and keep your Tested-by.

> > On Tue, May 29, 2018 at 3:11 AM 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. All Armada 38x SoCs are
> > > affected. 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>
> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> >
> > Tested-by: Chris Packham <judge.packham@gmail.com>

> [snip]

> >
> > > +       /* Disable the hidden slave on i2c0 of 38x */
> > > +       if (IS_ENABLED(CONFIG_ARMADA_38X) && bus->req_seq == 0)
> >
> > I'll submit a patch to add IS_ENABLED(CONFIG_KIRKWOOD) when this hits
> > u-boot.git.

> Have you hit this issue on Kirkwood? Which one?

Custom board with Kirkwood 88F6281_A0

Before
=> i2c probe Valid chip addresses: 48 50 51 52 53 64 70 71 72 73 74

After
=> i2c probe Valid chip addresses: 48 50 51 52 53 70 71 72 73 74

> Thanks,
> baruch

> > > +               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 +873,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),
> > > --
> > > 2.17.0

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

=}------------------------------------------------ooO--U--Ooo------------{=
>     - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

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

[Baruch Siach]

Hi Chris,

On Tue, May 29, 2018 at 10:32:47AM +1200, Chris Packham wrote:
> Did you intend to omit the u-boot mailing list?

No, sorry. Thanks for noticing. I'll resend with the change that Stefan 
suggested and keep your Tested-by.

> On Tue, May 29, 2018 at 3:11 AM 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. All Armada 38x SoCs are
> > affected. 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>
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> 
> Tested-by: Chris Packham <judge.packham@gmail.com>

[snip]

> 
> > +       /* Disable the hidden slave on i2c0 of 38x */
> > +       if (IS_ENABLED(CONFIG_ARMADA_38X) && bus->req_seq == 0)
> 
> I'll submit a patch to add IS_ENABLED(CONFIG_KIRKWOOD) when this hits
> u-boot.git.

Have you hit this issue on Kirkwood? Which one?

Thanks,
baruch

> > +               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 +873,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),
> > --
> > 2.17.0

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -