[PATCH v3 0/6] Enable Mali GPU on MediaTek Genio 700 EVK

[PATCH v3 0/6] Enable Mali GPU on MediaTek Genio 700 EVK

[Pablo Sun]

This series is based on linux-next, tag: next-20240927

Enables the GPU on mt8390-genio-700-evk.dts. 
The panfrost driver probed with dmesg:

panfrost 13000000.gpu: clock rate = 390000000
panfrost 13000000.gpu: mali-g57 id 0x9093 major 0x0 minor 0x0 status 0x0
panfrost 13000000.gpu: features: 00000000,000019f7, 
  issues: 00000003,80000400
panfrost 13000000.gpu: Features: L2:0x08130206 Shader:0x00000000
  Tiler:0x00000809 Mem:0x1 MMU:0x00002830 AS:0xff JS:0x7
panfrost 13000000.gpu: shader_present=0x10005 l2_present=0x1
[drm] Initialized panfrost 1.2.0 for 13000000.gpu on minor 0

Changes in v3:
- Drop patch "nvmem: mtk-efuse: Enable postprocess for mt8188 GPU
 speed binning" as suggested by Angelo.
- Add patch "dt-bindings: nvmem: mediatek: efuse: Reuse mt8186-efuse in mt8188"
 to reuse "mediatek,mt8186-efuse" compatible.
- Updated patch "arm64: dts: mediatek: mt8188: Add efuse for GPU speed binning"
 to revise the compatible string and remove Reviewed-By tag.

Pablo Sun (6):
  arm64: dts: mediatek: mt8188: Fix wrong clock provider in MFG1 power
    domain
  clk: mediatek: clk-mt8188-topckgen: Remove univpll from parents of
    mfg_core_tmp
  dt-bindings: nvmem: mediatek: efuse: Reuse mt8186-efuse in mt8188
  arm64: dts: mediatek: mt8188: Add efuse for GPU speed binning
  soc: mediatek: mediatek-regulator-coupler: Support mt8188
  arm64: dts: mediatek: mt8390-genio-700-evk: Enable Mali GPU

 .../bindings/nvmem/mediatek,efuse.yaml        |  4 +++
 arch/arm64/boot/dts/mediatek/mt8188.dtsi      | 13 ++++++--
 .../dts/mediatek/mt8390-genio-700-evk.dts     | 31 +++++++++++++++++++
 drivers/clk/mediatek/clk-mt8188-topckgen.c    |  9 ++++--
 drivers/soc/mediatek/mtk-regulator-coupler.c  |  1 +
 5 files changed, 52 insertions(+), 6 deletions(-)

-- 
2.45.2

[PATCH v3 1/6] arm64: dts: mediatek: mt8188: Fix wrong clock provider in MFG1 power domain

[Pablo Sun]

The clock index "CLK_APMIXED_MFGPLL" belongs to the "apmixedsys" provider,
so fix the index.

In addition, add a "mfg1" label so following commits could set
domain-supply for MFG1 power domain.

