[U-Boot] [PATCH v5 007/101] i2c: designware: Tidy up PCI support

[Simon Glass]

Hi Bin,

On Mon, 25 Nov 2019 at 22:09, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
>
> On Mon, Nov 25, 2019 at 12:11 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > This is hacked into the driver at present. It seems better to have it as
> > a separate driver that uses the base driver. Create a new file and put
> > the X86 code into it.
> >
> > Actually the Baytrail settings should really come from the device tree.
> >
> > Note that 'has_max_speed' is added as well. This is currently always false
> > but since only Baytrail provides the config, it does not affect operation
> > for other devices.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > Reviewed-by: Heiko Schocher <hs at denx.de>
> > ---
> >
> > Changes in v5: None
> > Changes in v4:
> > - Add a comment about the speed logic in __dw_i2c_set_bus_speed()
> > - Add a comment in the commit message about why has_max_speed is added
> > - Drop unwanted debug printf("bad\n")
> > - Fix indentation nit
> > - Rename new file to designware_i2c_pci.c
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> >  drivers/i2c/Makefile             |   3 +
> >  drivers/i2c/designware_i2c.c     | 106 +++++--------------------------
> >  drivers/i2c/designware_i2c.h     |  35 ++++++++++
> >  drivers/i2c/designware_i2c_pci.c |  79 +++++++++++++++++++++++
> >  4 files changed, 134 insertions(+), 89 deletions(-)
> >  create mode 100644 drivers/i2c/designware_i2c_pci.c
> >
> > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> > index c2f75d8755..f5a471f887 100644
> > --- a/drivers/i2c/Makefile
> > +++ b/drivers/i2c/Makefile
> > @@ -14,6 +14,9 @@ obj-$(CONFIG_SYS_I2C_AT91) += at91_i2c.o
> >  obj-$(CONFIG_SYS_I2C_CADENCE) += i2c-cdns.o
> >  obj-$(CONFIG_SYS_I2C_DAVINCI) += davinci_i2c.o
> >  obj-$(CONFIG_SYS_I2C_DW) += designware_i2c.o
> > +ifdef CONFIG_DM_PCI
> > +obj-$(CONFIG_SYS_I2C_DW) += designware_i2c_pci.o
> > +endif
> >  obj-$(CONFIG_SYS_I2C_FSL) += fsl_i2c.o
> >  obj-$(CONFIG_SYS_I2C_IHS) += ihs_i2c.o
> >  obj-$(CONFIG_SYS_I2C_INTEL) += intel_i2c.o
> > diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c
> > index 6daa90e744..99b7d09bb2 100644
> > --- a/drivers/i2c/designware_i2c.c
> > +++ b/drivers/i2c/designware_i2c.c
> > @@ -13,34 +13,6 @@
> >  #include <asm/io.h>
> >  #include "designware_i2c.h"
> >
> > -struct dw_scl_sda_cfg {
> > -       u32 ss_hcnt;
> > -       u32 fs_hcnt;
> > -       u32 ss_lcnt;
> > -       u32 fs_lcnt;
> > -       u32 sda_hold;
> > -};
> > -
> > -#ifdef CONFIG_X86
> > -/* BayTrail HCNT/LCNT/SDA hold time */
> > -static struct dw_scl_sda_cfg byt_config = {
> > -       .ss_hcnt = 0x200,
> > -       .fs_hcnt = 0x55,
> > -       .ss_lcnt = 0x200,
> > -       .fs_lcnt = 0x99,
> > -       .sda_hold = 0x6,
> > -};
> > -#endif
> > -
> > -struct dw_i2c {
> > -       struct i2c_regs *regs;
> > -       struct dw_scl_sda_cfg *scl_sda_cfg;
> > -       struct reset_ctl_bulk resets;
> > -#if CONFIG_IS_ENABLED(CLK)
> > -       struct clk clk;
> > -#endif
> > -};
> > -
> >  #ifdef CONFIG_SYS_I2C_DW_ENABLE_STATUS_UNSUPPORTED
> >  static int  dw_i2c_enable(struct i2c_regs *i2c_base, bool enable)
> >  {
> > @@ -90,7 +62,9 @@ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base,
> >         unsigned int ena;
> >         int i2c_spd;
> >
> > -       if (speed >= I2C_MAX_SPEED)
> > +       /* Allow max speed if there is no config , or the config allows it */
>
> nits: remove the space before ,

OK done.

>
> > +       if (speed >= I2C_MAX_SPEED &&
> > +           (!scl_sda_cfg || scl_sda_cfg->has_max_speed))
>
> So with this change, for Baytrail, this new logic does not match the old codes.
>
> Baytrail has the config, but its config does not have the
> has_max_speed, so the if () logic evaluates to false.

>From my reading of it, that is correct. The mode_max code is behind an
#ifdef in the old code.

>
> >                 i2c_spd = IC_SPEED_MODE_MAX;
> >         else if (speed >= I2C_FAST_SPEED)
> >                 i2c_spd = IC_SPEED_MODE_FAST;
> > @@ -106,7 +80,6 @@ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base,
> >         cntl = (readl(&i2c_base->ic_con) & (~IC_CON_SPD_MSK));
> >
> >         switch (i2c_spd) {
> > -#ifndef CONFIG_X86 /* No High-speed for BayTrail yet */
> >         case IC_SPEED_MODE_MAX:
> >                 cntl |= IC_CON_SPD_SS;
> >                 if (scl_sda_cfg) {
> > @@ -119,7 +92,6 @@ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base,
> >                 writel(hcnt, &i2c_base->ic_hs_scl_hcnt);
> >                 writel(lcnt, &i2c_base->ic_hs_scl_lcnt);
> >                 break;
> > -#endif
[..]

Regards,
Simon