[PATCHv6 0/5] socfpga: Enable SD/MMC support

[PATCHv6 0/5] socfpga: Enable SD/MMC support

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Hi,

This is v6 of the patch series to enable SD/MMC on the SOCFPGA platform.

V6 differences from V5:

* Adds a new patch to the series.
  [mmc: dw_mmc: Add support to set the SDR and DDR timing through clock framework]
  This patch enables the setting for the SDR and DDR setting through the common
  clock framework. This patch can be independently applied on its own. I lumped
  it into this series to show how the SOCFPGA can make use of it.

* Add a new clock type "altr,socfpga-sdmmc-sdr-clk" to the SOCFPGA clock driver.
  This clock type implements the clk_set_rate function which can set the SD/MMC's
  clock phase settings. Can now use the syscon driver in the set_rate function.

* Will have to CC DTS Maintainers as I am adding a new binding to the clock
  driver.

Thanks,

Dinh Nguyen (5):
  mmc: dw_mmc: Add support to set the SDR and DDR timing through clock
    framework
  clk: socfpga: Add a clock type for the SD/MMC driver
  dts: socfpga: Add support for SD/MMC on the SOCFPGA platform
  mmc: dw_mmc-socfpga: Remove the SOCFPGA specific platform for dw_mmc
  ARM: socfpga_defconfig: enable SD/MMC support

 .../devicetree/bindings/clock/altr_socfpga.txt     |   11 +-
 arch/arm/boot/dts/socfpga.dtsi                     |   19 ++-
 arch/arm/boot/dts/socfpga_arria5.dtsi              |   12 ++
 arch/arm/boot/dts/socfpga_cyclone5.dtsi            |   12 ++
 arch/arm/boot/dts/socfpga_vt.dts                   |   12 ++
 arch/arm/configs/socfpga_defconfig                 |    2 +
 drivers/clk/socfpga/clk.c                          |   86 ++++++++++++
 drivers/mmc/host/Kconfig                           |    8 --
 drivers/mmc/host/dw_mmc-socfpga.c                  |  138 --------------------
 drivers/mmc/host/dw_mmc.c                          |   23 ++++
 include/linux/mmc/dw_mmc.h                         |    6 +
 11 files changed, 180 insertions(+), 149 deletions(-)
 delete mode 100644 drivers/mmc/host/dw_mmc-socfpga.c

-- 
1.7.9.5

[PATCHv6 1/5] mmc: dw_mmc: Add support to set the SDR and DDR timing through clock framework

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

All implementations of the Synopsys DW SD/MMC IP have settings to control
the phase shift of the CIU clk. These phase shift settings are necessary for
the SD/MMC to correctly clock the card. All variants of the dw_mmc will need
these settings, but how they are implemented can vastly vary.

This patch enables the setting for the SDR and/or DDR settings through the
common clock framework.

Depends on the patch "mmc: dw_mmc: Enable the hold reg for certain speed modes",
that is already reading the "samsung,dw-mshc-sdr-timing" and
"samsung,dw-mshc-ddr-timing" bindings, this patch saves those values into a
u32 bitmask that can be passed to the clock framework to use.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
v6: Add this patch
v5: none
v4: none
v3: none
v2: none
---
 drivers/mmc/host/dw_mmc.c  |   23 +++++++++++++++++++++++
 include/linux/mmc/dw_mmc.h |    6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 480dafe..1fb5cff 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2431,6 +2431,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 	if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
 		pdata->cclk_in_drv = 0;
 
+	pdata->sdr_timing = (sdr_timing[1] | (sdr_timing[0] << 4));
+	pdata->ddr_timing = (ddr_timing[1] | (ddr_timing[0] << 4));
 	return pdata;
 }
 
