2019-12-30 09:13:56

by Peng Fan

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

From: Peng Fan <[email protected]>

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 <[email protected]>
---
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",
--
2.16.4

2020-01-12 02:34:05

by Shawn Guo

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

On Mon, Dec 30, 2019 at 09:13:00AM +0000, Peng Fan wrote:
> From: Peng Fan <[email protected]>
>
> 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 <[email protected]>

@Leonard, do you agree?

Shawn

> ---
> 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",
> --
> 2.16.4
>

2020-01-14 00:43:20

by Leonard Crestez

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

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 <[email protected]>
>>
>> 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 <[email protected]>
>
> @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",