Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Anton Vorontsov]

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

RE: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Huang Changming-R66093]

VGhhbmtzLCBBbnRvbi4NCklmIGl0IGlzIG5lY2Vzc2FyeSwgSSB3aWxsIHJlc2VuZCB0aGlzIHBh
dGNoIHRvIGxpbnV4cHBjLWRldkBsaXN0cy5vemxhYnMub3JnLg0KDQpCZXN0IFJlZ2FyZHMNCkpl
cnJ5IEh1YW5nDQoNCg0KPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBBbnRv
biBWb3JvbnRzb3YgW21haWx0bzpjYm91YXRtYWlscnVAZ21haWwuY29tXQ0KPiBTZW50OiBUdWVz
ZGF5LCBTZXB0ZW1iZXIgMTEsIDIwMTIgNDowNSBQTQ0KPiBUbzogSHVhbmcgQ2hhbmdtaW5nLVI2
NjA5Mw0KPiBDYzogbGludXgtbW1jQHZnZXIua2VybmVsLm9yZzsgS3VtYXIgR2FsYTsgbGludXhw
cGMtZGV2QGxpc3RzLm96bGFicy5vcmcNCj4gU3ViamVjdDogUmU6IFtQQVRDSCAyLzNdIHBvd2Vy
cGMvZXNkaGM6IGFkZCBwcm9wZXJ0eSB0byBkaXNhYmxlIHRoZSBDTUQyMw0KPiANCj4gT24gVHVl
LCBTZXAgMTEsIDIwMTIgYXQgMTI6NTQ6MjlBTSAtMDcwMCwgQW50b24gVm9yb250c292IHdyb3Rl
Og0KPiA+IE9uIFR1ZSwgU2VwIDExLCAyMDEyIGF0IDAzOjEyOjQ0UE0gKzA4MDAsIENoYW5nLQ0K
PiBNaW5nLkh1YW5nQGZyZWVzY2FsZS5jb20gd3JvdGU6DQo+ID4gPiBGcm9tOiBKZXJyeSBIdWFu
ZyA8Q2hhbmctTWluZy5IdWFuZ0BmcmVlc2NhbGUuY29tPg0KPiA+ID4NCj4gPiA+IEJlbG93IFNP
Q3MgZG9uJ3Qgc3VwcG9ydCB0aGUgY21kMjMgY29tbWFuZCBmb3IgTU1DIGNhcmQsIHRoZXJlZm9y
ZSwNCj4gPiA+IGRpc2FibGUgaXQgaW4gZGV2aWNlIHRyZWU6DQo+ID4gPiBQMTAyMCwgUDEwMjEs
IFAxMDIyLCBQMTAyNCwgUDEwMjUgYW5kIFA0MDgwDQo+ID4gPg0KPiA+ID4gU2lnbmVkLW9mZi1i
eTogSmVycnkgSHVhbmcgPENoYW5nLU1pbmcuSHVhbmdAZnJlZXNjYWxlLmNvbT4NCj4gPg0KPiA+
IEFja2VkLWJ5OiBBbnRvbiBWb3JvbnRzb3YgPGNib3VhdG1haWxydUBnbWFpbC5jb20+DQo+IA0K
PiBCdHcsIGFsdGhvdWdoIHRoZSBwYXRjaCBpcyB0cml2aWFsLCBJIGd1ZXNzIHlvdSBzdGlsbCB3
YW50IHRvIGxldCBrbm93DQo+IFBvd2VyUEMgZm9sa3MgYWJvdXQgaXQuIEFkZGluZyBDYyBhbmQg
Y29weWluZyB0aGUgcGF0Y2g6DQo+IA0KPiAtIC0gLSAtDQo+IEZyb206IEplcnJ5IEh1YW5nIDxD
aGFuZy1NaW5nLkh1YW5nQGZyZWVzY2FsZS5jb20+DQo+IA0KPiBCZWxvdyBTT0NzIGRvbid0IHN1
cHBvcnQgdGhlIGNtZDIzIGNvbW1hbmQgZm9yIE1NQyBjYXJkLCB0aGVyZWZvcmUsDQo+IGRpc2Fi
bGUgaXQgaW4gZGV2aWNlIHRyZWU6DQo+IFAxMDIwLCBQMTAyMSwgUDEwMjIsIFAxMDI0LCBQMTAy
NSBhbmQgUDQwODANCj4gDQo+IFNpZ25lZC1vZmYtYnk6IEplcnJ5IEh1YW5nIDxDaGFuZy1NaW5n
Lkh1YW5nQGZyZWVzY2FsZS5jb20+DQo+IENDOiBBbnRvbiBWb3JvbnRzb3YgPGNib3VhdG1haWxy
dUBnbWFpbC5jb20+DQo+IC0tLQ0KPiAgYXJjaC9wb3dlcnBjL2Jvb3QvZHRzL2ZzbC9wMTAyMHNp
LXBvc3QuZHRzaSB8ICAgIDEgKw0KPiAgYXJjaC9wb3dlcnBjL2Jvb3QvZHRzL2ZzbC9wMTAyMXNp
LXBvc3QuZHRzaSB8ICAgIDEgKw0KPiAgYXJjaC9wb3dlcnBjL2Jvb3QvZHRzL2ZzbC9wMTAyMnNp
LXBvc3QuZHRzaSB8ICAgIDEgKw0KPiAgYXJjaC9wb3dlcnBjL2Jvb3QvZHRzL2ZzbC9wNDA4MHNp
LXBvc3QuZHRzaSB8ICAgIDEgKw0KPiAgNCBmaWxlcyBjaGFuZ2VkLCA0IGluc2VydGlvbnMoKykN
Cj4gDQo+IGRpZmYgLS1naXQgYS9hcmNoL3Bvd2VycGMvYm9vdC9kdHMvZnNsL3AxMDIwc2ktcG9z
dC5kdHNpDQo+IGIvYXJjaC9wb3dlcnBjL2Jvb3QvZHRzL2ZzbC9wMTAyMHNpLXBvc3QuZHRzaQ0K
PiBpbmRleCA2OGNjNWU3Li43OTNhMzBiIDEwMDY0NA0KPiAtLS0gYS9hcmNoL3Bvd2VycGMvYm9v
dC9kdHMvZnNsL3AxMDIwc2ktcG9zdC5kdHNpDQo+ICsrKyBiL2FyY2gvcG93ZXJwYy9ib290L2R0
cy9mc2wvcDEwMjBzaS1wb3N0LmR0c2kNCj4gQEAgLTE1NCw2ICsxNTQsNyBAQA0KPiAgCXNkaGNA
MmUwMDAgew0KPiAgCQljb21wYXRpYmxlID0gImZzbCxwMTAyMC1lc2RoYyIsICJmc2wsZXNkaGMi
Ow0KPiAgCQlzZGhjaSxhdXRvLWNtZDEyOw0KPiArCQlzZGhjaSxuby1jbWQyMzsNCj4gIAl9Ow0K
PiAgL2luY2x1ZGUvICJwcTMtc2VjMy4zLTAuZHRzaSINCj4gDQo+IGRpZmYgLS1naXQgYS9hcmNo
L3Bvd2VycGMvYm9vdC9kdHMvZnNsL3AxMDIxc2ktcG9zdC5kdHNpDQo+IGIvYXJjaC9wb3dlcnBj
L2Jvb3QvZHRzL2ZzbC9wMTAyMXNpLXBvc3QuZHRzaQ0KPiBpbmRleCBhZGI4MmZkLi4yYjdmZDJh
IDEwMDY0NA0KPiAtLS0gYS9hcmNoL3Bvd2VycGMvYm9vdC9kdHMvZnNsL3AxMDIxc2ktcG9zdC5k
dHNpDQo+ICsrKyBiL2FyY2gvcG93ZXJwYy9ib290L2R0cy9mc2wvcDEwMjFzaS1wb3N0LmR0c2kN
Cj4gQEAgLTE0OSw2ICsxNDksNyBAQA0KPiAgL2luY2x1ZGUvICJwcTMtZXNkaGMtMC5kdHNpIg0K
PiAgCXNkaGNAMmUwMDAgew0KPiAgCQlzZGhjaSxhdXRvLWNtZDEyOw0KPiArCQlzZGhjaSxuby1j
bWQyMzsNCj4gIAl9Ow0KPiANCj4gIC9pbmNsdWRlLyAicHEzLXNlYzMuMy0wLmR0c2kiDQo+IGRp
ZmYgLS1naXQgYS9hcmNoL3Bvd2VycGMvYm9vdC9kdHMvZnNsL3AxMDIyc2ktcG9zdC5kdHNpDQo+
IGIvYXJjaC9wb3dlcnBjL2Jvb3QvZHRzL2ZzbC9wMTAyMnNpLXBvc3QuZHRzaQ0KPiBpbmRleCAw
NjIxNmI4Li4yMzM0YTUyIDEwMDY0NA0KPiAtLS0gYS9hcmNoL3Bvd2VycGMvYm9vdC9kdHMvZnNs
L3AxMDIyc2ktcG9zdC5kdHNpDQo+ICsrKyBiL2FyY2gvcG93ZXJwYy9ib290L2R0cy9mc2wvcDEw
MjJzaS1wb3N0LmR0c2kNCj4gQEAgLTIxNSw2ICsyMTUsNyBAQA0KPiAgCXNkaGNAMmUwMDAgew0K
PiAgCQljb21wYXRpYmxlID0gImZzbCxwMTAyMi1lc2RoYyIsICJmc2wsZXNkaGMiOw0KPiAgCQlz
ZGhjaSxhdXRvLWNtZDEyOw0KPiArCQlzZGhjaSxuby1jbWQyMzsNCj4gIAl9Ow0KPiANCj4gIC9p
bmNsdWRlLyAicHEzLXNlYzMuMy0wLmR0c2kiDQo+IGRpZmYgLS1naXQgYS9hcmNoL3Bvd2VycGMv
Ym9vdC9kdHMvZnNsL3A0MDgwc2ktcG9zdC5kdHNpDQo+IGIvYXJjaC9wb3dlcnBjL2Jvb3QvZHRz
L2ZzbC9wNDA4MHNpLXBvc3QuZHRzaQ0KPiBpbmRleCA4ZDM1ZDJjLi41YjM5OTUyIDEwMDY0NA0K
PiAtLS0gYS9hcmNoL3Bvd2VycGMvYm9vdC9kdHMvZnNsL3A0MDgwc2ktcG9zdC5kdHNpDQo+ICsr
KyBiL2FyY2gvcG93ZXJwYy9ib290L2R0cy9mc2wvcDQwODBzaS1wb3N0LmR0c2kNCj4gQEAgLTMz
Nyw2ICszMzcsNyBAQA0KPiAgCXNkaGNAMTE0MDAwIHsNCj4gIAkJdm9sdGFnZS1yYW5nZXMgPSA8
MzMwMCAzMzAwPjsNCj4gIAkJc2RoY2ksYXV0by1jbWQxMjsNCj4gKwkJc2RoY2ksbm8tY21kMjM7
DQo+ICAJfTsNCj4gDQo+ICAvaW5jbHVkZS8gInFvcmlxLWkyYy0wLmR0c2kiDQo+IC0tDQo+IDEu
Ny45LjUNCg0K

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Kumar Gala]

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.
>=20
> 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

