Re: [PATCH RFC v6 3/9] PM / devfreq: imx: Register interconnect device

[Leonard Crestez <leonard.crestez@nxp.com>]

On 13.12.2019 06:22, Chanwoo Choi wrote:
> Hi,
> 
> On 11/15/19 5:09 AM, Leonard Crestez wrote:
>> There is no single device which can represent the imx interconnect.
>> Instead of adding a virtual one just make the main &noc act as the
>> global interconnect provider.
>>
>> The imx interconnect provider driver will scale the NOC and DDRC based
>> on bandwidth request. More scalable nodes can be added in the future,
>> for example for audio/display/vpu/gpu NICs.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   drivers/devfreq/imx-devfreq.c | 37 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
>> index 620b344e87aa..585d340c0f6e 100644
>> --- a/drivers/devfreq/imx-devfreq.c
>> +++ b/drivers/devfreq/imx-devfreq.c
>> @@ -15,10 +15,11 @@
>>   struct imx_devfreq {
>>   	struct devfreq_dev_profile profile;
>>   	struct devfreq *devfreq;
>>   	struct clk *clk;
>>   	struct devfreq_passive_data passive_data;
>> +	struct platform_device *icc_pdev;
>>   };
>>   
>>   static int imx_devfreq_target(struct device *dev,
>>   			      unsigned long *freq, u32 flags)
>>   {
>> @@ -60,11 +61,40 @@ static int imx_devfreq_get_dev_status(struct device *dev,
>>   	return 0;
>>   }
>>   
>>   static void imx_devfreq_exit(struct device *dev)
>>   {
>> +	struct imx_devfreq *priv = dev_get_drvdata(dev);
>> +
>>   	dev_pm_opp_of_remove_table(dev);
>> +	platform_device_unregister(priv->icc_pdev);
>> +}
>> +
>> +/* imx_devfreq_init_icc() - register matching icc provider if required */
>> +static int imx_devfreq_init_icc(struct device *dev)
>> +{
>> +	struct imx_devfreq *priv = dev_get_drvdata(dev);
>> +	const char *icc_driver_name;
>> +
>> +	if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX))
>> +		return 0;
> 
> It is not proper to check the enable state of CONFIG_INTERCONNECT_IMX configuration
> on device driver. Why don't you add the 'select CONFIG_INTERCONNECT_IMX' on Kconfig?

Because it's optional.

You can disable interconnect support and just tweak frequencies using 
the devfreq sysfs API. But indeed would only really be useful for debugging.

>> +	if (!of_get_property(dev->of_node, "#interconnect-cells", 0))
>> +		return 0;
>> +
>> +	icc_driver_name = of_device_get_match_data(dev);
>> +	if (!icc_driver_name)
>> +		return 0;
>> +
>> +	priv->icc_pdev = platform_device_register_data(
>> +			dev, icc_driver_name, 0, NULL, 0);
>> +	if (IS_ERR(priv->icc_pdev)) {
>> +		dev_err(dev, "failed to register icc provider %s: %ld\n",
>> +				icc_driver_name, PTR_ERR(priv->devfreq));
>> +		return PTR_ERR(priv->devfreq);
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   static int imx_devfreq_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> @@ -120,18 +150,25 @@ static int imx_devfreq_probe(struct platform_device *pdev)
>>   		ret = PTR_ERR(priv->devfreq);
>>   		dev_err(dev, "failed to add devfreq device: %d\n", ret);
>>   		goto err;
>>   	}
>>   
>> +	ret = imx_devfreq_init_icc(dev);
>> +	if (ret)
>> +		goto err;
>> +
>>   	return 0;
>>   
>>   err:
>>   	dev_pm_opp_of_remove_table(dev);
>>   	return ret;
>>   }
>>   
>>   static const struct of_device_id imx_devfreq_of_match[] = {
>> +	{ .compatible = "fsl,imx8mq-noc", .data = "imx8mq-interconnect", },
>> +	{ .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", },
>> +	{ .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", },
>>   	{ .compatible = "fsl,imx8m-noc", },
>>   	{ .compatible = "fsl,imx8m-nic", },
>>   	{ /* sentinel */ },
>>   };
>>   MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);