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

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

[Iwona Winiarska]

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 <iwona.winiarska@intel.com>
---
 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

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

[Andrew Jeffery]

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 <iwona.winiarska@intel.com>

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

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

[Winiarska, Iwona]

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 <iwona.winiarska@intel.com>
> 
> 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
> 

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

[Andrew Jeffery]

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 <iwona.winiarska@intel.com>
> > 
> > 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

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

[Billy Tsai]

> 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 <iwona.winiarska@intel.com>
> > >
> > > 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 <billy_tsai@aspeedtech.com>

Billy