Re: [v2] mmc: sdhci-cadence: add HS400 enhanced strobe support

2017-03-03 Thread Masahiro Yamada 
Hi Piotr,


2017-03-03 17:31 GMT+09:00 Piotr Sroka :
> Add support for HS400ES mode to Cadence SDHCI driver.
>
> Signed-off-by: Piotr Sroka 


Thanks for your patch.  It looks almost good to me.
My comments are about some coding style issues only.
Please see below.




> +static void sdhci_cdns_get_emmc_mode(struct sdhci_cdns_priv *priv, u32 *mode)
> +{
> +   *mode = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
> +   *mode = *mode & SDHCI_CDNS_HRS06_MODE_MASK;
> +}
> +

Why doesn't this function simply return u32 value?





>
> +static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
> +struct mmc_ios *ios)
> +{
> +   struct sdhci_host *host = mmc_priv(mmc);
> +   struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> +   u32 timingMode;

Could you avoid CamelCase, please?
(perhaps does checkpatch.pl complain about this?).

I guess "mode" could be enough.



> +   priv->enhanced_strobe = ios->enhanced_strobe;
> +
> +   sdhci_cdns_get_emmc_mode(priv, );
> +
> +   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400 &&
> +   ios->enhanced_strobe)
> +   sdhci_cdns_set_emmc_mode(priv,
> +SDHCI_CDNS_HRS06_MODE_MMC_HS400ES);
> +
> +   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400ES &&
> +   !ios->enhanced_strobe)

You could stretch this if-conditional within 80-col
by renaming "timingMode" into "mode".





> +   host->mmc_host_ops.hs400_enhanced_strobe =
> +   sdhci_cdns_hs400_enhanced_strobe;

I should not say this if this is the only matter,
but can you move the 2nd line to the right by inserting some more tabs?



FYI:
If you have not checked the coding-style guideline yet, and you are interested,
it is a good idea to read Documentation/process/coding-style.rst


Some of my comments are based on the following statements in that document:

  - "LOCAL variable names should be short, and to the point."
  - "Descendants are always substantially shorter than the parent and
 are placed substantially to the right."

We need not too be nervous about the style,
but generally speaking, it will be nice to keep the style consistent
unless there is a specific reason.

Thanks!

-- 
Best Regards
Masahiro Yamada

Re: [v2] mmc: sdhci-cadence: add HS400 enhanced strobe support

2017-03-03 Thread Masahiro Yamada 
Hi Piotr,


2017-03-03 17:31 GMT+09:00 Piotr Sroka :
> Add support for HS400ES mode to Cadence SDHCI driver.
>
> Signed-off-by: Piotr Sroka 


Thanks for your patch.  It looks almost good to me.
My comments are about some coding style issues only.
Please see below.




> +static void sdhci_cdns_get_emmc_mode(struct sdhci_cdns_priv *priv, u32 *mode)
> +{
> +   *mode = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
> +   *mode = *mode & SDHCI_CDNS_HRS06_MODE_MASK;
> +}
> +

Why doesn't this function simply return u32 value?





>
> +static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
> +struct mmc_ios *ios)
> +{
> +   struct sdhci_host *host = mmc_priv(mmc);
> +   struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> +   u32 timingMode;

Could you avoid CamelCase, please?
(perhaps does checkpatch.pl complain about this?).

I guess "mode" could be enough.



> +   priv->enhanced_strobe = ios->enhanced_strobe;
> +
> +   sdhci_cdns_get_emmc_mode(priv, );
> +
> +   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400 &&
> +   ios->enhanced_strobe)
> +   sdhci_cdns_set_emmc_mode(priv,
> +SDHCI_CDNS_HRS06_MODE_MMC_HS400ES);
> +
> +   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400ES &&
> +   !ios->enhanced_strobe)

You could stretch this if-conditional within 80-col
by renaming "timingMode" into "mode".





> +   host->mmc_host_ops.hs400_enhanced_strobe =
> +   sdhci_cdns_hs400_enhanced_strobe;

I should not say this if this is the only matter,
but can you move the 2nd line to the right by inserting some more tabs?



FYI:
If you have not checked the coding-style guideline yet, and you are interested,
it is a good idea to read Documentation/process/coding-style.rst


Some of my comments are based on the following statements in that document:

  - "LOCAL variable names should be short, and to the point."
  - "Descendants are always substantially shorter than the parent and
 are placed substantially to the right."

We need not too be nervous about the style,
but generally speaking, it will be nice to keep the style consistent
unless there is a specific reason.

Thanks!

-- 
Best Regards
Masahiro Yamada

[v2] mmc: sdhci-cadence: add HS400 enhanced strobe support

2017-03-03 Thread Piotr Sroka 
Add support for HS400ES mode to Cadence SDHCI driver.

Signed-off-by: Piotr Sroka 
---
Changes in v2:
- Modify enhanced strobe function to handle disabling 
  enhanced strobe inside the function. 
  Do no relay on that mmc_set_ios() is called 
  immediately after host->ops->hs400_enhanced_strobe(host, >ios).