>=20
>=20
>> -----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
>>=20
>> 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>
>>>>=20
>>>> Below SOCs don't support the cmd23 command for MMC card, therefore,
>>>> disable it in device tree:
>>>> P1020, P1021, P1022, P1024, P1025 and P4080
>>>>=20
>>>> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
>>>=20
>>> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
>>=20
>> Btw, although the patch is trivial, I guess you still want to let =
know
>> PowerPC folks about it. Adding Cc and copying the patch:
>>=20
>> - - - -
>> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>>=20
>> Below SOCs don't support the cmd23 command for MMC card, therefore,
>> disable it in device tree:
>> P1020, P1021, P1022, P1024, P1025 and P4080
>>=20
>> 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(+)
>>=20
>> 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 =3D "fsl,p1020-esdhc", "fsl,esdhc";
>> 		sdhci,auto-cmd12;
>> +		sdhci,no-cmd23;
>> 	};
>> /include/ "pq3-sec3.3-0.dtsi"
>>=20
>> 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;
>> 	};
>>=20
>> /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 =3D "fsl,p1022-esdhc", "fsl,esdhc";
>> 		sdhci,auto-cmd12;
>> +		sdhci,no-cmd23;
>> 	};
>>=20
>> /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 =3D <3300 3300>;
>> 		sdhci,auto-cmd12;
>> +		sdhci,no-cmd23;
>> 	};
>>=20
>> /include/ "qoriq-i2c-0.dtsi"
>> --
>> 1.7.9.5
>=20

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Kumar Gala]

