[PATCH 1/2] Powerpc: Add voltage ranges support for T4

[PATCH 1/2] Powerpc: Add voltage ranges support for T4

[Haijun Zhang]

Special voltages that can be support by eSDHC of T4 in esdhc node.

Signed-off-by: Haijun Zhang <haijun.zhang@freescale.com>
Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
---
 Documentation/devicetree/bindings/mmc/fsl-esdhc.txt | 3 +++
 arch/powerpc/boot/dts/fsl/t4240si-post.dtsi         | 1 +
 2 files changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt b/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
index bd9be0b..4aeae6e 100644
--- a/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
+++ b/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
@@ -19,6 +19,8 @@ Optional properties:
     "bus-width = <1>" property.
   - sdhci,auto-cmd12: specifies that a controller can only handle auto
     CMD12.
+  - 3300 3300: specifies that eSDHC controller can support voltages ranges
+    from 3300 to 3300. This is an optional.
 
 Example:
 
@@ -29,4 +31,5 @@ sdhci@2e000 {
 	interrupt-parent = <&ipic>;
 	/* Filled in by U-Boot */
 	clock-frequency = <0>;
+	voltage-ranges = <3300 3300>;
 };
diff --git a/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi b/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
index bd611a9..1ef2d2d 100644
--- a/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
@@ -399,6 +399,7 @@
 	sdhc@114000 {
 		compatible = "fsl,t4240-esdhc", "fsl,esdhc";
 		sdhci,auto-cmd12;
+		voltage-ranges = <1800 1800 3300 3300>;
 	};
 /include/ "qoriq-i2c-0.dtsi"
 /include/ "qoriq-i2c-1.dtsi"
-- 
1.8.0

[PATCH 2/2] mmc: esdhc: get voltage from dts file

[Haijun Zhang]

Add voltage-range support in esdhc of T4, So we can choose
to read voltages from dts file as one optional.
If we can get a valid voltage-range from device node, we use
this voltage as the final voltage support. Else we still read
from capacity or from other provider.

Signed-off-by: Haijun Zhang <haijun.zhang@freescale.com>
Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
---
 drivers/mmc/host/sdhci-of-esdhc.c | 31 +++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c          |  3 +++
 include/linux/mmc/sdhci.h         |  1 +
 3 files changed, 35 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 15039e2..8b4b27a 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -262,6 +262,35 @@ static int esdhc_pltfm_bus_width(struct sdhci_host *host, int width)
 	return 0;
 }
 
+static void esdhc_get_voltage(struct sdhci_host *host,
+			struct platform_device *pdev)
+{
+	const u32 *voltage_ranges;
+	int num_ranges, i;
+	struct device_node *np;
+	np = pdev->dev.of_node;
+
+	voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
+	num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
+	if (!voltage_ranges || !num_ranges) {
+		dev_info(&pdev->dev, "OF: voltage-ranges unspecified\n");
+		return;
+	}
+
+	for (i = 0; i < num_ranges; i++) {
+		const int j = i * 2;
+		u32 mask;
+		mask = mmc_vddrange_to_ocrmask(be32_to_cpu(voltage_ranges[j]),
+				be32_to_cpu(voltage_ranges[j + 1]));
+		if (!mask) {
+			dev_info(&pdev->dev,
+				"OF: false voltage-ranges specified\n");
+			return;
+		}
+		host->ocr_mask |= mask;
+	}
+}
+
 static const struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = esdhc_readl,
 	.read_w = esdhc_readw,
@@ -317,6 +346,8 @@ static int sdhci_esdhc_probe(struct platform_device *pdev)
 	/* call to generic mmc_of_parse to support additional capabilities */
 	mmc_of_parse(host->mmc);
 
+	esdhc_get_voltage(host, pdev);
+
 	ret = sdhci_add_host(host);
 	if (ret)
 		sdhci_pltfm_free(pdev);
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a78bd4f..57541e0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3119,6 +3119,9 @@ int sdhci_add_host(struct sdhci_host *host)
 				   SDHCI_MAX_CURRENT_MULTIPLIER;
 	}
 
