| Message ID | 1458287661-21745-6-git-send-email-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Heiko Schocher |
| Headers | show |
Hi Stefan, On Fri, Mar 18, 2016 at 3:54 PM, Stefan Roese <sr@denx.de> wrote: > This patch adds support for the PCI(e) based I2C cores. Which can be > found for example on the Intel Bay Trail SoC. It has 7 I2C controllers > implemented as PCI devices. > > This patch also adds the fixed values for the timing registers for > BayTrail which are taken from the Linux designware I2C driver. > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Simon Glass <sjg@chromium.org> > Cc: Bin Meng <bmeng.cn@gmail.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Heiko Schocher <hs@denx.de> > --- > drivers/i2c/designware_i2c.c | 111 +++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 101 insertions(+), 10 deletions(-) > > diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c > index 4e5340d..f7f2eba 100644 > --- a/drivers/i2c/designware_i2c.c > +++ b/drivers/i2c/designware_i2c.c > @@ -8,11 +8,32 @@ > #include <common.h> > #include <dm.h> > #include <i2c.h> > +#include <pci.h> > #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; > }; > > static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) > @@ -42,6 +63,7 @@ static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) > * Set the i2c speed. > */ > static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base, > + struct dw_scl_sda_cfg *scl_sda_cfg, > unsigned int speed) > { > unsigned int cntl; > @@ -61,34 +83,55 @@ 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_HS; > - hcnt = (IC_CLK * MIN_HS_SCL_HIGHTIME) / NANO_TO_MICRO; > + cntl |= IC_CON_SPD_SS; > + if (scl_sda_cfg) { > + hcnt = scl_sda_cfg->fs_hcnt; > + lcnt = scl_sda_cfg->fs_lcnt; > + } else { > + hcnt = (IC_CLK * MIN_HS_SCL_HIGHTIME) / NANO_TO_MICRO; > + lcnt = (IC_CLK * MIN_HS_SCL_LOWTIME) / NANO_TO_MICRO; > + } > writel(hcnt, &i2c_base->ic_hs_scl_hcnt); > - lcnt = (IC_CLK * MIN_HS_SCL_LOWTIME) / NANO_TO_MICRO; > writel(lcnt, &i2c_base->ic_hs_scl_lcnt); > break; > +#endif > > case IC_SPEED_MODE_STANDARD: > cntl |= IC_CON_SPD_SS; > - hcnt = (IC_CLK * MIN_SS_SCL_HIGHTIME) / NANO_TO_MICRO; > + if (scl_sda_cfg) { > + hcnt = scl_sda_cfg->ss_hcnt; > + lcnt = scl_sda_cfg->ss_lcnt; > + } else { > + hcnt = (IC_CLK * MIN_SS_SCL_HIGHTIME) / NANO_TO_MICRO; > + lcnt = (IC_CLK * MIN_SS_SCL_LOWTIME) / NANO_TO_MICRO; > + } > writel(hcnt, &i2c_base->ic_ss_scl_hcnt); > - lcnt = (IC_CLK * MIN_SS_SCL_LOWTIME) / NANO_TO_MICRO; > writel(lcnt, &i2c_base->ic_ss_scl_lcnt); > break; > > case IC_SPEED_MODE_FAST: > default: > cntl |= IC_CON_SPD_FS; > - hcnt = (IC_CLK * MIN_FS_SCL_HIGHTIME) / NANO_TO_MICRO; > + if (scl_sda_cfg) { > + hcnt = scl_sda_cfg->fs_hcnt; > + lcnt = scl_sda_cfg->fs_lcnt; > + } else { > + hcnt = (IC_CLK * MIN_FS_SCL_HIGHTIME) / NANO_TO_MICRO; > + lcnt = (IC_CLK * MIN_FS_SCL_LOWTIME) / NANO_TO_MICRO; > + } > writel(hcnt, &i2c_base->ic_fs_scl_hcnt); > - lcnt = (IC_CLK * MIN_FS_SCL_LOWTIME) / NANO_TO_MICRO; > writel(lcnt, &i2c_base->ic_fs_scl_lcnt); > break; > } > > writel(cntl, &i2c_base->ic_con); > > + /* Configure SDA Hold Time if required */ > + if (scl_sda_cfg) > + writel(scl_sda_cfg->sda_hold, &i2c_base->ic_sda_hold); > + > /* Enable back i2c now speed set */ > dw_i2c_enable(i2c_base, 1); > > @@ -315,7 +358,7 @@ static void __dw_i2c_init(struct i2c_regs *i2c_base, int speed, int slaveaddr) > writel(IC_TX_TL, &i2c_base->ic_tx_tl); > writel(IC_STOP_DET, &i2c_base->ic_intr_mask); > #ifndef CONFIG_DM_I2C > - __dw_i2c_set_bus_speed(i2c_base, speed); > + __dw_i2c_set_bus_speed(i2c_base, NULL, speed); > writel(slaveaddr, &i2c_base->ic_sar); > #endif > > @@ -356,7 +399,7 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap, > unsigned int speed) > { > adap->speed = speed; > - return __dw_i2c_set_bus_speed(i2c_get_base(adap), speed); > + return __dw_i2c_set_bus_speed(i2c_get_base(adap), NULL, speed); > } > > static void dw_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) > @@ -451,7 +494,7 @@ static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) > { > struct dw_i2c *i2c = dev_get_priv(bus); > > - return __dw_i2c_set_bus_speed(i2c->regs, speed); > + return __dw_i2c_set_bus_speed(i2c->regs, i2c->scl_sda_cfg, speed); > } > > static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, > @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) > { > struct dw_i2c *priv = dev_get_priv(bus); > > +#ifdef CONFIG_X86 > + /* Save base address from PCI BAR */ > + priv->regs = (struct i2c_regs *) > + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); > + /* Use BayTrail specific timing values */ > + priv->scl_sda_cfg = &byt_config; > +#else How about: if (device_is_on_pci_bus(dev)) { do the PCI I2C stuff here; } See driver/net/designware.c for example. > /* Save base address from device-tree */ > priv->regs = (struct i2c_regs *)dev_get_addr(bus); > +#endif > > __dw_i2c_init(priv->regs, 0, 0); > > return 0; > } > > +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); > + } I believe the above should be wrapped by #ifdef CONFIG_DM_PCI #endif, otherwise it won't build when DM_PCI is not enabled. > + > + return 0; > +} > + > static const struct dm_i2c_ops designware_i2c_ops = { > .xfer = designware_i2c_xfer, > .probe_chip = designware_i2c_probe_chip, > @@ -499,9 +573,26 @@ U_BOOT_DRIVER(i2c_designware) = { > .name = "i2c_designware", > .id = UCLASS_I2C, > .of_match = designware_i2c_ids, > + .bind = designware_i2c_bind, > .probe = designware_i2c_probe, > .priv_auto_alloc_size = sizeof(struct dw_i2c), > .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 */ > -- Regards, Bin
Hi Bin, On 21.03.2016 09:54, Bin Meng wrote: > On Fri, Mar 18, 2016 at 3:54 PM, Stefan Roese <sr@denx.de> wrote: >> This patch adds support for the PCI(e) based I2C cores. Which can be >> found for example on the Intel Bay Trail SoC. It has 7 I2C controllers >> implemented as PCI devices. >> >> This patch also adds the fixed values for the timing registers for >> BayTrail which are taken from the Linux designware I2C driver. >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: Bin Meng <bmeng.cn@gmail.com> >> Cc: Marek Vasut <marex@denx.de> >> Cc: Heiko Schocher <hs@denx.de> >> --- >> drivers/i2c/designware_i2c.c | 111 +++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 101 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c >> index 4e5340d..f7f2eba 100644 >> --- a/drivers/i2c/designware_i2c.c >> +++ b/drivers/i2c/designware_i2c.c >> @@ -8,11 +8,32 @@ >> #include <common.h> >> #include <dm.h> >> #include <i2c.h> >> +#include <pci.h> >> #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; >> }; >> >> static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) >> @@ -42,6 +63,7 @@ static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) >> * Set the i2c speed. >> */ >> static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base, >> + struct dw_scl_sda_cfg *scl_sda_cfg, >> unsigned int speed) >> { >> unsigned int cntl; >> @@ -61,34 +83,55 @@ 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_HS; >> - hcnt = (IC_CLK * MIN_HS_SCL_HIGHTIME) / NANO_TO_MICRO; >> + cntl |= IC_CON_SPD_SS; >> + if (scl_sda_cfg) { >> + hcnt = scl_sda_cfg->fs_hcnt; >> + lcnt = scl_sda_cfg->fs_lcnt; >> + } else { >> + hcnt = (IC_CLK * MIN_HS_SCL_HIGHTIME) / NANO_TO_MICRO; >> + lcnt = (IC_CLK * MIN_HS_SCL_LOWTIME) / NANO_TO_MICRO; >> + } >> writel(hcnt, &i2c_base->ic_hs_scl_hcnt); >> - lcnt = (IC_CLK * MIN_HS_SCL_LOWTIME) / NANO_TO_MICRO; >> writel(lcnt, &i2c_base->ic_hs_scl_lcnt); >> break; >> +#endif >> >> case IC_SPEED_MODE_STANDARD: >> cntl |= IC_CON_SPD_SS; >> - hcnt = (IC_CLK * MIN_SS_SCL_HIGHTIME) / NANO_TO_MICRO; >> + if (scl_sda_cfg) { >> + hcnt = scl_sda_cfg->ss_hcnt; >> + lcnt = scl_sda_cfg->ss_lcnt; >> + } else { >> + hcnt = (IC_CLK * MIN_SS_SCL_HIGHTIME) / NANO_TO_MICRO; >> + lcnt = (IC_CLK * MIN_SS_SCL_LOWTIME) / NANO_TO_MICRO; >> + } >> writel(hcnt, &i2c_base->ic_ss_scl_hcnt); >> - lcnt = (IC_CLK * MIN_SS_SCL_LOWTIME) / NANO_TO_MICRO; >> writel(lcnt, &i2c_base->ic_ss_scl_lcnt); >> break; >> >> case IC_SPEED_MODE_FAST: >> default: >> cntl |= IC_CON_SPD_FS; >> - hcnt = (IC_CLK * MIN_FS_SCL_HIGHTIME) / NANO_TO_MICRO; >> + if (scl_sda_cfg) { >> + hcnt = scl_sda_cfg->fs_hcnt; >> + lcnt = scl_sda_cfg->fs_lcnt; >> + } else { >> + hcnt = (IC_CLK * MIN_FS_SCL_HIGHTIME) / NANO_TO_MICRO; >> + lcnt = (IC_CLK * MIN_FS_SCL_LOWTIME) / NANO_TO_MICRO; >> + } >> writel(hcnt, &i2c_base->ic_fs_scl_hcnt); >> - lcnt = (IC_CLK * MIN_FS_SCL_LOWTIME) / NANO_TO_MICRO; >> writel(lcnt, &i2c_base->ic_fs_scl_lcnt); >> break; >> } >> >> writel(cntl, &i2c_base->ic_con); >> >> + /* Configure SDA Hold Time if required */ >> + if (scl_sda_cfg) >> + writel(scl_sda_cfg->sda_hold, &i2c_base->ic_sda_hold); >> + >> /* Enable back i2c now speed set */ >> dw_i2c_enable(i2c_base, 1); >> >> @@ -315,7 +358,7 @@ static void __dw_i2c_init(struct i2c_regs *i2c_base, int speed, int slaveaddr) >> writel(IC_TX_TL, &i2c_base->ic_tx_tl); >> writel(IC_STOP_DET, &i2c_base->ic_intr_mask); >> #ifndef CONFIG_DM_I2C >> - __dw_i2c_set_bus_speed(i2c_base, speed); >> + __dw_i2c_set_bus_speed(i2c_base, NULL, speed); >> writel(slaveaddr, &i2c_base->ic_sar); >> #endif >> >> @@ -356,7 +399,7 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap, >> unsigned int speed) >> { >> adap->speed = speed; >> - return __dw_i2c_set_bus_speed(i2c_get_base(adap), speed); >> + return __dw_i2c_set_bus_speed(i2c_get_base(adap), NULL, speed); >> } >> >> static void dw_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) >> @@ -451,7 +494,7 @@ static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) >> { >> struct dw_i2c *i2c = dev_get_priv(bus); >> >> - return __dw_i2c_set_bus_speed(i2c->regs, speed); >> + return __dw_i2c_set_bus_speed(i2c->regs, i2c->scl_sda_cfg, speed); >> } >> >> static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, >> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) >> { >> struct dw_i2c *priv = dev_get_priv(bus); >> >> +#ifdef CONFIG_X86 >> + /* Save base address from PCI BAR */ >> + priv->regs = (struct i2c_regs *) >> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); >> + /* Use BayTrail specific timing values */ >> + priv->scl_sda_cfg = &byt_config; >> +#else > > How about: > > if (device_is_on_pci_bus(dev)) { > do the PCI I2C stuff here; > } I've tried this but it generated compilation errors on socfpga, as the dm_pci_xxx functions are not available there. So it definitely needs some #ifdef here. I could go with your suggestion and use #if CONFIG_DM_PCI as well. > See driver/net/designware.c for example. > >> /* Save base address from device-tree */ >> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >> +#endif >> >> __dw_i2c_init(priv->regs, 0, 0); >> >> return 0; >> } >> >> +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); >> + } > > I believe the above should be wrapped by #ifdef CONFIG_DM_PCI #endif, > otherwise it won't build when DM_PCI is not enabled. It does build and work on socfpga without CONFIG_DM_PCI enabled. I've double-checked this. The only problem is the dm_pci_xxx() in designware_i2c_probe() as I mentioned above. Thanks, Stefan
Hi Stefan, On Mon, Mar 21, 2016 at 5:03 PM, Stefan Roese <sr@denx.de> wrote: > Hi Bin, > > On 21.03.2016 09:54, Bin Meng wrote: >> On Fri, Mar 18, 2016 at 3:54 PM, Stefan Roese <sr@denx.de> wrote: >>> This patch adds support for the PCI(e) based I2C cores. Which can be >>> found for example on the Intel Bay Trail SoC. It has 7 I2C controllers >>> implemented as PCI devices. >>> >>> This patch also adds the fixed values for the timing registers for >>> BayTrail which are taken from the Linux designware I2C driver. >>> >>> Signed-off-by: Stefan Roese <sr@denx.de> >>> Cc: Simon Glass <sjg@chromium.org> >>> Cc: Bin Meng <bmeng.cn@gmail.com> >>> Cc: Marek Vasut <marex@denx.de> >>> Cc: Heiko Schocher <hs@denx.de> >>> --- >>> drivers/i2c/designware_i2c.c | 111 +++++++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 101 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c >>> index 4e5340d..f7f2eba 100644 >>> --- a/drivers/i2c/designware_i2c.c >>> +++ b/drivers/i2c/designware_i2c.c >>> @@ -8,11 +8,32 @@ >>> #include <common.h> >>> #include <dm.h> >>> #include <i2c.h> >>> +#include <pci.h> >>> #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; >>> }; >>> >>> static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) >>> @@ -42,6 +63,7 @@ static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) >>> * Set the i2c speed. >>> */ >>> static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base, >>> + struct dw_scl_sda_cfg *scl_sda_cfg, >>> unsigned int speed) >>> { >>> unsigned int cntl; >>> @@ -61,34 +83,55 @@ 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_HS; >>> - hcnt = (IC_CLK * MIN_HS_SCL_HIGHTIME) / NANO_TO_MICRO; >>> + cntl |= IC_CON_SPD_SS; >>> + if (scl_sda_cfg) { >>> + hcnt = scl_sda_cfg->fs_hcnt; >>> + lcnt = scl_sda_cfg->fs_lcnt; >>> + } else { >>> + hcnt = (IC_CLK * MIN_HS_SCL_HIGHTIME) / NANO_TO_MICRO; >>> + lcnt = (IC_CLK * MIN_HS_SCL_LOWTIME) / NANO_TO_MICRO; >>> + } >>> writel(hcnt, &i2c_base->ic_hs_scl_hcnt); >>> - lcnt = (IC_CLK * MIN_HS_SCL_LOWTIME) / NANO_TO_MICRO; >>> writel(lcnt, &i2c_base->ic_hs_scl_lcnt); >>> break; >>> +#endif >>> >>> case IC_SPEED_MODE_STANDARD: >>> cntl |= IC_CON_SPD_SS; >>> - hcnt = (IC_CLK * MIN_SS_SCL_HIGHTIME) / NANO_TO_MICRO; >>> + if (scl_sda_cfg) { >>> + hcnt = scl_sda_cfg->ss_hcnt; >>> + lcnt = scl_sda_cfg->ss_lcnt; >>> + } else { >>> + hcnt = (IC_CLK * MIN_SS_SCL_HIGHTIME) / NANO_TO_MICRO; >>> + lcnt = (IC_CLK * MIN_SS_SCL_LOWTIME) / NANO_TO_MICRO; >>> + } >>> writel(hcnt, &i2c_base->ic_ss_scl_hcnt); >>> - lcnt = (IC_CLK * MIN_SS_SCL_LOWTIME) / NANO_TO_MICRO; >>> writel(lcnt, &i2c_base->ic_ss_scl_lcnt); >>> break; >>> >>> case IC_SPEED_MODE_FAST: >>> default: >>> cntl |= IC_CON_SPD_FS; >>> - hcnt = (IC_CLK * MIN_FS_SCL_HIGHTIME) / NANO_TO_MICRO; >>> + if (scl_sda_cfg) { >>> + hcnt = scl_sda_cfg->fs_hcnt; >>> + lcnt = scl_sda_cfg->fs_lcnt; >>> + } else { >>> + hcnt = (IC_CLK * MIN_FS_SCL_HIGHTIME) / NANO_TO_MICRO; >>> + lcnt = (IC_CLK * MIN_FS_SCL_LOWTIME) / NANO_TO_MICRO; >>> + } >>> writel(hcnt, &i2c_base->ic_fs_scl_hcnt); >>> - lcnt = (IC_CLK * MIN_FS_SCL_LOWTIME) / NANO_TO_MICRO; >>> writel(lcnt, &i2c_base->ic_fs_scl_lcnt); >>> break; >>> } >>> >>> writel(cntl, &i2c_base->ic_con); >>> >>> + /* Configure SDA Hold Time if required */ >>> + if (scl_sda_cfg) >>> + writel(scl_sda_cfg->sda_hold, &i2c_base->ic_sda_hold); >>> + >>> /* Enable back i2c now speed set */ >>> dw_i2c_enable(i2c_base, 1); >>> >>> @@ -315,7 +358,7 @@ static void __dw_i2c_init(struct i2c_regs *i2c_base, int speed, int slaveaddr) >>> writel(IC_TX_TL, &i2c_base->ic_tx_tl); >>> writel(IC_STOP_DET, &i2c_base->ic_intr_mask); >>> #ifndef CONFIG_DM_I2C >>> - __dw_i2c_set_bus_speed(i2c_base, speed); >>> + __dw_i2c_set_bus_speed(i2c_base, NULL, speed); >>> writel(slaveaddr, &i2c_base->ic_sar); >>> #endif >>> >>> @@ -356,7 +399,7 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap, >>> unsigned int speed) >>> { >>> adap->speed = speed; >>> - return __dw_i2c_set_bus_speed(i2c_get_base(adap), speed); >>> + return __dw_i2c_set_bus_speed(i2c_get_base(adap), NULL, speed); >>> } >>> >>> static void dw_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) >>> @@ -451,7 +494,7 @@ static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) >>> { >>> struct dw_i2c *i2c = dev_get_priv(bus); >>> >>> - return __dw_i2c_set_bus_speed(i2c->regs, speed); >>> + return __dw_i2c_set_bus_speed(i2c->regs, i2c->scl_sda_cfg, speed); >>> } >>> >>> static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, >>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) >>> { >>> struct dw_i2c *priv = dev_get_priv(bus); >>> >>> +#ifdef CONFIG_X86 >>> + /* Save base address from PCI BAR */ >>> + priv->regs = (struct i2c_regs *) >>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); >>> + /* Use BayTrail specific timing values */ >>> + priv->scl_sda_cfg = &byt_config; >>> +#else >> >> How about: >> >> if (device_is_on_pci_bus(dev)) { >> do the PCI I2C stuff here; >> } > > I've tried this but it generated compilation errors on socfpga, as the > dm_pci_xxx functions are not available there. So it definitely needs > some #ifdef here. I could go with your suggestion and use > #if CONFIG_DM_PCI as well. > >> See driver/net/designware.c for example. >> >>> /* Save base address from device-tree */ >>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>> +#endif >>> >>> __dw_i2c_init(priv->regs, 0, 0); >>> >>> return 0; >>> } >>> >>> +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); >>> + } >> >> I believe the above should be wrapped by #ifdef CONFIG_DM_PCI #endif, >> otherwise it won't build when DM_PCI is not enabled. > > It does build and work on socfpga without CONFIG_DM_PCI enabled. I've > double-checked this. The only problem is the dm_pci_xxx() in > designware_i2c_probe() as I mentioned above. > Great. So looks we may consolidate one usage for such both PCI and SoC devices, like the one used in drivers/net/designware.c? Regards, Bin
Hi Bin, On 21.03.2016 10:03, Stefan Roese wrote: <snip> >>> static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, >>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) >>> { >>> struct dw_i2c *priv = dev_get_priv(bus); >>> >>> +#ifdef CONFIG_X86 >>> + /* Save base address from PCI BAR */ >>> + priv->regs = (struct i2c_regs *) >>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); >>> + /* Use BayTrail specific timing values */ >>> + priv->scl_sda_cfg = &byt_config; >>> +#else >> >> How about: >> >> if (device_is_on_pci_bus(dev)) { >> do the PCI I2C stuff here; >> } > > I've tried this but it generated compilation errors on socfpga, as the > dm_pci_xxx functions are not available there. So it definitely needs > some #ifdef here. I could go with your suggestion and use > #if CONFIG_DM_PCI as well. > >> See driver/net/designware.c for example. >> >>> /* Save base address from device-tree */ >>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>> +#endif Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results in this ugly compilation warning: drivers/i2c/designware_i2c.c: In function ‘designware_i2c_probe’: drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] priv->regs = (struct i2c_regs *)dev_get_addr(bus); ^ This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So I'm wondering, how dev_get_addr() should get used on x86. Has it been used anywhere here at all? Should we perhaps go back to a 32bit phy_addr representation again? So that dev_get_addr() matches the (void *) size again? The other option would to just leave the code as in v1 so that dev_get_addr() is not referenced on x86 here. Thanks, Stefan
Hi Stefan, On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <sr@denx.de> wrote: > Hi Bin, > > On 21.03.2016 10:03, Stefan Roese wrote: > > <snip> > >>>> static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, >>>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) >>>> { >>>> struct dw_i2c *priv = dev_get_priv(bus); >>>> >>>> +#ifdef CONFIG_X86 >>>> + /* Save base address from PCI BAR */ >>>> + priv->regs = (struct i2c_regs *) >>>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); >>>> + /* Use BayTrail specific timing values */ >>>> + priv->scl_sda_cfg = &byt_config; >>>> +#else >>> >>> How about: >>> >>> if (device_is_on_pci_bus(dev)) { >>> do the PCI I2C stuff here; >>> } >> >> I've tried this but it generated compilation errors on socfpga, as the >> dm_pci_xxx functions are not available there. So it definitely needs >> some #ifdef here. I could go with your suggestion and use >> #if CONFIG_DM_PCI as well. >> >>> See driver/net/designware.c for example. >>> >>>> /* Save base address from device-tree */ >>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>> +#endif > > Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results > in this ugly compilation warning: > > drivers/i2c/designware_i2c.c: In function ‘designware_i2c_probe’: > drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > priv->regs = (struct i2c_regs *)dev_get_addr(bus); > ^ > > This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So > I'm wondering, how dev_get_addr() should get used on x86. Has it > been used anywhere here at all? Should we perhaps go back to > a 32bit phy_addr representation again? So that dev_get_addr() > matches the (void *) size again? > dev_get_addr() is being used on x86 drivers. See ns16550_serial_ofdata_to_platdata() for example. There is no build warning for the ns16550 driver. > The other option would to just leave the code as in v1 so that > dev_get_addr() is not referenced on x86 here. > Regards, Bin
Hi Bin, On 21.03.2016 13:43, Bin Meng wrote: > On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <sr@denx.de> wrote: >> Hi Bin, >> >> On 21.03.2016 10:03, Stefan Roese wrote: >> >> <snip> >> >>>>> static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, >>>>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) >>>>> { >>>>> struct dw_i2c *priv = dev_get_priv(bus); >>>>> >>>>> +#ifdef CONFIG_X86 >>>>> + /* Save base address from PCI BAR */ >>>>> + priv->regs = (struct i2c_regs *) >>>>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); >>>>> + /* Use BayTrail specific timing values */ >>>>> + priv->scl_sda_cfg = &byt_config; >>>>> +#else >>>> >>>> How about: >>>> >>>> if (device_is_on_pci_bus(dev)) { >>>> do the PCI I2C stuff here; >>>> } >>> >>> I've tried this but it generated compilation errors on socfpga, as the >>> dm_pci_xxx functions are not available there. So it definitely needs >>> some #ifdef here. I could go with your suggestion and use >>> #if CONFIG_DM_PCI as well. >>> >>>> See driver/net/designware.c for example. >>>> >>>>> /* Save base address from device-tree */ >>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>> +#endif >> >> Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results >> in this ugly compilation warning: >> >> drivers/i2c/designware_i2c.c: In function ‘designware_i2c_probe’: >> drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] >> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >> ^ >> >> This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So >> I'm wondering, how dev_get_addr() should get used on x86. Has it >> been used anywhere here at all? Should we perhaps go back to >> a 32bit phy_addr representation again? So that dev_get_addr() >> matches the (void *) size again? >> > > dev_get_addr() is being used on x86 drivers. See > ns16550_serial_ofdata_to_platdata() for example. There is no build > warning for the ns16550 driver. I see, thanks. Its used differently there though. Let me see, if I cook something up... Thanks, Stefan
Hi Bin, On 21.03.2016 13:43, Bin Meng wrote: > On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <sr@denx.de> wrote: >> Hi Bin, >> >> On 21.03.2016 10:03, Stefan Roese wrote: >> >> <snip> >> >>>>> static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, >>>>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) >>>>> { >>>>> struct dw_i2c *priv = dev_get_priv(bus); >>>>> >>>>> +#ifdef CONFIG_X86 >>>>> + /* Save base address from PCI BAR */ >>>>> + priv->regs = (struct i2c_regs *) >>>>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); >>>>> + /* Use BayTrail specific timing values */ >>>>> + priv->scl_sda_cfg = &byt_config; >>>>> +#else >>>> >>>> How about: >>>> >>>> if (device_is_on_pci_bus(dev)) { >>>> do the PCI I2C stuff here; >>>> } >>> >>> I've tried this but it generated compilation errors on socfpga, as the >>> dm_pci_xxx functions are not available there. So it definitely needs >>> some #ifdef here. I could go with your suggestion and use >>> #if CONFIG_DM_PCI as well. >>> >>>> See driver/net/designware.c for example. >>>> >>>>> /* Save base address from device-tree */ >>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>> +#endif >> >> Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results >> in this ugly compilation warning: >> >> drivers/i2c/designware_i2c.c: In function ‘designware_i2c_probe’: >> drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] >> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >> ^ >> >> This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So >> I'm wondering, how dev_get_addr() should get used on x86. Has it >> been used anywhere here at all? Should we perhaps go back to >> a 32bit phy_addr representation again? So that dev_get_addr() >> matches the (void *) size again? >> > > dev_get_addr() is being used on x86 drivers. See > ns16550_serial_ofdata_to_platdata() for example. There is no build > warning for the ns16550 driver. Looking closer, the warning does not occur here, since the registers are stored in a u32 variable "base". And assigning a 64bit value to a 32bit variable as in "plat->base = addr" in ns16550.c does not cause any warnings. Here in the I2C driver though, the base address is stored as a pointer (pointer size is 32 bit for x86). And this triggers this warning, even though its effectively the same assignment. I could cast to u32 but this would cause problems on 64 bit architectures using this driver (in the future). So I came up with this approach: /* * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning the * register base directly in dev_get_addr() results in this compilation warning: * warning: cast to pointer from integer of different size * * Using this macro POINTER_SIZE_CAST, allows us to cast the result of * dev_get_addr() into a 32bit value before casting it to the pointer * (struct i2c_regs *). */ #ifdef CONFIG_X86 #define POINTER_SIZE_CAST u32 #endif ... static int designware_i2c_probe(struct udevice *bus) { struct dw_i2c *priv = dev_get_priv(bus); 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 { /* Save base address from device-tree */ priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); } But I'm not 100% happy with this approach. So what are the alternatives: a) Don't compile the dev_get_addr() part for x86 similar to what I've done in v1 b) This approach with POINTER_SIZE_CAST Any preferences of other ideas? Side note: My general feeling is, that dev_get_addr() should be able to get cast into a pointer on all platforms. This is how it is used in many drivers, btw. Since this is not possible on x86, we might have a problem here. Simon might have some ideas on this as well... Thanks, Stefan
Hi Stefan, On Mon, Mar 21, 2016 at 10:04 PM, Stefan Roese <sr@denx.de> wrote: > Hi Bin, > > On 21.03.2016 13:43, Bin Meng wrote: >> On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <sr@denx.de> wrote: >>> Hi Bin, >>> >>> On 21.03.2016 10:03, Stefan Roese wrote: >>> >>> <snip> >>> >>>>>> static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, >>>>>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) >>>>>> { >>>>>> struct dw_i2c *priv = dev_get_priv(bus); >>>>>> >>>>>> +#ifdef CONFIG_X86 >>>>>> + /* Save base address from PCI BAR */ >>>>>> + priv->regs = (struct i2c_regs *) >>>>>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); >>>>>> + /* Use BayTrail specific timing values */ >>>>>> + priv->scl_sda_cfg = &byt_config; >>>>>> +#else >>>>> >>>>> How about: >>>>> >>>>> if (device_is_on_pci_bus(dev)) { >>>>> do the PCI I2C stuff here; >>>>> } >>>> >>>> I've tried this but it generated compilation errors on socfpga, as the >>>> dm_pci_xxx functions are not available there. So it definitely needs >>>> some #ifdef here. I could go with your suggestion and use >>>> #if CONFIG_DM_PCI as well. >>>> >>>>> See driver/net/designware.c for example. >>>>> >>>>>> /* Save base address from device-tree */ >>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>>> +#endif >>> >>> Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results >>> in this ugly compilation warning: >>> >>> drivers/i2c/designware_i2c.c: In function ‘designware_i2c_probe’: >>> drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] >>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>> ^ >>> >>> This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So >>> I'm wondering, how dev_get_addr() should get used on x86. Has it >>> been used anywhere here at all? Should we perhaps go back to >>> a 32bit phy_addr representation again? So that dev_get_addr() >>> matches the (void *) size again? >>> >> >> dev_get_addr() is being used on x86 drivers. See >> ns16550_serial_ofdata_to_platdata() for example. There is no build >> warning for the ns16550 driver. > > Looking closer, the warning does not occur here, since the registers > are stored in a u32 variable "base". And assigning a 64bit value to a > 32bit variable as in "plat->base = addr" in ns16550.c does not cause any > warnings. > > Here in the I2C driver though, the base address is stored as a pointer > (pointer size is 32 bit for x86). And this triggers this warning, even > though its effectively the same assignment. I could cast to u32 but this > would cause problems on 64 bit architectures using this driver (in the > future). So I came up with this approach: Thanks for digging out these. > > /* > * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning the > * register base directly in dev_get_addr() results in this compilation warning: > * warning: cast to pointer from integer of different size > * > * Using this macro POINTER_SIZE_CAST, allows us to cast the result of > * dev_get_addr() into a 32bit value before casting it to the pointer > * (struct i2c_regs *). > */ > #ifdef CONFIG_X86 > #define POINTER_SIZE_CAST u32 > #endif > > ... > > static int designware_i2c_probe(struct udevice *bus) > { > struct dw_i2c *priv = dev_get_priv(bus); > > 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 { > /* Save base address from device-tree */ > priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); > } > > But I'm not 100% happy with this approach. > Yes, it's annoying. > So what are the alternatives: > > a) Don't compile the dev_get_addr() part for x86 similar to what I've > done in v1 > > b) This approach with POINTER_SIZE_CAST > > Any preferences of other ideas? > > Side note: My general feeling is, that dev_get_addr() should be able to > get cast into a pointer on all platforms. This is how it is used in many > drivers, btw. Since this is not possible on x86, we might have a problem > here. Simon might have some ideas on this as well... > I would like to hear Simon's input. Simon? Regards, Bin
Hi Simon, as you seem to be back from vacation (?), we (Bin and myself) would like to hear your expert comment on a x86 issue I've discovered while porting the Designware I2C driver to x86. Please see below: On 28.03.2016 08:01, Bin Meng wrote: > Hi Stefan, > > On Mon, Mar 21, 2016 at 10:04 PM, Stefan Roese <sr@denx.de> wrote: >> Hi Bin, >> >> On 21.03.2016 13:43, Bin Meng wrote: >>> On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <sr@denx.de> wrote: >>>> Hi Bin, >>>> >>>> On 21.03.2016 10:03, Stefan Roese wrote: >>>> >>>> <snip> >>>> >>>>>>> static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, >>>>>>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) >>>>>>> { >>>>>>> struct dw_i2c *priv = dev_get_priv(bus); >>>>>>> >>>>>>> +#ifdef CONFIG_X86 >>>>>>> + /* Save base address from PCI BAR */ >>>>>>> + priv->regs = (struct i2c_regs *) >>>>>>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); >>>>>>> + /* Use BayTrail specific timing values */ >>>>>>> + priv->scl_sda_cfg = &byt_config; >>>>>>> +#else >>>>>> >>>>>> How about: >>>>>> >>>>>> if (device_is_on_pci_bus(dev)) { >>>>>> do the PCI I2C stuff here; >>>>>> } >>>>> >>>>> I've tried this but it generated compilation errors on socfpga, as the >>>>> dm_pci_xxx functions are not available there. So it definitely needs >>>>> some #ifdef here. I could go with your suggestion and use >>>>> #if CONFIG_DM_PCI as well. >>>>> >>>>>> See driver/net/designware.c for example. >>>>>> >>>>>>> /* Save base address from device-tree */ >>>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>>>> +#endif >>>> >>>> Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results >>>> in this ugly compilation warning: >>>> >>>> drivers/i2c/designware_i2c.c: In function ‘designware_i2c_probe’: >>>> drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] >>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>> ^ >>>> >>>> This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So >>>> I'm wondering, how dev_get_addr() should get used on x86. Has it >>>> been used anywhere here at all? Should we perhaps go back to >>>> a 32bit phy_addr representation again? So that dev_get_addr() >>>> matches the (void *) size again? >>>> >>> >>> dev_get_addr() is being used on x86 drivers. See >>> ns16550_serial_ofdata_to_platdata() for example. There is no build >>> warning for the ns16550 driver. >> >> Looking closer, the warning does not occur here, since the registers >> are stored in a u32 variable "base". And assigning a 64bit value to a >> 32bit variable as in "plat->base = addr" in ns16550.c does not cause any >> warnings. >> >> Here in the I2C driver though, the base address is stored as a pointer >> (pointer size is 32 bit for x86). And this triggers this warning, even >> though its effectively the same assignment. I could cast to u32 but this >> would cause problems on 64 bit architectures using this driver (in the >> future). So I came up with this approach: > > Thanks for digging out these. > >> >> /* >> * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning the >> * register base directly in dev_get_addr() results in this compilation warning: >> * warning: cast to pointer from integer of different size >> * >> * Using this macro POINTER_SIZE_CAST, allows us to cast the result of >> * dev_get_addr() into a 32bit value before casting it to the pointer >> * (struct i2c_regs *). >> */ >> #ifdef CONFIG_X86 >> #define POINTER_SIZE_CAST u32 >> #endif >> >> ... >> >> static int designware_i2c_probe(struct udevice *bus) >> { >> struct dw_i2c *priv = dev_get_priv(bus); >> >> 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 { >> /* Save base address from device-tree */ >> priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); >> } >> >> But I'm not 100% happy with this approach. >> > > Yes, it's annoying. > >> So what are the alternatives: >> >> a) Don't compile the dev_get_addr() part for x86 similar to what I've >> done in v1 >> >> b) This approach with POINTER_SIZE_CAST >> >> Any preferences of other ideas? >> >> Side note: My general feeling is, that dev_get_addr() should be able to >> get cast into a pointer on all platforms. This is how it is used in many >> drivers, btw. Since this is not possible on x86, we might have a problem >> here. Simon might have some ideas on this as well... >> > > I would like to hear Simon's input. Simon? Yes, Simon, what do you think? Please also see my v2 of this patch which uses (__UINTPTR_TYPE__) for the cast: https://patchwork.ozlabs.org/patch/601113/ Thanks, Stefan
Hi Simon, On 04.04.2016 16:53, Stefan Roese wrote: > Hi Simon, > > as you seem to be back from vacation (?), we (Bin and myself) would > like to hear your expert comment on a x86 issue I've discovered > while porting the Designware I2C driver to x86. Please see below: > > On 28.03.2016 08:01, Bin Meng wrote: >> Hi Stefan, >> >> On Mon, Mar 21, 2016 at 10:04 PM, Stefan Roese <sr@denx.de> wrote: >>> Hi Bin, >>> >>> On 21.03.2016 13:43, Bin Meng wrote: >>>> On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <sr@denx.de> wrote: >>>>> Hi Bin, >>>>> >>>>> On 21.03.2016 10:03, Stefan Roese wrote: >>>>> >>>>> <snip> >>>>> >>>>>>>> static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, >>>>>>>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) >>>>>>>> { >>>>>>>> struct dw_i2c *priv = dev_get_priv(bus); >>>>>>>> >>>>>>>> +#ifdef CONFIG_X86 >>>>>>>> + /* Save base address from PCI BAR */ >>>>>>>> + priv->regs = (struct i2c_regs *) >>>>>>>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); >>>>>>>> + /* Use BayTrail specific timing values */ >>>>>>>> + priv->scl_sda_cfg = &byt_config; >>>>>>>> +#else >>>>>>> >>>>>>> How about: >>>>>>> >>>>>>> if (device_is_on_pci_bus(dev)) { >>>>>>> do the PCI I2C stuff here; >>>>>>> } >>>>>> >>>>>> I've tried this but it generated compilation errors on socfpga, as the >>>>>> dm_pci_xxx functions are not available there. So it definitely needs >>>>>> some #ifdef here. I could go with your suggestion and use >>>>>> #if CONFIG_DM_PCI as well. >>>>>> >>>>>>> See driver/net/designware.c for example. >>>>>>> >>>>>>>> /* Save base address from device-tree */ >>>>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>>>>> +#endif >>>>> >>>>> Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results >>>>> in this ugly compilation warning: >>>>> >>>>> drivers/i2c/designware_i2c.c: In function ‘designware_i2c_probe’: >>>>> drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] >>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>> ^ >>>>> >>>>> This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So >>>>> I'm wondering, how dev_get_addr() should get used on x86. Has it >>>>> been used anywhere here at all? Should we perhaps go back to >>>>> a 32bit phy_addr representation again? So that dev_get_addr() >>>>> matches the (void *) size again? >>>>> >>>> >>>> dev_get_addr() is being used on x86 drivers. See >>>> ns16550_serial_ofdata_to_platdata() for example. There is no build >>>> warning for the ns16550 driver. >>> >>> Looking closer, the warning does not occur here, since the registers >>> are stored in a u32 variable "base". And assigning a 64bit value to a >>> 32bit variable as in "plat->base = addr" in ns16550.c does not cause any >>> warnings. >>> >>> Here in the I2C driver though, the base address is stored as a pointer >>> (pointer size is 32 bit for x86). And this triggers this warning, even >>> though its effectively the same assignment. I could cast to u32 but this >>> would cause problems on 64 bit architectures using this driver (in the >>> future). So I came up with this approach: >> >> Thanks for digging out these. >> >>> >>> /* >>> * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning the >>> * register base directly in dev_get_addr() results in this compilation warning: >>> * warning: cast to pointer from integer of different size >>> * >>> * Using this macro POINTER_SIZE_CAST, allows us to cast the result of >>> * dev_get_addr() into a 32bit value before casting it to the pointer >>> * (struct i2c_regs *). >>> */ >>> #ifdef CONFIG_X86 >>> #define POINTER_SIZE_CAST u32 >>> #endif >>> >>> ... >>> >>> static int designware_i2c_probe(struct udevice *bus) >>> { >>> struct dw_i2c *priv = dev_get_priv(bus); >>> >>> 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 { >>> /* Save base address from device-tree */ >>> priv->regs = (struct i2c_regs *)(POINTER_SIZE_CAST)dev_get_addr(bus); >>> } >>> >>> But I'm not 100% happy with this approach. >>> >> >> Yes, it's annoying. >> >>> So what are the alternatives: >>> >>> a) Don't compile the dev_get_addr() part for x86 similar to what I've >>> done in v1 >>> >>> b) This approach with POINTER_SIZE_CAST >>> >>> Any preferences of other ideas? >>> >>> Side note: My general feeling is, that dev_get_addr() should be able to >>> get cast into a pointer on all platforms. This is how it is used in many >>> drivers, btw. Since this is not possible on x86, we might have a problem >>> here. Simon might have some ideas on this as well... >>> >> >> I would like to hear Simon's input. Simon? > > Yes, Simon, what do you think? > > Please also see my v2 of this patch which uses (__UINTPTR_TYPE__) > for the cast: > > https://patchwork.ozlabs.org/patch/601113/ Simon, could you please take a quick look at this patch? With the general problem of dev_get_addr() on x86 (as described above). Do you have some other suggestions to solve this? Or is the solution in v2 which uses (__UINTPTR_TYPE__) acceptable? https://patchwork.ozlabs.org/patch/601113/ Thanks, Stefan
Hi Stefan, On 11 April 2016 at 09:03, Stefan Roese <sr@denx.de> wrote: > Hi Simon, > > > On 04.04.2016 16:53, Stefan Roese wrote: >> >> Hi Simon, >> >> as you seem to be back from vacation (?), we (Bin and myself) would >> like to hear your expert comment on a x86 issue I've discovered >> while porting the Designware I2C driver to x86. Please see below: >> >> On 28.03.2016 08:01, Bin Meng wrote: >>> >>> Hi Stefan, >>> >>> On Mon, Mar 21, 2016 at 10:04 PM, Stefan Roese <sr@denx.de> wrote: >>>> >>>> Hi Bin, >>>> >>>> On 21.03.2016 13:43, Bin Meng wrote: >>>>> >>>>> On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <sr@denx.de> wrote: >>>>>> >>>>>> Hi Bin, >>>>>> >>>>>> On 21.03.2016 10:03, Stefan Roese wrote: >>>>>> >>>>>> <snip> >>>>>> >>>>>>>>> static int designware_i2c_probe_chip(struct udevice *bus, >>>>>>>>> uint chip_addr, >>>>>>>>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct >>>>>>>>> udevice *bus) >>>>>>>>> { >>>>>>>>> struct dw_i2c *priv = dev_get_priv(bus); >>>>>>>>> >>>>>>>>> +#ifdef CONFIG_X86 >>>>>>>>> + /* Save base address from PCI BAR */ >>>>>>>>> + priv->regs = (struct i2c_regs *) >>>>>>>>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, >>>>>>>>> PCI_REGION_MEM); >>>>>>>>> + /* Use BayTrail specific timing values */ >>>>>>>>> + priv->scl_sda_cfg = &byt_config; >>>>>>>>> +#else >>>>>>>> >>>>>>>> >>>>>>>> How about: >>>>>>>> >>>>>>>> if (device_is_on_pci_bus(dev)) { >>>>>>>> do the PCI I2C stuff here; >>>>>>>> } >>>>>>> >>>>>>> >>>>>>> I've tried this but it generated compilation errors on socfpga, as >>>>>>> the >>>>>>> dm_pci_xxx functions are not available there. So it definitely needs >>>>>>> some #ifdef here. I could go with your suggestion and use >>>>>>> #if CONFIG_DM_PCI as well. >>>>>>> >>>>>>>> See driver/net/designware.c for example. >>>>>>>> >>>>>>>>> /* Save base address from device-tree */ >>>>>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>>>>>> +#endif >>>>>> >>>>>> >>>>>> Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results >>>>>> in this ugly compilation warning: >>>>>> >>>>>> drivers/i2c/designware_i2c.c: In function ‘designware_i2c_probe’: >>>>>> drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from >>>>>> integer of different size [-Wint-to-pointer-cast] >>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>>> ^ >>>>>> >>>>>> This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So >>>>>> I'm wondering, how dev_get_addr() should get used on x86. Has it >>>>>> been used anywhere here at all? Should we perhaps go back to >>>>>> a 32bit phy_addr representation again? So that dev_get_addr() >>>>>> matches the (void *) size again? >>>>>> >>>>> >>>>> dev_get_addr() is being used on x86 drivers. See >>>>> ns16550_serial_ofdata_to_platdata() for example. There is no build >>>>> warning for the ns16550 driver. >>>> >>>> >>>> Looking closer, the warning does not occur here, since the registers >>>> are stored in a u32 variable "base". And assigning a 64bit value to a >>>> 32bit variable as in "plat->base = addr" in ns16550.c does not cause any >>>> warnings. >>>> >>>> Here in the I2C driver though, the base address is stored as a pointer >>>> (pointer size is 32 bit for x86). And this triggers this warning, even >>>> though its effectively the same assignment. I could cast to u32 but this >>>> would cause problems on 64 bit architectures using this driver (in the >>>> future). So I came up with this approach: >>> >>> >>> Thanks for digging out these. >>> >>>> >>>> /* >>>> * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning >>>> the >>>> * register base directly in dev_get_addr() results in this >>>> compilation warning: >>>> * warning: cast to pointer from integer of different size >>>> * >>>> * Using this macro POINTER_SIZE_CAST, allows us to cast the result of >>>> * dev_get_addr() into a 32bit value before casting it to the pointer >>>> * (struct i2c_regs *). >>>> */ >>>> #ifdef CONFIG_X86 >>>> #define POINTER_SIZE_CAST u32 >>>> #endif >>>> >>>> ... >>>> >>>> static int designware_i2c_probe(struct udevice *bus) >>>> { >>>> struct dw_i2c *priv = dev_get_priv(bus); >>>> >>>> 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 { >>>> /* Save base address from device-tree */ >>>> priv->regs = (struct i2c_regs >>>> *)(POINTER_SIZE_CAST)dev_get_addr(bus); >>>> } >>>> >>>> But I'm not 100% happy with this approach. >>>> >>> >>> Yes, it's annoying. >>> >>>> So what are the alternatives: >>>> >>>> a) Don't compile the dev_get_addr() part for x86 similar to what I've >>>> done in v1 >>>> >>>> b) This approach with POINTER_SIZE_CAST >>>> >>>> Any preferences of other ideas? >>>> >>>> Side note: My general feeling is, that dev_get_addr() should be able to >>>> get cast into a pointer on all platforms. This is how it is used in many >>>> drivers, btw. Since this is not possible on x86, we might have a problem >>>> here. Simon might have some ideas on this as well... >>>> >>> >>> I would like to hear Simon's input. Simon? >> >> >> Yes, Simon, what do you think? >> >> Please also see my v2 of this patch which uses (__UINTPTR_TYPE__) >> for the cast: >> >> https://patchwork.ozlabs.org/patch/601113/ > > > Simon, could you please take a quick look at this patch? With the > general problem of dev_get_addr() on x86 (as described above). Do you > have some other suggestions to solve this? Or is the solution in > v2 which uses (__UINTPTR_TYPE__) acceptable? > > https://patchwork.ozlabs.org/patch/601113/ I feel that you should store the return value from dev_get_addr() in an fdt_addr_t or a ulong. Then it can be cast to a pointer as you wish. Platform data should hold the ulong, and private data (dev_get_priv()) should hold the pointer. I'm not keen on the POINTER_SIZE_CAST idea. Does that fix the problem? Regards, Simon
Hi Simon. On 20.04.2016 16:40, Simon Glass wrote: > On 11 April 2016 at 09:03, Stefan Roese <sr@denx.de> wrote: >> Hi Simon, >> >> >> On 04.04.2016 16:53, Stefan Roese wrote: >>> >>> Hi Simon, >>> >>> as you seem to be back from vacation (?), we (Bin and myself) would >>> like to hear your expert comment on a x86 issue I've discovered >>> while porting the Designware I2C driver to x86. Please see below: >>> >>> On 28.03.2016 08:01, Bin Meng wrote: >>>> >>>> Hi Stefan, >>>> >>>> On Mon, Mar 21, 2016 at 10:04 PM, Stefan Roese <sr@denx.de> wrote: >>>>> >>>>> Hi Bin, >>>>> >>>>> On 21.03.2016 13:43, Bin Meng wrote: >>>>>> >>>>>> On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <sr@denx.de> wrote: >>>>>>> >>>>>>> Hi Bin, >>>>>>> >>>>>>> On 21.03.2016 10:03, Stefan Roese wrote: >>>>>>> >>>>>>> <snip> >>>>>>> >>>>>>>>>> static int designware_i2c_probe_chip(struct udevice *bus, >>>>>>>>>> uint chip_addr, >>>>>>>>>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct >>>>>>>>>> udevice *bus) >>>>>>>>>> { >>>>>>>>>> struct dw_i2c *priv = dev_get_priv(bus); >>>>>>>>>> >>>>>>>>>> +#ifdef CONFIG_X86 >>>>>>>>>> + /* Save base address from PCI BAR */ >>>>>>>>>> + priv->regs = (struct i2c_regs *) >>>>>>>>>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, >>>>>>>>>> PCI_REGION_MEM); >>>>>>>>>> + /* Use BayTrail specific timing values */ >>>>>>>>>> + priv->scl_sda_cfg = &byt_config; >>>>>>>>>> +#else >>>>>>>>> >>>>>>>>> >>>>>>>>> How about: >>>>>>>>> >>>>>>>>> if (device_is_on_pci_bus(dev)) { >>>>>>>>> do the PCI I2C stuff here; >>>>>>>>> } >>>>>>>> >>>>>>>> >>>>>>>> I've tried this but it generated compilation errors on socfpga, as >>>>>>>> the >>>>>>>> dm_pci_xxx functions are not available there. So it definitely needs >>>>>>>> some #ifdef here. I could go with your suggestion and use >>>>>>>> #if CONFIG_DM_PCI as well. >>>>>>>> >>>>>>>>> See driver/net/designware.c for example. >>>>>>>>> >>>>>>>>>> /* Save base address from device-tree */ >>>>>>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>>>>>>> +#endif >>>>>>> >>>>>>> >>>>>>> Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results >>>>>>> in this ugly compilation warning: >>>>>>> >>>>>>> drivers/i2c/designware_i2c.c: In function ‘designware_i2c_probe’: >>>>>>> drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from >>>>>>> integer of different size [-Wint-to-pointer-cast] >>>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>>>> ^ >>>>>>> >>>>>>> This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So >>>>>>> I'm wondering, how dev_get_addr() should get used on x86. Has it >>>>>>> been used anywhere here at all? Should we perhaps go back to >>>>>>> a 32bit phy_addr representation again? So that dev_get_addr() >>>>>>> matches the (void *) size again? >>>>>>> >>>>>> >>>>>> dev_get_addr() is being used on x86 drivers. See >>>>>> ns16550_serial_ofdata_to_platdata() for example. There is no build >>>>>> warning for the ns16550 driver. >>>>> >>>>> >>>>> Looking closer, the warning does not occur here, since the registers >>>>> are stored in a u32 variable "base". And assigning a 64bit value to a >>>>> 32bit variable as in "plat->base = addr" in ns16550.c does not cause any >>>>> warnings. >>>>> >>>>> Here in the I2C driver though, the base address is stored as a pointer >>>>> (pointer size is 32 bit for x86). And this triggers this warning, even >>>>> though its effectively the same assignment. I could cast to u32 but this >>>>> would cause problems on 64 bit architectures using this driver (in the >>>>> future). So I came up with this approach: >>>> >>>> >>>> Thanks for digging out these. >>>> >>>>> >>>>> /* >>>>> * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning >>>>> the >>>>> * register base directly in dev_get_addr() results in this >>>>> compilation warning: >>>>> * warning: cast to pointer from integer of different size >>>>> * >>>>> * Using this macro POINTER_SIZE_CAST, allows us to cast the result of >>>>> * dev_get_addr() into a 32bit value before casting it to the pointer >>>>> * (struct i2c_regs *). >>>>> */ >>>>> #ifdef CONFIG_X86 >>>>> #define POINTER_SIZE_CAST u32 >>>>> #endif >>>>> >>>>> ... >>>>> >>>>> static int designware_i2c_probe(struct udevice *bus) >>>>> { >>>>> struct dw_i2c *priv = dev_get_priv(bus); >>>>> >>>>> 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 { >>>>> /* Save base address from device-tree */ >>>>> priv->regs = (struct i2c_regs >>>>> *)(POINTER_SIZE_CAST)dev_get_addr(bus); >>>>> } >>>>> >>>>> But I'm not 100% happy with this approach. >>>>> >>>> >>>> Yes, it's annoying. >>>> >>>>> So what are the alternatives: >>>>> >>>>> a) Don't compile the dev_get_addr() part for x86 similar to what I've >>>>> done in v1 >>>>> >>>>> b) This approach with POINTER_SIZE_CAST >>>>> >>>>> Any preferences of other ideas? >>>>> >>>>> Side note: My general feeling is, that dev_get_addr() should be able to >>>>> get cast into a pointer on all platforms. This is how it is used in many >>>>> drivers, btw. Since this is not possible on x86, we might have a problem >>>>> here. Simon might have some ideas on this as well... >>>>> >>>> >>>> I would like to hear Simon's input. Simon? >>> >>> >>> Yes, Simon, what do you think? >>> >>> Please also see my v2 of this patch which uses (__UINTPTR_TYPE__) >>> for the cast: >>> >>> https://patchwork.ozlabs.org/patch/601113/ >> >> >> Simon, could you please take a quick look at this patch? With the >> general problem of dev_get_addr() on x86 (as described above). Do you >> have some other suggestions to solve this? Or is the solution in >> v2 which uses (__UINTPTR_TYPE__) acceptable? >> >> https://patchwork.ozlabs.org/patch/601113/ > > I feel that you should store the return value from dev_get_addr() in > an fdt_addr_t or a ulong. Then it can be cast to a pointer as you > wish. Platform data should hold the ulong, and private data > (dev_get_priv()) should hold the pointer. > > I'm not keen on the POINTER_SIZE_CAST idea. > > Does that fix the problem? Yes, it does. In a somewhat less ugly way. This is my current result: } else { ulong base; /* Save base address from device-tree */ /* * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. * So assigning the register base directly in dev_get_addr() * results in this compilation warning: * warning: cast to pointer from integer of different size * * Using an intermediate "ulong" variable before assigning * this pointer to the "regs" variable solves this issue. */ base = dev_get_addr(bus); priv->regs = (struct i2c_regs *)base; } If you think this is acceptable, I'll send a new patch version to the list. Thanks, Stefan
Hi Stefan, On 20 April 2016 at 08:58, Stefan Roese <sr@denx.de> wrote: > > Hi Simon. > > On 20.04.2016 16:40, Simon Glass wrote: > > > On 11 April 2016 at 09:03, Stefan Roese <sr@denx.de> wrote: > >> Hi Simon, > >> > >> > >> On 04.04.2016 16:53, Stefan Roese wrote: > >>> > >>> Hi Simon, > >>> > >>> as you seem to be back from vacation (?), we (Bin and myself) would > >>> like to hear your expert comment on a x86 issue I've discovered > >>> while porting the Designware I2C driver to x86. Please see below: > >>> > >>> On 28.03.2016 08:01, Bin Meng wrote: > >>>> > >>>> Hi Stefan, > >>>> > >>>> On Mon, Mar 21, 2016 at 10:04 PM, Stefan Roese <sr@denx.de> wrote: > >>>>> > >>>>> Hi Bin, > >>>>> > >>>>> On 21.03.2016 13:43, Bin Meng wrote: > >>>>>> > >>>>>> On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <sr@denx.de> wrote: > >>>>>>> > >>>>>>> Hi Bin, > >>>>>>> > >>>>>>> On 21.03.2016 10:03, Stefan Roese wrote: > >>>>>>> > >>>>>>> <snip> > >>>>>>> > >>>>>>>>>> static int designware_i2c_probe_chip(struct udevice *bus, > >>>>>>>>>> uint chip_addr, > >>>>>>>>>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct > >>>>>>>>>> udevice *bus) > >>>>>>>>>> { > >>>>>>>>>> struct dw_i2c *priv = dev_get_priv(bus); > >>>>>>>>>> > >>>>>>>>>> +#ifdef CONFIG_X86 > >>>>>>>>>> + /* Save base address from PCI BAR */ > >>>>>>>>>> + priv->regs = (struct i2c_regs *) > >>>>>>>>>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, > >>>>>>>>>> PCI_REGION_MEM); > >>>>>>>>>> + /* Use BayTrail specific timing values */ > >>>>>>>>>> + priv->scl_sda_cfg = &byt_config; > >>>>>>>>>> +#else > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> How about: > >>>>>>>>> > >>>>>>>>> if (device_is_on_pci_bus(dev)) { > >>>>>>>>> do the PCI I2C stuff here; > >>>>>>>>> } > >>>>>>>> > >>>>>>>> > >>>>>>>> I've tried this but it generated compilation errors on socfpga, as > >>>>>>>> the > >>>>>>>> dm_pci_xxx functions are not available there. So it definitely needs > >>>>>>>> some #ifdef here. I could go with your suggestion and use > >>>>>>>> #if CONFIG_DM_PCI as well. > >>>>>>>> > >>>>>>>>> See driver/net/designware.c for example. > >>>>>>>>> > >>>>>>>>>> /* Save base address from device-tree */ > >>>>>>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); > >>>>>>>>>> +#endif > >>>>>>> > >>>>>>> > >>>>>>> Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results > >>>>>>> in this ugly compilation warning: > >>>>>>> > >>>>>>> drivers/i2c/designware_i2c.c: In function ‘designware_i2c_probe’: > >>>>>>> drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from > >>>>>>> integer of different size [-Wint-to-pointer-cast] > >>>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); > >>>>>>> ^ > >>>>>>> > >>>>>>> This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So > >>>>>>> I'm wondering, how dev_get_addr() should get used on x86. Has it > >>>>>>> been used anywhere here at all? Should we perhaps go back to > >>>>>>> a 32bit phy_addr representation again? So that dev_get_addr() > >>>>>>> matches the (void *) size again? > >>>>>>> > >>>>>> > >>>>>> dev_get_addr() is being used on x86 drivers. See > >>>>>> ns16550_serial_ofdata_to_platdata() for example. There is no build > >>>>>> warning for the ns16550 driver. > >>>>> > >>>>> > >>>>> Looking closer, the warning does not occur here, since the registers > >>>>> are stored in a u32 variable "base". And assigning a 64bit value to a > >>>>> 32bit variable as in "plat->base = addr" in ns16550.c does not cause any > >>>>> warnings. > >>>>> > >>>>> Here in the I2C driver though, the base address is stored as a pointer > >>>>> (pointer size is 32 bit for x86). And this triggers this warning, even > >>>>> though its effectively the same assignment. I could cast to u32 but this > >>>>> would cause problems on 64 bit architectures using this driver (in the > >>>>> future). So I came up with this approach: > >>>> > >>>> > >>>> Thanks for digging out these. > >>>> > >>>>> > >>>>> /* > >>>>> * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning > >>>>> the > >>>>> * register base directly in dev_get_addr() results in this > >>>>> compilation warning: > >>>>> * warning: cast to pointer from integer of different size > >>>>> * > >>>>> * Using this macro POINTER_SIZE_CAST, allows us to cast the result of > >>>>> * dev_get_addr() into a 32bit value before casting it to the pointer > >>>>> * (struct i2c_regs *). > >>>>> */ > >>>>> #ifdef CONFIG_X86 > >>>>> #define POINTER_SIZE_CAST u32 > >>>>> #endif > >>>>> > >>>>> ... > >>>>> > >>>>> static int designware_i2c_probe(struct udevice *bus) > >>>>> { > >>>>> struct dw_i2c *priv = dev_get_priv(bus); > >>>>> > >>>>> 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 { > >>>>> /* Save base address from device-tree */ > >>>>> priv->regs = (struct i2c_regs > >>>>> *)(POINTER_SIZE_CAST)dev_get_addr(bus); > >>>>> } > >>>>> > >>>>> But I'm not 100% happy with this approach. > >>>>> > >>>> > >>>> Yes, it's annoying. > >>>> > >>>>> So what are the alternatives: > >>>>> > >>>>> a) Don't compile the dev_get_addr() part for x86 similar to what I've > >>>>> done in v1 > >>>>> > >>>>> b) This approach with POINTER_SIZE_CAST > >>>>> > >>>>> Any preferences of other ideas? > >>>>> > >>>>> Side note: My general feeling is, that dev_get_addr() should be able to > >>>>> get cast into a pointer on all platforms. This is how it is used in many > >>>>> drivers, btw. Since this is not possible on x86, we might have a problem > >>>>> here. Simon might have some ideas on this as well... > >>>>> > >>>> > >>>> I would like to hear Simon's input. Simon? > >>> > >>> > >>> Yes, Simon, what do you think? > >>> > >>> Please also see my v2 of this patch which uses (__UINTPTR_TYPE__) > >>> for the cast: > >>> > >>> https://patchwork.ozlabs.org/patch/601113/ > >> > >> > >> Simon, could you please take a quick look at this patch? With the > >> general problem of dev_get_addr() on x86 (as described above). Do you > >> have some other suggestions to solve this? Or is the solution in > >> v2 which uses (__UINTPTR_TYPE__) acceptable? > >> > >> https://patchwork.ozlabs.org/patch/601113/ > > > > I feel that you should store the return value from dev_get_addr() in > > an fdt_addr_t or a ulong. Then it can be cast to a pointer as you > > wish. Platform data should hold the ulong, and private data > > (dev_get_priv()) should hold the pointer. > > > > I'm not keen on the POINTER_SIZE_CAST idea. > > > > Does that fix the problem? > > Yes, it does. In a somewhat less ugly way. This is my current result: > > } else { > ulong base; > > /* Save base address from device-tree */ > > /* > * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. > * So assigning the register base directly in dev_get_addr() > * results in this compilation warning: > * warning: cast to pointer from integer of different size > * > * Using an intermediate "ulong" variable before assigning > * this pointer to the "regs" variable solves this issue. > */ > base = dev_get_addr(bus); > priv->regs = (struct i2c_regs *)base; > } > > If you think this is acceptable, I'll send a new patch version to > the list. Seems fine to me. Perhaps we should have dev_get_addr_ptr() to do this for us? Regards, Simon
Hi Simon, On 20.04.2016 17:09, Simon Glass wrote: > Hi Stefan, > > On 20 April 2016 at 08:58, Stefan Roese <sr@denx.de> wrote: >> >> Hi Simon. >> >> On 20.04.2016 16:40, Simon Glass wrote: >> >>> On 11 April 2016 at 09:03, Stefan Roese <sr@denx.de> wrote: >>>> Hi Simon, >>>> >>>> >>>> On 04.04.2016 16:53, Stefan Roese wrote: >>>>> >>>>> Hi Simon, >>>>> >>>>> as you seem to be back from vacation (?), we (Bin and myself) would >>>>> like to hear your expert comment on a x86 issue I've discovered >>>>> while porting the Designware I2C driver to x86. Please see below: >>>>> >>>>> On 28.03.2016 08:01, Bin Meng wrote: >>>>>> >>>>>> Hi Stefan, >>>>>> >>>>>> On Mon, Mar 21, 2016 at 10:04 PM, Stefan Roese <sr@denx.de> wrote: >>>>>>> >>>>>>> Hi Bin, >>>>>>> >>>>>>> On 21.03.2016 13:43, Bin Meng wrote: >>>>>>>> >>>>>>>> On Mon, Mar 21, 2016 at 8:04 PM, Stefan Roese <sr@denx.de> wrote: >>>>>>>>> >>>>>>>>> Hi Bin, >>>>>>>>> >>>>>>>>> On 21.03.2016 10:03, Stefan Roese wrote: >>>>>>>>> >>>>>>>>> <snip> >>>>>>>>> >>>>>>>>>>>> static int designware_i2c_probe_chip(struct udevice *bus, >>>>>>>>>>>> uint chip_addr, >>>>>>>>>>>> @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct >>>>>>>>>>>> udevice *bus) >>>>>>>>>>>> { >>>>>>>>>>>> struct dw_i2c *priv = dev_get_priv(bus); >>>>>>>>>>>> >>>>>>>>>>>> +#ifdef CONFIG_X86 >>>>>>>>>>>> + /* Save base address from PCI BAR */ >>>>>>>>>>>> + priv->regs = (struct i2c_regs *) >>>>>>>>>>>> + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, >>>>>>>>>>>> PCI_REGION_MEM); >>>>>>>>>>>> + /* Use BayTrail specific timing values */ >>>>>>>>>>>> + priv->scl_sda_cfg = &byt_config; >>>>>>>>>>>> +#else >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> How about: >>>>>>>>>>> >>>>>>>>>>> if (device_is_on_pci_bus(dev)) { >>>>>>>>>>> do the PCI I2C stuff here; >>>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> I've tried this but it generated compilation errors on socfpga, as >>>>>>>>>> the >>>>>>>>>> dm_pci_xxx functions are not available there. So it definitely needs >>>>>>>>>> some #ifdef here. I could go with your suggestion and use >>>>>>>>>> #if CONFIG_DM_PCI as well. >>>>>>>>>> >>>>>>>>>>> See driver/net/designware.c for example. >>>>>>>>>>> >>>>>>>>>>>> /* Save base address from device-tree */ >>>>>>>>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>>>>>>>>> +#endif >>>>>>>>> >>>>>>>>> >>>>>>>>> Enabling this code for x86 via if (device_is_on_pci_bus(dev)) results >>>>>>>>> in this ugly compilation warning: >>>>>>>>> >>>>>>>>> drivers/i2c/designware_i2c.c: In function ‘designware_i2c_probe’: >>>>>>>>> drivers/i2c/designware_i2c.c:530:16: warning: cast to pointer from >>>>>>>>> integer of different size [-Wint-to-pointer-cast] >>>>>>>>> priv->regs = (struct i2c_regs *)dev_get_addr(bus); >>>>>>>>> ^ >>>>>>>>> >>>>>>>>> This is because x86 defines fdt_addr_t / phys_addr_t as 64bit. So >>>>>>>>> I'm wondering, how dev_get_addr() should get used on x86. Has it >>>>>>>>> been used anywhere here at all? Should we perhaps go back to >>>>>>>>> a 32bit phy_addr representation again? So that dev_get_addr() >>>>>>>>> matches the (void *) size again? >>>>>>>>> >>>>>>>> >>>>>>>> dev_get_addr() is being used on x86 drivers. See >>>>>>>> ns16550_serial_ofdata_to_platdata() for example. There is no build >>>>>>>> warning for the ns16550 driver. >>>>>>> >>>>>>> >>>>>>> Looking closer, the warning does not occur here, since the registers >>>>>>> are stored in a u32 variable "base". And assigning a 64bit value to a >>>>>>> 32bit variable as in "plat->base = addr" in ns16550.c does not cause any >>>>>>> warnings. >>>>>>> >>>>>>> Here in the I2C driver though, the base address is stored as a pointer >>>>>>> (pointer size is 32 bit for x86). And this triggers this warning, even >>>>>>> though its effectively the same assignment. I could cast to u32 but this >>>>>>> would cause problems on 64 bit architectures using this driver (in the >>>>>>> future). So I came up with this approach: >>>>>> >>>>>> >>>>>> Thanks for digging out these. >>>>>> >>>>>>> >>>>>>> /* >>>>>>> * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. So assigning >>>>>>> the >>>>>>> * register base directly in dev_get_addr() results in this >>>>>>> compilation warning: >>>>>>> * warning: cast to pointer from integer of different size >>>>>>> * >>>>>>> * Using this macro POINTER_SIZE_CAST, allows us to cast the result of >>>>>>> * dev_get_addr() into a 32bit value before casting it to the pointer >>>>>>> * (struct i2c_regs *). >>>>>>> */ >>>>>>> #ifdef CONFIG_X86 >>>>>>> #define POINTER_SIZE_CAST u32 >>>>>>> #endif >>>>>>> >>>>>>> ... >>>>>>> >>>>>>> static int designware_i2c_probe(struct udevice *bus) >>>>>>> { >>>>>>> struct dw_i2c *priv = dev_get_priv(bus); >>>>>>> >>>>>>> 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 { >>>>>>> /* Save base address from device-tree */ >>>>>>> priv->regs = (struct i2c_regs >>>>>>> *)(POINTER_SIZE_CAST)dev_get_addr(bus); >>>>>>> } >>>>>>> >>>>>>> But I'm not 100% happy with this approach. >>>>>>> >>>>>> >>>>>> Yes, it's annoying. >>>>>> >>>>>>> So what are the alternatives: >>>>>>> >>>>>>> a) Don't compile the dev_get_addr() part for x86 similar to what I've >>>>>>> done in v1 >>>>>>> >>>>>>> b) This approach with POINTER_SIZE_CAST >>>>>>> >>>>>>> Any preferences of other ideas? >>>>>>> >>>>>>> Side note: My general feeling is, that dev_get_addr() should be able to >>>>>>> get cast into a pointer on all platforms. This is how it is used in many >>>>>>> drivers, btw. Since this is not possible on x86, we might have a problem >>>>>>> here. Simon might have some ideas on this as well... >>>>>>> >>>>>> >>>>>> I would like to hear Simon's input. Simon? >>>>> >>>>> >>>>> Yes, Simon, what do you think? >>>>> >>>>> Please also see my v2 of this patch which uses (__UINTPTR_TYPE__) >>>>> for the cast: >>>>> >>>>> https://patchwork.ozlabs.org/patch/601113/ >>>> >>>> >>>> Simon, could you please take a quick look at this patch? With the >>>> general problem of dev_get_addr() on x86 (as described above). Do you >>>> have some other suggestions to solve this? Or is the solution in >>>> v2 which uses (__UINTPTR_TYPE__) acceptable? >>>> >>>> https://patchwork.ozlabs.org/patch/601113/ >>> >>> I feel that you should store the return value from dev_get_addr() in >>> an fdt_addr_t or a ulong. Then it can be cast to a pointer as you >>> wish. Platform data should hold the ulong, and private data >>> (dev_get_priv()) should hold the pointer. >>> >>> I'm not keen on the POINTER_SIZE_CAST idea. >>> >>> Does that fix the problem? >> >> Yes, it does. In a somewhat less ugly way. This is my current result: >> >> } else { >> ulong base; >> >> /* Save base address from device-tree */ >> >> /* >> * On x86, "fdt_addr_t" is 64bit but "void *" only 32bit. >> * So assigning the register base directly in dev_get_addr() >> * results in this compilation warning: >> * warning: cast to pointer from integer of different size >> * >> * Using an intermediate "ulong" variable before assigning >> * this pointer to the "regs" variable solves this issue. >> */ >> base = dev_get_addr(bus); >> priv->regs = (struct i2c_regs *)base; >> } >> >> If you think this is acceptable, I'll send a new patch version to >> the list. > > Seems fine to me. Perhaps we should have dev_get_addr_ptr() to do > this for us? Might make sense. I can generate a small patch for this. Perhaps we should better use "uintptr_t" as type for the intermediate variable instead. But then we can effectively drop the intermediate variable and do the casting directly. What do you think? Thanks, Stefan
diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c index 4e5340d..f7f2eba 100644 --- a/drivers/i2c/designware_i2c.c +++ b/drivers/i2c/designware_i2c.c @@ -8,11 +8,32 @@ #include <common.h> #include <dm.h> #include <i2c.h> +#include <pci.h> #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; }; static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) @@ -42,6 +63,7 @@ static void dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) * Set the i2c speed. */ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base, + struct dw_scl_sda_cfg *scl_sda_cfg, unsigned int speed) { unsigned int cntl; @@ -61,34 +83,55 @@ 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_HS; - hcnt = (IC_CLK * MIN_HS_SCL_HIGHTIME) / NANO_TO_MICRO; + cntl |= IC_CON_SPD_SS; + if (scl_sda_cfg) { + hcnt = scl_sda_cfg->fs_hcnt; + lcnt = scl_sda_cfg->fs_lcnt; + } else { + hcnt = (IC_CLK * MIN_HS_SCL_HIGHTIME) / NANO_TO_MICRO; + lcnt = (IC_CLK * MIN_HS_SCL_LOWTIME) / NANO_TO_MICRO; + } writel(hcnt, &i2c_base->ic_hs_scl_hcnt); - lcnt = (IC_CLK * MIN_HS_SCL_LOWTIME) / NANO_TO_MICRO; writel(lcnt, &i2c_base->ic_hs_scl_lcnt); break; +#endif case IC_SPEED_MODE_STANDARD: cntl |= IC_CON_SPD_SS; - hcnt = (IC_CLK * MIN_SS_SCL_HIGHTIME) / NANO_TO_MICRO; + if (scl_sda_cfg) { + hcnt = scl_sda_cfg->ss_hcnt; + lcnt = scl_sda_cfg->ss_lcnt; + } else { + hcnt = (IC_CLK * MIN_SS_SCL_HIGHTIME) / NANO_TO_MICRO; + lcnt = (IC_CLK * MIN_SS_SCL_LOWTIME) / NANO_TO_MICRO; + } writel(hcnt, &i2c_base->ic_ss_scl_hcnt); - lcnt = (IC_CLK * MIN_SS_SCL_LOWTIME) / NANO_TO_MICRO; writel(lcnt, &i2c_base->ic_ss_scl_lcnt); break; case IC_SPEED_MODE_FAST: default: cntl |= IC_CON_SPD_FS; - hcnt = (IC_CLK * MIN_FS_SCL_HIGHTIME) / NANO_TO_MICRO; + if (scl_sda_cfg) { + hcnt = scl_sda_cfg->fs_hcnt; + lcnt = scl_sda_cfg->fs_lcnt; + } else { + hcnt = (IC_CLK * MIN_FS_SCL_HIGHTIME) / NANO_TO_MICRO; + lcnt = (IC_CLK * MIN_FS_SCL_LOWTIME) / NANO_TO_MICRO; + } writel(hcnt, &i2c_base->ic_fs_scl_hcnt); - lcnt = (IC_CLK * MIN_FS_SCL_LOWTIME) / NANO_TO_MICRO; writel(lcnt, &i2c_base->ic_fs_scl_lcnt); break; } writel(cntl, &i2c_base->ic_con); + /* Configure SDA Hold Time if required */ + if (scl_sda_cfg) + writel(scl_sda_cfg->sda_hold, &i2c_base->ic_sda_hold); + /* Enable back i2c now speed set */ dw_i2c_enable(i2c_base, 1); @@ -315,7 +358,7 @@ static void __dw_i2c_init(struct i2c_regs *i2c_base, int speed, int slaveaddr) writel(IC_TX_TL, &i2c_base->ic_tx_tl); writel(IC_STOP_DET, &i2c_base->ic_intr_mask); #ifndef CONFIG_DM_I2C - __dw_i2c_set_bus_speed(i2c_base, speed); + __dw_i2c_set_bus_speed(i2c_base, NULL, speed); writel(slaveaddr, &i2c_base->ic_sar); #endif @@ -356,7 +399,7 @@ static unsigned int dw_i2c_set_bus_speed(struct i2c_adapter *adap, unsigned int speed) { adap->speed = speed; - return __dw_i2c_set_bus_speed(i2c_get_base(adap), speed); + return __dw_i2c_set_bus_speed(i2c_get_base(adap), NULL, speed); } static void dw_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) @@ -451,7 +494,7 @@ static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) { struct dw_i2c *i2c = dev_get_priv(bus); - return __dw_i2c_set_bus_speed(i2c->regs, speed); + return __dw_i2c_set_bus_speed(i2c->regs, i2c->scl_sda_cfg, speed); } static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, @@ -476,14 +519,45 @@ static int designware_i2c_probe(struct udevice *bus) { struct dw_i2c *priv = dev_get_priv(bus); +#ifdef CONFIG_X86 + /* Save base address from PCI BAR */ + priv->regs = (struct i2c_regs *) + dm_pci_map_bar(bus, PCI_BASE_ADDRESS_0, PCI_REGION_MEM); + /* Use BayTrail specific timing values */ + priv->scl_sda_cfg = &byt_config; +#else /* Save base address from device-tree */ priv->regs = (struct i2c_regs *)dev_get_addr(bus); +#endif __dw_i2c_init(priv->regs, 0, 0); return 0; } +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 = { .xfer = designware_i2c_xfer, .probe_chip = designware_i2c_probe_chip, @@ -499,9 +573,26 @@ U_BOOT_DRIVER(i2c_designware) = { .name = "i2c_designware", .id = UCLASS_I2C, .of_match = designware_i2c_ids, + .bind = designware_i2c_bind, .probe = designware_i2c_probe, .priv_auto_alloc_size = sizeof(struct dw_i2c), .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 */
This patch adds support for the PCI(e) based I2C cores. Which can be found for example on the Intel Bay Trail SoC. It has 7 I2C controllers implemented as PCI devices. This patch also adds the fixed values for the timing registers for BayTrail which are taken from the Linux designware I2C driver. Signed-off-by: Stefan Roese <sr@denx.de> Cc: Simon Glass <sjg@chromium.org> Cc: Bin Meng <bmeng.cn@gmail.com> Cc: Marek Vasut <marex@denx.de> Cc: Heiko Schocher <hs@denx.de> --- drivers/i2c/designware_i2c.c | 111 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 10 deletions(-)