RE: [PATCH v3 6/6] mmc: dw_mmc: add samsung exynos5250 specificextentions

[Seungwon Jeon]

July 23, 2012, Thomas Abraham <thomas.abraham@xxxxxxxxxx> wrote:
> On 20 July 2012 16:08, Seungwon Jeon <tgih.jun@xxxxxxxxxxx> wrote:
> > July 20, 2012, Thomas Abraham <thomas.abraham@xxxxxxxxxx> wrote:
> >> On 19 July 2012 09:21, Seungwon Jeon <tgih.jun@xxxxxxxxxxx> 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
> >> 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.
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
> 
> >
> >> >
> >> >> +#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.
> --
> 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/