+	if (host->ocr_mask)
+		ocr_avail = host->ocr_mask;
+
 	mmc->ocr_avail = ocr_avail;
 	mmc->ocr_avail_sdio = ocr_avail;
 	if (host->ocr_avail_sdio)
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index e3c6a74..3e781b8 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -171,6 +171,7 @@ struct sdhci_host {
 	unsigned int            ocr_avail_sdio;	/* OCR bit masks */
 	unsigned int            ocr_avail_sd;
 	unsigned int            ocr_avail_mmc;
+	u32 ocr_mask;		/* available voltages */
 
 	wait_queue_head_t	buf_ready_int;	/* Waitqueue for Buffer Read Ready interrupt */
 	unsigned int		tuning_done;	/* Condition flag set when CMD19 succeeds */
-- 
1.8.0

Re: [PATCH 2/2] mmc: esdhc: get voltage from dts file

[Scott Wood]

On 07/22/2013 02:53:56 AM, Haijun Zhang wrote:
> Add voltage-range support in esdhc of T4, So we can choose
> to read voltages from dts file as one optional.
> If we can get a valid voltage-range from device node, we use
> this voltage as the final voltage support. Else we still read
> from capacity or from other provider.
>=20
> Signed-off-by: Haijun Zhang <haijun.zhang@freescale.com>
> Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> ---
>  drivers/mmc/host/sdhci-of-esdhc.c | 31 =20
> +++++++++++++++++++++++++++++++
>  drivers/mmc/host/sdhci.c          |  3 +++
>  include/linux/mmc/sdhci.h         |  1 +
>  3 files changed, 35 insertions(+)
>=20
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c =20
> b/drivers/mmc/host/sdhci-of-esdhc.c
> index 15039e2..8b4b27a 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -262,6 +262,35 @@ static int esdhc_pltfm_bus_width(struct =20
> sdhci_host *host, int width)
>  	return 0;
>  }
>=20
> +static void esdhc_get_voltage(struct sdhci_host *host,
> +			struct platform_device *pdev)
> +{
> +	const u32 *voltage_ranges;
> +	int num_ranges, i;
> +	struct device_node *np;
> +	np =3D pdev->dev.of_node;
> +
> +	voltage_ranges =3D of_get_property(np, "voltage-ranges", =20
> &num_ranges);
> +	num_ranges =3D num_ranges / sizeof(*voltage_ranges) / 2;
> +	if (!voltage_ranges || !num_ranges) {
> +		dev_info(&pdev->dev, "OF: voltage-ranges =20
> unspecified\n");
> +		return;
> +	}
> +
> +	for (i =3D 0; i < num_ranges; i++) {
> +		const int j =3D i * 2;
> +		u32 mask;
> +		mask =3D =20
> mmc_vddrange_to_ocrmask(be32_to_cpu(voltage_ranges[j]),
> +				be32_to_cpu(voltage_ranges[j + 1]));
> +		if (!mask) {
> +			dev_info(&pdev->dev,
> +				"OF: false voltage-ranges specified\n");
> +			return;
> +		}
> +		host->ocr_mask |=3D mask;
> +	}
> +}

Don't duplicate this code.  Move it somewhere common and share it.

Why did you remove the range index from the error string, and why did =20
you change it from dev_err to dev_info?

-Scott=

RE: [PATCH 2/2] mmc: esdhc: get voltage from dts file

[Zhang Haijun-B42677]

Thanks.

Regards
Haijun.


> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 23, 2013 1:41 AM
> To: Zhang Haijun-B42677
> Cc: linux-mmc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> cbouatmailru@gmail.com; cjb@laptop.org; Fleming Andy-AFLEMING; Zhang
> Haijun-B42677; Zhang Haijun-B42677
> Subject: Re: [PATCH 2/2] mmc: esdhc: get voltage from dts file
>=20
> On 07/22/2013 02:53:56 AM, Haijun Zhang wrote:
> > Add voltage-range support in esdhc of T4, So we can choose to read
> > voltages from dts file as one optional.
> > If we can get a valid voltage-range from device node, we use this
> > voltage as the final voltage support. Else we still read from capacity
> > or from other provider.
> >
> > Signed-off-by: Haijun Zhang <haijun.zhang@freescale.com>
> > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> > ---
> >  drivers/mmc/host/sdhci-of-esdhc.c | 31
> > +++++++++++++++++++++++++++++++
> >  drivers/mmc/host/sdhci.c          |  3 +++
> >  include/linux/mmc/sdhci.h         |  1 +
> >  3 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
> > b/drivers/mmc/host/sdhci-of-esdhc.c
> > index 15039e2..8b4b27a 100644
> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > @@ -262,6 +262,35 @@ static int esdhc_pltfm_bus_width(struct
> > sdhci_host *host, int width)
> >  	return 0;
> >  }
> >
> > +static void esdhc_get_voltage(struct sdhci_host *host,
> > +			struct platform_device *pdev)
> > +{
> > +	const u32 *voltage_ranges;
> > +	int num_ranges, i;
> > +	struct device_node *np;
> > +	np =3D pdev->dev.of_node;
> > +
> > +	voltage_ranges =3D of_get_property(np, "voltage-ranges",
> > &num_ranges);
> > +	num_ranges =3D num_ranges / sizeof(*voltage_ranges) / 2;
> > +	if (!voltage_ranges || !num_ranges) {
> > +		dev_info(&pdev->dev, "OF: voltage-ranges
> > unspecified\n");
> > +		return;
> > +	}
> > +
> > +	for (i =3D 0; i < num_ranges; i++) {
> > +		const int j =3D i * 2;
> > +		u32 mask;
> > +		mask =3D
> > mmc_vddrange_to_ocrmask(be32_to_cpu(voltage_ranges[j]),
> > +				be32_to_cpu(voltage_ranges[j + 1]));
> > +		if (!mask) {
> > +			dev_info(&pdev->dev,
> > +				"OF: false voltage-ranges specified\n");
> > +			return;
> > +		}
> > +		host->ocr_mask |=3D mask;
> > +	}
> > +}
>=20
> Don't duplicate this code.  Move it somewhere common and share it.
[Haijun Wrote:] So, move it drivers/mmc/host/sdhci-pltfm.c and share it as
Sdhc_get_voltage()....?
>=20
> Why did you remove the range index from the error string, and why did you
> change it from dev_err to dev_info?
[Haijun Wrote:] I'll correct this. In case voltage-range specified if there=
 is still err.
It should be err.
>=20
> -Scott

Re: [PATCH 2/2] mmc: esdhc: get voltage from dts file

[Scott Wood]

On 07/22/2013 09:38:33 PM, Zhang Haijun-B42677 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, July 23, 2013 1:41 AM
> > To: Zhang Haijun-B42677
> > Cc: linux-mmc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > cbouatmailru@gmail.com; cjb@laptop.org; Fleming Andy-AFLEMING; Zhang
> > Haijun-B42677; Zhang Haijun-B42677
> > Subject: Re: [PATCH 2/2] mmc: esdhc: get voltage from dts file
> >
> > On 07/22/2013 02:53:56 AM, Haijun Zhang wrote:
> > > Add voltage-range support in esdhc of T4, So we can choose to read
> > > voltages from dts file as one optional.
> > > If we can get a valid voltage-range from device node, we use this
> > > voltage as the final voltage support. Else we still read from =20
> capacity
> > > or from other provider.
> > >
> > > Signed-off-by: Haijun Zhang <haijun.zhang@freescale.com>
> > > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> > > ---
> > >  drivers/mmc/host/sdhci-of-esdhc.c | 31
> > > +++++++++++++++++++++++++++++++
> > >  drivers/mmc/host/sdhci.c          |  3 +++
> > >  include/linux/mmc/sdhci.h         |  1 +
> > >  3 files changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
> > > b/drivers/mmc/host/sdhci-of-esdhc.c
> > > index 15039e2..8b4b27a 100644
> > > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > > @@ -262,6 +262,35 @@ static int esdhc_pltfm_bus_width(struct
> > > sdhci_host *host, int width)
> > >  	return 0;
> > >  }
> > >
> > > +static void esdhc_get_voltage(struct sdhci_host *host,
> > > +			struct platform_device *pdev)
> > > +{
> > > +	const u32 *voltage_ranges;
> > > +	int num_ranges, i;
> > > +	struct device_node *np;
> > > +	np =3D pdev->dev.of_node;
> > > +
> > > +	voltage_ranges =3D of_get_property(np, "voltage-ranges",
> > > &num_ranges);
> > > +	num_ranges =3D num_ranges / sizeof(*voltage_ranges) / 2;
> > > +	if (!voltage_ranges || !num_ranges) {
> > > +		dev_info(&pdev->dev, "OF: voltage-ranges
> > > unspecified\n");
> > > +		return;
> > > +	}
> > > +
> > > +	for (i =3D 0; i < num_ranges; i++) {
> > > +		const int j =3D i * 2;
> > > +		u32 mask;
> > > +		mask =3D
> > > mmc_vddrange_to_ocrmask(be32_to_cpu(voltage_ranges[j]),
> > > +				be32_to_cpu(voltage_ranges[j + 1]));
> > > +		if (!mask) {
> > > +			dev_info(&pdev->dev,
> > > +				"OF: false voltage-ranges specified\n");
> > > +			return;
> > > +		}
> > > +		host->ocr_mask |=3D mask;
> > > +	}
> > > +}
> >
> > Don't duplicate this code.  Move it somewhere common and share it.
> [Haijun Wrote:] So, move it drivers/mmc/host/sdhci-pltfm.c and share =20
> it as
> Sdhc_get_voltage()....?

I'll let the MMC maintainer say what the appropriate place would be...  =20
Don't capitalize the function name, though. :-)

-Scott=

RE: [PATCH 2/2] mmc: esdhc: get voltage from dts file

[Zhang Haijun-B42677]

Thanks.

Regards
Haijun.


> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, July 23, 2013 10:42 AM
> To: Zhang Haijun-B42677
> Cc: Wood Scott-B07421; linux-mmc@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; cbouatmailru@gmail.com; cjb@laptop.org; Fleming
> Andy-AFLEMING
> Subject: Re: [PATCH 2/2] mmc: esdhc: get voltage from dts file
>=20
> On 07/22/2013 09:38:33 PM, Zhang Haijun-B42677 wrote:
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, July 23, 2013 1:41 AM
> > > To: Zhang Haijun-B42677
> > > Cc: linux-mmc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > cbouatmailru@gmail.com; cjb@laptop.org; Fleming Andy-AFLEMING; Zhang
> > > Haijun-B42677; Zhang Haijun-B42677
> > > Subject: Re: [PATCH 2/2] mmc: esdhc: get voltage from dts file
> > >
> > > On 07/22/2013 02:53:56 AM, Haijun Zhang wrote:
> > > > Add voltage-range support in esdhc of T4, So we can choose to read
> > > > voltages from dts file as one optional.
> > > > If we can get a valid voltage-range from device node, we use this
> > > > voltage as the final voltage support. Else we still read from
> > capacity
> > > > or from other provider.
> > > >
> > > > Signed-off-by: Haijun Zhang <haijun.zhang@freescale.com>
> > > > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> > > > ---
> > > >  drivers/mmc/host/sdhci-of-esdhc.c | 31
> > > > +++++++++++++++++++++++++++++++
> > > >  drivers/mmc/host/sdhci.c          |  3 +++
> > > >  include/linux/mmc/sdhci.h         |  1 +
> > > >  3 files changed, 35 insertions(+)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
> > > > b/drivers/mmc/host/sdhci-of-esdhc.c
> > > > index 15039e2..8b4b27a 100644
> > > > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > > > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > > > @@ -262,6 +262,35 @@ static int esdhc_pltfm_bus_width(struct
> > > > sdhci_host *host, int width)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static void esdhc_get_voltage(struct sdhci_host *host,
> > > > +			struct platform_device *pdev)
> > > > +{
> > > > +	const u32 *voltage_ranges;
> > > > +	int num_ranges, i;
> > > > +	struct device_node *np;
> > > > +	np =3D pdev->dev.of_node;
> > > > +
> > > > +	voltage_ranges =3D of_get_property(np, "voltage-ranges",
> > > > &num_ranges);
> > > > +	num_ranges =3D num_ranges / sizeof(*voltage_ranges) / 2;
> > > > +	if (!voltage_ranges || !num_ranges) {
> > > > +		dev_info(&pdev->dev, "OF: voltage-ranges
> > > > unspecified\n");
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	for (i =3D 0; i < num_ranges; i++) {
> > > > +		const int j =3D i * 2;
> > > > +		u32 mask;
> > > > +		mask =3D
> > > > mmc_vddrange_to_ocrmask(be32_to_cpu(voltage_ranges[j]),
> > > > +				be32_to_cpu(voltage_ranges[j + 1]));
> > > > +		if (!mask) {
> > > > +			dev_info(&pdev->dev,
> > > > +				"OF: false voltage-ranges specified\n");
> > > > +			return;
> > > > +		}
> > > > +		host->ocr_mask |=3D mask;
> > > > +	}
> > > > +}
> > >
> > > Don't duplicate this code.  Move it somewhere common and share it.
> > [Haijun Wrote:] So, move it drivers/mmc/host/sdhci-pltfm.c and share
> > it as Sdhc_get_voltage()....?
>=20
> I'll let the MMC maintainer say what the appropriate place would be...
> Don't capitalize the function name, though. :-)
[Haijun Wrote:] Thanks scott. I'm always expecting chris's advice.
>=20
> -Scott

Re: [PATCH 2/2] mmc: esdhc: get voltage from dts file

[Anton Vorontsov]

On Mon, Jul 22, 2013 at 09:41:34PM -0500, Scott Wood wrote:
[...]
> >> > +static void esdhc_get_voltage(struct sdhci_host *host,
> >> > +			struct platform_device *pdev)
> >> > +{
....
> >> > +}
> >>
> >> Don't duplicate this code.  Move it somewhere common and share it.
> >[Haijun Wrote:] So, move it drivers/mmc/host/sdhci-pltfm.c and
> >share it as
> >Sdhc_get_voltage()....?
> 
> I'll let the MMC maintainer say what the appropriate place would
> be...  Don't capitalize the function name, though. :-)

Somewhere in drivers/mmc/core/core.c, near mmc_vddrange_to_ocrmask() would
be most appropriate, IMO. #ifdef CONFIG_OF would be needed, though.

Thanks,

Anton

RE: [PATCH 1/2] Powerpc: Add voltage ranges support for T4

[Wrobel Heinz-R39252]

> Subject: [PATCH 1/2] Powerpc: Add voltage ranges support for T4
>=20
> Special voltages that can be support by eSDHC of T4 in esdhc node.
>=20
> Signed-off-by: Haijun Zhang <haijun.zhang@freescale.com>
> Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>

> --- a/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
> +++ b/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
> @@ -19,6 +19,8 @@ Optional properties:
>      "bus-width =3D <1>" property.
>    - sdhci,auto-cmd12: specifies that a controller can only handle auto
>      CMD12.
> +  - 3300 3300: specifies that eSDHC controller can support voltages
> ranges
> +    from 3300 to 3300. This is an optional.

"This is an optional." is an unclear statement.

> +++ b/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
> @@ -399,6 +399,7 @@
>  	sdhc@114000 {
>  		compatible =3D "fsl,t4240-esdhc", "fsl,esdhc";
>  		sdhci,auto-cmd12;
> +		voltage-ranges =3D <1800 1800 3300 3300>;

This is IMHO incorrect and potentially dangerous.
The T4 silicon will only support 1.8V on SDHC pins per hardware specificati=
on.
The Freescale T4240QDS reference board has extra voltage shifters added to =
allow 3.3V operation, but that is _not_ a silicon feature. It is a specific=
 board feature that may or may not translate to other boards, depending on =
how SD spec conformant a board builder wants to be.

If the intent is to state that a physical SDHC interface on a board has to =
be built to support 3.3V operation to be SD spec conformant for off-the-she=
lf cards because a reset would change the signal voltage to 3.3V, then I am=
 not sure that putting this down as silicon "feature" without further expla=
nation about the background anywhere is the right way to go.
IMHO silicon features are really just silicon features and not technically =
optional external circuitry additions implied by common use.

Best regards,

Heinz

Re: [PATCH 1/2] Powerpc: Add voltage ranges support for T4

[Kumar Gala]

On Jul 22, 2013, at 4:47 AM, Wrobel Heinz-R39252 wrote:

>> Subject: [PATCH 1/2] Powerpc: Add voltage ranges support for T4
>>=20
>> Special voltages that can be support by eSDHC of T4 in esdhc node.
>>=20
>> Signed-off-by: Haijun Zhang <haijun.zhang@freescale.com>
>> Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
>=20
>> --- a/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
>> @@ -19,6 +19,8 @@ Optional properties:
>>     "bus-width =3D <1>" property.
>>   - sdhci,auto-cmd12: specifies that a controller can only handle =
auto
>>     CMD12.
>> +  - 3300 3300: specifies that eSDHC controller can support voltages
>> ranges
>> +    from 3300 to 3300. This is an optional.
>=20
> "This is an optional." is an unclear statement.
>=20
>> +++ b/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
>> @@ -399,6 +399,7 @@
>> 	sdhc@114000 {
>> 		compatible =3D "fsl,t4240-esdhc", "fsl,esdhc";
>> 		sdhci,auto-cmd12;
>> +		voltage-ranges =3D <1800 1800 3300 3300>;
>=20
> This is IMHO incorrect and potentially dangerous.
> The T4 silicon will only support 1.8V on SDHC pins per hardware =
specification.
> The Freescale T4240QDS reference board has extra voltage shifters =
added to allow 3.3V operation, but that is _not_ a silicon feature. It =
is a specific board feature that may or may not translate to other =
boards, depending on how SD spec conformant a board builder wants to be.
>=20
> If the intent is to state that a physical SDHC interface on a board =
has to be built to support 3.3V operation to be SD spec conformant for =
off-the-shelf cards because a reset would change the signal voltage to =
3.3V, then I am not sure that putting this down as silicon "feature" =
without further explanation about the background anywhere is the right =
way to go.
> IMHO silicon features are really just silicon features and not =
technically optional external circuitry additions implied by common use.
>=20
> Best regards,
>=20
> Heinz

I'd say that the t4240si-post.dtsi should be:

	voltage-ranges =3D <1800 1800>;

Than have the t4240qds.dts do:

	voltage-ranges =3D <1800 1800 3300 3300>;

As the 3.3V sounds like a board specific feature.

[ send this as 2 patches, on for the t4240si-post.dtsi and another for =
the t4240qds.dts ]

- k
=09=

RE: [PATCH 1/2] Powerpc: Add voltage ranges support for T4

[Zhang Haijun-B42677]

Thanks.

Regards
Haijun.


> -----Original Message-----
> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> Sent: Monday, July 22, 2013 10:40 PM
> To: Wrobel Heinz-R39252
> Cc: Zhang Haijun-B42677; linux-mmc@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; Wood Scott-B07421; cjb@laptop.org; Fleming Andy-
> AFLEMING; cbouatmailru@gmail.com
> Subject: Re: [PATCH 1/2] Powerpc: Add voltage ranges support for T4
>=20
>=20
> On Jul 22, 2013, at 4:47 AM, Wrobel Heinz-R39252 wrote:
>=20
> >> Subject: [PATCH 1/2] Powerpc: Add voltage ranges support for T4
> >>
> >> Special voltages that can be support by eSDHC of T4 in esdhc node.
> >>
> >> Signed-off-by: Haijun Zhang <haijun.zhang@freescale.com>
> >> Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> >
> >> --- a/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
> >> +++ b/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
> >> @@ -19,6 +19,8 @@ Optional properties:
> >>     "bus-width =3D <1>" property.
> >>   - sdhci,auto-cmd12: specifies that a controller can only handle auto
> >>     CMD12.
> >> +  - 3300 3300: specifies that eSDHC controller can support voltages
> >> ranges
> >> +    from 3300 to 3300. This is an optional.
> >
> > "This is an optional." is an unclear statement.
> >
> >> +++ b/arch/powerpc/boot/dts/fsl/t4240si-post.dtsi
> >> @@ -399,6 +399,7 @@
> >> 	sdhc@114000 {
> >> 		compatible =3D "fsl,t4240-esdhc", "fsl,esdhc";
> >> 		sdhci,auto-cmd12;
> >> +		voltage-ranges =3D <1800 1800 3300 3300>;
> >
> > This is IMHO incorrect and potentially dangerous.
> > The T4 silicon will only support 1.8V on SDHC pins per hardware
> specification.
> > The Freescale T4240QDS reference board has extra voltage shifters added
> to allow 3.3V operation, but that is _not_ a silicon feature. It is a
> specific board feature that may or may not translate to other boards,
> depending on how SD spec conformant a board builder wants to be.
> >
> > If the intent is to state that a physical SDHC interface on a board has
> to be built to support 3.3V operation to be SD spec conformant for off-
> the-shelf cards because a reset would change the signal voltage to 3.3V,
> then I am not sure that putting this down as silicon "feature" without
> further explanation about the background anywhere is the right way to go.
> > IMHO silicon features are really just silicon features and not
> technically optional external circuitry additions implied by common use.
> >
> > Best regards,
> >
> > Heinz
>=20
> I'd say that the t4240si-post.dtsi should be:
>=20
> 	voltage-ranges =3D <1800 1800>;
>=20
> Than have the t4240qds.dts do:
>=20
> 	voltage-ranges =3D <1800 1800 3300 3300>;
>=20
> As the 3.3V sounds like a board specific feature.
>=20
> [ send this as 2 patches, on for the t4240si-post.dtsi and another for
> the t4240qds.dts ]
[Haijun Wrote:] ok, thanks Heinz and Kumar.
>=20
> - k
>=20