| Message ID | f4553b6aacc88a91ed407eb709097e2cd1bf1a19.1585188174.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | interconnect: Add imx support via devfreq | expand |
Hi Leonard, On 3/26/20 11:16 AM, Leonard Crestez wrote: > There is no single device which can represent the imx interconnect. > Instead of adding a virtual one just make the main &noc act as the > global interconnect provider. > > The imx interconnect provider driver will scale the NOC and DDRC based > on bandwidth request. More scalable nodes can be added in the future, > for example for audio/display/vpu/gpu NICs. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/devfreq/imx-bus.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c > index 285e0f1ae6b1..e9b13e43bf0a 100644 > --- a/drivers/devfreq/imx-bus.c > +++ b/drivers/devfreq/imx-bus.c > @@ -15,10 +15,11 @@ > struct imx_bus { > struct devfreq_dev_profile profile; > struct devfreq *devfreq; > struct clk *clk; > struct devfreq_passive_data passive_data; > + struct platform_device *icc_pdev; > }; > > static int imx_bus_target(struct device *dev, > unsigned long *freq, u32 flags) > { > @@ -60,11 +61,42 @@ static int imx_bus_get_dev_status(struct device *dev, > return 0; > } > > static void imx_bus_exit(struct device *dev) > { > + struct imx_bus *priv = dev_get_drvdata(dev); > + > dev_pm_opp_of_remove_table(dev); > + platform_device_unregister(priv->icc_pdev); > +} > + > +/* imx_bus_init_icc() - register matching icc provider if required */ Better to add following comments without 'imx_bus_init_icc() -' comment. /* Register matching icc provider if required */ > +static int imx_bus_init_icc(struct device *dev) > +{ > + struct imx_bus *priv = dev_get_drvdata(dev); > + const char *icc_driver_name; > + > + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) > + return 0; > + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) { > + dev_warn(dev, "imx interconnect drivers disabled\n"); > + return 0; > + } > + > + icc_driver_name = of_device_get_match_data(dev); > + if (!icc_driver_name) > + return 0; Recommend to add the error or warning message. > + > + priv->icc_pdev = platform_device_register_data( > + dev, icc_driver_name, -1, NULL, 0); > + if (IS_ERR(priv->icc_pdev)) { > + dev_err(dev, "failed to register icc provider %s: %ld\n", > + icc_driver_name, PTR_ERR(priv->devfreq)); > + return PTR_ERR(priv->devfreq); > + } > + > + return 0; > } > > static int imx_bus_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -112,18 +144,25 @@ static int imx_bus_probe(struct platform_device *pdev) > ret = PTR_ERR(priv->devfreq); > dev_err(dev, "failed to add devfreq device: %d\n", ret); > goto err; > } > > + ret = imx_bus_init_icc(dev); > + if (ret) Recommend to add the error message. > + goto err; > + > return 0; > > err: > dev_pm_opp_of_remove_table(dev); > return ret; > } > > static const struct of_device_id imx_bus_of_match[] = { > + { .compatible = "fsl,imx8mq-noc", .data = "imx8mq-interconnect", }, > + { .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", }, > + { .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", }, > { .compatible = "fsl,imx8m-noc", }, > { .compatible = "fsl,imx8m-nic", }, > { /* sentinel */ }, > }; > MODULE_DEVICE_TABLE(of, imx_bus_of_match); >
On 2020-04-01 2:00 AM, Chanwoo Choi wrote: > Hi Leonard, > > On 3/26/20 11:16 AM, Leonard Crestez wrote: >> There is no single device which can represent the imx interconnect. >> Instead of adding a virtual one just make the main &noc act as the >> global interconnect provider. >> >> The imx interconnect provider driver will scale the NOC and DDRC based >> on bandwidth request. More scalable nodes can be added in the future, >> for example for audio/display/vpu/gpu NICs. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> --- >> drivers/devfreq/imx-bus.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c >> index 285e0f1ae6b1..e9b13e43bf0a 100644 >> --- a/drivers/devfreq/imx-bus.c >> +++ b/drivers/devfreq/imx-bus.c >> @@ -15,10 +15,11 @@ >> struct imx_bus { >> struct devfreq_dev_profile profile; >> struct devfreq *devfreq; >> struct clk *clk; >> struct devfreq_passive_data passive_data; >> + struct platform_device *icc_pdev; >> }; >> >> static int imx_bus_target(struct device *dev, >> unsigned long *freq, u32 flags) >> { >> @@ -60,11 +61,42 @@ static int imx_bus_get_dev_status(struct device *dev, >> return 0; >> } >> >> static void imx_bus_exit(struct device *dev) >> { >> + struct imx_bus *priv = dev_get_drvdata(dev); >> + >> dev_pm_opp_of_remove_table(dev); >> + platform_device_unregister(priv->icc_pdev); >> +} >> + >> +/* imx_bus_init_icc() - register matching icc provider if required */ > > Better to add following comments without 'imx_bus_init_icc() -' comment. > /* Register matching icc provider if required */ This form looks like standard kernel-doc >> +static int imx_bus_init_icc(struct device *dev) >> +{ >> + struct imx_bus *priv = dev_get_drvdata(dev); >> + const char *icc_driver_name; >> + >> + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) >> + return 0; >> + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) { >> + dev_warn(dev, "imx interconnect drivers disabled\n"); >> + return 0; >> + } >> + >> + icc_driver_name = of_device_get_match_data(dev); >> + if (!icc_driver_name) >> + return 0; > > Recommend to add the error or warning message. Makes sense, imx8m-nic shouldn't have #interconnect-cells so if this condition is reach an error should be printed. >> + >> + priv->icc_pdev = platform_device_register_data( >> + dev, icc_driver_name, -1, NULL, 0); >> + if (IS_ERR(priv->icc_pdev)) { >> + dev_err(dev, "failed to register icc provider %s: %ld\n", >> + icc_driver_name, PTR_ERR(priv->devfreq)); >> + return PTR_ERR(priv->devfreq); >> + } >> + >> + return 0; >> } >> >> static int imx_bus_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -112,18 +144,25 @@ static int imx_bus_probe(struct platform_device *pdev) >> ret = PTR_ERR(priv->devfreq); >> dev_err(dev, "failed to add devfreq device: %d\n", ret); >> goto err; >> } >> >> + ret = imx_bus_init_icc(dev); >> + if (ret) > > Recommend to add the error message. imx_bus_init_icc already prints errors. Printing again here would always result in a second message. >> + goto err; >> + >> return 0; >> >> err: >> dev_pm_opp_of_remove_table(dev); >> return ret; >> } >> >> static const struct of_device_id imx_bus_of_match[] = { >> + { .compatible = "fsl,imx8mq-noc", .data = "imx8mq-interconnect", }, >> + { .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", }, >> + { .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", }, >> { .compatible = "fsl,imx8m-noc", }, >> { .compatible = "fsl,imx8m-nic", }, >> { /* sentinel */ }, >> }; >> MODULE_DEVICE_TABLE(of, imx_bus_of_match); >>
diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c index 285e0f1ae6b1..e9b13e43bf0a 100644 --- a/drivers/devfreq/imx-bus.c +++ b/drivers/devfreq/imx-bus.c @@ -15,10 +15,11 @@ struct imx_bus { struct devfreq_dev_profile profile; struct devfreq *devfreq; struct clk *clk; struct devfreq_passive_data passive_data; + struct platform_device *icc_pdev; }; static int imx_bus_target(struct device *dev, unsigned long *freq, u32 flags) { @@ -60,11 +61,42 @@ static int imx_bus_get_dev_status(struct device *dev, return 0; } static void imx_bus_exit(struct device *dev) { + struct imx_bus *priv = dev_get_drvdata(dev); + dev_pm_opp_of_remove_table(dev); + platform_device_unregister(priv->icc_pdev); +} + +/* imx_bus_init_icc() - register matching icc provider if required */ +static int imx_bus_init_icc(struct device *dev) +{ + struct imx_bus *priv = dev_get_drvdata(dev); + const char *icc_driver_name; + + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) + return 0; + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) { + dev_warn(dev, "imx interconnect drivers disabled\n"); + return 0; + } + + icc_driver_name = of_device_get_match_data(dev); + if (!icc_driver_name) + return 0; + + priv->icc_pdev = platform_device_register_data( + dev, icc_driver_name, -1, NULL, 0); + if (IS_ERR(priv->icc_pdev)) { + dev_err(dev, "failed to register icc provider %s: %ld\n", + icc_driver_name, PTR_ERR(priv->devfreq)); + return PTR_ERR(priv->devfreq); + } + + return 0; } static int imx_bus_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -112,18 +144,25 @@ static int imx_bus_probe(struct platform_device *pdev) ret = PTR_ERR(priv->devfreq); dev_err(dev, "failed to add devfreq device: %d\n", ret); goto err; } + ret = imx_bus_init_icc(dev); + if (ret) + goto err; + return 0; err: dev_pm_opp_of_remove_table(dev); return ret; } static const struct of_device_id imx_bus_of_match[] = { + { .compatible = "fsl,imx8mq-noc", .data = "imx8mq-interconnect", }, + { .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", }, + { .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", }, { .compatible = "fsl,imx8m-noc", }, { .compatible = "fsl,imx8m-nic", }, { /* sentinel */ }, }; MODULE_DEVICE_TABLE(of, imx_bus_of_match);
There is no single device which can represent the imx interconnect. Instead of adding a virtual one just make the main &noc act as the global interconnect provider. The imx interconnect provider driver will scale the NOC and DDRC based on bandwidth request. More scalable nodes can be added in the future, for example for audio/display/vpu/gpu NICs. Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- drivers/devfreq/imx-bus.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)