| Message ID | 1559205789-21822-1-git-send-email-ley.foon.tan@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Heiko Schocher |
| Headers | show |
| Series | [U-Boot] i2c: designware: Get clock rate from clock DM | expand |
On 5/30/19 10:43 AM, Ley Foon Tan wrote: > Get clock rate from clock DM if CONFIG_CLK is enabled. > Otherwise, uses IC_CLK define. > > Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com> > --- > drivers/i2c/designware_i2c.c | 54 +++++++++++++++++++++++++++++------- > 1 file changed, 44 insertions(+), 10 deletions(-) > > diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c > index 9ccc2411a6..a1ed30650c 100644 > --- a/drivers/i2c/designware_i2c.c > +++ b/drivers/i2c/designware_i2c.c > @@ -4,6 +4,7 @@ > * Vipin Kumar, ST Micoelectronics, vipin.kumar@st.com. > */ > > +#include <clk.h> > #include <common.h> > #include <dm.h> > #include <i2c.h> > @@ -35,6 +36,9 @@ 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 > @@ -78,7 +82,7 @@ static int dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) > */ > 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 speed, u32 bus_mhz) unsigned int bus_mhz , it's not a fixed-width register content, but just some random unsigned integer. [...] > @@ -523,8 +527,20 @@ static int designware_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, > static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) > { > struct dw_i2c *i2c = dev_get_priv(bus); > + ulong rate; > + > +#if CONFIG_IS_ENABLED(CLK) > + rate = clk_get_rate(&i2c->clk); Do we need to re-read the bus frequency for each transfer ? I would expect set_bus_speed callback to set the frequencies once and then keep them that way until it's called again. [...]
On Thu, May 30, 2019 at 4:59 PM Marek Vasut <marex@denx.de> wrote: > > On 5/30/19 10:43 AM, Ley Foon Tan wrote: > > Get clock rate from clock DM if CONFIG_CLK is enabled. > > Otherwise, uses IC_CLK define. > > > > Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com> > > --- > > drivers/i2c/designware_i2c.c | 54 +++++++++++++++++++++++++++++------- > > 1 file changed, 44 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c > > index 9ccc2411a6..a1ed30650c 100644 > > --- a/drivers/i2c/designware_i2c.c > > +++ b/drivers/i2c/designware_i2c.c > > @@ -4,6 +4,7 @@ > > * Vipin Kumar, ST Micoelectronics, vipin.kumar@st.com. > > */ > > > > +#include <clk.h> > > #include <common.h> > > #include <dm.h> > > #include <i2c.h> > > @@ -35,6 +36,9 @@ 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 > > @@ -78,7 +82,7 @@ static int dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) > > */ > > 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 speed, u32 bus_mhz) > > unsigned int bus_mhz , it's not a fixed-width register content, but just > some random unsigned integer. You mean change u32 to unsigned int? > > [...] > > > @@ -523,8 +527,20 @@ static int designware_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, > > static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) > > { > > struct dw_i2c *i2c = dev_get_priv(bus); > > + ulong rate; > > + > > +#if CONFIG_IS_ENABLED(CLK) > > + rate = clk_get_rate(&i2c->clk); > > Do we need to re-read the bus frequency for each transfer ? > I would expect set_bus_speed callback to set the frequencies once and > then keep them that way until it's called again. Yes, we can get clock rate when request clock in _probe(). Then keep a copy for future use. Will change it. Thanks. Regards Ley Foon
On 5/30/19 11:07 AM, Ley Foon Tan wrote: > On Thu, May 30, 2019 at 4:59 PM Marek Vasut <marex@denx.de> wrote: >> >> On 5/30/19 10:43 AM, Ley Foon Tan wrote: >>> Get clock rate from clock DM if CONFIG_CLK is enabled. >>> Otherwise, uses IC_CLK define. >>> >>> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com> >>> --- >>> drivers/i2c/designware_i2c.c | 54 +++++++++++++++++++++++++++++------- >>> 1 file changed, 44 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c >>> index 9ccc2411a6..a1ed30650c 100644 >>> --- a/drivers/i2c/designware_i2c.c >>> +++ b/drivers/i2c/designware_i2c.c >>> @@ -4,6 +4,7 @@ >>> * Vipin Kumar, ST Micoelectronics, vipin.kumar@st.com. >>> */ >>> >>> +#include <clk.h> >>> #include <common.h> >>> #include <dm.h> >>> #include <i2c.h> >>> @@ -35,6 +36,9 @@ 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 >>> @@ -78,7 +82,7 @@ static int dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) >>> */ >>> 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 speed, u32 bus_mhz) >> >> unsigned int bus_mhz , it's not a fixed-width register content, but just >> some random unsigned integer. > You mean change u32 to unsigned int? Yes >> [...] >> >>> @@ -523,8 +527,20 @@ static int designware_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, >>> static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) >>> { >>> struct dw_i2c *i2c = dev_get_priv(bus); >>> + ulong rate; >>> + >>> +#if CONFIG_IS_ENABLED(CLK) >>> + rate = clk_get_rate(&i2c->clk); >> >> Do we need to re-read the bus frequency for each transfer ? >> I would expect set_bus_speed callback to set the frequencies once and >> then keep them that way until it's called again. > Yes, we can get clock rate when request clock in _probe(). Then keep a > copy for future use. Not in .probe() , in set_bus_speed(). > Will change it. > > Thanks. > > Regards > Ley Foon >
On Thu, May 30, 2019 at 5:13 PM Marek Vasut <marex@denx.de> wrote: > > On 5/30/19 11:07 AM, Ley Foon Tan wrote: > > On Thu, May 30, 2019 at 4:59 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 5/30/19 10:43 AM, Ley Foon Tan wrote: > >>> Get clock rate from clock DM if CONFIG_CLK is enabled. > >>> Otherwise, uses IC_CLK define. > >>> > >>> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com> > >>> --- > >>> drivers/i2c/designware_i2c.c | 54 +++++++++++++++++++++++++++++------- > >>> 1 file changed, 44 insertions(+), 10 deletions(-) > >>> > >>> diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c > >>> index 9ccc2411a6..a1ed30650c 100644 > >>> --- a/drivers/i2c/designware_i2c.c > >>> +++ b/drivers/i2c/designware_i2c.c > >>> @@ -4,6 +4,7 @@ > >>> * Vipin Kumar, ST Micoelectronics, vipin.kumar@st.com. > >>> */ > >>> > >>> +#include <clk.h> > >>> #include <common.h> > >>> #include <dm.h> > >>> #include <i2c.h> > >>> @@ -35,6 +36,9 @@ 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 > >>> @@ -78,7 +82,7 @@ static int dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) > >>> */ > >>> 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 speed, u32 bus_mhz) > >> > >> unsigned int bus_mhz , it's not a fixed-width register content, but just > >> some random unsigned integer. > > You mean change u32 to unsigned int? > > Yes Okay. > > >> [...] > >> > >>> @@ -523,8 +527,20 @@ static int designware_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, > >>> static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) > >>> { > >>> struct dw_i2c *i2c = dev_get_priv(bus); > >>> + ulong rate; > >>> + > >>> +#if CONFIG_IS_ENABLED(CLK) > >>> + rate = clk_get_rate(&i2c->clk); > >> > >> Do we need to re-read the bus frequency for each transfer ? > >> I would expect set_bus_speed callback to set the frequencies once and > >> then keep them that way until it's called again. > > Yes, we can get clock rate when request clock in _probe(). Then keep a > > copy for future use. > > Not in .probe() , in set_bus_speed(). My patch is doing it in designware_i2c_set_bus_speed() already. We can't get clock rate in __dw_i2c_set_bus_speed(), because this function doesn't have struct udevice. Regards Ley Foon
On 6/10/19 8:07 AM, Ley Foon Tan wrote: [...] >>>> [...] >>>> >>>>> @@ -523,8 +527,20 @@ static int designware_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, >>>>> static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) >>>>> { >>>>> struct dw_i2c *i2c = dev_get_priv(bus); >>>>> + ulong rate; >>>>> + >>>>> +#if CONFIG_IS_ENABLED(CLK) >>>>> + rate = clk_get_rate(&i2c->clk); >>>> >>>> Do we need to re-read the bus frequency for each transfer ? >>>> I would expect set_bus_speed callback to set the frequencies once and >>>> then keep them that way until it's called again. >>> Yes, we can get clock rate when request clock in _probe(). Then keep a >>> copy for future use. >> >> Not in .probe() , in set_bus_speed(). > My patch is doing it in designware_i2c_set_bus_speed() already. We > can't get clock rate in __dw_i2c_set_bus_speed(), because this > function doesn't have struct udevice. include/i2c.h struct dm_i2c_ops {} : 388 /** 389 * set_bus_speed() - set the speed of a bus (optional) 390 * 391 * The bus speed value will be updated by the uclass if this function 392 * does not return an error. This method is optional - if it is not 393 * provided then the driver can read the speed from 394 * dev_get_uclass_priv(bus)->speed_hz 395 * 396 * @bus: Bus to adjust 397 * @speed: Requested speed in Hz 398 * @return 0 if OK, -EINVAL for invalid values 399 */ 400 int (*set_bus_speed)(struct udevice *bus, unsigned int speed); There's struct udevice right there ^
On Mon, Jun 10, 2019 at 8:07 PM Marek Vasut <marex@denx.de> wrote: > > On 6/10/19 8:07 AM, Ley Foon Tan wrote: > [...] > > >>>> [...] > >>>> > >>>>> @@ -523,8 +527,20 @@ static int designware_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, > >>>>> static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) > >>>>> { > >>>>> struct dw_i2c *i2c = dev_get_priv(bus); > >>>>> + ulong rate; > >>>>> + > >>>>> +#if CONFIG_IS_ENABLED(CLK) > >>>>> + rate = clk_get_rate(&i2c->clk); > >>>> > >>>> Do we need to re-read the bus frequency for each transfer ? > >>>> I would expect set_bus_speed callback to set the frequencies once and > >>>> then keep them that way until it's called again. > >>> Yes, we can get clock rate when request clock in _probe(). Then keep a > >>> copy for future use. > >> > >> Not in .probe() , in set_bus_speed(). > > My patch is doing it in designware_i2c_set_bus_speed() already. We > > can't get clock rate in __dw_i2c_set_bus_speed(), because this > > function doesn't have struct udevice. > > include/i2c.h struct dm_i2c_ops {} : > > 388 /** > 389 * set_bus_speed() - set the speed of a bus (optional) > 390 * > 391 * The bus speed value will be updated by the uclass if this > function > 392 * does not return an error. This method is optional - if it > is not > 393 * provided then the driver can read the speed from > 394 * dev_get_uclass_priv(bus)->speed_hz > 395 * > 396 * @bus: Bus to adjust > 397 * @speed: Requested speed in Hz > 398 * @return 0 if OK, -EINVAL for invalid values > 399 */ > 400 int (*set_bus_speed)(struct udevice *bus, unsigned int speed); > > There's struct udevice right there ^ I'm confused now. .set_bus_speed = designware_i2c_set_bus_speed and my patch is doing get clock rate in .set_bus_speed callback already. static 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, }; Regards Ley Foon
On 6/11/19 3:05 AM, Ley Foon Tan wrote: > On Mon, Jun 10, 2019 at 8:07 PM Marek Vasut <marex@denx.de> wrote: >> >> On 6/10/19 8:07 AM, Ley Foon Tan wrote: >> [...] >> >>>>>> [...] >>>>>> >>>>>>> @@ -523,8 +527,20 @@ static int designware_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, >>>>>>> static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) >>>>>>> { >>>>>>> struct dw_i2c *i2c = dev_get_priv(bus); >>>>>>> + ulong rate; >>>>>>> + >>>>>>> +#if CONFIG_IS_ENABLED(CLK) >>>>>>> + rate = clk_get_rate(&i2c->clk); >>>>>> >>>>>> Do we need to re-read the bus frequency for each transfer ? >>>>>> I would expect set_bus_speed callback to set the frequencies once and >>>>>> then keep them that way until it's called again. >>>>> Yes, we can get clock rate when request clock in _probe(). Then keep a >>>>> copy for future use. >>>> >>>> Not in .probe() , in set_bus_speed(). >>> My patch is doing it in designware_i2c_set_bus_speed() already. We >>> can't get clock rate in __dw_i2c_set_bus_speed(), because this >>> function doesn't have struct udevice. >> >> include/i2c.h struct dm_i2c_ops {} : >> >> 388 /** >> 389 * set_bus_speed() - set the speed of a bus (optional) >> 390 * >> 391 * The bus speed value will be updated by the uclass if this >> function >> 392 * does not return an error. This method is optional - if it >> is not >> 393 * provided then the driver can read the speed from >> 394 * dev_get_uclass_priv(bus)->speed_hz >> 395 * >> 396 * @bus: Bus to adjust >> 397 * @speed: Requested speed in Hz >> 398 * @return 0 if OK, -EINVAL for invalid values >> 399 */ >> 400 int (*set_bus_speed)(struct udevice *bus, unsigned int speed); >> >> There's struct udevice right there ^ > > I'm confused now. > > .set_bus_speed = designware_i2c_set_bus_speed and my patch is doing > get clock rate in .set_bus_speed callback already. > > static 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, > }; Uh, clearly I was confused as well, sorry. Acked-by: Marek Vasut <marex@denx.de>
diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c index 9ccc2411a6..a1ed30650c 100644 --- a/drivers/i2c/designware_i2c.c +++ b/drivers/i2c/designware_i2c.c @@ -4,6 +4,7 @@ * Vipin Kumar, ST Micoelectronics, vipin.kumar@st.com. */ +#include <clk.h> #include <common.h> #include <dm.h> #include <i2c.h> @@ -35,6 +36,9 @@ 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 @@ -78,7 +82,7 @@ static int dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) */ 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 speed, u32 bus_mhz) { unsigned int cntl; unsigned int hcnt, lcnt; @@ -104,8 +108,8 @@ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base, 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; + hcnt = (bus_mhz * MIN_HS_SCL_HIGHTIME) / NANO_TO_MICRO; + lcnt = (bus_mhz * MIN_HS_SCL_LOWTIME) / NANO_TO_MICRO; } writel(hcnt, &i2c_base->ic_hs_scl_hcnt); writel(lcnt, &i2c_base->ic_hs_scl_lcnt); @@ -118,8 +122,8 @@ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base, 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; + hcnt = (bus_mhz * MIN_SS_SCL_HIGHTIME) / NANO_TO_MICRO; + lcnt = (bus_mhz * MIN_SS_SCL_LOWTIME) / NANO_TO_MICRO; } writel(hcnt, &i2c_base->ic_ss_scl_hcnt); writel(lcnt, &i2c_base->ic_ss_scl_lcnt); @@ -132,8 +136,8 @@ static unsigned int __dw_i2c_set_bus_speed(struct i2c_regs *i2c_base, 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; + hcnt = (bus_mhz * MIN_FS_SCL_HIGHTIME) / NANO_TO_MICRO; + lcnt = (bus_mhz * MIN_FS_SCL_LOWTIME) / NANO_TO_MICRO; } writel(hcnt, &i2c_base->ic_fs_scl_hcnt); writel(lcnt, &i2c_base->ic_fs_scl_lcnt); @@ -388,7 +392,7 @@ static int __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, NULL, speed); + __dw_i2c_set_bus_speed(i2c_base, NULL, speed, IC_CLK); writel(slaveaddr, &i2c_base->ic_sar); #endif @@ -433,7 +437,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), NULL, speed); + return __dw_i2c_set_bus_speed(i2c_get_base(adap), NULL, speed, IC_CLK); } static void dw_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) @@ -523,8 +527,20 @@ static int designware_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, static int designware_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) { struct dw_i2c *i2c = dev_get_priv(bus); + ulong rate; + +#if CONFIG_IS_ENABLED(CLK) + rate = clk_get_rate(&i2c->clk); + if (IS_ERR_VALUE(rate)) + return -EINVAL; - return __dw_i2c_set_bus_speed(i2c->regs, i2c->scl_sda_cfg, speed); + /* Convert to MHz */ + rate /= 1000000; +#else + rate = IC_CLK; +#endif + return __dw_i2c_set_bus_speed(i2c->regs, i2c->scl_sda_cfg, speed, + rate); } static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr, @@ -568,6 +584,19 @@ static int designware_i2c_probe(struct udevice *bus) else reset_deassert_bulk(&priv->resets); +#if CONFIG_IS_ENABLED(CLK) + ret = clk_get_by_index(bus, 0, &priv->clk); + if (ret) + return ret; + + ret = clk_enable(&priv->clk); + if (ret && ret != -ENOSYS && ret != -ENOTSUPP) { + clk_free(&priv->clk); + dev_err(bus, "failed to enable clock\n"); + return ret; + } +#endif + return __dw_i2c_init(priv->regs, 0, 0); } @@ -575,6 +604,11 @@ static int designware_i2c_remove(struct udevice *dev) { struct dw_i2c *priv = dev_get_priv(dev); +#if CONFIG_IS_ENABLED(CLK) + clk_disable(&priv->clk); + clk_free(&priv->clk); +#endif + return reset_release_bulk(&priv->resets); }
Get clock rate from clock DM if CONFIG_CLK is enabled. Otherwise, uses IC_CLK define. Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com> --- drivers/i2c/designware_i2c.c | 54 +++++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 10 deletions(-)