Re: [PATCH 6/6 v2] MMC: JZ4740: Add support for LPM.

2019-10-21 Thread Ulf Hansson 
On Sat, 19 Oct 2019 at 22:44, Ezequiel Garcia
 wrote:
>
> On Friday, October 18, 2019 13:54 -03, Zhou Yanjie  
> wrote:
>
>
> >
> > >
> > > I also have a general question. Should we perhaps rename the driver
> > > from jz4740_mmc.c to ingenic.c (and the file for the DT bindings, the
> > > Kconfig, etc), as that seems like a more appropriate name? No?
> >
> > I am very much in favor of this proposal. Now jz4740_mmc.c is not only used
> > for the JZ4740 processor, it is also used for JZ4725, JZ4760, JZ4770, JZ4780
> > and X1000, and now Ingenic's processor is no longer named after JZ47xx,
> > it is divided into three product lines: M, T, and X. It is easy to cause
> > some
> > misunderstandings by using jz4740_mmc.c. At the same time, I think that
> > some register names also need to be adjusted. For example, the STLPPL
> > register name has only appeared in JZ4730 and JZ4740, and this register
> > in all subsequent processors is called CTRL. This time I was confused by
> > the STLPPL when I added drivers for the JZ4760's and X1000's LPM.
> >
>
> I am very much against renamings, for several reasons. As Paul already 
> mentioned, it's pointless and just adds noise to the git-log, making history 
> harder to recover. Driver file names don't really have to reflect the device 
> > > exactly. For the compatibility list, it's far easier to just git-grep for 
> compatible strings, or git-grep Documentation and/or Kconfig.

I have no strong opinions. What matters to me, is that people agree on
the best option, based on a case by case discussion.

>
> Renaming macros and register names, is equally pointless and equally 
> git-history invasive. Simply adding some documentation is enough.

Sounds like documentation is what people prefer here - and the DT doc
seems already fine in regards to that.

Perhaps some more words added to the header in driver's c-file could
be and option to consider, as today it only mentions "JZ4740 SD/MMC
controller driver".

Anyway, it's up to you. :-)

Kind regards
Uffe

Re: [PATCH 6/6 v2] MMC: JZ4740: Add support for LPM.

2019-10-19 Thread Ezequiel Garcia 
On Friday, October 18, 2019 13:54 -03, Zhou Yanjie  wrote: 
 

> 
> >
> > I also have a general question. Should we perhaps rename the driver
> > from jz4740_mmc.c to ingenic.c (and the file for the DT bindings, the
> > Kconfig, etc), as that seems like a more appropriate name? No?
> 
> I am very much in favor of this proposal. Now jz4740_mmc.c is not only used
> for the JZ4740 processor, it is also used for JZ4725, JZ4760, JZ4770, JZ4780
> and X1000, and now Ingenic's processor is no longer named after JZ47xx,
> it is divided into three product lines: M, T, and X. It is easy to cause 
> some
> misunderstandings by using jz4740_mmc.c. At the same time, I think that
> some register names also need to be adjusted. For example, the STLPPL
> register name has only appeared in JZ4730 and JZ4740, and this register
> in all subsequent processors is called CTRL. This time I was confused by
> the STLPPL when I added drivers for the JZ4760's and X1000's LPM.
> 

I am very much against renamings, for several reasons. As Paul already 
mentioned, it's pointless and just adds noise to the git-log, making history 
harder to recover. Driver file names don't really have to reflect the device 
exactly. For the compatibility list, it's far easier to just git-grep for 
compatible strings, or git-grep Documentation and/or Kconfig.

Renaming macros and register names, is equally pointless and equally 
git-history invasive. Simply adding some documentation is enough.

Thanks,
Ezequiel

Re: [PATCH 6/6 v2] MMC: JZ4740: Add support for LPM.

2019-10-19 Thread Zhou Yanjie 

Hi Uffe,

On 2019年10月18日 16:52, Ulf Hansson wrote:

On Sat, 12 Oct 2019 at 07:19, Zhou Yanjie  wrote:

add support for low power mode of Ingenic's MMC/SD Controller.

Signed-off-by: Zhou Yanjie 

I couldn't find a proper coverletter for the series, please provide
that next time as it really helps review.


I'm sorry, maybe some problems with my git send-email cause cover
later not to be sent out, next time I will pay attention to this problem.


Additionally, it seems like
you forgot to change the prefix of the patches to "mmc: jz4740" (or at
least you chosed upper case letters), but I will take care of that
this time. So, I have applied the series for next, thanks!


I'm very sorry, I have misunderstood, before I thought jz4740 as a proper
noun needs to be capitalized, I will pay attention to this next time.



