[PATCHv4] mmc: dw_mmc: Enable the hold reg for certain speed modes

Commit Message

Dinh Nguyen Dec. 16, 2013, 5:01 p.m. UTC

From: Dinh Nguyen <[e-mail]>

This patch will enable the SDMMC_CMD_USE_HOLD_REG bit when the slot is
operating all timing modes, except for SDR50, DDR50, SDR104, and MMC_HS200.

According to the Synopsys databook :"To meet the relatively high Input Hold
Time requirement for SDR12, SDR25, and other MMC speed modes, you should
program bit[29]use_hold_Reg of the CMD register to 1'b1;"..."However, for
the higher speed modes of SDR104, SDR50 and DDR50, you can meet the much
smaller Input Hold Time requirement of 0.8ns by bypassing the Hold Register
(Path A in Figure 10-8, programming CMD.use_hold_reg = 1'b0) and then adding
delay elements on the output path as indicated.

Also, "Never set CMD.use_hold_reg = 1 and cclk_in_drv phase shift to 0 at the
same time. This would add an extra one-cycle delay on the output path, resulting
in incorrect behavior."

This patch also checks the IHR(Implement Hold Register) in the HCON register.

This information is taking from the v2.50a of the Synopsys Designware Cores
Mobile Storage Host Databook.

Signed-off-by: Dinh Nguyen <[e-mail]>
Acked-by: Arnd Bergmann <[e-mail]>
Acked-by: Heiko Stuebner <[e-mail]>
Tested-by: Heiko Stuebner <[e-mail]>
---
v4: use_hold_reg should be set for all modes when cclk_in_drv is non-zero.
v3: Read the IHR(Implement Hold Register) in the HCON
v2: Add check for cclk_in_drv phase shift in conjunction with use_hold_reg.
---
 drivers/mmc/host/dw_mmc.c  |   52 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/mmc/host/dw_mmc.h  |    4 ++++
 include/linux/mmc/dw_mmc.h |    3 +++
 3 files changed, 59 insertions(+)

Comments

Zhangfei Gao Dec. 17, 2013, 8:11 a.m. UTC | #1

On 12/17/2013 01:01 AM, [e-mail] wrote:
> From: Dinh Nguyen <[e-mail]>
>
> This patch will enable the SDMMC_CMD_USE_HOLD_REG bit when the slot is
> operating all timing modes, except for SDR50, DDR50, SDR104, and MMC_HS200.
>
> According to the Synopsys databook :"To meet the relatively high Input Hold
> Time requirement for SDR12, SDR25, and other MMC speed modes, you should
> program bit[29]use_hold_Reg of the CMD register to 1'b1;"..."However, for
> the higher speed modes of SDR104, SDR50 and DDR50, you can meet the much
> smaller Input Hold Time requirement of 0.8ns by bypassing the Hold Register
> (Path A in Figure 10-8, programming CMD.use_hold_reg = 1'b0) and then adding
> delay elements on the output path as indicated.
>
> Also, "Never set CMD.use_hold_reg = 1 and cclk_in_drv phase shift to 0 at the
> same time. This would add an extra one-cycle delay on the output path, resulting
> in incorrect behavior."
>
> This patch also checks the IHR(Implement Hold Register) in the HCON register.
>
> This information is taking from the v2.50a of the Synopsys Designware Cores
> Mobile Storage Host Databook.
>
> Signed-off-by: Dinh Nguyen <[e-mail]>
> Acked-by: Arnd Bergmann <[e-mail]>
> Acked-by: Heiko Stuebner <[e-mail]>
> Tested-by: Heiko Stuebner <[e-mail]>
> ---
> v4: use_hold_reg should be set for all modes when cclk_in_drv is non-zero.
> v3: Read the IHR(Implement Hold Register) in the HCON
> v2: Add check for cclk_in_drv phase shift in conjunction with use_hold_reg.
> ---
>   drivers/mmc/host/dw_mmc.c  |   52 ++++++++++++++++++++++++++++++++++++++++++++
>   drivers/mmc/host/dw_mmc.h  |    4 ++++
>   include/linux/mmc/dw_mmc.h |    3 +++
>   3 files changed, 59 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>   	/*
>   	 * Use mirror of ios->clock to prevent race with mmc
>   	 * core ios update when finding the minimum.
> @@ -2339,6 +2361,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>   	const struct dw_mci_drv_data *drv_data = host->drv_data;
>   	int idx, ret;
>   	u32 clock_frequency;
> +	int sdr_timing[2];
> +	int ddr_timing[2];
>
>   	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>   	if (!pdata) {
> @@ -2389,6 +2413,25 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>   	if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
>   		pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
>
> +	/* Check for the "samsung,dw-mshc-sdr-timing" and the
> +	 * "samsung,dw-mshc-ddr-timing" bindings as this will tell us if we
> +	 * can safely set the SDMMC_CMD_USE_HOLD_REG bit. The second paramater
> +	 * in these 2 bindings is the value of the cclk_in_drv. If cclk_in_drv
> +	 * is 0, we cannot set the SDMMC_CMD_USE_HOLD_REG bit. The default
> +	 * behavior will be to set cclk_in_drv, as some platforms do not have
> +	 * to set the sdr or ddr timing parameters.
> +	 */
> +	sdr_timing[1] = ddr_timing[1] = 1;
> +	of_property_read_u32_array(np,
> +			"samsung,dw-mshc-sdr-timing", sdr_timing, 2);
> +
> +	of_property_read_u32_array(np,
> +			"samsung,dw-mshc-ddr-timing", ddr_timing, 2);
> +
> +	pdata->cclk_in_drv = 1;
> +	if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
> +		pdata->cclk_in_drv = 0;
> +

Have some concern about whether it is suitable putting "samsung,~" 
property in dw_mmc.c, is it supposed to be platform related?
Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
If they are really commonly used, how about change name and define in 
Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dinh Nguyen Dec. 17, 2013, 2:03 p.m. UTC | #2

Hi Zhangfei,

On 12/17/13 2:11 AM, zhangfei wrote:
>
>
> On 12/17/2013 01:01 AM, [e-mail] wrote:
>> From: Dinh Nguyen <[e-mail]>
>>
>> This patch will enable the SDMMC_CMD_USE_HOLD_REG bit when the slot is
>> operating all timing modes, except for SDR50, DDR50, SDR104, and
>> MMC_HS200.
>>
>> According to the Synopsys databook :"To meet the relatively high
>> Input Hold
>> Time requirement for SDR12, SDR25, and other MMC speed modes, you should
>> program bit[29]use_hold_Reg of the CMD register to 1'b1;"..."However,
>> for
>> the higher speed modes of SDR104, SDR50 and DDR50, you can meet the much
>> smaller Input Hold Time requirement of 0.8ns by bypassing the Hold
>> Register
>> (Path A in Figure 10-8, programming CMD.use_hold_reg = 1'b0) and then
>> adding
>> delay elements on the output path as indicated.
>>
>> Also, "Never set CMD.use_hold_reg = 1 and cclk_in_drv phase shift to
>> 0 at the
>> same time. This would add an extra one-cycle delay on the output
>> path, resulting
>> in incorrect behavior."
>>
>> This patch also checks the IHR(Implement Hold Register) in the HCON
>> register.
>>
>> This information is taking from the v2.50a of the Synopsys Designware
>> Cores
>> Mobile Storage Host Databook.
>>
>> Signed-off-by: Dinh Nguyen <[e-mail]>
>> Acked-by: Arnd Bergmann <[e-mail]>
>> Acked-by: Heiko Stuebner <[e-mail]>
>> Tested-by: Heiko Stuebner <[e-mail]>
>> ---
>> v4: use_hold_reg should be set for all modes when cclk_in_drv is
>> non-zero.
>> v3: Read the IHR(Implement Hold Register) in the HCON
>> v2: Add check for cclk_in_drv phase shift in conjunction with
>> use_hold_reg.
>> ---
>>   drivers/mmc/host/dw_mmc.c  |   52
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/mmc/host/dw_mmc.h  |    4 ++++
>>   include/linux/mmc/dw_mmc.h |    3 +++
>>   3 files changed, 59 insertions(+)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>       /*
>>        * Use mirror of ios->clock to prevent race with mmc
>>        * core ios update when finding the minimum.
>> @@ -2339,6 +2361,8 @@ static struct dw_mci_board
>> *dw_mci_parse_dt(struct dw_mci *host)
>>       const struct dw_mci_drv_data *drv_data = host->drv_data;
>>       int idx, ret;
>>       u32 clock_frequency;
>> +    int sdr_timing[2];
>> +    int ddr_timing[2];
>>
>>       pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>       if (!pdata) {
>> @@ -2389,6 +2413,25 @@ static struct dw_mci_board
>> *dw_mci_parse_dt(struct dw_mci *host)
>>       if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
>>           pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
>>
>> +    /* Check for the "samsung,dw-mshc-sdr-timing" and the
>> +     * "samsung,dw-mshc-ddr-timing" bindings as this will tell us if we
>> +     * can safely set the SDMMC_CMD_USE_HOLD_REG bit. The second
>> paramater
>> +     * in these 2 bindings is the value of the cclk_in_drv. If
>> cclk_in_drv
>> +     * is 0, we cannot set the SDMMC_CMD_USE_HOLD_REG bit. The default
>> +     * behavior will be to set cclk_in_drv, as some platforms do not
>> have
>> +     * to set the sdr or ddr timing parameters.
>> +     */
>> +    sdr_timing[1] = ddr_timing[1] = 1;
>> +    of_property_read_u32_array(np,
>> +            "samsung,dw-mshc-sdr-timing", sdr_timing, 2);
>> +
>> +    of_property_read_u32_array(np,
>> +            "samsung,dw-mshc-ddr-timing", ddr_timing, 2);
>> +
>> +    pdata->cclk_in_drv = 1;
>> +    if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>> +        pdata->cclk_in_drv = 0;
>> +
>
> Have some concern about whether it is suitable putting "samsung,~"
> property in dw_mmc.c, is it supposed to be platform related?
> Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
> If they are really commonly used, how about change name and define in
> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?
I had submitted a patch to make this a common binding before:

http://www.spinics.net/lists/devicetree/msg00638.html

I think the ultimate conclusion to that thread was that its perfectly
acceptable to re-use bindings from other
platforms.

Dinh
>
> Thanks

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Zhangfei Gao Dec. 17, 2013, 2:54 p.m. UTC | #3

On 12/17/2013 10:03 PM, Dinh Nguyen wrote:
> Hi Zhangfei,
>
> On 12/17/13 2:11 AM, zhangfei wrote:
>>
>>
>> On 12/17/2013 01:01 AM, [e-mail] wrote:
>>> From: Dinh Nguyen <[e-mail]>
>>>
>>> This patch will enable the SDMMC_CMD_USE_HOLD_REG bit when the slot is
>>> operating all timing modes, except for SDR50, DDR50, SDR104, and
>>> MMC_HS200.
>>>
>>> According to the Synopsys databook :"To meet the relatively high
>>> Input Hold
>>> Time requirement for SDR12, SDR25, and other MMC speed modes, you should
>>> program bit[29]use_hold_Reg of the CMD register to 1'b1;"..."However,
>>> for
>>> the higher speed modes of SDR104, SDR50 and DDR50, you can meet the much
>>> smaller Input Hold Time requirement of 0.8ns by bypassing the Hold
>>> Register
>>> (Path A in Figure 10-8, programming CMD.use_hold_reg = 1'b0) and then
>>> adding
>>> delay elements on the output path as indicated.
>>>
>>> Also, "Never set CMD.use_hold_reg = 1 and cclk_in_drv phase shift to
>>> 0 at the
>>> same time. This would add an extra one-cycle delay on the output
>>> path, resulting
>>> in incorrect behavior."
>>>
>>> This patch also checks the IHR(Implement Hold Register) in the HCON
>>> register.
>>>
>>> This information is taking from the v2.50a of the Synopsys Designware
>>> Cores
>>> Mobile Storage Host Databook.
>>>
>>> Signed-off-by: Dinh Nguyen <[e-mail]>
>>> Acked-by: Arnd Bergmann <[e-mail]>
>>> Acked-by: Heiko Stuebner <[e-mail]>
>>> Tested-by: Heiko Stuebner <[e-mail]>
>>> ---
>>> v4: use_hold_reg should be set for all modes when cclk_in_drv is
>>> non-zero.
>>> v3: Read the IHR(Implement Hold Register) in the HCON
>>> v2: Add check for cclk_in_drv phase shift in conjunction with
>>> use_hold_reg.
>>> ---
>>>    drivers/mmc/host/dw_mmc.c  |   52
>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>    drivers/mmc/host/dw_mmc.h  |    4 ++++
>>>    include/linux/mmc/dw_mmc.h |    3 +++
>>>    3 files changed, 59 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>        /*
>>>         * Use mirror of ios->clock to prevent race with mmc
>>>         * core ios update when finding the minimum.
>>> @@ -2339,6 +2361,8 @@ static struct dw_mci_board
>>> *dw_mci_parse_dt(struct dw_mci *host)
>>>        const struct dw_mci_drv_data *drv_data = host->drv_data;
>>>        int idx, ret;
>>>        u32 clock_frequency;
>>> +    int sdr_timing[2];
>>> +    int ddr_timing[2];
>>>
>>>        pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>>        if (!pdata) {
>>> @@ -2389,6 +2413,25 @@ static struct dw_mci_board
>>> *dw_mci_parse_dt(struct dw_mci *host)
>>>        if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
>>>            pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
>>>
>>> +    /* Check for the "samsung,dw-mshc-sdr-timing" and the
>>> +     * "samsung,dw-mshc-ddr-timing" bindings as this will tell us if we
>>> +     * can safely set the SDMMC_CMD_USE_HOLD_REG bit. The second
>>> paramater
>>> +     * in these 2 bindings is the value of the cclk_in_drv. If
>>> cclk_in_drv
>>> +     * is 0, we cannot set the SDMMC_CMD_USE_HOLD_REG bit. The default
>>> +     * behavior will be to set cclk_in_drv, as some platforms do not
>>> have
>>> +     * to set the sdr or ddr timing parameters.
>>> +     */
>>> +    sdr_timing[1] = ddr_timing[1] = 1;
>>> +    of_property_read_u32_array(np,
>>> +            "samsung,dw-mshc-sdr-timing", sdr_timing, 2);
>>> +
>>> +    of_property_read_u32_array(np,
>>> +            "samsung,dw-mshc-ddr-timing", ddr_timing, 2);
>>> +
>>> +    pdata->cclk_in_drv = 1;
>>> +    if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>>> +        pdata->cclk_in_drv = 0;
>>> +
>>
>> Have some concern about whether it is suitable putting "samsung,~"
>> property in dw_mmc.c, is it supposed to be platform related?
>> Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
>> If they are really commonly used, how about change name and define in
>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?
> I had submitted a patch to make this a common binding before:
>
> http://www.spinics.net/lists/devicetree/msg00638.html
>
> I think the ultimate conclusion to that thread was that its perfectly
> acceptable to re-use bindings from other
> platforms.
>

Hmm, ususally I may look for the properties of dw_mmc.c in 
synopsys-dw-mshc.txt.
If this is the conclusion before, then just ignore this noise.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jaehoon Chung Dec. 26, 2013, 2:57 a.m. UTC | #4

On 12/17/2013 11:54 PM, zhangfei wrote:
> 
> 
> On 12/17/2013 10:03 PM, Dinh Nguyen wrote:
>> Hi Zhangfei,
>>
>> On 12/17/13 2:11 AM, zhangfei wrote:
>>>
>>>
>>> On 12/17/2013 01:01 AM, [e-mail] wrote:
>>>> From: Dinh Nguyen <[e-mail]>
>>>>
>>>> This patch will enable the SDMMC_CMD_USE_HOLD_REG bit when the slot is
>>>> operating all timing modes, except for SDR50, DDR50, SDR104, and
>>>> MMC_HS200.
>>>>
>>>> According to the Synopsys databook :"To meet the relatively high
>>>> Input Hold
>>>> Time requirement for SDR12, SDR25, and other MMC speed modes, you should
>>>> program bit[29]use_hold_Reg of the CMD register to 1'b1;"..."However,
>>>> for
>>>> the higher speed modes of SDR104, SDR50 and DDR50, you can meet the much
>>>> smaller Input Hold Time requirement of 0.8ns by bypassing the Hold
>>>> Register
>>>> (Path A in Figure 10-8, programming CMD.use_hold_reg = 1'b0) and then
>>>> adding
>>>> delay elements on the output path as indicated.
>>>>
>>>> Also, "Never set CMD.use_hold_reg = 1 and cclk_in_drv phase shift to
>>>> 0 at the
>>>> same time. This would add an extra one-cycle delay on the output
>>>> path, resulting
>>>> in incorrect behavior."
>>>>
>>>> This patch also checks the IHR(Implement Hold Register) in the HCON
>>>> register.
>>>>
>>>> This information is taking from the v2.50a of the Synopsys Designware
>>>> Cores
>>>> Mobile Storage Host Databook.
>>>>
>>>> Signed-off-by: Dinh Nguyen <[e-mail]>
>>>> Acked-by: Arnd Bergmann <[e-mail]>
>>>> Acked-by: Heiko Stuebner <[e-mail]>
>>>> Tested-by: Heiko Stuebner <[e-mail]>
>>>> ---
>>>> v4: use_hold_reg should be set for all modes when cclk_in_drv is
>>>> non-zero.
>>>> v3: Read the IHR(Implement Hold Register) in the HCON
>>>> v2: Add check for cclk_in_drv phase shift in conjunction with
>>>> use_hold_reg.
>>>> ---
>>>>    drivers/mmc/host/dw_mmc.c  |   52
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>    drivers/mmc/host/dw_mmc.h  |    4 ++++
>>>>    include/linux/mmc/dw_mmc.h |    3 +++
>>>>    3 files changed, 59 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>        /*
>>>>         * Use mirror of ios->clock to prevent race with mmc
>>>>         * core ios update when finding the minimum.
>>>> @@ -2339,6 +2361,8 @@ static struct dw_mci_board
>>>> *dw_mci_parse_dt(struct dw_mci *host)
>>>>        const struct dw_mci_drv_data *drv_data = host->drv_data;
>>>>        int idx, ret;
>>>>        u32 clock_frequency;
>>>> +    int sdr_timing[2];
>>>> +    int ddr_timing[2];
>>>>
>>>>        pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>>>        if (!pdata) {
>>>> @@ -2389,6 +2413,25 @@ static struct dw_mci_board
>>>> *dw_mci_parse_dt(struct dw_mci *host)
>>>>        if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
>>>>            pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
>>>>
>>>> +    /* Check for the "samsung,dw-mshc-sdr-timing" and the
>>>> +     * "samsung,dw-mshc-ddr-timing" bindings as this will tell us if we
>>>> +     * can safely set the SDMMC_CMD_USE_HOLD_REG bit. The second
>>>> paramater
>>>> +     * in these 2 bindings is the value of the cclk_in_drv. If
>>>> cclk_in_drv
>>>> +     * is 0, we cannot set the SDMMC_CMD_USE_HOLD_REG bit. The default
>>>> +     * behavior will be to set cclk_in_drv, as some platforms do not
>>>> have
>>>> +     * to set the sdr or ddr timing parameters.
>>>> +     */
>>>> +    sdr_timing[1] = ddr_timing[1] = 1;
>>>> +    of_property_read_u32_array(np,
>>>> +            "samsung,dw-mshc-sdr-timing", sdr_timing, 2);
>>>> +
>>>> +    of_property_read_u32_array(np,
>>>> +            "samsung,dw-mshc-ddr-timing", ddr_timing, 2);
>>>> +
>>>> +    pdata->cclk_in_drv = 1;
>>>> +    if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>>>> +        pdata->cclk_in_drv = 0;
>>>> +
>>>
>>> Have some concern about whether it is suitable putting "samsung,~"
>>> property in dw_mmc.c, is it supposed to be platform related?
>>> Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
>>> If they are really commonly used, how about change name and define in
>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?
>> I had submitted a patch to make this a common binding before:
>>
>> http://www.spinics.net/lists/devicetree/msg00638.html
>>
>> I think the ultimate conclusion to that thread was that its perfectly
>> acceptable to re-use bindings from other
>> platforms.
>>
> 
> Hmm, ususally I may look for the properties of dw_mmc.c in synopsys-dw-mshc.txt.
> If this is the conclusion before, then just ignore this noise.
If can be removed the samsung property, then i think property of clock timing can be used into dw-mmc.c
But if samsung property is used, well. I think right that it's used into dw_mmc-exynos.c.
Dw-mmc.c is general driver..so we don't want to include any SoC specific code.

If i missed something, then let me know, plz.

Best Regards,
Jaehoon Chung
> 
> Thanks
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dinh Nguyen Dec. 26, 2013, 5:26 p.m. UTC | #5

Hi Jaehoon,

On 12/25/13 8:57 PM, Jaehoon Chung wrote:
> On 12/17/2013 11:54 PM, zhangfei wrote:
>>
>> On 12/17/2013 10:03 PM, Dinh Nguyen wrote:
>>> Hi Zhangfei,
>>>
>>> On 12/17/13 2:11 AM, zhangfei wrote:
>>>>
>>>> On 12/17/2013 01:01 AM, [e-mail] wrote:
>>>>> From: Dinh Nguyen <[e-mail]>
>>>>>
>>>>> This patch will enable the SDMMC_CMD_USE_HOLD_REG bit when the slot is
>>>>> operating all timing modes, except for SDR50, DDR50, SDR104, and
>>>>> MMC_HS200.
>>>>>
>>>>> According to the Synopsys databook :"To meet the relatively high
>>>>> Input Hold
>>>>> Time requirement for SDR12, SDR25, and other MMC speed modes, you should
>>>>> program bit[29]use_hold_Reg of the CMD register to 1'b1;"..."However,
>>>>> for
>>>>> the higher speed modes of SDR104, SDR50 and DDR50, you can meet the much
>>>>> smaller Input Hold Time requirement of 0.8ns by bypassing the Hold
>>>>> Register
>>>>> (Path A in Figure 10-8, programming CMD.use_hold_reg = 1'b0) and then
>>>>> adding
>>>>> delay elements on the output path as indicated.
>>>>>
>>>>> Also, "Never set CMD.use_hold_reg = 1 and cclk_in_drv phase shift to
>>>>> 0 at the
>>>>> same time. This would add an extra one-cycle delay on the output
>>>>> path, resulting
>>>>> in incorrect behavior."
>>>>>
>>>>> This patch also checks the IHR(Implement Hold Register) in the HCON
>>>>> register.
>>>>>
>>>>> This information is taking from the v2.50a of the Synopsys Designware
>>>>> Cores
>>>>> Mobile Storage Host Databook.
>>>>>
>>>>> Signed-off-by: Dinh Nguyen <[e-mail]>
>>>>> Acked-by: Arnd Bergmann <[e-mail]>
>>>>> Acked-by: Heiko Stuebner <[e-mail]>
>>>>> Tested-by: Heiko Stuebner <[e-mail]>
>>>>> ---
>>>>> v4: use_hold_reg should be set for all modes when cclk_in_drv is
>>>>> non-zero.
>>>>> v3: Read the IHR(Implement Hold Register) in the HCON
>>>>> v2: Add check for cclk_in_drv phase shift in conjunction with
>>>>> use_hold_reg.
>>>>> ---
>>>>>    drivers/mmc/host/dw_mmc.c  |   52
>>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>>    drivers/mmc/host/dw_mmc.h  |    4 ++++
>>>>>    include/linux/mmc/dw_mmc.h |    3 +++
>>>>>    3 files changed, 59 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>>        /*
>>>>>         * Use mirror of ios->clock to prevent race with mmc
>>>>>         * core ios update when finding the minimum.
>>>>> @@ -2339,6 +2361,8 @@ static struct dw_mci_board
>>>>> *dw_mci_parse_dt(struct dw_mci *host)
>>>>>        const struct dw_mci_drv_data *drv_data = host->drv_data;
>>>>>        int idx, ret;
>>>>>        u32 clock_frequency;
>>>>> +    int sdr_timing[2];
>>>>> +    int ddr_timing[2];
>>>>>
>>>>>        pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>>>>        if (!pdata) {
>>>>> @@ -2389,6 +2413,25 @@ static struct dw_mci_board
>>>>> *dw_mci_parse_dt(struct dw_mci *host)
>>>>>        if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
>>>>>            pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
>>>>>
>>>>> +    /* Check for the "samsung,dw-mshc-sdr-timing" and the
>>>>> +     * "samsung,dw-mshc-ddr-timing" bindings as this will tell us if we
>>>>> +     * can safely set the SDMMC_CMD_USE_HOLD_REG bit. The second
>>>>> paramater
>>>>> +     * in these 2 bindings is the value of the cclk_in_drv. If
>>>>> cclk_in_drv
>>>>> +     * is 0, we cannot set the SDMMC_CMD_USE_HOLD_REG bit. The default
>>>>> +     * behavior will be to set cclk_in_drv, as some platforms do not
>>>>> have
>>>>> +     * to set the sdr or ddr timing parameters.
>>>>> +     */
>>>>> +    sdr_timing[1] = ddr_timing[1] = 1;
>>>>> +    of_property_read_u32_array(np,
>>>>> +            "samsung,dw-mshc-sdr-timing", sdr_timing, 2);
>>>>> +
>>>>> +    of_property_read_u32_array(np,
>>>>> +            "samsung,dw-mshc-ddr-timing", ddr_timing, 2);
>>>>> +
>>>>> +    pdata->cclk_in_drv = 1;
>>>>> +    if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>>>>> +        pdata->cclk_in_drv = 0;
>>>>> +
>>>> Have some concern about whether it is suitable putting "samsung,~"
>>>> property in dw_mmc.c, is it supposed to be platform related?
>>>> Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
>>>> If they are really commonly used, how about change name and define in
>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?
>>> I had submitted a patch to make this a common binding before:
>>>
>>> http://www.spinics.net/lists/devicetree/msg00638.html
>>>
>>> I think the ultimate conclusion to that thread was that its perfectly
>>> acceptable to re-use bindings from other
>>> platforms.
>>>
>> Hmm, ususally I may look for the properties of dw_mmc.c in synopsys-dw-mshc.txt.
>> If this is the conclusion before, then just ignore this noise.
> If can be removed the samsung property, then i think property of clock timing can be used into dw-mmc.c
> But if samsung property is used, well. I think right that it's used into dw_mmc-exynos.c.
> Dw-mmc.c is general driver..so we don't want to include any SoC specific code.
Then do you suggest I go forward with an attempt to add a new generic
"snps,dw-mshc-sdr-timing"
binding?

Dinh
>
> If i missed something, then let me know, plz.
>
> Best Regards,
> Jaehoon Chung
>> Thanks
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dinh Nguyen Jan. 8, 2014, 12:18 a.m. UTC | #6

On Thu, 2013-12-26 at 11:26 -0600, Dinh Nguyen wrote:
> Hi Jaehoon,
> 
> On 12/25/13 8:57 PM, Jaehoon Chung wrote:
> > On 12/17/2013 11:54 PM, zhangfei wrote:
> >>
> >> On 12/17/2013 10:03 PM, Dinh Nguyen wrote:
> >>> Hi Zhangfei,
> >>>
> >>> On 12/17/13 2:11 AM, zhangfei wrote:
> >>>>
> >>>> On 12/17/2013 01:01 AM, [e-mail] wrote:
> >>>>> From: Dinh Nguyen <[e-mail]>
> >>>>>
> >>>>> This patch will enable the SDMMC_CMD_USE_HOLD_REG bit when the slot is
> >>>>> operating all timing modes, except for SDR50, DDR50, SDR104, and
> >>>>> MMC_HS200.
> >>>>>
> >>>>> According to the Synopsys databook :"To meet the relatively high
> >>>>> Input Hold
> >>>>> Time requirement for SDR12, SDR25, and other MMC speed modes, you should
> >>>>> program bit[29]use_hold_Reg of the CMD register to 1'b1;"..."However,
> >>>>> for
> >>>>> the higher speed modes of SDR104, SDR50 and DDR50, you can meet the much
> >>>>> smaller Input Hold Time requirement of 0.8ns by bypassing the Hold
> >>>>> Register
> >>>>> (Path A in Figure 10-8, programming CMD.use_hold_reg = 1'b0) and then
> >>>>> adding
> >>>>> delay elements on the output path as indicated.
> >>>>>
> >>>>> Also, "Never set CMD.use_hold_reg = 1 and cclk_in_drv phase shift to
> >>>>> 0 at the
> >>>>> same time. This would add an extra one-cycle delay on the output
> >>>>> path, resulting
> >>>>> in incorrect behavior."
> >>>>>
> >>>>> This patch also checks the IHR(Implement Hold Register) in the HCON
> >>>>> register.
> >>>>>
> >>>>> This information is taking from the v2.50a of the Synopsys Designware
> >>>>> Cores
> >>>>> Mobile Storage Host Databook.
> >>>>>
> >>>>> Signed-off-by: Dinh Nguyen <[e-mail]>
> >>>>> Acked-by: Arnd Bergmann <[e-mail]>
> >>>>> Acked-by: Heiko Stuebner <[e-mail]>
> >>>>> Tested-by: Heiko Stuebner <[e-mail]>
> >>>>> ---
> >>>>> v4: use_hold_reg should be set for all modes when cclk_in_drv is
> >>>>> non-zero.
> >>>>> v3: Read the IHR(Implement Hold Register) in the HCON
> >>>>> v2: Add check for cclk_in_drv phase shift in conjunction with
> >>>>> use_hold_reg.
> >>>>> ---
> >>>>>    drivers/mmc/host/dw_mmc.c  |   52
> >>>>> ++++++++++++++++++++++++++++++++++++++++++++
> >>>>>    drivers/mmc/host/dw_mmc.h  |    4 ++++
> >>>>>    include/linux/mmc/dw_mmc.h |    3 +++
> >>>>>    3 files changed, 59 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> >>>>>        /*
> >>>>>         * Use mirror of ios->clock to prevent race with mmc
> >>>>>         * core ios update when finding the minimum.
> >>>>> @@ -2339,6 +2361,8 @@ static struct dw_mci_board
> >>>>> *dw_mci_parse_dt(struct dw_mci *host)
> >>>>>        const struct dw_mci_drv_data *drv_data = host->drv_data;
> >>>>>        int idx, ret;
> >>>>>        u32 clock_frequency;
> >>>>> +    int sdr_timing[2];
> >>>>> +    int ddr_timing[2];
> >>>>>
> >>>>>        pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> >>>>>        if (!pdata) {
> >>>>> @@ -2389,6 +2413,25 @@ static struct dw_mci_board
> >>>>> *dw_mci_parse_dt(struct dw_mci *host)
> >>>>>        if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
> >>>>>            pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
> >>>>>
> >>>>> +    /* Check for the "samsung,dw-mshc-sdr-timing" and the
> >>>>> +     * "samsung,dw-mshc-ddr-timing" bindings as this will tell us if we
> >>>>> +     * can safely set the SDMMC_CMD_USE_HOLD_REG bit. The second
> >>>>> paramater
> >>>>> +     * in these 2 bindings is the value of the cclk_in_drv. If
> >>>>> cclk_in_drv
> >>>>> +     * is 0, we cannot set the SDMMC_CMD_USE_HOLD_REG bit. The default
> >>>>> +     * behavior will be to set cclk_in_drv, as some platforms do not
> >>>>> have
> >>>>> +     * to set the sdr or ddr timing parameters.
> >>>>> +     */
> >>>>> +    sdr_timing[1] = ddr_timing[1] = 1;
> >>>>> +    of_property_read_u32_array(np,
> >>>>> +            "samsung,dw-mshc-sdr-timing", sdr_timing, 2);
> >>>>> +
> >>>>> +    of_property_read_u32_array(np,
> >>>>> +            "samsung,dw-mshc-ddr-timing", ddr_timing, 2);
> >>>>> +
> >>>>> +    pdata->cclk_in_drv = 1;
> >>>>> +    if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
> >>>>> +        pdata->cclk_in_drv = 0;
> >>>>> +
> >>>> Have some concern about whether it is suitable putting "samsung,~"
> >>>> property in dw_mmc.c, is it supposed to be platform related?
> >>>> Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
> >>>> If they are really commonly used, how about change name and define in
> >>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?
> >>> I had submitted a patch to make this a common binding before:
> >>>
> >>> http://www.spinics.net/lists/devicetree/msg00638.html
> >>>
> >>> I think the ultimate conclusion to that thread was that its perfectly
> >>> acceptable to re-use bindings from other
> >>> platforms.
> >>>
> >> Hmm, ususally I may look for the properties of dw_mmc.c in synopsys-dw-mshc.txt.
> >> If this is the conclusion before, then just ignore this noise.
> > If can be removed the samsung property, then i think property of clock timing can be used into dw-mmc.c
> > But if samsung property is used, well. I think right that it's used into dw_mmc-exynos.c.
> > Dw-mmc.c is general driver..so we don't want to include any SoC specific code.
> Then do you suggest I go forward with an attempt to add a new generic
> "snps,dw-mshc-sdr-timing"
> binding?

Ping Jaehoon?

Do you think I need to add a generic "snps,dw-mshc-sdr-timing" and
"snps,dw-mshc-ddr-timing" bindings then?

Dinh
> 
> Dinh
> >
> > If i missed something, then let me know, plz.
> >
> > Best Regards,
> > Jaehoon Chung
> >> Thanks
> >>
> 
> 



--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jaehoon Chung Jan. 8, 2014, 12:37 a.m. UTC | #7

Hi, Dinh.

Sorry for replying too late.

..[snip]..
>>>>>>> +    sdr_timing[1] = ddr_timing[1] = 1;
>>>>>>> +    of_property_read_u32_array(np,
>>>>>>> +            "samsung,dw-mshc-sdr-timing", sdr_timing, 2);
>>>>>>> +
>>>>>>> +    of_property_read_u32_array(np,
>>>>>>> +            "samsung,dw-mshc-ddr-timing", ddr_timing, 2);
>>>>>>> +
>>>>>>> +    pdata->cclk_in_drv = 1;
>>>>>>> +    if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>>>>>>> +        pdata->cclk_in_drv = 0;
>>>>>>> +
>>>>>> Have some concern about whether it is suitable putting "samsung,~"
>>>>>> property in dw_mmc.c, is it supposed to be platform related?
>>>>>> Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
>>>>>> If they are really commonly used, how about change name and define in
>>>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?
>>>>> I had submitted a patch to make this a common binding before:
>>>>>
>>>>> http://www.spinics.net/lists/devicetree/msg00638.html
>>>>>
>>>>> I think the ultimate conclusion to that thread was that its perfectly
>>>>> acceptable to re-use bindings from other
>>>>> platforms.
>>>>>
>>>> Hmm, ususally I may look for the properties of dw_mmc.c in synopsys-dw-mshc.txt.
>>>> If this is the conclusion before, then just ignore this noise.
>>> If can be removed the samsung property, then i think property of clock timing can be used into dw-mmc.c
>>> But if samsung property is used, well. I think right that it's used into dw_mmc-exynos.c.
>>> Dw-mmc.c is general driver..so we don't want to include any SoC specific code.
>> Then do you suggest I go forward with an attempt to add a new generic
>> "snps,dw-mshc-sdr-timing"
>> binding?
> 
> Ping Jaehoon?
> 
> Do you think I need to add a generic "snps,dw-mshc-sdr-timing" and
> "snps,dw-mshc-ddr-timing" bindings then?

Well, i think it's also something wrong. ddr/sdr-timing is exynos specific value, not synopsys value.
If synopsys use the sdr/ddr timing, then it's right that "snps, dw-mshc-sdr/ddr-timing" is used.
But i didn't see sdr/ddr timing in synopsys DoC.
I know you want to control the hold-reg bit. 
But this approach is not good.
Rather, how about using the callback function for exynos specific value.
Then other SoC can also use it.

Best Regards,
Jaehoon Chung

> 
> Dinh
>>
>> Dinh
>>>
>>> If i missed something, then let me know, plz.
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>> Thanks
>>>>
>>
>>
> 
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dinh Nguyen Jan. 8, 2014, 2:12 p.m. UTC | #8

On 1/7/14 6:37 PM, Jaehoon Chung wrote:
> Hi, Dinh.
>
> Sorry for replying too late.
>
> ..[snip]..
>>>>>>>> +    sdr_timing[1] = ddr_timing[1] = 1;
>>>>>>>> +    of_property_read_u32_array(np,
>>>>>>>> +            "samsung,dw-mshc-sdr-timing", sdr_timing, 2);
>>>>>>>> +
>>>>>>>> +    of_property_read_u32_array(np,
>>>>>>>> +            "samsung,dw-mshc-ddr-timing", ddr_timing, 2);
>>>>>>>> +
>>>>>>>> +    pdata->cclk_in_drv = 1;
>>>>>>>> +    if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>>>>>>>> +        pdata->cclk_in_drv = 0;
>>>>>>>> +
>>>>>>> Have some concern about whether it is suitable putting "samsung,~"
>>>>>>> property in dw_mmc.c, is it supposed to be platform related?
>>>>>>> Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
>>>>>>> If they are really commonly used, how about change name and define in
>>>>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?
>>>>>> I had submitted a patch to make this a common binding before:
>>>>>>
>>>>>> http://www.spinics.net/lists/devicetree/msg00638.html
>>>>>>
>>>>>> I think the ultimate conclusion to that thread was that its perfectly
>>>>>> acceptable to re-use bindings from other
>>>>>> platforms.
>>>>>>
>>>>> Hmm, ususally I may look for the properties of dw_mmc.c in synopsys-dw-mshc.txt.
>>>>> If this is the conclusion before, then just ignore this noise.
>>>> If can be removed the samsung property, then i think property of clock timing can be used into dw-mmc.c
>>>> But if samsung property is used, well. I think right that it's used into dw_mmc-exynos.c.
>>>> Dw-mmc.c is general driver..so we don't want to include any SoC specific code.
>>> Then do you suggest I go forward with an attempt to add a new generic
>>> "snps,dw-mshc-sdr-timing"
>>> binding?
>> Ping Jaehoon?
>>
>> Do you think I need to add a generic "snps,dw-mshc-sdr-timing" and
>> "snps,dw-mshc-ddr-timing" bindings then?
> Well, i think it's also something wrong. ddr/sdr-timing is exynos specific value, not synopsys value.
> If synopsys use the sdr/ddr timing, then it's right that "snps, dw-mshc-sdr/ddr-timing" is used.
I went through the databook and I think that the timing values are
mentioned throughout the spec. It
just does not explicitly mentions ddr/sdr timing, but it is mentioned as
cclk_in_drv(aka drvsel), and
cclk_in_sample(aka smpsel). Even the macro in dw_mmc-exynos,
SDMMC_CLKSEL_CCLK_DRIVE() and
SDMMC_CLKSEL_CCLK_SAMPLE() is stating this.

So I do not believe that "samsung, dw-mshc-sdr-timing" and
"samsung,dw-mshc-ddr-timing" are not
exclusive to only the exynos platform. Just the fact that the SOCFPGA
platform can re-use the same
"samsung,dw-mshc-sdr-timing" property proves that this is not just an
exynos specific value.
> But i didn't see sdr/ddr timing in synopsys DoC.
> I know you want to control the hold-reg bit. 
> But this approach is not good.

The spec has a table on when to use the hold-reg bit. The conditions for
using the hold-reg bit is
only dependent the hold-reg hw implementation and the value of
cclk_in_drv. The value of cclk_in_drv
is specified by the 2nd parameter of "samsung,dw-mshc-sdr-timing".

So I don't understand why you think this approach is "not good"?

Thanks,
Dinh
> Rather, how about using the callback function for exynos specific value.
> Then other SoC can also use it.
>
> Best Regards,
> Jaehoon Chung
>
>> Dinh
>>> Dinh
>>>> If i missed something, then let me know, plz.
>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>> Thanks
>>>>>
>>>
>>
>>
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jaehoon Chung Jan. 9, 2014, 3:41 a.m. UTC | #9

Dear, Dinh

On 01/08/2014 11:12 PM, Dinh Nguyen wrote:
> 
> On 1/7/14 6:37 PM, Jaehoon Chung wrote:
>> Hi, Dinh.
>>
>> Sorry for replying too late.
>>
>> ..[snip]..
>>>>>>>>> +    sdr_timing[1] = ddr_timing[1] = 1;
>>>>>>>>> +    of_property_read_u32_array(np,
>>>>>>>>> +            "samsung,dw-mshc-sdr-timing", sdr_timing, 2);
>>>>>>>>> +
>>>>>>>>> +    of_property_read_u32_array(np,
>>>>>>>>> +            "samsung,dw-mshc-ddr-timing", ddr_timing, 2);
>>>>>>>>> +
>>>>>>>>> +    pdata->cclk_in_drv = 1;
>>>>>>>>> +    if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>>>>>>>>> +        pdata->cclk_in_drv = 0;
>>>>>>>>> +
>>>>>>>> Have some concern about whether it is suitable putting "samsung,~"
>>>>>>>> property in dw_mmc.c, is it supposed to be platform related?
>>>>>>>> Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
>>>>>>>> If they are really commonly used, how about change name and define in
>>>>>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?
>>>>>>> I had submitted a patch to make this a common binding before:
>>>>>>>
>>>>>>> http://www.spinics.net/lists/devicetree/msg00638.html
>>>>>>>
>>>>>>> I think the ultimate conclusion to that thread was that its perfectly
>>>>>>> acceptable to re-use bindings from other
>>>>>>> platforms.
>>>>>>>
>>>>>> Hmm, ususally I may look for the properties of dw_mmc.c in synopsys-dw-mshc.txt.
>>>>>> If this is the conclusion before, then just ignore this noise.
>>>>> If can be removed the samsung property, then i think property of clock timing can be used into dw-mmc.c
>>>>> But if samsung property is used, well. I think right that it's used into dw_mmc-exynos.c.
>>>>> Dw-mmc.c is general driver..so we don't want to include any SoC specific code.
>>>> Then do you suggest I go forward with an attempt to add a new generic
>>>> "snps,dw-mshc-sdr-timing"
>>>> binding?
>>> Ping Jaehoon?
>>>
>>> Do you think I need to add a generic "snps,dw-mshc-sdr-timing" and
>>> "snps,dw-mshc-ddr-timing" bindings then?
>> Well, i think it's also something wrong. ddr/sdr-timing is exynos specific value, not synopsys value.
>> If synopsys use the sdr/ddr timing, then it's right that "snps, dw-mshc-sdr/ddr-timing" is used.
> I went through the databook and I think that the timing values are
> mentioned throughout the spec. It
> just does not explicitly mentions ddr/sdr timing, but it is mentioned as
> cclk_in_drv(aka drvsel), and
> cclk_in_sample(aka smpsel). Even the macro in dw_mmc-exynos,
> SDMMC_CLKSEL_CCLK_DRIVE() and
> SDMMC_CLKSEL_CCLK_SAMPLE() is stating this.
> 
> So I do not believe that "samsung, dw-mshc-sdr-timing" and
> "samsung,dw-mshc-ddr-timing" are not
> exclusive to only the exynos platform. Just the fact that the SOCFPGA
> platform can re-use the same
> "samsung,dw-mshc-sdr-timing" property proves that this is not just an
> exynos specific value.

i didn't see the register that the sdr/ddr timing is used at Synopsys DoC file.
Even if SOCFPGA platform can reuse the samsung property, it's not general property, isn't?
Under SoC.. cclk_in_drv can be implemented differently.
We have just known that sdr/ddr timing is used at exynos/socfpga board.

I just found clk_drv/smpl_phase_ctrl at UHS register, I think this isn't ddr/sdr timing.

>> But i didn't see sdr/ddr timing in synopsys DoC.
>> I know you want to control the hold-reg bit. 
>> But this approach is not good.
> 
> The spec has a table on when to use the hold-reg bit. The conditions for
> using the hold-reg bit is
> only dependent the hold-reg hw implementation and the value of
> cclk_in_drv. The value of cclk_in_drv
> is specified by the 2nd parameter of "samsung,dw-mshc-sdr-timing".

Right, the spec is mentioned when hold-reg bit could be used.
But that's not that ddr/sdr timing is general value.

Best Regards,
Jaehoon Chung
> 
> So I don't understand why you think this approach is "not good"?
> 
> Thanks,
> Dinh
>> Rather, how about using the callback function for exynos specific value.
>> Then other SoC can also use it.
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>> Dinh
>>>> Dinh
>>>>> If i missed something, then let me know, plz.
>>>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>>> Thanks
>>>>>>
>>>>
>>>
>>>
>>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dinh Nguyen Jan. 9, 2014, 4:15 p.m. UTC | #10

On Thu, 2014-01-09 at 12:41 +0900, Jaehoon Chung wrote:
> Dear, Dinh
> 
> On 01/08/2014 11:12 PM, Dinh Nguyen wrote:
> > 
> > On 1/7/14 6:37 PM, Jaehoon Chung wrote:
> >> Hi, Dinh.
> >>
> >> Sorry for replying too late.
> >>
> >> ..[snip]..
> >>>>>>>>> +    sdr_timing[1] = ddr_timing[1] = 1;
> >>>>>>>>> +    of_property_read_u32_array(np,
> >>>>>>>>> +            "samsung,dw-mshc-sdr-timing", sdr_timing, 2);
> >>>>>>>>> +
> >>>>>>>>> +    of_property_read_u32_array(np,
> >>>>>>>>> +            "samsung,dw-mshc-ddr-timing", ddr_timing, 2);
> >>>>>>>>> +
> >>>>>>>>> +    pdata->cclk_in_drv = 1;
> >>>>>>>>> +    if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
> >>>>>>>>> +        pdata->cclk_in_drv = 0;
> >>>>>>>>> +
> >>>>>>>> Have some concern about whether it is suitable putting "samsung,~"
> >>>>>>>> property in dw_mmc.c, is it supposed to be platform related?
> >>>>>>>> Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
> >>>>>>>> If they are really commonly used, how about change name and define in
> >>>>>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?
> >>>>>>> I had submitted a patch to make this a common binding before:
> >>>>>>>
> >>>>>>> http://www.spinics.net/lists/devicetree/msg00638.html
> >>>>>>>
> >>>>>>> I think the ultimate conclusion to that thread was that its perfectly
> >>>>>>> acceptable to re-use bindings from other
> >>>>>>> platforms.
> >>>>>>>
> >>>>>> Hmm, ususally I may look for the properties of dw_mmc.c in synopsys-dw-mshc.txt.
> >>>>>> If this is the conclusion before, then just ignore this noise.
> >>>>> If can be removed the samsung property, then i think property of clock timing can be used into dw-mmc.c
> >>>>> But if samsung property is used, well. I think right that it's used into dw_mmc-exynos.c.
> >>>>> Dw-mmc.c is general driver..so we don't want to include any SoC specific code.
> >>>> Then do you suggest I go forward with an attempt to add a new generic
> >>>> "snps,dw-mshc-sdr-timing"
> >>>> binding?
> >>> Ping Jaehoon?
> >>>
> >>> Do you think I need to add a generic "snps,dw-mshc-sdr-timing" and
> >>> "snps,dw-mshc-ddr-timing" bindings then?
> >> Well, i think it's also something wrong. ddr/sdr-timing is exynos specific value, not synopsys value.
> >> If synopsys use the sdr/ddr timing, then it's right that "snps, dw-mshc-sdr/ddr-timing" is used.
> > I went through the databook and I think that the timing values are
> > mentioned throughout the spec. It
> > just does not explicitly mentions ddr/sdr timing, but it is mentioned as
> > cclk_in_drv(aka drvsel), and
> > cclk_in_sample(aka smpsel). Even the macro in dw_mmc-exynos,
> > SDMMC_CLKSEL_CCLK_DRIVE() and
> > SDMMC_CLKSEL_CCLK_SAMPLE() is stating this.
> > 
> > So I do not believe that "samsung, dw-mshc-sdr-timing" and
> > "samsung,dw-mshc-ddr-timing" are not
> > exclusive to only the exynos platform. Just the fact that the SOCFPGA
> > platform can re-use the same
> > "samsung,dw-mshc-sdr-timing" property proves that this is not just an
> > exynos specific value.
> 
> i didn't see the register that the sdr/ddr timing is used at Synopsys DoC file.
> Even if SOCFPGA platform can reuse the samsung property, it's not general property, isn't?

I think the Rockchip platform can also use it, but it hasn't been
clearly documented yet.

> Under SoC.. cclk_in_drv can be implemented differently.

Yes, agreed. Clearly the cclk_in_drv and cclk_in_sample methods to shift
the phase of the CIU clock is implemented differently. The exynos'
implementation is not getting touched at all.

> We have just known that sdr/ddr timing is used at exynos/socfpga board.
> 
> I just found clk_drv/smpl_phase_ctrl at UHS register, I think this isn't ddr/sdr timing.


But let's take a look at what are the possible values ddr/sdr timings
can take? From Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt:

"Valid values for SDR and DDR CIU clock timing for Exynos5250:
      - valid value for tx phase shift and rx phase shift is 0 to 7."

For socfpga, 0 = 45 degrees shift, 1 = 90 degrees shift,...[n++] and 7 =
315 degrees shift. If my guess is correct, it should also be the same
for exynos.


> 
> >> But i didn't see sdr/ddr timing in synopsys DoC.
> >> I know you want to control the hold-reg bit. 
> >> But this approach is not good.
> > 
> > The spec has a table on when to use the hold-reg bit. The conditions for
> > using the hold-reg bit is
> > only dependent the hold-reg hw implementation and the value of
> > cclk_in_drv. The value of cclk_in_drv
> > is specified by the 2nd parameter of "samsung,dw-mshc-sdr-timing".
> 
> Right, the spec is mentioned when hold-reg bit could be used.
> But that's not that ddr/sdr timing is general value.

Do you agree that the 2nd value of sdr/ddr timing is representing the
cclk_in_drv? Looking at the code dw_mmc-exynos.c, this is indeed what
the 2nd value in sdr/ddr timing is representing.


So we can come up with a more generic DTS binding that the generic
dw_mmc driver can use to set the hold-reg, but that binding still needs
to pass in the cclk_in_drv somehow, as cclk_in_drv is the only external
variable that is needed to determine when to set hold-reg.

And if sdr/ddr timing is already representing cclk_in_drv, doesn't it
make sense to re-use that binding?

Thanks,
Dinh
> 
> Best Regards,
> Jaehoon Chung
> > 
> > So I don't understand why you think this approach is "not good"?
> > 
> > Thanks,
> > Dinh
> >> Rather, how about using the callback function for exynos specific value.
> >> Then other SoC can also use it.
> >>
> >> Best Regards,
> >> Jaehoon Chung
> >>
> >>> Dinh
> >>>> Dinh
> >>>>> If i missed something, then let me know, plz.
> >>>>>
> >>>>> Best Regards,
> >>>>> Jaehoon Chung
> >>>>>> Thanks
> >>>>>>
> >>>>
> >>>
> >>>
> >>>
> > 
> > 
> 
> 



--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jaehoon Chung Jan. 13, 2014, 6:27 a.m. UTC | #11

Hi Dinh,

On 01/10/2014 01:15 AM, Dinh Nguyen wrote:
> On Thu, 2014-01-09 at 12:41 +0900, Jaehoon Chung wrote:
>> Dear, Dinh
>>
>> On 01/08/2014 11:12 PM, Dinh Nguyen wrote:
>>>
>>> On 1/7/14 6:37 PM, Jaehoon Chung wrote:
>>>> Hi, Dinh.
>>>>
>>>> Sorry for replying too late.
>>>>
>>>> ..[snip]..
>>>>>>>>>>> +    sdr_timing[1] = ddr_timing[1] = 1;
>>>>>>>>>>> +    of_property_read_u32_array(np,
>>>>>>>>>>> +            "samsung,dw-mshc-sdr-timing", sdr_timing, 2);
>>>>>>>>>>> +
>>>>>>>>>>> +    of_property_read_u32_array(np,
>>>>>>>>>>> +            "samsung,dw-mshc-ddr-timing", ddr_timing, 2);
>>>>>>>>>>> +
>>>>>>>>>>> +    pdata->cclk_in_drv = 1;
>>>>>>>>>>> +    if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
>>>>>>>>>>> +        pdata->cclk_in_drv = 0;
>>>>>>>>>>> +
>>>>>>>>>> Have some concern about whether it is suitable putting "samsung,~"
>>>>>>>>>> property in dw_mmc.c, is it supposed to be platform related?
>>>>>>>>>> Any conflict with drivers/mmc/host/dw_mmc-exynos.c?
>>>>>>>>>> If they are really commonly used, how about change name and define in
>>>>>>>>>> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt?
>>>>>>>>> I had submitted a patch to make this a common binding before:
>>>>>>>>>
>>>>>>>>> http://www.spinics.net/lists/devicetree/msg00638.html
>>>>>>>>>
>>>>>>>>> I think the ultimate conclusion to that thread was that its perfectly
>>>>>>>>> acceptable to re-use bindings from other
>>>>>>>>> platforms.
>>>>>>>>>
>>>>>>>> Hmm, ususally I may look for the properties of dw_mmc.c in synopsys-dw-mshc.txt.
>>>>>>>> If this is the conclusion before, then just ignore this noise.
>>>>>>> If can be removed the samsung property, then i think property of clock timing can be used into dw-mmc.c
>>>>>>> But if samsung property is used, well. I think right that it's used into dw_mmc-exynos.c.
>>>>>>> Dw-mmc.c is general driver..so we don't want to include any SoC specific code.
>>>>>> Then do you suggest I go forward with an attempt to add a new generic
>>>>>> "snps,dw-mshc-sdr-timing"
>>>>>> binding?
>>>>> Ping Jaehoon?
>>>>>
>>>>> Do you think I need to add a generic "snps,dw-mshc-sdr-timing" and
>>>>> "snps,dw-mshc-ddr-timing" bindings then?
>>>> Well, i think it's also something wrong. ddr/sdr-timing is exynos specific value, not synopsys value.
>>>> If synopsys use the sdr/ddr timing, then it's right that "snps, dw-mshc-sdr/ddr-timing" is used.
>>> I went through the databook and I think that the timing values are
>>> mentioned throughout the spec. It
>>> just does not explicitly mentions ddr/sdr timing, but it is mentioned as
>>> cclk_in_drv(aka drvsel), and
>>> cclk_in_sample(aka smpsel). Even the macro in dw_mmc-exynos,
>>> SDMMC_CLKSEL_CCLK_DRIVE() and
>>> SDMMC_CLKSEL_CCLK_SAMPLE() is stating this.
>>>
>>> So I do not believe that "samsung, dw-mshc-sdr-timing" and
>>> "samsung,dw-mshc-ddr-timing" are not
>>> exclusive to only the exynos platform. Just the fact that the SOCFPGA
>>> platform can re-use the same
>>> "samsung,dw-mshc-sdr-timing" property proves that this is not just an
>>> exynos specific value.
>>
>> i didn't see the register that the sdr/ddr timing is used at Synopsys DoC file.
>> Even if SOCFPGA platform can reuse the samsung property, it's not general property, isn't?
> 
> I think the Rockchip platform can also use it, but it hasn't been
> clearly documented yet.
> 
>> Under SoC.. cclk_in_drv can be implemented differently.
> 
> Yes, agreed. Clearly the cclk_in_drv and cclk_in_sample methods to shift
> the phase of the CIU clock is implemented differently. The exynos'
> implementation is not getting touched at all.
> 
>> We have just known that sdr/ddr timing is used at exynos/socfpga board.
>>
>> I just found clk_drv/smpl_phase_ctrl at UHS register, I think this isn't ddr/sdr timing.
> 
> 
> But let's take a look at what are the possible values ddr/sdr timings
> can take? From Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt:
> 
> "Valid values for SDR and DDR CIU clock timing for Exynos5250:
>       - valid value for tx phase shift and rx phase shift is 0 to 7."
> 
> For socfpga, 0 = 45 degrees shift, 1 = 90 degrees shift,...[n++] and 7 =
> 315 degrees shift. If my guess is correct, it should also be the same
> for exynos.

Right, socfpga is used with similar timing value.
But All SoC should not be same with exynos.

> 
> 
>>
>>>> But i didn't see sdr/ddr timing in synopsys DoC.
>>>> I know you want to control the hold-reg bit. 
>>>> But this approach is not good.
>>>
>>> The spec has a table on when to use the hold-reg bit. The conditions for
>>> using the hold-reg bit is
>>> only dependent the hold-reg hw implementation and the value of
>>> cclk_in_drv. The value of cclk_in_drv
>>> is specified by the 2nd parameter of "samsung,dw-mshc-sdr-timing".
>>
>> Right, the spec is mentioned when hold-reg bit could be used.
>> But that's not that ddr/sdr timing is general value.
> 
> Do you agree that the 2nd value of sdr/ddr timing is representing the
> cclk_in_drv? Looking at the code dw_mmc-exynos.c, this is indeed what
> the 2nd value in sdr/ddr timing is representing.

Agreed it. did you think that cclk_in_drv and DDR/SDR timing value is same?
Right..it's same meaning. Using Hold_REG is affected with presence of sdr/ddr timing.

> 
> 
> So we can come up with a more generic DTS binding that the generic
> dw_mmc driver can use to set the hold-reg, but that binding still needs
> to pass in the cclk_in_drv somehow, as cclk_in_drv is the only external
> variable that is needed to determine when to set hold-reg.
> 
> And if sdr/ddr timing is already representing cclk_in_drv, doesn't it
> make sense to re-use that binding?

I didn't know exactly which soc is used with sdr/ddr timing.
As socfpga and exynos is used, it's not become the general property.

Well, I understood your opinion. So i will consider more about this.

Best Regards,
Jaehoon Chung

> 
> Thanks,
> Dinh
>>
>> Best Regards,
>> Jaehoon Chung
>>>
>>> So I don't understand why you think this approach is "not good"?
>>>
>>> Thanks,
>>> Dinh
>>>> Rather, how about using the callback function for exynos specific value.
>>>> Then other SoC can also use it.
>>>>
>>>> Best Regards,
>>>> Jaehoon Chung
>>>>
>>>>> Dinh
>>>>>> Dinh
>>>>>>> If i missed something, then let me know, plz.
>>>>>>>
>>>>>>> Best Regards,
>>>>>>> Jaehoon Chung
>>>>>>>> Thanks
>>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>
>>
> 
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 4bce0de..080ebb6 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -279,6 +279,9 @@  static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command *cmd)
 			cmdr |= SDMMC_CMD_DAT_WR;
 	}
 
+	if (slot->host->use_hold_reg)
+		cmdr |= SDMMC_CMD_USE_HOLD_REG;
+
 	if (drv_data && drv_data->prepare_command)
 		drv_data->prepare_command(slot->host, &cmdr);
 
@@ -969,6 +972,25 @@  static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	mci_writel(slot->host, UHS_REG, regs);
 	slot->host->timing = ios->timing;
 
+	/* Per Synopsys spec, use_hold_reg should be set for all modes except for
+	 * high-speed SDR50, DDR50, SDR104, and MMC_HS200. However, use_hold_reg
+	 * should be cleared if the cclk_in_drv is 0. use_hold_reg should be set
+	 * for all modes if cclk_in_drv is set.
+	 */
+	slot->host->use_hold_reg = 1;
+	switch (slot->host->timing) {
+	case MMC_TIMING_UHS_SDR50:
+	case MMC_TIMING_UHS_SDR104:
+	case MMC_TIMING_UHS_DDR50:
+	case MMC_TIMING_MMC_HS200:
+		if (slot->host->pdata->cclk_in_drv == 0)
+			slot->host->use_hold_reg = 0;
+		break;
+	}
+
+	if (slot->host->can_use_hold_reg == 0)
+		slot->host->use_hold_reg = 0;
+
 	/*
 	 * Use mirror of ios->clock to prevent race with mmc
 	 * core ios update when finding the minimum.
@@ -2339,6 +2361,8 @@  static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 	const struct dw_mci_drv_data *drv_data = host->drv_data;
 	int idx, ret;
 	u32 clock_frequency;
+	int sdr_timing[2];
+	int ddr_timing[2];
 
 	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata) {
@@ -2389,6 +2413,25 @@  static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 	if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL))
 		pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
 
+	/* Check for the "samsung,dw-mshc-sdr-timing" and the
+	 * "samsung,dw-mshc-ddr-timing" bindings as this will tell us if we
+	 * can safely set the SDMMC_CMD_USE_HOLD_REG bit. The second paramater
+	 * in these 2 bindings is the value of the cclk_in_drv. If cclk_in_drv
+	 * is 0, we cannot set the SDMMC_CMD_USE_HOLD_REG bit. The default
+	 * behavior will be to set cclk_in_drv, as some platforms do not have
+	 * to set the sdr or ddr timing parameters.
+	 */
+	sdr_timing[1] = ddr_timing[1] = 1;
+	of_property_read_u32_array(np,
+			"samsung,dw-mshc-sdr-timing", sdr_timing, 2);
+
+	of_property_read_u32_array(np,
+			"samsung,dw-mshc-ddr-timing", ddr_timing, 2);
+
+	pdata->cclk_in_drv = 1;
+	if ((sdr_timing[1] == 0) || (ddr_timing[1] == 0))
+		pdata->cclk_in_drv = 0;
+
 	return pdata;
 }
 
@@ -2495,6 +2538,15 @@  int dw_mci_probe(struct dw_mci *host)
 		goto err_regulator;
 	}
 
+	/* Check to see if the HOLD REG is implemented. */
+	host->can_use_hold_reg = (mci_readl(host, HCON) & SDMMC_HCON_IHR) >> 22;
+
+	/* Can only use the HOLD REG is both conditions are true:
+	 * Hardware has implemented HOLD_REG and
+	 * cclk_in_drv is non-zero.
+	 */
+	host->can_use_hold_reg &= host->pdata->cclk_in_drv;
+
 	host->quirks = host->pdata->quirks;
 
 	spin_lock_init(&host->lock);
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 6bf24ab..dfd05c9 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -145,6 +145,10 @@ 
 #define SDMMC_IDMAC_ENABLE		BIT(7)
 #define SDMMC_IDMAC_FB			BIT(1)
 #define SDMMC_IDMAC_SWRESET		BIT(0)
+
+/* Hardware Configuration(HCON) register */
+#define SDMMC_HCON_IHR			BIT(22)
+
 /* Version ID register define */
 #define SDMMC_GET_VERID(x)		((x) & 0xFFFF)
 /* Card read threshold */
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index 6ce7d2c..2b5b8bf 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -191,6 +191,8 @@  struct dw_mci {
 	struct regulator	*vmmc;	/* Power regulator */
 	unsigned long		irq_flags; /* IRQ flags */
 	int			irq;
+	u32			can_use_hold_reg;
+	bool			use_hold_reg;
 };
 
 /* DMA ops for Internal/External DMAC interface */
@@ -238,6 +240,7 @@  struct dw_mci_board {
 	u32 caps;	/* Capabilities */
 	u32 caps2;	/* More capabilities */
 	u32 pm_caps;	/* PM capabilities */
+	u32 cclk_in_drv;	/*cclk_in_drv phase shift */
 	/*
 	 * Override fifo depth. If 0, autodetect it from the FIFOTH register,
 	 * but note that this may not be reliable after a bootloader has used