In sdhci_add_host()

We have the following

...
        mmc->caps |=3D MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | MMC_CAP_CMD23;

        if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12)
                host->flags |=3D SDHCI_AUTO_CMD12;

        /* Auto-CMD23 stuff only works in ADMA or PIO. */
        if ((host->version >=3D SDHCI_SPEC_300) &&
            ((host->flags & SDHCI_USE_ADMA) ||
             !(host->flags & SDHCI_USE_SDMA))) {
                host->flags |=3D 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=

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Chris Ball]

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.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Scott Wood]

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

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Kumar Gala]

On Sep 11, 2012, at 9:44 AM, Chris Ball wrote:

> Hi,
>=20
> On Tue, Sep 11 2012, Kumar Gala wrote:
>> In sdhci_add_host()
>>=20
>> We have the following
>>=20
>> ...
>>        mmc->caps |=3D MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE | =
MMC_CAP_CMD23;
>>=20
>>        if (host->quirks & SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12)
>>                host->flags |=3D SDHCI_AUTO_CMD12;
>>=20
>>        /* Auto-CMD23 stuff only works in ADMA or PIO. */
>>        if ((host->version >=3D SDHCI_SPEC_300) &&
>>            ((host->flags & SDHCI_USE_ADMA) ||
>>             !(host->flags & SDHCI_USE_SDMA))) {
>>                host->flags |=3D SDHCI_AUTO_CMD23;
>>                DBG("%s: Auto-CMD23 available\n", mmc_hostname(mmc));
>>        } else {
>>                DBG("%s: Auto-CMD23 unavailable\n", =
mmc_hostname(mmc));
>>        }
>>=20
>> ...
>>=20
>> 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?
>=20
> The main answer is:  No, because CMD23 is distinct from Auto-CMD23.
>=20
> Multiblock transfers (CMD23) date back from MMC cards (which is why
> they're an MMC host capability) and can also be used in SDHCI.
>=20
> 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.
>=20
> 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.
>=20
> 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=

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Chris Ball]

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.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

