| Message ID | 20120911080457.GA28235@lizard (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Sep 11, 2012, at 4:36 AM, Huang Changming-R66093 wrote: > Thanks, Anton. > If it is necessary, I will resend this patch to linuxppc-dev@lists.ozlabs.org. > > Best Regards > Jerry Huang I'm still not convinced this is the way to handle this issue. It seems as if the linux driver code makes some assumptions about CMD23 support that it shouldn't. - k > > >> -----Original Message----- >> From: Anton Vorontsov [mailto:cbouatmailru@gmail.com] >> Sent: Tuesday, September 11, 2012 4:05 PM >> To: Huang Changming-R66093 >> Cc: linux-mmc@vger.kernel.org; Kumar Gala; linuxppc-dev@lists.ozlabs.org >> Subject: Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23 >> >> On Tue, Sep 11, 2012 at 12:54:29AM -0700, Anton Vorontsov wrote: >>> On Tue, Sep 11, 2012 at 03:12:44PM +0800, Chang- >> Ming.Huang@freescale.com wrote: >>>> From: Jerry Huang <Chang-Ming.Huang@freescale.com> >>>> >>>> Below SOCs don't support the cmd23 command for MMC card, therefore, >>>> disable it in device tree: >>>> P1020, P1021, P1022, P1024, P1025 and P4080 >>>> >>>> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com> >>> >>> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com> >> >> Btw, although the patch is trivial, I guess you still want to let know >> PowerPC folks about it. Adding Cc and copying the patch: >> >> - - - - >> From: Jerry Huang <Chang-Ming.Huang@freescale.com> >> >> Below SOCs don't support the cmd23 command for MMC card, therefore, >> disable it in device tree: >> P1020, P1021, P1022, P1024, P1025 and P4080 >> >> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com> >> CC: Anton Vorontsov <cbouatmailru@gmail.com> >> --- >> arch/powerpc/boot/dts/fsl/p1020si-post.dtsi | 1 + >> arch/powerpc/boot/dts/fsl/p1021si-post.dtsi | 1 + >> arch/powerpc/boot/dts/fsl/p1022si-post.dtsi | 1 + >> arch/powerpc/boot/dts/fsl/p4080si-post.dtsi | 1 + >> 4 files changed, 4 insertions(+) >> >> diff --git a/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi >> b/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi >> index 68cc5e7..793a30b 100644 >> --- a/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi >> +++ b/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi >> @@ -154,6 +154,7 @@ >> sdhc@2e000 { >> compatible = "fsl,p1020-esdhc", "fsl,esdhc"; >> sdhci,auto-cmd12; >> + sdhci,no-cmd23; >> }; >> /include/ "pq3-sec3.3-0.dtsi" >> >> diff --git a/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi >> b/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi >> index adb82fd..2b7fd2a 100644 >> --- a/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi >> +++ b/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi >> @@ -149,6 +149,7 @@ >> /include/ "pq3-esdhc-0.dtsi" >> sdhc@2e000 { >> sdhci,auto-cmd12; >> + sdhci,no-cmd23; >> }; >> >> /include/ "pq3-sec3.3-0.dtsi" >> diff --git a/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi >> b/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi >> index 06216b8..2334a52 100644 >> --- a/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi >> +++ b/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi >> @@ -215,6 +215,7 @@ >> sdhc@2e000 { >> compatible = "fsl,p1022-esdhc", "fsl,esdhc"; >> sdhci,auto-cmd12; >> + sdhci,no-cmd23; >> }; >> >> /include/ "pq3-sec3.3-0.dtsi" >> diff --git a/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi >> b/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi >> index 8d35d2c..5b39952 100644 >> --- a/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi >> +++ b/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi >> @@ -337,6 +337,7 @@ >> sdhc@114000 { >> voltage-ranges = <3300 3300>; >> sdhci,auto-cmd12; >> + sdhci,no-cmd23; >> }; >> >> /include/ "qoriq-i2c-0.dtsi" >> -- >> 1.7.9.5 >
In sdhci_add_host()
We have the following
...
mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23;
if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12)
host->flags |= SDHCI_AUTO_CMD12;
/* Auto-CMD23 stuff only works in ADMA or PIO. */
if ((host->version >= SDHCI_SPEC_300) &&
((host->flags & SDHCI_USE_ADMA) ||
!(host->flags & SDHCI_USE_SDMA))) {
host->flags |= SDHCI_AUTO_CMD23;
DBG("%s: Auto-CMD23 available\n", mmc_hostname(mmc));
} else {
DBG("%s: Auto-CMD23 unavailable\n", mmc_hostname(mmc));
}
...
I'm not clear what the difference is between mmc->caps & host->flags, but shouldn't we move setting MMC_CAP_CMD23 inside the 'Auto-CMD23' if check?
- k
Hi, On Tue, Sep 11 2012, Kumar Gala wrote: > In sdhci_add_host() > > We have the following > > ... > mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23; > > if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) > host->flags |= SDHCI_AUTO_CMD12; > > /* Auto-CMD23 stuff only works in ADMA or PIO. */ > if ((host->version >= SDHCI_SPEC_300) && > ((host->flags & SDHCI_USE_ADMA) || > !(host->flags & SDHCI_USE_SDMA))) { > host->flags |= SDHCI_AUTO_CMD23; > DBG("%s: Auto-CMD23 available\n", mmc_hostname(mmc)); > } else { > DBG("%s: Auto-CMD23 unavailable\n", mmc_hostname(mmc)); > } > > ... > > I'm not clear what the difference is between mmc->caps & host->flags, but shouldn't we move setting MMC_CAP_CMD23 inside the 'Auto-CMD23' if check? The main answer is: No, because CMD23 is distinct from Auto-CMD23. Multiblock transfers (CMD23) date back from MMC cards (which is why they're an MMC host capability) and can also be used in SDHCI. Auto-CMD23 is a new feature in SDHCI 3.0 that reduces the overhead of sending CMD23. It doesn't work if we're using SDMA, though. As for capability vs. flags, the capability is more of an inherent property of the controller, and flags are runtime decisions on whether to use that capability, based on e.g. the presence of a quirk. So, I think the code's correct as written. Feel free to ask more questions if you're investigating a specific problem that you haven't mentioned yet. Thanks, - Chris.
On 09/11/2012 03:04 AM, Anton Vorontsov wrote: > On Tue, Sep 11, 2012 at 12:54:29AM -0700, Anton Vorontsov wrote: >> On Tue, Sep 11, 2012 at 03:12:44PM +0800, Chang-Ming.Huang@freescale.com wrote: >>> From: Jerry Huang <Chang-Ming.Huang@freescale.com> >>> >>> Below SOCs don't support the cmd23 command for MMC card, >>> therefore, disable it in device tree: >>> P1020, P1021, P1022, P1024, P1025 and P4080 >>> >>> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com> >> >> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com> > > Btw, although the patch is trivial, I guess you still want to let know > PowerPC folks about it. Adding Cc and copying the patch: > > - - - - > From: Jerry Huang <Chang-Ming.Huang@freescale.com> > > Below SOCs don't support the cmd23 command for MMC card, > therefore, disable it in device tree: > P1020, P1021, P1022, P1024, P1025 and P4080 > > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com> > CC: Anton Vorontsov <cbouatmailru@gmail.com> > --- > arch/powerpc/boot/dts/fsl/p1020si-post.dtsi | 1 + > arch/powerpc/boot/dts/fsl/p1021si-post.dtsi | 1 + > arch/powerpc/boot/dts/fsl/p1022si-post.dtsi | 1 + > arch/powerpc/boot/dts/fsl/p4080si-post.dtsi | 1 + > 4 files changed, 4 insertions(+) > > diff --git a/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi b/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi > index 68cc5e7..793a30b 100644 > --- a/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi > +++ b/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi > @@ -154,6 +154,7 @@ > sdhc@2e000 { > compatible = "fsl,p1020-esdhc", "fsl,esdhc"; > sdhci,auto-cmd12; > + sdhci,no-cmd23; > }; > /include/ "pq3-sec3.3-0.dtsi" > > diff --git a/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi b/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi > index adb82fd..2b7fd2a 100644 > --- a/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi > +++ b/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi > @@ -149,6 +149,7 @@ > /include/ "pq3-esdhc-0.dtsi" > sdhc@2e000 { > sdhci,auto-cmd12; > + sdhci,no-cmd23; > }; > > /include/ "pq3-sec3.3-0.dtsi" > diff --git a/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi b/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi > index 06216b8..2334a52 100644 > --- a/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi > +++ b/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi > @@ -215,6 +215,7 @@ > sdhc@2e000 { > compatible = "fsl,p1022-esdhc", "fsl,esdhc"; > sdhci,auto-cmd12; > + sdhci,no-cmd23; > }; > > /include/ "pq3-sec3.3-0.dtsi" > diff --git a/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi b/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi > index 8d35d2c..5b39952 100644 > --- a/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi > +++ b/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi > @@ -337,6 +337,7 @@ > sdhc@114000 { > voltage-ranges = <3300 3300>; > sdhci,auto-cmd12; > + sdhci,no-cmd23; > }; > > /include/ "qoriq-i2c-0.dtsi" > This won't help people with old device trees (forked for a custom board, tied to an old U-Boot, etc). The driver should infer this from the compatible string or version registers (ideally block-specific version registers, but if these are absent or inconclusive, SVR can be used). -Scott
On Sep 11, 2012, at 9:44 AM, Chris Ball wrote: > Hi, > > On Tue, Sep 11 2012, Kumar Gala wrote: >> In sdhci_add_host() >> >> We have the following >> >> ... >> mmc->caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23; >> >> if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12) >> host->flags |= SDHCI_AUTO_CMD12; >> >> /* Auto-CMD23 stuff only works in ADMA or PIO. */ >> if ((host->version >= SDHCI_SPEC_300) && >> ((host->flags & SDHCI_USE_ADMA) || >> !(host->flags & SDHCI_USE_SDMA))) { >> host->flags |= SDHCI_AUTO_CMD23; >> DBG("%s: Auto-CMD23 available\n", mmc_hostname(mmc)); >> } else { >> DBG("%s: Auto-CMD23 unavailable\n", mmc_hostname(mmc)); >> } >> >> ... >> >> I'm not clear what the difference is between mmc->caps & host->flags, but shouldn't we move setting MMC_CAP_CMD23 inside the 'Auto-CMD23' if check? > > The main answer is: No, because CMD23 is distinct from Auto-CMD23. > > Multiblock transfers (CMD23) date back from MMC cards (which is why > they're an MMC host capability) and can also be used in SDHCI. > > Auto-CMD23 is a new feature in SDHCI 3.0 that reduces the overhead > of sending CMD23. It doesn't work if we're using SDMA, though. > > As for capability vs. flags, the capability is more of an inherent > property of the controller, and flags are runtime decisions on whether > to use that capability, based on e.g. the presence of a quirk. > > So, I think the code's correct as written. Feel free to ask more > questions if you're investigating a specific problem that you haven't > mentioned yet. Chris, thanks for the info. Do you know what's required on controller side to handle cards that support CMD23? I'm trying to figure out if older controller's on FSL SoCs are missing some feature to allow CMD23 to work (vs Auto-CMD23). - k
Hi, On Tue, Sep 11 2012, Kumar Gala wrote: > thanks for the info. Do you know what's required on controller side > to handle cards that support CMD23? > > I'm trying to figure out if older controller's on FSL SoCs are missing > some feature to allow CMD23 to work (vs Auto-CMD23). It seems plausible that it's just not implemented on these controllers. It's a little strange, since the command's been specified for so long and we haven't seen any other controllers with problems. The patch would be correct if this is true. - Chris.
Best Regards Jerry Huang > -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, September 12, 2012 2:28 AM > To: Anton Vorontsov > Cc: Huang Changming-R66093; linuxppc-dev@lists.ozlabs.org; linux- > mmc@vger.kernel.org > Subject: Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23 > > On 09/11/2012 03:04 AM, Anton Vorontsov wrote: > > On Tue, Sep 11, 2012 at 12:54:29AM -0700, Anton Vorontsov wrote: > >> On Tue, Sep 11, 2012 at 03:12:44PM +0800, Chang- > Ming.Huang@freescale.com wrote: > >>> From: Jerry Huang <Chang-Ming.Huang@freescale.com> > >>> > >>> Below SOCs don't support the cmd23 command for MMC card, therefore, > >>> disable it in device tree: > >>> P1020, P1021, P1022, P1024, P1025 and P4080 > >>> > >>> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com> > >> > >> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com> > > > > Btw, although the patch is trivial, I guess you still want to let know > > PowerPC folks about it. Adding Cc and copying the patch: > > > > - - - - > > From: Jerry Huang <Chang-Ming.Huang@freescale.com> > > > > Below SOCs don't support the cmd23 command for MMC card, therefore, > > disable it in device tree: > > P1020, P1021, P1022, P1024, P1025 and P4080 > > > > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com> > > CC: Anton Vorontsov <cbouatmailru@gmail.com> > > --- > > arch/powerpc/boot/dts/fsl/p1020si-post.dtsi | 1 + > > arch/powerpc/boot/dts/fsl/p1021si-post.dtsi | 1 + > > arch/powerpc/boot/dts/fsl/p1022si-post.dtsi | 1 + > > arch/powerpc/boot/dts/fsl/p4080si-post.dtsi | 1 + > > 4 files changed, 4 insertions(+) > > > > diff --git a/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi > > b/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi > > index 68cc5e7..793a30b 100644 > > --- a/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi > > +++ b/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi > > @@ -154,6 +154,7 @@ > > sdhc@2e000 { > > compatible = "fsl,p1020-esdhc", "fsl,esdhc"; > > sdhci,auto-cmd12; > > + sdhci,no-cmd23; > > }; > > /include/ "pq3-sec3.3-0.dtsi" > > > > diff --git a/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi > > b/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi > > index adb82fd..2b7fd2a 100644 > > --- a/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi > > +++ b/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi > > @@ -149,6 +149,7 @@ > > /include/ "pq3-esdhc-0.dtsi" > > sdhc@2e000 { > > sdhci,auto-cmd12; > > + sdhci,no-cmd23; > > }; > > > > /include/ "pq3-sec3.3-0.dtsi" > > diff --git a/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi > > b/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi > > index 06216b8..2334a52 100644 > > --- a/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi > > +++ b/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi > > @@ -215,6 +215,7 @@ > > sdhc@2e000 { > > compatible = "fsl,p1022-esdhc", "fsl,esdhc"; > > sdhci,auto-cmd12; > > + sdhci,no-cmd23; > > }; > > > > /include/ "pq3-sec3.3-0.dtsi" > > diff --git a/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi > > b/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi > > index 8d35d2c..5b39952 100644 > > --- a/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi > > +++ b/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi > > @@ -337,6 +337,7 @@ > > sdhc@114000 { > > voltage-ranges = <3300 3300>; > > sdhci,auto-cmd12; > > + sdhci,no-cmd23; > > }; > > > > /include/ "qoriq-i2c-0.dtsi" > > > > This won't help people with old device trees (forked for a custom board, > tied to an old U-Boot, etc). The driver should infer this from the > compatible string or version registers (ideally block-specific version > registers, but if these are absent or inconclusive, SVR can be used). > I don't think it is the best way to do it. For the VVN2.2 or older, some silicon support this feature (mpc8536 and p2020), but other silicones don't support it (e.g. p4080, p102x). Though, the current p5/p4/p3 has supported this feature, can we sure the future silicon support it? So I think the best way is to specify it in device tree as 'sdhci,auto-cmd12'
On Wed, Sep 12, 2012 at 03:19:18AM +0000, Huang Changming-R66093 wrote: [...] > I don't think it is the best way to do it. For the VVN2.2 or older, > some silicon support this feature (mpc8536 and p2020), but other > silicones don't support it (e.g. p4080, p102x). Though, the current > p5/p4/p3 has supported this feature, can we sure the future silicon > support it? So I think the best way is to specify it in device tree > as 'sdhci,auto-cmd12' In addition to your current patches, you could just add another patch that blacklists affected SOC revisions based on the info from PVR/SVR. For example, see gfar_detect_errata() in drivers/net/ethernet/freescale/gianfar.c. That way you could help users that don't have the newest device trees. Thanks, Anton.
Best Regards Jerry Huang > -----Original Message----- > From: Chris Ball [mailto:cjb@laptop.org] > Sent: Wednesday, September 12, 2012 4:59 AM > To: Kumar Gala > Cc: Huang Changming-R66093; linuxppc-dev@lists.ozlabs.org list; linux- > mmc@vger.kernel.org; Anton Vorontsov > Subject: Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23 > > Hi, > > On Tue, Sep 11 2012, Kumar Gala wrote: > > thanks for the info. Do you know what's required on controller side > > to handle cards that support CMD23? > > > > I'm trying to figure out if older controller's on FSL SoCs are missing > > some feature to allow CMD23 to work (vs Auto-CMD23). > > It seems plausible that it's just not implemented on these controllers. > It's a little strange, since the command's been specified for so long and > we haven't seen any other controllers with problems. The patch would be > correct if this is true. > I didn't find any description about it, but after testing on FSL silicones, I got this result: Some silicones support this command, and some silicones don't support it, which will cause I/O error.
On Sep 12, 2012, at 1:18 AM, Huang Changming-R66093 wrote: > > > Best Regards > Jerry Huang > > >> -----Original Message----- >> From: Chris Ball [mailto:cjb@laptop.org] >> Sent: Wednesday, September 12, 2012 4:59 AM >> To: Kumar Gala >> Cc: Huang Changming-R66093; linuxppc-dev@lists.ozlabs.org list; linux- >> mmc@vger.kernel.org; Anton Vorontsov >> Subject: Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23 >> >> Hi, >> >> On Tue, Sep 11 2012, Kumar Gala wrote: >>> thanks for the info. Do you know what's required on controller side >>> to handle cards that support CMD23? >>> >>> I'm trying to figure out if older controller's on FSL SoCs are missing >>> some feature to allow CMD23 to work (vs Auto-CMD23). >> >> It seems plausible that it's just not implemented on these controllers. >> It's a little strange, since the command's been specified for so long and >> we haven't seen any other controllers with problems. The patch would be >> correct if this is true. >> > > I didn't find any description about it, but after testing on FSL silicones, I got this result: > Some silicones support this command, and some silicones don't support it, which will cause I/O error. Can you list out which SoCs support it and which don't. Having this list will be useful in understanding which controller versions supported it. - k
> > > >> -----Original Message----- > >> From: Chris Ball [mailto:cjb@laptop.org] > >> Sent: Wednesday, September 12, 2012 4:59 AM > >> To: Kumar Gala > >> Cc: Huang Changming-R66093; linuxppc-dev@lists.ozlabs.org list; > >> linux- mmc@vger.kernel.org; Anton Vorontsov > >> Subject: Re: [PATCH 2/3] powerpc/esdhc: add property to disable the > >> CMD23 > >> > >> Hi, > >> > >> On Tue, Sep 11 2012, Kumar Gala wrote: > >>> thanks for the info. Do you know what's required on controller side > >>> to handle cards that support CMD23? > >>> > >>> I'm trying to figure out if older controller's on FSL SoCs are > >>> missing some feature to allow CMD23 to work (vs Auto-CMD23). > >> > >> It seems plausible that it's just not implemented on these controllers. > >> It's a little strange, since the command's been specified for so long > >> and we haven't seen any other controllers with problems. The patch > >> would be correct if this is true. > >> > > > > I didn't find any description about it, but after testing on FSL > silicones, I got this result: > > Some silicones support this command, and some silicones don't support > it, which will cause I/O error. > > Can you list out which SoCs support it and which don't. Having this list > will be useful in understanding which controller versions supported it. > P1020, p1021, p1022, p1024, p1015 and p4080 can't support it. Mpc8536, p2020, and the other current DPAA silicon (e.g. p5020, p3041) support it.
Best Regards Jerry Huang > -----Original Message----- > From: Anton Vorontsov [mailto:cbouatmailru@gmail.com] > Sent: Wednesday, September 12, 2012 11:38 AM > To: Huang Changming-R66093 > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; linux- > mmc@vger.kernel.org > Subject: Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23 > > On Wed, Sep 12, 2012 at 03:19:18AM +0000, Huang Changming-R66093 wrote: > [...] > > I don't think it is the best way to do it. For the VVN2.2 or older, > > some silicon support this feature (mpc8536 and p2020), but other > > silicones don't support it (e.g. p4080, p102x). Though, the current > > p5/p4/p3 has supported this feature, can we sure the future silicon > > support it? So I think the best way is to specify it in device tree > > as 'sdhci,auto-cmd12' > > In addition to your current patches, you could just add another patch > that blacklists affected SOC revisions based on the info from PVR/SVR. > > For example, see gfar_detect_errata() in > drivers/net/ethernet/freescale/gianfar.c. > > That way you could help users that don't have the newest device trees. > Hi, Anton Thanks. But, these patches are just for latest kernel, if the user don't have the latest device tree, Then they don't have the latest kernel, and these patches can't be used for them directly, Though provide the function like gfar_detect_errata, they must modify their codes. And I don't know if the future silicon can support CMD23, if not, we must modify sdhc driver again. When we use the device tree to identify it, if the silicon does not support this feature, we just add this property into the new device tree, not need to modify SDHC driver. I think, this is the best way.
On Sep 12, 2012, at 9:02 PM, Huang Changming-R66093 wrote: >>> >>>> -----Original Message----- >>>> From: Chris Ball [mailto:cjb@laptop.org] >>>> Sent: Wednesday, September 12, 2012 4:59 AM >>>> To: Kumar Gala >>>> Cc: Huang Changming-R66093; linuxppc-dev@lists.ozlabs.org list; >>>> linux- mmc@vger.kernel.org; Anton Vorontsov >>>> Subject: Re: [PATCH 2/3] powerpc/esdhc: add property to disable the >>>> CMD23 >>>> >>>> Hi, >>>> >>>> On Tue, Sep 11 2012, Kumar Gala wrote: >>>>> thanks for the info. Do you know what's required on controller side >>>>> to handle cards that support CMD23? >>>>> >>>>> I'm trying to figure out if older controller's on FSL SoCs are >>>>> missing some feature to allow CMD23 to work (vs Auto-CMD23). >>>> >>>> It seems plausible that it's just not implemented on these controllers. >>>> It's a little strange, since the command's been specified for so long >>>> and we haven't seen any other controllers with problems. The patch >>>> would be correct if this is true. >>>> >>> >>> I didn't find any description about it, but after testing on FSL >> silicones, I got this result: >>> Some silicones support this command, and some silicones don't support >> it, which will cause I/O error. >> >> Can you list out which SoCs support it and which don't. Having this list >> will be useful in understanding which controller versions supported it. >> > P1020, p1021, p1022, p1024, p1015 and p4080 can't support it. > Mpc8536, p2020, and the other current DPAA silicon (e.g. p5020, p3041) support it. Based on this, why don't we use the HOSTVER register to detect instead of device tree: #define FSL_SDHC_VER_1_0 0x00 #define FSL_SDHC_VER_1_1 0x01 #define FSL_SDHC_VER_2_0 0x10 #define FSL_SDHC_VER_2_1 0x11 #define FSL_SDHC_VER_2_2 0x12 #define FSL_SDHC_VER_2_3 0x13 unsigned int vendor_version; vendor_version = sdhci_readw(host, SDHCI_HOST_VERSION); vendor_version = (vendor_version & SDHCI_VENDOR_VER_MASK) >> SDHCI_VENDOR_VER_SHIFT; if ((vendor_version == FSL_SDHC_VER_1_1) || (vendor_version == FSL_SDHC_VER_2_2)) host->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23; - k
Best Regards Jerry Huang > -----Original Message----- > From: Kumar Gala [mailto:galak@kernel.crashing.org] > Sent: Thursday, September 13, 2012 8:48 PM > To: Huang Changming-R66093 > Cc: Chris Ball; linuxppc-dev@lists.ozlabs.org list; linux- > mmc@vger.kernel.org; Anton Vorontsov > Subject: Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23 > > > On Sep 12, 2012, at 9:02 PM, Huang Changming-R66093 wrote: > > >>> > >>>> -----Original Message----- > >>>> From: Chris Ball [mailto:cjb@laptop.org] > >>>> Sent: Wednesday, September 12, 2012 4:59 AM > >>>> To: Kumar Gala > >>>> Cc: Huang Changming-R66093; linuxppc-dev@lists.ozlabs.org list; > >>>> linux- mmc@vger.kernel.org; Anton Vorontsov > >>>> Subject: Re: [PATCH 2/3] powerpc/esdhc: add property to disable the > >>>> CMD23 > >>>> > >>>> Hi, > >>>> > >>>> On Tue, Sep 11 2012, Kumar Gala wrote: > >>>>> thanks for the info. Do you know what's required on controller > >>>>> side to handle cards that support CMD23? > >>>>> > >>>>> I'm trying to figure out if older controller's on FSL SoCs are > >>>>> missing some feature to allow CMD23 to work (vs Auto-CMD23). > >>>> > >>>> It seems plausible that it's just not implemented on these > controllers. > >>>> It's a little strange, since the command's been specified for so > >>>> long and we haven't seen any other controllers with problems. The > >>>> patch would be correct if this is true. > >>>> > >>> > >>> I didn't find any description about it, but after testing on FSL > >> silicones, I got this result: > >>> Some silicones support this command, and some silicones don't > >>> support > >> it, which will cause I/O error. > >> > >> Can you list out which SoCs support it and which don't. Having this > >> list will be useful in understanding which controller versions > supported it. > >> > > P1020, p1021, p1022, p1024, p1015 and p4080 can't support it. > > Mpc8536, p2020, and the other current DPAA silicon (e.g. p5020, p3041) > support it. > > Based on this, why don't we use the HOSTVER register to detect instead of > device tree: > > > #define FSL_SDHC_VER_1_0 0x00 > #define FSL_SDHC_VER_1_1 0x01 > #define FSL_SDHC_VER_2_0 0x10 > #define FSL_SDHC_VER_2_1 0x11 > #define FSL_SDHC_VER_2_2 0x12 > #define FSL_SDHC_VER_2_3 0x13 > > unsigned int vendor_version; > > vendor_version = sdhci_readw(host, SDHCI_HOST_VERSION); vendor_version = > (vendor_version & SDHCI_VENDOR_VER_MASK) >> SDHCI_VENDOR_VER_SHIFT; > > if ((vendor_version == FSL_SDHC_VER_1_1) || (vendor_version == > FSL_SDHC_VER_2_2)) > host->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23; > I once thought about it, but if the future silicon does not support this feature, then we continue to modify these codes for new silicon?
>>>>> >>>>>> -----Original Message----- >>>>>> From: Chris Ball [mailto:cjb@laptop.org] >>>>>> Sent: Wednesday, September 12, 2012 4:59 AM >>>>>> To: Kumar Gala >>>>>> Cc: Huang Changming-R66093; linuxppc-dev@lists.ozlabs.org list; >>>>>> linux- mmc@vger.kernel.org; Anton Vorontsov >>>>>> Subject: Re: [PATCH 2/3] powerpc/esdhc: add property to disable the >>>>>> CMD23 >>>>>> >>>>>> Hi, >>>>>> >>>>>> On Tue, Sep 11 2012, Kumar Gala wrote: >>>>>>> thanks for the info. Do you know what's required on controller >>>>>>> side to handle cards that support CMD23? >>>>>>> >>>>>>> I'm trying to figure out if older controller's on FSL SoCs are >>>>>>> missing some feature to allow CMD23 to work (vs Auto-CMD23). >>>>>> >>>>>> It seems plausible that it's just not implemented on these >> controllers. >>>>>> It's a little strange, since the command's been specified for so >>>>>> long and we haven't seen any other controllers with problems. The >>>>>> patch would be correct if this is true. >>>>>> >>>>> >>>>> I didn't find any description about it, but after testing on FSL >>>> silicones, I got this result: >>>>> Some silicones support this command, and some silicones don't >>>>> support >>>> it, which will cause I/O error. >>>> >>>> Can you list out which SoCs support it and which don't. Having this >>>> list will be useful in understanding which controller versions >> supported it. >>>> >>> P1020, p1021, p1022, p1024, p1015 and p4080 can't support it. >>> Mpc8536, p2020, and the other current DPAA silicon (e.g. p5020, p3041) >> support it. >> >> Based on this, why don't we use the HOSTVER register to detect instead of >> device tree: >> >> >> #define FSL_SDHC_VER_1_0 0x00 >> #define FSL_SDHC_VER_1_1 0x01 >> #define FSL_SDHC_VER_2_0 0x10 >> #define FSL_SDHC_VER_2_1 0x11 >> #define FSL_SDHC_VER_2_2 0x12 >> #define FSL_SDHC_VER_2_3 0x13 >> >> unsigned int vendor_version; >> >> vendor_version = sdhci_readw(host, SDHCI_HOST_VERSION); vendor_version = >> (vendor_version & SDHCI_VENDOR_VER_MASK) >> SDHCI_VENDOR_VER_SHIFT; >> >> if ((vendor_version == FSL_SDHC_VER_1_1) || (vendor_version == >> FSL_SDHC_VER_2_2)) >> host->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23; >> > > I once thought about it, but if the future silicon does not support this feature, > then we continue to modify these codes for new silicon? Yes, but it seems extremely unlikely that future versions of the controller will remove this feature now that it exists. - k
Hi, On Thu, Sep 13 2012, Kumar Gala wrote: >>> Can you list out which SoCs support it and which don't. Having this list >>> will be useful in understanding which controller versions supported it. >>> >> P1020, p1021, p1022, p1024, p1015 and p4080 can't support it. >> Mpc8536, p2020, and the other current DPAA silicon (e.g. p5020, p3041) support it. > > Based on this, why don't we use the HOSTVER register to detect instead of device tree: I've got a mild preference for handling quirk assignment in the DT rather than in driver code, so I'd prefer to just push the original patch to mmc-next as-is. Does that sound okay? (I think the argument that there isn't going to be any new hardware with this problem is equally in favor of both methods.) Thanks, - Chris.
On Sep 17, 2012, at 7:36 AM, Chris Ball wrote: > Hi, > > On Thu, Sep 13 2012, Kumar Gala wrote: >>>> Can you list out which SoCs support it and which don't. Having this list >>>> will be useful in understanding which controller versions supported it. >>>> >>> P1020, p1021, p1022, p1024, p1015 and p4080 can't support it. >>> Mpc8536, p2020, and the other current DPAA silicon (e.g. p5020, p3041) support it. >> >> Based on this, why don't we use the HOSTVER register to detect instead of device tree: > > I've got a mild preference for handling quirk assignment in the DT > rather than in driver code, so I'd prefer to just push the original > patch to mmc-next as-is. Does that sound okay? Why? I only ask because I agree with Scott that this means you have to update your device tree to get proper functionality. > (I think the argument that there isn't going to be any new hardware > with this problem is equally in favor of both methods.) - k
Hi, On Mon, Sep 17 2012, Kumar Gala wrote: >>>> P1020, p1021, p1022, p1024, p1015 and p4080 can't support it. >>>> Mpc8536, p2020, and the other current DPAA silicon (e.g. p5020, p3041) support it. >>> >>> Based on this, why don't we use the HOSTVER register to detect instead of device tree: >> >> I've got a mild preference for handling quirk assignment in the DT >> rather than in driver code, so I'd prefer to just push the original >> patch to mmc-next as-is. Does that sound okay? > > Why? I only ask because I agree with Scott that this means you have to update your device tree to get proper functionality. Thanks, I'd missed that. I withdraw my preference; I'll pick up whichever method you all prefer. - Chris.
> On Sep 17, 2012, at 7:36 AM, Chris Ball wrote: > > > Hi, > > > > On Thu, Sep 13 2012, Kumar Gala wrote: > >>>> Can you list out which SoCs support it and which don't. Having > >>>> this list will be useful in understanding which controller versions > supported it. > >>>> > >>> P1020, p1021, p1022, p1024, p1015 and p4080 can't support it. > >>> Mpc8536, p2020, and the other current DPAA silicon (e.g. p5020, p3041) > support it. > >> > >> Based on this, why don't we use the HOSTVER register to detect instead > of device tree: > > > > I've got a mild preference for handling quirk assignment in the DT > > rather than in driver code, so I'd prefer to just push the original > > patch to mmc-next as-is. Does that sound okay? > > Why? I only ask because I agree with Scott that this means you have to > update your device tree to get proper functionality. > When the new silicon does not support CMD23, if we don't update the device tree, then we must update the SDHC driver. I prefer to add the property in device tree, because we just add this property in new device tree, we don't need more effort to modify driver.
On Sep 17, 2012, at 8:09 PM, Huang Changming-R66093 wrote: >> On Sep 17, 2012, at 7:36 AM, Chris Ball wrote: >> >>> Hi, >>> >>> On Thu, Sep 13 2012, Kumar Gala wrote: >>>>>> Can you list out which SoCs support it and which don't. Having >>>>>> this list will be useful in understanding which controller versions >> supported it. >>>>>> >>>>> P1020, p1021, p1022, p1024, p1015 and p4080 can't support it. >>>>> Mpc8536, p2020, and the other current DPAA silicon (e.g. p5020, p3041) >> support it. >>>> >>>> Based on this, why don't we use the HOSTVER register to detect instead >> of device tree: >>> >>> I've got a mild preference for handling quirk assignment in the DT >>> rather than in driver code, so I'd prefer to just push the original >>> patch to mmc-next as-is. Does that sound okay? >> >> Why? I only ask because I agree with Scott that this means you have to >> update your device tree to get proper functionality. >> > When the new silicon does not support CMD23, > if we don't update the device tree, then we must update the SDHC driver. > I prefer to add the property in device tree, > because we just add this property in new device tree, we don't need more effort to modify driver. > Jerry, I think doing it driver makes more sense because: 1. means older device tree's still work 2. odds that CMD23 not being supported in future devices is near 0% (Now that we support AutoCMD23 [and thus CMD23] we aren't likely to stop supporting it in future) 3. If IP changes you are going to have to update driver anyways for new features I really think we should NOT utilize device tree for this. - k
Hi, On Tue, Sep 18 2012, Kumar Gala wrote: >>>> I've got a mild preference for handling quirk assignment in the DT >>>> rather than in driver code, so I'd prefer to just push the original >>>> patch to mmc-next as-is. Does that sound okay? >>> >>> Why? I only ask because I agree with Scott that this means you have to >>> update your device tree to get proper functionality. >>> >> When the new silicon does not support CMD23, >> if we don't update the device tree, then we must update the SDHC driver. >> I prefer to add the property in device tree, >> because we just add this property in new device tree, we don't need more effort to modify driver. > > Jerry, > > I think doing it driver makes more sense because: > > 1. means older device tree's still work > 2. odds that CMD23 not being supported in future devices is near 0% > (Now that we support AutoCMD23 [and thus CMD23] we aren't likely to stop supporting it in future) > 3. If IP changes you are going to have to update driver anyways for new features > > I really think we should NOT utilize device tree for this. Of course, we could also make both (or perhaps neither) of you happy by merging both: if your DT says you don't support cmd23 *or* you hit the driver's blacklist, we avoid it. - Chris.
diff --git a/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi b/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi index 68cc5e7..793a30b 100644 --- a/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi +++ b/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi @@ -154,6 +154,7 @@ sdhc@2e000 { compatible = "fsl,p1020-esdhc", "fsl,esdhc"; sdhci,auto-cmd12; + sdhci,no-cmd23; }; /include/ "pq3-sec3.3-0.dtsi" diff --git a/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi b/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi index adb82fd..2b7fd2a 100644 --- a/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi +++ b/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi @@ -149,6 +149,7 @@ /include/ "pq3-esdhc-0.dtsi" sdhc@2e000 { sdhci,auto-cmd12; + sdhci,no-cmd23; }; /include/ "pq3-sec3.3-0.dtsi" diff --git a/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi b/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi index 06216b8..2334a52 100644 --- a/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi +++ b/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi @@ -215,6 +215,7 @@ sdhc@2e000 { compatible = "fsl,p1022-esdhc", "fsl,esdhc"; sdhci,auto-cmd12; + sdhci,no-cmd23; }; /include/ "pq3-sec3.3-0.dtsi" diff --git a/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi b/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi index 8d35d2c..5b39952 100644 --- a/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi +++ b/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi @@ -337,6 +337,7 @@ sdhc@114000 { voltage-ranges = <3300 3300>; sdhci,auto-cmd12; + sdhci,no-cmd23; }; /include/ "qoriq-i2c-0.dtsi"