diff mbox

[1/2] mmc: add Device Tree properties for UHS modes

Message ID Pine.LNX.4.64.1307261747210.22137@axis700.grange (mailing list archive)
State Changes Requested
Headers show

Commit Message

Guennadi Liakhovetski July 26, 2013, 3:51 p.m. UTC
Add DT properties for UHS SDR12, SDR25, SDR50, SDR104 and DDR50 modes and
for supported by the host in DDR mode VccQ values. Adding them to DT will
automatically enable respective MMC host capabilities.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
---
 Documentation/devicetree/bindings/mmc/mmc.txt |    7 +++++++
 drivers/mmc/core/host.c                       |   14 ++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

Comments

Stephen Warren July 26, 2013, 5:10 p.m. UTC | #1
On 07/26/2013 09:51 AM, Guennadi Liakhovetski wrote:
> Add DT properties for UHS SDR12, SDR25, SDR50, SDR104 and DDR50 modes and
> for supported by the host in DDR mode VccQ values. Adding them to DT will
> automatically enable respective MMC host capabilities.

> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt

> +- uhs-sdr12: the host supports UHS SDR12 mode
> +- uhs-sdr25: the host supports UHS SDR25 mode
> +- uhs-sdr50: the host supports UHS SDR50 mode
> +- uhs-sdr104: the host supports UHS SDR104 mode
> +- uhs-ddr50: the host supports UHS DDR50 mode
> +- ddr-1v2: the host can support DDR, using 1.2V VccQ
> +- ddr-1v8: the host can support DDR, using 1.8V VccQ

Surely the driver for the host controller already knows this, so there's
no need to represent it in DT?
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski July 26, 2013, 8:23 p.m. UTC | #2
Hi Stephen

On Fri, 26 Jul 2013, Stephen Warren wrote:

> On 07/26/2013 09:51 AM, Guennadi Liakhovetski wrote:
> > Add DT properties for UHS SDR12, SDR25, SDR50, SDR104 and DDR50 modes and
> > for supported by the host in DDR mode VccQ values. Adding them to DT will
> > automatically enable respective MMC host capabilities.
> 
> > diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> 
> > +- uhs-sdr12: the host supports UHS SDR12 mode
> > +- uhs-sdr25: the host supports UHS SDR25 mode
> > +- uhs-sdr50: the host supports UHS SDR50 mode
> > +- uhs-sdr104: the host supports UHS SDR104 mode
> > +- uhs-ddr50: the host supports UHS DDR50 mode
> > +- ddr-1v2: the host can support DDR, using 1.2V VccQ
> > +- ddr-1v8: the host can support DDR, using 1.8V VccQ
> 
> Surely the driver for the host controller already knows this, so there's
> no need to represent it in DT?

Not necessarily. One driver can handle several versions of the same chip, 
and some versions of the chip implement some of those features, others 
don't. So, your choice is really whether to specify a chip version in the 
compatible string or these properties. Now, when you consider that 
multiple drivers have to decide upon those, and sometimes you don't have 
an exact IP version of the SD/MMC controller but only the SoC version, you 
choice becomes: either _standard_ _common_ properties as above, or 
compatibility strings virtually in each driver for each SoC version. 
That's why I decided to use explicit properties for those. Example: 
sh_mmcif driver supports MMCIF IP blocks on various Renesas sh-, r-mobile 
and r-car SuperH and ARM SoCs. On some of them DDR50 is supported, on 
others it isn't.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 26, 2013, 9:36 p.m. UTC | #3
On 07/26/2013 02:23 PM, Guennadi Liakhovetski wrote:
> Hi Stephen
> 
> On Fri, 26 Jul 2013, Stephen Warren wrote:
> 
>> On 07/26/2013 09:51 AM, Guennadi Liakhovetski wrote:
>>> Add DT properties for UHS SDR12, SDR25, SDR50, SDR104 and DDR50 modes and
>>> for supported by the host in DDR mode VccQ values. Adding them to DT will
>>> automatically enable respective MMC host capabilities.
>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
>>
>>> +- uhs-sdr12: the host supports UHS SDR12 mode
>>> +- uhs-sdr25: the host supports UHS SDR25 mode
>>> +- uhs-sdr50: the host supports UHS SDR50 mode
>>> +- uhs-sdr104: the host supports UHS SDR104 mode
>>> +- uhs-ddr50: the host supports UHS DDR50 mode
>>> +- ddr-1v2: the host can support DDR, using 1.2V VccQ
>>> +- ddr-1v8: the host can support DDR, using 1.8V VccQ
>>
>> Surely the driver for the host controller already knows this, so there's
>> no need to represent it in DT?
> 
> Not necessarily. One driver can handle several versions of the same chip, 
> and some versions of the chip implement some of those features, others 
> don't.

Certainly.

> So, your choice is really whether to specify a chip version in the
> compatible string or these properties.

There's no choice there. The compatible property needs to specify all of
the following:

* A value which describes the exact chip the IP block is in. This can be
matched on to enable any quirks that need to be handled due to
integration of the IP into the particular chip. This value will list the
chip version. An example might be tegra20-sdhci.

* A value which describes the IP block version (if that IP block has a
version independent of the SoC that contains it, as e.g. a Synopsis IP
block might). A totally made-up example might be synopsis-dwc2-1.0.0

* Various more generic values that describe the HW, and that define a n
interface to the HW that is specific and complete enough that HW can
program to. An example might be usb-ehci (assuming a chip that doesn't
have so many quirks that a driver has to know more than just "it's an
EHCI controller).

