[U-Boot] [PATCH v3 011/108] i2c: Tidy up designware PCI support

Bin Meng bmeng.cn at gmail.com
Mon Oct 28 05:54:19 UTC 2019


+Stefan

Hi Simon,

On Mon, Oct 21, 2019 at 11:33 AM 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.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/i2c/Makefile         |   3 +
>  drivers/i2c/designware_i2c.c | 104 ++++++-----------------------------
>  drivers/i2c/designware_i2c.h |  35 ++++++++++++
>  drivers/i2c/dw_i2c_pci.c     |  78 ++++++++++++++++++++++++++
>  4 files changed, 132 insertions(+), 88 deletions(-)
>  create mode 100644 drivers/i2c/dw_i2c_pci.c
>
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index c2f75d87559..a7fa38b8dcf 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) += dw_i2c_pci.o

nits: designware_i2c_pci.o for consistency?

> +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 6daa90e7442..54e4a70c74c 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,8 @@ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base,
>         unsigned int ena;
>         int i2c_spd;
>
> -       if (speed >= I2C_MAX_SPEED)
> +       if (speed >= I2C_MAX_SPEED &&
> +           (!scl_sda_cfg || scl_sda_cfg->has_max_speed))

I think the logic should be && not ||

And I don't see scl_sda_cfg->has_max_speed in the original driver. Is
this new functionality?

>                 i2c_spd = IC_SPEED_MODE_MAX;
>         else if (speed >= I2C_FAST_SPEED)
>                 i2c_spd = IC_SPEED_MODE_FAST;
> @@ -106,7 +79,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 +91,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
>
>         case IC_SPEED_MODE_STANDARD:
>                 cntl |= IC_CON_SPD_SS;
> @@ -565,24 +536,20 @@ static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr,
>         return ret;
>  }
>
> -static int designware_i2c_probe(struct udevice *bus)
> +static int designware_i2c_ofdata_to_platdata(struct udevice *bus)
>  {
>         struct dw_i2c *priv = dev_get_priv(bus);
> -       int ret;
>
> -       if (device_is_on_pci_bus(bus)) {
> -#ifdef CONFIG_DM_PCI
> -               /* Save base address from PCI BAR */
> -               priv->regs = (struct i2c_regs *)
> -                       dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM);
> -#ifdef CONFIG_X86
> -               /* Use BayTrail specific timing values */
> -               priv->scl_sda_cfg = &byt_config;
> -#endif
> -#endif
> -       } else {
> -               priv->regs = (struct i2c_regs *)devfdt_get_addr_ptr(bus);
> -       }
> +       printf("bad\n");

What's this?

