linux-kernel - Re: [PATCH 2/2] cpufreq: ti-cpufreq: Make the AM625 efuse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240903060700.ekytq3rj3wz3yw4k@lcpd911>
Date: Tue, 3 Sep 2024 11:37:00 +0530
From: Dhruva Gole <d-gole@...com>
To: Bryan Brattlof <bb@...com>
CC: "Rafael J . Wysocki" <rafael@...nel.org>,
        Viresh Kumar
	<viresh.kumar@...aro.org>,
        Tero Kristo <kristo@...nel.org>, Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
	<conor+dt@...nel.org>, Nishanth Menon <nm@...com>,
        Vignesh Raghavendra
	<vigneshr@...com>, <vibhore@...com>,
        <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/2] cpufreq: ti-cpufreq: Make the AM625 efuse_offset 0

Hi Bryan,

On Sep 02, 2024 at 18:34:39 -0500, Bryan Brattlof wrote:
> On September  2, 2024 thus sayeth Dhruva Gole:
> > Since the efuse_offset is basically derived from the syscon node, we no
> > longer need to use any efuse_offset for AM625. This is in line with how
> > the AM62Ax and AM62Px are already doing.
> > 
> > Signed-off-by: Dhruva Gole <d-gole@...com>
> > ---
> >  drivers/cpufreq/ti-cpufreq.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/ti-cpufreq.c b/drivers/cpufreq/ti-cpufreq.c
> > index ba621ce1cdda..98e320832f78 100644
> > --- a/drivers/cpufreq/ti-cpufreq.c
> > +++ b/drivers/cpufreq/ti-cpufreq.c
> > @@ -313,7 +313,7 @@ static const struct soc_device_attribute k3_cpufreq_soc[] = {
> >  
> >  static struct ti_cpufreq_soc_data am625_soc_data = {
> >  	.efuse_xlate = am625_efuse_xlate,
> > -	.efuse_offset = 0x0018,
> > +	.efuse_offset = 0x0,
> >  	.efuse_mask = 0x07c0,
> >  	.efuse_shift = 0x6,
> >  	.rev_offset = 0x0014,
> 
> This may work but it really shouldn't. Unfortunately when I sent out the 
> am62ax and am62px support I missed the .rev_offset and now it's pointed 
> to some random spot in the WKUP_CTRL_MMR space. How it worked on my 

Thanks for taking the time to review.

If by "this" you mean the rev offset, then it's anyway unused. I have
mentioned the dependency [1] in the cover letter where I am using the
revision info based on the socinfo driver.

I have also applied the AM62Ax [2] patches that you'd sent and
tested it here on AM62A [3].

> bench was complete luck (or bad luck depending on how we view this).
> 
> We'll need to pull out a OMAP3 chip and try to separate the OMAP and K3 
> OPN decoding before we can move the .efuse_offset
> 
> ~Bryan

The efuse part of this driver is completely fine, and IMHO doesn't need
any playing with. What does need MAJOR fixing is the rev part of it. I
am wondering if we even really use the revision info to determine what
OPP the devices support in most cases (I am confident that it's useless
for AM62, 62A and 62P). In such cases we should rather even make the
get_revision part optional. I am open to suggestions if there's another
way to do it than what I have done in this series and in the dependency[1].

Looking at `drivers/cpufreq/sti-cpufreq.c`: If they fail to obtain a
version then they simply do version[0] = BIT(major);
If that's acceptable for devices that have revision as _don't care_ then I
can do that as well.

I am also open to the idea of moving the new K3 devices into a
new ti-k3-cpufreq driver if required, to avoid over complicating in this driver itself
with more quirks and legacy vs new SoC differences which exist at
a both SW and HW layer.

Perhaps Viresh/ Nishanth can comment on what they think is the best way
to move forward.

But otherwise,
I don't think these patches are fundamentally wrong or that they won't
work, unless there's something I've missed.

[1] https://lore.kernel.org/all/20240902092135.2826470-1-d-gole@ti.com
[2] https://lore.kernel.org/all/20240826-opp-v3-1-0934f8309e13@ti.com/
[3] https://gist.github.com/DhruvaG2000/d0c360b0bd7e43d0fd28cfe3eab941d2

-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated