Message ID | 2185953.Zqs1uTCBnV@typ (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hello Addy, On Wednesday 10 September 2014 at 16:16:10, Addy wrote: > > @@ -724,16 +816,28 @@ static int rk3x_i2c_probe(struct platform_device > > *pdev) > > > > return ret; > > > > } > > > > + i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb; > > + ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb); > > + if (ret != 0) { > > + dev_err(&pdev->dev, "Unable to register clock notifier\n"); > > + goto err_clk; > > + } > > + > > + i2c->clk_freq = clk_get_rate(i2c->clk); > > + rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); > > I have tested on rk3288-pinky board: > I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate() > to write div value to CLKDIV register. If not, system will crash. > > So we need call clk_enable() before rk3x_i2c_set_scl_rate(). Interesting. It works without problems on RK3188. Do you know if the clock has to be kept enabled for some time? Or is it okay to do it like this? clk_enable(i2c->clk); rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); clk_disable(i2c->clk); Cheers, Max
Hi Max On 2014/9/13 19:26, Max Schwarz wrote: > Hello Addy, > > On Wednesday 10 September 2014 at 16:16:10, Addy wrote: >>> @@ -724,16 +816,28 @@ static int rk3x_i2c_probe(struct platform_device >>> *pdev) >>> >>> return ret; >>> >>> } >>> >>> + i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb; >>> + ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb); >>> + if (ret != 0) { >>> + dev_err(&pdev->dev, "Unable to register clock notifier\n"); >>> + goto err_clk; >>> + } >>> + >>> + i2c->clk_freq = clk_get_rate(i2c->clk); >>> + rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); >> >> I have tested on rk3288-pinky board: >> I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate() >> to write div value to CLKDIV register. If not, system will crash. >> >> So we need call clk_enable() before rk3x_i2c_set_scl_rate(). > > Interesting. It works without problems on RK3188. Do you know if the clock has > to be kept enabled for some time? Or is it okay to do it like this? > > clk_enable(i2c->clk); > rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); > clk_disable(i2c->clk); > > Cheers, > Max > Before write value to i2c register, it is necessary to enable i2c's clock and parent clock! In the linux-rockchip branch code, the i2c's clock and parent clock are enabled by default. So there are no problems on it. But on rk388-pinky board, pclk_peri is disabled when i2c driver is probed. (Maybe its reference count drops to 0 and it is disabled by clk driver) Hi, Doug Do you known why pclk_peri is disabled when i2c driver is probed? My test branch is cros/chromeos-3.14. > >
Hi Addy, Am Montag, 15. September 2014, 11:11:24 schrieb addy ke: > On 2014/9/13 19:26, Max Schwarz wrote: > > On Wednesday 10 September 2014 at 16:16:10, Addy wrote: > >>> @@ -724,16 +816,28 @@ static int rk3x_i2c_probe(struct platform_device > >>> *pdev) > >>> > >>> return ret; > >>> > >>> } > >>> > >>> + i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb; > >>> + ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb); > >>> + if (ret != 0) { > >>> + dev_err(&pdev->dev, "Unable to register clock notifier\n"); > >>> + goto err_clk; > >>> + } > >>> + > >>> + i2c->clk_freq = clk_get_rate(i2c->clk); > >>> + rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); > >> > >> I have tested on rk3288-pinky board: > >> I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate() > >> to write div value to CLKDIV register. If not, system will crash. > >> > >> So we need call clk_enable() before rk3x_i2c_set_scl_rate(). > > > > Interesting. It works without problems on RK3188. Do you know if the clock > > has to be kept enabled for some time? Or is it okay to do it like this? > > > > clk_enable(i2c->clk); > > rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); > > clk_disable(i2c->clk); > > > > Cheers, > > > > Max > > Before write value to i2c register, it is necessary to enable i2c's clock > and parent clock! The parent clock is enabled by the clk framework when we enable the child clock. No need to do it explicitly. My question was whether we can immediately *disable* the clock after writing the divider register. Could you test the above snippet in the i2c probe function? I don't have access to a RK3288 board. > In the linux-rockchip branch code, the i2c's clock and parent clock are > enabled by default. So there are no problems on it. Yes, I saw that. I still think it's nice that we disable the clock when we don't need it. I'd like to keep that if possible. > But on rk388-pinky board, pclk_peri is disabled when i2c driver is probed. > (Maybe its reference count drops to 0 and it is disabled by clk driver) Yes, that's probably because no one is using childs of the pclk_peri clock yet. Cheers, Max
Am Montag, 15. September 2014, 10:29:21 schrieb Max Schwarz: > Hi Addy, > > Am Montag, 15. September 2014, 11:11:24 schrieb addy ke: > > On 2014/9/13 19:26, Max Schwarz wrote: > > > On Wednesday 10 September 2014 at 16:16:10, Addy wrote: > > >>> @@ -724,16 +816,28 @@ static int rk3x_i2c_probe(struct platform_device > > >>> *pdev) > > >>> > > >>> return ret; > > >>> > > >>> } > > >>> > > >>> + i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb; > > >>> + ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb); > > >>> + if (ret != 0) { > > >>> + dev_err(&pdev->dev, "Unable to register clock notifier\n"); > > >>> + goto err_clk; > > >>> + } > > >>> + > > >>> + i2c->clk_freq = clk_get_rate(i2c->clk); > > >>> + rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); > > >> > > >> I have tested on rk3288-pinky board: > > >> I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate() > > >> to write div value to CLKDIV register. If not, system will crash. > > >> > > >> So we need call clk_enable() before rk3x_i2c_set_scl_rate(). > > > > > > Interesting. It works without problems on RK3188. Do you know if the > > > clock > > > has to be kept enabled for some time? Or is it okay to do it like this? > > > > > > clk_enable(i2c->clk); > > > rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); > > > clk_disable(i2c->clk); > > > > > > Cheers, > > > > > > Max > > > > Before write value to i2c register, it is necessary to enable i2c's clock > > and parent clock! > > The parent clock is enabled by the clk framework when we enable the child > clock. No need to do it explicitly. Correct. To make it even more explicit, kernel/driver code should _never_ rely on a clock magically just being on, but instead must handle it properly. CLK_IGNORE_UNUSED and friends are just an intermediate measures when parts of the clock tree are still unhandled. Heiko > My question was whether we can immediately *disable* the clock after writing > the divider register. Could you test the above snippet in the i2c probe > function? I don't have access to a RK3288 board. > > > In the linux-rockchip branch code, the i2c's clock and parent clock are > > enabled by default. So there are no problems on it. > > Yes, I saw that. I still think it's nice that we disable the clock when we > don't need it. I'd like to keep that if possible. > > > But on rk388-pinky board, pclk_peri is disabled when i2c driver is probed. > > (Maybe its reference count drops to 0 and it is disabled by clk driver) > > Yes, that's probably because no one is using childs of the pclk_peri clock > yet. > > Cheers, > Max
On 2014/9/15 16:29, Max Schwarz wrote: > Hi Addy, > > Am Montag, 15. September 2014, 11:11:24 schrieb addy ke: >> On 2014/9/13 19:26, Max Schwarz wrote: >>> On Wednesday 10 September 2014 at 16:16:10, Addy wrote: >>>>> @@ -724,16 +816,28 @@ static int rk3x_i2c_probe(struct platform_device >>>>> *pdev) >>>>> >>>>> return ret; >>>>> >>>>> } >>>>> >>>>> + i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb; >>>>> + ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb); >>>>> + if (ret != 0) { >>>>> + dev_err(&pdev->dev, "Unable to register clock notifier\n"); >>>>> + goto err_clk; >>>>> + } >>>>> + >>>>> + i2c->clk_freq = clk_get_rate(i2c->clk); >>>>> + rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); >>>> >>>> I have tested on rk3288-pinky board: >>>> I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate() >>>> to write div value to CLKDIV register. If not, system will crash. >>>> >>>> So we need call clk_enable() before rk3x_i2c_set_scl_rate(). >>> >>> Interesting. It works without problems on RK3188. Do you know if the clock >>> has to be kept enabled for some time? Or is it okay to do it like this? >>> >>> clk_enable(i2c->clk); >>> rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); >>> clk_disable(i2c->clk); >>> >>> Cheers, >>> >>> Max >> >> Before write value to i2c register, it is necessary to enable i2c's clock >> and parent clock! > > The parent clock is enabled by the clk framework when we enable the child > clock. No need to do it explicitly. > > My question was whether we can immediately *disable* the clock after writing > the divider register. Could you test the above snippet in the i2c probe > function? I don't have access to a RK3288 board. > sure, we can. I have comfirmed on rk3288-pinky board. >> In the linux-rockchip branch code, the i2c's clock and parent clock are >> enabled by default. So there are no problems on it. > > Yes, I saw that. I still think it's nice that we disable the clock when we > don't need it. I'd like to keep that if possible. > >> But on rk388-pinky board, pclk_peri is disabled when i2c driver is probed. >> (Maybe its reference count drops to 0 and it is disabled by clk driver) > > Yes, that's probably because no one is using childs of the pclk_peri clock > yet. > > Cheers, > Max > > >
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c index 93cfc83..825632a 100644 --- a/drivers/i2c/busses/i2c-rk3x.c +++ b/drivers/i2c/busses/i2c-rk3x.c @@ -97,6 +97,8 @@ struct rk3x_i2c { /* Hardware resources */ void __iomem *regs; struct clk *clk; + struct notifier_block clk_rate_nb; + unsigned long clk_freq; /* Settings */ unsigned int scl_frequency; @@ -428,21 +430,114 @@ out: return IRQ_HANDLED; } -static void rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long scl_rate) +/** + * Calculate divider value for desired SCL frequency + * + * @clk_rate: I2C input clock rate + * @scl_rate: Desired SCL rate + * @div: Divider output + * + * Return: 0 on success, -EINVAL on unreachable SCL rate. In that case + * a best-effort divider value is returned in div. + **/ +static int rk3x_i2c_calc_div(unsigned long clk_rate, unsigned long scl_rate, + unsigned int *div) { - unsigned long i2c_rate = clk_get_rate(i2c->clk); - unsigned int div; + /* Range checks */ + if (scl_rate > clk_rate / 16) { + /* Return a best effort divider */ + *div = 0; + return -EINVAL; + } + + if (scl_rate < clk_rate / (16 * 65536)) { + /* Return a best effort divider */ + *div = 0xFFFF; + return -EINVAL; + } /* set DIV = DIVH = DIVL * SCL rate = (clk rate) / (8 * (DIVH + 1 + DIVL + 1)) * = (clk rate) / (16 * (DIV + 1)) */ - div = DIV_ROUND_UP(i2c_rate, scl_rate * 16) - 1; + *div = DIV_ROUND_UP(clk_rate, scl_rate * 16) - 1; + + return 0; +} + +static void rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long scl_rate) +{ + unsigned long i2c_rate = clk_get_rate(i2c->clk); + unsigned int div; + + if (rk3x_i2c_calc_div(i2c_rate, scl_rate, &div) != 0) { + dev_warn(i2c->dev, "Could not reach desired SCL freq %lu Hz\n", + scl_rate + ); + } i2c_writel(i2c, (div << 16) | (div & 0xffff), REG_CLKDIV); } /** + * rk3x_i2c_clk_notifier_cb - Clock rate change callback + * @nb: Pointer to notifier block + * @event: Notification reason + * @data: Pointer to notification data object + * + * The callback checks whether a valid bus frequency can be generated after the + * change. If so, the change is acknowledged, otherwise the change is aborted. + * New dividers are written to the HW in the pre- or post change notification + * depending on the scaling direction. + * + * Code adapted from i2c-cadence.c. + * + * Return: NOTIFY_STOP if the rate change should be aborted, NOTIFY_OK + * to acknowedge the change, NOTIFY_DONE if the notification is + * considered irrelevant. + */ +static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long + event, void *data) +{ + struct clk_notifier_data *ndata = data; + struct rk3x_i2c *i2c = container_of(nb, struct rk3x_i2c, clk_rate_nb); + + switch (event) { + case PRE_RATE_CHANGE: + { + unsigned int div; + + if (rk3x_i2c_calc_div(ndata->new_rate, i2c->scl_frequency, + &div) != 0) { + return NOTIFY_STOP; + } + + i2c->clk_freq = ndata->new_rate; + + /* scale up */ + if (ndata->new_rate > ndata->old_rate) + rk3x_i2c_set_scl_rate(i2c, ndata->new_rate); + + return NOTIFY_OK; + } + case POST_RATE_CHANGE: + i2c->clk_freq = ndata->new_rate; + + /* scale down */ + if (ndata->new_rate < ndata->old_rate) + rk3x_i2c_set_scl_rate(i2c, ndata->new_rate); + return NOTIFY_OK; + case ABORT_RATE_CHANGE: + /* scale up */ + if (ndata->new_rate > ndata->old_rate) + rk3x_i2c_set_scl_rate(i2c, ndata->old_rate); + return NOTIFY_OK; + default: + return NOTIFY_DONE; + } +} + +/** * Setup I2C registers for an I2C operation specified by msgs, num. * * Must be called with i2c->lock held. @@ -536,9 +631,6 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap, clk_enable(i2c->clk); - /* The clock rate might have changed, so setup the divider again */ - rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); - i2c->is_last_msg = false; /* @@ -724,16 +816,28 @@ static int rk3x_i2c_probe(struct platform_device *pdev) return ret; } + i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb; + ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb); + if (ret != 0) { + dev_err(&pdev->dev, "Unable to register clock notifier\n"); + goto err_clk; + } + + i2c->clk_freq = clk_get_rate(i2c->clk); + rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); + ret = i2c_add_adapter(&i2c->adap); if (ret < 0) { dev_err(&pdev->dev, "Could not register adapter\n"); - goto err_clk; + goto err_clk_notifier; } dev_info(&pdev->dev, "Initialized RK3xxx I2C bus at %p\n", i2c->regs); return 0; +err_clk_notifier: + clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb); err_clk: clk_unprepare(i2c->clk); return ret;