---
 drivers/mmc/host/sdhci-cadence.c | 57 +++-
 1 file changed, 50 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index 316cfec..6198ae2 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -40,6 +40,7 @@
 #define   SDHCI_CDNS_HRS06_MODE_MMC_DDR0x3
 #define   SDHCI_CDNS_HRS06_MODE_MMC_HS200  0x4
 #define   SDHCI_CDNS_HRS06_MODE_MMC_HS400  0x5
+#define   SDHCI_CDNS_HRS06_MODE_MMC_HS400ES0x6
 
 /* SRS - Slot Register Set (SDHCI-compatible) */
 #define SDHCI_CDNS_SRS_BASE0x200
@@ -64,6 +65,7 @@
 
 struct sdhci_cdns_priv {
void __iomem *hrs_addr;
+   bool enhanced_strobe;
 };
 
 static void sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
@@ -108,11 +110,28 @@ static unsigned int sdhci_cdns_get_timeout_clock(struct 
sdhci_host *host)
return host->max_clk / 1000;
 }
 
+static void sdhci_cdns_set_emmc_mode(struct sdhci_cdns_priv *priv, u32 mode)
+{
+   u32 tmp;
+
+   /* The speed mode for eMMC is selected by HRS06 register */
+   tmp = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
+   tmp &= ~SDHCI_CDNS_HRS06_MODE_MASK;
+   tmp |= mode;
+   writel(tmp, priv->hrs_addr + SDHCI_CDNS_HRS06);
+}
+
+static void sdhci_cdns_get_emmc_mode(struct sdhci_cdns_priv *priv, u32 *mode)
+{
+   *mode = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
+   *mode = *mode & SDHCI_CDNS_HRS06_MODE_MASK;
+}
+
 static void sdhci_cdns_set_uhs_signaling(struct sdhci_host *host,
 unsigned int timing)
 {
struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
-   u32 mode, tmp;
+   u32 mode;
 
switch (timing) {
case MMC_TIMING_MMC_HS:
@@ -125,18 +144,17 @@ static void sdhci_cdns_set_uhs_signaling(struct 
sdhci_host *host,
mode = SDHCI_CDNS_HRS06_MODE_MMC_HS200;
break;
case MMC_TIMING_MMC_HS400:
-   mode = SDHCI_CDNS_HRS06_MODE_MMC_HS400;
+   if (priv->enhanced_strobe)
+   mode = SDHCI_CDNS_HRS06_MODE_MMC_HS400ES;
+   else
+   mode = SDHCI_CDNS_HRS06_MODE_MMC_HS400;
break;
default:
mode = SDHCI_CDNS_HRS06_MODE_SD;
break;
}
 
-   /* The speed mode for eMMC is selected by HRS06 register */
-   tmp = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
-   tmp &= ~SDHCI_CDNS_HRS06_MODE_MASK;
-   tmp |= mode;
-   writel(tmp, priv->hrs_addr + SDHCI_CDNS_HRS06);
+   sdhci_cdns_set_emmc_mode(priv, mode);
 
/* For SD, fall back to the default handler */
if (mode == SDHCI_CDNS_HRS06_MODE_SD)
@@ -213,6 +231,28 @@ static int sdhci_cdns_execute_tuning(struct mmc_host *mmc, 
u32 opcode)
return sdhci_cdns_set_tune_val(host, end_of_streak - max_streak / 2);
 }
 
+static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
+struct mmc_ios *ios)
+{
+   struct sdhci_host *host = mmc_priv(mmc);
+   struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
+   u32 timingMode;
+
+   priv->enhanced_strobe = ios->enhanced_strobe;
+
+   sdhci_cdns_get_emmc_mode(priv, );
+
+   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400 &&
+   ios->enhanced_strobe)
+   sdhci_cdns_set_emmc_mode(priv,
+SDHCI_CDNS_HRS06_MODE_MMC_HS400ES);
+
+   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400ES &&
+   !ios->enhanced_strobe)
+   sdhci_cdns_set_emmc_mode(priv,
+SDHCI_CDNS_HRS06_MODE_MMC_HS400);
+}
+
 static int sdhci_cdns_probe(struct platform_device *pdev)
 {
struct sdhci_host *host;
@@ -240,8 +280,11 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
 
priv = sdhci_cdns_priv(host);
priv->hrs_addr = host->ioaddr;
+   priv->enhanced_strobe = false;
host->ioaddr += SDHCI_CDNS_SRS_BASE;
host->mmc_host_ops.execute_tuning = sdhci_cdns_execute_tuning;
+   host->mmc_host_ops.hs400_enhanced_strobe =
+   sdhci_cdns_hs400_enhanced_strobe;
 
ret = mmc_of_parse(host->mmc);
if (ret)
-- 
2.2.2

[v2] mmc: sdhci-cadence: add HS400 enhanced strobe support

2017-03-03 Thread Piotr Sroka 
Add support for HS400ES mode to Cadence SDHCI driver.

Signed-off-by: Piotr Sroka 
---
Changes in v2:
- Modify enhanced strobe function to handle disabling 
  enhanced strobe inside the function. 
  Do no relay on that mmc_set_ios() is called 
  immediately after host->ops->hs400_enhanced_strobe(host, >ios).
---
 drivers/mmc/host/sdhci-cadence.c | 57 +++-
 1 file changed, 50 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
index 316cfec..6198ae2 100644
--- a/drivers/mmc/host/sdhci-cadence.c
+++ b/drivers/mmc/host/sdhci-cadence.c
@@ -40,6 +40,7 @@
 #define   SDHCI_CDNS_HRS06_MODE_MMC_DDR0x3
 #define   SDHCI_CDNS_HRS06_MODE_MMC_HS200  0x4
 #define   SDHCI_CDNS_HRS06_MODE_MMC_HS400  0x5
+#define   SDHCI_CDNS_HRS06_MODE_MMC_HS400ES0x6
 
 /* SRS - Slot Register Set (SDHCI-compatible) */
 #define SDHCI_CDNS_SRS_BASE0x200
@@ -64,6 +65,7 @@
 
 struct sdhci_cdns_priv {
void __iomem *hrs_addr;
+   bool enhanced_strobe;
 };
 
 static void sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
@@ -108,11 +110,28 @@ static unsigned int sdhci_cdns_get_timeout_clock(struct 
sdhci_host *host)
return host->max_clk / 1000;
 }
 
