Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1540471 > unrolled thread

Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support

Started byRob Herring <robh@kernel.org>
First post2016-12-12 18:20 +0100
Last post2016-12-16 12:10 +0100
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support Rob Herring <robh@kernel.org> - 2016-12-12 18:20 +0100
    Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-12-13 05:50 +0100
      Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support Ulf Hansson <ulf.hansson@linaro.org> - 2016-12-13 09:00 +0100
        Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-12-14 03:30 +0100
          Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support Ulf Hansson <ulf.hansson@linaro.org> - 2016-12-16 12:10 +0100

#1540471 — Re: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support

FromRob Herring <robh@kernel.org>
Date2016-12-12 18:20 +0100
SubjectRe: [PATCH v5 2/2] mmc: sdhci-cadence: add Cadence SD4HC support
Message-ID<sNCqC-75R-35@gated-at.bofh.it>
On Thu, Dec 08, 2016 at 09:50:55PM +0900, Masahiro Yamada wrote:
> Add a driver for the Cadence SD4HC SD/SDIO/eMMC Controller.
> 
> For SD, it basically relies on the SDHCI standard code.
> For eMMC, this driver provides some callbacks to support the
> hardware part that is specific to this IP design.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> 
> Changes in v5:
>   - Fix calculation of the center of the tuned window
> 
> Changes in v4:
>   - Override mmc_host_ops.execute_tuning instead of the
>     .platform_execute_tuning implementation
> 
> Changes in v3:
>   - Remove unneeded explanation about HRS and SRS from DT binding;
>     the offsets to HRS/SRS are fixed for this hardware and this is
>     quite normal, like each hardware has a fixed register view except
>     the register base.  The detailed register map is what the driver
>     cares about, so no need to explain it in the binding.
> 
> Changes in v2:
>   - Remove unnecessary "select MMC_SDHCI_IO_ACCESSORS"
> 
>  .../devicetree/bindings/mmc/sdhci-cadence.txt      |  30 +++
>  drivers/mmc/host/Kconfig                           |  11 +
>  drivers/mmc/host/Makefile                          |   1 +
>  drivers/mmc/host/sdhci-cadence.c                   | 283 +++++++++++++++++++++
>  4 files changed, 325 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
>  create mode 100644 drivers/mmc/host/sdhci-cadence.c
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> new file mode 100644
> index 0000000..750374f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
> @@ -0,0 +1,30 @@
> +* Cadence SD/SDIO/eMMC Host Controller
> +
> +Required properties:
> +- compatible: should be "cdns,sd4hc".

Needs SoC specific compatible strings too.

> +- reg: offset and length of the register set for the device.
> +- interrupts: a single interrupt specifier.
> +- clocks: phandle to the input clock.
> +
> +Optional properties:
> +For eMMC configuration, supported speed modes are not indicated by the SDHCI
> +Capabilities Register.  Instead, the following properties should be specified
> +if supported.  See mmc.txt for details.
> +- mmc-ddr-1_8v
> +- mmc-ddr-1_2v
> +- mmc-hs200-1_8v
> +- mmc-hs200-1_2v
> +- mmc-hs400-1_8v
> +- mmc-hs400-1_2v

There's now a property to override SDHCI capabilities register. Maybe 
you should use that instead? I'll defer to Ulf.

> +
> +Example:
> +	emmc: sdhci@5a000000 {
> +		compatible = "cdns,sd4hc";
> +		reg = <0x5a000000 0x400>;
> +		interrupts = <0 78 4>;
> +		clocks = <&clk 4>;
> +		bus-width = <8>;
> +		mmc-ddr-1_8v;
> +		mmc-hs200-1_8v;
> +		mmc-hs400-1_8v;
> +	};

[toc] | [next] | [standalone]

#1540843

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2016-12-13 05:50 +0100
Message-ID<sNNcl-56a-5@gated-at.bofh.it>
In reply to#1540471
Hi Rob.

2016-12-13 2:14 GMT+09:00 Rob Herring <robh@kernel.org>:
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
>> new file mode 100644
>> index 0000000..750374f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
>> @@ -0,0 +1,30 @@
>> +* Cadence SD/SDIO/eMMC Host Controller
>> +
>> +Required properties:
>> +- compatible: should be "cdns,sd4hc".
>
> Needs SoC specific compatible strings too.


