[U-Boot] [PATCH 4/4] arm: zynq: use i2c cadence DM driver

[Luis Araneda]

Hi Michal,

On Fri, Jul 20, 2018 at 6:57 AM Michal Simek <monstr at monstr.eu> wrote:
> On 9.7.2018 07:00, Luis Araneda wrote:
> > [...]
> > --- a/arch/arm/dts/zynq-syzygy-hub.dts
> > +++ b/arch/arm/dts/zynq-syzygy-hub.dts
> > @@ -15,6 +15,7 @@
> >       aliases {
> >               ethernet0 = &gem0;
> >               serial0 = &uart0;
> > +             i2c0 = &i2c1;
> >               mmc0 = &sdhci0;
> >       };
> >
>
> This can be separate patch.

Ok

> > diff --git a/arch/arm/dts/zynq-zybo.dts b/arch/arm/dts/zynq-zybo.dts
> > index 3844822305..b5e0f3d9b3 100644
> > --- a/arch/arm/dts/zynq-zybo.dts
> > +++ b/arch/arm/dts/zynq-zybo.dts
> > @@ -14,6 +14,8 @@
> >               ethernet0 = &gem0;
> >               serial0 = &uart1;
> >               spi0 = &qspi;
> > +             i2c0 = &i2c0;
> > +             i2c1 = &i2c1;
> >               mmc0 = &sdhci0;
> >       };
> >
> > @@ -49,6 +51,14 @@
> >       };
> >  };
> >
> > +&i2c0 {
> > +     status = "okay";
> > +};
> > +
> > +&i2c1 {
> > +     status = "okay";
> > +};
> > +
>
> IIRC zybo has no connection from PS to i2c and it is done via PL.
> And deal was that only things which are in PS part should be listed in
> DTS file. It means this shouldn't be here.

The PS has I2C peripherals. For the Zybo in particular, there are two
I2Cs, which must be routed through the PL to be connected to the
EEPROM and HDMI port respectively. That means that if the PL is not
programmed, the I2C driver binds and works properly but the SCL and
SDA signals of the I2C peripherals are not connected to anything.
So, I'm not sure if the rule is applicable here, because the I2Cs are
in PS, but to properly use them you have to program the PL.

Besides, adding the aliases and nodes was the only way I could find to
make the driver work (bind) automatically. If I don't add the aliases,
the I2C devices are not assigned a bus (-1), and if I don't add the
nodes, the driver does not bind.

If the I2C nodes don't belong in the .dts file, and I'm using DM
(which will eventually be mandatory), does that means I would have to
bind the driver manually to obtain the current functionality without
DM? I suppose that's possible (don't know how). It sounds like the
functionality provided by device-tree overlays.

> >  &qspi {
> >       u-boot,dm-pre-reloc;
> >       status = "okay";
> > diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c
> > index dcbf788918..5b9ee10a90 100644
> > --- a/board/xilinx/zynq/board.c
> > +++ b/board/xilinx/zynq/board.c
> > @@ -8,6 +8,7 @@
> >  #include <dm/uclass.h>
> >  #include <fdtdec.h>
> >  #include <fpga.h>
> > +#include <i2c.h>
> >  #include <mmc.h>
> >  #include <watchdog.h>
> >  #include <wdt.h>
> > @@ -76,10 +77,25 @@ int board_late_init(void)
> >  int zynq_board_read_rom_ethaddr(unsigned char *ethaddr)
> >  {
> >  #if defined(CONFIG_MAC_ADDR_IN_I2C_EEPROM)
> > -     if (eeprom_read(CONFIG_MAC_ADDR_I2C_EEPROM_CHIP_ADDR,
> > -                     CONFIG_MAC_ADDR_I2C_EEPROM_DATA_ADDR_START,
> > -                     ethaddr, 6))
> > -             printf("I2C EEPROM MAC address read failed\n");
> > +     int ret;
> > +     struct udevice *bus, *dev;
> > +
> > +     ret = uclass_get_device_by_seq(UCLASS_I2C,
> > +                                    CONFIG_MAC_ADDR_I2C_EEPROM_BUS,
> > +                                    &bus);
> > +     if (!ret)
> > +             ret = i2c_get_chip(bus,
> > +                                CONFIG_MAC_ADDR_I2C_EEPROM_CHIP_ADDR,
> > +                                1, &dev);
> > +     if (!ret)
> > +             ret = i2c_set_chip_offset_len(dev,
> > +                                           CONFIG_MAC_ADDR_I2C_EEPROM_DATA_ADDR_LEN);
> > +     if (!ret)
> > +             ret = dm_i2c_read(dev,
> > +                               CONFIG_MAC_ADDR_I2C_EEPROM_DATA_ADDR_START,
> > +                               ethaddr, 6);
> > +     if (ret)
> > +             printf("I2C EEPROM MAC address read failed (%i)\n", ret);
> >  #endif
>
> In ZynqMP there is also call i2c_set_bus_num which is what you do above.
>
> I don't think that make sense to duplicate this code in board files.
> This should go to core. I think eeprom hasn't been converted to DM and
> that's the thing which should happen first. Then calling sequence should
> be the same in board files.

Ok. I'm assuming that you are referring to the eeprom command? because
I found the i2c_eeprom driver (CONFIG_I2C_EEPROM), which is using DM.
But, I'm not sure if we can use it because it has the same problems of
the I2C nodes above, and this is even worse, as the driver won't bind
unless the PL is programmed first.

> > --- a/configs/syzygy_hub_defconfig
> > +++ b/configs/syzygy_hub_defconfig
> > @@ -18,7 +18,6 @@ CONFIG_BOOTCOMMAND="run $modeboot || run distro_bootcmd"
> >  CONFIG_SPL_STACK_R=y
> >  CONFIG_SPL_OS_BOOT=y
> >  CONFIG_SYS_PROMPT="Zynq> "
> > -CONFIG_CMD_EEPROM=y
>
> this will caused regression.

Is it because someone could have been using the command?


Thanks,

Luis Araneda.