[RFC] thoughts about recent Samsung related patches

[RFC] thoughts about recent Samsung related patches

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2040 bytes --]

Hi,

while working on the esdhc-controller for the imx-platform, I noticed
the recently comitted changes for Samsung controllers and have some
remarks (sorry for being late):

1) Checking conditions for of host->ops->get_min_clock

The original commit e9510176ff728135383f0cdfc9c90cfe57f9e162 (sdhci: be
more strict with get_min_clock() usage) states why the additional checks
were added. Under this light, it could be argued that commit
cfd1f82f20e0c557a061189f7d8c30d623fbe313 (sdhci: remove useless
set_clock() check) could be reverted and this commit
ce5f036bbbfc6c21d7b55b8fdaa2e2bd56392d94 (sdhci-s3c: add support for the
non standard minimal clock value) could also be reverted if the samsung
platform driver just uses SDHCI_QUIRK_NONSTANDARD_CLOCK without setting
ops->set_clock?

2) 8-Bit data transfer support

The comitted version ae6d6c92212e94b12ab9365c23fb73acc2c3c2e7 (sdhci:
8-bit data transfer width support) looks different from another RFC
posted in February:
http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg01250.html 

As those two already differ, I think it might be wiser to move
8-bit-mode-handling to the platform-specific code? Even the documented
features of a SDHC differ across implementations, I fear side-effects
when using this kind of undocumented feature (official spec says
"reserved" when describing this bit).

3) NO_HI_SPD

Commit 5193250168ccdf87364e35a11965336dc088578c (sdhci: add no hi-speed
bit quirk support) adds a quirk which can be avoided by using
io-accessors like in sdhci-of-esdhc.c. Maybe we can even get rid of
more, older quirks this way to save precious quirk flags. Have to check
that later.

I hope my comments are applicable; because there is no freely available
datasheet, I can't verify all of my assumptions. Looking forward to
comments.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC] thoughts about recent Samsung related patches

[Kyungmin Park]

Hi

On Mon, Sep 6, 2010 at 8:00 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Hi,
>
> while working on the esdhc-controller for the imx-platform, I noticed
> the recently comitted changes for Samsung controllers and have some
> remarks (sorry for being late):
>
> 1) Checking conditions for of host->ops->get_min_clock
>
> The original commit e9510176ff728135383f0cdfc9c90cfe57f9e162 (sdhci: be
> more strict with get_min_clock() usage) states why the additional checks
> were added. Under this light, it could be argued that commit
> cfd1f82f20e0c557a061189f7d8c30d623fbe313 (sdhci: remove useless
> set_clock() check) could be reverted and this commit
> ce5f036bbbfc6c21d7b55b8fdaa2e2bd56392d94 (sdhci-s3c: add support for the
> non standard minimal clock value) could also be reverted if the samsung
> platform driver just uses SDHCI_QUIRK_NONSTANDARD_CLOCK without setting
> ops->set_clock?
In our case we only want to use the min_clock at probe time without
NONSTANDARD_CLOCK. and no need to NONSTANDARD_CLOCK at set_clock since
we need other parts also at sdhci.c
>
> 2) 8-Bit data transfer support
>
> The comitted version ae6d6c92212e94b12ab9365c23fb73acc2c3c2e7 (sdhci:
> 8-bit data transfer width support) looks different from another RFC
> posted in February:
> http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg01250.html
>
> As those two already differ, I think it might be wiser to move
> 8-bit-mode-handling to the platform-specific code? Even the documented
> features of a SDHC differ across implementations, I fear side-effects
> when using this kind of undocumented feature (official spec says
> "reserved" when describing this bit).

Okay it looks different from Samsung Spec.
WIDE8[5]: Extended Data Transfer Width (It is for MMC 8-bit card).
1: 8-bit operatoin
0: Bit width is designated by the bit 1(Data Transfer Width)
So no need to set the BIT 1 and BIT5 simultaneously.
I also wonder other Specs how described it.

>
> 3) NO_HI_SPD
>
> Commit 5193250168ccdf87364e35a11965336dc088578c (sdhci: add no hi-speed
> bit quirk support) adds a quirk which can be avoided by using
> io-accessors like in sdhci-of-esdhc.c. Maybe we can even get rid of
> more, older quirks this way to save precious quirk flags. Have to check
> that later.
>
> I hope my comments are applicable; because there is no freely available
> datasheet, I can't verify all of my assumptions. Looking forward to
> comments.
Good, I think it's possible. I'll try and send a patch.
>
> Kind regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAkyEyesACgkQD27XaX1/VRsm/QCePflpNSE0iQQWSUjf/PY723qK
> ZbEAoLxAIS7jIJY/lUQpQvbkJMN6ve19
> =98xD
> -----END PGP SIGNATURE-----
>
>

Re: [RFC] thoughts about recent Samsung related patches

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]


