| Message ID | 20180419104019.24406-1-enric.balletbo@collabora.com |
|---|---|
| Headers | show |
| Series | devfreq: rk3399_dmc: improve rk3399_dmc driver and it's documentation | expand |
On 19 April 2018 at 12:40, Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote: > From: Lin Huang <hl@rock-chips.com> > > We just return -EPROBE_DEFER error code to caller and do not > print error message when try to get center logic regulator > and DMC clock defer. > > Signed-off-by: Lin Huang <hl@rock-chips.com> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > > drivers/devfreq/rk3399_dmc.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c > index 44a379657cd5..5bfca028eaaf 100644 > --- a/drivers/devfreq/rk3399_dmc.c > +++ b/drivers/devfreq/rk3399_dmc.c > @@ -308,12 +308,18 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) > > data->vdd_center = devm_regulator_get(dev, "center"); > if (IS_ERR(data->vdd_center)) { > + if (PTR_ERR(data->vdd_center) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > dev_err(dev, "Cannot get the regulator \"center\"\n"); Doesn't the clock core already print an error message for this? Maybe a better way is simply to drop the printing instead of trying to have a special case for it? > return PTR_ERR(data->vdd_center); > } > > data->dmc_clk = devm_clk_get(dev, "dmc_clk"); > if (IS_ERR(data->dmc_clk)) { > + if (PTR_ERR(data->dmc_clk) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > dev_err(dev, "Cannot get the clk dmc_clk\n"); > return PTR_ERR(data->dmc_clk); > }; > -- > 2.17.0 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19 April 2018 at 12:40, Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote: > Dear all, > > These patches is an attempt to improve a little bit the rk3399_dmc > driver and it's documentation in order to have all in a better shape for > a future work I am doing. My final intention is add ddrfreq support for > rockchip drm driver, but the patches for this are still > work-in-progress. So let's start with this first patchset that is > basically some fixes/improvements for the rk3399_dmc driver. I didn't get cc:ed the series, but only the last part, which makes it bit hard to get the full context. Moreover, it would be good if you explained a bit more on what the above series actually does and why. Also I don't get why is the PM domain changes is in the series, as this seems to be about devfreq. My point is, if you want me to review, please try to be more precise so I know what to review. > > Best regards, > > Enric Balletbo i Serra (3): > dt-bindings: clock: add DDR3 standard speed bins. > devfreq: rk3399_dmc: remove wait for dcf irq event. > dt-bindings: devfreq: rk3399_dmc: remove interrupts as is not > required. > > Lin Huang (2): > devfreq: rk3399_dmc: do not print error when get supply and clk defer. > devfreq: rk3399_dmc: register devfreq notification to dmc driver. > > Nick Milner (1): > dt-bindings: devfreq: rk3399_dmc: improve binding documentation. > > .../bindings/devfreq/rk3399_dmc.txt | 204 +++++++++--------- > drivers/devfreq/rk3399_dmc.c | 106 +-------- > drivers/soc/rockchip/pm_domains.c | 31 +++ > include/dt-bindings/clock/ddr.h | 34 +++ > include/soc/rockchip/rk3399_dmc.h | 63 ++++++ > 5 files changed, 238 insertions(+), 200 deletions(-) > create mode 100644 include/dt-bindings/clock/ddr.h > create mode 100644 include/soc/rockchip/rk3399_dmc.h > > -- > 2.17.0 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2018-04-23 at 12:44 +0200, Ulf Hansson wrote: > On 19 April 2018 at 12:40, Enric Balletbo i Serra > <enric.balletbo@collabora.com> wrote: > > From: Lin Huang <hl@rock-chips.com> > > > > We just return -EPROBE_DEFER error code to caller and do not > > print error message when try to get center logic regulator > > and DMC clock defer. > > > > Signed-off-by: Lin Huang <hl@rock-chips.com> > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > --- > > > > drivers/devfreq/rk3399_dmc.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c > > index 44a379657cd5..5bfca028eaaf 100644 > > --- a/drivers/devfreq/rk3399_dmc.c > > +++ b/drivers/devfreq/rk3399_dmc.c > > @@ -308,12 +308,18 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) > > > > data->vdd_center = devm_regulator_get(dev, "center"); > > if (IS_ERR(data->vdd_center)) { > > + if (PTR_ERR(data->vdd_center) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + > > dev_err(dev, "Cannot get the regulator \"center\"\n"); > > Doesn't the clock core already print an error message for this? > s/clock/regulator? > Maybe a better way is simply to drop the printing instead of trying to > have a special case for it? > I don't think so. If you remove this print you: i) might get a print from the core, or ii) maybe not because of some path without a print. And even if you do get an error, it might not relate exactly to the driver that requested the resource, because you might be printing via pr_xxx, from a context without a struct dev to use dev_xxx. > > return PTR_ERR(data->vdd_center); > > } > > > > data->dmc_clk = devm_clk_get(dev, "dmc_clk"); > > if (IS_ERR(data->dmc_clk)) { > > + if (PTR_ERR(data->dmc_clk) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + > > dev_err(dev, "Cannot get the clk dmc_clk\n"); > > return PTR_ERR(data->dmc_clk); > > }; > > -- > > 2.17.0 > > > > Kind regards > Uffe > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> We have already wait dcf done in ATF, so don't need wait dcf irq > in kernel, besides, clear dcf irq in kernel will import competiton > between kernel and ATF, only handle dcf irq in ATF is a better way. > > Signed-off-by: Lin Huang <hl@rock-chips.com> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> This does not apply to 4.17-rc2 or a little less recent rc's. Would you please rebase to the latest? Cheers, MyungJoo > --- > > drivers/devfreq/rk3399_dmc.c | 53 +----------------------------------- > 1 file changed, 1 insertion(+), 52 deletions(-) > > diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c > index 5dfbfa3cc878..44a379657cd5 100644 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Lin Huang <hl@rock-chips.com> > > We just return -EPROBE_DEFER error code to caller and do not > print error message when try to get center logic regulator > and DMC clock defer. > > Signed-off-by: Lin Huang <hl@rock-chips.com> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com> I'll wait for the rebase of 3/6. > --- > > drivers/devfreq/rk3399_dmc.c | 6 ++++++ > 1 file changed, 6 insertions(+) > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018년 04월 19일 19:40, Enric Balletbo i Serra wrote: > From: Lin Huang <hl@rock-chips.com> > > We just return -EPROBE_DEFER error code to caller and do not > print error message when try to get center logic regulator > and DMC clock defer. > > Signed-off-by: Lin Huang <hl@rock-chips.com> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > > drivers/devfreq/rk3399_dmc.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c > index 44a379657cd5..5bfca028eaaf 100644 > --- a/drivers/devfreq/rk3399_dmc.c > +++ b/drivers/devfreq/rk3399_dmc.c > @@ -308,12 +308,18 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) > > data->vdd_center = devm_regulator_get(dev, "center"); > if (IS_ERR(data->vdd_center)) { > + if (PTR_ERR(data->vdd_center) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > dev_err(dev, "Cannot get the regulator \"center\"\n"); > return PTR_ERR(data->vdd_center); > } > > data->dmc_clk = devm_clk_get(dev, "dmc_clk"); > if (IS_ERR(data->dmc_clk)) { > + if (PTR_ERR(data->dmc_clk) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > dev_err(dev, "Cannot get the clk dmc_clk\n"); > return PTR_ERR(data->dmc_clk); > }; > Looks good to me. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Hi, On 2018년 04월 19일 19:40, Enric Balletbo i Serra wrote: > We have already wait dcf done in ATF, so don't need wait dcf irq > in kernel, besides, clear dcf irq in kernel will import competiton > between kernel and ATF, only handle dcf irq in ATF is a better way. > > Signed-off-by: Lin Huang <hl@rock-chips.com> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- The dcf_irq depends on the architecture and device. It means that the author or other people access the confidential document have to guarantee this change. So, Lin Huang is one of author of this patch, looks good to me. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> [snip]
Hi, On 2018년 04월 19일 19:40, Enric Balletbo i Serra wrote: > From: Lin Huang <hl@rock-chips.com> > > Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to > ensure that the pd driver and the dmc driver will not access at this > register at the same time. > > Signed-off-by: Lin Huang <hl@rock-chips.com> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > > drivers/devfreq/rk3399_dmc.c | 47 +---------------------- > drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++ > include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++ > 3 files changed, 96 insertions(+), 45 deletions(-) > create mode 100644 include/soc/rockchip/rk3399_dmc.h > > diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c > index 5bfca028eaaf..a1f320634d69 100644 > --- a/drivers/devfreq/rk3399_dmc.c > +++ b/drivers/devfreq/rk3399_dmc.c > @@ -27,51 +27,7 @@ > #include <linux/suspend.h> > > #include <soc/rockchip/rockchip_sip.h> > - > -struct dram_timing { > - unsigned int ddr3_speed_bin; > - unsigned int pd_idle; > - unsigned int sr_idle; > - unsigned int sr_mc_gate_idle; > - unsigned int srpd_lite_idle; > - unsigned int standby_idle; > - unsigned int auto_pd_dis_freq; > - unsigned int dram_dll_dis_freq; > - unsigned int phy_dll_dis_freq; > - unsigned int ddr3_odt_dis_freq; > - unsigned int ddr3_drv; > - unsigned int ddr3_odt; > - unsigned int phy_ddr3_ca_drv; > - unsigned int phy_ddr3_dq_drv; > - unsigned int phy_ddr3_odt; > - unsigned int lpddr3_odt_dis_freq; > - unsigned int lpddr3_drv; > - unsigned int lpddr3_odt; > - unsigned int phy_lpddr3_ca_drv; > - unsigned int phy_lpddr3_dq_drv; > - unsigned int phy_lpddr3_odt; > - unsigned int lpddr4_odt_dis_freq; > - unsigned int lpddr4_drv; > - unsigned int lpddr4_dq_odt; > - unsigned int lpddr4_ca_odt; > - unsigned int phy_lpddr4_ca_drv; > - unsigned int phy_lpddr4_ck_cs_drv; > - unsigned int phy_lpddr4_dq_drv; > - unsigned int phy_lpddr4_odt; > -}; > - > -struct rk3399_dmcfreq { > - struct device *dev; > - struct devfreq *devfreq; > - struct devfreq_simple_ondemand_data ondemand_data; > - struct clk *dmc_clk; > - struct devfreq_event_dev *edev; > - struct mutex lock; > - struct dram_timing timing; > - struct regulator *vdd_center; > - unsigned long rate, target_rate; > - unsigned long volt, target_volt; > -}; > +#include <soc/rockchip/rk3399_dmc.h> > > static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq, > u32 flags) > @@ -394,6 +350,7 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev) > > data->dev = dev; > platform_set_drvdata(pdev, data); > + pd_register_notify_to_dmc(data->devfreq); > > return 0; > } > diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c > index 53efc386b1ad..7acc836e7eb7 100644 > --- a/drivers/soc/rockchip/pm_domains.c > +++ b/drivers/soc/rockchip/pm_domains.c > @@ -8,6 +8,7 @@ > * published by the Free Software Foundation. > */ > > +#include <linux/devfreq.h> > #include <linux/io.h> > #include <linux/iopoll.h> > #include <linux/err.h> > @@ -76,9 +77,13 @@ struct rockchip_pmu { > const struct rockchip_pmu_info *info; > struct mutex mutex; /* mutex lock for pmu */ > struct genpd_onecell_data genpd_data; > + struct devfreq *devfreq; > + struct notifier_block dmc_nb; > struct generic_pm_domain *domains[]; > }; > > +static struct rockchip_pmu *dmc_pmu; > + > #define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd) > > #define DOMAIN(pwr, status, req, idle, ack, wakeup) \ > @@ -601,6 +606,30 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu, > return error; > } > > +static int dmc_notify(struct notifier_block *nb, unsigned long event, > + void *data) > +{ > + if (event == DEVFREQ_PRECHANGE) > + mutex_lock(&dmc_pmu->mutex); > + else if (event == DEVFREQ_POSTCHANGE) > + mutex_unlock(&dmc_pmu->mutex); > + > + return NOTIFY_OK; > +} > + > +int pd_register_notify_to_dmc(struct devfreq *devfreq) > +{ > + if (!dmc_pmu) > + return -EPROBE_DEFER; > + > + dmc_pmu->devfreq = devfreq; > + dmc_pmu->dmc_nb.notifier_call = dmc_notify; > + devfreq_register_notifier(dmc_pmu->devfreq, &dmc_pmu->dmc_nb, > + DEVFREQ_TRANSITION_NOTIFIER); > + return 0; > +} > +EXPORT_SYMBOL(pd_register_notify_to_dmc); I think that it is not proper to define the nonstandard function for only specific device driver. Maybe, It makes the code more complicated. Between linux kernel frameworks, we have to use the defined function by linux kernel frameworks. If drivers/soc/rockchip/pm_domains.c is able to get the devfreq instance through devicetree, the exported function is not necessary. Sorry for that I'm not sure the alternative. [snip] > diff --git a/include/soc/rockchip/rk3399_dmc.h b/include/soc/rockchip/rk3399_dmc.h > new file mode 100644 > index 000000000000..7ccdfff1a154 > --- /dev/null > +++ b/include/soc/rockchip/rk3399_dmc.h > @@ -0,0 +1,63 @@ [snip] > + > +int pd_register_notify_to_dmc(struct devfreq *devfreq); > + > +#endif >
>From: Lin Huang <hl@rock-chips.com> > >Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to >ensure that the pd driver and the dmc driver will not access at this >register at the same time. > >Signed-off-by: Lin Huang <hl@rock-chips.com> >Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >--- > > drivers/devfreq/rk3399_dmc.c | 47 +---------------------- > drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++ > include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++ > 3 files changed, 96 insertions(+), 45 deletions(-) > create mode 100644 include/soc/rockchip/rk3399_dmc.h > >diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c >index 5bfca028eaaf..a1f320634d69 100644 >--- a/drivers/devfreq/rk3399_dmc.c >+++ b/drivers/devfreq/rk3399_dmc.c [] >diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c >index 53efc386b1ad..7acc836e7eb7 100644 >--- a/drivers/soc/rockchip/pm_domains.c >+++ b/drivers/soc/rockchip/pm_domains.c [] >+static int dmc_notify(struct notifier_block *nb, unsigned long event, >+ void *data) >+{ >+ if (event == DEVFREQ_PRECHANGE) >+ mutex_lock(&dmc_pmu->mutex); >+ else if (event == DEVFREQ_POSTCHANGE) >+ mutex_unlock(&dmc_pmu->mutex); >+ >+ return NOTIFY_OK; >+} >+ Doesn't this incur a deadlock? 1. devfreq.c:update_freq calls devfreq_notify_transition(DEVFREQ_PRECHANGE) 2. pm_domain.c:dmc_notify calls mutex_lock(dmc_pmu->mutex) 3. devfreq.c:update_freq calls target callback 4. rk3399_dmc.c:rk3399_dmcfreq_target calls mutex_lock(&dmcfreq->lock) >>>>>> update_freq cannot proceed. <<<< Cheers, MyungJoo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On 24/04/18 06:22, MyungJoo Ham wrote: >> From: Lin Huang <hl@rock-chips.com> >> >> Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to >> ensure that the pd driver and the dmc driver will not access at this >> register at the same time. >> >> Signed-off-by: Lin Huang <hl@rock-chips.com> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >> --- >> >> drivers/devfreq/rk3399_dmc.c | 47 +---------------------- >> drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++ >> include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++ >> 3 files changed, 96 insertions(+), 45 deletions(-) >> create mode 100644 include/soc/rockchip/rk3399_dmc.h >> >> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c >> index 5bfca028eaaf..a1f320634d69 100644 >> --- a/drivers/devfreq/rk3399_dmc.c >> +++ b/drivers/devfreq/rk3399_dmc.c > [] >> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c >> index 53efc386b1ad..7acc836e7eb7 100644 >> --- a/drivers/soc/rockchip/pm_domains.c >> +++ b/drivers/soc/rockchip/pm_domains.c > [] >> +static int dmc_notify(struct notifier_block *nb, unsigned long event, >> + void *data) >> +{ >> + if (event == DEVFREQ_PRECHANGE) >> + mutex_lock(&dmc_pmu->mutex); >> + else if (event == DEVFREQ_POSTCHANGE) >> + mutex_unlock(&dmc_pmu->mutex); >> + >> + return NOTIFY_OK; >> +} >> + > > Doesn't this incur a deadlock? > > 1. devfreq.c:update_freq calls devfreq_notify_transition(DEVFREQ_PRECHANGE) > 2. pm_domain.c:dmc_notify calls mutex_lock(dmc_pmu->mutex) > 3. devfreq.c:update_freq calls target callback > 4. rk3399_dmc.c:rk3399_dmcfreq_target calls mutex_lock(&dmcfreq->lock) > >>>>>> update_freq cannot proceed. <<<< > Mmm, makes sense, but I did not detect this deadlock. As this patch is controversial let me remove this patch from these series and I'll send again with the other series that applies on top of these, the series I am working on are to add ddrfreq support in the drm rockchip driver. Thinking about it I guess makes more sense as 1-5 are just cleanups, 6 is a bit different, maybe more related to the work I am doing. Best regards, Enric > > Cheers, > MyungJoo > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear all, These patches is an attempt to improve a little bit the rk3399_dmc driver and it's documentation in order to have all in a better shape for a future work I am doing. My final intention is add ddrfreq support for rockchip drm driver, but the patches for this are still work-in-progress. So let's start with this first patchset that is basically some fixes/improvements for the rk3399_dmc driver. Best regards, Enric Balletbo i Serra (3): dt-bindings: clock: add DDR3 standard speed bins. devfreq: rk3399_dmc: remove wait for dcf irq event. dt-bindings: devfreq: rk3399_dmc: remove interrupts as is not required. Lin Huang (2): devfreq: rk3399_dmc: do not print error when get supply and clk defer. devfreq: rk3399_dmc: register devfreq notification to dmc driver. Nick Milner (1): dt-bindings: devfreq: rk3399_dmc: improve binding documentation. .../bindings/devfreq/rk3399_dmc.txt | 204 +++++++++--------- drivers/devfreq/rk3399_dmc.c | 106 +-------- drivers/soc/rockchip/pm_domains.c | 31 +++ include/dt-bindings/clock/ddr.h | 34 +++ include/soc/rockchip/rk3399_dmc.h | 63 ++++++ 5 files changed, 238 insertions(+), 200 deletions(-) create mode 100644 include/dt-bindings/clock/ddr.h create mode 100644 include/soc/rockchip/rk3399_dmc.h