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

July 23, 2012, Thomas Abraham [off-list ref] wrote:

On 20 July 2012 16:08, Seungwon Jeon [off-list ref] wrote:

quoted

July 20, 2012, Thomas Abraham [off-list ref] wrote:

quoted

On 19 July 2012 09:21, Seungwon Jeon [off-list ref] wrote:

[...]

quoted

quoted

quoted

quoted

+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.

MMC_CAP_8_BIT_DATA could be dependent on board.

A controller can have the MMC_CAP_8_BIT_DATA capability but the board
will decide the bus-width. The bus-width is specified in the dts files
of each board (or platform data). The bus-width for data transfer is
then decided by the MMC core code based on the caps and the bus-width
information. So MMC_CAP_8_BIT_DATA can be specified irrespective   of
whether the board supports 8-bit or not.

quoted

I agree about MMC_CAP_CMD23.
Additionally, MMC_CAP_CMD23 is applied for dw-mmc host driver without regard to Exynos5.

The caps listed in exynos5250_dwmmc_caps is applicable only for
Exynos5 SoC's. Could you please let me know if there is anything
incorrect here.

I mean that MMC_CAP_CMD23 is a capability which is implemented in driver without dependency of SOC.
So, other soc also includes MMC_CAP_CMD23.
It'd rather make a default caps than list in specific soc, considering the other soc.

[...]

quoted

quoted

quoted

quoted

+     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.

Do you mean checking DW_MCI_TYPE_EXYNOS5250?
But Above two case(ddr_timing/sdr_timing), CLKSEL can be accessed on other soc's.

The tests have only been completed on Exynos5250. I do not have boards
for other Samsung SoC's which have a dw_mmc port connected and used on
the board. When we have other platforms tested with this patchset, we
can extend the 'if' check in the above code for other SoC's.

My meaning  seem to be passed incorrectly.

+       if (ios->timing == MMC_TIMING_UHS_DDR50) {
                regs |= (0x1 << slot->id) << 16;
-       else
+               mci_writel(slot->host, CLKSEL, slot->host->ddr_timing);
What is the execution for non-samsung soc?
CLKSEL register is valid  only for Exynos.
dw_mci_set_ios shoud be aware of this.

+       } else {
                regs &= ~(0x1 << slot->id) << 16;
+               mci_writel(slot->host, CLKSEL, slot->host->sdr_timing);
This line is same.
+       }
+

quoted

quoted

Other implementations of dw-mmc might define custom register at 0x9C

Even so, register field can be different with Samsung soc.

Yes, with the correct checks for the type of SoC, differences in the
usage of 0x9C register can be handled.

quoted

quoted

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.

Yes, callback functions might be needed to accommodate various implementation
of host controller. It would be better to prepare this for other variant next.

Ok. I will relook at these patches and check if we really need split
these changes into a separate exynos specific file. If I again feel
that such a split is not required, I will reply back to you with
justification.

[...]

quoted

quoted

quoted

quoted

+     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.

Yes, these are required only for Exynos Soc.
Non-samsung host will have default value here, but it seems to be meaningless.

Non-Samsung platforms will not have this property in their dts files.
This property is required on only those platforms that want to define
sdr and ddr timing values. It is not required on platforms that do no
use it.

quoted

quoted

quoted

quoted

+     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.

One missed comment?

Yes, sorry, missed this last time. These are default values for SDR
and DDR timing in case it is not specified in dts file. It may be
better to make it mandatory to specify the SDR and DDR property in dts
files (for Exynos5250) and remove this default values. Non-samsung
platforms or platforms not using this property are unaffected by this.

These value(0x03030002, 0x03020001) cannot be acceptable in specific board type of Exynos5250.
Assuming that timing values are not specified in dts file and board is different,
default values may be not proper. As you mentioned, other approach will be needed.

Thanks,
Seungwon Jeon

quoted

quoted

quoted

quoted

+#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.

quoted

quoted

+#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.

Bit width is 2 for selclk_drv in exynos4210.
So bit mask of 3 is proper.

Let me clear it about divider value.
In case of Exynos4 SoC's, divider value(DIVRATIO) is reserved and host doesn't modify.
But value is fixed internally like following.
Exynos4210 : 2
Exynos4412 : 4

Ok. I will relook into this. Thanks.

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