Further classes of compatible value might exist, but you get the idea.

All of those values have to exist in the DT right from the very start,
so that the first DT is usable with a future kernel, which might decide
to vary the exact compatible value(s) it matches on, provided they're
all documented in the DT binding ABI, in order to enable/disable new
sets of quirsk.

> Now, when you consider that
> multiple drivers have to decide upon those, and sometimes you don't have 
> an exact IP version of the SD/MMC controller but only the SoC version, you 
> choice becomes: 

That would be a bug in the DT, given my assertions above.

> either _standard_ _common_ properties as above, or 
> compatibility strings virtually in each driver for each SoC version.

My preference would certainly be to derive the data from compatible
strings here. I'd prefer more that DT represent board differences rather
than SoC differences; drivers can encompass the SoC differences with
minimal code in general.

Related, is it really true that zero driver involvement is required to
enable these UHS features? If absolutely any HW can enable them without
any driver support at all, then perhaps it's still reasonable to create
DT properties to enable them. However, if driver support is required to
make those features actually work, the driver had better be validating
that support actually works on the HW it's running on before enabling
the feature (and can therefore pass the validation results to the SD
core rather than relying on DT properties to be set). I'm pretty sure
that our downstream Tegra SDHCI driver contains a lot of code to make
UHS actually work, even though the HW does support it, and hence with
this patch applied, the DT would request that it be enabled.

> That's why I decided to use explicit properties for those. Example: 
> sh_mmcif driver supports MMCIF IP blocks on various Renesas sh-, r-mobile 
> and r-car SuperH and ARM SoCs. On some of them DDR50 is supported, on 
> others it isn't.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski July 29, 2013, 6:20 a.m. UTC | #4
Hi Stephen

On Fri, 26 Jul 2013, Stephen Warren wrote:

> On 07/26/2013 02:23 PM, Guennadi Liakhovetski wrote:
> > Hi Stephen
> > 
> > On Fri, 26 Jul 2013, Stephen Warren wrote:
> > 
> >> On 07/26/2013 09:51 AM, Guennadi Liakhovetski wrote:
> >>> Add DT properties for UHS SDR12, SDR25, SDR50, SDR104 and DDR50 modes and
> >>> for supported by the host in DDR mode VccQ values. Adding them to DT will
> >>> automatically enable respective MMC host capabilities.
> >>
> >>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> >>
> >>> +- uhs-sdr12: the host supports UHS SDR12 mode
> >>> +- uhs-sdr25: the host supports UHS SDR25 mode
> >>> +- uhs-sdr50: the host supports UHS SDR50 mode
> >>> +- uhs-sdr104: the host supports UHS SDR104 mode
> >>> +- uhs-ddr50: the host supports UHS DDR50 mode
> >>> +- ddr-1v2: the host can support DDR, using 1.2V VccQ
> >>> +- ddr-1v8: the host can support DDR, using 1.8V VccQ
> >>
> >> Surely the driver for the host controller already knows this, so there's
> >> no need to represent it in DT?
> > 
> > Not necessarily. One driver can handle several versions of the same chip, 
> > and some versions of the chip implement some of those features, others 
> > don't.
> 
> Certainly.
> 
> > So, your choice is really whether to specify a chip version in the
> > compatible string or these properties.
> 
> There's no choice there. The compatible property needs to specify all of
> the following:
> 
> * A value which describes the exact chip the IP block is in. This can be
> matched on to enable any quirks that need to be handled due to
> integration of the IP into the particular chip. This value will list the
> chip version. An example might be tegra20-sdhci.
> 
> * A value which describes the IP block version (if that IP block has a
> version independent of the SoC that contains it, as e.g. a Synopsis IP
> block might). A totally made-up example might be synopsis-dwc2-1.0.0
> 
> * Various more generic values that describe the HW, and that define a n
> interface to the HW that is specific and complete enough that HW can
> program to. An example might be usb-ehci (assuming a chip that doesn't
> have so many quirks that a driver has to know more than just "it's an
> EHCI controller).

Yes, all these certainly make sense. As far as I understand, we are still 
in the process of defining good clear rules for DTs, there is an "ABI" 
discussion currently running on ALKML and IIRC this is also going to be a 
topic for one of coming conferences. So, hopefully we're approaching a 
state of a greater clarity about DT.

> Further classes of compatible value might exist, but you get the idea.
> 
> All of those values have to exist in the DT right from the very start,
> so that the first DT is usable with a future kernel, which might decide
> to vary the exact compatible value(s) it matches on, provided they're
> all documented in the DT binding ABI, in order to enable/disable new
> sets of quirsk.

Makes sense too, sure.

> > Now, when you consider that
> > multiple drivers have to decide upon those, and sometimes you don't have 
> > an exact IP version of the SD/MMC controller but only the SoC version, you 
> > choice becomes: 
> 
> That would be a bug in the DT, given my assertions above.

Sorry, what exactly would be a bug? Not having an exact IP version? But 
you did write above - "if that IP block has a version independent of the 
SoC that contains it." In my case it doesn't really. Those IPs only exist 
within those SoCs.

> > either _standard_ _common_ properties as above, or 
> > compatibility strings virtually in each driver for each SoC version.
> 
> My preference would certainly be to derive the data from compatible
> strings here. I'd prefer more that DT represent board differences rather
> than SoC differences; drivers can encompass the SoC differences with
> minimal code in general.

In general - yes, I fully agree. I wasn't sure about this specific case, 
exactly because those are standard properties, that, if implemented as DT 
properties, can be trivially re-used by all drivers with 0 additional 
code.

