Re: [PATCH v3 6/6] mmc: dw_mmc: add samsung exynos5250 specific extentions

From: Thomas Abraham
Date: Thu Jul 19 2012 - 14:48:44 EST

On 19 July 2012 09:21, Seungwon Jeon <tgih.jun@xxxxxxxxxxx> wrote:
> Hi,
>
> This version does not seems to consider previous reviews fully.
> Could you check the comments below?

I did try to address all the comments. I will check again and resubmit
if I have missed anything.

>
> July 12, 2012, Thomas Abraham <thomas.abraham@xxxxxxxxxx> wrote:
>> The instantiation of the Synopsis Designware controller on Exynos5250
>> include extension for SDR and DDR specific tx/rx phase shift timing
>> and CIU internal divider. In addition to that, the option to skip the
>> command hold stage is also introduced. Add support for these Exynos5250
>> specfic extenstions.
>>
>> Signed-off-by: Abhilash Kesavan <a.kesavan@xxxxxxxxxxx>
>> Signed-off-by: Thomas Abraham <thomas.abraham@xxxxxxxxxx>
>> ---
>> .../devicetree/bindings/mmc/synposis-dw-mshc.txt | 38 ++++++++++++++++++-
>> drivers/mmc/host/dw_mmc-pltfm.c | 15 +++++++
>> drivers/mmc/host/dw_mmc.c | 40 +++++++++++++++++++-
>> drivers/mmc/host/dw_mmc.h | 14 +++++++
>> include/linux/mmc/dw_mmc.h | 6 +++
>> 5 files changed, 110 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> index 3acd6c9..69d78c1 100644
>> --- a/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/synposis-dw-mshc.txt
>> @@ -7,6 +7,8 @@ Required Properties:
>>
>> * compatible: should be one of the following
>> - snps,dw-mshc: for controllers compliant with synopsis dw-mshc.
>> + - samsung,exynos5250-dw-mshc: for controllers with Samsung
>> + Exynos5250 specific extentions.
>>
>> * reg: physical base address of the dw-mshc controller and size of its memory
>> region.
>> @@ -74,13 +76,45 @@ Aliases:
>> the following format 'mshc{n}' where n is a unique number for the alias.
>>
>>
>> +Samsung Exynos4/5 specific properties:
>> +
>> +Some of the variants of Exynos4 (such as Exynos4412) and Exynos5 SoC's
>> +includes few extensions to the Synopsis Designware Mobile Storage Host
>> +Controller. The following properties are used to describe those extensions.
>> +
>> +* samsung,dw-mshc-sdr-timing: Specifies the value of CUI clock divider, CIU
>> + clock phase shift value in transmit mode and CIU clock phase shift value in
>> + receive mode for single data rate mode operation. Refer notes of the valid
>> + values below.
>> +
>> +* samsung,dw-mshc-ddr-timing: Specifies the value of CUI clock divider, CIU
>> + clock phase shift value in transmit mode and CIU clock phase shift value in
>> + receive mode for double data rate mode operation. Refer notes of the valid
>> + values below. The order of the cells should be
>> +
>> + - First Cell: CIU clock divider value (applicable only for Exynos5
>> + SoC's, should be zero for Exynos4 SoC's)
>> + - Second Cell: CIU clock phase shift value for tx mode.
>> + - Third Cell: CIU clock phase shift value for rx mode.
>> +
>> + Valid values for SDR and DDR CIU clock timing for Exynos5250:
>> +
>> + - valid values for CIU clock divider, tx phase shift and rx phase shift
>> + is 0 to 7.
>> +
>> + - When CIU clock divider value is set to 3, all possible 8 phase shift
>> + values can be used.
>> +
>> + - If CIU clock divider value is 0 (that is divide by 1), both tx and rx
>> + phase shift clocks should be 0.
>> +
>> Example:
>>
>> The MSHC controller node can be split into two portions, SoC specific and
>> board specific portions as listed below.
>>
>> dwmmc0@12200000 {
>> - compatible = "snps,dw-mshc";
>> + compatible = "samsung,exynos5250-dw-mshc";
>> reg = <0x12200000 0x1000>;
>> interrupts = <0 75 0>;
>> #address-cells = <1>;
>> @@ -94,6 +128,8 @@ Example:
>> no-write-protect;
>> fifo-depth = <0x80>;
>> card-detect-delay = <200>;
>> + samsung,dw-mshc-sdr-timing = <2 3 3>;
>> + samsung,dw-mshc-ddr-timing = <1 2 3>;
>>
>> slot@0 {
>> reg = <0>;
>> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
>> index 8d24f6d..900f412 100644
>> --- a/drivers/mmc/host/dw_mmc-pltfm.c
>> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
>> @@ -27,9 +27,24 @@ static struct dw_mci_drv_data synopsis_drv_data = {
>> .ctrl_type = DW_MCI_TYPE_SYNOPSIS,
>> };
>>
>> +static unsigned long exynos5250_dwmmc_caps[4] = {
>> + MMC_CAP_UHS_DDR50 | MMC_CAP_1_8V_DDR |
>> + MMC_CAP_8_BIT_DATA | MMC_CAP_CMD23,
>> + MMC_CAP_CMD23,
>> + MMC_CAP_CMD23,
>> + MMC_CAP_CMD23,
>> +};
>> +
> Kyungmin Park has already pointed .
> It's not still proper place for board specific caps.
> If I'm incorrect, please let me know.
> And why MMC_CAP_CMD23 is default caps for all channel of hosts?

The cap listed above are specifying controller capabilities for dw-mmc
controllers on Exynos5 SoC. They are not board specific caps. All the
Exynos5 dw-mmc controllers can support MMC_CAP_CMD23 cap and hence, it
has been listed for all the controllers. Please let me know if you
feel there is any change required here.

>
>> +static struct dw_mci_drv_data exynos5250_drv_data = {
>> + .ctrl_type = DW_MCI_TYPE_EXYNOS5250,
>> + .caps = exynos5250_dwmmc_caps,
>> +};
>> +
>> static const struct of_device_id dw_mci_pltfm_match[] = {
>> { .compatible = "snps,dw-mshc",
>> .data = (void *)&synopsis_drv_data, },
>> + { .compatible = "samsung,exynos5250-dw-mshc",
>> + .data = (void *)&exynos5250_drv_data, },
>> {},
>> };
>> MODULE_DEVICE_TABLE(of, dw_mci_pltfm_match);
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 3bc276d..bbf1209 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -236,6 +236,7 @@ static void dw_mci_set_timeout(struct dw_mci *host)
>> static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command *cmd)
>> {
>> struct mmc_data *data;
>> + struct dw_mci_slot *slot = mmc_priv(mmc);
>> u32 cmdr;
>> cmd->error = -EINPROGRESS;
>>
>> @@ -265,6 +266,17 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command *cmd)
>> cmdr |= SDMMC_CMD_DAT_WR;
>> }
>>
>> + /*
>> + * Samsung Exynos5250 extends the use of CMD register with the use of
>> + * bit 29 (which is reserved on standard MSHC controllers) for
>> + * optionally bypassing the HOLD register for command and data. The
>> + * HOLD register should be bypassed in case there is no phase shift
>> + * applied on CMD/DATA that is sent to the card.
>> + */
>> + if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250)
>> + if (SDMMC_CLKSEL_GET_SELCLK_DRV(mci_readl(slot->host, CLKSEL)))
>> + cmdr |= SDMMC_CMD_USE_HOLD_REG;
>> +
>> return cmdr;
>> }
>>
>> @@ -802,10 +814,19 @@ static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>> regs = mci_readl(slot->host, UHS_REG);
>>
>> /* DDR mode set */
>> - if (ios->timing == MMC_TIMING_UHS_DDR50)
>> + if (ios->timing == MMC_TIMING_UHS_DDR50) {
>> regs |= (0x1 << slot->id) << 16;
>> - else
>> + mci_writel(slot->host, CLKSEL, slot->host->ddr_timing);
>> + } else {
>> regs &= ~(0x1 << slot->id) << 16;
>> + mci_writel(slot->host, CLKSEL, slot->host->sdr_timing);
>> + }
>> +
>> + if (slot->host->drv_data->ctrl_type == DW_MCI_TYPE_EXYNOS5250) {
>> + slot->host->bus_hz = clk_get_rate(slot->host->ciu_clk);
>> + slot->host->bus_hz /= SDMMC_CLKSEL_GET_DIVRATIO(
>> + mci_readl(slot->host, CLKSEL));
>> + }
> As you know, CLKSEL is specific for Samsung soc.
> 0x09C(CLKSEL) is reserved area in Synopsys memory map.
> In case of non-samsung-soc, we cannot ensure this usage.
> In previous version, I have suggested separating the variant into another file.

