From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753067Ab2GWGRX (ORCPT ); Mon, 23 Jul 2012 02:17:23 -0400 Received: from mail-lb0-f174.google.com ([209.85.217.174]:52532 "EHLO mail-lb0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752899Ab2GWGRV (ORCPT ); Mon, 23 Jul 2012 02:17:21 -0400 MIME-Version: 1.0 In-Reply-To: <001101cd6663$deb769d0$9c263d70$%jun@samsung.com> References: <1342097668-25243-1-git-send-email-thomas.abraham@linaro.org> <1342097668-25243-7-git-send-email-thomas.abraham@linaro.org> <002901cd6561$bb6dc9e0$32495da0$%jun@samsung.com> <001101cd6663$deb769d0$9c263d70$%jun@samsung.com> Date: Mon, 23 Jul 2012 11:47:19 +0530 Message-ID: Subject: Re: [PATCH v3 6/6] mmc: dw_mmc: add samsung exynos5250 specific extentions From: Thomas Abraham To: Seungwon Jeon Cc: linux-mmc@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, cjb@laptop.org, grant.likely@secretlab.ca, rob.herring@calxeda.com, linux-samsung-soc@vger.kernel.org, kgene.kim@samsung.com, patches@linaro.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 20 July 2012 16:08, Seungwon Jeon wrote: > July 20, 2012, Thomas Abraham wrote: >> On 19 July 2012 09:21, Seungwon Jeon 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 (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. > >> > >> >> +#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. > 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: thomas.abraham@linaro.org (Thomas Abraham) Date: Mon, 23 Jul 2012 11:47:19 +0530 Subject: [PATCH v3 6/6] mmc: dw_mmc: add samsung exynos5250 specific extentions In-Reply-To: <001101cd6663$deb769d0$9c263d70$%jun@samsung.com> References: <1342097668-25243-1-git-send-email-thomas.abraham@linaro.org> <1342097668-25243-7-git-send-email-thomas.abraham@linaro.org> <002901cd6561$bb6dc9e0$32495da0$%jun@samsung.com> <001101cd6663$deb769d0$9c263d70$%jun@samsung.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 20 July 2012 16:08, Seungwon Jeon wrote: > July 20, 2012, Thomas Abraham wrote: >> On 19 July 2012 09:21, Seungwon Jeon 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 (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. > >> > >> >> +#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. > 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.