dt-bindings: mmc: fsl-imx-esdhc: allow more compatible combinations

Commit Message

Andreas Kemnade Jan. 5, 2023, 9:38 p.m. UTC

Currently make dtbs_check shows lots of errors because imx*.dtsi does
not use single compatibles but combinations of them.
Allow all the combinations used there.

Patches fixing the dtsi files according to binding documentation were
submitted multiple times and are commonly rejected, so relax the rules.
Example:
https://lore.kernel.org/linux-devicetree/[e-mail]/

Reason: compatibility of new dtbs with old kernels or bootloaders.

This will significantly reduce noise on make dtbs_check.

Signed-off-by: Andreas Kemnade <[e-mail]>
---
 .../bindings/mmc/fsl-imx-esdhc.yaml           | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Krzysztof Kozlowski Jan. 6, 2023, 8:41 a.m. UTC | #1

On 05/01/2023 22:38, Andreas Kemnade wrote:
> Currently make dtbs_check shows lots of errors because imx*.dtsi does
> not use single compatibles but combinations of them.
> Allow all the combinations used there.
> 
> Patches fixing the dtsi files according to binding documentation were
> submitted multiple times and are commonly rejected, so relax the rules.
> Example:
> https://lore.kernel.org/linux-devicetree/[e-mail]/
> 
> Reason: compatibility of new dtbs with old kernels or bootloaders.
> 
> This will significantly reduce noise on make dtbs_check.
> 
> Signed-off-by: Andreas Kemnade <[e-mail]>
> ---
>  .../bindings/mmc/fsl-imx-esdhc.yaml           | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> index dc6256f04b42..118ebb75f136 100644
> --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> @@ -37,6 +37,30 @@ properties:
>            - fsl,imx8mm-usdhc
>            - fsl,imxrt1050-usdhc
>            - nxp,s32g2-usdhc

You must drop the items from enum above. Binding saying:
compatible="A"
or:
compatible="A", "B"

is not correct. Either A is or is not compatible with B.

> +      - items:
> +          - const: fsl,imx50-esdhc
> +          - const: fsl,imx53-esdhc
> +      - items:
> +          - const: fsl,imx6sl-usdhc
> +          - const: fsl,imx6q-usdhc
> +      - items:
> +          - const: fsl,imx6sll-usdhc
> +          - const: fsl,imx6sx-usdhc
> +      - items:
> +          - const: fsl,imx6sx-usdhc
> +          - const: fsl,imx6sl-usdhc
> +      - items:
> +          - const: fsl,imx6ul-usdhc
> +          - const: fsl,imx6sx-usdhc
> +      - items:
> +          - const: fsl,imx6ull-usdhc
> +          - const: fsl,imx6sx-usdhc
> +      - items:
> +          - const: fsl,imx7d-usdhc
> +          - const: fsl,imx6sl-usdhc
> +      - items:
> +          - const: fsl,imx7ulp-usdhc
> +          - const: fsl,imx6sx-usdhc

Half of these should be enum (6ul, 7ulp etc) with fallback.

>        - items:
>            - enum:
>                - fsl,imx8mq-usdhc

Best regards,
Krzysztof

Andreas Kemnade Jan. 6, 2023, 7:33 p.m. UTC | #2

On Fri, 6 Jan 2023 09:41:01 +0100
Krzysztof Kozlowski <[e-mail]> wrote:

> On 05/01/2023 22:38, Andreas Kemnade wrote:
> > Currently make dtbs_check shows lots of errors because imx*.dtsi does
> > not use single compatibles but combinations of them.
> > Allow all the combinations used there.
> > 
> > Patches fixing the dtsi files according to binding documentation were
> > submitted multiple times and are commonly rejected, so relax the rules.
> > Example:
> > https://lore.kernel.org/linux-devicetree/[e-mail]/
> > 
> > Reason: compatibility of new dtbs with old kernels or bootloaders.
> > 
> > This will significantly reduce noise on make dtbs_check.
> > 
> > Signed-off-by: Andreas Kemnade <[e-mail]>
> > ---
> >  .../bindings/mmc/fsl-imx-esdhc.yaml           | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> > index dc6256f04b42..118ebb75f136 100644
> > --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> > @@ -37,6 +37,30 @@ properties:
> >            - fsl,imx8mm-usdhc
> >            - fsl,imxrt1050-usdhc
> >            - nxp,s32g2-usdhc  
> 
> You must drop the items from enum above. Binding saying:
> compatible="A"
> or:
> compatible="A", "B"
> 
> is not correct. Either A is or is not compatible with B.
> 
hmm, here we have A = B + some additional features
or
A = B + some additional features and additional quirks required.

For the latter we have e.g.
A=
static const struct esdhc_soc_data usdhc_imx6sx_data = {
        .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
                        | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
                        | ESDHC_FLAG_STATE_LOST_IN_LPMODE
                        | ESDHC_FLAG_BROKEN_AUTO_CMD23,
};
B=
static const struct esdhc_soc_data usdhc_imx6sl_data = {
        .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
                        | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_ERR004536
                        | ESDHC_FLAG_HS200
                        | ESDHC_FLAG_BROKEN_AUTO_CMD23,
};