RE: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Huang Changming-R66093]

DQoNCkJlc3QgUmVnYXJkcw0KSmVycnkgSHVhbmcNCg0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2Fn
ZS0tLS0tDQo+IEZyb206IFdvb2QgU2NvdHQtQjA3NDIxDQo+IFNlbnQ6IFdlZG5lc2RheSwgU2Vw
dGVtYmVyIDEyLCAyMDEyIDI6MjggQU0NCj4gVG86IEFudG9uIFZvcm9udHNvdg0KPiBDYzogSHVh
bmcgQ2hhbmdtaW5nLVI2NjA5MzsgbGludXhwcGMtZGV2QGxpc3RzLm96bGFicy5vcmc7IGxpbnV4
LQ0KPiBtbWNAdmdlci5rZXJuZWwub3JnDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggMi8zXSBwb3dl
cnBjL2VzZGhjOiBhZGQgcHJvcGVydHkgdG8gZGlzYWJsZSB0aGUgQ01EMjMNCj4gDQo+IE9uIDA5
LzExLzIwMTIgMDM6MDQgQU0sIEFudG9uIFZvcm9udHNvdiB3cm90ZToNCj4gPiBPbiBUdWUsIFNl
cCAxMSwgMjAxMiBhdCAxMjo1NDoyOUFNIC0wNzAwLCBBbnRvbiBWb3JvbnRzb3Ygd3JvdGU6DQo+
ID4+IE9uIFR1ZSwgU2VwIDExLCAyMDEyIGF0IDAzOjEyOjQ0UE0gKzA4MDAsIENoYW5nLQ0KPiBN
aW5nLkh1YW5nQGZyZWVzY2FsZS5jb20gd3JvdGU6DQo+ID4+PiBGcm9tOiBKZXJyeSBIdWFuZyA8
Q2hhbmctTWluZy5IdWFuZ0BmcmVlc2NhbGUuY29tPg0KPiA+Pj4NCj4gPj4+IEJlbG93IFNPQ3Mg
ZG9uJ3Qgc3VwcG9ydCB0aGUgY21kMjMgY29tbWFuZCBmb3IgTU1DIGNhcmQsIHRoZXJlZm9yZSwN
Cj4gPj4+IGRpc2FibGUgaXQgaW4gZGV2aWNlIHRyZWU6DQo+ID4+PiBQMTAyMCwgUDEwMjEsIFAx
MDIyLCBQMTAyNCwgUDEwMjUgYW5kIFA0MDgwDQo+ID4+Pg0KPiA+Pj4gU2lnbmVkLW9mZi1ieTog
SmVycnkgSHVhbmcgPENoYW5nLU1pbmcuSHVhbmdAZnJlZXNjYWxlLmNvbT4NCj4gPj4NCj4gPj4g
QWNrZWQtYnk6IEFudG9uIFZvcm9udHNvdiA8Y2JvdWF0bWFpbHJ1QGdtYWlsLmNvbT4NCj4gPg0K
PiA+IEJ0dywgYWx0aG91Z2ggdGhlIHBhdGNoIGlzIHRyaXZpYWwsIEkgZ3Vlc3MgeW91IHN0aWxs
IHdhbnQgdG8gbGV0IGtub3cNCj4gPiBQb3dlclBDIGZvbGtzIGFib3V0IGl0LiBBZGRpbmcgQ2Mg
YW5kIGNvcHlpbmcgdGhlIHBhdGNoOg0KPiA+DQo+ID4gLSAtIC0gLQ0KPiA+IEZyb206IEplcnJ5
IEh1YW5nIDxDaGFuZy1NaW5nLkh1YW5nQGZyZWVzY2FsZS5jb20+DQo+ID4NCj4gPiBCZWxvdyBT
T0NzIGRvbid0IHN1cHBvcnQgdGhlIGNtZDIzIGNvbW1hbmQgZm9yIE1NQyBjYXJkLCB0aGVyZWZv
cmUsDQo+ID4gZGlzYWJsZSBpdCBpbiBkZXZpY2UgdHJlZToNCj4gPiBQMTAyMCwgUDEwMjEsIFAx
MDIyLCBQMTAyNCwgUDEwMjUgYW5kIFA0MDgwDQo+ID4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBKZXJy
eSBIdWFuZyA8Q2hhbmctTWluZy5IdWFuZ0BmcmVlc2NhbGUuY29tPg0KPiA+IENDOiBBbnRvbiBW
b3JvbnRzb3YgPGNib3VhdG1haWxydUBnbWFpbC5jb20+DQo+ID4gLS0tDQo+ID4gIGFyY2gvcG93
ZXJwYy9ib290L2R0cy9mc2wvcDEwMjBzaS1wb3N0LmR0c2kgfCAgICAxICsNCj4gPiAgYXJjaC9w
b3dlcnBjL2Jvb3QvZHRzL2ZzbC9wMTAyMXNpLXBvc3QuZHRzaSB8ICAgIDEgKw0KPiA+ICBhcmNo
L3Bvd2VycGMvYm9vdC9kdHMvZnNsL3AxMDIyc2ktcG9zdC5kdHNpIHwgICAgMSArDQo+ID4gIGFy
Y2gvcG93ZXJwYy9ib290L2R0cy9mc2wvcDQwODBzaS1wb3N0LmR0c2kgfCAgICAxICsNCj4gPiAg
NCBmaWxlcyBjaGFuZ2VkLCA0IGluc2VydGlvbnMoKykNCj4gPg0KPiA+IGRpZmYgLS1naXQgYS9h
cmNoL3Bvd2VycGMvYm9vdC9kdHMvZnNsL3AxMDIwc2ktcG9zdC5kdHNpDQo+ID4gYi9hcmNoL3Bv
d2VycGMvYm9vdC9kdHMvZnNsL3AxMDIwc2ktcG9zdC5kdHNpDQo+ID4gaW5kZXggNjhjYzVlNy4u
NzkzYTMwYiAxMDA2NDQNCj4gPiAtLS0gYS9hcmNoL3Bvd2VycGMvYm9vdC9kdHMvZnNsL3AxMDIw
c2ktcG9zdC5kdHNpDQo+ID4gKysrIGIvYXJjaC9wb3dlcnBjL2Jvb3QvZHRzL2ZzbC9wMTAyMHNp
LXBvc3QuZHRzaQ0KPiA+IEBAIC0xNTQsNiArMTU0LDcgQEANCj4gPiAgCXNkaGNAMmUwMDAgew0K
PiA+ICAJCWNvbXBhdGlibGUgPSAiZnNsLHAxMDIwLWVzZGhjIiwgImZzbCxlc2RoYyI7DQo+ID4g
IAkJc2RoY2ksYXV0by1jbWQxMjsNCj4gPiArCQlzZGhjaSxuby1jbWQyMzsNCj4gPiAgCX07DQo+
ID4gIC9pbmNsdWRlLyAicHEzLXNlYzMuMy0wLmR0c2kiDQo+ID4NCj4gPiBkaWZmIC0tZ2l0IGEv
YXJjaC9wb3dlcnBjL2Jvb3QvZHRzL2ZzbC9wMTAyMXNpLXBvc3QuZHRzaQ0KPiA+IGIvYXJjaC9w
b3dlcnBjL2Jvb3QvZHRzL2ZzbC9wMTAyMXNpLXBvc3QuZHRzaQ0KPiA+IGluZGV4IGFkYjgyZmQu
LjJiN2ZkMmEgMTAwNjQ0DQo+ID4gLS0tIGEvYXJjaC9wb3dlcnBjL2Jvb3QvZHRzL2ZzbC9wMTAy
MXNpLXBvc3QuZHRzaQ0KPiA+ICsrKyBiL2FyY2gvcG93ZXJwYy9ib290L2R0cy9mc2wvcDEwMjFz
aS1wb3N0LmR0c2kNCj4gPiBAQCAtMTQ5LDYgKzE0OSw3IEBADQo+ID4gIC9pbmNsdWRlLyAicHEz
LWVzZGhjLTAuZHRzaSINCj4gPiAgCXNkaGNAMmUwMDAgew0KPiA+ICAJCXNkaGNpLGF1dG8tY21k
MTI7DQo+ID4gKwkJc2RoY2ksbm8tY21kMjM7DQo+ID4gIAl9Ow0KPiA+DQo+ID4gIC9pbmNsdWRl
LyAicHEzLXNlYzMuMy0wLmR0c2kiDQo+ID4gZGlmZiAtLWdpdCBhL2FyY2gvcG93ZXJwYy9ib290
L2R0cy9mc2wvcDEwMjJzaS1wb3N0LmR0c2kNCj4gPiBiL2FyY2gvcG93ZXJwYy9ib290L2R0cy9m
c2wvcDEwMjJzaS1wb3N0LmR0c2kNCj4gPiBpbmRleCAwNjIxNmI4Li4yMzM0YTUyIDEwMDY0NA0K
PiA+IC0tLSBhL2FyY2gvcG93ZXJwYy9ib290L2R0cy9mc2wvcDEwMjJzaS1wb3N0LmR0c2kNCj4g
PiArKysgYi9hcmNoL3Bvd2VycGMvYm9vdC9kdHMvZnNsL3AxMDIyc2ktcG9zdC5kdHNpDQo+ID4g
QEAgLTIxNSw2ICsyMTUsNyBAQA0KPiA+ICAJc2RoY0AyZTAwMCB7DQo+ID4gIAkJY29tcGF0aWJs
ZSA9ICJmc2wscDEwMjItZXNkaGMiLCAiZnNsLGVzZGhjIjsNCj4gPiAgCQlzZGhjaSxhdXRvLWNt
ZDEyOw0KPiA+ICsJCXNkaGNpLG5vLWNtZDIzOw0KPiA+ICAJfTsNCj4gPg0KPiA+ICAvaW5jbHVk
ZS8gInBxMy1zZWMzLjMtMC5kdHNpIg0KPiA+IGRpZmYgLS1naXQgYS9hcmNoL3Bvd2VycGMvYm9v
dC9kdHMvZnNsL3A0MDgwc2ktcG9zdC5kdHNpDQo+ID4gYi9hcmNoL3Bvd2VycGMvYm9vdC9kdHMv
ZnNsL3A0MDgwc2ktcG9zdC5kdHNpDQo+ID4gaW5kZXggOGQzNWQyYy4uNWIzOTk1MiAxMDA2NDQN
Cj4gPiAtLS0gYS9hcmNoL3Bvd2VycGMvYm9vdC9kdHMvZnNsL3A0MDgwc2ktcG9zdC5kdHNpDQo+
ID4gKysrIGIvYXJjaC9wb3dlcnBjL2Jvb3QvZHRzL2ZzbC9wNDA4MHNpLXBvc3QuZHRzaQ0KPiA+
IEBAIC0zMzcsNiArMzM3LDcgQEANCj4gPiAgCXNkaGNAMTE0MDAwIHsNCj4gPiAgCQl2b2x0YWdl
LXJhbmdlcyA9IDwzMzAwIDMzMDA+Ow0KPiA+ICAJCXNkaGNpLGF1dG8tY21kMTI7DQo+ID4gKwkJ
c2RoY2ksbm8tY21kMjM7DQo+ID4gIAl9Ow0KPiA+DQo+ID4gIC9pbmNsdWRlLyAicW9yaXEtaTJj
LTAuZHRzaSINCj4gPg0KPiANCj4gVGhpcyB3b24ndCBoZWxwIHBlb3BsZSB3aXRoIG9sZCBkZXZp
Y2UgdHJlZXMgKGZvcmtlZCBmb3IgYSBjdXN0b20gYm9hcmQsDQo+IHRpZWQgdG8gYW4gb2xkIFUt
Qm9vdCwgZXRjKS4gIFRoZSBkcml2ZXIgc2hvdWxkIGluZmVyIHRoaXMgZnJvbSB0aGUNCj4gY29t
cGF0aWJsZSBzdHJpbmcgb3IgdmVyc2lvbiByZWdpc3RlcnMgKGlkZWFsbHkgYmxvY2stc3BlY2lm
aWMgdmVyc2lvbg0KPiByZWdpc3RlcnMsIGJ1dCBpZiB0aGVzZSBhcmUgYWJzZW50IG9yIGluY29u
Y2x1c2l2ZSwgU1ZSIGNhbiBiZSB1c2VkKS4NCj4gDQoNCkkgZG9uJ3QgdGhpbmsgaXQgaXMgdGhl
IGJlc3Qgd2F5IHRvIGRvIGl0Lg0KRm9yIHRoZSBWVk4yLjIgb3Igb2xkZXIsIHNvbWUgc2lsaWNv
biBzdXBwb3J0IHRoaXMgZmVhdHVyZSAobXBjODUzNiBhbmQgcDIwMjApLCBidXQgb3RoZXIgc2ls
aWNvbmVzIGRvbid0IHN1cHBvcnQgaXQgKGUuZy4gcDQwODAsIHAxMDJ4KS4NClRob3VnaCwgdGhl
IGN1cnJlbnQgcDUvcDQvcDMgaGFzIHN1cHBvcnRlZCB0aGlzIGZlYXR1cmUsIGNhbiB3ZSBzdXJl
IHRoZSBmdXR1cmUgc2lsaWNvbiBzdXBwb3J0IGl0Pw0KU28gSSB0aGluayB0aGUgYmVzdCB3YXkg
aXMgdG8gc3BlY2lmeSBpdCBpbiBkZXZpY2UgdHJlZSBhcyAnc2RoY2ksYXV0by1jbWQxMicNCg==

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Anton Vorontsov]

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.

