Hi Peng, Thank you for the patch. On Wed, Jun 01, 2022 at 05:45:33PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@xxxxxxx> > > Add interconnect paths for i.MX8MP media blk ctrl > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx> > --- > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 1 + > drivers/soc/imx/imx8m-blk-ctrl.c | 31 +++++++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > index d9542dfff83f..2a1c6ff37e03 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > @@ -4,6 +4,7 @@ > */ > > #include <dt-bindings/clock/imx8mp-clock.h> > +#include <dt-bindings/interconnect/fsl,imx8mp.h> > #include <dt-bindings/power/imx8mp-power.h> > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/input/input.h> > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c > index 7f49385ed2f8..423cac0c9cb6 100644 > --- a/drivers/soc/imx/imx8m-blk-ctrl.c > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c > @@ -5,6 +5,7 @@ > */ > > #include <linux/device.h> > +#include <linux/interconnect.h> > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/platform_device.h> > @@ -37,6 +38,8 @@ struct imx8m_blk_ctrl_domain_data { > const char *name; > const char * const *clk_names; > int num_clks; > + const char * const *path_names; > + int num_paths; > const char *gpc_name; > u32 rst_mask; > u32 clk_mask; > @@ -52,11 +55,13 @@ struct imx8m_blk_ctrl_domain_data { > }; > > #define DOMAIN_MAX_CLKS 4 > +#define DOMAIN_MAX_PATHS 4 > > struct imx8m_blk_ctrl_domain { > struct generic_pm_domain genpd; > const struct imx8m_blk_ctrl_domain_data *data; > struct clk_bulk_data clks[DOMAIN_MAX_CLKS]; > + struct icc_bulk_data paths[DOMAIN_MAX_PATHS]; > struct device *power_dev; > struct imx8m_blk_ctrl *bc; > }; > @@ -117,6 +122,10 @@ static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd) > if (data->mipi_phy_rst_mask) > regmap_set_bits(bc->regmap, BLK_MIPI_RESET_DIV, data->mipi_phy_rst_mask); > > + ret = icc_bulk_set_bw(data->num_paths, domain->paths); > + if (ret) > + dev_err(bc->dev, "failed to set icc bw\n"); Don't we need to "release" the bandwidth on power off ? > + > /* disable upstream clocks */ > clk_bulk_disable_unprepare(data->num_clks, domain->clks); > > @@ -228,6 +237,18 @@ static int imx8m_blk_ctrl_probe(struct platform_device *pdev) > for (j = 0; j < data->num_clks; j++) > domain->clks[j].id = data->clk_names[j]; > > + for (j = 0; j < data->num_paths; j++) { > + domain->paths[j].name = data->path_names[j]; > + domain->paths[j].avg_bw = INT_MAX; > + domain->paths[j].peak_bw = INT_MAX; That's harsh :-) I suppose it's good enough to start with, but I wonder if we'll need more control later. > + } > + > + ret = devm_of_icc_bulk_get(dev, data->num_paths, domain->paths); > + if (ret) { > + dev_err_probe(dev, ret, "failed to get noc entries\n"); > + goto cleanup_pds; > + } > + > ret = devm_clk_bulk_get(dev, data->num_clks, domain->clks); > if (ret) { > dev_err_probe(dev, ret, "failed to get clock\n"); > @@ -647,6 +668,8 @@ static const struct imx8m_blk_ctrl_domain_data imx8mp_media_blk_ctl_domain_data[ > .gpc_name = "lcdif1", > .rst_mask = BIT(4) | BIT(5) | BIT(23), > .clk_mask = BIT(4) | BIT(5) | BIT(23), > + .path_names = (const char *[]){"lcdif-rd", "lcdif-wr"}, > + .num_paths = 2, > }, > [IMX8MP_MEDIABLK_PD_ISI] = { > .name = "mediablk-isi", > @@ -655,6 +678,8 @@ static const struct imx8m_blk_ctrl_domain_data imx8mp_media_blk_ctl_domain_data[ > .gpc_name = "isi", > .rst_mask = BIT(6) | BIT(7), > .clk_mask = BIT(6) | BIT(7), > + .path_names = (const char *[]){"isi0", "isi1", "isi2"}, > + .num_paths = 3, > }, > [IMX8MP_MEDIABLK_PD_MIPI_CSI2_2] = { > .name = "mediablk-mipi-csi2-2", > @@ -672,6 +697,8 @@ static const struct imx8m_blk_ctrl_domain_data imx8mp_media_blk_ctl_domain_data[ > .gpc_name = "lcdif2", > .rst_mask = BIT(11) | BIT(12) | BIT(24), > .clk_mask = BIT(11) | BIT(12) | BIT(24), > + .path_names = (const char *[]){"lcdif-rd", "lcdif-wr"}, > + .num_paths = 2, > }, > [IMX8MP_MEDIABLK_PD_ISP] = { > .name = "mediablk-isp", > @@ -680,6 +707,8 @@ static const struct imx8m_blk_ctrl_domain_data imx8mp_media_blk_ctl_domain_data[ > .gpc_name = "isp", > .rst_mask = BIT(16) | BIT(17) | BIT(18), > .clk_mask = BIT(16) | BIT(17) | BIT(18), > + .path_names = (const char *[]){"isp0", "isp1"}, > + .num_paths = 2, As far as I understand, there's a single power domain for both ISP instances (I'm not entirely sure this is on purpose or a design mistake, but that's another story), but one interconnect path for each ISP instance. Would there be any use case for controlling them separately ? I can't really think of any myself. > }, > [IMX8MP_MEDIABLK_PD_DWE] = { > .name = "mediablk-dwe", > @@ -688,6 +717,8 @@ static const struct imx8m_blk_ctrl_domain_data imx8mp_media_blk_ctl_domain_data[ > .gpc_name = "dwe", > .rst_mask = BIT(19) | BIT(20) | BIT(21), > .clk_mask = BIT(19) | BIT(20) | BIT(21), > + .path_names = (const char *[]){"dwe"}, > + .num_paths = 1, > }, > [IMX8MP_MEDIABLK_PD_MIPI_DSI_2] = { > .name = "mediablk-mipi-dsi-2", -- Regards, Laurent Pinchart