> Related, is it really true that zero driver involvement is required to
> enable these UHS features?

I think it is, in most cases at least.

> If absolutely any HW can enable them without
> any driver support at all, then perhaps it's still reasonable to create
> DT properties to enable them. However, if driver support is required to
> make those features actually work, the driver had better be validating
> that support actually works on the HW it's running on before enabling
> the feature (and can therefore pass the validation results to the SD
> core rather than relying on DT properties to be set).

I think it rather works the other way round. Those flags only mark 
hardware _capabilities_. But don't tell the driver to use them yet. 
Normally drivers just collect them in their .caps and .caps2 fields and 
pass on to the mmc core. The mmc core then card detection and querying and 
if a card and the host interface to it are suitable for certain features, 
marked in those capability fields, the core will then ask the host driver 
to enable it. Of course, I don't know all drivers some drivers might well 
behave differently, but even then they anyway are the first to find out 
about those capabilities. After calling mmc_of_parse() drivers get those 
capabilities back and can do whatever needs to be done to validate them, 
and can even clear them again, if they find out, that some of them aren't 
really available. mmc_of_parse() just makes driver's work a bit easier, 
but doesn't take control away from it.

> I'm pretty sure
> that our downstream Tegra SDHCI driver contains a lot of code to make
> UHS actually work, even though the HW does support it, and hence with
> this patch applied, the DT would request that it be enabled.
> 
> > That's why I decided to use explicit properties for those. Example: 
> > sh_mmcif driver supports MMCIF IP blocks on various Renesas sh-, r-mobile 
> > and r-car SuperH and ARM SoCs. On some of them DDR50 is supported, on 
> > others it isn't.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski July 29, 2013, 7:18 a.m. UTC | #5
On Mon, 29 Jul 2013, Guennadi Liakhovetski wrote:

> Hi Stephen
> 
> On Fri, 26 Jul 2013, Stephen Warren wrote:
> 
> > On 07/26/2013 02:23 PM, Guennadi Liakhovetski wrote:
> > > Hi Stephen
> > > 
> > > On Fri, 26 Jul 2013, Stephen Warren wrote:
> > > 
> > >> On 07/26/2013 09:51 AM, Guennadi Liakhovetski wrote:
> > >>> Add DT properties for UHS SDR12, SDR25, SDR50, SDR104 and DDR50 modes and
> > >>> for supported by the host in DDR mode VccQ values. Adding them to DT will
> > >>> automatically enable respective MMC host capabilities.
> > >>
> > >>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
> > >>
> > >>> +- uhs-sdr12: the host supports UHS SDR12 mode
> > >>> +- uhs-sdr25: the host supports UHS SDR25 mode
> > >>> +- uhs-sdr50: the host supports UHS SDR50 mode
> > >>> +- uhs-sdr104: the host supports UHS SDR104 mode
> > >>> +- uhs-ddr50: the host supports UHS DDR50 mode
> > >>> +- ddr-1v2: the host can support DDR, using 1.2V VccQ
> > >>> +- ddr-1v8: the host can support DDR, using 1.8V VccQ
> > >>
> > >> Surely the driver for the host controller already knows this, so there's
> > >> no need to represent it in DT?
> > > 
> > > Not necessarily. One driver can handle several versions of the same chip, 
> > > and some versions of the chip implement some of those features, others 
> > > don't.
> > 
> > Certainly.
> > 
> > > So, your choice is really whether to specify a chip version in the
> > > compatible string or these properties.
> > 
> > There's no choice there. The compatible property needs to specify all of
> > the following:
> > 
> > * A value which describes the exact chip the IP block is in. This can be
> > matched on to enable any quirks that need to be handled due to
> > integration of the IP into the particular chip. This value will list the
> > chip version. An example might be tegra20-sdhci.
> > 
> > * A value which describes the IP block version (if that IP block has a
> > version independent of the SoC that contains it, as e.g. a Synopsis IP
> > block might). A totally made-up example might be synopsis-dwc2-1.0.0
> > 
> > * Various more generic values that describe the HW, and that define a n
> > interface to the HW that is specific and complete enough that HW can
> > program to. An example might be usb-ehci (assuming a chip that doesn't
> > have so many quirks that a driver has to know more than just "it's an
> > EHCI controller).
> 
> Yes, all these certainly make sense. As far as I understand, we are still 
> in the process of defining good clear rules for DTs, there is an "ABI" 
> discussion currently running on ALKML and IIRC this is also going to be a 
> topic for one of coming conferences. So, hopefully we're approaching a 
> state of a greater clarity about DT.

A short addendum. At least with Renesas SoCs I see the situation in the 
following way: new SoC versions appear relatively frequently. They often 
re-use multiple IP blocks, drivers for which are scattered across the 
entire kernel. Often the difference between an IP block on SoC A and A+1 
can be expressed by some integers, e.g. maximum size of addressable 
memory. Some other differences might never get supported by the driver. 
And only occasionally the new IP would get a feature, support for which is 
added to the driver and really modifies its behaviour. With platform data 
therefore you either get very similar or identical configurations, maybe 
some .max_frame_size will change, but the driver won't change anyway, and 
only in some rare cases the driver really needs to be changed. With 
compatibility strings we have to change _all_ Renesas drivers for _each_ 
SoC version even if just to add a new struct of_device_id entry. This 
doesn't seem very productive to me.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pawel Moll July 29, 2013, 10:50 a.m. UTC | #6
On Mon, 2013-07-29 at 08:18 +0100, Guennadi Liakhovetski wrote:
> A short addendum. At least with Renesas SoCs I see the situation in the 
> following way: new SoC versions appear relatively frequently. 