I remember you mentioned the vendor prefix
stands for the SoC vendor, not the IP vendor.
The compatible prefixed with the IP vendor is prepared for a fallback.

I will add "socionext,sd4hc".




>> +- reg: offset and length of the register set for the device.
>> +- interrupts: a single interrupt specifier.
>> +- clocks: phandle to the input clock.
>> +
>> +Optional properties:
>> +For eMMC configuration, supported speed modes are not indicated by the SDHCI
>> +Capabilities Register.  Instead, the following properties should be specified
>> +if supported.  See mmc.txt for details.
>> +- mmc-ddr-1_8v
>> +- mmc-ddr-1_2v
>> +- mmc-hs200-1_8v
>> +- mmc-hs200-1_2v
>> +- mmc-hs400-1_8v
>> +- mmc-hs400-1_2v
>
> There's now a property to override SDHCI capabilities register. Maybe
> you should use that instead? I'll defer to Ulf.
>

I did not know this new property.

So, now we have two ways to specify MMC speed mode capabilities
by only touching DT.

[1] Add MMC mode flags directly, like I did.
[2] Use "sdhci-caps-mask" and "sdhci-caps"


The problem for [2] is that eMMC capabilities
do not perfectly correspond to the SDHCI capabilities register.


>> +- mmc-hs400-1_8v
>> +- mmc-hs400-1_2v

If the driver sets SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400,
we can use the bit63 of caps for specifying HS400.

But, this is not defined in the SDHCI standard.
#define  SDHCI_SUPPORT_HS400 0x80000000 /* Non-standard */



>> +- mmc-ddr-1_8v

For High Speed DDR, perhaps we can imply MMC_CAP_1_8V_DDR
from MMC_CAP_UHS_DDR50  (bit34 of caps)

This is not supported in the current code, but
if this is a good idea, I can send a patch.


>> +- mmc-ddr-1_2v

This does not have the corresponding bit, but
1.2V is not commonly used, so this is not a fatal problem.



What I can do at most now, is to delete the
Optional properties section entirely
so users can choose [1] or [2] as they like.



-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1540882

FromUlf Hansson <ulf.hansson@linaro.org>
Date2016-12-13 09:00 +0100
Message-ID<sNQad-6TX-27@gated-at.bofh.it>
In reply to#1540843
[...]

>>> +
>>> +Optional properties:
>>> +For eMMC configuration, supported speed modes are not indicated by the SDHCI
>>> +Capabilities Register.  Instead, the following properties should be specified
>>> +if supported.  See mmc.txt for details.
>>> +- mmc-ddr-1_8v
>>> +- mmc-ddr-1_2v
>>> +- mmc-hs200-1_8v
>>> +- mmc-hs200-1_2v
>>> +- mmc-hs400-1_8v
>>> +- mmc-hs400-1_2v
>>
>> There's now a property to override SDHCI capabilities register. Maybe
>> you should use that instead? I'll defer to Ulf.
>>
>
> I did not know this new property.
>
> So, now we have two ways to specify MMC speed mode capabilities
> by only touching DT.

Let me clarify a bit.

The point with the new bindings is to be able to override *broken*
SDHCI caps bits. So, not only related to the speed modes.

>
> [1] Add MMC mode flags directly, like I did.
> [2] Use "sdhci-caps-mask" and "sdhci-caps"
>
>
> The problem for [2] is that eMMC capabilities
> do not perfectly correspond to the SDHCI capabilities register.
>
>
>>> +- mmc-hs400-1_8v
>>> +- mmc-hs400-1_2v
>
> If the driver sets SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400,
> we can use the bit63 of caps for specifying HS400.
>
> But, this is not defined in the SDHCI standard.
> #define  SDHCI_SUPPORT_HS400 0x80000000 /* Non-standard */
>
>
>
>>> +- mmc-ddr-1_8v
>
> For High Speed DDR, perhaps we can imply MMC_CAP_1_8V_DDR
> from MMC_CAP_UHS_DDR50  (bit34 of caps)
>
> This is not supported in the current code, but
> if this is a good idea, I can send a patch.
>
>
>>> +- mmc-ddr-1_2v
>
> This does not have the corresponding bit, but
> 1.2V is not commonly used, so this is not a fatal problem.
>
>
>
> What I can do at most now, is to delete the
> Optional properties section entirely
> so users can choose [1] or [2] as they like.
>

