| Message ID | 20241002022138.29241-4-pablo.sun@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable Mali GPU on MediaTek Genio 700 EVK | expand |
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
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
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
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
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
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
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
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(+)