diff mbox series

[2/5] dt-bindings: mmc: sdhci-msm: constrain reg-names at least for one variant

Message ID 20220707075151.67335-3-krzysztof.kozlowski@linaro.org
State New
Headers show
Series dt-bindings: mmc: / ARM: qcom: correct reg-names and clock entries | expand

Commit Message

Krzysztof Kozlowski July 7, 2022, 7:51 a.m. UTC
The entries in arrays must have fixed order, so the bindings and Linux
driver expecting various combinations of 'reg' addresses was never
actually conforming to guidelines.

Specifically Linux driver always expects 'core' reg entry as second
item, but some DTSes are having there 'cqhci'.

Bring at least some sanity here by enforcing one known (used in DTS)
configuration of 'reg': 'hc' followed by 'core' for older variants.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/mmc/sdhci-msm.yaml       | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Doug Anderson July 7, 2022, 2:31 p.m. UTC | #1
Hi,

On Thu, Jul 7, 2022 at 1:04 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> The entries in arrays must have fixed order, so the bindings and Linux
> driver expecting various combinations of 'reg' addresses was never
> actually conforming to guidelines.
>
> Specifically Linux driver always expects 'core' reg entry as second
> item, but some DTSes are having there 'cqhci'.

This is a bit misleading and makes it sound like we've got a bug. In
truth the Linux driver looks at the compatible string. If it sees any
compatible listed as "v5" (or a slight variant of v5 to handle a
workaround for sc7180 / sdm845) then it _doesn't_ expect 'core' reg as
the second entry. See the variable `mci_removed`. The old bindings
".txt" file also had this to say:

                For SDCC version 5.0.0, MCI registers are removed from SDCC
                interface and some registers are moved to HC. New compatible
                string is added to support this change - "qcom,sdhci-msm-v5".

So I guess that means this is the documentation for all of the
combinations you have listed:

* hc only - v5 controller w/out CQE / ICE

* hc + core - v4 controller w/out CQE / ICE

* hc + cqhci - v5 controller w/ CQE and w/out ICE

* hc + cqhci + ice - v5 controller w/ CQE / ICE

* hc + core + cqhci + ice - v4 controller w/ CQE / ICE

Said another way, before v5 the "core" range existed. After v5 it
apparently doesn't so there's no way we could have specified it.

You'll notice that one of the options above implies that a v4
controller (with "core" specified) can have CQE and ICE. Is this
actually true, or was it a misunderstanding in the .txt to .yaml
conversion?

If it's true that a v4 controller can have CQE and ICE then your patch
is wrong in asserting that v4 controllers have only "hc" and "core".

If a v4 controller _can't_ have CQE and ICE then your patch is right
but incomplete. It should also be removing the option:
          - const: hc
          - const: core
          - const: cqhci
          - const: ice

I am not intimately familiar with Qualcomm MMC controller history.
That being said, the old .txt file said:

        - CQE register map (Optional, CQE support is present on SDHC
instance meant
                            for eMMC and version v4.2 and above)

To me this implies that a v4 controller could _at least_ have "cqhci".
I dunno about "ice". I seem to recall that this was the argument for
why the driver had to use reg-names to begin with and why the driver
looks for "cqhci" by name.

-Doug
Krzysztof Kozlowski July 11, 2022, 7:45 a.m. UTC | #2
On 07/07/2022 16:31, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jul 7, 2022 at 1:04 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> The entries in arrays must have fixed order, so the bindings and Linux
>> driver expecting various combinations of 'reg' addresses was never
>> actually conforming to guidelines.
>>
>> Specifically Linux driver always expects 'core' reg entry as second
>> item, but some DTSes are having there 'cqhci'.
> 
> This is a bit misleading and makes it sound like we've got a bug. In
> truth the Linux driver looks at the compatible string. If it sees any
> compatible listed as "v5" (or a slight variant of v5 to handle a
> workaround for sc7180 / sdm845) then it _doesn't_ expect 'core' reg as
> the second entry. See the variable `mci_removed`. The old bindings
> ".txt" file also had this to say:
> 
>                 For SDCC version 5.0.0, MCI registers are removed from SDCC
>                 interface and some registers are moved to HC. New compatible
>                 string is added to support this change - "qcom,sdhci-msm-v5".

You're right, thanks, I missed that part.

> 
> So I guess that means this is the documentation for all of the
> combinations you have listed:
> 
> * hc only - v5 controller w/out CQE / ICE
> 
> * hc + core - v4 controller w/out CQE / ICE
> 
> * hc + cqhci - v5 controller w/ CQE and w/out ICE
> 
> * hc + cqhci + ice - v5 controller w/ CQE / ICE
> 
> * hc + core + cqhci + ice - v4 controller w/ CQE / ICE
> 
> Said another way, before v5 the "core" range existed. After v5 it
> apparently doesn't so there's no way we could have specified it.
> 
> You'll notice that one of the options above implies that a v4
> controller (with "core" specified) can have CQE and ICE. Is this
> actually true, or was it a misunderstanding in the .txt to .yaml
> conversion?
> 
> If it's true that a v4 controller can have CQE and ICE then your patch
> is wrong in asserting that v4 controllers have only "hc" and "core".
> 
> If a v4 controller _can't_ have CQE and ICE then your patch is right
> but incomplete. It should also be removing the option:
>           - const: hc
>           - const: core
>           - const: cqhci
>           - const: ice
> 
> I am not intimately familiar with Qualcomm MMC controller history.
> That being said, the old .txt file said:
> 
>         - CQE register map (Optional, CQE support is present on SDHC
> instance meant
>                             for eMMC and version v4.2 and above)
> 
> To me this implies that a v4 controller could _at least_ have "cqhci".
> I dunno about "ice". I seem to recall that this was the argument for
> why the driver had to use reg-names to begin with and why the driver
> looks for "cqhci" by name.


I checked manual and SDCC v4 already supports CQE. ICE appears at v4.1
(MSM8996, MSM8953 and some more which do not have dedicated
compatibles), so the compatibles seems inaccurate. ICE capability can be
runtime detected, but we still need to provide ICE address space. Anyway
I don't want to dig too much into SDCC versions, so I'll allow
hc+core+cqhci+ice.

Thanks for the feedback, much appreciated!


Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
index 10707c4f7184..ac48e36a6949 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
@@ -176,6 +176,22 @@  required:
 allOf:
   - $ref: mmc-controller.yaml#
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sdhci-msm-v4
+    then:
+      properties:
+        reg:
+          minItems: 2
+          maxItems: 2
+        reg-names:
+          items:
+            - const: hc
+            - const: core
+
 unevaluatedProperties: false
 
 examples: