Re: [PATCH 10/13] clk: qcom: cpu-8996: fix ACD initialization

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 12/01/2023 16:35, Konrad Dybcio wrote:
> On 11.01.2023 20:20, Dmitry Baryshkov wrote:
> > The vendor kernel applies different order while programming SSSCTL and
> > L2ACDCR registers on power and performance clusters. However it was
> > demonstrated that doing this upstream results in the board reset. Make
> > both clusters use the same sequence, which fixes the reset.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> I think we should look for the source of why this doesn't work,
> e.g. does downstream program it earlier somewhere? Are we
> missing something else that may bite later?

I'm not sure what is the reason for downstream doing init in such 
sequence. Right now I'm sure that doing ACD init with the provided 
sequence fails the boot in some conditions. There might be the 
difference in the CPU init order. Or any other ordering issue. Or the 
lack of the CPR. Or Kryo LDO programming. There is a huge difference 
between vendor's 3.18 and the current 6.x.

I propose to take the patch in, as it fixes the boot and runtime issue 
and revisit it later if any of the problems occur. I don't fancy such 
approach usually, but without the documentation I don't see a way to 
find any particular reason for programming pwr and perf using the 
different order of operations.

> Konrad
> >   drivers/clk/qcom/clk-cpu-8996.c | 20 ++++++++------------
> >   1 file changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
> > index 47c58bb5f21a..1c00eb629b61 100644
> > --- a/drivers/clk/qcom/clk-cpu-8996.c
> > +++ b/drivers/clk/qcom/clk-cpu-8996.c
> > @@ -475,9 +475,9 @@ static int qcom_cpu_clk_msm8996_register_clks(struct device *dev,
> >   	return ret;
> >   }
> >   
> > -#define CPU_AFINITY_MASK 0xFFF
> > -#define PWRCL_CPU_REG_MASK 0x3
> > -#define PERFCL_CPU_REG_MASK 0x103
> > +#define CPU_CLUSTER_AFFINITY_MASK 0xf00
> > +#define PWRCL_AFFINITY_MASK 0x000
> > +#define PERFCL_AFFINITY_MASK 0x100
> >   
> >   #define L2ACDCR_REG 0x580ULL
> >   #define L2ACDTD_REG 0x581ULL
> > @@ -498,21 +498,17 @@ static void qcom_cpu_clk_msm8996_acd_init(struct regmap *regmap)
> >   	if (val == 0x00006a11)
> >   		goto out;
> >   
> > -	hwid = read_cpuid_mpidr() & CPU_AFINITY_MASK;
> > -
> >   	kryo_l2_set_indirect_reg(L2ACDTD_REG, 0x00006a11);
> >   	kryo_l2_set_indirect_reg(L2ACDDVMRC_REG, 0x000e0f0f);
> >   	kryo_l2_set_indirect_reg(L2ACDSSCR_REG, 0x00000601);
> >   
> > -	if (PWRCL_CPU_REG_MASK == (hwid | PWRCL_CPU_REG_MASK)) {
> > -		regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
> > -		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
> > -	}
> > +	kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
> >   
> > -	if (PERFCL_CPU_REG_MASK == (hwid | PERFCL_CPU_REG_MASK)) {
> > -		kryo_l2_set_indirect_reg(L2ACDCR_REG, 0x002c5ffd);
> > +	hwid = read_cpuid_mpidr();
> > +	if ((hwid & CPU_CLUSTER_AFFINITY_MASK) == PWRCL_AFFINITY_MASK)
> > +		regmap_write(regmap, PWRCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
> > +	else
> >   		regmap_write(regmap, PERFCL_REG_OFFSET + SSSCTL_OFFSET, 0xf);
> > -	}
> >   
> >   out:
> >   	spin_unlock_irqrestore(&qcom_clk_acd_lock, flags);

--
With best wishes
Dmitry