RE: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Huang Changming-R66093]

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
>=20
> Hi,
>=20
> 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).
>=20
> 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.
>=20

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, w=
hich will cause I/O error.

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Kumar Gala]

On Sep 12, 2012, at 1:18 AM, Huang Changming-R66093 wrote:

>=20
>=20
> Best Regards
> Jerry Huang
>=20
>=20
>> -----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
>>=20
>> Hi,
>>=20
>> 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?
>>>=20
>>> 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).
>>=20
>> 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.
>>=20
>=20
> 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=

RE: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Huang Changming-R66093]

> >
> >> -----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.
>=20
> 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.
>=20
P1020, p1021, p1022, p1024, p1015 and p4080 can't support it.
Mpc8536, p2020, and the other current DPAA silicon (e.g. p5020, p3041) supp=
ort it.

RE: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Huang Changming-R66093]

DQoNCkJlc3QgUmVnYXJkcw0KSmVycnkgSHVhbmcNCg0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2Fn
ZS0tLS0tDQo+IEZyb206IEFudG9uIFZvcm9udHNvdiBbbWFpbHRvOmNib3VhdG1haWxydUBnbWFp
bC5jb21dDQo+IFNlbnQ6IFdlZG5lc2RheSwgU2VwdGVtYmVyIDEyLCAyMDEyIDExOjM4IEFNDQo+
IFRvOiBIdWFuZyBDaGFuZ21pbmctUjY2MDkzDQo+IENjOiBXb29kIFNjb3R0LUIwNzQyMTsgbGlu
dXhwcGMtZGV2QGxpc3RzLm96bGFicy5vcmc7IGxpbnV4LQ0KPiBtbWNAdmdlci5rZXJuZWwub3Jn
DQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggMi8zXSBwb3dlcnBjL2VzZGhjOiBhZGQgcHJvcGVydHkg
dG8gZGlzYWJsZSB0aGUgQ01EMjMNCj4gDQo+IE9uIFdlZCwgU2VwIDEyLCAyMDEyIGF0IDAzOjE5
OjE4QU0gKzAwMDAsIEh1YW5nIENoYW5nbWluZy1SNjYwOTMgd3JvdGU6DQo+IFsuLi5dDQo+ID4g
SSBkb24ndCB0aGluayBpdCBpcyB0aGUgYmVzdCB3YXkgdG8gZG8gaXQuICBGb3IgdGhlIFZWTjIu
MiBvciBvbGRlciwNCj4gPiBzb21lIHNpbGljb24gc3VwcG9ydCB0aGlzIGZlYXR1cmUgKG1wYzg1
MzYgYW5kIHAyMDIwKSwgYnV0IG90aGVyDQo+ID4gc2lsaWNvbmVzIGRvbid0IHN1cHBvcnQgaXQg
KGUuZy4gcDQwODAsIHAxMDJ4KS4gIFRob3VnaCwgdGhlIGN1cnJlbnQNCj4gPiBwNS9wNC9wMyBo
YXMgc3VwcG9ydGVkIHRoaXMgZmVhdHVyZSwgY2FuIHdlIHN1cmUgdGhlIGZ1dHVyZSBzaWxpY29u
DQo+ID4gc3VwcG9ydCBpdD8gIFNvIEkgdGhpbmsgdGhlIGJlc3Qgd2F5IGlzIHRvIHNwZWNpZnkg
aXQgaW4gZGV2aWNlIHRyZWUNCj4gPiBhcyAnc2RoY2ksYXV0by1jbWQxMicNCj4gDQo+IEluIGFk
ZGl0aW9uIHRvIHlvdXIgY3VycmVudCBwYXRjaGVzLCB5b3UgY291bGQganVzdCBhZGQgYW5vdGhl
ciBwYXRjaA0KPiB0aGF0IGJsYWNrbGlzdHMgYWZmZWN0ZWQgU09DIHJldmlzaW9ucyBiYXNlZCBv
biB0aGUgaW5mbyBmcm9tIFBWUi9TVlIuDQo+IA0KPiBGb3IgZXhhbXBsZSwgc2VlIGdmYXJfZGV0
ZWN0X2VycmF0YSgpIGluDQo+IGRyaXZlcnMvbmV0L2V0aGVybmV0L2ZyZWVzY2FsZS9naWFuZmFy
LmMuDQo+IA0KPiBUaGF0IHdheSB5b3UgY291bGQgaGVscCB1c2VycyB0aGF0IGRvbid0IGhhdmUg
dGhlIG5ld2VzdCBkZXZpY2UgdHJlZXMuDQo+IA0KSGksIEFudG9uDQpUaGFua3MuDQpCdXQsIHRo
ZXNlIHBhdGNoZXMgYXJlIGp1c3QgZm9yIGxhdGVzdCBrZXJuZWwsIGlmIHRoZSB1c2VyIGRvbid0
IGhhdmUgdGhlIGxhdGVzdCBkZXZpY2UgdHJlZSwNClRoZW4gdGhleSBkb24ndCBoYXZlIHRoZSBs
YXRlc3Qga2VybmVsLCBhbmQgdGhlc2UgcGF0Y2hlcyBjYW4ndCBiZSB1c2VkIGZvciB0aGVtIGRp
cmVjdGx5LA0KVGhvdWdoIHByb3ZpZGUgdGhlIGZ1bmN0aW9uIGxpa2UgZ2Zhcl9kZXRlY3RfZXJy
YXRhLCB0aGV5IG11c3QgbW9kaWZ5IHRoZWlyIGNvZGVzLg0KDQpBbmQgSSBkb24ndCBrbm93IGlm
IHRoZSBmdXR1cmUgc2lsaWNvbiBjYW4gc3VwcG9ydCBDTUQyMywgaWYgbm90LCB3ZSBtdXN0IG1v
ZGlmeSBzZGhjIGRyaXZlciBhZ2Fpbi4NCldoZW4gd2UgdXNlIHRoZSBkZXZpY2UgdHJlZSB0byBp
ZGVudGlmeSBpdCwgaWYgdGhlIHNpbGljb24gZG9lcyBub3Qgc3VwcG9ydCB0aGlzIGZlYXR1cmUs
DQp3ZSBqdXN0IGFkZCB0aGlzIHByb3BlcnR5IGludG8gdGhlIG5ldyBkZXZpY2UgdHJlZSwgbm90
IG5lZWQgdG8gbW9kaWZ5IFNESEMgZHJpdmVyLg0KSSB0aGluaywgdGhpcyBpcyB0aGUgYmVzdCB3
YXkuDQogDQoNCg==

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Kumar Gala]