There is a check for type of SoC before using 0x9C as CLKSEL register.
Other implementations of dw-mmc might define custom register at 0x9C
but this will code will not execute on other SoC's and will not break
anything on other implementations. Regarding spliting this Exynos
specific code into another file, I prefer not to do it for now.
Spliting the code means adding new definitions of callback functions
which I am not sure is really required. The present code is fairly
simple one.

>
>>
>> mci_writel(slot->host, UHS_REG, regs);
>>
>> @@ -2086,6 +2107,7 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>> struct dw_mci_board *pdata;
>> struct device *dev = host->dev;
>> struct device_node *np = dev->of_node;
>> + u32 timing[3];
>> int idx, cnt;
>>
>> pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> @@ -2108,6 +2130,20 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>> if (of_get_property(np, of_quriks[idx].quirk, NULL))
>> pdata->quirks |= of_quriks[idx].id;
>>
>> + if (of_property_read_u32_array(dev->of_node,
>> + "samsung,dw-mshc-sdr-timing", timing, 3))
>> + host->sdr_timing = DW_MCI_DEF_SDR_TIMING;
> Host of non-samsung will reach here.
> host->sdr_timing is needed for this host? host->ddr_timing is the same.

Yes, but non-samsung hosts will not have have this property into their
dts file. So the code within the condition will not execute on
non-samsung hosts. SDR and DDR timing are required for Exynos5 SoC.

