Re: [PATCH 3/5] arm64: dts: imx8mp: Enable HS400-ES

[Adam Ford <aford173@gmail.com>]

On Mon, Mar 28, 2022 at 6:49 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 28/03/2022 13:09, Ahmad Fatoum wrote:
> > Hello Adam,
> >
> > On 28.03.22 12:47, Adam Ford wrote:
> >> On Mon, Mar 28, 2022 at 2:20 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >>>
> >>> Hello Adam,
> >>>
> >>> On 27.03.22 14:38, Adam Ford wrote:
> >>>> The SDHC controller in the imx8mp has the same controller
> >>>> as the imx8mm which supports HS400-ES. Change the compatible
> >>>> fallback to imx8mm to enable it.
> >>>
> >>> I believe that's a shortcoming of the Linux driver, which should explicitly list
> >>> fsl,imx8mp-usdhc in its compatibles and enable HS400-ES for it.
> >>>
> >>> I find dropping compatibles problematic, because like Linux matching
> >>> fsl,imx8mm-usdhc, but not fsl,imx8mp-usdhc, other software may match
> >>> fsl,imx7d-usdhc, but not fsl,imx8[mp]-usdhc.
> >>>
> >>> I'd prefer that either the kernel driver gains extra compatibles or that
> >>> the DTS lists extra compatibles and we refrain from dropping existing
> >>> (correct) ones.
> >>>
> >>
> >> I would argue that imx7d is not correct since the IP blocks between
> >> imx7d and imx8mm have different flags/quirks.  One of which includes
> >> HS400-ES, but there are other differences as well.
> >
> > The DTS currently says that an fsl,imx7d-usdhc is a subset of an
> > fsl,imx8mm-usdhc. So a driver could treat both HW the exact same
> > by focusing on the i.MX7D parts. Linux apparently did exactly
> > that so far. Is this not accurate?
> >
> >
> >>> What do you think?
> >>
> >> From my understanding of the fallback compatibility strings is to
> >> avoid having to add more and more compatible strings to the drivers
> >> when they do not serve a functional purpose. Based On a conversation
> >> with Krzysztof [1], he suggested we update the YAML file based on the
> >> fallback, but he wanted NXP to give their feedback as to what the
> >> right fallback strings should be.  Haibo from NXP sent me a hierarchy
> >> [1] which is what I used to update the YAML file.  Based on the YAML
> >> file, the fallback in each DTSI file was updated to ensure the use of
> >> the proper IP block.
> >
> > Myself I am in favor of moving to three compatibles instead of dropping one.
> > For some theoretical fsl,imx8mf-usdhc that's supposed to be exactly the same
> > as a fsl,imx8mm-usdhc, I don't mind omitting the fsl,imx7d-usdhc compatible,
> > but for existing device trees, this may introduce needless potential breakage
> > for other software that also uses Linux device trees.
> >
>
> Affecting existing users is indeed a concern with this approach, because
> in-kernel DTS might be used in other projects as well.
>
> I still cannot find here the answer whether fsl,imx8mm-usdhc is actually
> compatible with fsl,imx7d-usdhc. It's not about driver, but about
> hardware and programming model. imx8mm can support additional features
> and still be compatible with imx7d. However if any flags of imx7d are
> actually not valid for imx8mm, then it's different case.

The imx7d flags are:
 ESDHC_FLAG_USDHC
ESDHC_FLAG_STD_TUNING
 ESDHC_FLAG_HAVE_CAP1
ESDHC_FLAG_HS200
 ESDHC_FLAG_HS400
 ESDHC_FLAG_STATE_LOST_IN_LPMODE
 ESDHC_FLAG_BROKEN_AUTO_CMD23,

The imx8mm flags are:
 ESDHC_FLAG_USDHC
 ESDHC_FLAG_STD_TUNING
 ESDHC_FLAG_HAVE_CAP1
ESDHC_FLAG_HS200
 ESDHC_FLAG_HS400
 ESDHC_FLAG_HS400_ES
 ESDHC_FLAG_STATE_LOST_IN_LPMODE

It does not have the ESDHC_FLAG_BROKEN_AUTO_CMD23 that is present in the imx7d.

Maybe Haibo can comment on whether or not that would be an issue for the 8m[mnp]

I will defer to Krzysztof and Haibo as to the proper method that we
should add HS400-ES.  I don't have an issue adding the imx8mn or
imx8mp compatible flags to the esdhc driver if that's the decision.
If that is the decision, my follow-up question would be how the YAML
should look, and if it needs to change at all.

adam

>
>
> Best regards,
> Krzysztof