Re: [PATCH v3 6/6] mmc: dw_mmc: add samsung exynos5250 specific extentions
On 20 July 2012 16:08, Seungwon Jeon tgih@samsung.com wrote: July 20, 2012, Thomas Abraham thomas.abra...@linaro.org wrote: On 19 July 2012 09:21, Seungwon Jeon tgih@samsung.com wrote: [...] +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. 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. [...] + 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. 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. 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. [...] + 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. + 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
RE: [PATCH v3 6/6] mmc: dw_mmc: add samsung exynos5250 specific extentions
July 23, 2012, Thomas Abraham thomas.abra...@linaro.org wrote: On 20 July 2012 16:08, Seungwon Jeon tgih@samsung.com wrote: July 20, 2012, Thomas Abraham thomas.abra...@linaro.org wrote: On 19 July 2012 09:21, Seungwon Jeon tgih@samsung.com wrote: [...] +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. 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. [...] + 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. + } + 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. 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. [...] + 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
Re: [PATCH v3 6/6] mmc: dw_mmc: add samsung exynos5250 specific extentions
On 19 July 2012 09:21, Seungwon Jeon tgih@samsung.com 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.abra...@linaro.org 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.kesa...@samsung.com Signed-off-by: Thomas Abraham thomas.abra...@linaro.org --- .../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@1220 { - compatible = snps,dw-mshc; + compatible = samsung,exynos5250-dw-mshc; reg = 0x1220 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
RE: [PATCH v3 6/6] mmc: dw_mmc: add samsung exynos5250 specific extentions
Hi, This version does not seems to consider previous reviews fully. Could you check the comments below? July 12, 2012, Thomas Abraham thomas.abra...@linaro.org 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.kesa...@samsung.com Signed-off-by: Thomas Abraham thomas.abra...@linaro.org --- .../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@1220 { - compatible = snps,dw-mshc; + compatible = samsung,exynos5250-dw-mshc; reg = 0x1220 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? +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, }, + {
[PATCH v3 6/6] mmc: dw_mmc: add samsung exynos5250 specific extentions
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.kesa...@samsung.com Signed-off-by: Thomas Abraham thomas.abra...@linaro.org --- .../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@1220 { - compatible = snps,dw-mshc; + compatible = samsung,exynos5250-dw-mshc; reg = 0x1220 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, +}; + +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)