On Sep 12, 2012, at 9:02 PM, Huang Changming-R66093 wrote:

>>>=20
>>>> -----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
>>>>=20
>>>> Hi,
>>>>=20
>>>> 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?
>>>>>=20
>>>>> 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).
>>>>=20
>>>> 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.
>>>>=20
>>>=20
>>> 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.
>>=20
>> 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.
>>=20
> 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 =3D sdhci_readw(host, SDHCI_HOST_VERSION);
vendor_version =3D (vendor_version & SDHCI_VENDOR_VER_MASK) >> =
SDHCI_VENDOR_VER_SHIFT;

if ((vendor_version =3D=3D FSL_SDHC_VER_1_1) || (vendor_version =3D=3D =
FSL_SDHC_VER_2_2))
	host->quirks2 |=3D SDHCI_QUIRK2_HOST_NO_CMD23;

- k

RE: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Huang Changming-R66093]

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
>=20
>=20
> On Sep 12, 2012, at 9:02 PM, Huang Changming-R66093 wrote:
>=20
> >>>
> >>>> -----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.
>=20
> Based on this, why don't we use the HOSTVER register to detect instead of
> device tree:
>=20
>=20
> #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
>=20
> unsigned int vendor_version;
>=20
> vendor_version =3D sdhci_readw(host, SDHCI_HOST_VERSION); vendor_version =
=3D
> (vendor_version & SDHCI_VENDOR_VER_MASK) >> SDHCI_VENDOR_VER_SHIFT;
>=20
> if ((vendor_version =3D=3D FSL_SDHC_VER_1_1) || (vendor_version =3D=3D
> FSL_SDHC_VER_2_2))
> 	host->quirks2 |=3D SDHCI_QUIRK2_HOST_NO_CMD23;
>=20

