RE: [PATCH 4/8] soc: imx: add icc paths for i.MX8MP media blk ctrl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> Subject: Re: [PATCH 4/8] soc: imx: add icc paths for i.MX8MP media blk ctrl
> 
> 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 ?

In the NXP ATF, the NoC settings are only configured during power on.
So I just follow ATF implementation.

Per IC design team, SW only get a value when power up. So I would
not diverge here.

If bandwidth in NoC registers are taken into control, release is needed.

> 
> > +
> >  	/* 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.

The current settings are only for priority, ext_control, mode settings. Actually
bandwidth settings are not provided. The value here is just to make
icc_bulk_set_bw could configure HW.

Yes, the values are not very good, we could step by step to move on
to make better.

> 
> > +		}
> > +
> > +		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.

I have no idea. I just follow the DoC to list them.

Thanks,
Peng.

> 
> >  	},
> >  	[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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux