diff mbox

[U-Boot,2/2] rockchip: mmc: handle deprecation of 'clock-freq-min-max'

Message ID 1493106727-53800-3-git-send-email-philipp.tomsich@theobroma-systems.com
State Accepted
Commit ff71f9ac33369dbe88f5a492af21c7c605a0569d
Delegated to: Simon Glass
Headers show

Commit Message

Philipp Tomsich April 25, 2017, 7:52 a.m. UTC 
The 'clock-freq-min-max' property was deprecated in the upstream
(i.e. Linux) DTS bindings in favor of the 'max-frequency' property.

With the latest RK3399 DTSI does no longer include the deprecated
property and the rockchip_dw_mmc driver requiring it to be present,
the driver doesn't bind to the node in the RK3399 DTSI any longer
(thus breaking access to the SD card on the RK3399-Q7 board).

To fix this, we implement a similar logic as in the Linux driver: if
the deprecated property is present, we issue a warning (if DEBUG is
enabled); if it is missing, we require 'max-frequency' to be set and
use it to create a min/max value-pair.

See https://github.com/torvalds/linux/commit/b023030f10573de738bbe8df63d43acab64c9f7b
for the deprecation/matching change in Linux.

Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---

 drivers/mmc/rockchip_dw_mmc.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Comments

Jaehoon Chung April 27, 2017, 4:21 a.m. UTC | #1 
On 04/25/2017 04:52 PM, Philipp Tomsich wrote:
> The 'clock-freq-min-max' property was deprecated in the upstream
> (i.e. Linux) DTS bindings in favor of the 'max-frequency' property.

It's difference wit Linux kernel. "clock-freq-min-max" was deprecated in Linux.
Linux kernel is supporting to find the best clock value in core.c.

There are defined the freqs[] = {400000, ...., 100000}
then it should be looped to find the value from 400K to 100K.

As i know, u-boot doesn't support this..
If you set to min value as 400K..doesn't check the other values..

Best Regards,
Jaehoon Chung

> 
> With the latest RK3399 DTSI does no longer include the deprecated
> property and the rockchip_dw_mmc driver requiring it to be present,
> the driver doesn't bind to the node in the RK3399 DTSI any longer
> (thus breaking access to the SD card on the RK3399-Q7 board).
> 
> To fix this, we implement a similar logic as in the Linux driver: if
> the deprecated property is present, we issue a warning (if DEBUG is
> enabled); if it is missing, we require 'max-frequency' to be set and
> use it to create a min/max value-pair.
> 
> See https://github.com/torvalds/linux/commit/b023030f10573de738bbe8df63d43acab64c9f7b
> for the deprecation/matching change in Linux.
> 
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> ---
> 
>  drivers/mmc/rockchip_dw_mmc.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
> index c36eda0..432ae20 100644
> --- a/drivers/mmc/rockchip_dw_mmc.c
> +++ b/drivers/mmc/rockchip_dw_mmc.c
> @@ -76,9 +76,25 @@ static int rockchip_dwmmc_ofdata_to_platdata(struct udevice *dev)
>  		return -EINVAL;
>  	priv->fifo_mode = fdtdec_get_bool(gd->fdt_blob, dev_of_offset(dev),
>  					  "fifo-mode");
> +
> +	/*
> +	 * 'clock-freq-min-max' is deprecated
> +	 * (see https://github.com/torvalds/linux/commit/b023030f10573de738bbe8df63d43acab64c9f7b)
> +	 */
>  	if (fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev),
> -				 "clock-freq-min-max", priv->minmax, 2))
> -		return -EINVAL;
> +				 "clock-freq-min-max", priv->minmax, 2)) {
> +		int val = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
> +					  "max-frequency", -EINVAL);
> +
> +		if (val < 0)
> +			return val;
> +
> +		priv->minmax[0] = 400000;  /* 400 kHz */
> +		priv->minmax[1] = val;
> +	} else {
> +		debug("%s: 'clock-freq-min-max' property was deprecated.\n",
> +		      __func__);
> +	}
>  #endif
>  	return 0;
>  }
>
Philipp Tomsich April 27, 2017, 7:50 a.m. UTC | #2 
I am aware of this kernel behaviour, but felt that it does not directly apply for
this specific change, as the original DTS (before the deprecation of this property)
had defined the minimum frequency for the affected chips as <400000>.

For reference, here’s the DTS change that this is tracking:
-               clock-freq-min-max = <400000 150000000>;
+               max-frequency = <150000000>;

During the init-op, the following code in dw_mmc.c ensures that we are always
enumerating at f_min (which is always 400kHz with this driver):
        /* Enumerate at 400KHz */
      	dwmci_setup_bus(host, mmc->cfg->f_min);
So the UBoot MMC-core relies on the drivers to select a single frequency.

In other words: this commit does not change the current state of affairs, as
all affected devices (i.e. those that the rockchip-dw-mmc binds to) already
have the 400kHz f_min set.