I once thought about it, but if the future silicon does not support this fe=
ature,
then we continue to modify these codes for new silicon?

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Kumar Gala]

>>>>>=20
>>>>>> -----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
>>>>>>=20
>>>>>> Hi,
>>>>>>=20
>>>>>> 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?
>>>>>>>=20
>>>>>>> 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).
>>>>>>=20
>>>>>> 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.
>>>>>>=20
>>>>>=20
>>>>> 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.
>>>>=20
>>>> 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.
>>>>=20
>>> 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.
>>=20
>> Based on this, why don't we use the HOSTVER register to detect =
instead of
>> device tree:
>>=20
>>=20
>> #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
>>=20
>> unsigned int vendor_version;
>>=20
>> vendor_version =3D sdhci_readw(host, SDHCI_HOST_VERSION); =
vendor_version =3D
>> (vendor_version & SDHCI_VENDOR_VER_MASK) >> SDHCI_VENDOR_VER_SHIFT;
>>=20
>> if ((vendor_version =3D=3D FSL_SDHC_VER_1_1) || (vendor_version =3D=3D
>> FSL_SDHC_VER_2_2))
>> 	host->quirks2 |=3D SDHCI_QUIRK2_HOST_NO_CMD23;
>>=20
>=20
> 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=

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Chris Ball]

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.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Kumar Gala]

On Sep 17, 2012, at 7:36 AM, Chris Ball wrote:

> Hi,
>=20
> 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.
>>>>=20
>>> 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.
>>=20
>> Based on this, why don't we use the HOSTVER register to detect =
instead of device tree:
>=20
> 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=

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Chris Ball]

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.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

RE: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Huang Changming-R66093]

> On Sep 17, 2012, at 7:36 AM, Chris Ball wrote:
>=20
> > 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?
>=20
> Why?  I only ask because I agree with Scott that this means you have to
> update your device tree to get proper functionality.
>=20
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 ef=
fort to modify driver.

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Kumar Gala]

On Sep 17, 2012, at 8:09 PM, Huang Changming-R66093 wrote:

>> On Sep 17, 2012, at 7:36 AM, Chris Ball wrote:
>>=20
>>> Hi,
>>>=20
>>> 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.
>>>>>>=20
>>>>> 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.
>>>>=20
>>>> Based on this, why don't we use the HOSTVER register to detect =
instead
>> of device tree:
>>>=20
>>> 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?
>>=20
>> Why?  I only ask because I agree with Scott that this means you have =
to
>> update your device tree to get proper functionality.
>>=20
> 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.
>=20

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

Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23

[Chris Ball]

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.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child