Re: [PATCH 1/3] clk: imx: pll14xx: avoid modify dram pll

[Leonard Crestez]

On 12.01.2020 04:33, Shawn Guo wrote:
> On Mon, Dec 30, 2019 at 09:13:00AM +0000, Peng Fan wrote:
>> From: Peng Fan <peng.fan@xxxxxxx>
>>
>> The dram pll is only expected to be modified in firmware,
>> so we should only support read clk frequency in Linux Kernel.
>>
>> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> 
> @Leonard, do you agree?

I'm not sure this improves anything.

As far as I understand the only way this could be a problem is if a 
driver deliberately does clk_set_rate on the dram pll and this shouldn't 
happen.

There is an infinite number of ways for clock consumers to break the 
system by manipulating clocks and providers don't need to guard against 
every scenario.

>> ---
>>   drivers/clk/imx/clk-pll14xx.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
>> index 5b0519a81a7a..9288b21d4d59 100644
>> --- a/drivers/clk/imx/clk-pll14xx.c
>> +++ b/drivers/clk/imx/clk-pll14xx.c
>> @@ -69,8 +69,6 @@ struct imx_pll14xx_clk imx_1443x_pll = {
>>   
>>   struct imx_pll14xx_clk imx_1443x_dram_pll = {
>>   	.type = PLL_1443X,
>> -	.rate_table = imx_pll1443x_tbl,
>> -	.rate_count = ARRAY_SIZE(imx_pll1443x_tbl),
>>   	.flags = CLK_GET_RATE_NOCACHE,
>>   };
>>   
>> @@ -376,6 +374,10 @@ static const struct clk_ops clk_pll1443x_ops = {
>>   	.set_rate	= clk_pll1443x_set_rate,
>>   };
>>   
>> +static const struct clk_ops clk_pll1443x_min_ops = {
>> +	.recalc_rate	= clk_pll1443x_recalc_rate,
>> +};
>> +
>>   struct clk_hw *imx_clk_hw_pll14xx(const char *name, const char *parent_name,
>>   				  void __iomem *base,
>>   				  const struct imx_pll14xx_clk *pll_clk)
>> @@ -403,7 +405,10 @@ struct clk_hw *imx_clk_hw_pll14xx(const char *name, const char *parent_name,
>>   			init.ops = &clk_pll1416x_ops;
>>   		break;
>>   	case PLL_1443X:
>> -		init.ops = &clk_pll1443x_ops;
>> +		if (!pll_clk->rate_table)
>> +			init.ops = &clk_pll1443x_min_ops;
>> +		else
>> +			init.ops = &clk_pll1443x_ops;
>>   		break;
>>   	default:
>>   		pr_err("%s: Unknown pll type for pll clk %s\n",