Re: [RFC V3 4/4] mmc: dw_mmc: exynos: configure SMU in exynos5420.

[Tomasz Figa <t.figa@samsung.com>]

Hi Seungwon,

On Thursday 29 of August 2013 19:36:28 Seungwon Jeon wrote:
> Yuvaraj Kumar wrote:
> > On Thu, Aug 29, 2013 at 3:14 PM, Seungwon Jeon <tgih.jun@samsung.com> 
wrote:
> > > On Thu, August 29, 2013, Alim Akhtar wrote:
> > >> Hi Seungwon,
> > >> 
> > >> On Thu, Aug 29, 2013 at 1:53 PM, Seungwon Jeon 
<tgih.jun@samsung.com> wrote:
> > >> > On Wed, August 28, 2013, Yuvaraj Kumar C D wrote:
> > >> >> Exynos5420 Mobile Storage Host controller has Security Management
> > >> >> Unit
> > >> >> (SMU) for channel 0 and channel 1 (mainly for eMMC).This patch
> > >> >> configures SMU for exynos5420.
> > >> >> 
> > >> >> This patch is on top of the below patch by Doug Anderson.
> > >> >> mmc: dw_mmc: Add exynos resume_noirq callback to clear WAKEUP_INT
> > >> >> 
> > >> >> changes since V2:
> > >> >>       1.Droppped the bypass-smu quirk.
> > >> >>       2.Changed the subject line for this patch
> > >> >>       
> > >> >>         add a quirk for SMU -> configure SMU in exynos5420
> > >> >> 
> > >> >> changes since V1:
> > >> >>       1.avoid code duplication by calling dw_mci_exynos_priv_init
> > >> >>       in
> > >> >>       
> > >> >>         resume path.
> > >> >> 
> > >> >> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@samsung.com>
> > >> >> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> > >> >> ---
> > >> >> 
> > >> >>  drivers/mmc/host/dw_mmc-exynos.c |   29
> > >> >>  +++++++++++++++++++++++++++++
> > >> >>  1 file changed, 29 insertions(+)
> > >> >> 
> > >> >> diff --git a/drivers/mmc/host/dw_mmc-exynos.c
> > >> >> b/drivers/mmc/host/dw_mmc-exynos.c index 19c845b..db28f10 100644
> > >> >> --- a/drivers/mmc/host/dw_mmc-exynos.c
> > >> >> +++ b/drivers/mmc/host/dw_mmc-exynos.c
> > >> >> @@ -35,6 +35,25 @@
> > >> >> 
> > >> >>  #define EXYNOS4210_FIXED_CIU_CLK_DIV 2
> > >> >>  #define EXYNOS4412_FIXED_CIU_CLK_DIV 4
> > >> >> 
> > >> >> +/* Block number in eMMC */
> > >> >> +#define DWMCI_BLOCK_NUM                      0xFFFFFFFF
> > >> >> +
> > >> >> +#define SDMMC_EMMCP_BASE             0x1000
> > >> >> +#define SDMMC_MPSECURITY             (SDMMC_EMMCP_BASE + 0x0010)
> > >> >> +#define SDMMC_MPSBEGIN0                      (SDMMC_EMMCP_BASE +
> > >> >> 0x0200) +#define SDMMC_MPSEND0                       
> > >> >> (SDMMC_EMMCP_BASE + 0x0204) +#define SDMMC_MPSCTRL0             
> > >> >>          (SDMMC_EMMCP_BASE + 0x020C) +
> > >> >> +/* SMU control bits */
> > >> >> +#define DWMCI_MPSCTRL_SECURE_READ_BIT                BIT(7)
> > >> >> +#define DWMCI_MPSCTRL_SECURE_WRITE_BIT               BIT(6)
> > >> >> +#define DWMCI_MPSCTRL_NON_SECURE_READ_BIT    BIT(5)
> > >> >> +#define DWMCI_MPSCTRL_NON_SECURE_WRITE_BIT   BIT(4)
> > >> >> +#define DWMCI_MPSCTRL_USE_FUSE_KEY           BIT(3)
> > >> >> +#define DWMCI_MPSCTRL_ECB_MODE                       BIT(2)
> > >> >> +#define DWMCI_MPSCTRL_ENCRYPTION             BIT(1)
> > >> >> +#define DWMCI_MPSCTRL_VALID                  BIT(0)
> > >> >> +
> > >> >> 
> > >> >>  /* Variations in Exynos specific dw-mshc controller */
> > >> >>  enum dw_mci_exynos_type {
> > >> >>  
> > >> >>       DW_MCI_TYPE_EXYNOS4210,
> > >> >> 
> > >> >> @@ -74,6 +93,15 @@ static int dw_mci_exynos_priv_init(struct
> > >> >> dw_mci *host)> >> >> 
> > >> >>  {
> > >> >>  
> > >> >>       struct dw_mci_exynos_priv_data *priv = host->priv;
> > >> >> 
> > >> >> +     if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS5420) {
> > >> >> +             mci_writel(host, MPSBEGIN0, 0);
> > >> >> +             mci_writel(host, MPSEND0, DWMCI_BLOCK_NUM);
> > >> >> +             mci_writel(host, MPSCTRL0,
> > >> >> DWMCI_MPSCTRL_SECURE_WRITE_BIT | +                    
> > >> >> DWMCI_MPSCTRL_NON_SECURE_READ_BIT |
> > >> >> +                     DWMCI_MPSCTRL_VALID |
> > >> >> +                     DWMCI_MPSCTRL_NON_SECURE_WRITE_BIT);
> > >> > 
> > >> > Yuvaraj,
> > >> > 
> > >> > Just one thing to check.
> > >> > ch#0 and #1 of three hosts are only valid for SMU control.
> > >> > Did you consider #2 host?
> > >> > It seems not.
> > >> 
> > >> Only host#0 and host#1 has SMU (On exynos5420).
> > >> Host #2 does not contain SMU.
> > > 
> > > Let me clear it.
> > > I mean that current change allows for ch2 to access registers related
> > > to SMU, even though ch2 doesn't actually has SMU. It's not valid IO
> > > area.> 
> > No,host#0 and host#1 are compatible with "samsung,exynos5420-dw-mshc"
> > but host#2 is compatible with the "samsung,exynos5250-dw-mshc".
> > Below is the DT patch posted in another thread.
> > [1] [PATCH V4] ARM: dts: Add dwmmc DT nodes for exynos5420 SOC
> > http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg21985.
> > html I will resubmit the DT patch[1] with reg = <0x12220000 0x1000> for
> > host#2.
> Hmm. If we notice exynos5420's other newly added registers apart from
> SMU, it is difficult to say exynos5420 is compatible with exynos5250
> fully for #2. But because there is no use case for new registers, it
> could be acceptable. Ok, we should be careful to configure DT file
> though.

Could you elaborate a bit more on the differences in DW MMC IP between 
Exynos5250 and Exynos5420? If the differences are significant, then it 
might be necessary to use different compatible value, as it should not 
depend on any use case (or lack of).

If the DW MMC version used in Exynos 5420 provides the whole set of 
functionality of the version used in Exynos 5250 and some extra extensions, 
then it can be called "compatible with Exynos 5250", but if you ever need 
to support those extra extensions, you will have to add a separate 
compatible string for it.

With regard to SMU that is present only on selected instances, you can as 
well create two compatible values for the DW MMC IP on Exynos 5420, e.g. 
"samsung,exynos5420-dw-mshc" and "samsung,exynos5420-dw-mshc-smu".

[Adding devicetree ML and DT maintainers on CC, as this is something they 
should be able to comment on.]

Best regards,
Tomasz