diff mbox series

[U-Boot] arm: mvebu: move i2c slave disable to generic SPL code

Message ID 9accb099dc87fd17cd6c352c7df1c7e55cd2a838.1527434746.git.baruch@tkos.co.il
State Rejected
Delegated to: Stefan Roese
Headers show
Series [U-Boot] arm: mvebu: move i2c slave disable to generic SPL code | expand

Commit Message

Baruch Siach May 27, 2018, 3:25 p.m. UTC
The hidden i2c slave that interferes the i2c bus is not board specific.
All Armada 38x SoCs are affected. Move the code disabling this slave to
generic code to make it work on all affected hardware.

Cc: Marek Behún <marek.behun@nic.cz>
Cc: Rabeeh Khoury <rabeeh@solid-run.com>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 arch/arm/mach-mvebu/spl.c                | 16 ++++++++++++++++
 board/CZ.NIC/turris_omnia/turris_omnia.c |  9 ---------
 2 files changed, 16 insertions(+), 9 deletions(-)

Comments

Chris Packham May 28, 2018, 8:11 a.m. UTC | #1
On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:

> The hidden i2c slave that interferes the i2c bus is not board specific.
> All Armada 38x SoCs are affected. Move the code disabling this slave to
> generic code to make it work on all affected hardware.

I can't find a definition of this but the register seems to work for
kirkwood as well (not surprising since it's probably a common IP block). Is
there any chance we can find a home for this that's available to boards
that don't use SPL?


> Cc: Marek Behún <marek.behun@nic.cz>
> Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>   arch/arm/mach-mvebu/spl.c                | 16 ++++++++++++++++
>   board/CZ.NIC/turris_omnia/turris_omnia.c |  9 ---------
>   2 files changed, 16 insertions(+), 9 deletions(-)

> diff --git a/arch/arm/mach-mvebu/spl.c b/arch/arm/mach-mvebu/spl.c
> index 50b24f5760b7..cbd900fee5d1 100644
> --- a/arch/arm/mach-mvebu/spl.c
> +++ b/arch/arm/mach-mvebu/spl.c
> @@ -8,10 +8,13 @@
>   #include <debug_uart.h>
>   #include <fdtdec.h>
>   #include <spl.h>
> +#include <linux/bitops.h>
>   #include <asm/io.h>
>   #include <asm/arch/cpu.h>
>   #include <asm/arch/soc.h>

> +#define MVTWSI_ARMADA_DEBUG_REG        0x8c
> +
>   static u32 get_boot_device(void)
>   {
>          u32 val;
> @@ -69,10 +72,23 @@ u32 spl_boot_device(void)
>          return get_boot_device();
>   }

> +static void disable_i2c_slave(void)
> +{
> +       u32 i2c_debug_reg;
> +
> +       /* Disable I2C debug mode blocking 0x64 I2C address */
> +       i2c_debug_reg = readl(MVEBU_TWSI_BASE + MVTWSI_ARMADA_DEBUG_REG);
> +       i2c_debug_reg &= ~BIT(18);
> +       writel(i2c_debug_reg, MVEBU_TWSI_BASE + MVTWSI_ARMADA_DEBUG_REG);
> +}
> +
>   void board_init_f(ulong dummy)
>   {
>          int ret;

> +       if (IS_ENABLED(CONFIG_ARMADA_38X))
> +               disable_i2c_slave();
> +
>          /*
>           * Pin muxing needs to be done before UART output, since
>           * on A38x the UART pins need some re-muxing for output
> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c
b/board/CZ.NIC/turris_omnia/turris_omnia.c
> index da663cf1bb0c..044c959d1b13 100644
> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
> @@ -50,8 +50,6 @@ DECLARE_GLOBAL_DATA_PTR;
>   #define OMNIA_ATSHA204_OTP_MAC0                3
>   #define OMNIA_ATSHA204_OTP_MAC1                4

> -#define MVTWSI_ARMADA_DEBUG_REG                0x8c
> -
>   /*
>    * Those values and defines are taken from the Marvell U-Boot version
>    * "u-boot-2013.01-2014_T3.0"
> @@ -297,8 +295,6 @@ static int set_regdomain(void)

>   int board_early_init_f(void)
>   {
> -       u32 i2c_debug_reg;
> -
>          /* Configure MPP */
>          writel(0x11111111, MVEBU_MPP_BASE + 0x00);
>          writel(0x11111111, MVEBU_MPP_BASE + 0x04);
> @@ -321,11 +317,6 @@ 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 */
> -       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);
> -
>          return 0;
>   }

> --
> 2.17.0

> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Stefan Roese May 28, 2018, 8:20 a.m. UTC | #2
On 28.05.2018 10:11, Chris Packham wrote:
> On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
> 
>> The hidden i2c slave that interferes the i2c bus is not board specific.
>> All Armada 38x SoCs are affected. Move the code disabling this slave to
>> generic code to make it work on all affected hardware.
> 
> I can't find a definition of this but the register seems to work for
> kirkwood as well (not surprising since it's probably a common IP block). Is
> there any chance we can find a home for this that's available to boards
> that don't use SPL?

Yes, please. Can't we move this into the I2C driver instead (mvtwsi.c)?
No need to use MVEBU_TWSI_BASE then.

Thanks,
Stefan
Baruch Siach May 28, 2018, 8:22 a.m. UTC | #3
Hi Chris,

On Mon, May 28, 2018 at 08:11:39PM +1200, Chris Packham wrote:
> On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
> > The hidden i2c slave that interferes the i2c bus is not board specific.
> > All Armada 38x SoCs are affected. Move the code disabling this slave to
> > generic code to make it work on all affected hardware.
> 
> I can't find a definition of this but the register seems to work for
> kirkwood as well (not surprising since it's probably a common IP block). Is
> there any chance we can find a home for this that's available to boards
> that don't use SPL?

This workaround is Armada 38x specific. Are you aware of any 38x SoC based 
system that does not use SPL?

As far as I can see 38x support requires SPL. CONFIG_ARMADA_38X selects 
CONFIG_ARMADA_32BIT that in turn selects CONFIG_SPL_DM. CONFIG_SPL_DM depends 
on CONFIG_SPL.

baruch

> > Cc: Marek Behún <marek.behun@nic.cz>
> > Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---
> >   arch/arm/mach-mvebu/spl.c                | 16 ++++++++++++++++
> >   board/CZ.NIC/turris_omnia/turris_omnia.c |  9 ---------
> >   2 files changed, 16 insertions(+), 9 deletions(-)
> 
> > diff --git a/arch/arm/mach-mvebu/spl.c b/arch/arm/mach-mvebu/spl.c
> > index 50b24f5760b7..cbd900fee5d1 100644
> > --- a/arch/arm/mach-mvebu/spl.c
> > +++ b/arch/arm/mach-mvebu/spl.c
> > @@ -8,10 +8,13 @@
> >   #include <debug_uart.h>
> >   #include <fdtdec.h>
> >   #include <spl.h>
> > +#include <linux/bitops.h>
> >   #include <asm/io.h>
> >   #include <asm/arch/cpu.h>
> >   #include <asm/arch/soc.h>
> 
> > +#define MVTWSI_ARMADA_DEBUG_REG        0x8c
> > +
> >   static u32 get_boot_device(void)
> >   {
> >          u32 val;
> > @@ -69,10 +72,23 @@ u32 spl_boot_device(void)
> >          return get_boot_device();
> >   }
> 
> > +static void disable_i2c_slave(void)
> > +{
> > +       u32 i2c_debug_reg;
> > +
> > +       /* Disable I2C debug mode blocking 0x64 I2C address */
> > +       i2c_debug_reg = readl(MVEBU_TWSI_BASE + MVTWSI_ARMADA_DEBUG_REG);
> > +       i2c_debug_reg &= ~BIT(18);
> > +       writel(i2c_debug_reg, MVEBU_TWSI_BASE + MVTWSI_ARMADA_DEBUG_REG);
> > +}
> > +
> >   void board_init_f(ulong dummy)
> >   {
> >          int ret;
> 
> > +       if (IS_ENABLED(CONFIG_ARMADA_38X))
> > +               disable_i2c_slave();
> > +
> >          /*
> >           * Pin muxing needs to be done before UART output, since
> >           * on A38x the UART pins need some re-muxing for output
> > diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c
> b/board/CZ.NIC/turris_omnia/turris_omnia.c
> > index da663cf1bb0c..044c959d1b13 100644
> > --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
> > +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
> > @@ -50,8 +50,6 @@ DECLARE_GLOBAL_DATA_PTR;
> >   #define OMNIA_ATSHA204_OTP_MAC0                3
> >   #define OMNIA_ATSHA204_OTP_MAC1                4
> 
> > -#define MVTWSI_ARMADA_DEBUG_REG                0x8c
> > -
> >   /*
> >    * Those values and defines are taken from the Marvell U-Boot version
> >    * "u-boot-2013.01-2014_T3.0"
> > @@ -297,8 +295,6 @@ static int set_regdomain(void)
> 
> >   int board_early_init_f(void)
> >   {
> > -       u32 i2c_debug_reg;
> > -
> >          /* Configure MPP */
> >          writel(0x11111111, MVEBU_MPP_BASE + 0x00);
> >          writel(0x11111111, MVEBU_MPP_BASE + 0x04);
> > @@ -321,11 +317,6 @@ 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 */
> > -       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);
> > -
> >          return 0;
> >   }
Chris Packham May 28, 2018, 8:55 a.m. UTC | #4
Hi Baruch,

On Mon, May 28, 2018 at 8:22 PM Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Chris,

> On Mon, May 28, 2018 at 08:11:39PM +1200, Chris Packham wrote:
> > On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
> > > The hidden i2c slave that interferes the i2c bus is not board
specific.
> > > All Armada 38x SoCs are affected. Move the code disabling this slave
to
> > > generic code to make it work on all affected hardware.
> >
> > I can't find a definition of this but the register seems to work for
> > kirkwood as well (not surprising since it's probably a common IP
block). Is
> > there any chance we can find a home for this that's available to boards
> > that don't use SPL?

> This workaround is Armada 38x specific. Are you aware of any 38x SoC based
> system that does not use SPL?

> As far as I can see 38x support requires SPL. CONFIG_ARMADA_38X selects
> CONFIG_ARMADA_32BIT that in turn selects CONFIG_SPL_DM. CONFIG_SPL_DM
depends
> on CONFIG_SPL.

The original workaround was implemented for the Turris Omnia board, as
you've highlighted this is debug register is documented for Armada-385. On
a hunch I tried clearing bit 18 of register 0xf101108c on a Kirkwood based
board and found that it also works to suppress the 0x64 self address. It's
highly likely that the i2c block is common to many Marvell SoCs and it just
happens to be documented on A-385.

If we could get this into the mvtwsi.c driver we could allow other SoCs to
disable the self address. Because it's only documented on A-385 we probably
don't want to do this unconditionally (unless someone on Marvell's SoC team
can confirm this register is valid) but I can confirm it works for Kirkwood.


> baruch