so there is the difference in ESDHC_FLAG_STATE_LOST_IN_LPMODE.
That might make no difference in some usage scenario (e.g. some bootloader
not doing any LPMODE), but I wonder why
we need to *enforce* specifying such half-compatible things.

Regards,
Andreas

Krzysztof Kozlowski Jan. 7, 2023, 1:23 p.m. UTC | #3

On 06/01/2023 20:33, Andreas Kemnade wrote:
> On Fri, 6 Jan 2023 09:41:01 +0100
> Krzysztof Kozlowski <[e-mail]> wrote:
> 
>> On 05/01/2023 22:38, Andreas Kemnade wrote:
>>> Currently make dtbs_check shows lots of errors because imx*.dtsi does
>>> not use single compatibles but combinations of them.
>>> Allow all the combinations used there.
>>>
>>> Patches fixing the dtsi files according to binding documentation were
>>> submitted multiple times and are commonly rejected, so relax the rules.
>>> Example:
>>> https://lore.kernel.org/linux-devicetree/[e-mail]/
>>>
>>> Reason: compatibility of new dtbs with old kernels or bootloaders.
>>>
>>> This will significantly reduce noise on make dtbs_check.
>>>
>>> Signed-off-by: Andreas Kemnade <[e-mail]>
>>> ---
>>>  .../bindings/mmc/fsl-imx-esdhc.yaml           | 24 +++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
>>> index dc6256f04b42..118ebb75f136 100644
>>> --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
>>> +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
>>> @@ -37,6 +37,30 @@ properties:
>>>            - fsl,imx8mm-usdhc
>>>            - fsl,imxrt1050-usdhc
>>>            - nxp,s32g2-usdhc  
>>
>> You must drop the items from enum above. Binding saying:
>> compatible="A"
>> or:
>> compatible="A", "B"
>>
>> is not correct. Either A is or is not compatible with B.
>>
> hmm, here we have A = B + some additional features
> or
> A = B + some additional features and additional quirks required.

So why do you allow A alone?

> 
> For the latter we have e.g.
> A=
> static const struct esdhc_soc_data usdhc_imx6sx_data = {
>         .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>                         | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>                         | ESDHC_FLAG_STATE_LOST_IN_LPMODE
>                         | ESDHC_FLAG_BROKEN_AUTO_CMD23,
> };
> B=
> static const struct esdhc_soc_data usdhc_imx6sl_data = {
>         .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>                         | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_ERR004536
>                         | ESDHC_FLAG_HS200
>                         | ESDHC_FLAG_BROKEN_AUTO_CMD23,
> };
> 
> so there is the difference in ESDHC_FLAG_STATE_LOST_IN_LPMODE.
> That might make no difference in some usage scenario (e.g. some bootloader
> not doing any LPMODE), but I wonder why
> we need to *enforce* specifying such half-compatible things.

I asked to remove half-compatible. Not to enforce.

Best regards,
Krzysztof

Andreas Kemnade Jan. 7, 2023, 1:43 p.m. UTC | #4

On Sat, 7 Jan 2023 14:23:08 +0100
Krzysztof Kozlowski <[e-mail]> wrote:

> On 06/01/2023 20:33, Andreas Kemnade wrote:
> > On Fri, 6 Jan 2023 09:41:01 +0100
> > Krzysztof Kozlowski <[e-mail]> wrote:
> >   
> >> On 05/01/2023 22:38, Andreas Kemnade wrote:  
> >>> Currently make dtbs_check shows lots of errors because imx*.dtsi does
> >>> not use single compatibles but combinations of them.
> >>> Allow all the combinations used there.
> >>>
> >>> Patches fixing the dtsi files according to binding documentation were
> >>> submitted multiple times and are commonly rejected, so relax the rules.
> >>> Example:
> >>> https://lore.kernel.org/linux-devicetree/[e-mail]/
> >>>
> >>> Reason: compatibility of new dtbs with old kernels or bootloaders.
> >>>
> >>> This will significantly reduce noise on make dtbs_check.
> >>>
> >>> Signed-off-by: Andreas Kemnade <[e-mail]>
> >>> ---
> >>>  .../bindings/mmc/fsl-imx-esdhc.yaml           | 24 +++++++++++++++++++
> >>>  1 file changed, 24 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> >>> index dc6256f04b42..118ebb75f136 100644
> >>> --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> >>> +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> >>> @@ -37,6 +37,30 @@ properties:
> >>>            - fsl,imx8mm-usdhc
> >>>            - fsl,imxrt1050-usdhc
> >>>            - nxp,s32g2-usdhc    
> >>
> >> You must drop the items from enum above. Binding saying:
> >> compatible="A"
> >> or:
> >> compatible="A", "B"
> >>
> >> is not correct. Either A is or is not compatible with B.
> >>  
> > hmm, here we have A = B + some additional features
> > or
> > A = B + some additional features and additional quirks required.  
> 
> So why do you allow A alone?
> 
because A is full-compatible, and B is half-compatible, because
the additional required quirks are not applied.
> > 
> > For the latter we have e.g.
> > A=
> > static const struct esdhc_soc_data usdhc_imx6sx_data = {
> >         .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> >                         | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
> >                         | ESDHC_FLAG_STATE_LOST_IN_LPMODE
> >                         | ESDHC_FLAG_BROKEN_AUTO_CMD23,
> > };
> > B=
> > static const struct esdhc_soc_data usdhc_imx6sl_data = {
> >         .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
> >                         | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_ERR004536
> >                         | ESDHC_FLAG_HS200
> >                         | ESDHC_FLAG_BROKEN_AUTO_CMD23,
> > };
> > 
> > so there is the difference in ESDHC_FLAG_STATE_LOST_IN_LPMODE.
> > That might make no difference in some usage scenario (e.g. some bootloader
> > not doing any LPMODE), but I wonder why
> > we need to *enforce* specifying such half-compatible things.  
> 
> I asked to remove half-compatible. Not to enforce.
> 
well B is half-compatible, I (and others) have sent patches to remove,
but they were rejected. I consider these patches the way to go.