@@ -2478,6 +2480,27 @@ int dw_mci_probe(struct dw_mci *host)
 		dev_dbg(host->dev, "ciu clock not available\n");
 		host->bus_hz = host->pdata->bus_hz;
 	} else {
+		/* If the CIU clk is available, we check for SDR and DDR
+		 * timing phase shift settings. But not all platforms
+		 * may have populated these settings, the driver will not fail
+		 * if these settings are not specified.
+		 */
+		if (host->pdata->sdr_timing) {
+			host->sdr_clk = devm_clk_get(host->dev, "sdr_mmc_clk");
+			if (IS_ERR(host->sdr_clk))
+				dev_dbg(host->dev, "sdr_mmc clock not available\n");
+			else
+				clk_set_rate(host->sdr_clk, host->pdata->sdr_timing);
+		}
+
+		if (host->pdata->ddr_timing) {
+			host->ddr_clk = devm_clk_get(host->dev, "ddr_mmc_clk");
+			if (IS_ERR(host->ddr_clk))
+				dev_dbg(host->dev, "ddr_mmc clock not available\n");
+			else
+				clk_set_rate(host->ddr_clk, host->pdata->ddr_timing);
+		}
+
 		ret = clk_prepare_enable(host->ciu_clk);
 		if (ret) {
 			dev_err(host->dev, "failed to enable ciu clock\n");
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 2b5b8bf..ad90ad1 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -83,6 +83,8 @@ struct mmc_data;
  * @priv: Implementation defined private data.
  * @biu_clk: Pointer to bus interface unit clock instance.
  * @ciu_clk: Pointer to card interface unit clock instance.
+ * @sdr_clk: Pointer to the SDR clock instance.
+ * @ddr_clk: Pointer to the DDR clock instance.
  * @slot: Slots sharing this MMC controller.
  * @fifo_depth: depth of FIFO.
  * @data_shift: log2 of FIFO item size.
@@ -170,6 +172,8 @@ struct dw_mci {
 	void			*priv;
 	struct clk		*biu_clk;
 	struct clk		*ciu_clk;
+	struct clk		*sdr_clk;
+	struct clk		*ddr_clk;
 	struct dw_mci_slot	*slot[MAX_MCI_SLOTS];
 
 	/* FIFO push and pull */
@@ -241,6 +245,8 @@ struct dw_mci_board {
 	u32 caps2;	/* More capabilities */
 	u32 pm_caps;	/* PM capabilities */
 	u32 cclk_in_drv;	/*cclk_in_drv phase shift */
+	u32 sdr_timing;	/* Single data rate timing setting. */
+	u32 ddr_timing;	/* Double data rate timing setting. */
 	/*
 	 * Override fifo depth. If 0, autodetect it from the FIFOTH register,
 	 * but note that this may not be reliable after a bootloader has used
-- 
1.7.9.5

Re: [PATCHv6 1/5] mmc: dw_mmc: Add support to set the SDR and DDR timing through clock framework

[zhangfei]

Dear Dinh

On 12/13/2013 04:30 AM, dinguyen@altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
>
> All implementations of the Synopsys DW SD/MMC IP have settings to control
> the phase shift of the CIU clk. These phase shift settings are necessary for
> the SD/MMC to correctly clock the card. All variants of the dw_mmc will need
> these settings, but how they are implemented can vastly vary.
>
> This patch enables the setting for the SDR and/or DDR settings through the
> common clock framework.
>
> Depends on the patch "mmc: dw_mmc: Enable the hold reg for certain speed modes",
> that is already reading the "samsung,dw-mshc-sdr-timing" and
> "samsung,dw-mshc-ddr-timing" bindings, this patch saves those values into a
> u32 bitmask that can be passed to the clock framework to use.
>
> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> ---
> v6: Add this patch
> v5: none
> v4: none
> v3: none
> v2: none
> ---
>   drivers/mmc/host/dw_mmc.c  |   23 +++++++++++++++++++++++
>   include/linux/mmc/dw_mmc.h |    6 ++++++
>   2 files changed, 29 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 480dafe..1fb5cff 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2431,6 +2431,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>   	if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>   		pdata->cclk_in_drv = 0;
>
> +	pdata->sdr_timing = (sdr_timing[1] | (sdr_timing[0] << 4));
> +	pdata->ddr_timing = (ddr_timing[1] | (ddr_timing[0] << 4));
>   	return pdata;
>   }
>
> @@ -2478,6 +2480,27 @@ int dw_mci_probe(struct dw_mci *host)
>   		dev_dbg(host->dev, "ciu clock not available\n");
>   		host->bus_hz = host->pdata->bus_hz;
>   	} else {
> +		/* If the CIU clk is available, we check for SDR and DDR
> +		 * timing phase shift settings. But not all platforms
> +		 * may have populated these settings, the driver will not fail
> +		 * if these settings are not specified.
> +		 */
> +		if (host->pdata->sdr_timing) {
> +			host->sdr_clk = devm_clk_get(host->dev, "sdr_mmc_clk");
> +			if (IS_ERR(host->sdr_clk))
> +				dev_dbg(host->dev, "sdr_mmc clock not available\n");
> +			else
> +				clk_set_rate(host->sdr_clk, host->pdata->sdr_timing);
> +		}
> +
> +		if (host->pdata->ddr_timing) {
> +			host->ddr_clk = devm_clk_get(host->dev, "ddr_mmc_clk");
> +			if (IS_ERR(host->ddr_clk))
> +				dev_dbg(host->dev, "ddr_mmc clock not available\n");
> +			else
> +				clk_set_rate(host->ddr_clk, host->pdata->ddr_timing);
> +		}
> +

Just curious, does pdata->sdr_timing and pdata->ddr_timing are fixed, 
fixed as each controller, or different with different board.
In case they are fixed, or fixed in each controller, and only required 
to be set in probe only once, maybe they can be hide in clk_mmc_ops.prepare
And the clock can be used as ciu_clock, or parent of ciu_clock, which is 
called in dw_mmc.c, maybe these code can be ignored.



>   		ret = clk_prepare_enable(host->ciu_clk);
>   		if (ret) {
>   			dev_err(host->dev, "failed to enable ciu clock\n");
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 2b5b8bf..ad90ad1 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -83,6 +83,8 @@ struct mmc_data;
>    * @priv: Implementation defined private data.
>    * @biu_clk: Pointer to bus interface unit clock instance.
>    * @ciu_clk: Pointer to card interface unit clock instance.
> + * @sdr_clk: Pointer to the SDR clock instance.
> + * @ddr_clk: Pointer to the DDR clock instance.
>    * @slot: Slots sharing this MMC controller.
>    * @fifo_depth: depth of FIFO.
>    * @data_shift: log2 of FIFO item size.
> @@ -170,6 +172,8 @@ struct dw_mci {
>   	void			*priv;
>   	struct clk		*biu_clk;
>   	struct clk		*ciu_clk;
> +	struct clk		*sdr_clk;
> +	struct clk		*ddr_clk;
>   	struct dw_mci_slot	*slot[MAX_MCI_SLOTS];
>
>   	/* FIFO push and pull */
> @@ -241,6 +245,8 @@ struct dw_mci_board {
>   	u32 caps2;	/* More capabilities */
>   	u32 pm_caps;	/* PM capabilities */
>   	u32 cclk_in_drv;	/*cclk_in_drv phase shift */
> +	u32 sdr_timing;	/* Single data rate timing setting. */
> +	u32 ddr_timing;	/* Double data rate timing setting. */

Are they fixed or not?

Thanks
Zhangfei

Re: [PATCHv6 1/5] mmc: dw_mmc: Add support to set the SDR and DDR timing through clock framework

[Dinh Nguyen]

Hi Zhangfei,

On 12/14/13 8:05 PM, zhangfei wrote:
> Dear Dinh
>
> On 12/13/2013 04:30 AM, dinguyen@altera.com wrote:
>> From: Dinh Nguyen <dinguyen@altera.com>
>>
>> All implementations of the Synopsys DW SD/MMC IP have settings to
>> control
>> the phase shift of the CIU clk. These phase shift settings are
>> necessary for
>> the SD/MMC to correctly clock the card. All variants of the dw_mmc
>> will need
>> these settings, but how they are implemented can vastly vary.
>>
>> This patch enables the setting for the SDR and/or DDR settings
>> through the
>> common clock framework.
>>
>> Depends on the patch "mmc: dw_mmc: Enable the hold reg for certain
>> speed modes",
>> that is already reading the "samsung,dw-mshc-sdr-timing" and
>> "samsung,dw-mshc-ddr-timing" bindings, this patch saves those values
>> into a
>> u32 bitmask that can be passed to the clock framework to use.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
>> ---
>> v6: Add this patch
>> v5: none
>> v4: none
>> v3: none
>> v2: none
>> ---
>>   drivers/mmc/host/dw_mmc.c  |   23 +++++++++++++++++++++++
>>   include/linux/mmc/dw_mmc.h |    6 ++++++
>>   2 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 480dafe..1fb5cff 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -2431,6 +2431,8 @@ static struct dw_mci_board
>> *dw_mci_parse_dt(struct dw_mci *host)
>>       if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>>           pdata->cclk_in_drv = 0;
>>
>> +    pdata->sdr_timing = (sdr_timing[1] | (sdr_timing[0] << 4));
>> +    pdata->ddr_timing = (ddr_timing[1] | (ddr_timing[0] << 4));
>>       return pdata;
>>   }
>>
>> @@ -2478,6 +2480,27 @@ int dw_mci_probe(struct dw_mci *host)
>>           dev_dbg(host->dev, "ciu clock not available\n");
>>           host->bus_hz = host->pdata->bus_hz;
>>       } else {
>> +        /* If the CIU clk is available, we check for SDR and DDR
>> +         * timing phase shift settings. But not all platforms
>> +         * may have populated these settings, the driver will not fail
>> +         * if these settings are not specified.
>> +         */
>> +        if (host->pdata->sdr_timing) {
>> +            host->sdr_clk = devm_clk_get(host->dev, "sdr_mmc_clk");
>> +            if (IS_ERR(host->sdr_clk))
>> +                dev_dbg(host->dev, "sdr_mmc clock not available\n");
>> +            else
>> +                clk_set_rate(host->sdr_clk, host->pdata->sdr_timing);
>> +        }
>> +
>> +        if (host->pdata->ddr_timing) {
>> +            host->ddr_clk = devm_clk_get(host->dev, "ddr_mmc_clk");
>> +            if (IS_ERR(host->ddr_clk))
>> +                dev_dbg(host->dev, "ddr_mmc clock not available\n");
>> +            else
>> +                clk_set_rate(host->ddr_clk, host->pdata->ddr_timing);
>> +        }
>> +
>
> Just curious, does pdata->sdr_timing and pdata->ddr_timing are fixed,
> fixed as each controller, or different with different board.
Yes, they are fixed for each controller per SoC.
> In case they are fixed, or fixed in each controller, and only required
> to be set in probe only once, maybe they can be hide in
> clk_mmc_ops.prepare
> And the clock can be used as ciu_clock, or parent of ciu_clock, which
> is called in dw_mmc.c, maybe these code can be ignored.
>
Please see v5 of this patch: https://patchwork.kernel.org/patch/3311121/
The issue with V5 was that there is no way for the dw_mmc driver to pass
the clock phase settings
in the clk_mmc_ops.prepare function.
>
>
>>           ret = clk_prepare_enable(host->ciu_clk);
>>           if (ret) {
>>               dev_err(host->dev, "failed to enable ciu clock\n");
>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>> index 2b5b8bf..ad90ad1 100644
>> --- a/include/linux/mmc/dw_mmc.h
>> +++ b/include/linux/mmc/dw_mmc.h
>> @@ -83,6 +83,8 @@ struct mmc_data;
>>    * @priv: Implementation defined private data.
>>    * @biu_clk: Pointer to bus interface unit clock instance.
>>    * @ciu_clk: Pointer to card interface unit clock instance.
>> + * @sdr_clk: Pointer to the SDR clock instance.
>> + * @ddr_clk: Pointer to the DDR clock instance.
>>    * @slot: Slots sharing this MMC controller.
>>    * @fifo_depth: depth of FIFO.
>>    * @data_shift: log2 of FIFO item size.
>> @@ -170,6 +172,8 @@ struct dw_mci {
>>       void            *priv;
>>       struct clk        *biu_clk;
>>       struct clk        *ciu_clk;
>> +    struct clk        *sdr_clk;
>> +    struct clk        *ddr_clk;
>>       struct dw_mci_slot    *slot[MAX_MCI_SLOTS];
>>
>>       /* FIFO push and pull */
>> @@ -241,6 +245,8 @@ struct dw_mci_board {
>>       u32 caps2;    /* More capabilities */
>>       u32 pm_caps;    /* PM capabilities */
>>       u32 cclk_in_drv;    /*cclk_in_drv phase shift */
>> +    u32 sdr_timing;    /* Single data rate timing setting. */
>> +    u32 ddr_timing;    /* Double data rate timing setting. */
>
> Are they fixed or not?
They are fixed values that is represented in the DTS bindings.

Dinh
>
> Thanks
> Zhangfei

Re: [PATCHv6 1/5] mmc: dw_mmc: Add support to set the SDR and DDR timing through clock framework

[zhangfei]

On 12/15/2013 11:16 AM, Dinh Nguyen wrote:
> Hi Zhangfei,
>
>>> @@ -2478,6 +2480,27 @@ int dw_mci_probe(struct dw_mci *host)
>>>            dev_dbg(host->dev, "ciu clock not available\n");
>>>            host->bus_hz = host->pdata->bus_hz;
>>>        } else {
>>> +        /* If the CIU clk is available, we check for SDR and DDR
>>> +         * timing phase shift settings. But not all platforms
>>> +         * may have populated these settings, the driver will not fail
>>> +         * if these settings are not specified.
>>> +         */
>>> +        if (host->pdata->sdr_timing) {
>>> +            host->sdr_clk = devm_clk_get(host->dev, "sdr_mmc_clk");
>>> +            if (IS_ERR(host->sdr_clk))
>>> +                dev_dbg(host->dev, "sdr_mmc clock not available\n");
>>> +            else
>>> +                clk_set_rate(host->sdr_clk, host->pdata->sdr_timing);
>>> +        }
>>> +
>>> +        if (host->pdata->ddr_timing) {
>>> +            host->ddr_clk = devm_clk_get(host->dev, "ddr_mmc_clk");
>>> +            if (IS_ERR(host->ddr_clk))
>>> +                dev_dbg(host->dev, "ddr_mmc clock not available\n");
>>> +            else
>>> +                clk_set_rate(host->ddr_clk, host->pdata->ddr_timing);
>>> +        }
>>> +
>>
>> Just curious, does pdata->sdr_timing and pdata->ddr_timing are fixed,
>> fixed as each controller, or different with different board.
> Yes, they are fixed for each controller per SoC.
>> In case they are fixed, or fixed in each controller, and only required
>> to be set in probe only once, maybe they can be hide in
>> clk_mmc_ops.prepare
>> And the clock can be used as ciu_clock, or parent of ciu_clock, which
>> is called in dw_mmc.c, maybe these code can be ignored.
>>
> Please see v5 of this patch: https://patchwork.kernel.org/patch/3311121/
> The issue with V5 was that there is no way for the dw_mmc driver to pass
> the clock phase settings
> in the clk_mmc_ops.prepare function.

Personally, I think v5 is simpler :)

Could these fixed para be used as internal para when register.
Like
socfpga_gate_clk_init
rc = of_property_read_u32_array(node, "clk-gate", clk_gate, 2);
         compatible = "altr,socfpga-gate-clk";
	clocks = <&mainclk>;
	div-reg = <0x64 0 2>;
	clk-gate = <0x60 1>;
Could we also define mmc-ciu-clock with para clk-init.

Thanks

Re: [PATCHv6 1/5] mmc: dw_mmc: Add support to set the SDR and DDR timing through clock framework

[Dinh Nguyen]

On 12/14/13 10:37 PM, zhangfei wrote:
>
>
> On 12/15/2013 11:16 AM, Dinh Nguyen wrote:
>> Hi Zhangfei,
>>
>>>> @@ -2478,6 +2480,27 @@ int dw_mci_probe(struct dw_mci *host)
>>>>            dev_dbg(host->dev, "ciu clock not available\n");
>>>>            host->bus_hz = host->pdata->bus_hz;
>>>>        } else {
>>>> +        /* If the CIU clk is available, we check for SDR and DDR
>>>> +         * timing phase shift settings. But not all platforms
>>>> +         * may have populated these settings, the driver will not
>>>> fail
>>>> +         * if these settings are not specified.
>>>> +         */
>>>> +        if (host->pdata->sdr_timing) {
>>>> +            host->sdr_clk = devm_clk_get(host->dev, "sdr_mmc_clk");
>>>> +            if (IS_ERR(host->sdr_clk))
>>>> +                dev_dbg(host->dev, "sdr_mmc clock not available\n");
>>>> +            else
>>>> +                clk_set_rate(host->sdr_clk, host->pdata->sdr_timing);
>>>> +        }
>>>> +
>>>> +        if (host->pdata->ddr_timing) {
>>>> +            host->ddr_clk = devm_clk_get(host->dev, "ddr_mmc_clk");
>>>> +            if (IS_ERR(host->ddr_clk))
>>>> +                dev_dbg(host->dev, "ddr_mmc clock not available\n");
>>>> +            else
>>>> +                clk_set_rate(host->ddr_clk, host->pdata->ddr_timing);
>>>> +        }
>>>> +
>>>
>>> Just curious, does pdata->sdr_timing and pdata->ddr_timing are fixed,
>>> fixed as each controller, or different with different board.
>> Yes, they are fixed for each controller per SoC.
>>> In case they are fixed, or fixed in each controller, and only required
>>> to be set in probe only once, maybe they can be hide in
>>> clk_mmc_ops.prepare
>>> And the clock can be used as ciu_clock, or parent of ciu_clock, which
>>> is called in dw_mmc.c, maybe these code can be ignored.
>>>
>> Please see v5 of this patch: https://patchwork.kernel.org/patch/3311121/
>> The issue with V5 was that there is no way for the dw_mmc driver to pass
>> the clock phase settings
>> in the clk_mmc_ops.prepare function.
>
> Personally, I think v5 is simpler :)
I agree...
>
> Could these fixed para be used as internal para when register.
> Like
> socfpga_gate_clk_init
> rc = of_property_read_u32_array(node, "clk-gate", clk_gate, 2);
>         compatible = "altr,socfpga-gate-clk";
>     clocks = <&mainclk>;
>     div-reg = <0x64 0 2>;
>     clk-gate = <0x60 1>;
> Could we also define mmc-ciu-clock with para clk-init.
Hmm..this was one of Arnd's suggestion. Will this also work for you? If
so then I think I can
proceed down this path, as I think this should be the most direct and
simplest.

Thanks,
Dinh
>
> Thanks

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6 1/5] mmc: dw_mmc: Add support to set the SDR and DDR timing through clock framework

[Zhangfei Gao]

On 16 December 2013 11:24, Dinh Nguyen <dinh.linux@gmail.com> wrote:
>
>>>>> @@ -2478,6 +2480,27 @@ int dw_mci_probe(struct dw_mci *host)
>>>>>            dev_dbg(host->dev, "ciu clock not available\n");
>>>>>            host->bus_hz = host->pdata->bus_hz;
>>>>>        } else {
>>>>> +        /* If the CIU clk is available, we check for SDR and DDR
>>>>> +         * timing phase shift settings. But not all platforms
>>>>> +         * may have populated these settings, the driver will not
>>>>> fail
>>>>> +         * if these settings are not specified.
>>>>> +         */
>>>>> +        if (host->pdata->sdr_timing) {
>>>>> +            host->sdr_clk = devm_clk_get(host->dev, "sdr_mmc_clk");
>>>>> +            if (IS_ERR(host->sdr_clk))
>>>>> +                dev_dbg(host->dev, "sdr_mmc clock not available\n");
>>>>> +            else
>>>>> +                clk_set_rate(host->sdr_clk, host->pdata->sdr_timing);
>>>>> +        }
>>>>> +
>>>>> +        if (host->pdata->ddr_timing) {
>>>>> +            host->ddr_clk = devm_clk_get(host->dev, "ddr_mmc_clk");
>>>>> +            if (IS_ERR(host->ddr_clk))
>>>>> +                dev_dbg(host->dev, "ddr_mmc clock not available\n");
>>>>> +            else
>>>>> +                clk_set_rate(host->ddr_clk, host->pdata->ddr_timing);
>>>>> +        }
>>>>> +
>>>>
>>>> Just curious, does pdata->sdr_timing and pdata->ddr_timing are fixed,
>>>> fixed as each controller, or different with different board.
>>> Yes, they are fixed for each controller per SoC.
>>>> In case they are fixed, or fixed in each controller, and only required
>>>> to be set in probe only once, maybe they can be hide in
>>>> clk_mmc_ops.prepare
>>>> And the clock can be used as ciu_clock, or parent of ciu_clock, which
>>>> is called in dw_mmc.c, maybe these code can be ignored.
>>>>
>>> Please see v5 of this patch: https://patchwork.kernel.org/patch/3311121/
>>> The issue with V5 was that there is no way for the dw_mmc driver to pass
>>> the clock phase settings
>>> in the clk_mmc_ops.prepare function.
>>
>> Personally, I think v5 is simpler :)
> I agree...
>>
>> Could these fixed para be used as internal para when register.
>> Like
>> socfpga_gate_clk_init
>> rc = of_property_read_u32_array(node, "clk-gate", clk_gate, 2);
>>         compatible = "altr,socfpga-gate-clk";
>>     clocks = <&mainclk>;
>>     div-reg = <0x64 0 2>;
>>     clk-gate = <0x60 1>;
>> Could we also define mmc-ciu-clock with para clk-init.
> Hmm..this was one of Arnd's suggestion. Will this also work for you? If
> so then I think I can
> proceed down this path, as I think this should be the most direct and
> simplest.
>

Yes, go ahead.
sdr_timing & ddr_timing only takes effect on socfpga-mmc.
It is good to put in clk driver specifically for socfpga, instead of
putting in common dw_mmc.c

Thanks

RE: [PATCHv6 1/5] mmc: dw_mmc: Add support to set the SDR and DDR timing through clock framework

[Seungwon Jeon]

On Friday, December 13, 2013, Dinh Nguyen wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> All implementations of the Synopsys DW SD/MMC IP have settings to control
> the phase shift of the CIU clk. These phase shift settings are necessary for
> the SD/MMC to correctly clock the card. All variants of the dw_mmc will need
> these settings, but how they are implemented can vastly vary.
> 
> This patch enables the setting for the SDR and/or DDR settings through the
> common clock framework.
> 
> Depends on the patch "mmc: dw_mmc: Enable the hold reg for certain speed modes",
> that is already reading the "samsung,dw-mshc-sdr-timing" and
> "samsung,dw-mshc-ddr-timing" bindings, this patch saves those values into a
> u32 bitmask that can be passed to the clock framework to use.

I didn't understand this patch.
As you mentioned, clock implementation can be different each SOC.
And what is the difference with existing ciu clock?

> 
> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> ---
> v6: Add this patch
> v5: none
> v4: none
> v3: none
> v2: none
> ---
>  drivers/mmc/host/dw_mmc.c  |   23 +++++++++++++++++++++++
>  include/linux/mmc/dw_mmc.h |    6 ++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 480dafe..1fb5cff 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2431,6 +2431,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>  	if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>  		pdata->cclk_in_drv = 0;
> 
> +	pdata->sdr_timing = (sdr_timing[1] | (sdr_timing[0] << 4));
> +	pdata->ddr_timing = (ddr_timing[1] | (ddr_timing[0] << 4));
Can you explain what it is for?

Thanks,
Seungwon

>  	return pdata;
>  }
> 
> @@ -2478,6 +2480,27 @@ int dw_mci_probe(struct dw_mci *host)
>  		dev_dbg(host->dev, "ciu clock not available\n");
>  		host->bus_hz = host->pdata->bus_hz;
>  	} else {
> +		/* If the CIU clk is available, we check for SDR and DDR
> +		 * timing phase shift settings. But not all platforms
> +		 * may have populated these settings, the driver will not fail
> +		 * if these settings are not specified.
> +		 */
> +		if (host->pdata->sdr_timing) {
> +			host->sdr_clk = devm_clk_get(host->dev, "sdr_mmc_clk");
> +			if (IS_ERR(host->sdr_clk))
> +				dev_dbg(host->dev, "sdr_mmc clock not available\n");
> +			else
> +				clk_set_rate(host->sdr_clk, host->pdata->sdr_timing);
> +		}
> +
> +		if (host->pdata->ddr_timing) {
> +			host->ddr_clk = devm_clk_get(host->dev, "ddr_mmc_clk");
> +			if (IS_ERR(host->ddr_clk))
> +				dev_dbg(host->dev, "ddr_mmc clock not available\n");
> +			else
> +				clk_set_rate(host->ddr_clk, host->pdata->ddr_timing);
> +		}
> +
>  		ret = clk_prepare_enable(host->ciu_clk);
>  		if (ret) {
>  			dev_err(host->dev, "failed to enable ciu clock\n");
> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
> index 2b5b8bf..ad90ad1 100644
> --- a/include/linux/mmc/dw_mmc.h
> +++ b/include/linux/mmc/dw_mmc.h
> @@ -83,6 +83,8 @@ struct mmc_data;
>   * @priv: Implementation defined private data.
>   * @biu_clk: Pointer to bus interface unit clock instance.
>   * @ciu_clk: Pointer to card interface unit clock instance.
> + * @sdr_clk: Pointer to the SDR clock instance.
> + * @ddr_clk: Pointer to the DDR clock instance.
>   * @slot: Slots sharing this MMC controller.
>   * @fifo_depth: depth of FIFO.
>   * @data_shift: log2 of FIFO item size.
> @@ -170,6 +172,8 @@ struct dw_mci {
>  	void			*priv;
>  	struct clk		*biu_clk;
>  	struct clk		*ciu_clk;
> +	struct clk		*sdr_clk;
> +	struct clk		*ddr_clk;
>  	struct dw_mci_slot	*slot[MAX_MCI_SLOTS];
> 
>  	/* FIFO push and pull */
> @@ -241,6 +245,8 @@ struct dw_mci_board {
>  	u32 caps2;	/* More capabilities */
>  	u32 pm_caps;	/* PM capabilities */
>  	u32 cclk_in_drv;	/*cclk_in_drv phase shift */
> +	u32 sdr_timing;	/* Single data rate timing setting. */
> +	u32 ddr_timing;	/* Double data rate timing setting. */
>  	/*
>  	 * Override fifo depth. If 0, autodetect it from the FIFOTH register,
>  	 * but note that this may not be reliable after a bootloader has used
> --
> 1.7.9.5
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv6 2/5] clk: socfpga: Add a clock type for the SD/MMC driver

[dinguyen-EIB2kfCEclfQT0dZR+AlfA]

From: Dinh Nguyen <dinguyen-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>

Add a "altr,socfpga-sdmmc-sdr-clk" clock type in the SOCFPGA clock driver. This
clock type is not really a "clock" for say, but a mechanism to set the phase
shift of the clock that is used to feed the SD/MMC CIU's clock. This clock does
not have parent so it is designated as a CLK_IS_ROOT.

This clock implements the set_clk_rate method that is meant to receive the SDR
settings that is designated by the "samsung,dw-mshc-sdr-timing" binding. The
SD/MMC driver passes this clock phase information into the clock driver to use.

This enables the SD/MMC driver to touch registers that are located outside of
the SD/MMC IP, which helps make the core SD/MMC driver generic.

Signed-off-by: Dinh Nguyen <dinguyen-E5Uy0aZIfLoAvxtiuMwx3w@public.gmane.org>
---
v6: Add a new clock type "altr,socfpga-sdmmc-sdr-clk" that will be used to
set the phase shift settings.
v5: Use the "snps,dw-mshc" binding
v4: Use the sdmmc_clk prepare function to set the phase shift settings
v3: Not use the syscon driver because as of 3.13-rc1, the syscon driver is
loaded after the clock driver.
v2: Use the syscon driver
---
 drivers/clk/socfpga/clk.c |   86 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/clk/socfpga/clk.c b/drivers/clk/socfpga/clk.c
index 280c983..f4c983e 100644
--- a/drivers/clk/socfpga/clk.c
+++ b/drivers/clk/socfpga/clk.c
@@ -21,8 +21,10 @@
 #include <linux/clkdev.h>
 #include <linux/clk-provider.h>
 #include <linux/io.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/regmap.h>
 
 /* Clock Manager offsets */
 #define CLKMGR_CTRL	0x0
@@ -69,6 +71,84 @@ struct socfpga_clk {
 };
 #define to_socfpga_clk(p) container_of(p, struct socfpga_clk, hw.hw)
 
+/* SDMMC Group for System Manager defines */
+#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
+	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
+struct sdmmc_sdr_clk {
+	struct clk_hw	hw;
+	u32	reg;
+};
+#define to_sdmmc_sdr_clk(p) container_of(p, struct sdmmc_sdr_clk, hw)
+
+static int sdr_clk_set_rate(struct clk_hw *hwclk, unsigned long rate,
+					unsigned long parent_rate)
+{
+	struct sdmmc_sdr_clk *sdmmc_sdr_clk = to_sdmmc_sdr_clk(hwclk);
+	struct regmap *sys_mgr_base_addr;
+	u32 hs_timing;
+
+	sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
+	if (IS_ERR(sys_mgr_base_addr)) {
+		pr_err("%s: failed to find altr,sys-mgr regmap!\n", __func__);
+		return -EINVAL;
+	}
+	hs_timing = SYSMGR_SDMMC_CTRL_SET(((rate > 4) & 0xf), (rate & 0xf));
+	regmap_write(sys_mgr_base_addr, sdmmc_sdr_clk->reg, hs_timing);
+	return 0;
+}
+
+static unsigned long sdr_clk_recalc_rate(struct clk_hw *hwclk,
+						unsigned long parent_rate)
+{
+	return parent_rate;
+}
+
+static long sdr_clk_round_rate(struct clk_hw *hwclk, unsigned long rate,
+						unsigned long *parent_rate)
+{
+	return rate;
+}
+
+static const struct clk_ops sdmmc_sdr_clk_ops = {
+	.recalc_rate = sdr_clk_recalc_rate,
+	.round_rate = sdr_clk_round_rate,
+	.set_rate = sdr_clk_set_rate,
+};
+
+static __init struct clk *socfpga_sdmmc_sdr_clk_init(struct device_node *node,
+						const struct clk_ops *ops)
+{
+	u32 reg;
+	struct clk *clk;
+	struct sdmmc_sdr_clk *sdmmc_sdr_clk;
+	const char *clk_name = node->name;
+	struct clk_init_data init;
+	int rc;
+
+	rc = of_property_read_u32(node, "reg", &reg);
+
+	sdmmc_sdr_clk = kzalloc(sizeof(*sdmmc_sdr_clk), GFP_KERNEL);
+	if (WARN_ON(!sdmmc_sdr_clk))
+		return NULL;
+
+	sdmmc_sdr_clk->reg = reg;
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = CLK_IS_ROOT;
+	init.num_parents = 0;
+	sdmmc_sdr_clk->hw.init = &init;
+
+	clk = clk_register(NULL, &sdmmc_sdr_clk->hw);
+	if (WARN_ON(IS_ERR(clk))) {
+		kfree(sdmmc_sdr_clk);
+		return NULL;
+	}
+	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	return clk;
+}
+
 static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
 					 unsigned long parent_rate)
 {
@@ -332,10 +412,16 @@ static void __init socfpga_gate_init(struct device_node *node)
 	socfpga_gate_clk_init(node, &gateclk_ops);
 }
 
+static void __init socfpga_sdmmc_init(struct device_node *node)
+{
+	socfpga_sdmmc_sdr_clk_init(node, &sdmmc_sdr_clk_ops);
+}
+
 static struct of_device_id socfpga_child_clocks[] = {
 	{ .compatible = "altr,socfpga-pll-clock", socfpga_pll_init, },
 	{ .compatible = "altr,socfpga-perip-clk", socfpga_periph_init, },
 	{ .compatible = "altr,socfpga-gate-clk", socfpga_gate_init, },
+	{ .compatible = "altr,socfpga-sdmmc-sdr-clk", socfpga_sdmmc_init, },
 	{},
 };
 
-- 
1.7.9.5


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6 2/5] clk: socfpga: Add a clock type for the SD/MMC driver

[Arnd Bergmann]

On Thursday 12 December 2013, dinguyen@altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> Add a "altr,socfpga-sdmmc-sdr-clk" clock type in the SOCFPGA clock driver. This
> clock type is not really a "clock" for say, but a mechanism to set the phase
> shift of the clock that is used to feed the SD/MMC CIU's clock. This clock does
> not have parent so it is designated as a CLK_IS_ROOT.
> 
> This clock implements the set_clk_rate method that is meant to receive the SDR
> settings that is designated by the "samsung,dw-mshc-sdr-timing" binding. The
> SD/MMC driver passes this clock phase information into the clock driver to use.
> 
> This enables the SD/MMC driver to touch registers that are located outside of
> the SD/MMC IP, which helps make the core SD/MMC driver generic.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@alter.com>

Ok, this looks like it will work, but we haven't concluded if this is the
best way to do it. I'm looking for guidance from Mike here.

The alternatives I can see for this particular problem are:

1. fake a clock and use the 'clk_set_rate' callback to set the phase
   rather than the clock frequency (as you do in this patch).
2. extend the common clock API to provide a 'clk_set_phase' interface
   that you can call on the CIU clock to set this, so you don't have
   to fake anything. This seems to be the nicest interface if we have
   the same problem in more drivers.
3. make the phase setting private to whichever clock is used for CIU
   (as one of your previous patches did, which I did not like).
4. make the phase an additional argument to the clock specifier,
   so you would have <&mmcclock 12345 5678> rather than just <&mmcclock>
   in the mmc host device node to specify the two possible phases.

I would also like to get buy-in from Zhangfei Gao, since he is working
on the same thing. Please coordinate with him to make sure whatever
one of you comes up with works for the other one too. At the moment,
patches are flying so fast without much discussion inbetween that I'd
prefer Chris to hold off from merging either one until you have both
revieved and Acked each other's patches.

	Arnd

Re: [PATCHv6 2/5] clk: socfpga: Add a clock type for the SD/MMC driver

[zhangfei]

Dear Arnd

On 12/15/2013 05:33 AM, Arnd Bergmann wrote:
> On Thursday 12 December 2013, dinguyen@altera.com wrote:
>> From: Dinh Nguyen <dinguyen@altera.com>
>>
>> Add a "altr,socfpga-sdmmc-sdr-clk" clock type in the SOCFPGA clock driver. This
>> clock type is not really a "clock" for say, but a mechanism to set the phase
>> shift of the clock that is used to feed the SD/MMC CIU's clock. This clock does
>> not have parent so it is designated as a CLK_IS_ROOT.
>>
>> This clock implements the set_clk_rate method that is meant to receive the SDR
>> settings that is designated by the "samsung,dw-mshc-sdr-timing" binding. The
>> SD/MMC driver passes this clock phase information into the clock driver to use.
>>
>> This enables the SD/MMC driver to touch registers that are located outside of
>> the SD/MMC IP, which helps make the core SD/MMC driver generic.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@alter.com>
>
> Ok, this looks like it will work, but we haven't concluded if this is the
> best way to do it. I'm looking for guidance from Mike here.
>
> The alternatives I can see for this particular problem are:
>
> 1. fake a clock and use the 'clk_set_rate' callback to set the phase
>     rather than the clock frequency (as you do in this patch).
> 2. extend the common clock API to provide a 'clk_set_phase' interface
>     that you can call on the CIU clock to set this, so you don't have
>     to fake anything. This seems to be the nicest interface if we have
>     the same problem in more drivers.
> 3. make the phase setting private to whichever clock is used for CIU
>     (as one of your previous patches did, which I did not like).
> 4. make the phase an additional argument to the clock specifier,
>     so you would have <&mmcclock 12345 5678> rather than just <&mmcclock>
>     in the mmc host device node to specify the two possible phases.
>
> I would also like to get buy-in from Zhangfei Gao, since he is working
> on the same thing. Please coordinate with him to make sure whatever
> one of you comes up with works for the other one too. At the moment,
> patches are flying so fast without much discussion inbetween that I'd
> prefer Chris to hold off from merging either one until you have both
> revieved and Acked each other's patches.
>

This is our case
In this version ip, supported now.
Only several mode (LEGACY, HS) are supported in the code, where fixed 
div and sample is need to be set, so we can hide them in clock.
And each controller have different value & register, so different 
controller use different clock.
These clock can be ciu_clock, whose parent are original clock input.

In next version ip, which still not in hand.
HS200 and other mode have to be supported, tuning process have to be 
included, good example is drivers/mmc/host/dw_mmc-exynos.c
.execute_tuning         = dw_mci_exynos_execute_tuning,
It have to choose the best point during the tuning process.
Some other mode may still need tuning, which is special in our case.

Zhangfei

Re: [PATCHv6 2/5] clk: socfpga: Add a clock type for the SD/MMC driver

[Mike Turquette]

Quoting zhangfei (2013-12-14 18:18:45)
> Dear Arnd
> 
> On 12/15/2013 05:33 AM, Arnd Bergmann wrote:
> > On Thursday 12 December 2013, dinguyen@altera.com wrote:
> >> From: Dinh Nguyen <dinguyen@altera.com>
> >>
> >> Add a "altr,socfpga-sdmmc-sdr-clk" clock type in the SOCFPGA clock driver. This
> >> clock type is not really a "clock" for say, but a mechanism to set the phase
> >> shift of the clock that is used to feed the SD/MMC CIU's clock. This clock does
> >> not have parent so it is designated as a CLK_IS_ROOT.
> >>
> >> This clock implements the set_clk_rate method that is meant to receive the SDR
> >> settings that is designated by the "samsung,dw-mshc-sdr-timing" binding. The
> >> SD/MMC driver passes this clock phase information into the clock driver to use.
> >>
> >> This enables the SD/MMC driver to touch registers that are located outside of
> >> the SD/MMC IP, which helps make the core SD/MMC driver generic.
> >>
> >> Signed-off-by: Dinh Nguyen <dinguyen@alter.com>
> >
> > Ok, this looks like it will work, but we haven't concluded if this is the
> > best way to do it. I'm looking for guidance from Mike here.
> >
> > The alternatives I can see for this particular problem are:
> >
> > 1. fake a clock and use the 'clk_set_rate' callback to set the phase
> >     rather than the clock frequency (as you do in this patch).
> > 2. extend the common clock API to provide a 'clk_set_phase' interface
> >     that you can call on the CIU clock to set this, so you don't have
> >     to fake anything. This seems to be the nicest interface if we have
> >     the same problem in more drivers.

clk_set_phase has been proposed before and now may be the time to add
it. There are two things that need to be addressed:

1) what are the values for the phase? This needs to work for others that
must set clk phase, so we need to consider all those requirements before
making a new function declaration in clk.h

2) is setting a clock's phase something done dynamically? Put another
way, does the same clock has it's phase set multiple times while the
system is running? For static configuration that only happens during
initialization we do not need a new API. The clock driver can handle it
privately. For dynamic operations though we likely need a new API.

I've Cc'd Peter since I think Tegra has clock phase requirements as well
that we discussed some time back.

Regards,
Mike

> > 3. make the phase setting private to whichever clock is used for CIU
> >     (as one of your previous patches did, which I did not like).
> > 4. make the phase an additional argument to the clock specifier,
> >     so you would have <&mmcclock 12345 5678> rather than just <&mmcclock>
> >     in the mmc host device node to specify the two possible phases.
> >
> > I would also like to get buy-in from Zhangfei Gao, since he is working
> > on the same thing. Please coordinate with him to make sure whatever
> > one of you comes up with works for the other one too. At the moment,
> > patches are flying so fast without much discussion inbetween that I'd
> > prefer Chris to hold off from merging either one until you have both
> > revieved and Acked each other's patches.
> >
> 
> This is our case
> In this version ip, supported now.
> Only several mode (LEGACY, HS) are supported in the code, where fixed 
> div and sample is need to be set, so we can hide them in clock.
> And each controller have different value & register, so different 
> controller use different clock.
> These clock can be ciu_clock, whose parent are original clock input.
> 
> In next version ip, which still not in hand.
> HS200 and other mode have to be supported, tuning process have to be 
> included, good example is drivers/mmc/host/dw_mmc-exynos.c
> .execute_tuning         = dw_mci_exynos_execute_tuning,
> It have to choose the best point during the tuning process.
> Some other mode may still need tuning, which is special in our case.
> 
> Zhangfei
> 
> 
> 
> 
> 

Re: [PATCHv6 2/5] clk: socfpga: Add a clock type for the SD/MMC driver

[Emilio López]

Hi Mike et al,

El 15/12/13 01:51, Mike Turquette escribió:
> clk_set_phase has been proposed before and now may be the time to add
> it. There are two things that need to be addressed:
>
> 1) what are the values for the phase? This needs to work for others that
> must set clk phase, so we need to consider all those requirements before
> making a new function declaration in clk.h
>
> 2) is setting a clock's phase something done dynamically? Put another
> way, does the same clock has it's phase set multiple times while the
> system is running? For static configuration that only happens during
> initialization we do not need a new API. The clock driver can handle it
> privately. For dynamic operations though we likely need a new API.

We on sunxi also need this for our (not yet merged) MMC driver; we 
currently have it implemented as an exported

void clk_sunxi_mmc_phase_control(struct clk_hw *hw, u8 sample, u8 output)

that takes the MMC clock (and only the MMC clock) and does the setup 
(it's basically configuring two values, "sample" and "output", into the 
clock register). I really don't know what does this do/why is it 
required/when is it used; I'm cc'ing Hans and David who can hopefully 
explain that part.

I also saw a similar requirement from the gmac people (on cc too), who 
needed to set the phy type (or something like that) on one of the clock 
registers; so I'm thinking maybe a generic "tune something" approach 
that lets users configure some clock-dependent, arbitrary aspect of the 
clock is the way forward. Otherwise, we're going to end up bloating the 
clock framework with a lot of callback pointers that are going to be 
used in one or two clocks out of potentially hundreds on the whole system.

Cheers,

Emilio
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6 2/5] clk: socfpga: Add a clock type for the SD/MMC driver

