| Message ID | 20200814000621.8415-14-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Introduce memory interconnect for NVIDIA Tegra SoCs | expand |
Hi Dmitry, I add the minor comment. Except of some comments, it looks good to me. On 8/14/20 9:05 AM, Dmitry Osipenko wrote: > The clk_round_rate() won't be usable for building OPP table once > interconnect support will be added to the EMC driver because that CLK API > function limits the rounded rate based on the clk rate that is imposed by > active clk-users, and thus, the rounding won't work as expected if > interconnect will set the minimum EMC clock rate before devfreq driver is > loaded. The struct tegra_mc contains memory timings which could be used by > the devfreq driver for building up OPP table instead of rounding clock > rate, this patch implements this idea. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/devfreq/tegra30-devfreq.c | 93 +++++++++++++++++++++---------- > 1 file changed, 65 insertions(+), 28 deletions(-) > > diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c > index 423dd35c95b3..6c2f56b34535 100644 > --- a/drivers/devfreq/tegra30-devfreq.c > +++ b/drivers/devfreq/tegra30-devfreq.c > @@ -19,6 +19,8 @@ > #include <linux/reset.h> > #include <linux/workqueue.h> > > +#include <soc/tegra/mc.h> > + > #include "governor.h" > > #define ACTMON_GLB_STATUS 0x0 > @@ -153,6 +155,18 @@ struct tegra_devfreq_device { > unsigned long target_freq; > }; > > +struct tegra_devfreq_soc_data { > + const char *mc_compatible; > +}; > + > +static const struct tegra_devfreq_soc_data tegra30_soc = { > + .mc_compatible = "nvidia,tegra30-mc", > +}; > + > +static const struct tegra_devfreq_soc_data tegra124_soc = { > + .mc_compatible = "nvidia,tegra124-mc", > +}; > + > struct tegra_devfreq { > struct devfreq *devfreq; > > @@ -771,15 +785,49 @@ static struct devfreq_governor tegra_devfreq_governor = { > .interrupt_driven = true, > }; > > +static struct tegra_mc *tegra_get_memory_controller(const char *compatible) > +{ > + struct platform_device *pdev; > + struct device_node *np; > + struct tegra_mc *mc; > + > + np = of_find_compatible_node(NULL, NULL, compatible); > + if (!np) > + return ERR_PTR(-ENOENT); > + > + pdev = of_find_device_by_node(np); > + of_node_put(np); > + if (!pdev) > + return ERR_PTR(-ENODEV); > + > + mc = platform_get_drvdata(pdev); > + if (!mc) > + return ERR_PTR(-EPROBE_DEFER); > + > + return mc; > +} > + > static int tegra_devfreq_probe(struct platform_device *pdev) > { > + const struct tegra_devfreq_soc_data *soc_data; > struct tegra_devfreq_device *dev; > struct tegra_devfreq *tegra; > struct devfreq *devfreq; > + struct tegra_mc *mc; > unsigned int i; > - long rate; > int err; > > + soc_data = of_device_get_match_data(&pdev->dev); I think that better to check whether 'soc_data' is not NULL. > + > + mc = tegra_get_memory_controller(soc_data->mc_compatible); > + if (IS_ERR(mc)) > + return PTR_ERR(mc); You better to add error log. > + > + if (!mc->num_timings) { > + dev_info(&pdev->dev, "memory controller has no timings\n"); > + return -ENODEV; > + } > + > tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL); > if (!tegra) > return -ENOMEM; > @@ -825,6 +873,20 @@ static int tegra_devfreq_probe(struct platform_device *pdev) > return err; > } > > + for (i = 0; i < mc->num_timings; i++) { > + /* > + * Memory Controller timings are sorted in ascending clock > + * rate order, so the last timing will be the max freq. > + */ > + tegra->max_freq = mc->timings[i].rate / KHZ; > + > + err = dev_pm_opp_add(&pdev->dev, tegra->max_freq, 0); > + if (err) { > + dev_err(&pdev->dev, "Failed to add OPP: %d\n", err); > + goto remove_opps; > + } > + } > + > reset_control_assert(tegra->reset); > > err = clk_prepare_enable(tegra->clock); > @@ -836,37 +898,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev) > > reset_control_deassert(tegra->reset); > > - rate = clk_round_rate(tegra->emc_clock, ULONG_MAX); > - if (rate < 0) { > - dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate); > - return rate; > - } > - > - tegra->max_freq = rate / KHZ; > - > for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) { > dev = tegra->devices + i; > dev->config = actmon_device_configs + i; > dev->regs = tegra->regs + dev->config->offset; > } > > - for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) { > - rate = clk_round_rate(tegra->emc_clock, rate); > - > - if (rate < 0) { > - dev_err(&pdev->dev, > - "Failed to round clock rate: %ld\n", rate); > - err = rate; > - goto remove_opps; > - } > - > - err = dev_pm_opp_add(&pdev->dev, rate / KHZ, 0); > - if (err) { > - dev_err(&pdev->dev, "Failed to add OPP: %d\n", err); > - goto remove_opps; > - } > - } > - > platform_set_drvdata(pdev, tegra); > > tegra->clk_rate_change_nb.notifier_call = tegra_actmon_clk_notify_cb; > @@ -921,8 +958,8 @@ static int tegra_devfreq_remove(struct platform_device *pdev) > } > > static const struct of_device_id tegra_devfreq_of_match[] = { > - { .compatible = "nvidia,tegra30-actmon" }, > - { .compatible = "nvidia,tegra124-actmon" }, > + { .compatible = "nvidia,tegra30-actmon", .data = &tegra30_soc, }, > + { .compatible = "nvidia,tegra124-actmon", .data = &tegra124_soc, }, > { }, > }; > >
14.08.2020 05:02, Chanwoo Choi пишет: > Hi Dmitry, > > I add the minor comment. Except of some comments, it looks good to me. Hello, Chanwoo! Thank you for the review! ... >> +struct tegra_devfreq_soc_data { >> + const char *mc_compatible; >> +}; >> + >> +static const struct tegra_devfreq_soc_data tegra30_soc = { >> + .mc_compatible = "nvidia,tegra30-mc", >> +}; >> + >> +static const struct tegra_devfreq_soc_data tegra124_soc = { >> + .mc_compatible = "nvidia,tegra124-mc", >> +}; . >> + soc_data = of_device_get_match_data(&pdev->dev); > > I think that better to check whether 'soc_data' is not NULL. It's a quite common misconception among kernel developers that of_device_get_match_data() may "accidentally" return NULL, but it couldn't if every driver's of_match[] entry has a non-NULL .data field and because the OF-matching already happened at the driver's probe-time [1], which is the case of this driver. [1] https://elixir.bootlin.com/linux/v5.8/source/drivers/of/device.c#L189 Hence the NULL-checking is unnecessary. When I first encountered the of_device_get_match_data(), I was also thinking that adding the NULL-checks is a good idea, but later on somebody pointed out to me (maybe Thierry) that it's unnecessary to do. >> + >> + mc = tegra_get_memory_controller(soc_data->mc_compatible); >> + if (IS_ERR(mc)) >> + return PTR_ERR(mc); > > You better to add error log. In practice we should get only -EPROBE_DEFER here ever. I'll consider adding the message in the next revision, at least just for consistency. Thanks! ... >> static const struct of_device_id tegra_devfreq_of_match[] = { >> - { .compatible = "nvidia,tegra30-actmon" }, >> - { .compatible = "nvidia,tegra124-actmon" }, >> + { .compatible = "nvidia,tegra30-actmon", .data = &tegra30_soc, }, >> + { .compatible = "nvidia,tegra124-actmon", .data = &tegra124_soc, }, >> { }, >> };
Hi, On 8/15/20 1:47 AM, Dmitry Osipenko wrote: > 14.08.2020 05:02, Chanwoo Choi пишет: >> Hi Dmitry, >> >> I add the minor comment. Except of some comments, it looks good to me. > > Hello, Chanwoo! Thank you for the review! > > ... >>> +struct tegra_devfreq_soc_data { >>> + const char *mc_compatible; >>> +}; >>> + >>> +static const struct tegra_devfreq_soc_data tegra30_soc = { >>> + .mc_compatible = "nvidia,tegra30-mc", >>> +}; >>> + >>> +static const struct tegra_devfreq_soc_data tegra124_soc = { >>> + .mc_compatible = "nvidia,tegra124-mc", >>> +}; > . >>> + soc_data = of_device_get_match_data(&pdev->dev); >> >> I think that better to check whether 'soc_data' is not NULL. > > It's a quite common misconception among kernel developers that > of_device_get_match_data() may "accidentally" return NULL, but it > couldn't if every driver's of_match[] entry has a non-NULL .data field > and because the OF-matching already happened at the driver's probe-time > [1], which is the case of this driver. > > [1] https://elixir.bootlin.com/linux/v5.8/source/drivers/of/device.c#L189 > > Hence the NULL-checking is unnecessary. > > When I first encountered the of_device_get_match_data(), I was also > thinking that adding the NULL-checks is a good idea, but later on > somebody pointed out to me (maybe Thierry) that it's unnecessary to do. OK. Thanks. > >>> + >>> + mc = tegra_get_memory_controller(soc_data->mc_compatible); >>> + if (IS_ERR(mc)) >>> + return PTR_ERR(mc); >> >> You better to add error log. > > In practice we should get only -EPROBE_DEFER here ever. I'll consider > adding the message in the next revision, at least just for consistency. In order to handle -EPROBE_DEFER, recommend the using of dev_err_probe(). (snip)
28.08.2020 04:47, Chanwoo Choi пишет: > Hi, ... >> Hence the NULL-checking is unnecessary. >> >> When I first encountered the of_device_get_match_data(), I was also >> thinking that adding the NULL-checks is a good idea, but later on >> somebody pointed out to me (maybe Thierry) that it's unnecessary to do. > > OK. Thanks. > >> >>>> + >>>> + mc = tegra_get_memory_controller(soc_data->mc_compatible); >>>> + if (IS_ERR(mc)) >>>> + return PTR_ERR(mc); >>> >>> You better to add error log. >> >> In practice we should get only -EPROBE_DEFER here ever. I'll consider >> adding the message in the next revision, at least just for consistency. > > In order to handle -EPROBE_DEFER, recommend the using of dev_err_probe(). Hello, Chanwoo! Thank you for the suggestion! I wasn't aware about the dev_err_probe() until recently and will use this new helper in the v6! Thanks!
diff --git a/drivers/devfreq/tegra30-devfreq.c b/drivers/devfreq/tegra30-devfreq.c index 423dd35c95b3..6c2f56b34535 100644 --- a/drivers/devfreq/tegra30-devfreq.c +++ b/drivers/devfreq/tegra30-devfreq.c @@ -19,6 +19,8 @@ #include <linux/reset.h> #include <linux/workqueue.h> +#include <soc/tegra/mc.h> + #include "governor.h" #define ACTMON_GLB_STATUS 0x0 @@ -153,6 +155,18 @@ struct tegra_devfreq_device { unsigned long target_freq; }; +struct tegra_devfreq_soc_data { + const char *mc_compatible; +}; + +static const struct tegra_devfreq_soc_data tegra30_soc = { + .mc_compatible = "nvidia,tegra30-mc", +}; + +static const struct tegra_devfreq_soc_data tegra124_soc = { + .mc_compatible = "nvidia,tegra124-mc", +}; + struct tegra_devfreq { struct devfreq *devfreq; @@ -771,15 +785,49 @@ static struct devfreq_governor tegra_devfreq_governor = { .interrupt_driven = true, }; +static struct tegra_mc *tegra_get_memory_controller(const char *compatible) +{ + struct platform_device *pdev; + struct device_node *np; + struct tegra_mc *mc; + + np = of_find_compatible_node(NULL, NULL, compatible); + if (!np) + return ERR_PTR(-ENOENT); + + pdev = of_find_device_by_node(np); + of_node_put(np); + if (!pdev) + return ERR_PTR(-ENODEV); + + mc = platform_get_drvdata(pdev); + if (!mc) + return ERR_PTR(-EPROBE_DEFER); + + return mc; +} + static int tegra_devfreq_probe(struct platform_device *pdev) { + const struct tegra_devfreq_soc_data *soc_data; struct tegra_devfreq_device *dev; struct tegra_devfreq *tegra; struct devfreq *devfreq; + struct tegra_mc *mc; unsigned int i; - long rate; int err; + soc_data = of_device_get_match_data(&pdev->dev); + + mc = tegra_get_memory_controller(soc_data->mc_compatible); + if (IS_ERR(mc)) + return PTR_ERR(mc); + + if (!mc->num_timings) { + dev_info(&pdev->dev, "memory controller has no timings\n"); + return -ENODEV; + } + tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL); if (!tegra) return -ENOMEM; @@ -825,6 +873,20 @@ static int tegra_devfreq_probe(struct platform_device *pdev) return err; } + for (i = 0; i < mc->num_timings; i++) { + /* + * Memory Controller timings are sorted in ascending clock + * rate order, so the last timing will be the max freq. + */ + tegra->max_freq = mc->timings[i].rate / KHZ; + + err = dev_pm_opp_add(&pdev->dev, tegra->max_freq, 0); + if (err) { + dev_err(&pdev->dev, "Failed to add OPP: %d\n", err); + goto remove_opps; + } + } + reset_control_assert(tegra->reset); err = clk_prepare_enable(tegra->clock); @@ -836,37 +898,12 @@ static int tegra_devfreq_probe(struct platform_device *pdev) reset_control_deassert(tegra->reset); - rate = clk_round_rate(tegra->emc_clock, ULONG_MAX); - if (rate < 0) { - dev_err(&pdev->dev, "Failed to round clock rate: %ld\n", rate); - return rate; - } - - tegra->max_freq = rate / KHZ; - for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) { dev = tegra->devices + i; dev->config = actmon_device_configs + i; dev->regs = tegra->regs + dev->config->offset; } - for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) { - rate = clk_round_rate(tegra->emc_clock, rate); - - if (rate < 0) { - dev_err(&pdev->dev, - "Failed to round clock rate: %ld\n", rate); - err = rate; - goto remove_opps; - } - - err = dev_pm_opp_add(&pdev->dev, rate / KHZ, 0); - if (err) { - dev_err(&pdev->dev, "Failed to add OPP: %d\n", err); - goto remove_opps; - } - } - platform_set_drvdata(pdev, tegra); tegra->clk_rate_change_nb.notifier_call = tegra_actmon_clk_notify_cb; @@ -921,8 +958,8 @@ static int tegra_devfreq_remove(struct platform_device *pdev) } static const struct of_device_id tegra_devfreq_of_match[] = { - { .compatible = "nvidia,tegra30-actmon" }, - { .compatible = "nvidia,tegra124-actmon" }, + { .compatible = "nvidia,tegra30-actmon", .data = &tegra30_soc, }, + { .compatible = "nvidia,tegra124-actmon", .data = &tegra124_soc, }, { }, };
The clk_round_rate() won't be usable for building OPP table once interconnect support will be added to the EMC driver because that CLK API function limits the rounded rate based on the clk rate that is imposed by active clk-users, and thus, the rounding won't work as expected if interconnect will set the minimum EMC clock rate before devfreq driver is loaded. The struct tegra_mc contains memory timings which could be used by the devfreq driver for building up OPP table instead of rounding clock rate, this patch implements this idea. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/devfreq/tegra30-devfreq.c | 93 +++++++++++++++++++++---------- 1 file changed, 65 insertions(+), 28 deletions(-)