I also have a general question. Should we perhaps rename the driver
from jz4740_mmc.c to ingenic.c (and the file for the DT bindings, the
Kconfig, etc), as that seems like a more appropriate name? No?


I am very much in favor of this proposal. Now jz4740_mmc.c is not only used
for the JZ4740 processor, it is also used for JZ4725, JZ4760, JZ4770, JZ4780
and X1000, and now Ingenic's processor is no longer named after JZ47xx,
it is divided into three product lines: M, T, and X. It is easy to cause 
some

misunderstandings by using jz4740_mmc.c. At the same time, I think that
some register names also need to be adjusted. For example, the STLPPL
register name has only appeared in JZ4730 and JZ4740, and this register
in all subsequent processors is called CTRL. This time I was confused by
the STLPPL when I added drivers for the JZ4760's and X1000's LPM.

I also can send a patch to rename it if you need.

Best regards!



Kind regards
Uffe



---
  drivers/mmc/host/jz4740_mmc.c | 23 +++
  1 file changed, 23 insertions(+)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index 44a04fe..4cbe7fb 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -43,6 +43,7 @@
  #define JZ_REG_MMC_RESP_FIFO   0x34
  #define JZ_REG_MMC_RXFIFO  0x38
  #define JZ_REG_MMC_TXFIFO  0x3C
+#define JZ_REG_MMC_LPM 0x40
  #define JZ_REG_MMC_DMAC0x44

  #define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7)
@@ -102,6 +103,12 @@
  #define JZ_MMC_DMAC_DMA_SEL BIT(1)
  #define JZ_MMC_DMAC_DMA_EN BIT(0)

+#defineJZ_MMC_LPM_DRV_RISING BIT(31)
+#defineJZ_MMC_LPM_DRV_RISING_QTR_PHASE_DLY BIT(31)
+#defineJZ_MMC_LPM_DRV_RISING_1NS_DLY BIT(30)
+#defineJZ_MMC_LPM_SMP_RISING_QTR_OR_HALF_PHASE_DLY BIT(29)
+#defineJZ_MMC_LPM_LOW_POWER_MODE_EN BIT(0)
+
  #define JZ_MMC_CLK_RATE 2400

  enum jz4740_mmc_version {
@@ -860,6 +867,22 @@ static int jz4740_mmc_set_clock_rate(struct 
jz4740_mmc_host *host, int rate)
 }

 writew(div, host->base + JZ_REG_MMC_CLKRT);
+
+   if (real_rate > 2500) {
+   if (host->version >= JZ_MMC_X1000) {
+   writel(JZ_MMC_LPM_DRV_RISING_QTR_PHASE_DLY |
+  JZ_MMC_LPM_SMP_RISING_QTR_OR_HALF_PHASE_DLY |
+  JZ_MMC_LPM_LOW_POWER_MODE_EN,
+  host->base + JZ_REG_MMC_LPM);
+   } else if (host->version >= JZ_MMC_JZ4760) {
+   writel(JZ_MMC_LPM_DRV_RISING |
+  JZ_MMC_LPM_LOW_POWER_MODE_EN,
+  host->base + JZ_REG_MMC_LPM);
+   } else if (host->version >= JZ_MMC_JZ4725B)
+   writel(JZ_MMC_LPM_LOW_POWER_MODE_EN,
+  host->base + JZ_REG_MMC_LPM);
+   }
+
 return real_rate;
  }

--
2.7.4

Re: [PATCH 6/6 v2] MMC: JZ4740: Add support for LPM.

2019-10-18 Thread Paul Cercueil 

Hi Uffe,


Le ven., oct. 18, 2019 at 10:52, Ulf Hansson  a 
écrit :

On Sat, 12 Oct 2019 at 07:19, Zhou Yanjie  wrote:


 add support for low power mode of Ingenic's MMC/SD Controller.

 Signed-off-by: Zhou Yanjie 


I couldn't find a proper coverletter for the series, please provide
that next time as it really helps review. Additionally, it seems like
you forgot to change the prefix of the patches to "mmc: jz4740" (or at
least you chosed upper case letters), but I will take care of that
this time. So, I have applied the series for next, thanks!

I also have a general question. Should we perhaps rename the driver
from jz4740_mmc.c to ingenic.c (and the file for the DT bindings, the
Kconfig, etc), as that seems like a more appropriate name? No?


Is there a kernel policy regarding renaming drivers? Since it trashes 
the git history. Anyway you're the subsystem maintainer so I guess 
that's up to you. I can send a patch to rename it if you want.


Cheers,
-Paul




