Re: [PATCH V2 8/9] interconnect: imx: configure NoC mode/prioriry/ext_control

[Georgi Djakov <djakov@kernel.org>]

Hi Peng,

On 16.06.22 10:33, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Introduce imx_icc_noc_setting structure to describe a master port setting
> Pass imx_icc_noc_setting as a parameter from specific driver
> Set priority level, mode, ext control in imx_icc_node_set
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>   drivers/interconnect/imx/imx.c    | 43 ++++++++++++++++++++++++++----
>   drivers/interconnect/imx/imx.h    | 44 ++++++++++++++++++++++++++++++-
>   drivers/interconnect/imx/imx8mm.c |  2 +-
>   drivers/interconnect/imx/imx8mn.c |  2 +-
>   drivers/interconnect/imx/imx8mq.c |  2 +-
>   5 files changed, 84 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c
> index 78557fe6da2c..bd728caf2b85 100644

[..]

> @@ -37,8 +40,24 @@ static int imx_icc_node_set(struct icc_node *node)
>   {
>   	struct device *dev = node->provider->dev;
>   	struct imx_icc_node *node_data = node->data;
> +	void __iomem *base;
> +	u32 prio;
>   	u64 freq;
>   
> +	if (node_data->setting && !node_data->setting->ignore && node->peak_bw) {
> +		base = node_data->setting->reg + node_data->imx_provider->noc_base;
> +		if (node_data->setting->mode == IMX_NOC_MODE_FIXED) {
> +			prio = node_data->setting->prio_level;
> +			prio = PRIORITY_COMP_MARK | (prio << 8) | prio;
> +			writel(prio, base + IMX_NOC_PRIO_REG);
> +			writel(node_data->setting->mode, base + IMX_NOC_MODE_REG);
> +			writel(node_data->setting->ext_control, base + IMX_NOC_EXT_CTL_REG);
> +		} else {
> +			dev_info(dev, "mode: %d not supported\n", node_data->setting->mode);
> +			return -ENOTSUPP;

Nit: I believe that -EOPNOTSUPP is the preferred error code.

> +		}
> +	}
> +
>   	if (!node_data->qos_dev)
>   		return 0;
>   

[..]

> @@ -237,7 +263,8 @@ static int get_max_node_id(struct imx_icc_node_desc *nodes, int nodes_count)
>   }
>   
>   int imx_icc_register(struct platform_device *pdev,
> -		     struct imx_icc_node_desc *nodes, int nodes_count)
> +		     struct imx_icc_node_desc *nodes, int nodes_count,
> +		     struct imx_icc_noc_setting *settings)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct icc_onecell_data *data;
> @@ -267,13 +294,19 @@ int imx_icc_register(struct platform_device *pdev,
>   	provider->dev->of_node = dev->parent->of_node;
>   	platform_set_drvdata(pdev, imx_provider);
>   
> +	if (settings) {
> +		imx_provider->noc_base = devm_of_iomap(dev, provider->dev->of_node, 0, NULL);
> +		if (!imx_provider->noc_base)

devm_of_iomap() returns ERR_PTR(). So we should check it with IS_ERR().

Thanks,
Georgi

> +			return PTR_ERR(imx_provider->noc_base);
> +	}
> +
>   	ret = icc_provider_add(provider);
>   	if (ret) {
>   		dev_err(dev, "error adding interconnect provider: %d\n", ret);
>   		return ret;
>   	}
>   
> -	ret = imx_icc_register_nodes(imx_provider, nodes, nodes_count);
> +	ret = imx_icc_register_nodes(imx_provider, nodes, nodes_count, settings);
>   	if (ret)
>   		goto provider_del;
>