| Message ID | 1631554694-9599-1-git-send-email-abel.vesa@nxp.com |
|---|---|
| Headers | show |
| Series | Add interconnect and devfreq support for i.MX8MQ | expand |
Hi, OPP is mandatory for devfreq driver. Also, must need to add the OPP levels to devicetree file, it is better to show the supported OPP list for the developer who don't know the detailed background of driver. If there are no any critical issue. I prefer the existing approach for the readability. On 21. 9. 14. 오전 2:38, Abel Vesa wrote: > i.MX8M platforms get their dram OPPs from the EL3. > We don't need to duplicate that in the kernel dram dts node. > We should just trust the OPPs provided by the EL3. > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > --- > drivers/devfreq/imx8m-ddrc.c | 50 +++--------------------------------- > 1 file changed, 3 insertions(+), 47 deletions(-) > > diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c > index 583123bf2100..f18a5c3c1c03 100644 > --- a/drivers/devfreq/imx8m-ddrc.c > +++ b/drivers/devfreq/imx8m-ddrc.c > @@ -321,38 +321,9 @@ static int imx8m_ddrc_init_freq_info(struct device *dev) > if (freq->dram_core_parent_index == 2 && > freq->dram_alt_parent_index == 0) > return -ENODEV; > - } > - > - return 0; > -} > - > -static int imx8m_ddrc_check_opps(struct device *dev) > -{ > - struct imx8m_ddrc *priv = dev_get_drvdata(dev); > - struct imx8m_ddrc_freq *freq_info; > - struct dev_pm_opp *opp; > - unsigned long freq; > - int i, opp_count; > - > - /* Enumerate DT OPPs and disable those not supported by firmware */ > - opp_count = dev_pm_opp_get_opp_count(dev); > - if (opp_count < 0) > - return opp_count; > - for (i = 0, freq = 0; i < opp_count; ++i, ++freq) { > - opp = dev_pm_opp_find_freq_ceil(dev, &freq); > - if (IS_ERR(opp)) { > - dev_err(dev, "Failed enumerating OPPs: %ld\n", > - PTR_ERR(opp)); > - return PTR_ERR(opp); > - } > - dev_pm_opp_put(opp); > > - freq_info = imx8m_ddrc_find_freq(priv, freq); > - if (!freq_info) { > - dev_info(dev, "Disable unsupported OPP %luHz %luMT/s\n", > - freq, DIV_ROUND_CLOSEST(freq, 250000)); > - dev_pm_opp_disable(dev, freq); > - } > + if (dev_pm_opp_add(dev, freq->rate * 250000, 0)) > + return -ENODEV; > } > > return 0; > @@ -360,7 +331,6 @@ static int imx8m_ddrc_check_opps(struct device *dev) > > static void imx8m_ddrc_exit(struct device *dev) > { > - dev_pm_opp_of_remove_table(dev); > } > > static int imx8m_ddrc_probe(struct platform_device *pdev) > @@ -407,16 +377,7 @@ static int imx8m_ddrc_probe(struct platform_device *pdev) > return ret; > } > > - ret = dev_pm_opp_of_add_table(dev); > - if (ret < 0) { > - dev_err(dev, "failed to get OPP table\n"); > - return ret; > - } > - > - ret = imx8m_ddrc_check_opps(dev); > - if (ret < 0) > - goto err; > - > + priv->profile.polling_ms = 1000; This change is not related to role of this patch. Need to make the separate patch. > priv->profile.target = imx8m_ddrc_target; > priv->profile.exit = imx8m_ddrc_exit; > priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq; > @@ -427,13 +388,8 @@ static int imx8m_ddrc_probe(struct platform_device *pdev) > if (IS_ERR(priv->devfreq)) { > ret = PTR_ERR(priv->devfreq); > dev_err(dev, "failed to add devfreq device: %d\n", ret); > - goto err; > } > > - return 0; > - > -err: > - dev_pm_opp_of_remove_table(dev); > return ret; > } > >
Hi, As I commented on patch5, you keep the OPP list on devicetree file and then you better to use the 'suspend_opp' property for setting the highest frequency during suspend/resume. On 21. 9. 14. 오전 2:38, Abel Vesa wrote: > Seems that, in order to be able to resume from suspend, the dram rate > needs to be the highest one available. Therefore, add the late system > suspend/resume PM ops which set the highest rate on suspend and the > latest one used before suspending on resume. > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > --- > drivers/devfreq/imx8m-ddrc.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c > index f18a5c3c1c03..f39741b4a0b0 100644 > --- a/drivers/devfreq/imx8m-ddrc.c > +++ b/drivers/devfreq/imx8m-ddrc.c > @@ -72,6 +72,8 @@ struct imx8m_ddrc { > struct clk *dram_alt; > struct clk *dram_apb; > > + unsigned long suspend_rate; > + unsigned long resume_rate; > int freq_count; > struct imx8m_ddrc_freq freq_table[IMX8M_DDRC_MAX_FREQ_COUNT]; > }; > @@ -271,6 +273,22 @@ static int imx8m_ddrc_target(struct device *dev, unsigned long *freq, u32 flags) > return ret; > } > > +static int imx8m_ddrc_suspend(struct device *dev) > +{ > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > + > + priv->resume_rate = clk_get_rate(priv->dram_core); > + > + return imx8m_ddrc_target(dev, &priv->suspend_rate, 0); > +} > + > +static int imx8m_ddrc_resume(struct device *dev) > +{ > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > + > + return imx8m_ddrc_target(dev, &priv->resume_rate, 0); > +} > + > static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq) > { > struct imx8m_ddrc *priv = dev_get_drvdata(dev); > @@ -324,6 +342,9 @@ static int imx8m_ddrc_init_freq_info(struct device *dev) > > if (dev_pm_opp_add(dev, freq->rate * 250000, 0)) > return -ENODEV; > + > + if (index == 0) > + priv->suspend_rate = freq->rate * 250000; > } > > return 0; > @@ -399,11 +420,16 @@ static const struct of_device_id imx8m_ddrc_of_match[] = { > }; > MODULE_DEVICE_TABLE(of, imx8m_ddrc_of_match); > > +static const struct dev_pm_ops imx8m_ddrc_pm_ops = { > + SET_LATE_SYSTEM_SLEEP_PM_OPS(imx8m_ddrc_suspend, imx8m_ddrc_resume) > +}; > + > static struct platform_driver imx8m_ddrc_platdrv = { > .probe = imx8m_ddrc_probe, > .driver = { > .name = "imx8m-ddrc-devfreq", > - .of_match_table = imx8m_ddrc_of_match, > + .pm = &imx8m_ddrc_pm_ops, > + .of_match_table = of_match_ptr(imx8m_ddrc_of_match), > }, > }; > module_platform_driver(imx8m_ddrc_platdrv); >
On 21. 9. 15. 오후 12:29, Chanwoo Choi wrote: > Hi, > > OPP is mandatory for devfreq driver. Also, must need to add > the OPP levels to devicetree file, it is better to show > the supported OPP list for the developer who don't know > the detailed background of driver. If there are no any > critical issue. I prefer the existing approach for the readability. Also, by keeping the existing approach, the user is able to select the their own OPP entries among the all supported frequencies from EL3. Even if the some clock support the multiple frequencies, the user might want to use the a few frequency instead of using all supported frequencies. > > On 21. 9. 14. 오전 2:38, Abel Vesa wrote: >> i.MX8M platforms get their dram OPPs from the EL3. >> We don't need to duplicate that in the kernel dram dts node. >> We should just trust the OPPs provided by the EL3. >> >> Signed-off-by: Abel Vesa <abel.vesa@nxp.com> >> --- >> drivers/devfreq/imx8m-ddrc.c | 50 +++--------------------------------- >> 1 file changed, 3 insertions(+), 47 deletions(-) >> >> diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c >> index 583123bf2100..f18a5c3c1c03 100644 >> --- a/drivers/devfreq/imx8m-ddrc.c >> +++ b/drivers/devfreq/imx8m-ddrc.c >> @@ -321,38 +321,9 @@ static int imx8m_ddrc_init_freq_info(struct >> device *dev) >> if (freq->dram_core_parent_index == 2 && >> freq->dram_alt_parent_index == 0) >> return -ENODEV; >> - } >> - >> - return 0; >> -} >> - >> -static int imx8m_ddrc_check_opps(struct device *dev) >> -{ >> - struct imx8m_ddrc *priv = dev_get_drvdata(dev); >> - struct imx8m_ddrc_freq *freq_info; >> - struct dev_pm_opp *opp; >> - unsigned long freq; >> - int i, opp_count; >> - >> - /* Enumerate DT OPPs and disable those not supported by firmware */ >> - opp_count = dev_pm_opp_get_opp_count(dev); >> - if (opp_count < 0) >> - return opp_count; >> - for (i = 0, freq = 0; i < opp_count; ++i, ++freq) { >> - opp = dev_pm_opp_find_freq_ceil(dev, &freq); >> - if (IS_ERR(opp)) { >> - dev_err(dev, "Failed enumerating OPPs: %ld\n", >> - PTR_ERR(opp)); >> - return PTR_ERR(opp); >> - } >> - dev_pm_opp_put(opp); >> - freq_info = imx8m_ddrc_find_freq(priv, freq); >> - if (!freq_info) { >> - dev_info(dev, "Disable unsupported OPP %luHz %luMT/s\n", >> - freq, DIV_ROUND_CLOSEST(freq, 250000)); >> - dev_pm_opp_disable(dev, freq); >> - } >> + if (dev_pm_opp_add(dev, freq->rate * 250000, 0)) >> + return -ENODEV; >> } >> return 0; >> @@ -360,7 +331,6 @@ static int imx8m_ddrc_check_opps(struct device *dev) >> static void imx8m_ddrc_exit(struct device *dev) >> { >> - dev_pm_opp_of_remove_table(dev); >> } >> static int imx8m_ddrc_probe(struct platform_device *pdev) >> @@ -407,16 +377,7 @@ static int imx8m_ddrc_probe(struct >> platform_device *pdev) >> return ret; >> } >> - ret = dev_pm_opp_of_add_table(dev); >> - if (ret < 0) { >> - dev_err(dev, "failed to get OPP table\n"); >> - return ret; >> - } >> - >> - ret = imx8m_ddrc_check_opps(dev); >> - if (ret < 0) >> - goto err; >> - >> + priv->profile.polling_ms = 1000; > > This change is not related to role of this patch. > Need to make the separate patch. > >> priv->profile.target = imx8m_ddrc_target; >> priv->profile.exit = imx8m_ddrc_exit; >> priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq; >> @@ -427,13 +388,8 @@ static int imx8m_ddrc_probe(struct >> platform_device *pdev) >> if (IS_ERR(priv->devfreq)) { >> ret = PTR_ERR(priv->devfreq); >> dev_err(dev, "failed to add devfreq device: %d\n", ret); >> - goto err; >> } >> - return 0; >> - >> -err: >> - dev_pm_opp_of_remove_table(dev); >> return ret; >> } >> > >
Am Mittwoch, dem 29.09.2021 um 14:44 +0300 schrieb Abel Vesa: > On 21-09-24 12:20:26, Martin Kepplinger wrote: > > hi Abel, > > > > thank you for the update (this is actually v2 of this RFC right?)! > > > > all in all this runs fine on the imx8mq (Librem 5 and devkit) I > > use. For all > > the pl301 nodes I'm not yet sure what I can actually test / switch > > frequencies. > > > > You can start by looking into each of the following: > > $ ls -1d /sys/devices/platform/soc@0/*/devfreq/*/trans_stat > > and look if the transitions happen when a specific driver that is a > icc user suspends. > > You can also look at: > > /sys/kernel/debug/interconnect/interconnect_summary > > and: > > /sys/kernel/debug/interconnect/interconnect_graph > > > But I still have one problem: lcdif/mxfb already has the > > interconnect dram > > DT property and I use the following call to request bandwidth: > > https://source.puri.sm/martin.kepplinger/linux-next/-/commit/d690e4c021293f938eb2253607f92f5a64f15688 > > (mainlining this is on our todo list). > > > > With your patchset, I get: > > > > [ 0.792960] genirq: Flags mismatch irq 30. 00000004 (mxsfb-drm) > > vs. 00000004 (mxsfb-drm) > > [ 0.801143] mxsfb 30320000.lcd-controller: Failed to install IRQ > > handler > > [ 0.808058] mxsfb: probe of 30320000.lcd-controller failed with > > error -16 > > > > so the main devfreq user (mxsfb) is not there :) why? > > > > OK, I admit, this patchset doesn't provide support for all the icc > consumer drivers. > But that should come at a later stage. I only provided example like > fec and usdhc, to show > how it all fits together. > > > and when I remove the interconnect property from the lcdif DT node, > > mxsfb > > probes again, but of course it doesn't lower dram freq as needed. > > > > Do I do the icc calls wrong in mxsfb despite it working without > > your > > patchset, or may there be something wrong on your side that breaks > > the mxsfb IRQ? > > > > Do you have the following changes into your tree? > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > index 00dd8e39a595..c43a84622af5 > 100644 > > --- > a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > +++ > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > @@ -524,7 +524,7 @@ lcdif: lcd-controller@30320000 > { > <&clk > IMX8MQ_VIDEO_PLL1>, > <&clk > IMX8MQ_VIDEO_PLL1_OUT>; > assigned-clock-rates = <0>, <0>, <0>, > <594000000>; > - interconnects = <&noc > IMX8MQ_ICM_LCDIF &noc IMX8MQ_ICS_DRAM>; > + interconnects = <&icc > IMX8MQ_ICM_LCDIF &icc IMX8MQ_ICS_DRAM>; > interconnect-names = > "dram"; > status = > "disabled"; > > > > @@ -1117,7 +1117,7 @@ mipi_csi1: csi@30a70000 > { > <&src > IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET>, > <&src > IMX8MQ_RESET_MIPI_CSI1_ESC_RESET>; > fsl,mipi-phy-gpr = <&iomuxc_gpr > 0x88>; > - interconnects = <&noc IMX8MQ_ICM_CSI1 > &noc IMX8MQ_ICS_DRAM>; > + interconnects = <&icc IMX8MQ_ICM_CSI1 > &icc IMX8MQ_ICS_DRAM>; > interconnect-names = > "dram"; > status = > "disabled"; > > > > @@ -1169,7 +1169,7 @@ mipi_csi2: csi@30b60000 > { > <&src > IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET>, > <&src > IMX8MQ_RESET_MIPI_CSI2_ESC_RESET>; > fsl,mipi-phy-gpr = <&iomuxc_gpr > 0xa4>; > - interconnects = <&noc IMX8MQ_ICM_CSI2 > &noc IMX8MQ_ICS_DRAM>; > + interconnects = <&icc IMX8MQ_ICM_CSI2 > &icc IMX8MQ_ICS_DRAM>; > interconnect-names = > "dram"; > status = > "disabled"; > > > I forgot to update these in the current version of the patchset. Will > do in the next version. > > Also, would help a lot if you could give me a link to a tree you're > testing with. > That way I can look exactly at what's going on. > > thanks Abel, with the above fix of existing interconnects properties my system runs as expected and here's the output of for each in `ls -1d /sys/devices/platform/soc@0/*/devfreq/*`; do echo $each; cat $each/trans_stat; done for mxsfb requesting (max) bandwith (display on): /sys/devices/platform/soc@0/32700000.noc/devfreq/32700000.noc From : To : 133333333 400000000 800000000 time(ms) 133333333: 0 1 0 624 400000000: 0 0 1 28 * 800000000: 1 0 0 30624 Total transition : 3 /sys/devices/platform/soc@0/3d400000.memory- controller/devfreq/3d400000.memory-controller From : To : 25000000 100000000 800000000 time(ms) 25000000: 0 0 1 620 100000000: 0 0 0 0 * 800000000: 1 0 0 30652 Total transition : 2 /sys/devices/platform/soc@0/soc@0:pl301@0/devfreq/soc@0:pl301@0 From : To : 25000000 133333333 333333333 time(ms) 25000000: 0 0 1 616 133333333: 0 0 0 0 * 333333333: 1 0 0 30668 Total transition : 2 /sys/devices/platform/soc@0/soc@0:pl301@1/devfreq/soc@0:pl301@1 From : To : 25000000 266666666 time(ms) * 25000000: 0 0 31284 266666666: 0 0 0 Total transition : 0 /sys/devices/platform/soc@0/soc@0:pl301@2/devfreq/soc@0:pl301@2 From : To : 25000000 800000000 time(ms) * 25000000: 0 0 31288 800000000: 1 0 0 Total transition : 1 /sys/devices/platform/soc@0/soc@0:pl301@3/devfreq/soc@0:pl301@3 From : To : 25000000 800000000 time(ms) * 25000000: 0 0 31292 800000000: 1 0 0 Total transition : 1 /sys/devices/platform/soc@0/soc@0:pl301@4/devfreq/soc@0:pl301@4 From : To : 25000000 333333333 time(ms) 25000000: 0 1 648 * 333333333: 0 0 30652 Total transition : 1 /sys/devices/platform/soc@0/soc@0:pl301@5/devfreq/soc@0:pl301@5 From : To : 25000000 500000000 time(ms) * 25000000: 0 0 31304 500000000: 1 0 0 Total transition : 1 /sys/devices/platform/soc@0/soc@0:pl301@6/devfreq/soc@0:pl301@6 From : To : 25000000 500000000 time(ms) * 25000000: 0 0 31308 500000000: 0 0 0 Total transition : 0 /sys/devices/platform/soc@0/soc@0:pl301@7/devfreq/soc@0:pl301@7 From : To : 25000000 128000000 500000000 time(ms) * 25000000: 0 0 0 31312 128000000: 0 0 0 0 500000000: 1 0 0 0 Total transition : 1 /sys/devices/platform/soc@0/soc@0:pl301@8/devfreq/soc@0:pl301@8 From : To : 25000000 133333333 time(ms) * 25000000: 0 0 31316 133333333: 0 0 0 Total transition : 0 /sys/devices/platform/soc@0/soc@0:pl301@9/devfreq/soc@0:pl301@9 From : To : 25000000 133333333 266666666 time(ms) 25000000: 0 0 5 1052 133333333: 0 0 0 0 * 266666666: 5 0 0 30268 Total transition : 10 but with display off (mxsfb not requesting anything), I get the same fast freqs for noc and memory-controller. They should use the lowest freqs. Only pl301@4 switches to 25mhz in that case. That's odd. said (still) out-of-tree mxsfb request is https://source.puri.sm/martin.kepplinger/linux-next/-/commit/ee7b1453295932da1e292b734afa7a03651ad9ba and the exact tree I'm running for the above is https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.15-rc3/librem5__integration_byzantium_test_new_devfreq_interconnect thanks, martin
On 21-09-30 10:03:46, Martin Kepplinger wrote: > Am Mittwoch, dem 29.09.2021 um 14:44 +0300 schrieb Abel Vesa: > > On 21-09-24 12:20:26, Martin Kepplinger wrote: > > > hi Abel, > > > > > > thank you for the update (this is actually v2 of this RFC right?)! > > > > > > all in all this runs fine on the imx8mq (Librem 5 and devkit) I > > > use. For all > > > the pl301 nodes I'm not yet sure what I can actually test / switch > > > frequencies. > > > > > > > You can start by looking into each of the following: > > > > $ ls -1d /sys/devices/platform/soc@0/*/devfreq/*/trans_stat > > > > and look if the transitions happen when a specific driver that is a > > icc user suspends. > > > > You can also look at: > > > > /sys/kernel/debug/interconnect/interconnect_summary > > > > and: > > > > /sys/kernel/debug/interconnect/interconnect_graph > > > > > But I still have one problem: lcdif/mxfb already has the > > > interconnect dram > > > DT property and I use the following call to request bandwidth: > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2Fmartin.kepplinger%2Flinux-next%2F-%2Fcommit%2Fd690e4c021293f938eb2253607f92f5a64f15688&data=04%7C01%7Cabel.vesa%40nxp.com%7C7fab8aca3a5f43d56f5608d983e8da67%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637685858400552603%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=%2FzyEQdOLU8jQuUpqJ74GTWyfrDvavz%2BxZAgv1tcIu9Y%3D&reserved=0 > > > (mainlining this is on our todo list). > > > > > > With your patchset, I get: > > > > > > [ 0.792960] genirq: Flags mismatch irq 30. 00000004 (mxsfb-drm) > > > vs. 00000004 (mxsfb-drm) > > > [ 0.801143] mxsfb 30320000.lcd-controller: Failed to install IRQ > > > handler > > > [ 0.808058] mxsfb: probe of 30320000.lcd-controller failed with > > > error -16 > > > > > > so the main devfreq user (mxsfb) is not there :) why? > > > > > > > OK, I admit, this patchset doesn't provide support for all the icc > > consumer drivers. > > But that should come at a later stage. I only provided example like > > fec and usdhc, to show > > how it all fits together. > > > > > and when I remove the interconnect property from the lcdif DT node, > > > mxsfb > > > probes again, but of course it doesn't lower dram freq as needed. > > > > > > Do I do the icc calls wrong in mxsfb despite it working without > > > your > > > patchset, or may there be something wrong on your side that breaks > > > the mxsfb IRQ? > > > > > > > Do you have the following changes into your tree? > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > index 00dd8e39a595..c43a84622af5 > > 100644 > > > > --- > > a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > +++ > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > @@ -524,7 +524,7 @@ lcdif: lcd-controller@30320000 > > { > > <&clk > > IMX8MQ_VIDEO_PLL1>, > > <&clk > > IMX8MQ_VIDEO_PLL1_OUT>; > > assigned-clock-rates = <0>, <0>, <0>, > > <594000000>; > > - interconnects = <&noc > > IMX8MQ_ICM_LCDIF &noc IMX8MQ_ICS_DRAM>; > > + interconnects = <&icc > > IMX8MQ_ICM_LCDIF &icc IMX8MQ_ICS_DRAM>; > > interconnect-names = > > "dram"; > > status = > > "disabled"; > > > > > > > > @@ -1117,7 +1117,7 @@ mipi_csi1: csi@30a70000 > > { > > <&src > > IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET>, > > <&src > > IMX8MQ_RESET_MIPI_CSI1_ESC_RESET>; > > fsl,mipi-phy-gpr = <&iomuxc_gpr > > 0x88>; > > - interconnects = <&noc IMX8MQ_ICM_CSI1 > > &noc IMX8MQ_ICS_DRAM>; > > + interconnects = <&icc IMX8MQ_ICM_CSI1 > > &icc IMX8MQ_ICS_DRAM>; > > interconnect-names = > > "dram"; > > status = > > "disabled"; > > > > > > > > @@ -1169,7 +1169,7 @@ mipi_csi2: csi@30b60000 > > { > > <&src > > IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET>, > > <&src > > IMX8MQ_RESET_MIPI_CSI2_ESC_RESET>; > > fsl,mipi-phy-gpr = <&iomuxc_gpr > > 0xa4>; > > - interconnects = <&noc IMX8MQ_ICM_CSI2 > > &noc IMX8MQ_ICS_DRAM>; > > + interconnects = <&icc IMX8MQ_ICM_CSI2 > > &icc IMX8MQ_ICS_DRAM>; > > interconnect-names = > > "dram"; > > status = > > "disabled"; > > > > > > I forgot to update these in the current version of the patchset. Will > > do in the next version. > > > > Also, would help a lot if you could give me a link to a tree you're > > testing with. > > That way I can look exactly at what's going on. > > > > > > > thanks Abel, with the above fix of existing interconnects properties my > system runs as expected and here's the output of > > for each in `ls -1d /sys/devices/platform/soc@0/*/devfreq/*`; do echo > $each; cat $each/trans_stat; done > > for mxsfb requesting (max) bandwith (display on): > > /sys/devices/platform/soc@0/32700000.noc/devfreq/32700000.noc > From : To > : 133333333 400000000 800000000 time(ms) > 133333333: 0 1 0 624 > 400000000: 0 0 1 28 > * 800000000: 1 0 0 30624 > Total transition : 3 > /sys/devices/platform/soc@0/3d400000.memory- > controller/devfreq/3d400000.memory-controller > From : To > : 25000000 100000000 800000000 time(ms) > 25000000: 0 0 1 620 > 100000000: 0 0 0 0 > * 800000000: 1 0 0 30652 > Total transition : 2 > /sys/devices/platform/soc@0/soc@0:pl301@0/devfreq/soc@0:pl301@0 > From : To > : 25000000 133333333 333333333 time(ms) > 25000000: 0 0 1 616 > 133333333: 0 0 0 0 > * 333333333: 1 0 0 30668 > Total transition : 2 > /sys/devices/platform/soc@0/soc@0:pl301@1/devfreq/soc@0:pl301@1 > From : To > : 25000000 266666666 time(ms) > * 25000000: 0 0 31284 > 266666666: 0 0 0 > Total transition : 0 > /sys/devices/platform/soc@0/soc@0:pl301@2/devfreq/soc@0:pl301@2 > From : To > : 25000000 800000000 time(ms) > * 25000000: 0 0 31288 > 800000000: 1 0 0 > Total transition : 1 > /sys/devices/platform/soc@0/soc@0:pl301@3/devfreq/soc@0:pl301@3 > From : To > : 25000000 800000000 time(ms) > * 25000000: 0 0 31292 > 800000000: 1 0 0 > Total transition : 1 > /sys/devices/platform/soc@0/soc@0:pl301@4/devfreq/soc@0:pl301@4 > From : To > : 25000000 333333333 time(ms) > 25000000: 0 1 648 > * 333333333: 0 0 30652 > Total transition : 1 > /sys/devices/platform/soc@0/soc@0:pl301@5/devfreq/soc@0:pl301@5 > From : To > : 25000000 500000000 time(ms) > * 25000000: 0 0 31304 > 500000000: 1 0 0 > Total transition : 1 > /sys/devices/platform/soc@0/soc@0:pl301@6/devfreq/soc@0:pl301@6 > From : To > : 25000000 500000000 time(ms) > * 25000000: 0 0 31308 > 500000000: 0 0 0 > Total transition : 0 > /sys/devices/platform/soc@0/soc@0:pl301@7/devfreq/soc@0:pl301@7 > From : To > : 25000000 128000000 500000000 time(ms) > * 25000000: 0 0 0 31312 > 128000000: 0 0 0 0 > 500000000: 1 0 0 0 > Total transition : 1 > /sys/devices/platform/soc@0/soc@0:pl301@8/devfreq/soc@0:pl301@8 > From : To > : 25000000 133333333 time(ms) > * 25000000: 0 0 31316 > 133333333: 0 0 0 > Total transition : 0 > /sys/devices/platform/soc@0/soc@0:pl301@9/devfreq/soc@0:pl301@9 > From : To > : 25000000 133333333 266666666 time(ms) > 25000000: 0 0 5 1052 > 133333333: 0 0 0 0 > * 266666666: 5 0 0 30268 > Total transition : 10 > > > but with display off (mxsfb not requesting anything), I get the same > fast freqs for noc and memory-controller. They should use the lowest > freqs. Only pl301@4 switches to 25mhz in that case. That's odd. > Well, have a look at: /sys/devices/platform/soc@0/soc@0:pl301@9/devfreq/soc@0:pl301@9 even in the output you gave here, you can see that there are 5 transisions between 25MHz and 266MHz. BTW, that is the USDHC pl301. I'm assuming you're booting with rootfs from usdhc not through nfs, right? Anyway, the noc and dram clocks rate only drop when there is no user enabling its own icc path to the dram. Keep in mind that the benefit of this approach is not only to drop the dram clock rate, but also to drop the rates of all the bus clocks on whenever possible. Yes, the perfect scenario would be, from power consumption point of view at least, have dram clock rate as low as possible and as long as possible, which implicitly means there is no one requesting the higher rate. If you want to observe the transitions number change for the dram devfreq node as well, you can run a simple sync from userspace and that will trigger a "high rate" request for the usdhc. Note, this will only happen if there are no other users asking for the higher rate. > said (still) out-of-tree mxsfb request is > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2Fmartin.kepplinger%2Flinux-next%2F-%2Fcommit%2Fee7b1453295932da1e292b734afa7a03651ad9ba&data=04%7C01%7Cabel.vesa%40nxp.com%7C7fab8aca3a5f43d56f5608d983e8da67%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637685858400552603%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=tY9IN8nAgYrPRD0BRLW%2FZbWEps9DTVIQi8G5jY5aw3Q%3D&reserved=0 > > and the exact tree I'm running for the above is > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2Fmartin.kepplinger%2Flinux-next%2F-%2Fcommits%2F5.15-rc3%2Flibrem5__integration_byzantium_test_new_devfreq_interconnect&data=04%7C01%7Cabel.vesa%40nxp.com%7C7fab8aca3a5f43d56f5608d983e8da67%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637685858400552603%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Kpo7sVLdgzwMv8MPX7X%2FNUxoLcvWjFVIuerlh7Cr2D4%3D&reserved=0 > > thanks, > > martin >
Am Donnerstag, dem 30.09.2021 um 17:22 +0300 schrieb Abel Vesa: > On 21-09-30 10:03:46, Martin Kepplinger wrote: > > Am Mittwoch, dem 29.09.2021 um 14:44 +0300 schrieb Abel Vesa: > > > On 21-09-24 12:20:26, Martin Kepplinger wrote: > > > > hi Abel, > > > > > > > > thank you for the update (this is actually v2 of this RFC > > > > right?)! > > > > > > > > all in all this runs fine on the imx8mq (Librem 5 and devkit) I > > > > use. For all > > > > the pl301 nodes I'm not yet sure what I can actually test / > > > > switch > > > > frequencies. > > > > > > > > > > You can start by looking into each of the following: > > > > > > $ ls -1d /sys/devices/platform/soc@0/*/devfreq/*/trans_stat > > > > > > and look if the transitions happen when a specific driver that is > > > a > > > icc user suspends. > > > > > > You can also look at: > > > > > > /sys/kernel/debug/interconnect/interconnect_summary > > > > > > and: > > > > > > /sys/kernel/debug/interconnect/interconnect_graph > > > > > > > But I still have one problem: lcdif/mxfb already has the > > > > interconnect dram > > > > DT property and I use the following call to request bandwidth: > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2Fmartin.kepplinger%2Flinux-next%2F-%2Fcommit%2Fd690e4c021293f938eb2253607f92f5a64f15688&data=04%7C01%7Cabel.vesa%40nxp.com%7C7fab8aca3a5f43d56f5608d983e8da67%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637685858400552603%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=%2FzyEQdOLU8jQuUpqJ74GTWyfrDvavz%2BxZAgv1tcIu9Y%3D&reserved=0 > > > > (mainlining this is on our todo list). > > > > > > > > With your patchset, I get: > > > > > > > > [ 0.792960] genirq: Flags mismatch irq 30. 00000004 (mxsfb- > > > > drm) > > > > vs. 00000004 (mxsfb-drm) > > > > [ 0.801143] mxsfb 30320000.lcd-controller: Failed to install > > > > IRQ > > > > handler > > > > [ 0.808058] mxsfb: probe of 30320000.lcd-controller failed > > > > with > > > > error -16 > > > > > > > > so the main devfreq user (mxsfb) is not there :) why? > > > > > > > > > > OK, I admit, this patchset doesn't provide support for all the > > > icc > > > consumer drivers. > > > But that should come at a later stage. I only provided example > > > like > > > fec and usdhc, to show > > > how it all fits together. > > > > > > > and when I remove the interconnect property from the lcdif DT > > > > node, > > > > mxsfb > > > > probes again, but of course it doesn't lower dram freq as > > > > needed. > > > > > > > > Do I do the icc calls wrong in mxsfb despite it working without > > > > your > > > > patchset, or may there be something wrong on your side that > > > > breaks > > > > the mxsfb IRQ? > > > > > > > > > > Do you have the following changes into your tree? > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > index 00dd8e39a595..c43a84622af5 > > > 100644 > > > > > > > > > --- > > > a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > > > > > +++ > > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > > > > > > > @@ -524,7 +524,7 @@ lcdif: lcd-controller@30320000 > > > { > > > <&clk > > > IMX8MQ_VIDEO_PLL1>, > > > <&clk > > > IMX8MQ_VIDEO_PLL1_OUT>; > > > assigned-clock-rates = <0>, <0>, > > > <0>, > > > <594000000>; > > > - interconnects = <&noc > > > IMX8MQ_ICM_LCDIF &noc IMX8MQ_ICS_DRAM>; > > > + interconnects = <&icc > > > IMX8MQ_ICM_LCDIF &icc IMX8MQ_ICS_DRAM>; > > > interconnect-names = > > > "dram"; > > > status = > > > "disabled"; > > > > > > > > > > > > > > > > > > @@ -1117,7 +1117,7 @@ mipi_csi1: csi@30a70000 > > > { > > > > > > <&src > > > IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET>, > > > <&src > > > IMX8MQ_RESET_MIPI_CSI1_ESC_RESET>; > > > fsl,mipi-phy-gpr = <&iomuxc_gpr > > > 0x88>; > > > - interconnects = <&noc > > > IMX8MQ_ICM_CSI1 > > > &noc IMX8MQ_ICS_DRAM>; > > > + interconnects = <&icc > > > IMX8MQ_ICM_CSI1 > > > &icc IMX8MQ_ICS_DRAM>; > > > interconnect-names = > > > "dram"; > > > status = > > > "disabled"; > > > > > > > > > > > > > > > > > > @@ -1169,7 +1169,7 @@ mipi_csi2: csi@30b60000 > > > { > > > > > > <&src > > > IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET>, > > > <&src > > > IMX8MQ_RESET_MIPI_CSI2_ESC_RESET>; > > > fsl,mipi-phy-gpr = <&iomuxc_gpr > > > 0xa4>; > > > - interconnects = <&noc > > > IMX8MQ_ICM_CSI2 > > > &noc IMX8MQ_ICS_DRAM>; > > > + interconnects = <&icc > > > IMX8MQ_ICM_CSI2 > > > &icc IMX8MQ_ICS_DRAM>; > > > interconnect-names = > > > "dram"; > > > status = > > > "disabled"; > > > > > > > > > > > > I forgot to update these in the current version of the patchset. > > > Will > > > do in the next version. > > > > > > Also, would help a lot if you could give me a link to a tree > > > you're > > > testing with. > > > That way I can look exactly at what's going on. > > > > > > > > > > > > thanks Abel, with the above fix of existing interconnects > > properties my > > system runs as expected and here's the output of > > > > for each in `ls -1d /sys/devices/platform/soc@0/*/devfreq/*`; do > > echo > > $each; cat $each/trans_stat; done > > > > for mxsfb requesting (max) bandwith (display on): > > > > /sys/devices/platform/soc@0/32700000.noc/devfreq/32700000.noc > > From : To > > : 133333333 400000000 800000000 time(ms) > > 133333333: 0 1 0 624 > > 400000000: 0 0 1 28 > > * 800000000: 1 0 0 30624 > > Total transition : 3 > > /sys/devices/platform/soc@0/3d400000.memory- > > controller/devfreq/3d400000.memory-controller > > From : To > > : 25000000 100000000 800000000 time(ms) > > 25000000: 0 0 1 620 > > 100000000: 0 0 0 0 > > * 800000000: 1 0 0 30652 > > Total transition : 2 > > /sys/devices/platform/soc@0/soc@0:pl301@0/devfreq/soc@0:pl301@0 > > From : To > > : 25000000 133333333 333333333 time(ms) > > 25000000: 0 0 1 616 > > 133333333: 0 0 0 0 > > * 333333333: 1 0 0 30668 > > Total transition : 2 > > /sys/devices/platform/soc@0/soc@0:pl301@1/devfreq/soc@0:pl301@1 > > From : To > > : 25000000 266666666 time(ms) > > * 25000000: 0 0 31284 > > 266666666: 0 0 0 > > Total transition : 0 > > /sys/devices/platform/soc@0/soc@0:pl301@2/devfreq/soc@0:pl301@2 > > From : To > > : 25000000 800000000 time(ms) > > * 25000000: 0 0 31288 > > 800000000: 1 0 0 > > Total transition : 1 > > /sys/devices/platform/soc@0/soc@0:pl301@3/devfreq/soc@0:pl301@3 > > From : To > > : 25000000 800000000 time(ms) > > * 25000000: 0 0 31292 > > 800000000: 1 0 0 > > Total transition : 1 > > /sys/devices/platform/soc@0/soc@0:pl301@4/devfreq/soc@0:pl301@4 > > From : To > > : 25000000 333333333 time(ms) > > 25000000: 0 1 648 > > * 333333333: 0 0 30652 > > Total transition : 1 > > /sys/devices/platform/soc@0/soc@0:pl301@5/devfreq/soc@0:pl301@5 > > From : To > > : 25000000 500000000 time(ms) > > * 25000000: 0 0 31304 > > 500000000: 1 0 0 > > Total transition : 1 > > /sys/devices/platform/soc@0/soc@0:pl301@6/devfreq/soc@0:pl301@6 > > From : To > > : 25000000 500000000 time(ms) > > * 25000000: 0 0 31308 > > 500000000: 0 0 0 > > Total transition : 0 > > /sys/devices/platform/soc@0/soc@0:pl301@7/devfreq/soc@0:pl301@7 > > From : To > > : 25000000 128000000 500000000 time(ms) > > * 25000000: 0 0 0 31312 > > 128000000: 0 0 0 0 > > 500000000: 1 0 0 0 > > Total transition : 1 > > /sys/devices/platform/soc@0/soc@0:pl301@8/devfreq/soc@0:pl301@8 > > From : To > > : 25000000 133333333 time(ms) > > * 25000000: 0 0 31316 > > 133333333: 0 0 0 > > Total transition : 0 > > /sys/devices/platform/soc@0/soc@0:pl301@9/devfreq/soc@0:pl301@9 > > From : To > > : 25000000 133333333 266666666 time(ms) > > 25000000: 0 0 5 1052 > > 133333333: 0 0 0 0 > > * 266666666: 5 0 0 30268 > > Total transition : 10 > > > > > > but with display off (mxsfb not requesting anything), I get the > > same > > fast freqs for noc and memory-controller. They should use the > > lowest > > freqs. Only pl301@4 switches to 25mhz in that case. That's odd. > > > > Well, have a look at: > > /sys/devices/platform/soc@0/soc@0:pl301@9/devfreq/soc@0:pl301@9 > > even in the output you gave here, you can see that there are 5 > transisions between 25MHz and 266MHz. BTW, that is the USDHC pl301. > > I'm assuming you're booting with rootfs from usdhc not through nfs, > right? Anyway, the noc and dram clocks rate only drop when there is > no user enabling its own icc path to the dram. > > Keep in mind that the benefit of this approach is not only to drop > the > dram clock rate, but also to drop the rates of all the bus clocks on > whenever possible. > > Yes, the perfect scenario would be, from power consumption point of > view at least, > have dram clock rate as low as possible and as long as possible, > which > implicitly means there is no one requesting the higher rate. > > If you want to observe the transitions number change for the dram > devfreq node as well, you can run a simple sync from userspace and > that will > trigger a "high rate" request for the usdhc. Note, this will only > happen > if there are no other users asking for the higher rate. correct, I boot from usdhc. and when I disable interconnect in usdhc the behaviour actually makes sense. tree: https://source.puri.sm/martin.kepplinger/linux-next/-/commits/5.15-rc4/librem5__integration_byzantium_new_devfreq_interconnect And I can see the system using a bit less power in the "display off" case now (and various freqs switching to the lowest). I didn't yet test whether the new "consumers" (for example usb) correctly request more bandwidth now. The only thing I see is with the "display on" case, that "32700000.interconnect" is switched to 800mhz now, where it used 400mhz before this patchset. I should be able to find out why though :) so, for a proof of concept (and after what you mentioned to change for a next revision) this looks good to me. thanks a lot! martin
On Mon, Sep 13, 2021 at 12:39 PM Abel Vesa <abel.vesa@nxp.com> wrote: > > Now that the imx generic interconnect doesn't use the > imx_icc_node_adj_desc, we remove it from all the i.MX8M > platform drivers. > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > --- > drivers/interconnect/imx/imx.h | 19 ++++------------- > drivers/interconnect/imx/imx8mm.c | 32 +++++++++------------------- > drivers/interconnect/imx/imx8mn.c | 28 +++++++------------------ > drivers/interconnect/imx/imx8mq.c | 35 ++++++++++--------------------- > 4 files changed, 33 insertions(+), 81 deletions(-) > I noticed there are no interconnect nodes in the imx8mm nor the imx8mn, so it appears to me like the mini and nano code won't do anything. adam > diff --git a/drivers/interconnect/imx/imx.h b/drivers/interconnect/imx/imx.h > index 75da51076c68..5c9f5138f6aa 100644 > --- a/drivers/interconnect/imx/imx.h > +++ b/drivers/interconnect/imx/imx.h > @@ -14,15 +14,6 @@ > > #define IMX_ICC_MAX_LINKS 4 > > -/* > - * struct imx_icc_node_adj - Describe a dynamic adjustable node > - */ > -struct imx_icc_node_adj_desc { > - unsigned int bw_mul, bw_div; > - const char *phandle_name; > - bool main_noc; > -}; > - > /* > * struct imx_icc_node - Describe an interconnect node > * @name: name of the node > @@ -35,23 +26,21 @@ struct imx_icc_node_desc { > u16 id; > u16 links[IMX_ICC_MAX_LINKS]; > u16 num_links; > - const struct imx_icc_node_adj_desc *adj; > }; > > -#define DEFINE_BUS_INTERCONNECT(_name, _id, _adj, ...) \ > +#define DEFINE_BUS_INTERCONNECT(_name, _id, ...) \ > { \ > .id = _id, \ > .name = _name, \ > - .adj = _adj, \ > .num_links = ARRAY_SIZE(((int[]){ __VA_ARGS__ })), \ > .links = { __VA_ARGS__ }, \ > } > > #define DEFINE_BUS_MASTER(_name, _id, _dest_id) \ > - DEFINE_BUS_INTERCONNECT(_name, _id, NULL, _dest_id) > + DEFINE_BUS_INTERCONNECT(_name, _id, _dest_id) > > -#define DEFINE_BUS_SLAVE(_name, _id, _adj) \ > - DEFINE_BUS_INTERCONNECT(_name, _id, _adj) > +#define DEFINE_BUS_SLAVE(_name, _id) \ > + DEFINE_BUS_INTERCONNECT(_name, _id) > > int imx_icc_register(struct platform_device *pdev, > struct imx_icc_node_desc *nodes, > diff --git a/drivers/interconnect/imx/imx8mm.c b/drivers/interconnect/imx/imx8mm.c > index 1083490bb391..0c16110bef9d 100644 > --- a/drivers/interconnect/imx/imx8mm.c > +++ b/drivers/interconnect/imx/imx8mm.c > @@ -14,18 +14,6 @@ > > #include "imx.h" > > -static const struct imx_icc_node_adj_desc imx8mm_dram_adj = { > - .bw_mul = 1, > - .bw_div = 16, > - .phandle_name = "fsl,ddrc", > -}; > - > -static const struct imx_icc_node_adj_desc imx8mm_noc_adj = { > - .bw_mul = 1, > - .bw_div = 16, > - .main_noc = true, > -}; > - > /* > * Describe bus masters, slaves and connections between them > * > @@ -33,43 +21,43 @@ static const struct imx_icc_node_adj_desc imx8mm_noc_adj = { > * PL301 nics which are skipped/merged into PL301_MAIN > */ > static struct imx_icc_node_desc nodes[] = { > - DEFINE_BUS_INTERCONNECT("NOC", IMX8MM_ICN_NOC, &imx8mm_noc_adj, > + DEFINE_BUS_INTERCONNECT("NOC", IMX8MM_ICN_NOC, > IMX8MM_ICS_DRAM, IMX8MM_ICN_MAIN), > > - DEFINE_BUS_SLAVE("DRAM", IMX8MM_ICS_DRAM, &imx8mm_dram_adj), > - DEFINE_BUS_SLAVE("OCRAM", IMX8MM_ICS_OCRAM, NULL), > + DEFINE_BUS_SLAVE("DRAM", IMX8MM_ICS_DRAM), > + DEFINE_BUS_SLAVE("OCRAM", IMX8MM_ICS_OCRAM), > DEFINE_BUS_MASTER("A53", IMX8MM_ICM_A53, IMX8MM_ICN_NOC), > > /* VPUMIX */ > DEFINE_BUS_MASTER("VPU H1", IMX8MM_ICM_VPU_H1, IMX8MM_ICN_VIDEO), > DEFINE_BUS_MASTER("VPU G1", IMX8MM_ICM_VPU_G1, IMX8MM_ICN_VIDEO), > DEFINE_BUS_MASTER("VPU G2", IMX8MM_ICM_VPU_G2, IMX8MM_ICN_VIDEO), > - DEFINE_BUS_INTERCONNECT("PL301_VIDEO", IMX8MM_ICN_VIDEO, NULL, IMX8MM_ICN_NOC), > + DEFINE_BUS_INTERCONNECT("PL301_VIDEO", IMX8MM_ICN_VIDEO, IMX8MM_ICN_NOC), > > /* GPUMIX */ > DEFINE_BUS_MASTER("GPU 2D", IMX8MM_ICM_GPU2D, IMX8MM_ICN_GPU), > DEFINE_BUS_MASTER("GPU 3D", IMX8MM_ICM_GPU3D, IMX8MM_ICN_GPU), > - DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MM_ICN_GPU, NULL, IMX8MM_ICN_NOC), > + DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MM_ICN_GPU, IMX8MM_ICN_NOC), > > /* DISPLAYMIX */ > DEFINE_BUS_MASTER("CSI", IMX8MM_ICM_CSI, IMX8MM_ICN_MIPI), > DEFINE_BUS_MASTER("LCDIF", IMX8MM_ICM_LCDIF, IMX8MM_ICN_MIPI), > - DEFINE_BUS_INTERCONNECT("PL301_MIPI", IMX8MM_ICN_MIPI, NULL, IMX8MM_ICN_NOC), > + DEFINE_BUS_INTERCONNECT("PL301_MIPI", IMX8MM_ICN_MIPI, IMX8MM_ICN_NOC), > > /* HSIO */ > DEFINE_BUS_MASTER("USB1", IMX8MM_ICM_USB1, IMX8MM_ICN_HSIO), > DEFINE_BUS_MASTER("USB2", IMX8MM_ICM_USB2, IMX8MM_ICN_HSIO), > DEFINE_BUS_MASTER("PCIE", IMX8MM_ICM_PCIE, IMX8MM_ICN_HSIO), > - DEFINE_BUS_INTERCONNECT("PL301_HSIO", IMX8MM_ICN_HSIO, NULL, IMX8MM_ICN_NOC), > + DEFINE_BUS_INTERCONNECT("PL301_HSIO", IMX8MM_ICN_HSIO, IMX8MM_ICN_NOC), > > /* Audio */ > DEFINE_BUS_MASTER("SDMA2", IMX8MM_ICM_SDMA2, IMX8MM_ICN_AUDIO), > DEFINE_BUS_MASTER("SDMA3", IMX8MM_ICM_SDMA3, IMX8MM_ICN_AUDIO), > - DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MM_ICN_AUDIO, NULL, IMX8MM_ICN_MAIN), > + DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MM_ICN_AUDIO, IMX8MM_ICN_MAIN), > > /* Ethernet */ > DEFINE_BUS_MASTER("ENET", IMX8MM_ICM_ENET, IMX8MM_ICN_ENET), > - DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MM_ICN_ENET, NULL, IMX8MM_ICN_MAIN), > + DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MM_ICN_ENET, IMX8MM_ICN_MAIN), > > /* Other */ > DEFINE_BUS_MASTER("SDMA1", IMX8MM_ICM_SDMA1, IMX8MM_ICN_MAIN), > @@ -77,7 +65,7 @@ static struct imx_icc_node_desc nodes[] = { > DEFINE_BUS_MASTER("USDHC1", IMX8MM_ICM_USDHC1, IMX8MM_ICN_MAIN), > DEFINE_BUS_MASTER("USDHC2", IMX8MM_ICM_USDHC2, IMX8MM_ICN_MAIN), > DEFINE_BUS_MASTER("USDHC3", IMX8MM_ICM_USDHC3, IMX8MM_ICN_MAIN), > - DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MM_ICN_MAIN, NULL, > + DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MM_ICN_MAIN, > IMX8MM_ICN_NOC, IMX8MM_ICS_OCRAM), > }; > > diff --git a/drivers/interconnect/imx/imx8mn.c b/drivers/interconnect/imx/imx8mn.c > index ad97e55fd4e5..8d16bd5cf006 100644 > --- a/drivers/interconnect/imx/imx8mn.c > +++ b/drivers/interconnect/imx/imx8mn.c > @@ -11,18 +11,6 @@ > > #include "imx.h" > > -static const struct imx_icc_node_adj_desc imx8mn_dram_adj = { > - .bw_mul = 1, > - .bw_div = 4, > - .phandle_name = "fsl,ddrc", > -}; > - > -static const struct imx_icc_node_adj_desc imx8mn_noc_adj = { > - .bw_mul = 1, > - .bw_div = 4, > - .main_noc = true, > -}; > - > /* > * Describe bus masters, slaves and connections between them > * > @@ -30,23 +18,23 @@ static const struct imx_icc_node_adj_desc imx8mn_noc_adj = { > * PL301 nics which are skipped/merged into PL301_MAIN > */ > static struct imx_icc_node_desc nodes[] = { > - DEFINE_BUS_INTERCONNECT("NOC", IMX8MN_ICN_NOC, &imx8mn_noc_adj, > + DEFINE_BUS_INTERCONNECT("NOC", IMX8MN_ICN_NOC, > IMX8MN_ICS_DRAM, IMX8MN_ICN_MAIN), > > - DEFINE_BUS_SLAVE("DRAM", IMX8MN_ICS_DRAM, &imx8mn_dram_adj), > - DEFINE_BUS_SLAVE("OCRAM", IMX8MN_ICS_OCRAM, NULL), > + DEFINE_BUS_SLAVE("DRAM", IMX8MN_ICS_DRAM), > + DEFINE_BUS_SLAVE("OCRAM", IMX8MN_ICS_OCRAM), > DEFINE_BUS_MASTER("A53", IMX8MN_ICM_A53, IMX8MN_ICN_NOC), > > /* GPUMIX */ > DEFINE_BUS_MASTER("GPU", IMX8MN_ICM_GPU, IMX8MN_ICN_GPU), > - DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MN_ICN_GPU, NULL, IMX8MN_ICN_NOC), > + DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MN_ICN_GPU, IMX8MN_ICN_NOC), > > /* DISPLAYMIX */ > DEFINE_BUS_MASTER("CSI1", IMX8MN_ICM_CSI1, IMX8MN_ICN_MIPI), > DEFINE_BUS_MASTER("CSI2", IMX8MN_ICM_CSI2, IMX8MN_ICN_MIPI), > DEFINE_BUS_MASTER("ISI", IMX8MN_ICM_ISI, IMX8MN_ICN_MIPI), > DEFINE_BUS_MASTER("LCDIF", IMX8MN_ICM_LCDIF, IMX8MN_ICN_MIPI), > - DEFINE_BUS_INTERCONNECT("PL301_MIPI", IMX8MN_ICN_MIPI, NULL, IMX8MN_ICN_NOC), > + DEFINE_BUS_INTERCONNECT("PL301_MIPI", IMX8MN_ICN_MIPI, IMX8MN_ICN_NOC), > > /* USB goes straight to NOC */ > DEFINE_BUS_MASTER("USB", IMX8MN_ICM_USB, IMX8MN_ICN_NOC), > @@ -54,11 +42,11 @@ static struct imx_icc_node_desc nodes[] = { > /* Audio */ > DEFINE_BUS_MASTER("SDMA2", IMX8MN_ICM_SDMA2, IMX8MN_ICN_AUDIO), > DEFINE_BUS_MASTER("SDMA3", IMX8MN_ICM_SDMA3, IMX8MN_ICN_AUDIO), > - DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MN_ICN_AUDIO, NULL, IMX8MN_ICN_MAIN), > + DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MN_ICN_AUDIO, IMX8MN_ICN_MAIN), > > /* Ethernet */ > DEFINE_BUS_MASTER("ENET", IMX8MN_ICM_ENET, IMX8MN_ICN_ENET), > - DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MN_ICN_ENET, NULL, IMX8MN_ICN_MAIN), > + DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MN_ICN_ENET, IMX8MN_ICN_MAIN), > > /* Other */ > DEFINE_BUS_MASTER("SDMA1", IMX8MN_ICM_SDMA1, IMX8MN_ICN_MAIN), > @@ -66,7 +54,7 @@ static struct imx_icc_node_desc nodes[] = { > DEFINE_BUS_MASTER("USDHC1", IMX8MN_ICM_USDHC1, IMX8MN_ICN_MAIN), > DEFINE_BUS_MASTER("USDHC2", IMX8MN_ICM_USDHC2, IMX8MN_ICN_MAIN), > DEFINE_BUS_MASTER("USDHC3", IMX8MN_ICM_USDHC3, IMX8MN_ICN_MAIN), > - DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MN_ICN_MAIN, NULL, > + DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MN_ICN_MAIN > IMX8MN_ICN_NOC, IMX8MN_ICS_OCRAM), > }; > > diff --git a/drivers/interconnect/imx/imx8mq.c b/drivers/interconnect/imx/imx8mq.c > index d7768d3c6d8a..b8c36d668946 100644 > --- a/drivers/interconnect/imx/imx8mq.c > +++ b/drivers/interconnect/imx/imx8mq.c > @@ -12,18 +12,6 @@ > > #include "imx.h" > > -static const struct imx_icc_node_adj_desc imx8mq_dram_adj = { > - .bw_mul = 1, > - .bw_div = 4, > - .phandle_name = "fsl,ddrc", > -}; > - > -static const struct imx_icc_node_adj_desc imx8mq_noc_adj = { > - .bw_mul = 1, > - .bw_div = 4, > - .main_noc = true, > -}; > - > /* > * Describe bus masters, slaves and connections between them > * > @@ -31,43 +19,42 @@ static const struct imx_icc_node_adj_desc imx8mq_noc_adj = { > * PL301 nics which are skipped/merged into PL301_MAIN > */ > static struct imx_icc_node_desc nodes[] = { > - DEFINE_BUS_INTERCONNECT("NOC", IMX8MQ_ICN_NOC, &imx8mq_noc_adj, > - IMX8MQ_ICS_DRAM, IMX8MQ_ICN_MAIN), > + DEFINE_BUS_INTERCONNECT("NOC", IMX8MQ_ICN_NOC, IMX8MQ_ICS_DRAM, IMX8MQ_ICN_MAIN), > > - DEFINE_BUS_SLAVE("DRAM", IMX8MQ_ICS_DRAM, &imx8mq_dram_adj), > - DEFINE_BUS_SLAVE("OCRAM", IMX8MQ_ICS_OCRAM, NULL), > + DEFINE_BUS_SLAVE("DRAM", IMX8MQ_ICS_DRAM), > + DEFINE_BUS_SLAVE("OCRAM", IMX8MQ_ICS_OCRAM), > DEFINE_BUS_MASTER("A53", IMX8MQ_ICM_A53, IMX8MQ_ICN_NOC), > > /* VPUMIX */ > DEFINE_BUS_MASTER("VPU", IMX8MQ_ICM_VPU, IMX8MQ_ICN_VIDEO), > - DEFINE_BUS_INTERCONNECT("PL301_VIDEO", IMX8MQ_ICN_VIDEO, NULL, IMX8MQ_ICN_NOC), > + DEFINE_BUS_INTERCONNECT("PL301_VIDEO", IMX8MQ_ICN_VIDEO, IMX8MQ_ICN_NOC), > > /* GPUMIX */ > DEFINE_BUS_MASTER("GPU", IMX8MQ_ICM_GPU, IMX8MQ_ICN_GPU), > - DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MQ_ICN_GPU, NULL, IMX8MQ_ICN_NOC), > + DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MQ_ICN_GPU, IMX8MQ_ICN_NOC), > > /* DISPMIX (only for DCSS) */ > DEFINE_BUS_MASTER("DC", IMX8MQ_ICM_DCSS, IMX8MQ_ICN_DCSS), > - DEFINE_BUS_INTERCONNECT("PL301_DC", IMX8MQ_ICN_DCSS, NULL, IMX8MQ_ICN_NOC), > + DEFINE_BUS_INTERCONNECT("PL301_DC", IMX8MQ_ICN_DCSS, IMX8MQ_ICN_NOC), > > /* USBMIX */ > DEFINE_BUS_MASTER("USB1", IMX8MQ_ICM_USB1, IMX8MQ_ICN_USB), > DEFINE_BUS_MASTER("USB2", IMX8MQ_ICM_USB2, IMX8MQ_ICN_USB), > - DEFINE_BUS_INTERCONNECT("PL301_USB", IMX8MQ_ICN_USB, NULL, IMX8MQ_ICN_NOC), > + DEFINE_BUS_INTERCONNECT("PL301_USB", IMX8MQ_ICN_USB, IMX8MQ_ICN_NOC), > > /* PL301_DISPLAY (IPs other than DCSS, inside SUPERMIX) */ > DEFINE_BUS_MASTER("CSI1", IMX8MQ_ICM_CSI1, IMX8MQ_ICN_DISPLAY), > DEFINE_BUS_MASTER("CSI2", IMX8MQ_ICM_CSI2, IMX8MQ_ICN_DISPLAY), > DEFINE_BUS_MASTER("LCDIF", IMX8MQ_ICM_LCDIF, IMX8MQ_ICN_DISPLAY), > - DEFINE_BUS_INTERCONNECT("PL301_DISPLAY", IMX8MQ_ICN_DISPLAY, NULL, IMX8MQ_ICN_MAIN), > + DEFINE_BUS_INTERCONNECT("PL301_DISPLAY", IMX8MQ_ICN_DISPLAY, IMX8MQ_ICN_MAIN), > > /* AUDIO */ > DEFINE_BUS_MASTER("SDMA2", IMX8MQ_ICM_SDMA2, IMX8MQ_ICN_AUDIO), > - DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MQ_ICN_AUDIO, NULL, IMX8MQ_ICN_DISPLAY), > + DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MQ_ICN_AUDIO, IMX8MQ_ICN_DISPLAY), > > /* ENET */ > DEFINE_BUS_MASTER("ENET", IMX8MQ_ICM_ENET, IMX8MQ_ICN_ENET), > - DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MQ_ICN_ENET, NULL, IMX8MQ_ICN_MAIN), > + DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MQ_ICN_ENET, IMX8MQ_ICN_MAIN), > > /* OTHER */ > DEFINE_BUS_MASTER("SDMA1", IMX8MQ_ICM_SDMA1, IMX8MQ_ICN_MAIN), > @@ -76,7 +63,7 @@ static struct imx_icc_node_desc nodes[] = { > DEFINE_BUS_MASTER("USDHC2", IMX8MQ_ICM_USDHC2, IMX8MQ_ICN_MAIN), > DEFINE_BUS_MASTER("PCIE1", IMX8MQ_ICM_PCIE1, IMX8MQ_ICN_MAIN), > DEFINE_BUS_MASTER("PCIE2", IMX8MQ_ICM_PCIE2, IMX8MQ_ICN_MAIN), > - DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MQ_ICN_MAIN, NULL, > + DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MQ_ICN_MAIN, > IMX8MQ_ICN_NOC, IMX8MQ_ICS_OCRAM), > }; > > -- > 2.31.1 >
On 21-10-18 07:41:31, Adam Ford wrote: > On Mon, Sep 13, 2021 at 12:39 PM Abel Vesa <abel.vesa@nxp.com> wrote: > > > > Now that the imx generic interconnect doesn't use the > > imx_icc_node_adj_desc, we remove it from all the i.MX8M > > platform drivers. > > > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > > --- > > drivers/interconnect/imx/imx.h | 19 ++++------------- > > drivers/interconnect/imx/imx8mm.c | 32 +++++++++------------------- > > drivers/interconnect/imx/imx8mn.c | 28 +++++++------------------ > > drivers/interconnect/imx/imx8mq.c | 35 ++++++++++--------------------- > > 4 files changed, 33 insertions(+), 81 deletions(-) > > > > I noticed there are no interconnect nodes in the imx8mm nor the > imx8mn, so it appears to me like the mini and nano code won't do > anything. > icc + imx-bus + imx8m-ddrc is an approach to have support for dropping the bus and ddr clock rates when there is no user needing the higher rates. This used to be done in our NXP tree via something called busfreq. The problem with busfreq is that it cannot be upstreamed and it's quite limited. As of now, there is no support for dropping bus and ddr clock rates for any of the i.MX platforms in upstream. Parts of the implementation of this new approach made it upstream a couple of years ago. Therefore, even if you compile the interconnect driver in, you won't have the full functionality. With this patchset we'll get support for i.MX8MQ first. The rest of 8M platforms would get support once we agree on i.MX8MQ since the generic part would not change at all. I'll send a subsequent updated version soon, with all the comments taken care of. > > adam > > diff --git a/drivers/interconnect/imx/imx.h b/drivers/interconnect/imx/imx.h > > index 75da51076c68..5c9f5138f6aa 100644 > > --- a/drivers/interconnect/imx/imx.h > > +++ b/drivers/interconnect/imx/imx.h > > @@ -14,15 +14,6 @@ > > > > #define IMX_ICC_MAX_LINKS 4 > > > > -/* > > - * struct imx_icc_node_adj - Describe a dynamic adjustable node > > - */ > > -struct imx_icc_node_adj_desc { > > - unsigned int bw_mul, bw_div; > > - const char *phandle_name; > > - bool main_noc; > > -}; > > - > > /* > > * struct imx_icc_node - Describe an interconnect node > > * @name: name of the node > > @@ -35,23 +26,21 @@ struct imx_icc_node_desc { > > u16 id; > > u16 links[IMX_ICC_MAX_LINKS]; > > u16 num_links; > > - const struct imx_icc_node_adj_desc *adj; > > }; > > > > -#define DEFINE_BUS_INTERCONNECT(_name, _id, _adj, ...) \ > > +#define DEFINE_BUS_INTERCONNECT(_name, _id, ...) \ > > { \ > > .id = _id, \ > > .name = _name, \ > > - .adj = _adj, \ > > .num_links = ARRAY_SIZE(((int[]){ __VA_ARGS__ })), \ > > .links = { __VA_ARGS__ }, \ > > } > > > > #define DEFINE_BUS_MASTER(_name, _id, _dest_id) \ > > - DEFINE_BUS_INTERCONNECT(_name, _id, NULL, _dest_id) > > + DEFINE_BUS_INTERCONNECT(_name, _id, _dest_id) > > > > -#define DEFINE_BUS_SLAVE(_name, _id, _adj) \ > > - DEFINE_BUS_INTERCONNECT(_name, _id, _adj) > > +#define DEFINE_BUS_SLAVE(_name, _id) \ > > + DEFINE_BUS_INTERCONNECT(_name, _id) > > > > int imx_icc_register(struct platform_device *pdev, > > struct imx_icc_node_desc *nodes, > > diff --git a/drivers/interconnect/imx/imx8mm.c b/drivers/interconnect/imx/imx8mm.c > > index 1083490bb391..0c16110bef9d 100644 > > --- a/drivers/interconnect/imx/imx8mm.c > > +++ b/drivers/interconnect/imx/imx8mm.c > > @@ -14,18 +14,6 @@ > > > > #include "imx.h" > > > > -static const struct imx_icc_node_adj_desc imx8mm_dram_adj = { > > - .bw_mul = 1, > > - .bw_div = 16, > > - .phandle_name = "fsl,ddrc", > > -}; > > - > > -static const struct imx_icc_node_adj_desc imx8mm_noc_adj = { > > - .bw_mul = 1, > > - .bw_div = 16, > > - .main_noc = true, > > -}; > > - > > /* > > * Describe bus masters, slaves and connections between them > > * > > @@ -33,43 +21,43 @@ static const struct imx_icc_node_adj_desc imx8mm_noc_adj = { > > * PL301 nics which are skipped/merged into PL301_MAIN > > */ > > static struct imx_icc_node_desc nodes[] = { > > - DEFINE_BUS_INTERCONNECT("NOC", IMX8MM_ICN_NOC, &imx8mm_noc_adj, > > + DEFINE_BUS_INTERCONNECT("NOC", IMX8MM_ICN_NOC, > > IMX8MM_ICS_DRAM, IMX8MM_ICN_MAIN), > > > > - DEFINE_BUS_SLAVE("DRAM", IMX8MM_ICS_DRAM, &imx8mm_dram_adj), > > - DEFINE_BUS_SLAVE("OCRAM", IMX8MM_ICS_OCRAM, NULL), > > + DEFINE_BUS_SLAVE("DRAM", IMX8MM_ICS_DRAM), > > + DEFINE_BUS_SLAVE("OCRAM", IMX8MM_ICS_OCRAM), > > DEFINE_BUS_MASTER("A53", IMX8MM_ICM_A53, IMX8MM_ICN_NOC), > > > > /* VPUMIX */ > > DEFINE_BUS_MASTER("VPU H1", IMX8MM_ICM_VPU_H1, IMX8MM_ICN_VIDEO), > > DEFINE_BUS_MASTER("VPU G1", IMX8MM_ICM_VPU_G1, IMX8MM_ICN_VIDEO), > > DEFINE_BUS_MASTER("VPU G2", IMX8MM_ICM_VPU_G2, IMX8MM_ICN_VIDEO), > > - DEFINE_BUS_INTERCONNECT("PL301_VIDEO", IMX8MM_ICN_VIDEO, NULL, IMX8MM_ICN_NOC), > > + DEFINE_BUS_INTERCONNECT("PL301_VIDEO", IMX8MM_ICN_VIDEO, IMX8MM_ICN_NOC), > > > > /* GPUMIX */ > > DEFINE_BUS_MASTER("GPU 2D", IMX8MM_ICM_GPU2D, IMX8MM_ICN_GPU), > > DEFINE_BUS_MASTER("GPU 3D", IMX8MM_ICM_GPU3D, IMX8MM_ICN_GPU), > > - DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MM_ICN_GPU, NULL, IMX8MM_ICN_NOC), > > + DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MM_ICN_GPU, IMX8MM_ICN_NOC), > > > > /* DISPLAYMIX */ > > DEFINE_BUS_MASTER("CSI", IMX8MM_ICM_CSI, IMX8MM_ICN_MIPI), > > DEFINE_BUS_MASTER("LCDIF", IMX8MM_ICM_LCDIF, IMX8MM_ICN_MIPI), > > - DEFINE_BUS_INTERCONNECT("PL301_MIPI", IMX8MM_ICN_MIPI, NULL, IMX8MM_ICN_NOC), > > + DEFINE_BUS_INTERCONNECT("PL301_MIPI", IMX8MM_ICN_MIPI, IMX8MM_ICN_NOC), > > > > /* HSIO */ > > DEFINE_BUS_MASTER("USB1", IMX8MM_ICM_USB1, IMX8MM_ICN_HSIO), > > DEFINE_BUS_MASTER("USB2", IMX8MM_ICM_USB2, IMX8MM_ICN_HSIO), > > DEFINE_BUS_MASTER("PCIE", IMX8MM_ICM_PCIE, IMX8MM_ICN_HSIO), > > - DEFINE_BUS_INTERCONNECT("PL301_HSIO", IMX8MM_ICN_HSIO, NULL, IMX8MM_ICN_NOC), > > + DEFINE_BUS_INTERCONNECT("PL301_HSIO", IMX8MM_ICN_HSIO, IMX8MM_ICN_NOC), > > > > /* Audio */ > > DEFINE_BUS_MASTER("SDMA2", IMX8MM_ICM_SDMA2, IMX8MM_ICN_AUDIO), > > DEFINE_BUS_MASTER("SDMA3", IMX8MM_ICM_SDMA3, IMX8MM_ICN_AUDIO), > > - DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MM_ICN_AUDIO, NULL, IMX8MM_ICN_MAIN), > > + DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MM_ICN_AUDIO, IMX8MM_ICN_MAIN), > > > > /* Ethernet */ > > DEFINE_BUS_MASTER("ENET", IMX8MM_ICM_ENET, IMX8MM_ICN_ENET), > > - DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MM_ICN_ENET, NULL, IMX8MM_ICN_MAIN), > > + DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MM_ICN_ENET, IMX8MM_ICN_MAIN), > > > > /* Other */ > > DEFINE_BUS_MASTER("SDMA1", IMX8MM_ICM_SDMA1, IMX8MM_ICN_MAIN), > > @@ -77,7 +65,7 @@ static struct imx_icc_node_desc nodes[] = { > > DEFINE_BUS_MASTER("USDHC1", IMX8MM_ICM_USDHC1, IMX8MM_ICN_MAIN), > > DEFINE_BUS_MASTER("USDHC2", IMX8MM_ICM_USDHC2, IMX8MM_ICN_MAIN), > > DEFINE_BUS_MASTER("USDHC3", IMX8MM_ICM_USDHC3, IMX8MM_ICN_MAIN), > > - DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MM_ICN_MAIN, NULL, > > + DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MM_ICN_MAIN, > > IMX8MM_ICN_NOC, IMX8MM_ICS_OCRAM), > > }; > > > > diff --git a/drivers/interconnect/imx/imx8mn.c b/drivers/interconnect/imx/imx8mn.c > > index ad97e55fd4e5..8d16bd5cf006 100644 > > --- a/drivers/interconnect/imx/imx8mn.c > > +++ b/drivers/interconnect/imx/imx8mn.c > > @@ -11,18 +11,6 @@ > > > > #include "imx.h" > > > > -static const struct imx_icc_node_adj_desc imx8mn_dram_adj = { > > - .bw_mul = 1, > > - .bw_div = 4, > > - .phandle_name = "fsl,ddrc", > > -}; > > - > > -static const struct imx_icc_node_adj_desc imx8mn_noc_adj = { > > - .bw_mul = 1, > > - .bw_div = 4, > > - .main_noc = true, > > -}; > > - > > /* > > * Describe bus masters, slaves and connections between them > > * > > @@ -30,23 +18,23 @@ static const struct imx_icc_node_adj_desc imx8mn_noc_adj = { > > * PL301 nics which are skipped/merged into PL301_MAIN > > */ > > static struct imx_icc_node_desc nodes[] = { > > - DEFINE_BUS_INTERCONNECT("NOC", IMX8MN_ICN_NOC, &imx8mn_noc_adj, > > + DEFINE_BUS_INTERCONNECT("NOC", IMX8MN_ICN_NOC, > > IMX8MN_ICS_DRAM, IMX8MN_ICN_MAIN), > > > > - DEFINE_BUS_SLAVE("DRAM", IMX8MN_ICS_DRAM, &imx8mn_dram_adj), > > - DEFINE_BUS_SLAVE("OCRAM", IMX8MN_ICS_OCRAM, NULL), > > + DEFINE_BUS_SLAVE("DRAM", IMX8MN_ICS_DRAM), > > + DEFINE_BUS_SLAVE("OCRAM", IMX8MN_ICS_OCRAM), > > DEFINE_BUS_MASTER("A53", IMX8MN_ICM_A53, IMX8MN_ICN_NOC), > > > > /* GPUMIX */ > > DEFINE_BUS_MASTER("GPU", IMX8MN_ICM_GPU, IMX8MN_ICN_GPU), > > - DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MN_ICN_GPU, NULL, IMX8MN_ICN_NOC), > > + DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MN_ICN_GPU, IMX8MN_ICN_NOC), > > > > /* DISPLAYMIX */ > > DEFINE_BUS_MASTER("CSI1", IMX8MN_ICM_CSI1, IMX8MN_ICN_MIPI), > > DEFINE_BUS_MASTER("CSI2", IMX8MN_ICM_CSI2, IMX8MN_ICN_MIPI), > > DEFINE_BUS_MASTER("ISI", IMX8MN_ICM_ISI, IMX8MN_ICN_MIPI), > > DEFINE_BUS_MASTER("LCDIF", IMX8MN_ICM_LCDIF, IMX8MN_ICN_MIPI), > > - DEFINE_BUS_INTERCONNECT("PL301_MIPI", IMX8MN_ICN_MIPI, NULL, IMX8MN_ICN_NOC), > > + DEFINE_BUS_INTERCONNECT("PL301_MIPI", IMX8MN_ICN_MIPI, IMX8MN_ICN_NOC), > > > > /* USB goes straight to NOC */ > > DEFINE_BUS_MASTER("USB", IMX8MN_ICM_USB, IMX8MN_ICN_NOC), > > @@ -54,11 +42,11 @@ static struct imx_icc_node_desc nodes[] = { > > /* Audio */ > > DEFINE_BUS_MASTER("SDMA2", IMX8MN_ICM_SDMA2, IMX8MN_ICN_AUDIO), > > DEFINE_BUS_MASTER("SDMA3", IMX8MN_ICM_SDMA3, IMX8MN_ICN_AUDIO), > > - DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MN_ICN_AUDIO, NULL, IMX8MN_ICN_MAIN), > > + DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MN_ICN_AUDIO, IMX8MN_ICN_MAIN), > > > > /* Ethernet */ > > DEFINE_BUS_MASTER("ENET", IMX8MN_ICM_ENET, IMX8MN_ICN_ENET), > > - DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MN_ICN_ENET, NULL, IMX8MN_ICN_MAIN), > > + DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MN_ICN_ENET, IMX8MN_ICN_MAIN), > > > > /* Other */ > > DEFINE_BUS_MASTER("SDMA1", IMX8MN_ICM_SDMA1, IMX8MN_ICN_MAIN), > > @@ -66,7 +54,7 @@ static struct imx_icc_node_desc nodes[] = { > > DEFINE_BUS_MASTER("USDHC1", IMX8MN_ICM_USDHC1, IMX8MN_ICN_MAIN), > > DEFINE_BUS_MASTER("USDHC2", IMX8MN_ICM_USDHC2, IMX8MN_ICN_MAIN), > > DEFINE_BUS_MASTER("USDHC3", IMX8MN_ICM_USDHC3, IMX8MN_ICN_MAIN), > > - DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MN_ICN_MAIN, NULL, > > + DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MN_ICN_MAIN > > IMX8MN_ICN_NOC, IMX8MN_ICS_OCRAM), > > }; > > > > diff --git a/drivers/interconnect/imx/imx8mq.c b/drivers/interconnect/imx/imx8mq.c > > index d7768d3c6d8a..b8c36d668946 100644 > > --- a/drivers/interconnect/imx/imx8mq.c > > +++ b/drivers/interconnect/imx/imx8mq.c > > @@ -12,18 +12,6 @@ > > > > #include "imx.h" > > > > -static const struct imx_icc_node_adj_desc imx8mq_dram_adj = { > > - .bw_mul = 1, > > - .bw_div = 4, > > - .phandle_name = "fsl,ddrc", > > -}; > > - > > -static const struct imx_icc_node_adj_desc imx8mq_noc_adj = { > > - .bw_mul = 1, > > - .bw_div = 4, > > - .main_noc = true, > > -}; > > - > > /* > > * Describe bus masters, slaves and connections between them > > * > > @@ -31,43 +19,42 @@ static const struct imx_icc_node_adj_desc imx8mq_noc_adj = { > > * PL301 nics which are skipped/merged into PL301_MAIN > > */ > > static struct imx_icc_node_desc nodes[] = { > > - DEFINE_BUS_INTERCONNECT("NOC", IMX8MQ_ICN_NOC, &imx8mq_noc_adj, > > - IMX8MQ_ICS_DRAM, IMX8MQ_ICN_MAIN), > > + DEFINE_BUS_INTERCONNECT("NOC", IMX8MQ_ICN_NOC, IMX8MQ_ICS_DRAM, IMX8MQ_ICN_MAIN), > > > > - DEFINE_BUS_SLAVE("DRAM", IMX8MQ_ICS_DRAM, &imx8mq_dram_adj), > > - DEFINE_BUS_SLAVE("OCRAM", IMX8MQ_ICS_OCRAM, NULL), > > + DEFINE_BUS_SLAVE("DRAM", IMX8MQ_ICS_DRAM), > > + DEFINE_BUS_SLAVE("OCRAM", IMX8MQ_ICS_OCRAM), > > DEFINE_BUS_MASTER("A53", IMX8MQ_ICM_A53, IMX8MQ_ICN_NOC), > > > > /* VPUMIX */ > > DEFINE_BUS_MASTER("VPU", IMX8MQ_ICM_VPU, IMX8MQ_ICN_VIDEO), > > - DEFINE_BUS_INTERCONNECT("PL301_VIDEO", IMX8MQ_ICN_VIDEO, NULL, IMX8MQ_ICN_NOC), > > + DEFINE_BUS_INTERCONNECT("PL301_VIDEO", IMX8MQ_ICN_VIDEO, IMX8MQ_ICN_NOC), > > > > /* GPUMIX */ > > DEFINE_BUS_MASTER("GPU", IMX8MQ_ICM_GPU, IMX8MQ_ICN_GPU), > > - DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MQ_ICN_GPU, NULL, IMX8MQ_ICN_NOC), > > + DEFINE_BUS_INTERCONNECT("PL301_GPU", IMX8MQ_ICN_GPU, IMX8MQ_ICN_NOC), > > > > /* DISPMIX (only for DCSS) */ > > DEFINE_BUS_MASTER("DC", IMX8MQ_ICM_DCSS, IMX8MQ_ICN_DCSS), > > - DEFINE_BUS_INTERCONNECT("PL301_DC", IMX8MQ_ICN_DCSS, NULL, IMX8MQ_ICN_NOC), > > + DEFINE_BUS_INTERCONNECT("PL301_DC", IMX8MQ_ICN_DCSS, IMX8MQ_ICN_NOC), > > > > /* USBMIX */ > > DEFINE_BUS_MASTER("USB1", IMX8MQ_ICM_USB1, IMX8MQ_ICN_USB), > > DEFINE_BUS_MASTER("USB2", IMX8MQ_ICM_USB2, IMX8MQ_ICN_USB), > > - DEFINE_BUS_INTERCONNECT("PL301_USB", IMX8MQ_ICN_USB, NULL, IMX8MQ_ICN_NOC), > > + DEFINE_BUS_INTERCONNECT("PL301_USB", IMX8MQ_ICN_USB, IMX8MQ_ICN_NOC), > > > > /* PL301_DISPLAY (IPs other than DCSS, inside SUPERMIX) */ > > DEFINE_BUS_MASTER("CSI1", IMX8MQ_ICM_CSI1, IMX8MQ_ICN_DISPLAY), > > DEFINE_BUS_MASTER("CSI2", IMX8MQ_ICM_CSI2, IMX8MQ_ICN_DISPLAY), > > DEFINE_BUS_MASTER("LCDIF", IMX8MQ_ICM_LCDIF, IMX8MQ_ICN_DISPLAY), > > - DEFINE_BUS_INTERCONNECT("PL301_DISPLAY", IMX8MQ_ICN_DISPLAY, NULL, IMX8MQ_ICN_MAIN), > > + DEFINE_BUS_INTERCONNECT("PL301_DISPLAY", IMX8MQ_ICN_DISPLAY, IMX8MQ_ICN_MAIN), > > > > /* AUDIO */ > > DEFINE_BUS_MASTER("SDMA2", IMX8MQ_ICM_SDMA2, IMX8MQ_ICN_AUDIO), > > - DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MQ_ICN_AUDIO, NULL, IMX8MQ_ICN_DISPLAY), > > + DEFINE_BUS_INTERCONNECT("PL301_AUDIO", IMX8MQ_ICN_AUDIO, IMX8MQ_ICN_DISPLAY), > > > > /* ENET */ > > DEFINE_BUS_MASTER("ENET", IMX8MQ_ICM_ENET, IMX8MQ_ICN_ENET), > > - DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MQ_ICN_ENET, NULL, IMX8MQ_ICN_MAIN), > > + DEFINE_BUS_INTERCONNECT("PL301_ENET", IMX8MQ_ICN_ENET, IMX8MQ_ICN_MAIN), > > > > /* OTHER */ > > DEFINE_BUS_MASTER("SDMA1", IMX8MQ_ICM_SDMA1, IMX8MQ_ICN_MAIN), > > @@ -76,7 +63,7 @@ static struct imx_icc_node_desc nodes[] = { > > DEFINE_BUS_MASTER("USDHC2", IMX8MQ_ICM_USDHC2, IMX8MQ_ICN_MAIN), > > DEFINE_BUS_MASTER("PCIE1", IMX8MQ_ICM_PCIE1, IMX8MQ_ICN_MAIN), > > DEFINE_BUS_MASTER("PCIE2", IMX8MQ_ICM_PCIE2, IMX8MQ_ICN_MAIN), > > - DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MQ_ICN_MAIN, NULL, > > + DEFINE_BUS_INTERCONNECT("PL301_MAIN", IMX8MQ_ICN_MAIN, > > IMX8MQ_ICN_NOC, IMX8MQ_ICS_OCRAM), > > }; > > > > -- > > 2.31.1 > >
On 21-09-15 12:37:45, Chanwoo Choi wrote: > Hi, > > As I commented on patch5, you keep the OPP list on devicetree file > and then you better to use the 'suspend_opp' property > for setting the highest frequency during suspend/resume. > Hi, I think there is no mechanism in place to ensure that the suspend opp will be set only after all the icc users have suspended. I only tested briefly, but I can tell you that there are cases where some icc user asks for a different opp right after the suspend opp was set. This leads to suspending with a different rate than the one from suspend opp. So I guess I still need the late system sleep pm opps to circumvent such situations. > On 21. 9. 14. 오전 2:38, Abel Vesa wrote: > > Seems that, in order to be able to resume from suspend, the dram rate > > needs to be the highest one available. Therefore, add the late system > > suspend/resume PM ops which set the highest rate on suspend and the > > latest one used before suspending on resume. > > > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > > --- > > drivers/devfreq/imx8m-ddrc.c | 28 +++++++++++++++++++++++++++- > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c > > index f18a5c3c1c03..f39741b4a0b0 100644 > > --- a/drivers/devfreq/imx8m-ddrc.c > > +++ b/drivers/devfreq/imx8m-ddrc.c > > @@ -72,6 +72,8 @@ struct imx8m_ddrc { > > struct clk *dram_alt; > > struct clk *dram_apb; > > + unsigned long suspend_rate; > > + unsigned long resume_rate; > > int freq_count; > > struct imx8m_ddrc_freq freq_table[IMX8M_DDRC_MAX_FREQ_COUNT]; > > }; > > @@ -271,6 +273,22 @@ static int imx8m_ddrc_target(struct device *dev, unsigned long *freq, u32 flags) > > return ret; > > } > > +static int imx8m_ddrc_suspend(struct device *dev) > > +{ > > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > + > > + priv->resume_rate = clk_get_rate(priv->dram_core); > > + > > + return imx8m_ddrc_target(dev, &priv->suspend_rate, 0); > > +} > > + > > +static int imx8m_ddrc_resume(struct device *dev) > > +{ > > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > + > > + return imx8m_ddrc_target(dev, &priv->resume_rate, 0); > > +} > > + > > static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq) > > { > > struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > @@ -324,6 +342,9 @@ static int imx8m_ddrc_init_freq_info(struct device *dev) > > if (dev_pm_opp_add(dev, freq->rate * 250000, 0)) > > return -ENODEV; > > + > > + if (index == 0) > > + priv->suspend_rate = freq->rate * 250000; > > } > > return 0; > > @@ -399,11 +420,16 @@ static const struct of_device_id imx8m_ddrc_of_match[] = { > > }; > > MODULE_DEVICE_TABLE(of, imx8m_ddrc_of_match); > > +static const struct dev_pm_ops imx8m_ddrc_pm_ops = { > > + SET_LATE_SYSTEM_SLEEP_PM_OPS(imx8m_ddrc_suspend, imx8m_ddrc_resume) > > +}; > > + > > static struct platform_driver imx8m_ddrc_platdrv = { > > .probe = imx8m_ddrc_probe, > > .driver = { > > .name = "imx8m-ddrc-devfreq", > > - .of_match_table = imx8m_ddrc_of_match, > > + .pm = &imx8m_ddrc_pm_ops, > > + .of_match_table = of_match_ptr(imx8m_ddrc_of_match), > > }, > > }; > > module_platform_driver(imx8m_ddrc_platdrv); > > > > > -- > Best Regards, > Samsung Electronics > Chanwoo Choi
Am Montag, dem 13.09.2021 um 20:38 +0300 schrieb Abel Vesa: > Seems that, in order to be able to resume from suspend, the dram rate > needs to be the highest one available. Therefore, add the late system > suspend/resume PM ops which set the highest rate on suspend and the > latest one used before suspending on resume. Hi Abel, wouldn't this mean that s2idle / freeze would be kind of broken by this? Does is make sense to test the lowest rate? How would I force that here? (just for testing) Also, you could think about splitting this series up a bit and do this patch seperately onto mainline (before or after the other work). thank you martin > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > --- > drivers/devfreq/imx8m-ddrc.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m- > ddrc.c > index f18a5c3c1c03..f39741b4a0b0 100644 > --- a/drivers/devfreq/imx8m-ddrc.c > +++ b/drivers/devfreq/imx8m-ddrc.c > @@ -72,6 +72,8 @@ struct imx8m_ddrc { > struct clk *dram_alt; > struct clk *dram_apb; > > + unsigned long suspend_rate; > + unsigned long resume_rate; > int freq_count; > struct imx8m_ddrc_freq freq_table[IMX8M_DDRC_MAX_FREQ_COUNT]; > }; > @@ -271,6 +273,22 @@ static int imx8m_ddrc_target(struct device *dev, > unsigned long *freq, u32 flags) > return ret; > } > > +static int imx8m_ddrc_suspend(struct device *dev) > +{ > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > + > + priv->resume_rate = clk_get_rate(priv->dram_core); > + > + return imx8m_ddrc_target(dev, &priv->suspend_rate, 0); > +} > + > +static int imx8m_ddrc_resume(struct device *dev) > +{ > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > + > + return imx8m_ddrc_target(dev, &priv->resume_rate, 0); > +} > + > static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long > *freq) > { > struct imx8m_ddrc *priv = dev_get_drvdata(dev); > @@ -324,6 +342,9 @@ static int imx8m_ddrc_init_freq_info(struct > device *dev) > > if (dev_pm_opp_add(dev, freq->rate * 250000, 0)) > return -ENODEV; > + > + if (index == 0) > + priv->suspend_rate = freq->rate * 250000; > } > > return 0; > @@ -399,11 +420,16 @@ static const struct of_device_id > imx8m_ddrc_of_match[] = { > }; > MODULE_DEVICE_TABLE(of, imx8m_ddrc_of_match); > > +static const struct dev_pm_ops imx8m_ddrc_pm_ops = { > + SET_LATE_SYSTEM_SLEEP_PM_OPS(imx8m_ddrc_suspend, > imx8m_ddrc_resume) > +}; > + > static struct platform_driver imx8m_ddrc_platdrv = { > .probe = imx8m_ddrc_probe, > .driver = { > .name = "imx8m-ddrc-devfreq", > - .of_match_table = imx8m_ddrc_of_match, > + .pm = &imx8m_ddrc_pm_ops, > + .of_match_table = of_match_ptr(imx8m_ddrc_of_match), > }, > }; > module_platform_driver(imx8m_ddrc_platdrv);
On 21-11-10 13:15:26, Martin Kepplinger wrote: > Am Montag, dem 13.09.2021 um 20:38 +0300 schrieb Abel Vesa: > > Seems that, in order to be able to resume from suspend, the dram rate > > needs to be the highest one available. Therefore, add the late system > > suspend/resume PM ops which set the highest rate on suspend and the > > latest one used before suspending on resume. > > Hi Abel, wouldn't this mean that s2idle / freeze would be kind of > broken by this? > Nope. Only the DDR rate needs to be raised at 800M before suspending. Everything else stays the same. > Does is make sense to test the lowest rate? How would I force that > here? (just for testing) You can try, but it will surely freeze. See [1] what you need to change for testing. > > Also, you could think about splitting this series up a bit and do this > patch seperately onto mainline (before or after the other work). > Well, I sent as RFC until now. Seems there are no big issues with the approach. So I'll split the patches between subsystems on the next iteration. > thank you > martin > > > > > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > > --- > > drivers/devfreq/imx8m-ddrc.c | 28 +++++++++++++++++++++++++++- > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m- > > ddrc.c > > index f18a5c3c1c03..f39741b4a0b0 100644 > > --- a/drivers/devfreq/imx8m-ddrc.c > > +++ b/drivers/devfreq/imx8m-ddrc.c > > @@ -72,6 +72,8 @@ struct imx8m_ddrc { > > struct clk *dram_alt; > > struct clk *dram_apb; > > > > + unsigned long suspend_rate; > > + unsigned long resume_rate; > > int freq_count; > > struct imx8m_ddrc_freq freq_table[IMX8M_DDRC_MAX_FREQ_COUNT]; > > }; > > @@ -271,6 +273,22 @@ static int imx8m_ddrc_target(struct device *dev, > > unsigned long *freq, u32 flags) > > return ret; > > } > > > > +static int imx8m_ddrc_suspend(struct device *dev) > > +{ > > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > + > > + priv->resume_rate = clk_get_rate(priv->dram_core); > > + > > + return imx8m_ddrc_target(dev, &priv->suspend_rate, 0); > > +} > > + > > +static int imx8m_ddrc_resume(struct device *dev) > > +{ > > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > + > > + return imx8m_ddrc_target(dev, &priv->resume_rate, 0); > > +} > > + > > static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long > > *freq) > > { > > struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > @@ -324,6 +342,9 @@ static int imx8m_ddrc_init_freq_info(struct > > device *dev) > > > > if (dev_pm_opp_add(dev, freq->rate * 250000, 0)) > > return -ENODEV; > > + > > + if (index == 0) [1] Change this line to: if (index == 1) It will select the 166935483 freq for suspending. > > + priv->suspend_rate = freq->rate * 250000; > > } > > > > return 0; > > @@ -399,11 +420,16 @@ static const struct of_device_id > > imx8m_ddrc_of_match[] = { > > }; > > MODULE_DEVICE_TABLE(of, imx8m_ddrc_of_match); > > > > +static const struct dev_pm_ops imx8m_ddrc_pm_ops = { > > + SET_LATE_SYSTEM_SLEEP_PM_OPS(imx8m_ddrc_suspend, > > imx8m_ddrc_resume) > > +}; > > + > > static struct platform_driver imx8m_ddrc_platdrv = { > > .probe = imx8m_ddrc_probe, > > .driver = { > > .name = "imx8m-ddrc-devfreq", > > - .of_match_table = imx8m_ddrc_of_match, > > + .pm = &imx8m_ddrc_pm_ops, > > + .of_match_table = of_match_ptr(imx8m_ddrc_of_match), > > }, > > }; > > module_platform_driver(imx8m_ddrc_platdrv); > >
Am Dienstag, dem 30.11.2021 um 22:06 +0200 schrieb Abel Vesa: > On 21-11-10 13:15:26, Martin Kepplinger wrote: > > Am Montag, dem 13.09.2021 um 20:38 +0300 schrieb Abel Vesa: > > > Seems that, in order to be able to resume from suspend, the dram > > > rate > > > needs to be the highest one available. Therefore, add the late > > > system > > > suspend/resume PM ops which set the highest rate on suspend and > > > the > > > latest one used before suspending on resume. > > > > Hi Abel, wouldn't this mean that s2idle / freeze would be kind of > > broken by this? > > > > Nope. Only the DDR rate needs to be raised at 800M before suspending. > Everything else stays the same. well, for s2idle, linux stays running, so no calling out to atf (dram retention...) is happening, so it'll stay at 800M *during* being suspended. I tested that by observing power consumption of the system - although for now without the cpu-sleep state (via your workaround) due to https://lore.kernel.org/linux-arm-kernel/6ca0bcabfa3b6643f9ab7e311cd8697df223c5cb.camel@puri.sm/ > > > Does is make sense to test the lowest rate? How would I force that > > here? (just for testing) > > You can try, but it will surely freeze. See [1] what you need to > change > for testing. thanks, that looks nicer than forcing 50M. > > > > Also, you could think about splitting this series up a bit and do > > this > > patch seperately onto mainline (before or after the other work). > > > > Well, I sent as RFC until now. Seems there are no big issues with the > approach. So I'll split the patches between subsystems on the next > iteration. great, looking forward to it! > > > thank you > > martin > > > > > > > > > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > > > --- > > > drivers/devfreq/imx8m-ddrc.c | 28 +++++++++++++++++++++++++++- > > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/devfreq/imx8m-ddrc.c > > > b/drivers/devfreq/imx8m- > > > ddrc.c > > > index f18a5c3c1c03..f39741b4a0b0 100644 > > > --- a/drivers/devfreq/imx8m-ddrc.c > > > +++ b/drivers/devfreq/imx8m-ddrc.c > > > @@ -72,6 +72,8 @@ struct imx8m_ddrc { > > > struct clk *dram_alt; > > > struct clk *dram_apb; > > > > > > + unsigned long suspend_rate; > > > + unsigned long resume_rate; > > > int freq_count; > > > struct imx8m_ddrc_freq > > > freq_table[IMX8M_DDRC_MAX_FREQ_COUNT]; > > > }; > > > @@ -271,6 +273,22 @@ static int imx8m_ddrc_target(struct device > > > *dev, > > > unsigned long *freq, u32 flags) > > > return ret; > > > } > > > > > > +static int imx8m_ddrc_suspend(struct device *dev) > > > +{ > > > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > > + > > > + priv->resume_rate = clk_get_rate(priv->dram_core); > > > + > > > + return imx8m_ddrc_target(dev, &priv->suspend_rate, 0); > > > +} > > > + > > > +static int imx8m_ddrc_resume(struct device *dev) > > > +{ > > > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > > + > > > + return imx8m_ddrc_target(dev, &priv->resume_rate, 0); > > > +} > > > + > > > static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned > > > long > > > *freq) > > > { > > > struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > > @@ -324,6 +342,9 @@ static int imx8m_ddrc_init_freq_info(struct > > > device *dev) > > > > > > if (dev_pm_opp_add(dev, freq->rate * 250000, 0)) > > > return -ENODEV; > > > + > > > + if (index == 0) > > [1] Change this line to: > if (index == 1) > > It will select the 166935483 freq for suspending. > > > > + priv->suspend_rate = freq->rate * 250000; > > > } > > > > > > return 0; > > > @@ -399,11 +420,16 @@ static const struct of_device_id > > > imx8m_ddrc_of_match[] = { > > > }; > > > MODULE_DEVICE_TABLE(of, imx8m_ddrc_of_match); > > > > > > +static const struct dev_pm_ops imx8m_ddrc_pm_ops = { > > > + SET_LATE_SYSTEM_SLEEP_PM_OPS(imx8m_ddrc_suspend, > > > imx8m_ddrc_resume) > > > +}; > > > + > > > static struct platform_driver imx8m_ddrc_platdrv = { > > > .probe = imx8m_ddrc_probe, > > > .driver = { > > > .name = "imx8m-ddrc-devfreq", > > > - .of_match_table = imx8m_ddrc_of_match, > > > + .pm = &imx8m_ddrc_pm_ops, > > > + .of_match_table = > > > of_match_ptr(imx8m_ddrc_of_match), > > > }, > > > }; > > > module_platform_driver(imx8m_ddrc_platdrv); > > > >
Am Dienstag, dem 30.11.2021 um 22:06 +0200 schrieb Abel Vesa: > On 21-11-10 13:15:26, Martin Kepplinger wrote: > > Am Montag, dem 13.09.2021 um 20:38 +0300 schrieb Abel Vesa: > > > Seems that, in order to be able to resume from suspend, the dram > > > rate > > > needs to be the highest one available. Therefore, add the late > > > system > > > suspend/resume PM ops which set the highest rate on suspend and > > > the > > > latest one used before suspending on resume. > > > > Hi Abel, wouldn't this mean that s2idle / freeze would be kind of > > broken by this? > > > > Nope. Only the DDR rate needs to be raised at 800M before suspending. > Everything else stays the same. fyi I just tested this and you're right. freezes when not at 800M. So for this patchset, I think this is fine as it enables and fixes stuff. It would not hurt to mention s2idle at least, where of course 800M should not be selected, as no userspace is running at all. But I'd be fine with looking at that later. > > > Does is make sense to test the lowest rate? How would I force that > > here? (just for testing) > > You can try, but it will surely freeze. See [1] what you need to > change > for testing. > > > > Also, you could think about splitting this series up a bit and do > > this > > patch seperately onto mainline (before or after the other work). > > > > Well, I sent as RFC until now. Seems there are no big issues with the > approach. So I'll split the patches between subsystems on the next > iteration. > > > thank you > > martin > > > > > > > > > > Signed-off-by: Abel Vesa <abel.vesa@nxp.com> > > > --- > > > drivers/devfreq/imx8m-ddrc.c | 28 +++++++++++++++++++++++++++- > > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/devfreq/imx8m-ddrc.c > > > b/drivers/devfreq/imx8m- > > > ddrc.c > > > index f18a5c3c1c03..f39741b4a0b0 100644 > > > --- a/drivers/devfreq/imx8m-ddrc.c > > > +++ b/drivers/devfreq/imx8m-ddrc.c > > > @@ -72,6 +72,8 @@ struct imx8m_ddrc { > > > struct clk *dram_alt; > > > struct clk *dram_apb; > > > > > > + unsigned long suspend_rate; > > > + unsigned long resume_rate; > > > int freq_count; > > > struct imx8m_ddrc_freq > > > freq_table[IMX8M_DDRC_MAX_FREQ_COUNT]; > > > }; > > > @@ -271,6 +273,22 @@ static int imx8m_ddrc_target(struct device > > > *dev, > > > unsigned long *freq, u32 flags) > > > return ret; > > > } > > > > > > +static int imx8m_ddrc_suspend(struct device *dev) > > > +{ > > > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > > + > > > + priv->resume_rate = clk_get_rate(priv->dram_core); > > > + > > > + return imx8m_ddrc_target(dev, &priv->suspend_rate, 0); > > > +} > > > + > > > +static int imx8m_ddrc_resume(struct device *dev) > > > +{ > > > + struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > > + > > > + return imx8m_ddrc_target(dev, &priv->resume_rate, 0); > > > +} > > > + > > > static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned > > > long > > > *freq) > > > { > > > struct imx8m_ddrc *priv = dev_get_drvdata(dev); > > > @@ -324,6 +342,9 @@ static int imx8m_ddrc_init_freq_info(struct > > > device *dev) > > > > > > if (dev_pm_opp_add(dev, freq->rate * 250000, 0)) > > > return -ENODEV; > > > + > > > + if (index == 0) > > [1] Change this line to: > if (index == 1) > > It will select the 166935483 freq for suspending. > > > > + priv->suspend_rate = freq->rate * 250000; > > > } > > > > > > return 0; > > > @@ -399,11 +420,16 @@ static const struct of_device_id > > > imx8m_ddrc_of_match[] = { > > > }; > > > MODULE_DEVICE_TABLE(of, imx8m_ddrc_of_match); > > > > > > +static const struct dev_pm_ops imx8m_ddrc_pm_ops = { > > > + SET_LATE_SYSTEM_SLEEP_PM_OPS(imx8m_ddrc_suspend, > > > imx8m_ddrc_resume) > > > +}; > > > + > > > static struct platform_driver imx8m_ddrc_platdrv = { > > > .probe = imx8m_ddrc_probe, > > > .driver = { > > > .name = "imx8m-ddrc-devfreq", > > > - .of_match_table = imx8m_ddrc_of_match, > > > + .pm = &imx8m_ddrc_pm_ops, > > > + .of_match_table = > > > of_match_ptr(imx8m_ddrc_of_match), > > > }, > > > }; > > > module_platform_driver(imx8m_ddrc_platdrv); > > > >
In vendor tree there is something called busfreq but it is non-upstreamable. This approach with interconnect+devfreq gives us the same functionality by using already upstream and mature subsystems. This series is more of a proof-of-concept. Abel Vesa (19): dt-bindings: interconnect: imx8mq: Add missing pl301 and SAI ids devfreq: imx-bus: Switch governor to powersave devfreq: imx-bus: Decouple imx-bus from icc made devfreq: imx8m-ddrc: Change governor to powersave devfreq: imx8m-ddrc: Use the opps acquired from EL3 devfreq: imx8m-ddrc: Add late system sleep PM ops interconnect: imx: Switch from imx_icc_node_adj_desc to fsl,icc-id node assignment interconnect: imx8: Remove the imx_icc_node_adj_desc interconnect: imx8mq: Add the pl301_per_m and pl301_wakeup nodes and subnodes interconnect: imx8mq: Add of_match_table interconnect: imx: Add imx_icc_get_bw and imx_icc_aggregate functions arm64: dts: imx8mq: Add fsl,icc-id property to ddrc node arm64: dts: imx8mq: Add fsl,icc-id to noc node arm64: dts: imx8mq: Add all pl301 nodes arm64: dts: imx8mq: Add the interconnect node arm64: dts: imx8mq: Add interconnect properties to icc consumer nodes net: ethernet: fec_main: Add interconnect support mmc: sdhci-esdhc-imx: Add interconnect support arm64: defconfig: Add necessary configs for icc+devfreq on i.MX8MQ arch/arm64/boot/dts/freescale/imx8mq.dtsi | 222 +++++++++++++++++++++- arch/arm64/configs/defconfig | 9 +- drivers/devfreq/imx-bus.c | 42 +--- drivers/devfreq/imx8m-ddrc.c | 78 +++----- drivers/interconnect/imx/imx.c | 92 +++++---- drivers/interconnect/imx/imx.h | 19 +- drivers/interconnect/imx/imx8mm.c | 32 +--- drivers/interconnect/imx/imx8mn.c | 28 +-- drivers/interconnect/imx/imx8mq.c | 59 +++--- drivers/mmc/host/sdhci-esdhc-imx.c | 27 +++ drivers/net/ethernet/freescale/fec.h | 3 + drivers/net/ethernet/freescale/fec_main.c | 19 ++ include/dt-bindings/interconnect/imx8mq.h | 9 + 13 files changed, 422 insertions(+), 217 deletions(-)