Regards,
Andreas

Krzysztof Kozlowski Jan. 7, 2023, 2 p.m. UTC | #5

On 07/01/2023 14:43, Andreas Kemnade wrote:
> On Sat, 7 Jan 2023 14:23:08 +0100
> Krzysztof Kozlowski <[e-mail]> wrote:
> 
>> On 06/01/2023 20:33, Andreas Kemnade wrote:
>>> On Fri, 6 Jan 2023 09:41:01 +0100
>>> Krzysztof Kozlowski <[e-mail]> wrote:
>>>   
>>>> On 05/01/2023 22:38, Andreas Kemnade wrote:  
>>>>> Currently make dtbs_check shows lots of errors because imx*.dtsi does
>>>>> not use single compatibles but combinations of them.
>>>>> Allow all the combinations used there.
>>>>>
>>>>> Patches fixing the dtsi files according to binding documentation were
>>>>> submitted multiple times and are commonly rejected, so relax the rules.
>>>>> Example:
>>>>> https://lore.kernel.org/linux-devicetree/[e-mail]/
>>>>>
>>>>> Reason: compatibility of new dtbs with old kernels or bootloaders.
>>>>>
>>>>> This will significantly reduce noise on make dtbs_check.
>>>>>
>>>>> Signed-off-by: Andreas Kemnade <[e-mail]>
>>>>> ---
>>>>>  .../bindings/mmc/fsl-imx-esdhc.yaml           | 24 +++++++++++++++++++
>>>>>  1 file changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
>>>>> index dc6256f04b42..118ebb75f136 100644
>>>>> --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
>>>>> +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
>>>>> @@ -37,6 +37,30 @@ properties:
>>>>>            - fsl,imx8mm-usdhc
>>>>>            - fsl,imxrt1050-usdhc
>>>>>            - nxp,s32g2-usdhc    
>>>>
>>>> You must drop the items from enum above. Binding saying:
>>>> compatible="A"
>>>> or:
>>>> compatible="A", "B"
>>>>
>>>> is not correct. Either A is or is not compatible with B.
>>>>  
>>> hmm, here we have A = B + some additional features
>>> or
>>> A = B + some additional features and additional quirks required.  
>>
>> So why do you allow A alone?
>>
> because A is full-compatible, and B is half-compatible, because
> the additional required quirks are not applied.

As I explained you in private message you sent me:

That's not how compatibles are working. If device is not compatible with
B, then you cannot have it as fallback, so the patch is not correct.

If device is A and is compatible with B, then keeping A and A+B is also
incorrect because it is redundant.

This is not only here, it's everywhere, so I do not see the point to
make exception for this device. Patch is incorrect.

Best regards,
Krzysztof


>>>
>>> For the latter we have e.g.
>>> A=
>>> static const struct esdhc_soc_data usdhc_imx6sx_data = {
>>>         .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>>>                         | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>>>                         | ESDHC_FLAG_STATE_LOST_IN_LPMODE
>>>                         | ESDHC_FLAG_BROKEN_AUTO_CMD23,
>>> };
>>> B=
>>> static const struct esdhc_soc_data usdhc_imx6sl_data = {
>>>         .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>>>                         | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_ERR004536
>>>                         | ESDHC_FLAG_HS200
>>>                         | ESDHC_FLAG_BROKEN_AUTO_CMD23,
>>> };
>>>
>>> so there is the difference in ESDHC_FLAG_STATE_LOST_IN_LPMODE.
>>> That might make no difference in some usage scenario (e.g. some bootloader
>>> not doing any LPMODE), but I wonder why
>>> we need to *enforce* specifying such half-compatible things.  
>>
>> I asked to remove half-compatible. Not to enforce.
>>
> well B is half-compatible, I (and others) have sent patches to remove,
> but they were rejected. I consider these patches the way to go.

No, they are not correct.

Best regards,
Krzysztof

Andreas Kemnade Jan. 7, 2023, 2:07 p.m. UTC | #6

On Sat, 7 Jan 2023 15:00:56 +0100
Krzysztof Kozlowski <[e-mail]> wrote:

[...]
> >> I asked to remove half-compatible. Not to enforce.
> >>  
so you are saying that allowing
compatible = "A", "B" 
is not ok, if B is not fully compatible. I agree with that
one.

> > well B is half-compatible, I (and others) have sent patches to remove,
> > but they were rejected. I consider these patches the way to go.  
> 
> No, they are not correct.
> 
Maybe there is some misunderstanding
"these patches" = e.g. 
https://lore.kernel.org/linux-devicetree/[e-mail]/

Regards,
Andreas

Krzysztof Kozlowski Jan. 7, 2023, 2:09 p.m. UTC | #7

On 07/01/2023 15:07, Andreas Kemnade wrote:
> On Sat, 7 Jan 2023 15:00:56 +0100
> Krzysztof Kozlowski <[e-mail]> wrote:
> 
> [...]
>>>> I asked to remove half-compatible. Not to enforce.
>>>>  
> so you are saying that allowing
> compatible = "A", "B" 
> is not ok, if B is not fully compatible. I agree with that
> one.

I did not say that. It's not related to this problem.

Again - you cannot have device which is and is not compatible with
something else. It's not a Schroedinger's cat to be in two states,
unless you explicitly document the cases (there are exception). If this
is such exception, it requires it's own documentation.

Best regards,
Krzysztof

Andreas Kemnade Jan. 7, 2023, 3:01 p.m. UTC | #8

On Sat, 7 Jan 2023 15:09:24 +0100
Krzysztof Kozlowski <[e-mail]> wrote:

> On 07/01/2023 15:07, Andreas Kemnade wrote:
> > On Sat, 7 Jan 2023 15:00:56 +0100
> > Krzysztof Kozlowski <[e-mail]> wrote:
> > 
> > [...]  
> >>>> I asked to remove half-compatible. Not to enforce.
> >>>>    
> > so you are saying that allowing
> > compatible = "A", "B" 
> > is not ok, if B is not fully compatible. I agree with that
> > one.  
> 
> I did not say that. It's not related to this problem.
> 
You said "I asked to remove half-compatible" that means to me
remove "B" if not fully compatible with A which sounds sane to me.

> Again - you cannot have device which is and is not compatible with
> something else. It's not a Schroedinger's cat to be in two states,
> unless you explicitly document the cases (there are exception). If this
> is such exception, it requires it's own documentation.
> 
so conclusion:
If having A and B half-compatible with A:

compatible = "A" only: is allowed to specifiy it the binding (status quo),
  but not allowed to make the actual dtsi match the binding documentation
  https://lore.kernel.org/linux-devicetree/[e-mail]/
  and
  https://lore.kernel.org/linux-devicetree/[e-mail]/