What frequency are we talking about? Once per year? Once per month? I'm
not trying to be picky, it really makes a difference...

> With compatibility strings we have to change _all_ Renesas drivers for _each_ 
> SoC version even if just to add a new struct of_device_id entry. This 
> doesn't seem very productive to me.

There is at least precedence in the MMC world - have a look at
drivers/mmc/host/mmci.c and the struct variant_data... There seem to be
a new variant every 6 months or so (I must admit guilt of of making some
of them ;-) coming from two different companies.

And although it's a primecell (aka amba_bus) device, so the compatibilty
string remains the same, the struct amba_id was being extended every
time which is pretty much the same thing.

Pawel

PS. Having said all that I'm hoping the MMCI evolution has finally
stopped so no new variants will be needed ;-)


--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pawel Moll July 29, 2013, 10:57 a.m. UTC | #7
On Fri, 2013-07-26 at 18:10 +0100, Stephen Warren wrote:
> > +- uhs-sdr12: the host supports UHS SDR12 mode
> > +- uhs-sdr25: the host supports UHS SDR25 mode
> > +- uhs-sdr50: the host supports UHS SDR50 mode
> > +- uhs-sdr104: the host supports UHS SDR104 mode
> > +- uhs-ddr50: the host supports UHS DDR50 mode
> > +- ddr-1v2: the host can support DDR, using 1.2V VccQ
> > +- ddr-1v8: the host can support DDR, using 1.8V VccQ

I understand that they are equivalent of the mmc caps #defines. Having
absolutely no UHS knowledge (nor access to the spec) I'm just wondering
if some of them couldn't be derived from the "environment", being the
way the host is wired up?

An example would be the classic OCR VDD fields - the voltage range bits
are taken from the regulator "vmmc" supply (see
mmc_regulator_get_supply()).

Of course I this may be completely wrong approach here...

Pawel


--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski July 29, 2013, 11:10 a.m. UTC | #8
On Mon, 29 Jul 2013, Pawel Moll wrote:

> On Fri, 2013-07-26 at 18:10 +0100, Stephen Warren wrote:
> > > +- uhs-sdr12: the host supports UHS SDR12 mode
> > > +- uhs-sdr25: the host supports UHS SDR25 mode
> > > +- uhs-sdr50: the host supports UHS SDR50 mode
> > > +- uhs-sdr104: the host supports UHS SDR104 mode
> > > +- uhs-ddr50: the host supports UHS DDR50 mode
> > > +- ddr-1v2: the host can support DDR, using 1.2V VccQ
> > > +- ddr-1v8: the host can support DDR, using 1.8V VccQ
> 
> I understand that they are equivalent of the mmc caps #defines. Having
> absolutely no UHS knowledge (nor access to the spec) I'm just wondering
> if some of them couldn't be derived from the "environment", being the
> way the host is wired up?

Indeed it would be good to get a reply from someone with access to the 
specs.

Thanks
Guennadi

> An example would be the classic OCR VDD fields - the voltage range bits
> are taken from the regulator "vmmc" supply (see
> mmc_regulator_get_supply()).
> 
> Of course I this may be completely wrong approach here...
> 
> Pawel

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski July 29, 2013, 11:27 a.m. UTC | #9
On Mon, 29 Jul 2013, Pawel Moll wrote:

> On Mon, 2013-07-29 at 08:18 +0100, Guennadi Liakhovetski wrote:
> > A short addendum. At least with Renesas SoCs I see the situation in the 
> > following way: new SoC versions appear relatively frequently. 
> 
> What frequency are we talking about? Once per year? Once per month? I'm
> not trying to be picky, it really makes a difference...

Definitely not every month - not until now in the mainline at least. I 
currently count 9 SoCs, added since 2010, which makes about 2-3 SoCs per 
year.

Thanks
Guennadi

> > With compatibility strings we have to change _all_ Renesas drivers for _each_ 
> > SoC version even if just to add a new struct of_device_id entry. This 
> > doesn't seem very productive to me.
> 
> There is at least precedence in the MMC world - have a look at
> drivers/mmc/host/mmci.c and the struct variant_data... There seem to be
> a new variant every 6 months or so (I must admit guilt of of making some
> of them ;-) coming from two different companies.
> 
> And although it's a primecell (aka amba_bus) device, so the compatibilty
> string remains the same, the struct amba_id was being extended every
> time which is pretty much the same thing.
> 
> Pawel
> 
> PS. Having said all that I'm hoping the MMCI evolution has finally
> stopped so no new variants will be needed ;-)

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pawel Moll July 29, 2013, 11:42 a.m. UTC | #10
On Mon, 2013-07-29 at 12:27 +0100, Guennadi Liakhovetski wrote:
> On Mon, 29 Jul 2013, Pawel Moll wrote:
> 
> > On Mon, 2013-07-29 at 08:18 +0100, Guennadi Liakhovetski wrote:
> > > A short addendum. At least with Renesas SoCs I see the situation in the 
> > > following way: new SoC versions appear relatively frequently. 
> > 
> > What frequency are we talking about? Once per year? Once per month? I'm
> > not trying to be picky, it really makes a difference...
> 
> Definitely not every month - not until now in the mainline at least. I 
> currently count 9 SoCs, added since 2010, which makes about 2-3 SoCs per 
> year.

So this is actually a slower rate that I've faced in my previous life
working for a silicon vendor ;-) And my experience is that the IPs were
different between the SoCs indeed but:

1. Not all of them at the same time (so no extra compatible values for
others).
2. When there was a change it required change in a driver as well (so
adding a compatible value is not an issue).
3. The changes were *completely* unpredictable (so even comprehensive
list of DT properties wouldn't help)

To summarize, count this as another vote for using compatible rather
then "universal & future-proof" set of properties. Unless there is a
very good rationale for it (I'm sure such cases exist)

Thanks!

Pawel


--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski July 29, 2013, 12:05 p.m. UTC | #11
On Mon, 29 Jul 2013, Pawel Moll wrote:

> On Mon, 2013-07-29 at 12:27 +0100, Guennadi Liakhovetski wrote:
> > On Mon, 29 Jul 2013, Pawel Moll wrote:
> > 
> > > On Mon, 2013-07-29 at 08:18 +0100, Guennadi Liakhovetski wrote:
> > > > A short addendum. At least with Renesas SoCs I see the situation in the 
> > > > following way: new SoC versions appear relatively frequently. 
> > > 
> > > What frequency are we talking about? Once per year? Once per month? I'm
> > > not trying to be picky, it really makes a difference...
> > 
> > Definitely not every month - not until now in the mainline at least. I 
> > currently count 9 SoCs, added since 2010, which makes about 2-3 SoCs per 
> > year.
> 
> So this is actually a slower rate that I've faced in my previous life
> working for a silicon vendor ;-) And my experience is that the IPs were
> different between the SoCs indeed but:
> 
> 1. Not all of them at the same time (so no extra compatible values for
> others).

No, that's exactly the problem. We absolutely do not want to write 
compatible="vendor,soc-v1"; in a .dts file for SoC v2 just because we 
currently think, that we don't have to distinguish between those SoC 
versions in this specific driver. I think we all know it quite well, that 
there are (practically) always differences - sometimes documented, 
sometimes undocumented. And if you later find out, that you do have to 
differentiate in the driver - it's too late. Even if we disregard the 
argument of ugliness of having to set compatibility with soc-v1, soc-v2, 
soc-v3 in different DT nodes on an SoC v4.

Thanks
Guennadi

> 2. When there was a change it required change in a driver as well (so
> adding a compatible value is not an issue).
> 3. The changes were *completely* unpredictable (so even comprehensive
> list of DT properties wouldn't help)
> 
> To summarize, count this as another vote for using compatible rather
> then "universal & future-proof" set of properties. Unless there is a
> very good rationale for it (I'm sure such cases exist)
> 
> Thanks!
> 
> Pawel
> 
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pawel Moll July 29, 2013, 1:07 p.m. UTC | #12
On Mon, 2013-07-29 at 13:05 +0100, Guennadi Liakhovetski wrote:
> No, that's exactly the problem. We absolutely do not want to write 
> compatible="vendor,soc-v1"; in a .dts file for SoC v2 just because we 
> currently think, that we don't have to distinguish between those SoC 
> versions in this specific driver. I think we all know it quite well, that 
> there are (practically) always differences - sometimes documented, 
> sometimes undocumented. And if you later find out, that you do have to 
> differentiate in the driver - it's too late. Even if we disregard the 
> argument of ugliness of having to set compatibility with soc-v1, soc-v2, 
> soc-v3 in different DT nodes on an SoC v4.

First of all I think your example calls for more than one compatible
string - if it seems that soc,v2 is almost like soc,v1, make it
compatible = "soc-v2", "soc-v1" and don't touch the driver (as in: keep
it compatible with "soc-v1" only). Then, when the realisation comes, you
can simply add the "soc-v2" of_device_id with .data pointing at new
features.

Now the other thing - do you have a driver for a SoC at all? What I mean
is that in most cases it's the components/peripherals/IPs (whatever you
call them) matter, not the SoC itself, so the SoC compatible value can
be unique if you wish (and, if it is a *.dtsi, it will be compatbile
with "simple-bus" anyway ;-). Then the IP nodes can follow the rule
above.

Hope it makes some sense?

Pawel


--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski July 29, 2013, 1:30 p.m. UTC | #13
On Mon, 29 Jul 2013, Pawel Moll wrote:

> On Mon, 2013-07-29 at 13:05 +0100, Guennadi Liakhovetski wrote:
> > No, that's exactly the problem. We absolutely do not want to write 
> > compatible="vendor,soc-v1"; in a .dts file for SoC v2 just because we 
> > currently think, that we don't have to distinguish between those SoC 
> > versions in this specific driver. I think we all know it quite well, that 
> > there are (practically) always differences - sometimes documented, 
> > sometimes undocumented. And if you later find out, that you do have to 
> > differentiate in the driver - it's too late. Even if we disregard the 
> > argument of ugliness of having to set compatibility with soc-v1, soc-v2, 
> > soc-v3 in different DT nodes on an SoC v4.
> 
> First of all I think your example calls for more than one compatible
> string - if it seems that soc,v2 is almost like soc,v1, make it
> compatible = "soc-v2", "soc-v1" and don't touch the driver (as in: keep
> it compatible with "soc-v1" only). Then, when the realisation comes, you
> can simply add the "soc-v2" of_device_id with .data pointing at new
> features.

Yes, this is also a possibility, but it seems only a little bit better. 
Why should a DT node on SoC vN have a compatible string with SoC vK for 
some random for an abstract user absolutely unrelated SoC version?... And 
that vK would be different for different devices... So on SoC v5 MMC can 
be compatible with with v1, sound with v2, camera with v3... Don't you 
think it would look like a mess?