Kind regards
Uffe



 ---
  drivers/mmc/host/jz4740_mmc.c | 23 +++
  1 file changed, 23 insertions(+)

 diff --git a/drivers/mmc/host/jz4740_mmc.c 
b/drivers/mmc/host/jz4740_mmc.c

 index 44a04fe..4cbe7fb 100644
 --- a/drivers/mmc/host/jz4740_mmc.c
 +++ b/drivers/mmc/host/jz4740_mmc.c
 @@ -43,6 +43,7 @@
  #define JZ_REG_MMC_RESP_FIFO   0x34
  #define JZ_REG_MMC_RXFIFO  0x38
  #define JZ_REG_MMC_TXFIFO  0x3C
 +#define JZ_REG_MMC_LPM 0x40
  #define JZ_REG_MMC_DMAC0x44

  #define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7)
 @@ -102,6 +103,12 @@
  #define JZ_MMC_DMAC_DMA_SEL BIT(1)
  #define JZ_MMC_DMAC_DMA_EN BIT(0)

 +#defineJZ_MMC_LPM_DRV_RISING BIT(31)
 +#defineJZ_MMC_LPM_DRV_RISING_QTR_PHASE_DLY BIT(31)
 +#defineJZ_MMC_LPM_DRV_RISING_1NS_DLY BIT(30)
 +#defineJZ_MMC_LPM_SMP_RISING_QTR_OR_HALF_PHASE_DLY BIT(29)
 +#defineJZ_MMC_LPM_LOW_POWER_MODE_EN BIT(0)
 +
  #define JZ_MMC_CLK_RATE 2400

  enum jz4740_mmc_version {
 @@ -860,6 +867,22 @@ static int jz4740_mmc_set_clock_rate(struct 
jz4740_mmc_host *host, int rate)

 }

 writew(div, host->base + JZ_REG_MMC_CLKRT);
 +
 +   if (real_rate > 2500) {
 +   if (host->version >= JZ_MMC_X1000) {
 +   writel(JZ_MMC_LPM_DRV_RISING_QTR_PHASE_DLY |
 +  
JZ_MMC_LPM_SMP_RISING_QTR_OR_HALF_PHASE_DLY |

 +  JZ_MMC_LPM_LOW_POWER_MODE_EN,
 +  host->base + JZ_REG_MMC_LPM);
 +   } else if (host->version >= JZ_MMC_JZ4760) {
 +   writel(JZ_MMC_LPM_DRV_RISING |
 +  JZ_MMC_LPM_LOW_POWER_MODE_EN,
 +  host->base + JZ_REG_MMC_LPM);
 +   } else if (host->version >= JZ_MMC_JZ4725B)
 +   writel(JZ_MMC_LPM_LOW_POWER_MODE_EN,
 +  host->base + JZ_REG_MMC_LPM);
 +   }
 +
 return real_rate;
  }

 --
 2.7.4

Re: [PATCH 6/6 v2] MMC: JZ4740: Add support for LPM.

2019-10-18 Thread Ulf Hansson 
On Sat, 12 Oct 2019 at 07:19, Zhou Yanjie  wrote:
>
> add support for low power mode of Ingenic's MMC/SD Controller.
>
> Signed-off-by: Zhou Yanjie 

I couldn't find a proper coverletter for the series, please provide
that next time as it really helps review. Additionally, it seems like
you forgot to change the prefix of the patches to "mmc: jz4740" (or at
least you chosed upper case letters), but I will take care of that
this time. So, I have applied the series for next, thanks!

I also have a general question. Should we perhaps rename the driver
from jz4740_mmc.c to ingenic.c (and the file for the DT bindings, the
Kconfig, etc), as that seems like a more appropriate name? No?

Kind regards
Uffe