[Hans de Goede]

Hi,

On 12/16/2013 09:55 PM, Emilio López wrote:
> Hi Mike et al,
>
> El 15/12/13 01:51, Mike Turquette escribió:
>> clk_set_phase has been proposed before and now may be the time to add
>> it. There are two things that need to be addressed:
>>
>> 1) what are the values for the phase? This needs to work for others that
>> must set clk phase, so we need to consider all those requirements before
>> making a new function declaration in clk.h
>>
>> 2) is setting a clock's phase something done dynamically? Put another
>> way, does the same clock has it's phase set multiple times while the
>> system is running? For static configuration that only happens during
>> initialization we do not need a new API. The clock driver can handle it
>> privately. For dynamic operations though we likely need a new API.
>
> We on sunxi also need this for our (not yet merged) MMC driver; we currently have it implemented as an exported
>
> void clk_sunxi_mmc_phase_control(struct clk_hw *hw, u8 sample, u8 output)
>
> that takes the MMC clock (and only the MMC clock) and does the setup (it's basically configuring two values, "sample" and "output", into the clock register). I really don't know what does this do/why is it required/when is it used; I'm cc'ing Hans and David who can hopefully explain that part.

I'm afraid I cannot explain that part, the MMC controller in the sunxi SoCs
is undocumented, so what we're doing there comes straight from the android
kernel code. I do understand most of the bits of the sunxi-mci driver, but
this bit is black magic to me.

Regards,

Hans

Re: [PATCHv6 2/5] clk: socfpga: Add a clock type for the SD/MMC driver

[David Lanzendörfer]

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

Hi
> that takes the MMC clock (and only the MMC clock) and does the setup
> (it's basically configuring two values, "sample" and "output", into the
> clock register). I really don't know what does this do/why is it
> required/when is it used; I'm cc'ing Hans and David who can hopefully
> explain that part.
Since the read/write operations have to happen asynchronously and in a manner
so that the data provided by the SD-Card can be fetched on time, a specific
clock phase shift is required as you can see in the linked picture[1].
That's what the function is doing: It accesses the special registers of the
MMC clock device and configures this phase offset.

> I also saw a similar requirement from the gmac people (on cc too), who
> needed to set the phy type (or something like that) on one of the clock
> registers; so I'm thinking maybe a generic "tune something" approach
> that lets users configure some clock-dependent, arbitrary aspect of the
> clock is the way forward. Otherwise, we're going to end up bloating the
> clock framework with a lot of callback pointers that are going to be
> used in one or two clocks out of potentially hundreds on the whole system.
It totally makes sense, since GMAC will be doing asynchrous
operations as well.

-David

[1] http://www.chipestimate.com/techtalk/images/11022010_fig2_3.JPG

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCHv6 2/5] clk: socfpga: Add a clock type for the SD/MMC driver

[Mike Turquette]

Quoting David Lanzendörfer (2013-12-16 13:54:21)
> Hi
> > that takes the MMC clock (and only the MMC clock) and does the setup
> > (it's basically configuring two values, "sample" and "output", into the
> > clock register). I really don't know what does this do/why is it
> > required/when is it used; I'm cc'ing Hans and David who can hopefully
> > explain that part.
> Since the read/write operations have to happen asynchronously and in a manner
> so that the data provided by the SD-Card can be fetched on time, a specific
> clock phase shift is required as you can see in the linked picture[1].
> That's what the function is doing: It accesses the special registers of the
> MMC clock device and configures this phase offset.

Yeah, clock phase is a fairly common thing to tune but there are two
things to consider when crafting a new api like clk_set_phase(...):

1) are we adjusting the phase of the clock signal or the phase of
something else related to the clock rate? This is just something to
think about. Sometimes it would be better to have
video_signal_set_phase() instead of clk_set_phase().

2) what is the input parameter to clk_set_phase? I guess the two best
options are degrees (zero to 359), or a fraction of the clock period
(1/4, 1/2, etc).

Any thoughts?

Regards,
Mike

> 
> > I also saw a similar requirement from the gmac people (on cc too), who
> > needed to set the phy type (or something like that) on one of the clock
> > registers; so I'm thinking maybe a generic "tune something" approach
> > that lets users configure some clock-dependent, arbitrary aspect of the
> > clock is the way forward. Otherwise, we're going to end up bloating the
> > clock framework with a lot of callback pointers that are going to be
> > used in one or two clocks out of potentially hundreds on the whole system.
> It totally makes sense, since GMAC will be doing asynchrous
> operations as well.
> 
> -David
> 
> [1] http://www.chipestimate.com/techtalk/images/11022010_fig2_3.JPG