> Now the other thing - do you have a driver for a SoC at all? What I mean
> is that in most cases it's the components/peripherals/IPs (whatever you
> call them) matter, not the SoC itself, so the SoC compatible value can
> be unique if you wish (and, if it is a *.dtsi, it will be compatbile
> with "simple-bus" anyway ;-). Then the IP nodes can follow the rule
> above.

Sure, I don't mean the top-level root compatibility, I mean device 
compatibility.

Thanks
Guennadi

> Hope it makes some sense?
> 
> Pawel

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pawel Moll July 29, 2013, 1:37 p.m. UTC | #14
On Mon, 2013-07-29 at 14:30 +0100, Guennadi Liakhovetski wrote:
> Yes, this is also a possibility, but it seems only a little bit better. 
> Why should a DT node on SoC vN have a compatible string with SoC vK for 
> some random for an abstract user absolutely unrelated SoC version?... And 
> that vK would be different for different devices... So on SoC v5 MMC can 
> be compatible with with v1, sound with v2, camera with v3... Don't you 
> think it would look like a mess?

It won't be SoC v5 MMC then. It will be just MMC vX. SoC v6 will still
have MMC vX, but SoC v7 already MMC vY (where, most likely, Y=X+1). I
appreciate the fact that the SoC datasheets don't always explicitly
mention the IP revision, but it's still there. Also, quite often the
differences don't lie in the IP itself (it costs a lot of money to
license a new version of eg. MMC host controller from Synopsys), but in
the glue logic/adaptation layer etc. But in such case it probably should
be described in the tree itself. Which gets us to square one :-)

Pawel

PS. Of course MMC vY may be just a superset of vX, so then it will be
compatible with "MMC vY", "MMC vX".


--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring July 29, 2013, 1:39 p.m. UTC | #15
On 07/29/2013 02:18 AM, Guennadi Liakhovetski wrote:
> On Mon, 29 Jul 2013, Guennadi Liakhovetski wrote:
> 
>> Hi Stephen
>>
>> On Fri, 26 Jul 2013, Stephen Warren wrote:
>>
>>> On 07/26/2013 02:23 PM, Guennadi Liakhovetski wrote:
>>>> Hi Stephen
>>>>
>>>> On Fri, 26 Jul 2013, Stephen Warren wrote:
>>>>
>>>>> On 07/26/2013 09:51 AM, Guennadi Liakhovetski wrote:
>>>>>> Add DT properties for UHS SDR12, SDR25, SDR50, SDR104 and DDR50 modes and
>>>>>> for supported by the host in DDR mode VccQ values. Adding them to DT will
>>>>>> automatically enable respective MMC host capabilities.
>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
>>>>>
>>>>>> +- uhs-sdr12: the host supports UHS SDR12 mode
>>>>>> +- uhs-sdr25: the host supports UHS SDR25 mode
>>>>>> +- uhs-sdr50: the host supports UHS SDR50 mode
>>>>>> +- uhs-sdr104: the host supports UHS SDR104 mode
>>>>>> +- uhs-ddr50: the host supports UHS DDR50 mode
>>>>>> +- ddr-1v2: the host can support DDR, using 1.2V VccQ
>>>>>> +- ddr-1v8: the host can support DDR, using 1.8V VccQ
>>>>>
>>>>> Surely the driver for the host controller already knows this, so there's
>>>>> no need to represent it in DT?
>>>>
>>>> Not necessarily. One driver can handle several versions of the same chip, 
>>>> and some versions of the chip implement some of those features, others 
>>>> don't.
>>>
>>> Certainly.
>>>
>>>> So, your choice is really whether to specify a chip version in the
>>>> compatible string or these properties.
>>>
>>> There's no choice there. The compatible property needs to specify all of
>>> the following:
>>>
>>> * A value which describes the exact chip the IP block is in. This can be
>>> matched on to enable any quirks that need to be handled due to
>>> integration of the IP into the particular chip. This value will list the
>>> chip version. An example might be tegra20-sdhci.
>>>
>>> * A value which describes the IP block version (if that IP block has a
>>> version independent of the SoC that contains it, as e.g. a Synopsis IP
>>> block might). A totally made-up example might be synopsis-dwc2-1.0.0
>>>
>>> * Various more generic values that describe the HW, and that define a n
>>> interface to the HW that is specific and complete enough that HW can
>>> program to. An example might be usb-ehci (assuming a chip that doesn't
>>> have so many quirks that a driver has to know more than just "it's an
>>> EHCI controller).
>>
>> Yes, all these certainly make sense. As far as I understand, we are still 
>> in the process of defining good clear rules for DTs, there is an "ABI" 
>> discussion currently running on ALKML and IIRC this is also going to be a 
>> topic for one of coming conferences. So, hopefully we're approaching a 
>> state of a greater clarity about DT.
> 
> A short addendum. At least with Renesas SoCs I see the situation in the 
> following way: new SoC versions appear relatively frequently. They often 
> re-use multiple IP blocks, drivers for which are scattered across the 
> entire kernel. Often the difference between an IP block on SoC A and A+1 
> can be expressed by some integers, e.g. maximum size of addressable 
> memory. Some other differences might never get supported by the driver. 
> And only occasionally the new IP would get a feature, support for which is 
> added to the driver and really modifies its behaviour. With platform data 
> therefore you either get very similar or identical configurations, maybe 
> some .max_frame_size will change, but the driver won't change anyway, and 
> only in some rare cases the driver really needs to be changed. With 
> compatibility strings we have to change _all_ Renesas drivers for _each_ 
> SoC version even if just to add a new struct of_device_id entry. This 
> doesn't seem very productive to me.

