[U-Boot] arm: mvebu: switch clearfog to use device-tree i2c and gpio

From: Jon Nettleton <jon@solid-run.com>

This switches the clearfog boards to use DM based gpio and i2c
drivers.  The io expanders are configured via their device-tree
entries.

Signed-off-by: Jon Nettleton <jon@solid-run.com>
[baruch: add DT i2c aliases]
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---

The context lines in this patch depend on the patch adding SATA support.

https://patchwork.ozlabs.org/patch/921400/
---
 arch/arm/dts/armada-388-clearfog.dts |  2 ++
 board/solidrun/clearfog/clearfog.c   | 22 ----------------------
 configs/clearfog_defconfig           |  6 ++++++
 include/configs/clearfog.h           |  7 -------
 4 files changed, 8 insertions(+), 29 deletions(-)

On Tue, May 29, 2018 at 4:11 AM Baruch Siach <baruch@tkos.co.il> wrote:

> From: Jon Nettleton <jon@solid-run.com>

> This switches the clearfog boards to use DM based gpio and i2c
> drivers.  The io expanders are configured via their device-tree
> entries.

> Signed-off-by: Jon Nettleton <jon@solid-run.com>
> [baruch: add DT i2c aliases]
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---

> The context lines in this patch depend on the patch adding SATA support.

> https://patchwork.ozlabs.org/patch/921400/
> ---
>   arch/arm/dts/armada-388-clearfog.dts |  2 ++
>   board/solidrun/clearfog/clearfog.c   | 22 ----------------------
>   configs/clearfog_defconfig           |  6 ++++++
>   include/configs/clearfog.h           |  7 -------
>   4 files changed, 8 insertions(+), 29 deletions(-)

> diff --git a/arch/arm/dts/armada-388-clearfog.dts
b/arch/arm/dts/armada-388-clearfog.dts
> index bc52bc0167d3..a0b566a5ae0e 100644
> --- a/arch/arm/dts/armada-388-clearfog.dts
> +++ b/arch/arm/dts/armada-388-clearfog.dts
> @@ -62,6 +62,8 @@
>                  ethernet2 = &eth1;
>                  ethernet3 = &eth2;
>                  spi1 = &spi1;
> +               i2c0 = &i2c0;
> +               i2c1 = &i2c1;
>          };