Re: [PATCHv6 2/5] clk: socfpga: Add a clock type for the SD/MMC driver

[Chen-Yu Tsai]

Hi,

On Tue, Dec 17, 2013 at 4:55 AM, Emilio López <emilio@elopez.com.ar> wrote:
> I also saw a similar requirement from the gmac people (on cc too), who
> needed to set the phy type (or something like that) on one of the clock

The GMAC clock register controls the tx clock source (a mux) and the
interface type, which is most likely a gate to control the direction
of the tx clock.

So I modeled it as a composite clock source for now.
I'm open to other options.

> registers; so I'm thinking maybe a generic "tune something" approach that
> lets users configure some clock-dependent, arbitrary aspect of the clock is
> the way forward. Otherwise, we're going to end up bloating the clock
> framework with a lot of callback pointers that are going to be used in one
> or two clocks out of potentially hundreds on the whole system.

ChenYu

[PATCHv6 3/5] dts: socfpga: Add support for SD/MMC on the SOCFPGA platform

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Adds a new binding, "altr,socfpga-sdmmc-sdr-clk". This is a new clock
binding that the SD/MMC driver can use the common clock framework to
set the appropriate clock phase shift settings for the CIU clock.

Also add the "syscon" binding to the "altr,sys-mgr" node. The clock
driver can use the syscon driver to toggle the register for the SD/MMC
clock phase shift settings.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
v6: Add documentation for "altr,socfpga-sdmmc-sdr-clk". Add "syscon" to the
sysmgr binding.
v5: Use the "snps,dw-mshc" binding
v4: Re-use "rockchip,rk2928-dw-mshc" binding
v3: none
v2: none
---
 .../devicetree/bindings/clock/altr_socfpga.txt     |   11 +++++++++--
 arch/arm/boot/dts/socfpga.dtsi                     |   19 ++++++++++++++++++-
 arch/arm/boot/dts/socfpga_arria5.dtsi              |   12 ++++++++++++
 arch/arm/boot/dts/socfpga_cyclone5.dtsi            |   12 ++++++++++++
 arch/arm/boot/dts/socfpga_vt.dts                   |   12 ++++++++++++
 5 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/altr_socfpga.txt b/Documentation/devicetree/bindings/clock/altr_socfpga.txt