If there is no change in the IP, then you can continue to use the
previous compatible string in your binding for the new SoC. The kernel
only has to be updated when there is a new feature, bug or other change.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski July 29, 2013, 2:40 p.m. UTC | #16
Hi Rob

On Mon, 29 Jul 2013, Rob Herring wrote:

[snip]

> If there is no change in the IP, then you can continue to use the
> previous compatible string in your binding for the new SoC. The kernel
> only has to be updated when there is a new feature, bug or other change.

Please, see my other replies in this thread.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 29, 2013, 5:21 p.m. UTC | #17
On 07/29/2013 07:30 AM, Guennadi Liakhovetski wrote:
> On Mon, 29 Jul 2013, Pawel Moll wrote:
> 
>> On Mon, 2013-07-29 at 13:05 +0100, Guennadi Liakhovetski wrote:
>>> No, that's exactly the problem. We absolutely do not want to write 
>>> compatible="vendor,soc-v1"; in a .dts file for SoC v2 just because we 
>>> currently think, that we don't have to distinguish between those SoC 
>>> versions in this specific driver. I think we all know it quite well, that 
>>> there are (practically) always differences - sometimes documented, 
>>> sometimes undocumented. And if you later find out, that you do have to 
>>> differentiate in the driver - it's too late. Even if we disregard the 
>>> argument of ugliness of having to set compatibility with soc-v1, soc-v2, 
>>> soc-v3 in different DT nodes on an SoC v4.
>>
>> First of all I think your example calls for more than one compatible
>> string - if it seems that soc,v2 is almost like soc,v1, make it
>> compatible = "soc-v2", "soc-v1" and don't touch the driver (as in: keep
>> it compatible with "soc-v1" only). Then, when the realisation comes, you
>> can simply add the "soc-v2" of_device_id with .data pointing at new
>> features.
> 
> Yes, this is also a possibility, but it seems only a little bit better. 
> Why should a DT node on SoC vN have a compatible string with SoC vK for 
> some random for an abstract user absolutely unrelated SoC version?...

Precisely because the HW /is/ compatible.

If the HW interface (registers) in two chips A and B is such that a
driver for chip A can be expected to run unmodified on chip B too
(albeit perhaps without yet supporting any new features, or perhaps not
running at full performance etc.), then chip B /is/ compatible with chip
A, and it makes sense to mark it so in DT. That way, an old kernel which
had support for chip A can run on chip B. A newer kernel might come
along later with explicit support for chip B, and hence run faster
and/or expose new features, but the driver for chip A can still work on
chip B.

> And 
> that vK would be different for different devices... So on SoC v5 MMC can 
> be compatible with with v1, sound with v2, camera with v3... Don't you 
> think it would look like a mess?

I don't consider that a mess, no. It's nice evolutionary HW design:-)
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 29, 2013, 5:28 p.m. UTC | #18
On 07/29/2013 12:20 AM, Guennadi Liakhovetski wrote:
> Hi Stephen
> 
> On Fri, 26 Jul 2013, Stephen Warren wrote:
> 
>> On 07/26/2013 02:23 PM, Guennadi Liakhovetski wrote:
>>> Hi Stephen
>>>
>>> On Fri, 26 Jul 2013, Stephen Warren wrote:
>>>
>>>> On 07/26/2013 09:51 AM, Guennadi Liakhovetski wrote:
>>>>> Add DT properties for UHS SDR12, SDR25, SDR50, SDR104 and DDR50 modes and
>>>>> for supported by the host in DDR mode VccQ values. Adding them to DT will
>>>>> automatically enable respective MMC host capabilities.
>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
>>>>
>>>>> +- uhs-sdr12: the host supports UHS SDR12 mode
>>>>> +- uhs-sdr25: the host supports UHS SDR25 mode
>>>>> +- uhs-sdr50: the host supports UHS SDR50 mode
>>>>> +- uhs-sdr104: the host supports UHS SDR104 mode
>>>>> +- uhs-ddr50: the host supports UHS DDR50 mode
>>>>> +- ddr-1v2: the host can support DDR, using 1.2V VccQ
>>>>> +- ddr-1v8: the host can support DDR, using 1.8V VccQ
>>>>
>>>> Surely the driver for the host controller already knows this, so there's
>>>> no need to represent it in DT?
>>>
>>> Not necessarily. One driver can handle several versions of the same chip, 
>>> and some versions of the chip implement some of those features, others 
>>> don't.
>>
>> Certainly.
>>
>>> So, your choice is really whether to specify a chip version in the
>>> compatible string or these properties.
>>
>> There's no choice there. The compatible property needs to specify all of
>> the following:
>>
>> * A value which describes the exact chip the IP block is in. This can be
>> matched on to enable any quirks that need to be handled due to
>> integration of the IP into the particular chip. This value will list the
>> chip version. An example might be tegra20-sdhci.
>>
>> * A value which describes the IP block version (if that IP block has a
>> version independent of the SoC that contains it, as e.g. a Synopsis IP
>> block might). A totally made-up example might be synopsis-dwc2-1.0.0
>>
>> * Various more generic values that describe the HW, and that define a n
>> interface to the HW that is specific and complete enough that HW can
>> program to. An example might be usb-ehci (assuming a chip that doesn't
>> have so many quirks that a driver has to know more than just "it's an
>> EHCI controller).
> 
> Yes, all these certainly make sense. As far as I understand, we are still 
> in the process of defining good clear rules for DTs, there is an "ABI" 
> discussion currently running on ALKML and IIRC this is also going to be a 
> topic for one of coming conferences. So, hopefully we're approaching a 
> state of a greater clarity about DT.