> > > Cc: Marek Behún <marek.behun@nic.cz>
> > > Cc: Rabeeh Khoury <rabeeh@solid-run.com>
> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > ---
> > >   arch/arm/mach-mvebu/spl.c                | 16 ++++++++++++++++
> > >   board/CZ.NIC/turris_omnia/turris_omnia.c |  9 ---------
> > >   2 files changed, 16 insertions(+), 9 deletions(-)
> >
> > > diff --git a/arch/arm/mach-mvebu/spl.c b/arch/arm/mach-mvebu/spl.c
> > > index 50b24f5760b7..cbd900fee5d1 100644
> > > --- a/arch/arm/mach-mvebu/spl.c
> > > +++ b/arch/arm/mach-mvebu/spl.c
> > > @@ -8,10 +8,13 @@
> > >   #include <debug_uart.h>
> > >   #include <fdtdec.h>
> > >   #include <spl.h>
> > > +#include <linux/bitops.h>
> > >   #include <asm/io.h>
> > >   #include <asm/arch/cpu.h>
> > >   #include <asm/arch/soc.h>
> >
> > > +#define MVTWSI_ARMADA_DEBUG_REG        0x8c
> > > +
> > >   static u32 get_boot_device(void)
> > >   {
> > >          u32 val;
> > > @@ -69,10 +72,23 @@ u32 spl_boot_device(void)
> > >          return get_boot_device();
> > >   }
> >
> > > +static void disable_i2c_slave(void)
> > > +{
> > > +       u32 i2c_debug_reg;
> > > +
> > > +       /* Disable I2C debug mode blocking 0x64 I2C address */
> > > +       i2c_debug_reg = readl(MVEBU_TWSI_BASE +
MVTWSI_ARMADA_DEBUG_REG);
> > > +       i2c_debug_reg &= ~BIT(18);
> > > +       writel(i2c_debug_reg, MVEBU_TWSI_BASE +
MVTWSI_ARMADA_DEBUG_REG);
> > > +}
> > > +
> > >   void board_init_f(ulong dummy)
> > >   {
> > >          int ret;
> >
> > > +       if (IS_ENABLED(CONFIG_ARMADA_38X))
> > > +               disable_i2c_slave();
> > > +
> > >          /*
> > >           * Pin muxing needs to be done before UART output, since
> > >           * on A38x the UART pins need some re-muxing for output
> > > diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c
> > b/board/CZ.NIC/turris_omnia/turris_omnia.c
> > > index da663cf1bb0c..044c959d1b13 100644
> > > --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
> > > +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
> > > @@ -50,8 +50,6 @@ DECLARE_GLOBAL_DATA_PTR;
> > >   #define OMNIA_ATSHA204_OTP_MAC0                3
> > >   #define OMNIA_ATSHA204_OTP_MAC1                4
> >
> > > -#define MVTWSI_ARMADA_DEBUG_REG                0x8c
> > > -
> > >   /*
> > >    * Those values and defines are taken from the Marvell U-Boot
version
> > >    * "u-boot-2013.01-2014_T3.0"
> > > @@ -297,8 +295,6 @@ static int set_regdomain(void)
> >
> > >   int board_early_init_f(void)
> > >   {
> > > -       u32 i2c_debug_reg;
> > > -
> > >          /* Configure MPP */
> > >          writel(0x11111111, MVEBU_MPP_BASE + 0x00);
> > >          writel(0x11111111, MVEBU_MPP_BASE + 0x04);
> > > @@ -321,11 +317,6 @@ 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 */
> > > -       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);
> > > -
> > >          return 0;
> > >   }

> --
>       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 -
Stefan Roese May 28, 2018, 9:04 a.m. UTC | #5
(Added Jagan and Maxime to Cc)

On 28.05.2018 10:55, Chris Packham wrote:
> Hi Baruch,
> 
> On Mon, May 28, 2018 at 8:22 PM Baruch Siach <baruch@tkos.co.il> wrote:
> 
>> Hi Chris,
> 
>> On Mon, May 28, 2018 at 08:11:39PM +1200, Chris Packham wrote:
>>> On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
>>>> The hidden i2c slave that interferes the i2c bus is not board
> specific.
>>>> All Armada 38x SoCs are affected. Move the code disabling this slave
> to
>>>> generic code to make it work on all affected hardware.
>>>
>>> I can't find a definition of this but the register seems to work for
>>> kirkwood as well (not surprising since it's probably a common IP
> block). Is
>>> there any chance we can find a home for this that's available to boards
>>> that don't use SPL?
> 
>> This workaround is Armada 38x specific. Are you aware of any 38x SoC based
>> system that does not use SPL?
> 
>> As far as I can see 38x support requires SPL. CONFIG_ARMADA_38X selects
>> CONFIG_ARMADA_32BIT that in turn selects CONFIG_SPL_DM. CONFIG_SPL_DM
> depends
>> on CONFIG_SPL.
> 
> The original workaround was implemented for the Turris Omnia board, as
> you've highlighted this is debug register is documented for Armada-385. On
> a hunch I tried clearing bit 18 of register 0xf101108c on a Kirkwood based
> board and found that it also works to suppress the 0x64 self address. It's
> highly likely that the i2c block is common to many Marvell SoCs and it just
> happens to be documented on A-385.

Thanks for testing this Chris. This I2C IP core is also used by many
Allwinner SoCs:

	{ .compatible = "allwinner,sun6i-a31-i2c", },


so it might be possible, that this "feature" is also available here.
Perhaps somebody from the "sunxi front" can confirm this?

> If we could get this into the mvtwsi.c driver we could allow other SoCs to
> disable the self address. Because it's only documented on A-385 we probably
> don't want to do this unconditionally (unless someone on Marvell's SoC team
> can confirm this register is valid) but I can confirm it works for Kirkwood.

Yes, it needs conditionals until its confirmed for all platforms using
this driver.

Thanks,
Stefan
Baruch Siach May 28, 2018, 9:56 a.m. UTC | #6
Hi Chris, Stefan,