> +       priv->regs = (struct i2c_regs *)devfdt_get_addr_ptr(bus);
> +
> +       return 0;
> +}
> +
> +int designware_i2c_probe(struct udevice *bus)
> +{
> +       struct dw_i2c *priv = dev_get_priv(bus);
> +       int ret;
>
>         ret = reset_get_bulk(bus, &priv->resets);
>         if (ret)
> @@ -606,7 +573,7 @@ static int designware_i2c_probe(struct udevice *bus)
>         return __dw_i2c_init(priv->regs, 0, 0);
>  }
>
> -static int designware_i2c_remove(struct udevice *dev)
> +int designware_i2c_remove(struct udevice *dev)
>  {
>         struct dw_i2c *priv = dev_get_priv(dev);
>
> @@ -618,30 +585,7 @@ static int designware_i2c_remove(struct udevice *dev)
>         return reset_release_bulk(&priv->resets);
>  }
>
> -static int designware_i2c_bind(struct udevice *dev)
> -{
> -       static int num_cards;
> -       char name[20];
> -
> -       /* Create a unique device name for PCI type devices */
> -       if (device_is_on_pci_bus(dev)) {
> -               /*
> -                * ToDo:
> -                * Setting req_seq in the driver is probably not recommended.
> -                * But without a DT alias the number is not configured. And
> -                * using this driver is impossible for PCIe I2C devices.
> -                * This can be removed, once a better (correct) way for this
> -                * is found and implemented.
> -                */
> -               dev->req_seq = num_cards;
> -               sprintf(name, "i2c_designware#%u", num_cards++);
> -               device_set_name(dev, name);
> -       }
> -
> -       return 0;
> -}
> -
> -static const struct dm_i2c_ops designware_i2c_ops = {
> +const struct dm_i2c_ops designware_i2c_ops = {
>         .xfer           = designware_i2c_xfer,
>         .probe_chip     = designware_i2c_probe_chip,
>         .set_bus_speed  = designware_i2c_set_bus_speed,
> @@ -656,7 +600,7 @@ U_BOOT_DRIVER(i2c_designware) = {
>         .name   = "i2c_designware",
>         .id     = UCLASS_I2C,
>         .of_match = designware_i2c_ids,
> -       .bind   = designware_i2c_bind,
> +       .ofdata_to_platdata = designware_i2c_ofdata_to_platdata,
>         .probe  = designware_i2c_probe,
>         .priv_auto_alloc_size = sizeof(struct dw_i2c),
>         .remove = designware_i2c_remove,
> @@ -664,20 +608,4 @@ U_BOOT_DRIVER(i2c_designware) = {
>         .ops    = &designware_i2c_ops,
>  };
>
> -#ifdef CONFIG_X86
> -static struct pci_device_id designware_pci_supported[] = {
> -       /* Intel BayTrail has 7 I2C controller located on the PCI bus */
> -       { PCI_VDEVICE(INTEL, 0x0f41) },
> -       { PCI_VDEVICE(INTEL, 0x0f42) },
> -       { PCI_VDEVICE(INTEL, 0x0f43) },
> -       { PCI_VDEVICE(INTEL, 0x0f44) },
> -       { PCI_VDEVICE(INTEL, 0x0f45) },
> -       { PCI_VDEVICE(INTEL, 0x0f46) },
> -       { PCI_VDEVICE(INTEL, 0x0f47) },
> -       {},
> -};
> -
> -U_BOOT_PCI_DEVICE(i2c_designware, designware_pci_supported);
> -#endif
> -
>  #endif /* CONFIG_DM_I2C */
> diff --git a/drivers/i2c/designware_i2c.h b/drivers/i2c/designware_i2c.h
> index 20ff20d9b83..48766d08067 100644
> --- a/drivers/i2c/designware_i2c.h
> +++ b/drivers/i2c/designware_i2c.h
> @@ -7,6 +7,8 @@
>  #ifndef __DW_I2C_H_
>  #define __DW_I2C_H_
>
> +#include <reset.h>
> +
>  struct i2c_regs {
>         u32 ic_con;             /* 0x00 */
>         u32 ic_tar;             /* 0x04 */
> @@ -131,4 +133,37 @@ struct i2c_regs {
>  #define I2C_FAST_SPEED         400000
>  #define I2C_STANDARD_SPEED     100000
>
> +/**
> + * struct dw_scl_sda_cfg - I2C timing configuration
> + *
> + * @has_max_speed: Support maximum speed (1Mbps)
> + * @ss_hcnt: Standard speed high time in ns
> + * @fs_hcnt: Fast speed high time in ns
> + * @ss_lcnt: Standard speed low time in ns
> + * @fs_lcnt: Fast speed low time in ns
> + * @sda_hold: SDA hold time
> + */
> +struct dw_scl_sda_cfg {
> +       bool has_max_speed;
> +       u32 ss_hcnt;
> +       u32 fs_hcnt;
> +       u32 ss_lcnt;
> +       u32 fs_lcnt;
> +       u32 sda_hold;
> +};
> +
> +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
> +};
> +
> +extern const struct dm_i2c_ops designware_i2c_ops;
> +
> +int designware_i2c_probe(struct udevice *bus);
> +int designware_i2c_remove(struct udevice *dev);
> +
>  #endif /* __DW_I2C_H_ */
> diff --git a/drivers/i2c/dw_i2c_pci.c b/drivers/i2c/dw_i2c_pci.c
> new file mode 100644
> index 00000000000..065c0aa5994
> --- /dev/null
> +++ b/drivers/i2c/dw_i2c_pci.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2009
> + * Vipin Kumar, ST Micoelectronics, vipin.kumar at st.com.
> + * Copyright 2019 Google Inc
> + */
> +
> +#include <dm.h>
> +#include "designware_i2c.h"
> +
> +/* 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,
> +};
> +
> +static int designware_i2c_pci_probe(struct udevice *dev)
> +{
> +       struct dw_i2c *priv = dev_get_priv(dev);
> +
> +       /* Save base address from PCI BAR */
> +       priv->regs = (struct i2c_regs *)
> +               dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0, PCI_REGION_MEM);
> +       if (IS_ENABLED(CONFIG_INTEL_BAYTRAIL))
> +               /* Use BayTrail specific timing values */
> +               priv->scl_sda_cfg = &byt_config;
> +
> +       return designware_i2c_probe(dev);
> +}
> +
> +static int designware_i2c_pci_bind(struct udevice *dev)
> +{
> +       static int num_cards;
> +       char name[20];
> +
> +       /*
> +        * Create a unique device name for PCI type devices
> +        * ToDo:
> +        * Setting req_seq in the driver is probably not recommended.
> +        * But without a DT alias the number is not configured. And
> +        * using this driver is impossible for PCIe I2C devices.
> +        * This can be removed, once a better (correct) way for this
> +        * is found and implemented.
> +        */
> +       dev->req_seq = num_cards;
> +       sprintf(name, "i2c_designware#%u", num_cards++);
> +       device_set_name(dev, name);
> +
> +       return 0;
> +}
> +
> +U_BOOT_DRIVER(i2c_designware_pci) = {
> +       .name   = "i2c_designware_pci",
> +       .id     = UCLASS_I2C,
> +       .bind   = designware_i2c_pci_bind,
> +       .probe  = designware_i2c_pci_probe,
> +       .priv_auto_alloc_size = sizeof(struct dw_i2c),
> +       .remove = designware_i2c_remove,
> +       .flags = DM_FLAG_OS_PREPARE,
> +       .ops    = &designware_i2c_ops,
> +};
> +
> +static struct pci_device_id designware_pci_supported[] = {
> +       /* Intel BayTrail has 7 I2C controller located on the PCI bus */
> +       { PCI_VDEVICE(INTEL, 0x0f41) },
> +       { PCI_VDEVICE(INTEL, 0x0f42) },
> +       { PCI_VDEVICE(INTEL, 0x0f43) },
> +       { PCI_VDEVICE(INTEL, 0x0f44) },
> +       { PCI_VDEVICE(INTEL, 0x0f45) },
> +       { PCI_VDEVICE(INTEL, 0x0f46) },
> +       { PCI_VDEVICE(INTEL, 0x0f47) },
> +       {},
> +};
> +
> +U_BOOT_PCI_DEVICE(i2c_designware_pci, designware_pci_supported);
> --

Regards,
Bin


More information about the U-Boot mailing list