I don't think the rules for the compatible property are up in the air;
they've been pretty well communicated since at least the start of my
involvement in DT perhaps 2 years ago.

>> Further classes of compatible value might exist, but you get the idea.
>>
>> All of those values have to exist in the DT right from the very start,
>> so that the first DT is usable with a future kernel, which might decide
>> to vary the exact compatible value(s) it matches on, provided they're
>> all documented in the DT binding ABI, in order to enable/disable new
>> sets of quirsk.
> 
> Makes sense too, sure.
> 
>>> Now, when you consider that
>>> multiple drivers have to decide upon those, and sometimes you don't have 
>>> an exact IP version of the SD/MMC controller but only the SoC version, you 
>>> choice becomes: 
>>
>> That would be a bug in the DT, given my assertions above.
> 
> Sorry, what exactly would be a bug? Not having an exact IP version?

Yes.

> But 
> you did write above - "if that IP block has a version independent of the 
> SoC that contains it." In my case it doesn't really. Those IPs only exist 
> within those SoCs.

I think this has been well covered in the rest of the thread, but if the
IP block doesn't have a version independent from the SoC itself (because
the SoC is developed as a whole, rather than being pieced together out
of a pool of IP blocks for example), then the IP block's version *is*
the SoC version. In my list of "sources" of compatible values above,
it's possible that the values for multiple of those bullet points end up
being the same.

>> Related, is it really true that zero driver involvement is required to
>> enable these UHS features?
> 
> I think it is, in most cases at least.
> 
>> If absolutely any HW can enable them without
>> any driver support at all, then perhaps it's still reasonable to create
>> DT properties to enable them. However, if driver support is required to
>> make those features actually work, the driver had better be validating
>> that support actually works on the HW it's running on before enabling
>> the feature (and can therefore pass the validation results to the SD
>> core rather than relying on DT properties to be set).
> 
> I think it rather works the other way round. Those flags only mark 
> hardware _capabilities_.

Which HW though?

Presumably eMMC/SD-card capabilities are reported by the card through
the various ID commands.

HW/driver capabilities would be reported by the driver itself. I still
don't buy the argument that no driver support is required, since
historically the different speed modes on MMC have required different
clock rates to the HW, different clock divider rounding/selection
schemes, different bug workarounds, etc.

Board capabilities (i.e. was the board routed with the right trace
impedance/... to support the UHS features, rather than something slower
because the designer didn't care about the UHS features and they're more
expensive) seem fine to go in DT.

> But don't tell the driver to use them yet.
> Normally drivers just collect them in their .caps and .caps2 fields and 
> pass on to the mmc core.

So you would expect individual drivers to parse these (perhaps using
common code) and filter the values depending on their capabilities? If
so, why not just have the drivers set the values up themselves. Why is
there a need to represent the data in DT?

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt b/Documentation/devicetree/bindings/mmc/mmc.txt
index 458b57f..de4a716 100644
--- a/Documentation/devicetree/bindings/mmc/mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/mmc.txt
@@ -29,6 +29,13 @@  Optional properties:
 - cap-power-off-card: powering off the card is safe
 - cap-sdio-irq: enable SDIO IRQ signalling on this interface
 - full-pwr-cycle: full power cycle of the card is supported
+- uhs-sdr12: the host supports UHS SDR12 mode
+- uhs-sdr25: the host supports UHS SDR25 mode
+- uhs-sdr50: the host supports UHS SDR50 mode
+- uhs-sdr104: the host supports UHS SDR104 mode
+- uhs-ddr50: the host supports UHS DDR50 mode
+- ddr-1v2: the host can support DDR, using 1.2V VccQ
+- ddr-1v8: the host can support DDR, using 1.8V VccQ
 
 *NOTE* on CD and WP polarity. To use common for all SD/MMC host controllers line
 polarity properties, we have to fix the meaning of the "normal" and "inverted"
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 6fb6f77..e12fba0 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -423,6 +423,20 @@  int mmc_of_parse(struct mmc_host *host)
 		host->caps |= MMC_CAP_POWER_OFF_CARD;
 	if (of_find_property(np, "cap-sdio-irq", &len))
 		host->caps |= MMC_CAP_SDIO_IRQ;
+	if (of_find_property(np, "uhs-sdr12", &len))
+		host->caps |= MMC_CAP_UHS_SDR12;
+	if (of_find_property(np, "uhs-sdr25", &len))
+		host->caps |= MMC_CAP_UHS_SDR25;
+	if (of_find_property(np, "uhs-sdr50", &len))
+		host->caps |= MMC_CAP_UHS_SDR50;
+	if (of_find_property(np, "uhs-sdr104", &len))
+		host->caps |= MMC_CAP_UHS_SDR104;
+	if (of_find_property(np, "uhs-ddr50", &len))
+		host->caps |= MMC_CAP_UHS_DDR50;
+	if (of_find_property(np, "ddr-1v2", &len))
+		host->caps |= MMC_CAP_1_2V_DDR;
+	if (of_find_property(np, "ddr-1v8", &len))
+		host->caps |= MMC_CAP_1_8V_DDR;
 	if (of_find_property(np, "full-pwr-cycle", &len))
 		host->caps2 |= MMC_CAP2_FULL_PWR_CYCLE;
 	if (of_find_property(np, "keep-power-in-suspend", &len))