> ---
>  drivers/mmc/host/jz4740_mmc.c | 23 +++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
> index 44a04fe..4cbe7fb 100644
> --- a/drivers/mmc/host/jz4740_mmc.c
> +++ b/drivers/mmc/host/jz4740_mmc.c
> @@ -43,6 +43,7 @@
>  #define JZ_REG_MMC_RESP_FIFO   0x34
>  #define JZ_REG_MMC_RXFIFO  0x38
>  #define JZ_REG_MMC_TXFIFO  0x3C
> +#define JZ_REG_MMC_LPM 0x40
>  #define JZ_REG_MMC_DMAC0x44
>
>  #define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7)
> @@ -102,6 +103,12 @@
>  #define JZ_MMC_DMAC_DMA_SEL BIT(1)
>  #define JZ_MMC_DMAC_DMA_EN BIT(0)
>
> +#defineJZ_MMC_LPM_DRV_RISING BIT(31)
> +#defineJZ_MMC_LPM_DRV_RISING_QTR_PHASE_DLY BIT(31)
> +#defineJZ_MMC_LPM_DRV_RISING_1NS_DLY BIT(30)
> +#defineJZ_MMC_LPM_SMP_RISING_QTR_OR_HALF_PHASE_DLY BIT(29)
> +#defineJZ_MMC_LPM_LOW_POWER_MODE_EN BIT(0)
> +
>  #define JZ_MMC_CLK_RATE 2400
>
>  enum jz4740_mmc_version {
> @@ -860,6 +867,22 @@ static int jz4740_mmc_set_clock_rate(struct 
> jz4740_mmc_host *host, int rate)
> }
>
> writew(div, host->base + JZ_REG_MMC_CLKRT);
> +
> +   if (real_rate > 2500) {
> +   if (host->version >= JZ_MMC_X1000) {
> +   writel(JZ_MMC_LPM_DRV_RISING_QTR_PHASE_DLY |
> +  
> JZ_MMC_LPM_SMP_RISING_QTR_OR_HALF_PHASE_DLY |
> +  JZ_MMC_LPM_LOW_POWER_MODE_EN,
> +  host->base + JZ_REG_MMC_LPM);
> +   } else if (host->version >= JZ_MMC_JZ4760) {
> +   writel(JZ_MMC_LPM_DRV_RISING |
> +  JZ_MMC_LPM_LOW_POWER_MODE_EN,
> +  host->base + JZ_REG_MMC_LPM);
> +   } else if (host->version >= JZ_MMC_JZ4725B)
> +   writel(JZ_MMC_LPM_LOW_POWER_MODE_EN,
> +  host->base + JZ_REG_MMC_LPM);
> +   }
> +
> return real_rate;
>  }
>
> --
> 2.7.4
>
>

[PATCH 6/6 v2] MMC: JZ4740: Add support for LPM.

2019-10-11 Thread Zhou Yanjie 
add support for low power mode of Ingenic's MMC/SD Controller.

Signed-off-by: Zhou Yanjie 
---
 drivers/mmc/host/jz4740_mmc.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
index 44a04fe..4cbe7fb 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
@@ -43,6 +43,7 @@
 #define JZ_REG_MMC_RESP_FIFO   0x34
 #define JZ_REG_MMC_RXFIFO  0x38
 #define JZ_REG_MMC_TXFIFO  0x3C
+#define JZ_REG_MMC_LPM 0x40
 #define JZ_REG_MMC_DMAC0x44
 
 #define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7)
@@ -102,6 +103,12 @@
 #define JZ_MMC_DMAC_DMA_SEL BIT(1)
 #define JZ_MMC_DMAC_DMA_EN BIT(0)
 
+#defineJZ_MMC_LPM_DRV_RISING BIT(31)
+#defineJZ_MMC_LPM_DRV_RISING_QTR_PHASE_DLY BIT(31)
+#defineJZ_MMC_LPM_DRV_RISING_1NS_DLY BIT(30)
+#defineJZ_MMC_LPM_SMP_RISING_QTR_OR_HALF_PHASE_DLY BIT(29)
+#defineJZ_MMC_LPM_LOW_POWER_MODE_EN BIT(0)
+
 #define JZ_MMC_CLK_RATE 2400
 
 enum jz4740_mmc_version {
@@ -860,6 +867,22 @@ static int jz4740_mmc_set_clock_rate(struct 
jz4740_mmc_host *host, int rate)
}
 
writew(div, host->base + JZ_REG_MMC_CLKRT);
+
+   if (real_rate > 2500) {
+   if (host->version >= JZ_MMC_X1000) {
+   writel(JZ_MMC_LPM_DRV_RISING_QTR_PHASE_DLY |
+  JZ_MMC_LPM_SMP_RISING_QTR_OR_HALF_PHASE_DLY |
+  JZ_MMC_LPM_LOW_POWER_MODE_EN,
+  host->base + JZ_REG_MMC_LPM);
+   } else if (host->version >= JZ_MMC_JZ4760) {
+   writel(JZ_MMC_LPM_DRV_RISING |
+  JZ_MMC_LPM_LOW_POWER_MODE_EN,
+  host->base + JZ_REG_MMC_LPM);
+   } else if (host->version >= JZ_MMC_JZ4725B)
+   writel(JZ_MMC_LPM_LOW_POWER_MODE_EN,
+  host->base + JZ_REG_MMC_LPM);
+   }
+
return real_rate;
 }
 
-- 
2.7.4