>          chosen {
> diff --git a/board/solidrun/clearfog/clearfog.c
b/board/solidrun/clearfog/clearfog.c
> index ede303d4ebf9..4e1386c8a223 100644
> --- a/board/solidrun/clearfog/clearfog.c
> +++ b/board/solidrun/clearfog/clearfog.c
> @@ -32,22 +32,6 @@ DECLARE_GLOBAL_DATA_PTR;
>   #define BOARD_GPP_POL_LOW      0x0
>   #define BOARD_GPP_POL_MID      0x0

> -/* IO expander on Marvell GP board includes e.g. fan enabling */
> -struct marvell_io_exp {
> -       u8 chip;
> -       u8 addr;
> -       u8 val;
> -};
> -
> -static struct marvell_io_exp io_exp[] = {
> -       { 0x20, 2, 0x40 },      /* Deassert both mini pcie reset signals
*/
> -       { 0x20, 6, 0xf9 },
> -       { 0x20, 2, 0x46 },      /* rst signals and ena USB3 current
limiter */
> -       { 0x20, 6, 0xb9 },
> -       { 0x20, 3, 0x00 },      /* Set SFP_TX_DIS to zero */
> -       { 0x20, 7, 0xbf },      /* Drive SFP_TX_DIS to zero */
> -};
> -

Will this actually work? As far as I can see u-boot lacks a gpio-hog
implementation so despite the fact that these are all enumerated in the dts
they won't have the same effect.

I'd be glad to be proven wrong because I'd like to use a gpio-hog for some
other boards.

>   static struct serdes_map board_serdes_map[] = {
>          {SATA0, SERDES_SPEED_3_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>          {SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
> @@ -126,8 +110,6 @@ int board_early_init_f(void)

>   int board_init(void)
>   {
> -       int i;
> -
>          /* Address of boot parameters */
>          gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100;

> @@ -142,10 +124,6 @@ int board_init(void)
>          setbits_le32(MVEBU_GPIO0_BASE + 0x0, BIT(19));
>          mdelay(10);

> -       /* Init I2C IO expanders */
> -       for (i = 0; i < ARRAY_SIZE(io_exp); i++)
> -               i2c_write(io_exp[i].chip, io_exp[i].addr, 1,
&io_exp[i].val, 1);
> -
>          return 0;
>   }

> diff --git a/configs/clearfog_defconfig b/configs/clearfog_defconfig
> index 3972c31ef978..2baf33701bca 100644
> --- a/configs/clearfog_defconfig
> +++ b/configs/clearfog_defconfig
> @@ -1,6 +1,7 @@
>   CONFIG_ARM=y
>   CONFIG_ARCH_MVEBU=y
>   CONFIG_SYS_TEXT_BASE=0x00800000
> +CONFIG_SPL_GPIO_SUPPORT=y
>   CONFIG_SPL_LIBCOMMON_SUPPORT=y
>   CONFIG_SPL_LIBGENERIC_SUPPORT=y
>   CONFIG_SYS_MALLOC_F_LEN=0x2000
> @@ -19,6 +20,7 @@ CONFIG_DISPLAY_BOARDINFO_LATE=y
>   CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x141
>   CONFIG_SPL_I2C_SUPPORT=y
>   # CONFIG_CMD_FLASH is not set
> +CONFIG_CMD_GPIO=y
>   CONFIG_CMD_I2C=y
>   CONFIG_CMD_MMC=y
>   CONFIG_CMD_PCI=y
> @@ -34,6 +36,10 @@ CONFIG_ENV_IS_IN_MMC=y
>   CONFIG_NET_RANDOM_ETHADDR=y
>   CONFIG_SPL_OF_TRANSLATE=y
>   CONFIG_SCSI_AHCI=y
> +CONFIG_DM_GPIO=y
> +CONFIG_DM_PCA953X=y
> +CONFIG_DM_I2C=y
> +CONFIG_SYS_I2C_MVTWSI=y
>   CONFIG_MMC_SDHCI=y
>   CONFIG_MMC_SDHCI_SDMA=y
>   CONFIG_MMC_SDHCI_MV=y
> diff --git a/include/configs/clearfog.h b/include/configs/clearfog.h
> index ed6f000db16b..1141aee08b6e 100644
> --- a/include/configs/clearfog.h
> +++ b/include/configs/clearfog.h
> @@ -21,13 +21,6 @@
>    * Commands configuration
>    */

> -/* I2C */
> -#define CONFIG_SYS_I2C
> -#define CONFIG_SYS_I2C_MVTWSI
> -#define CONFIG_I2C_MVTWSI_BASE0                MVEBU_TWSI_BASE
> -#define CONFIG_SYS_I2C_SLAVE           0x0
> -#define CONFIG_SYS_I2C_SPEED           100000
> -
>   /* SPI NOR flash default params, used by sf commands */
>   #define CONFIG_SF_DEFAULT_BUS          1

> --
> 2.17.0

> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot

Hi Chris,

On Tue, May 29, 2018 at 10:53:38AM +1200, Chris Packham wrote:
> On Tue, May 29, 2018 at 4:11 AM Baruch Siach <baruch@tkos.co.il> wrote:
> 
> > From: Jon Nettleton <jon@solid-run.com>
> 
> > This switches the clearfog boards to use DM based gpio and i2c
> > drivers.  The io expanders are configured via their device-tree
> > entries.
> 
> > Signed-off-by: Jon Nettleton <jon@solid-run.com>
> > [baruch: add DT i2c aliases]
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > ---

[snip]

> > -static struct marvell_io_exp io_exp[] = {
> > -       { 0x20, 2, 0x40 },      /* Deassert both mini pcie reset signals
> */
> > -       { 0x20, 6, 0xf9 },
> > -       { 0x20, 2, 0x46 },      /* rst signals and ena USB3 current
> limiter */
> > -       { 0x20, 6, 0xb9 },
> > -       { 0x20, 3, 0x00 },      /* Set SFP_TX_DIS to zero */
> > -       { 0x20, 7, 0xbf },      /* Drive SFP_TX_DIS to zero */
> > -};
> > -
> 
> Will this actually work? As far as I can see u-boot lacks a gpio-hog
> implementation so despite the fact that these are all enumerated in the dts
> they won't have the same effect.
> 
> I'd be glad to be proven wrong because I'd like to use a gpio-hog for some
> other boards.

I asked Jon (back on Cc) exactly that before submitting this patch. His 
response:

  I think that is fine. They can still be toggled and controlled via the gpio 
  interface within u-boot. I assume once a proper patch is mainline this will 
  just start to work.

My testing showed now regression because of this patch.

Thanks for reviewing,
baruch

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

> Hi Chris,

> On Tue, May 29, 2018 at 10:53:38AM +1200, Chris Packham wrote:
> > On Tue, May 29, 2018 at 4:11 AM Baruch Siach <baruch@tkos.co.il> wrote:
> >
> > > From: Jon Nettleton <jon@solid-run.com>
> >
> > > This switches the clearfog boards to use DM based gpio and i2c
> > > drivers.  The io expanders are configured via their device-tree
> > > entries.
> >
> > > Signed-off-by: Jon Nettleton <jon@solid-run.com>
> > > [baruch: add DT i2c aliases]
> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > ---

> [snip]

> > > -static struct marvell_io_exp io_exp[] = {
> > > -       { 0x20, 2, 0x40 },      /* Deassert both mini pcie reset
signals
> > */
> > > -       { 0x20, 6, 0xf9 },
> > > -       { 0x20, 2, 0x46 },      /* rst signals and ena USB3 current
> > limiter */
> > > -       { 0x20, 6, 0xb9 },
> > > -       { 0x20, 3, 0x00 },      /* Set SFP_TX_DIS to zero */
> > > -       { 0x20, 7, 0xbf },      /* Drive SFP_TX_DIS to zero */
> > > -};
> > > -
> >
> > Will this actually work? As far as I can see u-boot lacks a gpio-hog
> > implementation so despite the fact that these are all enumerated in the
dts
> > they won't have the same effect.
> >
> > I'd be glad to be proven wrong because I'd like to use a gpio-hog for
some
> > other boards.

> I asked Jon (back on Cc) exactly that before submitting this patch. His
> response:

>    I think that is fine. They can still be toggled and controlled via the
gpio
>    interface within u-boot. I assume once a proper patch is mainline this
will
>    just start to work.

> My testing showed now regression because of this patch.

OK. Did you test with a power cycle? I know some of the pca gpio chips
don't have a reset line so the state may be different depending on if you
power cycled or just used the reset/reboot command. It could also be that
u-boot doesn't access these devices so there's no problem.

I kinda was hoping for "here's a gpio-hog implementation @ patchwork/1234"
:) but I guess I'll have to look into making one myself.


> Thanks for reviewing,

Assuming there's no problem with power-on behaviour

Reviewed-by: Chris Packham <judge.packham@gmail.com>

> 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 -

On 29.05.2018 07:10, Chris Packham wrote:
> On Tue, May 29, 2018 at 4:58 PM Baruch Siach <baruch@tkos.co.il> wrote:
> 
>> Hi Chris,
> 
>> On Tue, May 29, 2018 at 10:53:38AM +1200, Chris Packham wrote:
>>> On Tue, May 29, 2018 at 4:11 AM Baruch Siach <baruch@tkos.co.il> wrote:
>>>
>>>> From: Jon Nettleton <jon@solid-run.com>
>>>
>>>> This switches the clearfog boards to use DM based gpio and i2c
>>>> drivers.  The io expanders are configured via their device-tree
>>>> entries.
>>>
>>>> Signed-off-by: Jon Nettleton <jon@solid-run.com>
>>>> [baruch: add DT i2c aliases]
>>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>>> ---
> 
>> [snip]
> 
>>>> -static struct marvell_io_exp io_exp[] = {
>>>> -       { 0x20, 2, 0x40 },      /* Deassert both mini pcie reset
> signals
>>> */
>>>> -       { 0x20, 6, 0xf9 },
>>>> -       { 0x20, 2, 0x46 },      /* rst signals and ena USB3 current
>>> limiter */
>>>> -       { 0x20, 6, 0xb9 },
>>>> -       { 0x20, 3, 0x00 },      /* Set SFP_TX_DIS to zero */
>>>> -       { 0x20, 7, 0xbf },      /* Drive SFP_TX_DIS to zero */
>>>> -};
>>>> -
>>>
>>> Will this actually work? As far as I can see u-boot lacks a gpio-hog
>>> implementation so despite the fact that these are all enumerated in the
> dts
>>> they won't have the same effect.
>>>
>>> I'd be glad to be proven wrong because I'd like to use a gpio-hog for
> some
>>> other boards.
> 
>> I asked Jon (back on Cc) exactly that before submitting this patch. His
>> response:
> 
>>     I think that is fine. They can still be toggled and controlled via the
> gpio
>>     interface within u-boot. I assume once a proper patch is mainline this
> will
>>     just start to work.
> 
>> My testing showed now regression because of this patch.
> 
> OK. Did you test with a power cycle? I know some of the pca gpio chips
> don't have a reset line so the state may be different depending on if you
> power cycled or just used the reset/reboot command. It could also be that
> u-boot doesn't access these devices so there's no problem.
> 
> I kinda was hoping for "here's a gpio-hog implementation @ patchwork/1234"
> :) but I guess I'll have to look into making one myself.

I would prefer to not drop these I2C accesses from the ClearFog board
C code now, as we don't have a way to do this via the DT right now.
Why are they dropped with this patch? Because of the I2C API
changes? Please move to the DM I2C API then (dm_i2c_read/write).

Thanks,
Stefan

On Tue, May 29, 2018 at 7:37 AM Stefan Roese <sr@denx.de> wrote:

> On 29.05.2018 07:10, Chris Packham wrote:
> > On Tue, May 29, 2018 at 4:58 PM Baruch Siach <baruch@tkos.co.il> wrote:
> >
> >> Hi Chris,
> >
> >> On Tue, May 29, 2018 at 10:53:38AM +1200, Chris Packham wrote:
> >>> On Tue, May 29, 2018 at 4:11 AM Baruch Siach <baruch@tkos.co.il>
> wrote:
> >>>
> >>>> From: Jon Nettleton <jon@solid-run.com>
> >>>
> >>>> This switches the clearfog boards to use DM based gpio and i2c
> >>>> drivers.  The io expanders are configured via their device-tree
> >>>> entries.
> >>>
> >>>> Signed-off-by: Jon Nettleton <jon@solid-run.com>
> >>>> [baruch: add DT i2c aliases]
> >>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> >>>> ---
> >
> >> [snip]
> >
> >>>> -static struct marvell_io_exp io_exp[] = {
> >>>> -       { 0x20, 2, 0x40 },      /* Deassert both mini pcie reset
> > signals
> >>> */
> >>>> -       { 0x20, 6, 0xf9 },
> >>>> -       { 0x20, 2, 0x46 },      /* rst signals and ena USB3 current
> >>> limiter */
> >>>> -       { 0x20, 6, 0xb9 },
> >>>> -       { 0x20, 3, 0x00 },      /* Set SFP_TX_DIS to zero */
> >>>> -       { 0x20, 7, 0xbf },      /* Drive SFP_TX_DIS to zero */
> >>>> -};
> >>>> -
> >>>
> >>> Will this actually work? As far as I can see u-boot lacks a gpio-hog
> >>> implementation so despite the fact that these are all enumerated in the
> > dts
> >>> they won't have the same effect.
> >>>
> >>> I'd be glad to be proven wrong because I'd like to use a gpio-hog for
> > some
> >>> other boards.
> >
> >> I asked Jon (back on Cc) exactly that before submitting this patch. His
> >> response:
> >
> >>     I think that is fine. They can still be toggled and controlled via
> the
> > gpio
> >>     interface within u-boot. I assume once a proper patch is mainline
> this
> > will
> >>     just start to work.
> >
> >> My testing showed now regression because of this patch.
> >
> > OK. Did you test with a power cycle? I know some of the pca gpio chips
> > don't have a reset line so the state may be different depending on if you
> > power cycled or just used the reset/reboot command. It could also be that
> > u-boot doesn't access these devices so there's no problem.
> >
> > I kinda was hoping for "here's a gpio-hog implementation @
> patchwork/1234"
> > :) but I guess I'll have to look into making one myself.
>
> I would prefer to not drop these I2C accesses from the ClearFog board
> C code now, as we don't have a way to do this via the DT right now.
> Why are they dropped with this patch? Because of the I2C API
> changes? Please move to the DM I2C API then (dm_i2c_read/write).
>
> Thanks,
> Stefan
>

Stefan,

These do not need to be setup independently any longer.  Moving to DM_GPIO
allows the PCI driver to handle the reset lines.  The USB3 reset and
current limiter should be handled in board_xhci_enable() which can be done,
although I have never seen an issue using USB3 with this configuration
since I have implemented it.  Since u-boot doesn't understand the SFP setup
at all I would prefer if it left SFP configuration up to Linux.  Just
enabling TX on an SFP module without understanding any of the specifics of
the configuration is a bit dangerous when dealing with modules that aren't
just SGMII 1000base-Tx.

I will follow up with a patch for board_xhci_enable().

Thanks,
Jon

Hi Jon,

On 29.05.2018 11:02, Jon Nettleton wrote:
> On Tue, May 29, 2018 at 7:37 AM Stefan Roese <sr@denx.de 
> <mailto:sr@denx.de>> wrote:
> 
>     On 29.05.2018 07:10, Chris Packham wrote:
>      > On Tue, May 29, 2018 at 4:58 PM Baruch Siach <baruch@tkos.co.il
>     <mailto:baruch@tkos.co.il>> wrote:
>      >
>      >> Hi Chris,
>      >
>      >> On Tue, May 29, 2018 at 10:53:38AM +1200, Chris Packham wrote:
>      >>> On Tue, May 29, 2018 at 4:11 AM Baruch Siach <baruch@tkos.co.il
>     <mailto:baruch@tkos.co.il>> wrote:
>      >>>
>      >>>> From: Jon Nettleton <jon@solid-run.com <mailto:jon@solid-run.com>>
>      >>>
>      >>>> This switches the clearfog boards to use DM based gpio and i2c
>      >>>> drivers.  The io expanders are configured via their device-tree
>      >>>> entries.
>      >>>
>      >>>> Signed-off-by: Jon Nettleton <jon@solid-run.com
>     <mailto:jon@solid-run.com>>
>      >>>> [baruch: add DT i2c aliases]
>      >>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il
>     <mailto:baruch@tkos.co.il>>
>      >>>> ---
>      >
>      >> [snip]
>      >
>      >>>> -static struct marvell_io_exp io_exp[] = {
>      >>>> -       { 0x20, 2, 0x40 },      /* Deassert both mini pcie reset
>      > signals
>      >>> */
>      >>>> -       { 0x20, 6, 0xf9 },
>      >>>> -       { 0x20, 2, 0x46 },      /* rst signals and ena USB3
>     current
>      >>> limiter */
>      >>>> -       { 0x20, 6, 0xb9 },
>      >>>> -       { 0x20, 3, 0x00 },      /* Set SFP_TX_DIS to zero */
>      >>>> -       { 0x20, 7, 0xbf },      /* Drive SFP_TX_DIS to zero */
>      >>>> -};
>      >>>> -
>      >>>
>      >>> Will this actually work? As far as I can see u-boot lacks a
>     gpio-hog
>      >>> implementation so despite the fact that these are all
>     enumerated in the
>      > dts
>      >>> they won't have the same effect.
>      >>>
>      >>> I'd be glad to be proven wrong because I'd like to use a
>     gpio-hog for
>      > some
>      >>> other boards.
>      >
>      >> I asked Jon (back on Cc) exactly that before submitting this
>     patch. His
>      >> response:
>      >
>      >>     I think that is fine. They can still be toggled and
>     controlled via the
>      > gpio
>      >>     interface within u-boot. I assume once a proper patch is
>     mainline this
>      > will
>      >>     just start to work.
>      >
>      >> My testing showed now regression because of this patch.
>      >
>      > OK. Did you test with a power cycle? I know some of the pca gpio
>     chips
>      > don't have a reset line so the state may be different depending
>     on if you
>      > power cycled or just used the reset/reboot command. It could also
>     be that
>      > u-boot doesn't access these devices so there's no problem.
>      >
>      > I kinda was hoping for "here's a gpio-hog implementation @
>     patchwork/1234"
>      > :) but I guess I'll have to look into making one myself.
> 
>     I would prefer to not drop these I2C accesses from the ClearFog board
>     C code now, as we don't have a way to do this via the DT right now.
>     Why are they dropped with this patch? Because of the I2C API
>     changes? Please move to the DM I2C API then (dm_i2c_read/write).
> 
>     Thanks,
>     Stefan
> 
> 
> Stefan,
> 
> These do not need to be setup independently any longer.  Moving to 
> DM_GPIO allows the PCI driver to handle the reset lines.  The USB3 reset 
> and current limiter should be handled in board_xhci_enable() which can 
> be done, although I have never seen an issue using USB3 with this 
> configuration since I have implemented it.  Since u-boot doesn't 
> understand the SFP setup at all I would prefer if it left SFP 
> configuration up to Linux.  Just enabling TX on an SFP module without 
> understanding any of the specifics of the configuration is a bit 
> dangerous when dealing with modules that aren't just SGMII 1000base-Tx.

Thanks for the explanation. With this I'm okay with removing the
I2C code from the board file:

Reviewed-by: Stefan Roese <sr@denx.de>

> I will follow up with a patch for board_xhci_enable().

Good.

Thanks,
Stefan

On 28.05.2018 18:10, Baruch Siach wrote:
> From: Jon Nettleton <jon@solid-run.com>
> 
> This switches the clearfog boards to use DM based gpio and i2c
> drivers.  The io expanders are configured via their device-tree
> entries.
> 
> Signed-off-by: Jon Nettleton <jon@solid-run.com>
> [baruch: add DT i2c aliases]
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Applied to u-boot-marvell/master.

Thanks,
Stefan