+static void sdhci_cdns_set_emmc_mode(struct sdhci_cdns_priv *priv, u32 mode)
+{
+   u32 tmp;
+
+   /* The speed mode for eMMC is selected by HRS06 register */
+   tmp = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
+   tmp &= ~SDHCI_CDNS_HRS06_MODE_MASK;
+   tmp |= mode;
+   writel(tmp, priv->hrs_addr + SDHCI_CDNS_HRS06);
+}
+
+static void sdhci_cdns_get_emmc_mode(struct sdhci_cdns_priv *priv, u32 *mode)
+{
+   *mode = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
+   *mode = *mode & SDHCI_CDNS_HRS06_MODE_MASK;
+}
+
 static void sdhci_cdns_set_uhs_signaling(struct sdhci_host *host,
 unsigned int timing)
 {
struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
-   u32 mode, tmp;
+   u32 mode;
 
switch (timing) {
case MMC_TIMING_MMC_HS:
@@ -125,18 +144,17 @@ static void sdhci_cdns_set_uhs_signaling(struct 
sdhci_host *host,
mode = SDHCI_CDNS_HRS06_MODE_MMC_HS200;
break;
case MMC_TIMING_MMC_HS400:
-   mode = SDHCI_CDNS_HRS06_MODE_MMC_HS400;
+   if (priv->enhanced_strobe)
+   mode = SDHCI_CDNS_HRS06_MODE_MMC_HS400ES;
+   else
+   mode = SDHCI_CDNS_HRS06_MODE_MMC_HS400;
break;
default:
mode = SDHCI_CDNS_HRS06_MODE_SD;
break;
}
 
-   /* The speed mode for eMMC is selected by HRS06 register */
-   tmp = readl(priv->hrs_addr + SDHCI_CDNS_HRS06);
-   tmp &= ~SDHCI_CDNS_HRS06_MODE_MASK;
-   tmp |= mode;
-   writel(tmp, priv->hrs_addr + SDHCI_CDNS_HRS06);
+   sdhci_cdns_set_emmc_mode(priv, mode);
 
/* For SD, fall back to the default handler */
if (mode == SDHCI_CDNS_HRS06_MODE_SD)
@@ -213,6 +231,28 @@ static int sdhci_cdns_execute_tuning(struct mmc_host *mmc, 
u32 opcode)
return sdhci_cdns_set_tune_val(host, end_of_streak - max_streak / 2);
 }
 
+static void sdhci_cdns_hs400_enhanced_strobe(struct mmc_host *mmc,
+struct mmc_ios *ios)
+{
+   struct sdhci_host *host = mmc_priv(mmc);
+   struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
+   u32 timingMode;
+
+   priv->enhanced_strobe = ios->enhanced_strobe;
+
+   sdhci_cdns_get_emmc_mode(priv, );
+
+   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400 &&
+   ios->enhanced_strobe)
+   sdhci_cdns_set_emmc_mode(priv,
+SDHCI_CDNS_HRS06_MODE_MMC_HS400ES);
+
+   if (timingMode == SDHCI_CDNS_HRS06_MODE_MMC_HS400ES &&
+   !ios->enhanced_strobe)
+   sdhci_cdns_set_emmc_mode(priv,
+SDHCI_CDNS_HRS06_MODE_MMC_HS400);
+}
+
 static int sdhci_cdns_probe(struct platform_device *pdev)
 {
struct sdhci_host *host;
@@ -240,8 +280,11 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
 
priv = sdhci_cdns_priv(host);
priv->hrs_addr = host->ioaddr;
+   priv->enhanced_strobe = false;
host->ioaddr += SDHCI_CDNS_SRS_BASE;
host->mmc_host_ops.execute_tuning = sdhci_cdns_execute_tuning;
+   host->mmc_host_ops.hs400_enhanced_strobe =
+   sdhci_cdns_hs400_enhanced_strobe;
 
ret = mmc_of_parse(host->mmc);
if (ret)
-- 
2.2.2