2024-04-17 13:49:23

by Winiarska, Iwona

[permalink] [raw]
Subject: [PATCH] peci: aspeed: Clear clock_divider value before setting it

PECI clock divider is programmed on 10:8 bits of PECI Control register.
Before setting a new value, clear bits read from hardware.

Signed-off-by: Iwona Winiarska <[email protected]>
---
drivers/peci/controller/peci-aspeed.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/peci/controller/peci-aspeed.c b/drivers/peci/controller/peci-aspeed.c
index 7fdc25afcf2f..de7046e6b9c4 100644
--- a/drivers/peci/controller/peci-aspeed.c
+++ b/drivers/peci/controller/peci-aspeed.c
@@ -351,6 +351,7 @@ static int clk_aspeed_peci_set_rate(struct clk_hw *hw, unsigned long rate,
clk_aspeed_peci_find_div_values(this_rate, &msg_timing, &clk_div_exp);

val = readl(aspeed_peci->base + ASPEED_PECI_CTRL);
+ val &= ~ASPEED_PECI_CTRL_CLK_DIV_MASK;
val |= FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, clk_div_exp);
writel(val, aspeed_peci->base + ASPEED_PECI_CTRL);

--
2.44.0



2024-04-17 23:42:22

by Andrew Jeffery

[permalink] [raw]
Subject: Re: [PATCH] peci: aspeed: Clear clock_divider value before setting it

Hi Iwona,

On Wed, 2024-04-17 at 15:48 +0200, Iwona Winiarska wrote:
> PECI clock divider is programmed on 10:8 bits of PECI Control register.
> Before setting a new value, clear bits read from hardware.
>
> Signed-off-by: Iwona Winiarska <[email protected]>

I think it would be best to add a Fixes: tag and Cc: stable in
accordance with the stable tree rules. Are you happy to do so?

Andrew


2024-04-18 13:41:46

by Winiarska, Iwona

[permalink] [raw]
Subject: Re: [PATCH] peci: aspeed: Clear clock_divider value before setting it

On Thu, 2024-04-18 at 09:11 +0930, Andrew Jeffery wrote:
> Hi Iwona,
>
> On Wed, 2024-04-17 at 15:48 +0200, Iwona Winiarska wrote:
> > PECI clock divider is programmed on 10:8 bits of PECI Control register.
> > Before setting a new value, clear bits read from hardware.
> >
> > Signed-off-by: Iwona Winiarska <[email protected]>
>
> I think it would be best to add a Fixes: tag and Cc: stable in
> accordance with the stable tree rules. Are you happy to do so?

Hi!

Technically, the initial value of this register should be 0, but I've added the
clear just to be on the safe side and to not have to rely on that.
I don't think we're ever programming invalid values in the real-world scenarios,
and because of that - I don't think this is stable material.

Thanks
-Iwona

>
> Andrew
>

2024-04-18 23:28:11

by Andrew Jeffery

[permalink] [raw]
Subject: Re: [PATCH] peci: aspeed: Clear clock_divider value before setting it

On Thu, 2024-04-18 at 13:41 +0000, Winiarska, Iwona wrote:
> On Thu, 2024-04-18 at 09:11 +0930, Andrew Jeffery wrote:
> > Hi Iwona,
> >
> > On Wed, 2024-04-17 at 15:48 +0200, Iwona Winiarska wrote:
> > > PECI clock divider is programmed on 10:8 bits of PECI Control register.
> > > Before setting a new value, clear bits read from hardware.
> > >
> > > Signed-off-by: Iwona Winiarska <[email protected]>
> >
> > I think it would be best to add a Fixes: tag and Cc: stable in
> > accordance with the stable tree rules. Are you happy to do so?
>
> Hi!
>
> Technically, the initial value of this register should be 0, but I've added the
> clear just to be on the safe side and to not have to rely on that.

Yeah, it could cause havoc with an unbind/bind sequence if people are
messing with the clocks in between.

> I don't think we're ever programming invalid values in the real-world scenarios,
> and because of that - I don't think this is stable material.

Right, I don't expect people are doing the above in environments where
stability is a concern.

Thanks for elaborating.

Andrew

2024-06-12 02:24:16

by Billy Tsai

[permalink] [raw]
Subject: Re: [PATCH] peci: aspeed: Clear clock_divider value before setting it

> On Thu, 2024-04-18 at 13:41 +0000, Winiarska, Iwona wrote:
> > On Thu, 2024-04-18 at 09:11 +0930, Andrew Jeffery wrote:
> > > Hi Iwona,
> > >
> > > On Wed, 2024-04-17 at 15:48 +0200, Iwona Winiarska wrote:
> > > > PECI clock divider is programmed on 10:8 bits of PECI Control register.
> > > > Before setting a new value, clear bits read from hardware.
> > > >
> > > > Signed-off-by: Iwona Winiarska <[email protected]>
> > >
> > > I think it would be best to add a Fixes: tag and Cc: stable in
> > > accordance with the stable tree rules. Are you happy to do so?
> >
> > Hi!
> >
> > Technically, the initial value of this register should be 0, but I've added the
> > clear just to be on the safe side and to not have to rely on that.

> Yeah, it could cause havoc with an unbind/bind sequence if people are
> messing with the clocks in between.

> > I don't think we're ever programming invalid values in the real-world scenarios,
> > and because of that - I don't think this is stable material.

> Right, I don't expect people are doing the above in environments where
> stability is a concern.

Reviewed-by: Billy Tsai <[email protected]>

Billy