index 0045433..a2e75f0 100644
--- a/Documentation/devicetree/bindings/clock/altr_socfpga.txt
+++ b/Documentation/devicetree/bindings/clock/altr_socfpga.txt
@@ -11,10 +11,17 @@ Required properties:
 		PLL clock.
 	"altr,socfpga-gate-clk" - Clocks that directly feed peripherals and
 		can get gated.
+	"altr,socfpga-sdmmc-sdr-clk" - Clock that controls the SD/MMC SDR phase
+		shift settings for the SD/MMC
 
-- reg : shall be the control register offset from CLOCK_MANAGER's base for the clock.
+- reg : shall be one of the following:
+	* For the "altr,socfpga-sdmmc-sdr-clk" clock, reg will the register
+		offset that controls the SD/MMC SDR phase shift settings.
+	* For all of the other clocks, control register offset from
+		CLOCK_MANAGER's base for the clock.
 - clocks : shall be the input parent clock phandle for the clock. This is
-	either an oscillator or a pll output.
+	either an oscillator or a pll output. This is an optional field for
+	the "altr,socfpga-sdmmc-sdr-clk" clock.
 - #clock-cells : from common clock binding, shall be set to 0.
 
 Optional properties:
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index f936476..4be9aaf 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -436,6 +436,12 @@
 						clocks = <&f2s_periph_ref_clk>, <&main_qspi_clk>, <&per_qspi_clk>;
 						clk-gate = <0xa0 11>;
 					};