>
>> + else
>> + host->sdr_timing = SDMMC_CLKSEL_TIMING(timing[0],
>> + timing[1], timing[2]);
>> +
>> + if (of_property_read_u32_array(dev->of_node,
>> + "samsung,dw-mshc-ddr-timing", timing, 3))
>> + host->ddr_timing = DW_MCI_DEF_DDR_TIMING;
>> + else
>> + host->ddr_timing = SDMMC_CLKSEL_TIMING(timing[0],
>> + timing[1], timing[2]);
>> +
>> if (of_property_read_u32(np, "fifo-depth", &pdata->fifo_depth))
>> dev_info(dev, "fifo-depth property not found, using "
>> "value of FIFOTH register as default\n");
>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> index 1ecaa02..6c17282 100644
>> --- a/drivers/mmc/host/dw_mmc.h
>> +++ b/drivers/mmc/host/dw_mmc.h
>> @@ -53,6 +53,7 @@
>> #define SDMMC_IDINTEN 0x090
>> #define SDMMC_DSCADDR 0x094
>> #define SDMMC_BUFADDR 0x098
>> +#define SDMMC_CLKSEL 0x09C /* specific to Samsung Exynos5250 */
>> #define SDMMC_DATA(x) (x)
>>
>> /*
>> @@ -111,6 +112,7 @@
>> #define SDMMC_INT_ERROR 0xbfc2
>> /* Command register defines */
>> #define SDMMC_CMD_START BIT(31)
>> +#define SDMMC_CMD_USE_HOLD_REG BIT(29)
>> #define SDMMC_CMD_CCS_EXP BIT(23)
>> #define SDMMC_CMD_CEATA_RD BIT(22)
>> #define SDMMC_CMD_UPD_CLK BIT(21)
>> @@ -142,6 +144,17 @@
>> /* Version ID register define */
>> #define SDMMC_GET_VERID(x) ((x) & 0xFFFF)
>>
>> +#define DW_MCI_DEF_SDR_TIMING 0x03030002
>> +#define DW_MCI_DEF_DDR_TIMING 0x03020001
> What is the basis for these timing?
> These values is board-specific.
>
>> +#define SDMMC_CLKSEL_CCLK_SAMPLE(x) (((x) & 3) << 0)
>> +#define SDMMC_CLKSEL_CCLK_DRIVE(x) (((x) & 3) << 16)
>> +#define SDMMC_CLKSEL_CCLK_DIVIDER(x) (((x) & 3) << 24)
> If it's for exynos5, it will be 7 not 3.