On Mon, May 28, 2018 at 10:20:01AM +0200, Stefan Roese wrote:
> On 28.05.2018 10:11, Chris Packham wrote:
> > On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
> > 
> > > The hidden i2c slave that interferes the i2c bus is not board specific.
> > > All Armada 38x SoCs are affected. Move the code disabling this slave to
> > > generic code to make it work on all affected hardware.
> > 
> > I can't find a definition of this but the register seems to work for
> > kirkwood as well (not surprising since it's probably a common IP block). Is
> > there any chance we can find a home for this that's available to boards
> > that don't use SPL?
> 
> Yes, please. Can't we move this into the I2C driver instead (mvtwsi.c)?
> No need to use MVEBU_TWSI_BASE then.

The trouble is that the Turris Omnia board needs the i2c in SPL to read the 
RAM configuration from EEPROM. But I could not get the __twsi_i2c_init() 
routine (runs in both DM and non-DM) to be called from SPL.

I'm testing on (currently non-DM) 388 Clearfog Base.

In my testing here the hidden i2c slave interferes bus communication that runs 
at any speed other than 100KHz. If that is not a problem for Turris Omnia I 
can move the slave disable code to __twsi_i2c_init().

baruch
Stefan Roese May 28, 2018, 10:05 a.m. UTC | #7
Hi Baruch,

On 28.05.2018 11:56, Baruch Siach wrote:
> Hi Chris, Stefan,
> 
> On Mon, May 28, 2018 at 10:20:01AM +0200, Stefan Roese wrote:
>> On 28.05.2018 10:11, Chris Packham wrote:
>>> On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
>>>
>>>> The hidden i2c slave that interferes the i2c bus is not board specific.
>>>> All Armada 38x SoCs are affected. Move the code disabling this slave to
>>>> generic code to make it work on all affected hardware.
>>>
>>> I can't find a definition of this but the register seems to work for
>>> kirkwood as well (not surprising since it's probably a common IP block). Is
>>> there any chance we can find a home for this that's available to boards
>>> that don't use SPL?
>>
>> Yes, please. Can't we move this into the I2C driver instead (mvtwsi.c)?
>> No need to use MVEBU_TWSI_BASE then.
> 
> The trouble is that the Turris Omnia board needs the i2c in SPL to read the
> RAM configuration from EEPROM. But I could not get the __twsi_i2c_init()
> routine (runs in both DM and non-DM) to be called from SPL.

Are you sure? How can you use the I2C functions of this driver, if the
init / probe function is not called?

> I'm testing on (currently non-DM) 388 Clearfog Base.
> 
> In my testing here the hidden i2c slave interferes bus communication that runs
> at any speed other than 100KHz. If that is not a problem for Turris Omnia I
> can move the slave disable code to __twsi_i2c_init().
> 
> baruch
> 

Thanks,
Stefan
Baruch Siach May 28, 2018, 10:08 a.m. UTC | #8
Hi Stefan,

On Mon, May 28, 2018 at 12:05:43PM +0200, Stefan Roese wrote:
> On 28.05.2018 11:56, Baruch Siach wrote:
> > On Mon, May 28, 2018 at 10:20:01AM +0200, Stefan Roese wrote:
> > > On 28.05.2018 10:11, Chris Packham wrote:
> > > > On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
> > > > 
> > > > > The hidden i2c slave that interferes the i2c bus is not board specific.
> > > > > All Armada 38x SoCs are affected. Move the code disabling this slave to
> > > > > generic code to make it work on all affected hardware.
> > > > 
> > > > I can't find a definition of this but the register seems to work for
> > > > kirkwood as well (not surprising since it's probably a common IP block). Is
> > > > there any chance we can find a home for this that's available to boards
> > > > that don't use SPL?
> > > 
> > > Yes, please. Can't we move this into the I2C driver instead (mvtwsi.c)?
> > > No need to use MVEBU_TWSI_BASE then.
> > 
> > The trouble is that the Turris Omnia board needs the i2c in SPL to read the
> > RAM configuration from EEPROM. But I could not get the __twsi_i2c_init()
> > routine (runs in both DM and non-DM) to be called from SPL.
> 
> Are you sure? How can you use the I2C functions of this driver, if the
> init / probe function is not called?

__twsi_i2c_init() is called from the main U-Boot image, but not from SPL as 
far as my testing shows. Clearfog doesn't use i2c from SPL, but the Turris 
board does.

baruch

> > I'm testing on (currently non-DM) 388 Clearfog Base.
> > 
> > In my testing here the hidden i2c slave interferes bus communication that runs
> > at any speed other than 100KHz. If that is not a problem for Turris Omnia I
> > can move the slave disable code to __twsi_i2c_init().
> > 
> > baruch
Stefan Roese May 28, 2018, 10:24 a.m. UTC | #9
On 28.05.2018 12:08, Baruch Siach wrote:
> Hi Stefan,
> 
> On Mon, May 28, 2018 at 12:05:43PM +0200, Stefan Roese wrote:
>> On 28.05.2018 11:56, Baruch Siach wrote:
>>> On Mon, May 28, 2018 at 10:20:01AM +0200, Stefan Roese wrote:
>>>> On 28.05.2018 10:11, Chris Packham wrote:
>>>>> On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
>>>>>
>>>>>> The hidden i2c slave that interferes the i2c bus is not board specific.
>>>>>> All Armada 38x SoCs are affected. Move the code disabling this slave to
>>>>>> generic code to make it work on all affected hardware.
>>>>>
>>>>> I can't find a definition of this but the register seems to work for
>>>>> kirkwood as well (not surprising since it's probably a common IP block). Is
>>>>> there any chance we can find a home for this that's available to boards
>>>>> that don't use SPL?
>>>>
>>>> Yes, please. Can't we move this into the I2C driver instead (mvtwsi.c)?
>>>> No need to use MVEBU_TWSI_BASE then.
>>>
>>> The trouble is that the Turris Omnia board needs the i2c in SPL to read the
>>> RAM configuration from EEPROM. But I could not get the __twsi_i2c_init()
>>> routine (runs in both DM and non-DM) to be called from SPL.
>>
>> Are you sure? How can you use the I2C functions of this driver, if the
>> init / probe function is not called?
> 
> __twsi_i2c_init() is called from the main U-Boot image, but not from SPL as
> far as my testing shows. Clearfog doesn't use i2c from SPL, but the Turris
> board does.

Can't you switch to DM_I2C then? This should make sure, that the probe
function is always called before the xfer functions are used.

Thanks,
Stefan
Baruch Siach May 28, 2018, 10:29 a.m. UTC | #10
Hi Stefan,

On Mon, May 28, 2018 at 12:24:37PM +0200, Stefan Roese wrote:
> On 28.05.2018 12:08, Baruch Siach wrote:
> > On Mon, May 28, 2018 at 12:05:43PM +0200, Stefan Roese wrote:
> > > On 28.05.2018 11:56, Baruch Siach wrote:
> > > > On Mon, May 28, 2018 at 10:20:01AM +0200, Stefan Roese wrote:
> > > > > On 28.05.2018 10:11, Chris Packham wrote:
> > > > > > On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
> > > > > > 
> > > > > > > The hidden i2c slave that interferes the i2c bus is not board specific.
> > > > > > > All Armada 38x SoCs are affected. Move the code disabling this slave to
> > > > > > > generic code to make it work on all affected hardware.
> > > > > > 
> > > > > > I can't find a definition of this but the register seems to work for
> > > > > > kirkwood as well (not surprising since it's probably a common IP block). Is
> > > > > > there any chance we can find a home for this that's available to boards
> > > > > > that don't use SPL?
> > > > > 
> > > > > Yes, please. Can't we move this into the I2C driver instead (mvtwsi.c)?
> > > > > No need to use MVEBU_TWSI_BASE then.
> > > > 
> > > > The trouble is that the Turris Omnia board needs the i2c in SPL to read the
> > > > RAM configuration from EEPROM. But I could not get the __twsi_i2c_init()
> > > > routine (runs in both DM and non-DM) to be called from SPL.
> > > 
> > > Are you sure? How can you use the I2C functions of this driver, if the
> > > init / probe function is not called?
> > 
> > __twsi_i2c_init() is called from the main U-Boot image, but not from SPL as
> > far as my testing shows. Clearfog doesn't use i2c from SPL, but the Turris
> > board does.
> 
> Can't you switch to DM_I2C then? This should make sure, that the probe
> function is always called before the xfer functions are used.

I plan to do so for Clearfog. But the turris_omnia_defconfig does not enable 
DM_I2C. So that would regress Turris.

baruch
Stefan Roese May 28, 2018, 10:33 a.m. UTC | #11
On 28.05.2018 12:29, Baruch Siach wrote:
> Hi Stefan,
> 
> On Mon, May 28, 2018 at 12:24:37PM +0200, Stefan Roese wrote:
>> On 28.05.2018 12:08, Baruch Siach wrote:
>>> On Mon, May 28, 2018 at 12:05:43PM +0200, Stefan Roese wrote:
>>>> On 28.05.2018 11:56, Baruch Siach wrote:
>>>>> On Mon, May 28, 2018 at 10:20:01AM +0200, Stefan Roese wrote:
>>>>>> On 28.05.2018 10:11, Chris Packham wrote:
>>>>>>> On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
>>>>>>>
>>>>>>>> The hidden i2c slave that interferes the i2c bus is not board specific.
>>>>>>>> All Armada 38x SoCs are affected. Move the code disabling this slave to
>>>>>>>> generic code to make it work on all affected hardware.
>>>>>>>
>>>>>>> I can't find a definition of this but the register seems to work for
>>>>>>> kirkwood as well (not surprising since it's probably a common IP block). Is
>>>>>>> there any chance we can find a home for this that's available to boards
>>>>>>> that don't use SPL?
>>>>>>
>>>>>> Yes, please. Can't we move this into the I2C driver instead (mvtwsi.c)?
>>>>>> No need to use MVEBU_TWSI_BASE then.
>>>>>
>>>>> The trouble is that the Turris Omnia board needs the i2c in SPL to read the
>>>>> RAM configuration from EEPROM. But I could not get the __twsi_i2c_init()
>>>>> routine (runs in both DM and non-DM) to be called from SPL.
>>>>
>>>> Are you sure? How can you use the I2C functions of this driver, if the
>>>> init / probe function is not called?
>>>
>>> __twsi_i2c_init() is called from the main U-Boot image, but not from SPL as
>>> far as my testing shows. Clearfog doesn't use i2c from SPL, but the Turris
>>> board does.
>>
>> Can't you switch to DM_I2C then? This should make sure, that the probe
>> function is always called before the xfer functions are used.
> 
> I plan to do so for Clearfog. But the turris_omnia_defconfig does not enable
> DM_I2C. So that would regress Turris.

Regress? Definitely not. We plan to remove all non-DM parts at some
time. So a move from the ancient driver parts to DM is definitely
preferred (especially if the platform and the driver already support
DM) - better sooner than later.

Thanks,
Stefan
Baruch Siach May 28, 2018, 10:42 a.m. UTC | #12
Hi Stafan,

On Mon, May 28, 2018 at 12:33:41PM +0200, Stefan Roese wrote:
> On 28.05.2018 12:29, Baruch Siach wrote:
> > On Mon, May 28, 2018 at 12:24:37PM +0200, Stefan Roese wrote:
> > > On 28.05.2018 12:08, Baruch Siach wrote:
> > > > On Mon, May 28, 2018 at 12:05:43PM +0200, Stefan Roese wrote:
> > > > > On 28.05.2018 11:56, Baruch Siach wrote:
> > > > > > On Mon, May 28, 2018 at 10:20:01AM +0200, Stefan Roese wrote:
> > > > > > > On 28.05.2018 10:11, Chris Packham wrote:
> > > > > > > > On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
> > > > > > > > 
> > > > > > > > > The hidden i2c slave that interferes the i2c bus is not board specific.
> > > > > > > > > All Armada 38x SoCs are affected. Move the code disabling this slave to
> > > > > > > > > generic code to make it work on all affected hardware.
> > > > > > > > 
> > > > > > > > I can't find a definition of this but the register seems to work for
> > > > > > > > kirkwood as well (not surprising since it's probably a common IP block). Is
> > > > > > > > there any chance we can find a home for this that's available to boards
> > > > > > > > that don't use SPL?
> > > > > > > 
> > > > > > > Yes, please. Can't we move this into the I2C driver instead (mvtwsi.c)?
> > > > > > > No need to use MVEBU_TWSI_BASE then.
> > > > > > 
> > > > > > The trouble is that the Turris Omnia board needs the i2c in SPL to read the
> > > > > > RAM configuration from EEPROM. But I could not get the __twsi_i2c_init()
> > > > > > routine (runs in both DM and non-DM) to be called from SPL.
> > > > > 
> > > > > Are you sure? How can you use the I2C functions of this driver, if the
> > > > > init / probe function is not called?
> > > > 
> > > > __twsi_i2c_init() is called from the main U-Boot image, but not from SPL as
> > > > far as my testing shows. Clearfog doesn't use i2c from SPL, but the Turris
> > > > board does.
> > > 
> > > Can't you switch to DM_I2C then? This should make sure, that the probe
> > > function is always called before the xfer functions are used.
> > 
> > I plan to do so for Clearfog. But the turris_omnia_defconfig does not enable
> > DM_I2C. So that would regress Turris.
> 
> Regress? Definitely not. We plan to remove all non-DM parts at some
> time. So a move from the ancient driver parts to DM is definitely
> preferred (especially if the platform and the driver already support
> DM) - better sooner than later.

What I meant to say is that moving the i2c slave disable code from Turris code 
to the mvtwsi driver would regress Turris unless Turris migrates to DM_I2C 
first. I have no access to the Turris hardware, so I can't test the DM_I2C 
migration on that platform.

If you prefer I can leave the Turris code alone. Instead, I'll add the i2c 
slave disable to mvtwsi. Once Turris migrates to DM_I2C we can remove the 
redundant code from its platform code.

What do you think?

baruch
Stefan Roese May 28, 2018, 12:34 p.m. UTC | #13
On 28.05.2018 12:42, Baruch Siach wrote:

<snip>

>>>>> __twsi_i2c_init() is called from the main U-Boot image, but not from SPL as
>>>>> far as my testing shows. Clearfog doesn't use i2c from SPL, but the Turris
>>>>> board does.
>>>>
>>>> Can't you switch to DM_I2C then? This should make sure, that the probe
>>>> function is always called before the xfer functions are used.
>>>
>>> I plan to do so for Clearfog. But the turris_omnia_defconfig does not enable
>>> DM_I2C. So that would regress Turris.
>>
>> Regress? Definitely not. We plan to remove all non-DM parts at some
>> time. So a move from the ancient driver parts to DM is definitely
>> preferred (especially if the platform and the driver already support
>> DM) - better sooner than later.
> 
> What I meant to say is that moving the i2c slave disable code from Turris code
> to the mvtwsi driver would regress Turris unless Turris migrates to DM_I2C
> first. I have no access to the Turris hardware, so I can't test the DM_I2C
> migration on that platform.
> 
> If you prefer I can leave the Turris code alone. Instead, I'll add the i2c
> slave disable to mvtwsi. Once Turris migrates to DM_I2C we can remove the
> redundant code from its platform code.
> 
> What do you think?

I would prefer to do this conversion right now and pollute the driver
with stuff that is not needed for DM users.

Marek Behún is the maintainer of the Turris Omnia board and should be
able to test such a change quite easily.

Perhaps its best for now to just add a comment to this code in
board/CZ.NIC/turris_omnia/turris_omnia.c, that this code is not needed
any more, once the board moves over to DM_I2C. And create the same
functionality in the I2C driver for DM_I2C users. This is what you
will be using / testing then and Marek can (read should) move to DM_I2C
soon and remove this code from the board C file then.

What do you think?

Thanks,
Stefan
Marek Behún May 28, 2018, 12:43 p.m. UTC | #14
On Mon, 28 May 2018 14:34:07 +0200
Stefan Roese <sr@denx.de> wrote:

> On 28.05.2018 12:42, Baruch Siach wrote:
> 
> <snip>
> 
> >>>>> __twsi_i2c_init() is called from the main U-Boot image, but not
> >>>>> from SPL as far as my testing shows. Clearfog doesn't use i2c
> >>>>> from SPL, but the Turris board does.  
> >>>>
> >>>> Can't you switch to DM_I2C then? This should make sure, that the
> >>>> probe function is always called before the xfer functions are
> >>>> used.  
> >>>
> >>> I plan to do so for Clearfog. But the turris_omnia_defconfig does
> >>> not enable DM_I2C. So that would regress Turris.  
> >>
> >> Regress? Definitely not. We plan to remove all non-DM parts at some
> >> time. So a move from the ancient driver parts to DM is definitely
> >> preferred (especially if the platform and the driver already
> >> support DM) - better sooner than later.  
> > 
> > What I meant to say is that moving the i2c slave disable code from
> > Turris code to the mvtwsi driver would regress Turris unless Turris
> > migrates to DM_I2C first. I have no access to the Turris hardware,
> > so I can't test the DM_I2C migration on that platform.
> > 
> > If you prefer I can leave the Turris code alone. Instead, I'll add
> > the i2c slave disable to mvtwsi. Once Turris migrates to DM_I2C we
> > can remove the redundant code from its platform code.
> > 
> > What do you think?  
> 
> I would prefer to do this conversion right now and pollute the driver
> with stuff that is not needed for DM users.
> 
> Marek Behún is the maintainer of the Turris Omnia board and should be
> able to test such a change quite easily.
> 
> Perhaps its best for now to just add a comment to this code in
> board/CZ.NIC/turris_omnia/turris_omnia.c, that this code is not needed
> any more, once the board moves over to DM_I2C. And create the same
> functionality in the I2C driver for DM_I2C users. This is what you
> will be using / testing then and Marek can (read should) move to
> DM_I2C soon and remove this code from the board C file then.
> 
> What do you think?
> 
> Thanks,
> Stefan

Turris Omnia is currently broken in mainline anyway. As far as I am
concerned, do what Stefan suggests, add a comment to turris_omnia.c. As
soon as I will be more free from Turris Mox work, I am going to work on
Turris Omnia...

Marek
Maxime Ripard May 29, 2018, 3:39 p.m. UTC | #15
Hi!

On Mon, May 28, 2018 at 11:04:38AM +0200, Stefan Roese wrote:
> (Added Jagan and Maxime to Cc)
> 
> On 28.05.2018 10:55, Chris Packham wrote:
> > Hi Baruch,
> > 
> > On Mon, May 28, 2018 at 8:22 PM Baruch Siach <baruch@tkos.co.il> wrote:
> > 
> > > Hi Chris,
> > 
> > > On Mon, May 28, 2018 at 08:11:39PM +1200, Chris Packham wrote:
> > > > On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
> > > > > The hidden i2c slave that interferes the i2c bus is not board
> > specific.
> > > > > All Armada 38x SoCs are affected. Move the code disabling this slave
> > to
> > > > > generic code to make it work on all affected hardware.
> > > > 
> > > > I can't find a definition of this but the register seems to work for
> > > > kirkwood as well (not surprising since it's probably a common IP
> > block). Is
> > > > there any chance we can find a home for this that's available to boards
> > > > that don't use SPL?
> > 
> > > This workaround is Armada 38x specific. Are you aware of any 38x SoC based
> > > system that does not use SPL?
> > 
> > > As far as I can see 38x support requires SPL. CONFIG_ARMADA_38X selects
> > > CONFIG_ARMADA_32BIT that in turn selects CONFIG_SPL_DM. CONFIG_SPL_DM
> > depends
> > > on CONFIG_SPL.
> > 
> > The original workaround was implemented for the Turris Omnia board, as
> > you've highlighted this is debug register is documented for Armada-385. On
> > a hunch I tried clearing bit 18 of register 0xf101108c on a Kirkwood based
> > board and found that it also works to suppress the 0x64 self address. It's
> > highly likely that the i2c block is common to many Marvell SoCs and it just
> > happens to be documented on A-385.
> 
> Thanks for testing this Chris. This I2C IP core is also used by many
> Allwinner SoCs:
> 
> 	{ .compatible = "allwinner,sun6i-a31-i2c", },
> 
> 
> so it might be possible, that this "feature" is also available here.
> Perhaps somebody from the "sunxi front" can confirm this?

I went back to the thread a bit, but I couldn't really understand what
the issue was exactly. Do you have a spurious slave with the
controller at the address 0x64? When would it happen? Anytime you for
example probe the i2c bus?

Maxime
Stefan Roese May 29, 2018, 3:43 p.m. UTC | #16
Hi Maxime,

On 29.05.2018 17:39, Maxime Ripard wrote:
> On Mon, May 28, 2018 at 11:04:38AM +0200, Stefan Roese wrote:
>> (Added Jagan and Maxime to Cc)
>>
>> On 28.05.2018 10:55, Chris Packham wrote:
>>> Hi Baruch,
>>>
>>> On Mon, May 28, 2018 at 8:22 PM Baruch Siach <baruch@tkos.co.il> wrote:
>>>
>>>> Hi Chris,
>>>
>>>> On Mon, May 28, 2018 at 08:11:39PM +1200, Chris Packham wrote:
>>>>> On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
>>>>>> The hidden i2c slave that interferes the i2c bus is not board
>>> specific.
>>>>>> All Armada 38x SoCs are affected. Move the code disabling this slave
>>> to
>>>>>> generic code to make it work on all affected hardware.
>>>>>
>>>>> I can't find a definition of this but the register seems to work for
>>>>> kirkwood as well (not surprising since it's probably a common IP
>>> block). Is
>>>>> there any chance we can find a home for this that's available to boards
>>>>> that don't use SPL?
>>>
>>>> This workaround is Armada 38x specific. Are you aware of any 38x SoC based
>>>> system that does not use SPL?
>>>
>>>> As far as I can see 38x support requires SPL. CONFIG_ARMADA_38X selects
>>>> CONFIG_ARMADA_32BIT that in turn selects CONFIG_SPL_DM. CONFIG_SPL_DM
>>> depends
>>>> on CONFIG_SPL.
>>>
>>> The original workaround was implemented for the Turris Omnia board, as
>>> you've highlighted this is debug register is documented for Armada-385. On
>>> a hunch I tried clearing bit 18 of register 0xf101108c on a Kirkwood based
>>> board and found that it also works to suppress the 0x64 self address. It's
>>> highly likely that the i2c block is common to many Marvell SoCs and it just
>>> happens to be documented on A-385.
>>
>> Thanks for testing this Chris. This I2C IP core is also used by many
>> Allwinner SoCs:
>>
>> 	{ .compatible = "allwinner,sun6i-a31-i2c", },
>>
>>
>> so it might be possible, that this "feature" is also available here.
>> Perhaps somebody from the "sunxi front" can confirm this?
> 
> I went back to the thread a bit, but I couldn't really understand what
> the issue was exactly. Do you have a spurious slave with the
> controller at the address 0x64? When would it happen? Anytime you for
> example probe the i2c bus?

On the Marvell platforms (Kirkwood, Armada 38x, possibly others), the
SoC has the address 0x64 reserved as its I2C slave address (for other
masters). So this address is not available for I2C devices attached
to the I2C bus. With this patch (via this debug register), this slave
address can be "free'ed" making it available for other I2C devices.

Thanks,
Stefan
Maxime Ripard May 29, 2018, 4:32 p.m. UTC | #17
On Tue, May 29, 2018 at 05:43:30PM +0200, Stefan Roese wrote:
> > On Mon, May 28, 2018 at 11:04:38AM +0200, Stefan Roese wrote:
> > > (Added Jagan and Maxime to Cc)
> > > 
> > > On 28.05.2018 10:55, Chris Packham wrote:
> > > > Hi Baruch,
> > > > 
> > > > On Mon, May 28, 2018 at 8:22 PM Baruch Siach <baruch@tkos.co.il> wrote:
> > > > 
> > > > > Hi Chris,
> > > > 
> > > > > On Mon, May 28, 2018 at 08:11:39PM +1200, Chris Packham wrote:
> > > > > > On Mon, May 28, 2018 at 3:27 AM Baruch Siach <baruch@tkos.co.il> wrote:
> > > > > > > The hidden i2c slave that interferes the i2c bus is not board
> > > > specific.
> > > > > > > All Armada 38x SoCs are affected. Move the code disabling this slave
> > > > to
> > > > > > > generic code to make it work on all affected hardware.
> > > > > > 
> > > > > > I can't find a definition of this but the register seems to work for
> > > > > > kirkwood as well (not surprising since it's probably a common IP
> > > > block). Is
> > > > > > there any chance we can find a home for this that's available to boards
> > > > > > that don't use SPL?
> > > > 
> > > > > This workaround is Armada 38x specific. Are you aware of any 38x SoC based
> > > > > system that does not use SPL?
> > > > 
> > > > > As far as I can see 38x support requires SPL. CONFIG_ARMADA_38X selects
> > > > > CONFIG_ARMADA_32BIT that in turn selects CONFIG_SPL_DM. CONFIG_SPL_DM
> > > > depends
> > > > > on CONFIG_SPL.
> > > > 
> > > > The original workaround was implemented for the Turris Omnia board, as
> > > > you've highlighted this is debug register is documented for Armada-385. On
> > > > a hunch I tried clearing bit 18 of register 0xf101108c on a Kirkwood based
> > > > board and found that it also works to suppress the 0x64 self address. It's
> > > > highly likely that the i2c block is common to many Marvell SoCs and it just
> > > > happens to be documented on A-385.
> > > 
> > > Thanks for testing this Chris. This I2C IP core is also used by many
> > > Allwinner SoCs:
> > > 
> > > 	{ .compatible = "allwinner,sun6i-a31-i2c", },
> > > 
> > > 
> > > so it might be possible, that this "feature" is also available here.
> > > Perhaps somebody from the "sunxi front" can confirm this?
> > 
> > I went back to the thread a bit, but I couldn't really understand what
> > the issue was exactly. Do you have a spurious slave with the
> > controller at the address 0x64? When would it happen? Anytime you for
> > example probe the i2c bus?
> 
> On the Marvell platforms (Kirkwood, Armada 38x, possibly others), the
> SoC has the address 0x64 reserved as its I2C slave address (for other
> masters). So this address is not available for I2C devices attached
> to the I2C bus. With this patch (via this debug register), this slave
> address can be "free'ed" making it available for other I2C devices.

Ok. And that fantom slave can be accessed from the controller itself
when it's the master, or it's only usable by other masters on the bus?

ie, an i2cdetect running on that bus from that controller would detect
the slave you were mentionning? If so, then we don't have it on the
Allwinner SoCs (or it's not enabled by default at least).

Maxime
Baruch Siach May 30, 2018, 4:25 a.m. UTC | #18
Hi Maxime,

On Tue, May 29, 2018 at 06:32:01PM +0200, Maxime Ripard wrote:
> On Tue, May 29, 2018 at 05:43:30PM +0200, Stefan Roese wrote:
> > > On Mon, May 28, 2018 at 11:04:38AM +0200, Stefan Roese wrote:
> > > I went back to the thread a bit, but I couldn't really understand what
> > > the issue was exactly. Do you have a spurious slave with the
> > > controller at the address 0x64? When would it happen? Anytime you for
> > > example probe the i2c bus?
> > 
> > On the Marvell platforms (Kirkwood, Armada 38x, possibly others), the
> > SoC has the address 0x64 reserved as its I2C slave address (for other
> > masters). So this address is not available for I2C devices attached
> > to the I2C bus. With this patch (via this debug register), this slave
> > address can be "free'ed" making it available for other I2C devices.
> 
> Ok. And that fantom slave can be accessed from the controller itself
> when it's the master, or it's only usable by other masters on the bus?
> 
> ie, an i2cdetect running on that bus from that controller would detect
> the slave you were mentionning? If so, then we don't have it on the
> Allwinner SoCs (or it's not enabled by default at least).

Thanks for testing.

On Armada 38x SoCs this phantom slave only shows up when the bus rate is 
100KHz. At higher rates this slave doesn't show in i2cdetect output, but it 
does interfere with long i2c transfers causing bus lockup.

baruch
Maxime Ripard May 31, 2018, 9:40 a.m. UTC | #19
On Wed, May 30, 2018 at 07:25:14AM +0300, Baruch Siach wrote:
> Hi Maxime,
> 
> On Tue, May 29, 2018 at 06:32:01PM +0200, Maxime Ripard wrote:
> > On Tue, May 29, 2018 at 05:43:30PM +0200, Stefan Roese wrote:
> > > > On Mon, May 28, 2018 at 11:04:38AM +0200, Stefan Roese wrote:
> > > > I went back to the thread a bit, but I couldn't really understand what
> > > > the issue was exactly. Do you have a spurious slave with the
> > > > controller at the address 0x64? When would it happen? Anytime you for
> > > > example probe the i2c bus?
> > > 
> > > On the Marvell platforms (Kirkwood, Armada 38x, possibly others), the
> > > SoC has the address 0x64 reserved as its I2C slave address (for other
> > > masters). So this address is not available for I2C devices attached
> > > to the I2C bus. With this patch (via this debug register), this slave
> > > address can be "free'ed" making it available for other I2C devices.
> > 
> > Ok. And that fantom slave can be accessed from the controller itself
> > when it's the master, or it's only usable by other masters on the bus?
> > 
> > ie, an i2cdetect running on that bus from that controller would detect
> > the slave you were mentionning? If so, then we don't have it on the
> > Allwinner SoCs (or it's not enabled by default at least).
> 
> Thanks for testing.
> 
> On Armada 38x SoCs this phantom slave only shows up when the bus rate is 
> 100KHz. At higher rates this slave doesn't show in i2cdetect output, but it 
> does interfere with long i2c transfers causing bus lockup.

I've never noticed anything similar on any Allwinner SoC, so I guess
you can leave out the sunxi case in your code :)

Maxime
diff mbox series

Patch

diff --git a/arch/arm/mach-mvebu/spl.c b/arch/arm/mach-mvebu/spl.c
index 50b24f5760b7..cbd900fee5d1 100644
--- a/arch/arm/mach-mvebu/spl.c
+++ b/arch/arm/mach-mvebu/spl.c
@@ -8,10 +8,13 @@ 
 #include <debug_uart.h>
 #include <fdtdec.h>
 #include <spl.h>
+#include <linux/bitops.h>
 #include <asm/io.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/soc.h>
 
+#define MVTWSI_ARMADA_DEBUG_REG	0x8c
+
 static u32 get_boot_device(void)
 {
 	u32 val;
@@ -69,10 +72,23 @@  u32 spl_boot_device(void)
 	return get_boot_device();
 }
 
+static void disable_i2c_slave(void)
+{
+	u32 i2c_debug_reg;
+
+	/* Disable I2C debug mode blocking 0x64 I2C address */
+	i2c_debug_reg = readl(MVEBU_TWSI_BASE + MVTWSI_ARMADA_DEBUG_REG);
+	i2c_debug_reg &= ~BIT(18);
+	writel(i2c_debug_reg, MVEBU_TWSI_BASE + MVTWSI_ARMADA_DEBUG_REG);
+}
+
 void board_init_f(ulong dummy)
 {
 	int ret;
 
+	if (IS_ENABLED(CONFIG_ARMADA_38X))
+		disable_i2c_slave();
+
 	/*
 	 * Pin muxing needs to be done before UART output, since
 	 * on A38x the UART pins need some re-muxing for output
diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
index da663cf1bb0c..044c959d1b13 100644
--- a/board/CZ.NIC/turris_omnia/turris_omnia.c
+++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
@@ -50,8 +50,6 @@  DECLARE_GLOBAL_DATA_PTR;
 #define OMNIA_ATSHA204_OTP_MAC0		3
 #define OMNIA_ATSHA204_OTP_MAC1		4
 
-#define MVTWSI_ARMADA_DEBUG_REG		0x8c
-
 /*
  * Those values and defines are taken from the Marvell U-Boot version
  * "u-boot-2013.01-2014_T3.0"
@@ -297,8 +295,6 @@  static int set_regdomain(void)
 
 int board_early_init_f(void)
 {
-	u32 i2c_debug_reg;
-
 	/* Configure MPP */
 	writel(0x11111111, MVEBU_MPP_BASE + 0x00);
 	writel(0x11111111, MVEBU_MPP_BASE + 0x04);
@@ -321,11 +317,6 @@  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 */
-	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);
-
 	return 0;
 }