+
+					sdr_mmc_clk: sdr_mmc_clk {
+						#clock-cells = <0>;
+						compatible = "altr,socfpga-sdmmc-sdr-clk";
+						reg = <0x108>;
+					};
 				};
 			};
 
@@ -469,6 +475,17 @@
 			cache-level = <2>;
 		};
 
+		mmc: dwmmc0@ff704000 {
+			compatible = "snps,dw-mshc";
+			reg = <0xff704000 0x1000>;
+			interrupts = <0 139 4>;
+			fifo-depth = <0x400>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&l4_mp_clk>, <&sdmmc_clk>, <&sdr_mmc_clk>;
+			clock-names = "biu", "ciu", "sdr_mmc_clk";
+		};
+
 		/* Local timer */
 		timer@fffec600 {
 			compatible = "arm,cortex-a9-twd-timer";
@@ -523,7 +540,7 @@
 		};
 
 		sysmgr@ffd08000 {
-				compatible = "altr,sys-mgr";
+				compatible = "altr,sys-mgr", "syscon";
 				reg = <0xffd08000 0x4000>;
 			};
 	};
diff --git a/arch/arm/boot/dts/socfpga_arria5.dtsi b/arch/arm/boot/dts/socfpga_arria5.dtsi
index a85b404..112b7e2 100644
--- a/arch/arm/boot/dts/socfpga_arria5.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria5.dtsi
@@ -27,6 +27,18 @@
 			};
 		};
 
+		dwmmc0@ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+			samsung,dw-mshc-sdr-timing = <0 3>;
+
+			slot@0 {
+				reg = <0>;
+				bus-width = <4>;
+			};
+		};
+
 		serial0@ffc02000 {
 			clock-frequency = <100000000>;
 		};
diff --git a/arch/arm/boot/dts/socfpga_cyclone5.dtsi b/arch/arm/boot/dts/socfpga_cyclone5.dtsi
index a8716f6..52b1501 100644
--- a/arch/arm/boot/dts/socfpga_cyclone5.dtsi
+++ b/arch/arm/boot/dts/socfpga_cyclone5.dtsi
@@ -28,6 +28,18 @@
 			};
 		};
 
+		dwmmc0@ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+			samsung,dw-mshc-sdr-timing = <0 3>;
+
+			slot@0 {
+				reg = <0>;
+				bus-width = <4>;
+			};
+		};
+
 		ethernet@ff702000 {
 			phy-mode = "rgmii";
 			phy-addr = <0xffffffff>; /* probe for phy addr */
diff --git a/arch/arm/boot/dts/socfpga_vt.dts b/arch/arm/boot/dts/socfpga_vt.dts
index d1ec0ca..9e93768 100644
--- a/arch/arm/boot/dts/socfpga_vt.dts
+++ b/arch/arm/boot/dts/socfpga_vt.dts
@@ -41,6 +41,18 @@
 			};
 		};
 