Yes, I missed that. Thanks. I will fix this.

>
>> +#define SDMMC_CLKSEL_TIMING(x, y, z) (SDMMC_CLKSEL_CCLK_SAMPLE(x) | \
>> + SDMMC_CLKSEL_CCLK_DRIVE(y) | \
>> + SDMMC_CLKSEL_CCLK_DIVIDER(z))
>> +#define SDMMC_CLKSEL_GET_DIVRATIO(x) ((((x) >> 24) & 0x7) + 1)
>> +#define SDMMC_CLKSEL_GET_SELCLK_DRV(x) (((x) >> 16) & 0x7)
>> +
> Is this patch considered only for exynos5250?
> In case of exynos4210, the number of bits is different.
> If upper macros is backward-compatible, it would be better.

These consider the Exynos4210 and Exynos4412 implementations as well.
The device tree documentation clearly states that the possible values
for each of the dividers. For Exynos4 SoC's, the divider value is
between 1 to 4 (or 0 to 3). So a bit mask of 7 is backward compatilble
for Exynos4.

Thanks for your review and comments on this patch.

Regards,
Thomas.

>
> Best regards,
> Seungwon Jeon
>
>> /* Register access macros */
>> #define mci_readl(dev, reg) \
>> __raw_readl((dev)->regs + SDMMC_##reg)
>> @@ -184,6 +197,7 @@ extern int dw_mci_resume(struct dw_mci *host);
>>
>> /* Variations in the dw_mci controller */
>> #define DW_MCI_TYPE_SYNOPSIS 0
>> +#define DW_MCI_TYPE_EXYNOS5250 1 /* Samsung Exynos5250 Extensions */
>>
>> /* dw_mci platform driver data */
>> struct dw_mci_drv_data {
>> diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
>> index ae45e4f..32c778f 100644
>> --- a/include/linux/mmc/dw_mmc.h
>> +++ b/include/linux/mmc/dw_mmc.h
>> @@ -82,6 +82,8 @@ struct mmc_data;
>> * @biu_clk: Pointer to bus interface unit clock instance.
>> * @ciu_clk: Pointer to card interface unit clock instance.
>> * @slot: Slots sharing this MMC controller.
>> + * @sdr_timing: Clock phase shifting for driving and sampling in sdr mode
>> + * @ddr_timing: Clock phase shifting for driving and sampling in ddr mode
>> * @fifo_depth: depth of FIFO.
>> * @data_shift: log2 of FIFO item size.
>> * @part_buf_start: Start index in part_buf.
>> @@ -166,6 +168,10 @@ struct dw_mci {
>> struct clk *ciu_clk;
>> struct dw_mci_slot *slot[MAX_MCI_SLOTS];
>>
>> + /* Phase Shift Value (for exynos5250 variant) */
>> + u32 sdr_timing;
>> + u32 ddr_timing;
>> +
>> /* FIFO push and pull */
>> int fifo_depth;
>> int data_shift;
>> --
>> 1.6.6.rc2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/