Fixes: eaf73e4224a3 ("arm64: dts: mediatek: mt8188: Add support for SoC power domains")
Signed-off-by: Pablo Sun <pablo.sun@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 arch/arm64/boot/dts/mediatek/mt8188.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8188.dtsi b/arch/arm64/boot/dts/mediatek/mt8188.dtsi
index cd27966d2e3c..02a5bb4dbd1f 100644
--- a/arch/arm64/boot/dts/mediatek/mt8188.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8188.dtsi
@@ -956,9 +956,9 @@ mfg0: power-domain@MT8188_POWER_DOMAIN_MFG0 {
 					#size-cells = <0>;
 					#power-domain-cells = <1>;
 
-					power-domain@MT8188_POWER_DOMAIN_MFG1 {
+					mfg1: power-domain@MT8188_POWER_DOMAIN_MFG1 {
 						reg = <MT8188_POWER_DOMAIN_MFG1>;
-						clocks = <&topckgen CLK_APMIXED_MFGPLL>,
+						clocks = <&apmixedsys CLK_APMIXED_MFGPLL>,
 							 <&topckgen CLK_TOP_MFG_CORE_TMP>;
 						clock-names = "mfg", "alt";
 						mediatek,infracfg = <&infracfg_ao>;
-- 
2.45.2

[PATCH v3 2/6] clk: mediatek: clk-mt8188-topckgen: Remove univpll from parents of mfg_core_tmp

[Pablo Sun]

Same as MT8195, MT8188 GPU clock is primarly supplied by the dedicated
mfgpll. The clock "mfg_core_tmp" is only used as an alt clock when
setting mfgpll clock rate.

If we keep the univpll parents from mfg_core_tmp, when setting
GPU frequency to 390000000, the common clock framework would switch
the parent to univpll, instead of setting mfgpll to 390000000:

    mfgpll                            0        0        0   949999756
    univpll                           2        2        0  2340000000
       univpll_d6                     1        1        0   390000000
          top_mfg_core_tmp            1        1        0   390000000
             mfg_ck_fast_ref          1        1        0   390000000
                mfgcfg_bg3d           1        1        0   390000000

This results in failures when subsequent devfreq operations need to
switch to other frequencies. So remove univpll from the parent list.

This solution is taken from commit 72d38ed720e9 ("clk: mediatek:
clk-mt8195-topckgen: Drop univplls from mfg mux parents")

Signed-off-by: Pablo Sun <pablo.sun@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/clk/mediatek/clk-mt8188-topckgen.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8188-topckgen.c b/drivers/clk/mediatek/clk-mt8188-topckgen.c
index c4baf4076ed6..6b07abe9a8f5 100644
--- a/drivers/clk/mediatek/clk-mt8188-topckgen.c
+++ b/drivers/clk/mediatek/clk-mt8188-topckgen.c
@@ -342,11 +342,14 @@ static const char * const dsp7_parents[] = {
 	"univpll_d3"
 };
 
+/*
+ * MFG can be also parented to "univpll_d6" and "univpll_d7":
+ * these have been removed from the parents list to let us
+ * achieve GPU DVFS without any special clock handlers.
+ */
 static const char * const mfg_core_tmp_parents[] = {
 	"clk26m",
-	"mainpll_d5_d2",
-	"univpll_d6",
-	"univpll_d7"
+	"mainpll_d5_d2"
 };
 
 static const char * const camtg_parents[] = {
-- 
2.45.2

[PATCH v3 3/6] dt-bindings: nvmem: mediatek: efuse: Reuse mt8186-efuse in mt8188

[Pablo Sun]

mt8188 has the same GPU speed binning efuse field just
like mt8186, which requires post-processing to convert to the
bit field format specified by OPP table.

Add the binding for the compatible list:
  "mediatek,mt8188-efuse", "mediatek,mt8186-efuse"
so mt8188 uses the same conversion.

Suggested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Pablo Sun <pablo.sun@mediatek.com>
---
 Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
index 32b8c1eb4e80..70815a3329bf 100644
--- a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
+++ b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
@@ -39,6 +39,10 @@ properties:
               - mediatek,mt8195-efuse
               - mediatek,mt8516-efuse
           - const: mediatek,efuse
+      - items:
+          - enum:
+              - mediatek,mt8188-efuse
+          - const: mediatek,mt8186-efuse
       - const: mediatek,mt8173-efuse
         deprecated: true
 
-- 
2.45.2

[PATCH v3 4/6] arm64: dts: mediatek: mt8188: Add efuse for GPU speed binning

[Pablo Sun]

The OPP table of mt8188 GPU contains duplicated frequencies
for different speed bins.

In order to support OPP table, we need to provide the speed bin info
in the efuse data so the GPU driver could properly set the
supported hardware speed bin.

Same as mt8186, the efuse data for mt8188's GPU speed binning
requires post-process to convert the bit field format expected
by the OPP table.

Signed-off-by: Pablo Sun <pablo.sun@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8188.dtsi | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8188.dtsi b/arch/arm64/boot/dts/mediatek/mt8188.dtsi
index 02a5bb4dbd1f..2d9378c16e42 100644
--- a/arch/arm64/boot/dts/mediatek/mt8188.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8188.dtsi
@@ -1744,7 +1744,7 @@ imp_iic_wrap_en: clock-controller@11ec2000 {
 		};
 
 		efuse: efuse@11f20000 {
-			compatible = "mediatek,mt8188-efuse", "mediatek,efuse";
+			compatible = "mediatek,mt8188-efuse", "mediatek,mt8186-efuse";
 			reg = <0 0x11f20000 0 0x1000>;
 			#address-cells = <1>;
 			#size-cells = <1>;
@@ -1752,6 +1752,11 @@ efuse: efuse@11f20000 {
 			lvts_efuse_data1: lvts1-calib@1ac {
 				reg = <0x1ac 0x40>;
 			};
+
+			gpu_speedbin: gpu-speedbin@580 {
+				reg = <0x581 0x1>;
+				bits = <0 3>;
+			};
 		};
 
 		gpu: gpu@13000000 {
@@ -1763,6 +1768,8 @@ gpu: gpu@13000000 {
 				     <GIC_SPI 382 IRQ_TYPE_LEVEL_HIGH 0>,
 				     <GIC_SPI 381 IRQ_TYPE_LEVEL_HIGH 0>;
 			interrupt-names = "job", "mmu", "gpu";
+			nvmem-cells = <&gpu_speedbin>;
+			nvmem-cell-names = "speed-bin";
 			operating-points-v2 = <&gpu_opp_table>;
 			power-domains = <&spm MT8188_POWER_DOMAIN_MFG2>,
 					<&spm MT8188_POWER_DOMAIN_MFG3>,
-- 
2.45.2

[PATCH v3 5/6] soc: mediatek: mediatek-regulator-coupler: Support mt8188

[Pablo Sun]

The Mali GPU in mt8188 also requires coupled power supplies,
that is, the "vsram" voltage should follow the "vgpu" voltage.
Therefore add the compatible to enable this coupling behavior.

Signed-off-by: Pablo Sun <pablo.sun@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-regulator-coupler.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c b/drivers/soc/mediatek/mtk-regulator-coupler.c
index ad2ed42aa697..0b6a2884145e 100644
--- a/drivers/soc/mediatek/mtk-regulator-coupler.c
+++ b/drivers/soc/mediatek/mtk-regulator-coupler.c
@@ -147,6 +147,7 @@ static int mediatek_regulator_coupler_init(void)
 {
 	if (!of_machine_is_compatible("mediatek,mt8183") &&
 	    !of_machine_is_compatible("mediatek,mt8186") &&
+	    !of_machine_is_compatible("mediatek,mt8188") &&
 	    !of_machine_is_compatible("mediatek,mt8192"))
 		return 0;
 
-- 
2.45.2

[PATCH v3 6/6] arm64: dts: mediatek: mt8390-genio-700-evk: Enable Mali GPU

[Pablo Sun]

Configure GPU regulator supplies and enable GPU for GENIO 700 EVK.

The GPU in MT8390 & MT8188 has two power inputs: "DVDD_GPU" and
"DVDD_SRAM_GPU". In Genio 700 EVK, DVDD_GPU is supplied by
mt6359_vproc2_buck_reg, and DVDD_SRAM_GPU is supplied by
mt6359_vsram_others_ldo_reg.

According to section 5.2 "Recommended Operating Conditions" in
MT8390 IoT Application Processor Datasheet v1.9, The recommended
operating voltage ranges are:

- DVDD_GPU: min 0.55V, max 0.86V, typical 0.75V
- DVDD_SRAM_GPU: min 0.71V, max 0.92V, typical 0.85V

To further optimize power saving, we couple DVDD_SRAM_GPU to
DVDD_GPU according to the following relation:

- For opp-880000000 or lower frequency, keep 0.75V
- For opp-915000000 and higher, DVDD_SRAM_GPU should follow
  DVDD_GPU. The exact voltage for DVDD_GPU should be decided by
  speed binning.

This rule is derived from the OPP table in the link.

In addition, set the voltage spread to 6250 uV, the step size of
'ldo_vsram_others' regulator of mt6359, otherwise the regulator
set_voltage operation fails.

Link: https://gitlab.com/mediatek/aiot/rity/meta-mediatek-bsp/-/blob/eedd6aedd4b0cfc0ee79b9c9b9650dfa73cf87f6/recipes-kernel/dtbo/mt8390/gpu-mali.dts
Signed-off-by: Pablo Sun <pablo.sun@mediatek.com>
Suggested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../dts/mediatek/mt8390-genio-700-evk.dts     | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8390-genio-700-evk.dts b/arch/arm64/boot/dts/mediatek/mt8390-genio-700-evk.dts
index 1474bef7e754..0a6c9871b41e 100644
--- a/arch/arm64/boot/dts/mediatek/mt8390-genio-700-evk.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8390-genio-700-evk.dts
@@ -190,6 +190,11 @@ usb_p2_vbus: regulator-10 {
 	};
 };
 
+&gpu {
+	mali-supply = <&mt6359_vproc2_buck_reg>;
+	status = "okay";
+};
+
 &i2c0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&i2c0_pins>;
@@ -253,6 +258,14 @@ &i2c6 {
 	status = "okay";
 };
 
+&mfg0 {
+	domain-supply = <&mt6359_vproc2_buck_reg>;
+};
+
+&mfg1 {
+	domain-supply = <&mt6359_vsram_others_ldo_reg>;
+};
+
 &mmc0 {
 	status = "okay";
 	pinctrl-names = "default", "state_uhs";
@@ -314,6 +327,15 @@ &mt6359_vpa_buck_reg {
 	regulator-max-microvolt = <3100000>;
 };
 
+&mt6359_vproc2_buck_reg {
+	/* The name "vgpu" is required by mtk-regulator-coupler */
+	regulator-name = "vgpu";
+	regulator-min-microvolt = <550000>;
+	regulator-max-microvolt = <800000>;
+	regulator-coupled-with = <&mt6359_vsram_others_ldo_reg>;
+	regulator-coupled-max-spread = <6250>;
+};
+
 &mt6359_vpu_buck_reg {
 	regulator-always-on;
 };
@@ -326,6 +348,15 @@ &mt6359_vsim1_ldo_reg {
 	regulator-enable-ramp-delay = <480>;
 };
 
+&mt6359_vsram_others_ldo_reg {
+	/* The name "vsram_gpu" is required by mtk-regulator-coupler */
+	regulator-name = "vsram_gpu";
+	regulator-min-microvolt = <750000>;
+	regulator-max-microvolt = <800000>;
+	regulator-coupled-with = <&mt6359_vproc2_buck_reg>;
+	regulator-coupled-max-spread = <6250>;
+};
+
 &mt6359_vufs_ldo_reg {
 	regulator-always-on;
 };
-- 
2.45.2

Re: [PATCH v3 3/6] dt-bindings: nvmem: mediatek: efuse: Reuse mt8186-efuse in mt8188

[Krzysztof Kozlowski]

On Wed, Oct 02, 2024 at 10:21:35AM +0800, Pablo Sun wrote:
> mt8188 has the same GPU speed binning efuse field just
> like mt8186, which requires post-processing to convert to the
> bit field format specified by OPP table.
> 
> Add the binding for the compatible list:
>   "mediatek,mt8188-efuse", "mediatek,mt8186-efuse"
> so mt8188 uses the same conversion.
> 
> Suggested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Pablo Sun <pablo.sun@mediatek.com>
> ---
>  Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> index 32b8c1eb4e80..70815a3329bf 100644
> --- a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> @@ -39,6 +39,10 @@ properties:
>                - mediatek,mt8195-efuse
>                - mediatek,mt8516-efuse
>            - const: mediatek,efuse
> +      - items:
> +          - enum:
> +              - mediatek,mt8188-efuse
> +          - const: mediatek,mt8186-efuse

And this is not compatible with generic one? This is confusing. Why are
you adding generic fallbacks if they are not valid?

Best regards,
Krzysztof

Re: [PATCH v3 3/6] dt-bindings: nvmem: mediatek: efuse: Reuse mt8186-efuse in mt8188

[AngeloGioacchino Del Regno]

Il 02/10/24 08:11, Krzysztof Kozlowski ha scritto:
> On Wed, Oct 02, 2024 at 10:21:35AM +0800, Pablo Sun wrote:
>> mt8188 has the same GPU speed binning efuse field just
>> like mt8186, which requires post-processing to convert to the
>> bit field format specified by OPP table.
>>
>> Add the binding for the compatible list:
>>    "mediatek,mt8188-efuse", "mediatek,mt8186-efuse"
>> so mt8188 uses the same conversion.
>>
>> Suggested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> Signed-off-by: Pablo Sun <pablo.sun@mediatek.com>
>> ---
>>   Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
>> index 32b8c1eb4e80..70815a3329bf 100644
>> --- a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
>> +++ b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
>> @@ -39,6 +39,10 @@ properties:
>>                 - mediatek,mt8195-efuse
>>                 - mediatek,mt8516-efuse
>>             - const: mediatek,efuse
>> +      - items:
>> +          - enum:
>> +              - mediatek,mt8188-efuse
>> +          - const: mediatek,mt8186-efuse
> 
> And this is not compatible with generic one? This is confusing. Why are
> you adding generic fallbacks if they are not valid?
> 

It was my suggestion to start dropping the usage of the generic "mediatek,efuse"
fallback, as I've seen multiple times feedback saying to not use generic fallbacks.

Was that wrong?

Cheers,
Angelo

Re: (subset) [PATCH v3 0/6] Enable Mali GPU on MediaTek Genio 700 EVK

[AngeloGioacchino Del Regno]

On Wed, 02 Oct 2024 10:21:32 +0800, Pablo Sun wrote:
> This series is based on linux-next, tag: next-20240927
> 
> Enables the GPU on mt8390-genio-700-evk.dts.
> The panfrost driver probed with dmesg:
> 
> panfrost 13000000.gpu: clock rate = 390000000
> panfrost 13000000.gpu: mali-g57 id 0x9093 major 0x0 minor 0x0 status 0x0
> panfrost 13000000.gpu: features: 00000000,000019f7,
>   issues: 00000003,80000400
> panfrost 13000000.gpu: Features: L2:0x08130206 Shader:0x00000000
>   Tiler:0x00000809 Mem:0x1 MMU:0x00002830 AS:0xff JS:0x7
> panfrost 13000000.gpu: shader_present=0x10005 l2_present=0x1
> [drm] Initialized panfrost 1.2.0 for 13000000.gpu on minor 0
> 
> [...]

Applied to v6.12-next/soc, thanks!

[5/6] soc: mediatek: mediatek-regulator-coupler: Support mt8188
      https://git.kernel.org/mediatek/c/32ab0090

Cheers,
Angelo

Re: (subset) [PATCH v3 0/6] Enable Mali GPU on MediaTek Genio 700 EVK

[AngeloGioacchino Del Regno]

On Wed, 02 Oct 2024 10:21:32 +0800, Pablo Sun wrote:
> This series is based on linux-next, tag: next-20240927
> 
> Enables the GPU on mt8390-genio-700-evk.dts.
> The panfrost driver probed with dmesg:
> 
> panfrost 13000000.gpu: clock rate = 390000000
> panfrost 13000000.gpu: mali-g57 id 0x9093 major 0x0 minor 0x0 status 0x0
> panfrost 13000000.gpu: features: 00000000,000019f7,
>   issues: 00000003,80000400
> panfrost 13000000.gpu: Features: L2:0x08130206 Shader:0x00000000
>   Tiler:0x00000809 Mem:0x1 MMU:0x00002830 AS:0xff JS:0x7
> panfrost 13000000.gpu: shader_present=0x10005 l2_present=0x1
> [drm] Initialized panfrost 1.2.0 for 13000000.gpu on minor 0
> 
> [...]

Applied to v6.12-next/dts64, thanks!

[1/6] arm64: dts: mediatek: mt8188: Fix wrong clock provider in MFG1 power domain
      https://git.kernel.org/mediatek/c/4007651c
[6/6] arm64: dts: mediatek: mt8390-genio-700-evk: Enable Mali GPU
      https://git.kernel.org/mediatek/c/0158b773

Cheers,
Angelo

Re: [PATCH v3 3/6] dt-bindings: nvmem: mediatek: efuse: Reuse mt8186-efuse in mt8188

[Rob Herring]

On Wed, Oct 02, 2024 at 09:42:32AM +0200, AngeloGioacchino Del Regno wrote:
> Il 02/10/24 08:11, Krzysztof Kozlowski ha scritto:
> > On Wed, Oct 02, 2024 at 10:21:35AM +0800, Pablo Sun wrote:
> > > mt8188 has the same GPU speed binning efuse field just
> > > like mt8186, which requires post-processing to convert to the
> > > bit field format specified by OPP table.

What about all the other efuses? The fallback needs to be a subset of 
the 1st compatible.

> > > 
> > > Add the binding for the compatible list:
> > >    "mediatek,mt8188-efuse", "mediatek,mt8186-efuse"
> > > so mt8188 uses the same conversion.
> > > 
> > > Suggested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > Signed-off-by: Pablo Sun <pablo.sun@mediatek.com>
> > > ---
> > >   Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> > > index 32b8c1eb4e80..70815a3329bf 100644
> > > --- a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> > > +++ b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> > > @@ -39,6 +39,10 @@ properties:
> > >                 - mediatek,mt8195-efuse
> > >                 - mediatek,mt8516-efuse
> > >             - const: mediatek,efuse
> > > +      - items:
> > > +          - enum:
> > > +              - mediatek,mt8188-efuse
> > > +          - const: mediatek,mt8186-efuse
> > 
> > And this is not compatible with generic one? This is confusing. Why are
> > you adding generic fallbacks if they are not valid?
> > 
> 
> It was my suggestion to start dropping the usage of the generic "mediatek,efuse"
> fallback, as I've seen multiple times feedback saying to not use generic fallbacks.
> 
> Was that wrong?

No, but any fallback seems seems a bit odd here. It's one of those 
things that's going to change with every chip.

Rob

Re: [PATCH v3 3/6] dt-bindings: nvmem: mediatek: efuse: Reuse mt8186-efuse in mt8188

[Krzysztof Kozlowski]

On Wed, Oct 02, 2024 at 09:42:32AM +0200, AngeloGioacchino Del Regno wrote:
> Il 02/10/24 08:11, Krzysztof Kozlowski ha scritto:
> > On Wed, Oct 02, 2024 at 10:21:35AM +0800, Pablo Sun wrote:
> > > mt8188 has the same GPU speed binning efuse field just
> > > like mt8186, which requires post-processing to convert to the
> > > bit field format specified by OPP table.
> > > 
> > > Add the binding for the compatible list:
> > >    "mediatek,mt8188-efuse", "mediatek,mt8186-efuse"
> > > so mt8188 uses the same conversion.
> > > 
> > > Suggested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > Signed-off-by: Pablo Sun <pablo.sun@mediatek.com>
> > > ---
> > >   Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> > > index 32b8c1eb4e80..70815a3329bf 100644
> > > --- a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> > > +++ b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> > > @@ -39,6 +39,10 @@ properties:
> > >                 - mediatek,mt8195-efuse
> > >                 - mediatek,mt8516-efuse
> > >             - const: mediatek,efuse
> > > +      - items:
> > > +          - enum:
> > > +              - mediatek,mt8188-efuse
> > > +          - const: mediatek,mt8186-efuse
> > 
> > And this is not compatible with generic one? This is confusing. Why are
> > you adding generic fallbacks if they are not valid?
> > 
> 
> It was my suggestion to start dropping the usage of the generic "mediatek,efuse"
> fallback, as I've seen multiple times feedback saying to not use generic fallbacks.
> 
> Was that wrong?

No, just nothing provided the background that such change is
intentional. Please mention in commit msg that the preferred way from
now on is not using the generic fallback.  Maybe even add it to the
binding itself as comment, so people won't grow the enum with fallback.

Best regards,
Krzysztof

Re: [PATCH v3 3/6] dt-bindings: nvmem: mediatek: efuse: Reuse mt8186-efuse in mt8188

[AngeloGioacchino Del Regno]

Il 03/10/24 10:13, Krzysztof Kozlowski ha scritto:
> On Wed, Oct 02, 2024 at 09:42:32AM +0200, AngeloGioacchino Del Regno wrote:
>> Il 02/10/24 08:11, Krzysztof Kozlowski ha scritto:
>>> On Wed, Oct 02, 2024 at 10:21:35AM +0800, Pablo Sun wrote:
>>>> mt8188 has the same GPU speed binning efuse field just
>>>> like mt8186, which requires post-processing to convert to the
>>>> bit field format specified by OPP table.
>>>>
>>>> Add the binding for the compatible list:
>>>>     "mediatek,mt8188-efuse", "mediatek,mt8186-efuse"
>>>> so mt8188 uses the same conversion.
>>>>
>>>> Suggested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>> Signed-off-by: Pablo Sun <pablo.sun@mediatek.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
>>>> index 32b8c1eb4e80..70815a3329bf 100644
>>>> --- a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
>>>> +++ b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
>>>> @@ -39,6 +39,10 @@ properties:
>>>>                  - mediatek,mt8195-efuse
>>>>                  - mediatek,mt8516-efuse
>>>>              - const: mediatek,efuse
>>>> +      - items:
>>>> +          - enum:
>>>> +              - mediatek,mt8188-efuse
>>>> +          - const: mediatek,mt8186-efuse
>>>
>>> And this is not compatible with generic one? This is confusing. Why are
>>> you adding generic fallbacks if they are not valid?
>>>
>>
>> It was my suggestion to start dropping the usage of the generic "mediatek,efuse"
>> fallback, as I've seen multiple times feedback saying to not use generic fallbacks.
>>
>> Was that wrong?
> 
> No, just nothing provided the background that such change is
> intentional. Please mention in commit msg that the preferred way from
> now on is not using the generic fallback.  Maybe even add it to the
> binding itself as comment, so people won't grow the enum with fallback.
> 

Cool. Thanks for the explanation, krzk.

Pablo, can you please add a comment in the binding saying that no more entries
shall be added to the generic fallback set?

Having a comment there, instead of just into the commit message, would be (imo)
better... as then, anyone trying to add new compatibles will be more likely to
read that.

Cheers,
Angelo

Re: [PATCH v3 3/6] dt-bindings: nvmem: mediatek: efuse: Reuse mt8186-efuse in mt8188

[Pablo Sun]

Hi Rob, Angelo, and Krzysztof,

Thanks for the thoughtful review, I'd like to follow-up on
Rob's comment:

On 10/3/24 05:11, Rob Herring wrote:
[snip]
> > > diff --git a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> > > index 32b8c1eb4e80..70815a3329bf 100644
> > > --- a/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> > > +++ b/Documentation/devicetree/bindings/nvmem/mediatek,efuse.yaml
> > > @@ -39,6 +39,10 @@ properties:
> > >                 - mediatek,mt8195-efuse
> > >                 - mediatek,mt8516-efuse
> > >             - const: mediatek,efuse
> > > +      - items:
> > > +          - enum:
> > > +              - mediatek,mt8188-efuse
> > > +          - const: mediatek,mt8186-efuse
[snip]
> 
> What about all the other efuses? The fallback needs to be a subset of
> the 1st compatible.
[snip]
> 
> No, but any fallback seems seems a bit odd here. It's one of those
> things that's going to change with every chip.

I think you all agree that the common fallback "mediatek,efuse" should
not longer be used, and such deprecation should be commented both commit
message and the YAML, same as "rockchip,rockchip-efuse" in rockchip-efuse.yaml.

But, Rob has mentioned that I should only define a fallback
if and only if mediatek,mt8186-efuse is a **subset** of mediatek,mt8188-efuse.

It is true that I can merely confirm that they share the same "GPU speed bin"
efuse bit definition and conversion routines.

At this moment I cannot confirm:

- mt8188 has every efuse cells mt8186 defined.
- Every mt8188 efuse cells values must be interpreted the same as mt8186.

So, I don't think mt8186-efuse is a subset of mt8188-efuse in this sense.

Do you think it would be better that we re-use this GPU speed bin conversion
in the eFuse driver implementation, rather than reuse it in the dt-binding?

This is also what Angelo suggested initially for v2 modification.

Many thanks,
Pablo