| Message ID | 20230105213856.1828360-1-andreas@kemnade.info |
|---|---|
| State | New |
| Headers | show |
| Series | dt-bindings: mmc: fsl-imx-esdhc: allow more compatible combinations | expand |
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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ > > Reason: compatibility of new dtbs with old kernels or bootloaders. > > This will significantly reduce noise on make dtbs_check. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > .../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
On Fri, 6 Jan 2023 09:41:01 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ > > > > Reason: compatibility of new dtbs with old kernels or bootloaders. > > > > This will significantly reduce noise on make dtbs_check. > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > --- > > .../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
On 06/01/2023 20:33, Andreas Kemnade wrote: > On Fri, 6 Jan 2023 09:41:01 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ >>> >>> Reason: compatibility of new dtbs with old kernels or bootloaders. >>> >>> This will significantly reduce noise on make dtbs_check. >>> >>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info> >>> --- >>> .../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
On Sat, 7 Jan 2023 14:23:08 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 06/01/2023 20:33, Andreas Kemnade wrote: > > On Fri, 6 Jan 2023 09:41:01 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ > >>> > >>> Reason: compatibility of new dtbs with old kernels or bootloaders. > >>> > >>> This will significantly reduce noise on make dtbs_check. > >>> > >>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > >>> --- > >>> .../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
On 07/01/2023 14:43, Andreas Kemnade wrote: > On Sat, 7 Jan 2023 14:23:08 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 06/01/2023 20:33, Andreas Kemnade wrote: >>> On Fri, 6 Jan 2023 09:41:01 +0100 >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ >>>>> >>>>> Reason: compatibility of new dtbs with old kernels or bootloaders. >>>>> >>>>> This will significantly reduce noise on make dtbs_check. >>>>> >>>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info> >>>>> --- >>>>> .../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
On Sat, 7 Jan 2023 15:00:56 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ Regards, Andreas
On 07/01/2023 15:07, Andreas Kemnade wrote: > On Sat, 7 Jan 2023 15:00:56 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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
On Sat, 7 Jan 2023 15:09:24 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 07/01/2023 15:07, Andreas Kemnade wrote: > > On Sat, 7 Jan 2023 15:00:56 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ and https://lore.kernel.org/linux-devicetree/20210924091439.2561931-5-andreas@kemnade.info/ 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
On 07/01/2023 16:01, Andreas Kemnade wrote: > On Sat, 7 Jan 2023 15:09:24 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 07/01/2023 15:07, Andreas Kemnade wrote: >>> On Sat, 7 Jan 2023 15:00:56 +0100 >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ > and > https://lore.kernel.org/linux-devicetree/20210924091439.2561931-5-andreas@kemnade.info/ > > 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
On Sat, 7 Jan 2023 16:07:35 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 07/01/2023 16:01, Andreas Kemnade wrote: > > On Sat, 7 Jan 2023 15:09:24 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > >> On 07/01/2023 15:07, Andreas Kemnade wrote: > >>> On Sat, 7 Jan 2023 15:00:56 +0100 > >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ > > and > > https://lore.kernel.org/linux-devicetree/20210924091439.2561931-5-andreas@kemnade.info/ > > > > 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
On 07/01/2023 16:54, Andreas Kemnade wrote: > On Sat, 7 Jan 2023 16:07:35 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 07/01/2023 16:01, Andreas Kemnade wrote: >>> On Sat, 7 Jan 2023 15:09:24 +0100 >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>> >>>> On 07/01/2023 15:07, Andreas Kemnade wrote: >>>>> On Sat, 7 Jan 2023 15:00:56 +0100 >>>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ >>> and >>> https://lore.kernel.org/linux-devicetree/20210924091439.2561931-5-andreas@kemnade.info/ >>> >>> 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
On Sun, 8 Jan 2023 15:45:44 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 07/01/2023 16:54, Andreas Kemnade wrote: > > On Sat, 7 Jan 2023 16:07:35 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > >> On 07/01/2023 16:01, Andreas Kemnade wrote: > >>> On Sat, 7 Jan 2023 15:09:24 +0100 > >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >>> > >>>> On 07/01/2023 15:07, Andreas Kemnade wrote: > >>>>> On Sat, 7 Jan 2023 15:00:56 +0100 > >>>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ > >>> and > >>> https://lore.kernel.org/linux-devicetree/20210924091439.2561931-5-andreas@kemnade.info/ > >>> > >>> 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
On Sat, Jan 07, 2023 at 04:54:57PM +0100, Andreas Kemnade wrote: > On Sat, 7 Jan 2023 16:07:35 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > On 07/01/2023 16:01, Andreas Kemnade wrote: > > > On Sat, 7 Jan 2023 15:09:24 +0100 > > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > > > >> On 07/01/2023 15:07, Andreas Kemnade wrote: > > >>> On Sat, 7 Jan 2023 15:00:56 +0100 > > >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ > > > and > > > https://lore.kernel.org/linux-devicetree/20210924091439.2561931-5-andreas@kemnade.info/ > > > > > > 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
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
Hi Rob, On Sun, 8 Jan 2023 16:46:33 -0600 Rob Herring <robh@kernel.org> 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
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
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/72e1194e10ccb4f87aed96265114f0963e805092.camel@pengutronix.de/ Reason: compatibility of new dtbs with old kernels or bootloaders. This will significantly reduce noise on make dtbs_check. Signed-off-by: Andreas Kemnade <andreas@kemnade.info> --- .../bindings/mmc/fsl-imx-esdhc.yaml | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+)