If you feel that this it is worthwhile, we can also change mmc/mmc.c to not
just rely on the init-op, but explicitly set the frequency and then iterate over
the 400KHz … 100kHz array.
However, this would then affect all mmc-drivers, so I would rather keep this
as a separate change that allows everyone to test on their boards.

Best regards,
Philipp.


> On 27 Apr 2017, at 06:21, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> 
> On 04/25/2017 04:52 PM, Philipp Tomsich wrote:
>> The 'clock-freq-min-max' property was deprecated in the upstream
>> (i.e. Linux) DTS bindings in favor of the 'max-frequency' property.
> 
> It's difference wit Linux kernel. "clock-freq-min-max" was deprecated in Linux.
> Linux kernel is supporting to find the best clock value in core.c.
> 
> There are defined the freqs[] = {400000, ...., 100000}
> then it should be looped to find the value from 400K to 100K.
> 
> As i know, u-boot doesn't support this..
> If you set to min value as 400K..doesn't check the other values..
> 
> Best Regards,
> Jaehoon Chung
> 
>> 
>> With the latest RK3399 DTSI does no longer include the deprecated
>> property and the rockchip_dw_mmc driver requiring it to be present,
>> the driver doesn't bind to the node in the RK3399 DTSI any longer
>> (thus breaking access to the SD card on the RK3399-Q7 board).
>> 
>> To fix this, we implement a similar logic as in the Linux driver: if
>> the deprecated property is present, we issue a warning (if DEBUG is
>> enabled); if it is missing, we require 'max-frequency' to be set and
>> use it to create a min/max value-pair.
>> 
>> See https://github.com/torvalds/linux/commit/b023030f10573de738bbe8df63d43acab64c9f7b
>> for the deprecation/matching change in Linux.
>> 
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>> 
>> drivers/mmc/rockchip_dw_mmc.c | 20 ++++++++++++++++++--
>> 1 file changed, 18 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
>> index c36eda0..432ae20 100644
>> --- a/drivers/mmc/rockchip_dw_mmc.c
>> +++ b/drivers/mmc/rockchip_dw_mmc.c
>> @@ -76,9 +76,25 @@ static int rockchip_dwmmc_ofdata_to_platdata(struct udevice *dev)
>> 		return -EINVAL;
>> 	priv->fifo_mode = fdtdec_get_bool(gd->fdt_blob, dev_of_offset(dev),
>> 					  "fifo-mode");
>> +
>> +	/*
>> +	 * 'clock-freq-min-max' is deprecated
>> +	 * (see https://github.com/torvalds/linux/commit/b023030f10573de738bbe8df63d43acab64c9f7b)
>> +	 */
>> 	if (fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev),
>> -				 "clock-freq-min-max", priv->minmax, 2))
>> -		return -EINVAL;
>> +				 "clock-freq-min-max", priv->minmax, 2)) {
>> +		int val = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
>> +					  "max-frequency", -EINVAL);
>> +
>> +		if (val < 0)
>> +			return val;
>> +
>> +		priv->minmax[0] = 400000;  /* 400 kHz */
>> +		priv->minmax[1] = val;
>> +	} else {
>> +		debug("%s: 'clock-freq-min-max' property was deprecated.\n",
>> +		      __func__);
>> +	}
>> #endif
>> 	return 0;
>> }
>> 
>
Kever Yang April 28, 2017, 7:24 a.m. UTC | #3 
Hi Jaehoon,


On 04/27/2017 12:21 PM, Jaehoon Chung wrote:
> On 04/25/2017 04:52 PM, Philipp Tomsich wrote:
>> The 'clock-freq-min-max' property was deprecated in the upstream
>> (i.e. Linux) DTS bindings in favor of the 'max-frequency' property.
> It's difference wit Linux kernel. "clock-freq-min-max" was deprecated in Linux.
> Linux kernel is supporting to find the best clock value in core.c.

We need to sync the dts definition with kernel, because we might using
just the dts from kernel without modification in the future.
I'm not sure Philipp's patch is correct or not, but we can figure it 
out, right?