I think a better approach is to use the new sdhci-caps* bindings to
mask those caps that can't be trusted. And then use the generic mmc
bindings for speed modes instead.

That should be a safer approach, right!?

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1541645

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2016-12-14 03:30 +0100
Message-ID<sO7up-BJ-1@gated-at.bofh.it>
In reply to#1540882
Hi Ulf,

2016-12-13 16:52 GMT+09:00 Ulf Hansson <ulf.hansson@linaro.org>:
> [...]
>
>>>> +
>>>> +Optional properties:
>>>> +For eMMC configuration, supported speed modes are not indicated by the SDHCI
>>>> +Capabilities Register.  Instead, the following properties should be specified
>>>> +if supported.  See mmc.txt for details.
>>>> +- mmc-ddr-1_8v
>>>> +- mmc-ddr-1_2v
>>>> +- mmc-hs200-1_8v
>>>> +- mmc-hs200-1_2v
>>>> +- mmc-hs400-1_8v
>>>> +- mmc-hs400-1_2v
>>>
>>> There's now a property to override SDHCI capabilities register. Maybe
>>> you should use that instead? I'll defer to Ulf.
>>>
>>
>> I did not know this new property.
>>
>> So, now we have two ways to specify MMC speed mode capabilities
>> by only touching DT.
>
> Let me clarify a bit.
>
> The point with the new bindings is to be able to override *broken*
> SDHCI caps bits. So, not only related to the speed modes.

Right.

So my approach ([1] Add MMC mode flags directly)
basically sounds OK.



>>
>> [1] Add MMC mode flags directly, like I did.
>> [2] Use "sdhci-caps-mask" and "sdhci-caps"
>>
>>
>> The problem for [2] is that eMMC capabilities
>> do not perfectly correspond to the SDHCI capabilities register.
>>
>>
>>>> +- mmc-hs400-1_8v
>>>> +- mmc-hs400-1_2v
>>
>> If the driver sets SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400,
>> we can use the bit63 of caps for specifying HS400.
>>
>> But, this is not defined in the SDHCI standard.
>> #define  SDHCI_SUPPORT_HS400 0x80000000 /* Non-standard */
>>
>>
>>
>>>> +- mmc-ddr-1_8v
>>
>> For High Speed DDR, perhaps we can imply MMC_CAP_1_8V_DDR
>> from MMC_CAP_UHS_DDR50  (bit34 of caps)
>>
>> This is not supported in the current code, but
>> if this is a good idea, I can send a patch.
>>
>>
>>>> +- mmc-ddr-1_2v
>>
>> This does not have the corresponding bit, but
>> 1.2V is not commonly used, so this is not a fatal problem.
>>
>>
>>
>> What I can do at most now, is to delete the
>> Optional properties section entirely
>> so users can choose [1] or [2] as they like.
>>
>
> I think a better approach is to use the new sdhci-caps* bindings to
> mask those caps that can't be trusted. And then use the generic mmc
> bindings for speed modes instead.
>
> That should be a safer approach, right!?

Right.

But, if I know the caps register bits 63-32 are all zero,
I need not explicitly specify sdhci-caps-mask, right?



-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1543379

FromUlf Hansson <ulf.hansson@linaro.org>
Date2016-12-16 12:10 +0100
Message-ID<sOYyK-2L0-47@gated-at.bofh.it>
In reply to#1541645
[...]

>>
>> I think a better approach is to use the new sdhci-caps* bindings to
>> mask those caps that can't be trusted. And then use the generic mmc
>> bindings for speed modes instead.
>>
>> That should be a safer approach, right!?
>
> Right.
>
> But, if I know the caps register bits 63-32 are all zero,
> I need not explicitly specify sdhci-caps-mask, right?

Maybe not. I don't have a strong opinion here, so l am fine with
whatever you choose.

Kind regards
Uffe

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web