compatible = "A", "B" in the binding definition: is not allowed ("I asked to remove
   half-compatible" (= removing B))

having mismatch between binding definition and devicetree causes dtbs_check errors
   -> also not nice.

I rather drop this patch and learn to live with dtbs_check errors
for this one since I have no idea how to proceed. All roads are blocked.
This all causes too much churn.

Regards,
Andreas

Krzysztof Kozlowski Jan. 7, 2023, 3:07 p.m. UTC | #9

On 07/01/2023 16:01, Andreas Kemnade wrote:
> On Sat, 7 Jan 2023 15:09:24 +0100
> Krzysztof Kozlowski <[e-mail]> wrote:
> 
>> On 07/01/2023 15:07, Andreas Kemnade wrote:
>>> On Sat, 7 Jan 2023 15:00:56 +0100
>>> Krzysztof Kozlowski <[e-mail]> wrote:
>>>
>>> [...]  
>>>>>> I asked to remove half-compatible. Not to enforce.
>>>>>>    
>>> so you are saying that allowing
>>> compatible = "A", "B" 
>>> is not ok, if B is not fully compatible. I agree with that
>>> one.  
>>
>> I did not say that. It's not related to this problem.
>>
> You said "I asked to remove half-compatible" that means to me
> remove "B" if not fully compatible with A which sounds sane to me.
> 
>> Again - you cannot have device which is and is not compatible with
>> something else. It's not a Schroedinger's cat to be in two states,
>> unless you explicitly document the cases (there are exception). If this
>> is such exception, it requires it's own documentation.
>>
> so conclusion:
> If having A and B half-compatible with A:
> 
> compatible = "A" only: is allowed to specifiy it the binding (status quo),
>   but not allowed to make the actual dtsi match the binding documentation
>   https://lore.kernel.org/linux-devicetree/[e-mail]/
>   and
>   https://lore.kernel.org/linux-devicetree/[e-mail]/
> 
> compatible = "A", "B" in the binding definition: is not allowed ("I asked to remove
>    half-compatible" (= removing B))

No, half compatible is the A in such case.

> 
> having mismatch between binding definition and devicetree causes dtbs_check errors
>    -> also not nice.
> 
> I rather drop this patch and learn to live with dtbs_check errors
> for this one since I have no idea how to proceed. All roads are blocked.
> This all causes too much churn.

And why you cannot implement what I asked for?

Best regards,
Krzysztof

Andreas Kemnade Jan. 7, 2023, 3:54 p.m. UTC | #10

On Sat, 7 Jan 2023 16:07:35 +0100
Krzysztof Kozlowski <[e-mail]> wrote:

> On 07/01/2023 16:01, Andreas Kemnade wrote:
> > On Sat, 7 Jan 2023 15:09:24 +0100
> > Krzysztof Kozlowski <[e-mail]> wrote:
> >   
> >> On 07/01/2023 15:07, Andreas Kemnade wrote:  
> >>> On Sat, 7 Jan 2023 15:00:56 +0100
> >>> Krzysztof Kozlowski <[e-mail]> wrote:
> >>>
> >>> [...]    
> >>>>>> I asked to remove half-compatible. Not to enforce.
> >>>>>>      
> >>> so you are saying that allowing
> >>> compatible = "A", "B" 
> >>> is not ok, if B is not fully compatible. I agree with that
> >>> one.    
> >>
> >> I did not say that. It's not related to this problem.
> >>  
> > You said "I asked to remove half-compatible" that means to me
> > remove "B" if not fully compatible with A which sounds sane to me.
> >   
> >> Again - you cannot have device which is and is not compatible with
> >> something else. It's not a Schroedinger's cat to be in two states,
> >> unless you explicitly document the cases (there are exception). If this
> >> is such exception, it requires it's own documentation.
> >>  
> > so conclusion:
> > If having A and B half-compatible with A:
> > 
> > compatible = "A" only: is allowed to specifiy it the binding (status quo),
> >   but not allowed to make the actual dtsi match the binding documentation
> >   https://lore.kernel.org/linux-devicetree/[e-mail]/
> >   and
> >   https://lore.kernel.org/linux-devicetree/[e-mail]/
> > 
> > compatible = "A", "B" in the binding definition: is not allowed ("I asked to remove
> >    half-compatible" (= removing B))  
> 
> No, half compatible is the A in such case.
> 
I think that there is some misunderstanding in here. I try once again.

Define compatible with "X" here:
To me it means:

device fully works with flags defined in:

static const struct esdhc_soc_data usdhc_X_data = { ... };

with usdhc_X_data referenced in
        { .compatible = "X", .data = &usdhc_X_data, },


So if there is only "A" matching with above definition of compatibility
  compatible = "A" would sound sane to me.

And scrutinizing the flags more and not just wanting to achieve error-free
dtbs_check, I think is this in most cases where there is only "A". 

If there is "A" and "B" which match that compatibility definition, you
say that only compatible = "A", "B" is allowed, but not compatible = "A".
In that case I would have no problem with that.

But if there is only "A" but no "B" matching the above definition, I would expect
that only compatible = "A" is allowed but *not* compatible = "A", "B".

Regards,
Andreas

Krzysztof Kozlowski Jan. 8, 2023, 2:45 p.m. UTC | #11

On 07/01/2023 16:54, Andreas Kemnade wrote:
> On Sat, 7 Jan 2023 16:07:35 +0100
> Krzysztof Kozlowski <[e-mail]> wrote:
> 
>> On 07/01/2023 16:01, Andreas Kemnade wrote:
>>> On Sat, 7 Jan 2023 15:09:24 +0100
>>> Krzysztof Kozlowski <[e-mail]> wrote:
>>>   
>>>> On 07/01/2023 15:07, Andreas Kemnade wrote:  
>>>>> On Sat, 7 Jan 2023 15:00:56 +0100
>>>>> Krzysztof Kozlowski <[e-mail]> wrote:
>>>>>
>>>>> [...]    
>>>>>>>> I asked to remove half-compatible. Not to enforce.
>>>>>>>>      
>>>>> so you are saying that allowing
>>>>> compatible = "A", "B" 
>>>>> is not ok, if B is not fully compatible. I agree with that
>>>>> one.    
>>>>
>>>> I did not say that. It's not related to this problem.
>>>>  
>>> You said "I asked to remove half-compatible" that means to me
>>> remove "B" if not fully compatible with A which sounds sane to me.
>>>   
>>>> Again - you cannot have device which is and is not compatible with
>>>> something else. It's not a Schroedinger's cat to be in two states,
>>>> unless you explicitly document the cases (there are exception). If this
>>>> is such exception, it requires it's own documentation.
>>>>  
>>> so conclusion:
>>> If having A and B half-compatible with A:
>>>
>>> compatible = "A" only: is allowed to specifiy it the binding (status quo),
>>>   but not allowed to make the actual dtsi match the binding documentation
>>>   https://lore.kernel.org/linux-devicetree/[e-mail]/
>>>   and
>>>   https://lore.kernel.org/linux-devicetree/[e-mail]/
>>>
>>> compatible = "A", "B" in the binding definition: is not allowed ("I asked to remove
>>>    half-compatible" (= removing B))  
>>
>> No, half compatible is the A in such case.
>>
> I think that there is some misunderstanding in here. I try once again.
> 
> Define compatible with "X" here:
> To me it means:
> 
> device fully works with flags defined in:
> 
> static const struct esdhc_soc_data usdhc_X_data = { ... };
> 
> with usdhc_X_data referenced in
>         { .compatible = "X", .data = &usdhc_X_data, },
> 
> 
> So if there is only "A" matching with above definition of compatibility
>   compatible = "A" would sound sane to me.
> 
> And scrutinizing the flags more and not just wanting to achieve error-free
> dtbs_check, I think is this in most cases where there is only "A". 
> 
> If there is "A" and "B" which match that compatibility definition, you
> say that only compatible = "A", "B" is allowed, but not compatible = "A".
> In that case I would have no problem with that.
> 
> But if there is only "A" but no "B" matching the above definition, I would expect
> that only compatible = "A" is allowed but *not* compatible = "A", "B".

Sorry, I don't follow. I also do not understand what "matching" means in
these terms (binding driver? of_match?) and also I do not know what is
the "above definition".

Devicetree spec defines the compatibility - so this is the definition.
There will be differences when applying it to different cases.

Best regards,
Krzysztof

Andreas Kemnade Jan. 8, 2023, 5:20 p.m. UTC | #12

On Sun, 8 Jan 2023 15:45:44 +0100
Krzysztof Kozlowski <[e-mail]> wrote:

> On 07/01/2023 16:54, Andreas Kemnade wrote:
> > On Sat, 7 Jan 2023 16:07:35 +0100
> > Krzysztof Kozlowski <[e-mail]> wrote:
> >   
> >> On 07/01/2023 16:01, Andreas Kemnade wrote:  
> >>> On Sat, 7 Jan 2023 15:09:24 +0100
> >>> Krzysztof Kozlowski <[e-mail]> wrote:
> >>>     
> >>>> On 07/01/2023 15:07, Andreas Kemnade wrote:    
> >>>>> On Sat, 7 Jan 2023 15:00:56 +0100
> >>>>> Krzysztof Kozlowski <[e-mail]> wrote:
> >>>>>
> >>>>> [...]      
> >>>>>>>> I asked to remove half-compatible. Not to enforce.
> >>>>>>>>        
> >>>>> so you are saying that allowing
> >>>>> compatible = "A", "B" 
> >>>>> is not ok, if B is not fully compatible. I agree with that
> >>>>> one.      
> >>>>
> >>>> I did not say that. It's not related to this problem.
> >>>>    
> >>> You said "I asked to remove half-compatible" that means to me
> >>> remove "B" if not fully compatible with A which sounds sane to me.
> >>>     
> >>>> Again - you cannot have device which is and is not compatible with
> >>>> something else. It's not a Schroedinger's cat to be in two states,
> >>>> unless you explicitly document the cases (there are exception). If this
> >>>> is such exception, it requires it's own documentation.
> >>>>    
> >>> so conclusion:
> >>> If having A and B half-compatible with A:
> >>>
> >>> compatible = "A" only: is allowed to specifiy it the binding (status quo),
> >>>   but not allowed to make the actual dtsi match the binding documentation
> >>>   https://lore.kernel.org/linux-devicetree/[e-mail]/
> >>>   and
> >>>   https://lore.kernel.org/linux-devicetree/[e-mail]/
> >>>
> >>> compatible = "A", "B" in the binding definition: is not allowed ("I asked to remove
> >>>    half-compatible" (= removing B))    
> >>
> >> No, half compatible is the A in such case.
> >>  
> > I think that there is some misunderstanding in here. I try once again.
> > 
> > Define compatible with "X" here:
> > To me it means:
> > 
> > device fully works with flags defined in:
> > 
> > static const struct esdhc_soc_data usdhc_X_data = { ... };
> > 
> > with usdhc_X_data referenced in
> >         { .compatible = "X", .data = &usdhc_X_data, },
> > 
> > 
> > So if there is only "A" matching with above definition of compatibility
> >   compatible = "A" would sound sane to me.
> > 
> > And scrutinizing the flags more and not just wanting to achieve error-free
> > dtbs_check, I think is this in most cases where there is only "A". 
> > 
> > If there is "A" and "B" which match that compatibility definition, you
> > say that only compatible = "A", "B" is allowed, but not compatible = "A".
> > In that case I would have no problem with that.
> > 
> > But if there is only "A" but no "B" matching the above definition, I would expect
> > that only compatible = "A" is allowed but *not* compatible = "A", "B".  
> 
> Sorry, I don't follow. I also do not understand what "matching" means in
> these terms (binding driver? of_match?) and also I do not know what is
> the "above definition".
> 
> Devicetree spec defines the compatibility - so this is the definition.
> There will be differences when applying it to different cases.
> 
Ok, lets stop talking about A and B, lets be more specific.
Hmm, I try to insert the missing bits here:

I am not convinced anymore that my patch is correct
- for dtb compatible formality
- for pure technical reasons

I am not convinced that your proposal is correct either.
- for pure technical reasons (for same resan as you state)

Especially this part I consider faulty:
+      - items:
+          - const: fsl,imx6sx-usdhc
+          - const: fsl,imx6sl-usdhc

Keyword: ESDHC_FLAG_STATE_LOST_IN_LPMODE (detailed that in
an earlier mail).

Regards
Andreas

Rob Herring Jan. 8, 2023, 10:46 p.m. UTC | #13

On Sat, Jan 07, 2023 at 04:54:57PM +0100, Andreas Kemnade wrote:
> On Sat, 7 Jan 2023 16:07:35 +0100
> Krzysztof Kozlowski <[e-mail]> wrote:
> 
> > On 07/01/2023 16:01, Andreas Kemnade wrote:
> > > On Sat, 7 Jan 2023 15:09:24 +0100
> > > Krzysztof Kozlowski <[e-mail]> wrote:
> > >   
> > >> On 07/01/2023 15:07, Andreas Kemnade wrote:  
> > >>> On Sat, 7 Jan 2023 15:00:56 +0100
> > >>> Krzysztof Kozlowski <[e-mail]> wrote:
> > >>>
> > >>> [...]    
> > >>>>>> I asked to remove half-compatible. Not to enforce.
> > >>>>>>      
> > >>> so you are saying that allowing
> > >>> compatible = "A", "B" 
> > >>> is not ok, if B is not fully compatible. I agree with that
> > >>> one.    
> > >>
> > >> I did not say that. It's not related to this problem.
> > >>  
> > > You said "I asked to remove half-compatible" that means to me
> > > remove "B" if not fully compatible with A which sounds sane to me.
> > >   
> > >> Again - you cannot have device which is and is not compatible with
> > >> something else. It's not a Schroedinger's cat to be in two states,
> > >> unless you explicitly document the cases (there are exception). If this
> > >> is such exception, it requires it's own documentation.
> > >>  
> > > so conclusion:
> > > If having A and B half-compatible with A:
> > > 
> > > compatible = "A" only: is allowed to specifiy it the binding (status quo),
> > >   but not allowed to make the actual dtsi match the binding documentation
> > >   https://lore.kernel.org/linux-devicetree/[e-mail]/
> > >   and
> > >   https://lore.kernel.org/linux-devicetree/[e-mail]/
> > > 
> > > compatible = "A", "B" in the binding definition: is not allowed ("I asked to remove
> > >    half-compatible" (= removing B))  
> > 
> > No, half compatible is the A in such case.
> > 
> I think that there is some misunderstanding in here. I try once again.
> 
> Define compatible with "X" here:
> To me it means:
> 
> device fully works with flags defined in:
> 
> static const struct esdhc_soc_data usdhc_X_data = { ... };
> 
> with usdhc_X_data referenced in
>         { .compatible = "X", .data = &usdhc_X_data, },
> 
> 
> So if there is only "A" matching with above definition of compatibility
>   compatible = "A" would sound sane to me.
> 
> And scrutinizing the flags more and not just wanting to achieve error-free
> dtbs_check, I think is this in most cases where there is only "A". 
> 
> If there is "A" and "B" which match that compatibility definition, you
> say that only compatible = "A", "B" is allowed, but not compatible = "A".
> In that case I would have no problem with that.
> 
> But if there is only "A" but no "B" matching the above definition, I would expect
> that only compatible = "A" is allowed but *not* compatible = "A", "B".

A is either compatible with B or it isn't. You can look at that from 
the h/w perspective and client/OS perspective. From the h/w side, is the 
h/w interface the same or only has additions which can be ignored? On 
the client side, the question is whether a client that only understands 
B could use A's h/w without change. Looking at the match data is a 
good indicator of that for Linux. It's also possible the answer is 
different for different clients, but we only need 1 client that could 
benefit from compatibility.

Rob

Krzysztof Kozlowski Jan. 9, 2023, 9:14 a.m. UTC | #14

On 08/01/2023 18:20, Andreas Kemnade wrote:
>>>>
>>>> No, half compatible is the A in such case.
>>>>  
>>> I think that there is some misunderstanding in here. I try once again.
>>>
>>> Define compatible with "X" here:
>>> To me it means:
>>>
>>> device fully works with flags defined in:
>>>
>>> static const struct esdhc_soc_data usdhc_X_data = { ... };
>>>
>>> with usdhc_X_data referenced in
>>>         { .compatible = "X", .data = &usdhc_X_data, },
>>>
>>>
>>> So if there is only "A" matching with above definition of compatibility
>>>   compatible = "A" would sound sane to me.
>>>
>>> And scrutinizing the flags more and not just wanting to achieve error-free
>>> dtbs_check, I think is this in most cases where there is only "A". 
>>>
>>> If there is "A" and "B" which match that compatibility definition, you
>>> say that only compatible = "A", "B" is allowed, but not compatible = "A".
>>> In that case I would have no problem with that.
>>>
>>> But if there is only "A" but no "B" matching the above definition, I would expect
>>> that only compatible = "A" is allowed but *not* compatible = "A", "B".  
>>
>> Sorry, I don't follow. I also do not understand what "matching" means in
>> these terms (binding driver? of_match?) and also I do not know what is
>> the "above definition".
>>
>> Devicetree spec defines the compatibility - so this is the definition.
>> There will be differences when applying it to different cases.
>>
> Ok, lets stop talking about A and B, lets be more specific.
> Hmm, I try to insert the missing bits here:
> 
> I am not convinced anymore that my patch is correct
> - for dtb compatible formality
> - for pure technical reasons
> 
> I am not convinced that your proposal is correct either.
> - for pure technical reasons (for same resan as you state)
> 
> Especially this part I consider faulty:
> +      - items:
> +          - const: fsl,imx6sx-usdhc
> +          - const: fsl,imx6sl-usdhc
> 
> Keyword: ESDHC_FLAG_STATE_LOST_IN_LPMODE (detailed that in
> an earlier mail).

I am not discussing here real compatibility for your hardware, as I
don't know whether 6SX is or is not compatible with 6SL. I am saying
that it either is or is not. Cannot be both at the same time.

Now for the question about 6SX+6SL. Rob responded here detailing when
compatibility of SX and SL is valid. If your hardware fits this case,
then remove the alone SX from enum and add SX+SL list like in this patch.

If your hardware does not fit, so there is no single 6SX client which
can use 6SL compatible and work somehow (e.g. half-speed, reduced
capabilities but still work correctly), then please bring back the DTS
patches. I am not sure if other people who commented on DTS, are
following our discussion here...

Best regards,
Krzysztof

Andreas Kemnade Jan. 9, 2023, 11:15 p.m. UTC | #15

Hi Rob,

On Sun, 8 Jan 2023 16:46:33 -0600
Rob Herring <[e-mail]> wrote:

> A is either compatible with B or it isn't.

I think English and many natural languages allow to use the adjective
in a non-black and white way to express things. So there can be things
more compatible than others with certain levels of "gray" in between.
But "compatible" in an artifical language like the devicetree can of course
be more restrictive, so there needs to be a certian level of gray where the
limit needs to be defined.The definition you give below.
Thanks for that.

> You can look at that from 
> the h/w perspective and client/OS perspective. From the h/w side, is the 
> h/w interface the same or only has additions which can be ignored? On 
> the client side, the question is whether a client that only understands 
> B could use A's h/w without change. Looking at the match data is a 
> good indicator of that for Linux.

It seems that from a Linux client perspective that is a no in different
cases, so B is not compatible.

> It's also possible the answer is 
> different for different clients, but we only need 1 client that could 
> benefit from compatibility.
> 
On U-Boot side things seem to look different, since high speed modes
are not enabled at all and pm is not done that much, so no quirks needed
for that.
Looking at recent mainline u-boot.
       { .compatible = "fsl,imx51-esdhc", },
        { .compatible = "fsl,imx53-esdhc", },
        { .compatible = "fsl,imx6ul-usdhc", },
        { .compatible = "fsl,imx6sx-usdhc", },
        { .compatible = "fsl,imx6sl-usdhc", },
        { .compatible = "fsl,imx6q-usdhc", },

So U-Boot will benefit from that additional compatible for fsl,imx6[su]ll-usdhc.

The first list of compatibles in U-Boot commit (96f0407b00f) was:
       { .compatible = "fsl,imx6ul-usdhc", },
       { .compatible = "fsl,imx6sx-usdhc", },
       { .compatible = "fsl,imx6sl-usdhc", },
       { .compatible = "fsl,imx6q-usdhc", },
       { .compatible = "fsl,imx7d-usdhc", },

So replacing imx5X fallback compatibles with imx6something might be
helpful for that old U-Boot. But I cannot fully jugde that,
so I will not touch. 

Well, I could also delete entries of this list and push a bunch of U-Boot forks
somewhere, so creating a large number of different clients, which would then
justify a long list of compatibles ;-)

But what initially worried me would be that there could be a client out there
knowing only "B" and using all features but missing the in that case needed
quirks for "A". Then adding "B" would cause harm. But apparently that is nothing
to worry about.

I will send a reduced patch with just the things which are 100% clear to me.

Regards,
Andreas

Patch

diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
index dc6256f04b42..118ebb75f136 100644
--- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
+++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
@@ -37,6 +37,30 @@  properties:
           - fsl,imx8mm-usdhc
           - fsl,imxrt1050-usdhc
           - nxp,s32g2-usdhc
+      - items:
+          - const: fsl,imx50-esdhc
+          - const: fsl,imx53-esdhc
+      - items:
+          - const: fsl,imx6sl-usdhc
+          - const: fsl,imx6q-usdhc
+      - items:
+          - const: fsl,imx6sll-usdhc
+          - const: fsl,imx6sx-usdhc
+      - items:
+          - const: fsl,imx6sx-usdhc
+          - const: fsl,imx6sl-usdhc
+      - items:
+          - const: fsl,imx6ul-usdhc
+          - const: fsl,imx6sx-usdhc
+      - items:
+          - const: fsl,imx6ull-usdhc
+          - const: fsl,imx6sx-usdhc
+      - items:
+          - const: fsl,imx7d-usdhc
+          - const: fsl,imx6sl-usdhc
+      - items:
+          - const: fsl,imx7ulp-usdhc
+          - const: fsl,imx6sx-usdhc
       - items:
           - enum:
               - fsl,imx8mq-usdhc