> > 2) 8-Bit data transfer support
> >
> > The comitted version ae6d6c92212e94b12ab9365c23fb73acc2c3c2e7 (sdhci:
> > 8-bit data transfer width support) looks different from another RFC
> > posted in February:
> > http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg01250.html
> >
> > As those two already differ, I think it might be wiser to move
> > 8-bit-mode-handling to the platform-specific code? Even the documented
> > features of a SDHC differ across implementations, I fear side-effects
> > when using this kind of undocumented feature (official spec says
> > "reserved" when describing this bit).
> 
> Okay it looks different from Samsung Spec.
> WIDE8[5]: Extended Data Transfer Width (It is for MMC 8-bit card).
> 1: 8-bit operatoin
> 0: Bit width is designated by the bit 1(Data Transfer Width)
> So no need to set the BIT 1 and BIT5 simultaneously.
> I also wonder other Specs how described it.

esdhc occupied BIT 2 (originally HI_SPD) for this :(
%10 means 8 bit transfer. %11 is reserved.

> Good, I think it's possible. I'll try and send a patch.

Thanks!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC] thoughts about recent Samsung related patches

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

> > I hope my comments are applicable; because there is no freely available
> > datasheet, I can't verify all of my assumptions. Looking forward to
> > comments.
> Good, I think it's possible. I'll try and send a patch.

Any news on this ?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC] thoughts about recent Samsung related patches

[Kyungmin Park]

On Mon, Sep 13, 2010 at 9:31 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>> > I hope my comments are applicable; because there is no freely available
>> > datasheet, I can't verify all of my assumptions. Looking forward to
>> > comments.
>> Good, I think it's possible. I'll try and send a patch.
>
> Any news on this ?

I tried and no problem to work, but I'm thinking does it really
required for reducing one quirk?
With this change sdhci-s3c should use the
CONFIG_MMC_SDHCI_IO_ACCESSORS and it introduces the io functions for
every io access.

So I'm not sure these change is required for s3c sdhci.

Give you opinions.

Thank you,
Kyungmin Park
>
> Regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAkyOGYwACgkQD27XaX1/VRt27wCgkDJcNU61qyoor1Wzkrce5R/A
> 44YAnRvNMzJ1WbB5IbCgmV+kLuP1+h2C
> =mOsI
> -----END PGP SIGNATURE-----
>
>

Re: cleaning up sdhci? (was Re: [RFC] thoughts about recent Samsung related patches)

[Alan Cox]

> That is, if some implementations of a controller do not adhere to
> that standard and change some bits in the register layout, well, then
> they have to live with the quirks and their performance issues IMHO.

The ATA layer uses I/O accessors although it abstracts them a bit (so
its got 'read status' type abstractions. For performance reasons one or
two places the abstraction is higher level (eg block data transfers)
but with those in place it doesn't seem to be measurable.

> Seeing Alan's patchset, it seems to be a good time to do some
> sdhci-cleanup. My impression is that we could get generalize a few
> quirks by using io-accessors and overriding capabilties-flags. I am

I avoided doing any of it by making I/O access functions fake results
because that seemed to be a recipe for hard to debug future problems.
When dealing with different register layouts or specific bugs it seems
sensible

(and 8250 serial does this succesfully with some devices faking certain
register bits their hardware gets wrong)

> open for other opinions, though. All in all, it would be great if we
> all could combine our efforts and get some kind of master-plan :)
> Will take a look at Alan's patches now...

If there is going to be a grand clean up my preference long term would
be that there is no

	if (host->ops->foo)
		host->ops->foo(...)
	else {
		do foo
	
	}

but that host->ops gets all the NULL entries filled in and the core
code simply calls host->ops->xxx in each case.

Alan

cleaning up sdhci? (was Re: [RFC] thoughts about recent Samsung related patches)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1998 bytes --]

On Tue, Sep 14, 2010 at 09:05:30AM +0900, Kyungmin Park wrote:
> On Mon, Sep 13, 2010 at 9:31 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> >> > I hope my comments are applicable; because there is no freely available
> >> > datasheet, I can't verify all of my assumptions. Looking forward to
> >> > comments.
> >> Good, I think it's possible. I'll try and send a patch.
> >
> > Any news on this ?
> 
> I tried and no problem to work, but I'm thinking does it really
> required for reducing one quirk?
> With this change sdhci-s3c should use the
> CONFIG_MMC_SDHCI_IO_ACCESSORS and it introduces the io functions for
> every io access.
> 
> So I'm not sure these change is required for s3c sdhci.
> 
> Give you opinions.

I know what you mean. Still, I'd prefer to see that sdhci.c implements
stuff as generic as possible and trying to follow the standard closely.
Exceptions to that should be handled elsewhere if possible. That is, if
some implementations of a controller do not adhere to that standard and
change some bits in the register layout, well, then they have to live
with the quirks and their performance issues IMHO.

BTW it is not only the HI_SPD issue, which is a candidate for the io-
accessors. I am also interested to get the 8BITBUS patch reverted. It
breaks some SD cards on some boards using the esdhc controller :(
Support for a 8bit-bus needs a bit more thought, it seems...

Seeing Alan's patchset, it seems to be a good time to do some
sdhci-cleanup. My impression is that we could get generalize a few
quirks by using io-accessors and overriding capabilties-flags. I am open
for other opinions, though. All in all, it would be great if we all
could combine our efforts and get some kind of master-plan :)
Will take a look at Alan's patches now...

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]