Thanks,
- Kever
>
> There are defined the freqs[] = {400000, ...., 100000}
> then it should be looped to find the value from 400K to 100K.
>
> As i know, u-boot doesn't support this..
> If you set to min value as 400K..doesn't check the other values..
>
> Best Regards,
> Jaehoon Chung
>
>> With the latest RK3399 DTSI does no longer include the deprecated
>> property and the rockchip_dw_mmc driver requiring it to be present,
>> the driver doesn't bind to the node in the RK3399 DTSI any longer
>> (thus breaking access to the SD card on the RK3399-Q7 board).
>>
>> To fix this, we implement a similar logic as in the Linux driver: if
>> the deprecated property is present, we issue a warning (if DEBUG is
>> enabled); if it is missing, we require 'max-frequency' to be set and
>> use it to create a min/max value-pair.
>>
>> See https://github.com/torvalds/linux/commit/b023030f10573de738bbe8df63d43acab64c9f7b
>> for the deprecation/matching change in Linux.
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>>
>>   drivers/mmc/rockchip_dw_mmc.c | 20 ++++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
>> index c36eda0..432ae20 100644
>> --- a/drivers/mmc/rockchip_dw_mmc.c
>> +++ b/drivers/mmc/rockchip_dw_mmc.c
>> @@ -76,9 +76,25 @@ static int rockchip_dwmmc_ofdata_to_platdata(struct udevice *dev)
>>   		return -EINVAL;
>>   	priv->fifo_mode = fdtdec_get_bool(gd->fdt_blob, dev_of_offset(dev),
>>   					  "fifo-mode");
>> +
>> +	/*
>> +	 * 'clock-freq-min-max' is deprecated
>> +	 * (see https://github.com/torvalds/linux/commit/b023030f10573de738bbe8df63d43acab64c9f7b)
>> +	 */
>>   	if (fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev),
>> -				 "clock-freq-min-max", priv->minmax, 2))
>> -		return -EINVAL;
>> +				 "clock-freq-min-max", priv->minmax, 2)) {
>> +		int val = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
>> +					  "max-frequency", -EINVAL);
>> +
>> +		if (val < 0)
>> +			return val;
>> +
>> +		priv->minmax[0] = 400000;  /* 400 kHz */
>> +		priv->minmax[1] = val;
>> +	} else {
>> +		debug("%s: 'clock-freq-min-max' property was deprecated.\n",
>> +		      __func__);
>> +	}
>>   #endif
>>   	return 0;
>>   }
>>
>
Simon Glass May 2, 2017, 11:12 a.m. UTC | #4 
Hi Jaehoon,


On 04/27/2017 12:21 PM, Jaehoon Chung wrote:
> On 04/25/2017 04:52 PM, Philipp Tomsich wrote:
>> The 'clock-freq-min-max' property was deprecated in the upstream
>> (i.e. Linux) DTS bindings in favor of the 'max-frequency' property.
> It's difference wit Linux kernel. "clock-freq-min-max" was deprecated in Linux.
> Linux kernel is supporting to find the best clock value in core.c.

We need to sync the dts definition with kernel, because we might using
just the dts from kernel without modification in the future.
I'm not sure Philipp's patch is correct or not, but we can figure it
out, right?

Thanks,
- Kever
>
> There are defined the freqs[] = {400000, ...., 100000}
> then it should be looped to find the value from 400K to 100K.
>
> As i know, u-boot doesn't support this..
> If you set to min value as 400K..doesn't check the other values..
>
> Best Regards,
> Jaehoon Chung
>
>> With the latest RK3399 DTSI does no longer include the deprecated
>> property and the rockchip_dw_mmc driver requiring it to be present,
>> the driver doesn't bind to the node in the RK3399 DTSI any longer
>> (thus breaking access to the SD card on the RK3399-Q7 board).
>>
>> To fix this, we implement a similar logic as in the Linux driver: if
>> the deprecated property is present, we issue a warning (if DEBUG is
>> enabled); if it is missing, we require 'max-frequency' to be set and
>> use it to create a min/max value-pair.
>>
>> See https://github.com/torvalds/linux/commit/b023030f10573de738bbe8df63d43acab64c9f7b
>> for the deprecation/matching change in Linux.
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> ---
>>
>>   drivers/mmc/rockchip_dw_mmc.c | 20 ++++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
Applied to u-boot-rockchip/next, thanks!
diff mbox

Patch

diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
index c36eda0..432ae20 100644
--- a/drivers/mmc/rockchip_dw_mmc.c
+++ b/drivers/mmc/rockchip_dw_mmc.c
@@ -76,9 +76,25 @@  static int rockchip_dwmmc_ofdata_to_platdata(struct udevice *dev)
 		return -EINVAL;
 	priv->fifo_mode = fdtdec_get_bool(gd->fdt_blob, dev_of_offset(dev),
 					  "fifo-mode");
+
+	/*
+	 * 'clock-freq-min-max' is deprecated
+	 * (see https://github.com/torvalds/linux/commit/b023030f10573de738bbe8df63d43acab64c9f7b)
+	 */
 	if (fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev),
-				 "clock-freq-min-max", priv->minmax, 2))
-		return -EINVAL;
+				 "clock-freq-min-max", priv->minmax, 2)) {
+		int val = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
+					  "max-frequency", -EINVAL);
+
+		if (val < 0)
+			return val;
+
+		priv->minmax[0] = 400000;  /* 400 kHz */
+		priv->minmax[1] = val;
+	} else {
+		debug("%s: 'clock-freq-min-max' property was deprecated.\n",
+		      __func__);
+	}
 #endif
 	return 0;
 }