+		dwmmc0@ff704000 {
+			num-slots = <1>;
+			supports-highspeed;
+			broken-cd;
+			samsung,dw-mshc-sdr-timing = <0 3>;
+
+			slot@0 {
+			reg = <0>;
+			bus-width = <4>;
+			};
+		};
+
 		ethernet@ff700000 {
 			phy-mode = "gmii";
 			status = "okay";
-- 
1.7.9.5

[PATCHv6 4/5] mmc: dw_mmc-socfpga: Remove the SOCFPGA specific platform for dw_mmc

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

It turns now that the only really platform specific code that is needed for
SOCFPGA is using the SDMMC_CMD_USE_HOLD_REG in the prepare_command function.
Since the Rockchip already has this functionality, re-use the code that is
already in dw_mmc-pltfm.c.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
v6: none
v5: none
v4: Remove dw_mmc-socfpga platform specific code
v3: none
v2: none
---
 drivers/mmc/host/Kconfig          |    8 ---
 drivers/mmc/host/dw_mmc-socfpga.c |  138 -------------------------------------
 2 files changed, 146 deletions(-)
 delete mode 100644 drivers/mmc/host/dw_mmc-socfpga.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 7fc5099..6737a4f 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -567,14 +567,6 @@ config MMC_DW_EXYNOS
 	  Synopsys DesignWare Memory Card Interface driver. Select this option
 	  for platforms based on Exynos4 and Exynos5 SoC's.
 
-config MMC_DW_SOCFPGA
-	tristate "SOCFPGA specific extensions for Synopsys DW Memory Card Interface"
-	depends on MMC_DW && MFD_SYSCON
-	select MMC_DW_PLTFM
-	help
-	  This selects support for Altera SoCFPGA specific extensions to the
-	  Synopsys DesignWare Memory Card Interface driver.
-
 config MMC_DW_PCI
 	tristate "Synopsys Designware MCI support on PCI bus"
 	depends on MMC_DW && PCI
diff --git a/drivers/mmc/host/dw_mmc-socfpga.c b/drivers/mmc/host/dw_mmc-socfpga.c
deleted file mode 100644
index 3e8e53a..0000000
--- a/drivers/mmc/host/dw_mmc-socfpga.c
+++ /dev/null
@@ -1,138 +0,0 @@
-/*
- * Altera SoCFPGA Specific Extensions for Synopsys DW Multimedia Card Interface
- * driver
- *
- *  Copyright (C) 2012, Samsung Electronics Co., Ltd.
- *  Copyright (C) 2013 Altera Corporation
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * Taken from dw_mmc-exynos.c
- */
-#include <linux/clk.h>
-#include <linux/mfd/syscon.h>
-#include <linux/mmc/host.h>
-#include <linux/mmc/dw_mmc.h>
-#include <linux/module.h>
-#include <linux/of.h>
-#include <linux/platform_device.h>
-#include <linux/regmap.h>
-
-#include "dw_mmc.h"
-#include "dw_mmc-pltfm.h"
-
-#define SYSMGR_SDMMCGRP_CTRL_OFFSET		0x108
-#define DRV_CLK_PHASE_SHIFT_SEL_MASK	0x7
-#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
-	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
-
-/* SOCFPGA implementation specific driver private data */
-struct dw_mci_socfpga_priv_data {
-	u8	ciu_div; /* card interface unit divisor */
-	u32	hs_timing; /* bitmask for CIU clock phase shift */
-	struct regmap   *sysreg; /* regmap for system manager register */
-};
-
-static int dw_mci_socfpga_priv_init(struct dw_mci *host)
-{
-	return 0;
-}
-
-static int dw_mci_socfpga_setup_clock(struct dw_mci *host)
-{
-	struct dw_mci_socfpga_priv_data *priv = host->priv;
-
-	clk_disable_unprepare(host->ciu_clk);
-	regmap_write(priv->sysreg, SYSMGR_SDMMCGRP_CTRL_OFFSET,
-		priv->hs_timing);
-	clk_prepare_enable(host->ciu_clk);
-
-	host->bus_hz /= (priv->ciu_div + 1);
-	return 0;
-}
-
-static void dw_mci_socfpga_prepare_command(struct dw_mci *host, u32 *cmdr)
-{
-	struct dw_mci_socfpga_priv_data *priv = host->priv;
-
-	if (priv->hs_timing & DRV_CLK_PHASE_SHIFT_SEL_MASK)
-		*cmdr |= SDMMC_CMD_USE_HOLD_REG;
-}
-
-static int dw_mci_socfpga_parse_dt(struct dw_mci *host)
-{
-	struct dw_mci_socfpga_priv_data *priv;
-	struct device_node *np = host->dev->of_node;
-	u32 timing[2];
-	u32 div = 0;
-	int ret;
-
-	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		dev_err(host->dev, "mem alloc failed for private data\n");
-		return -ENOMEM;
-	}
-
-	priv->sysreg = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
-	if (IS_ERR(priv->sysreg)) {
-		dev_err(host->dev, "regmap for altr,sys-mgr lookup failed.\n");
-		return PTR_ERR(priv->sysreg);
-	}
-
-	ret = of_property_read_u32(np, "altr,dw-mshc-ciu-div", &div);
-	if (ret)
-		dev_info(host->dev, "No dw-mshc-ciu-div specified, assuming 1");
-	priv->ciu_div = div;
-
-	ret = of_property_read_u32_array(np,
-			"altr,dw-mshc-sdr-timing", timing, 2);
-	if (ret)
-		return ret;
-
-	priv->hs_timing = SYSMGR_SDMMC_CTRL_SET(timing[0], timing[1]);
-	host->priv = priv;
-	return 0;
-}
-
-static const struct dw_mci_drv_data socfpga_drv_data = {
-	.init			= dw_mci_socfpga_priv_init,
-	.setup_clock		= dw_mci_socfpga_setup_clock,
-	.prepare_command	= dw_mci_socfpga_prepare_command,
-	.parse_dt		= dw_mci_socfpga_parse_dt,
-};
-
-static const struct of_device_id dw_mci_socfpga_match[] = {
-	{ .compatible = "altr,socfpga-dw-mshc",
-			.data = &socfpga_drv_data, },
-	{},
-};
-MODULE_DEVICE_TABLE(of, dw_mci_socfpga_match);
-
-static int dw_mci_socfpga_probe(struct platform_device *pdev)
-{
-	const struct dw_mci_drv_data *drv_data;
-	const struct of_device_id *match;
-
-	match = of_match_node(dw_mci_socfpga_match, pdev->dev.of_node);
-	drv_data = match->data;
-	return dw_mci_pltfm_register(pdev, drv_data);
-}
-
-static struct platform_driver dw_mci_socfpga_pltfm_driver = {
-	.probe		= dw_mci_socfpga_probe,
-	.remove		= __exit_p(dw_mci_pltfm_remove),
-	.driver		= {
-		.name		= "dwmmc_socfpga",
-		.of_match_table	= dw_mci_socfpga_match,
-		.pm		= &dw_mci_pltfm_pmops,
-	},
-};
-
-module_platform_driver(dw_mci_socfpga_pltfm_driver);
-
-MODULE_DESCRIPTION("Altera SOCFPGA Specific DW-MSHC Driver Extension");
-MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("platform:dwmmc-socfpga");
-- 
1.7.9.5

[PATCHv6 5/5] ARM: socfpga_defconfig: enable SD/MMC support

[dinguyen]

From: Dinh Nguyen <dinguyen@altera.com>

Enables the dw_mmc driver for SOCFPGA.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
v6: none
v5: Add dw_mmc driver into socfpga_defconfig
v4: none
v3: none
v2: none
---
 arch/arm/configs/socfpga_defconfig |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/socfpga_defconfig b/arch/arm/configs/socfpga_defconfig
index 4e1ce21..8fff96ba 100644
--- a/arch/arm/configs/socfpga_defconfig
+++ b/arch/arm/configs/socfpga_defconfig
@@ -82,3 +82,5 @@ CONFIG_DEBUG_INFO=y
 CONFIG_ENABLE_DEFAULT_TRACERS=y
 CONFIG_DEBUG_USER=y
 CONFIG_XZ_DEC=y
+CONFIG_MMC=y
+CONFIG_MMC_DW=y
-- 
1.7.9.5