[PATCH 0/2] dts: ti: k3-am62: Use opp_efuse_table for opp-table

[PATCH 0/2] dts: ti: k3-am62: Use opp_efuse_table for opp-table

[Dhruva Gole]

Hi,

In this series, I add another entry in the wkup_conf for the syscon node,
and then use that for the syscon in opp-table. Then, since the driver
can now pick the efuse reg directly, we no longer need the old offset, hence
make it 0 here itself.

This series has 2 patches that need to go in at the same time, hence
preferrably via your tree Viresh if that's fine by all the concerned
maintainers.

It also depends on the series previously sent [1]

[1] Message-Id: <20240902092135.2826470-1-d-gole@ti.com>

Cc: Bryan Brattlof <bb@ti.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
Cc: Vibhore Vardhan <vibhore@ti.com>
Cc: Nishanth Menon <nm@ti.com>

Dhruva Gole (2):
  arm64: dts: ti: k3-am62: use opp_efuse_table for opp-table syscon
  cpufreq: ti-cpufreq: Make the AM625 efuse_offset 0

 arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi | 7 ++++++-
 arch/arm64/boot/dts/ti/k3-am625.dtsi       | 2 +-
 drivers/cpufreq/ti-cpufreq.c               | 2 +-
 3 files changed, 8 insertions(+), 3 deletions(-)


base-commit: ecc768a84f0b8e631986f9ade3118fa37852fef0
-- 
2.34.1

[PATCH 1/2] arm64: dts: ti: k3-am62: use opp_efuse_table for opp-table syscon

[Dhruva Gole]

Add another entry in the wkup_conf for the syscon node, and then use
that for the syscon in opp-table.

Marking entire wkup_conf as "syscon", "simple-mfd" is wrong and needs to
be addressed similar to how other child-nodes in wkup_conf are implemented
in the same file.

Signed-off-by: Dhruva Gole <d-gole@ti.com>
---

This patch was sent previously here [1]. However there were some fixes
needed in the driver to make it work which have to go in as well. The
dependencies have been mentioned in the coverletter and a driver patch
that needs to go in after this patch is included as part of this series
itself.

[1] https://lore.kernel.org/all/20240827080007.2097276-1-d-gole@ti.com/

Cc: Bryan Brattlof <bb@ti.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
Cc: Vibhore Vardhan <vibhore@ti.com>

---
 arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi | 7 ++++++-
 arch/arm64/boot/dts/ti/k3-am625.dtsi       | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
index e0afafd532a5..b2b65e31c7cf 100644
--- a/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi
@@ -10,7 +10,7 @@
 &cbass_wakeup {
 	wkup_conf: syscon@43000000 {
 		bootph-all;
-		compatible = "syscon", "simple-mfd";
+		compatible = "simple-bus";
 		reg = <0x00 0x43000000 0x00 0x20000>;
 		#address-cells = <1>;
 		#size-cells = <1>;
@@ -22,6 +22,11 @@ chipid: chipid@14 {
 			reg = <0x14 0x4>;
 		};
 
+		opp_efuse_table: syscon@18 {
+			compatible = "ti,am62-opp-efuse-table", "syscon";
+			reg = <0x18 0x4>;
+		};
+
 		cpsw_mac_syscon: ethernet-mac-syscon@200 {
 			compatible = "ti,am62p-cpsw-mac-efuse", "syscon";
 			reg = <0x200 0x8>;
diff --git a/arch/arm64/boot/dts/ti/k3-am625.dtsi b/arch/arm64/boot/dts/ti/k3-am625.dtsi
index c3d1db47dc9f..c249883a8a8d 100644
--- a/arch/arm64/boot/dts/ti/k3-am625.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am625.dtsi
@@ -108,7 +108,7 @@ cpu3: cpu@3 {
 	a53_opp_table: opp-table {
 		compatible = "operating-points-v2-ti-cpu";
 		opp-shared;
-		syscon = <&wkup_conf>;
+		syscon = <&opp_efuse_table>;
 
 		opp-200000000 {
 			opp-hz = /bits/ 64 <200000000>;
-- 
2.34.1

[PATCH 2/2] cpufreq: ti-cpufreq: Make the AM625 efuse_offset 0

[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@ti.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,
-- 
2.34.1

Re: [PATCH 2/2] cpufreq: ti-cpufreq: Make the AM625 efuse_offset 0

[Bryan Brattlof]

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@ti.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 
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

Re: [PATCH 2/2] cpufreq: ti-cpufreq: Make the AM625 efuse_offset 0

[Dhruva Gole]

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@ti.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