RE: [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support

2020-07-16 Thread Wan Mohamad, Wan Ahmad Zainie
Hi Vinod.

Thanks for the review.

> -Original Message-
> From: Vinod Koul 
> Sent: Monday, July 13, 2020 1:40 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> 
> Cc: kis...@ti.com; robh...@kernel.org; Shevchenko, Andriy
> ; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; MP, Sureshkumar
> ; Raja Subramanian, Lakshmi Bai
> 
> Subject: Re: [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support
> 
> On 02-07-20, 08:09, Wan Ahmad Zainie wrote:
> > Add support for eMMC PHY on Intel Keem Bay SoC.
> >
> > Signed-off-by: Wan Ahmad Zainie
> > 
> > ---
> >  drivers/phy/intel/Kconfig|  12 +
> >  drivers/phy/intel/Makefile   |   1 +
> >  drivers/phy/intel/phy-keembay-emmc.c | 314
> > +++
> >  3 files changed, 327 insertions(+)
> >  create mode 100644 drivers/phy/intel/phy-keembay-emmc.c
> >
> > diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
> > index 7b47682a4e0e..8ddda4fb95d2 100644
> > --- a/drivers/phy/intel/Kconfig
> > +++ b/drivers/phy/intel/Kconfig
> > @@ -22,3 +22,15 @@ config PHY_INTEL_EMMC
> > select GENERIC_PHY
> > help
> >   Enable this to support the Intel EMMC PHY
> > +
> > +config PHY_KEEMBAY_EMMC
> 
> Pls keep this in alphabetical sort

PHY_INTEL_ followed by PHY_KEEMBAY_ is alphabetically sorted.
Could you please help to clarify?

> 
> > +   tristate "Intel Keem Bay EMMC PHY driver"
> > +   depends on ARM64 || COMPILE_TEST
> 
> Intel and ARM64, aha, fun times!

😊

> 
> > +   depends on OF && HAS_IOMEM
> > +   select GENERIC_PHY
> > +   select REGMAP_MMIO
> > +   help
> > + Choose this option if you have an Intel Keem Bay SoC.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called phy-keembay-emmc.
> 
> phy-keembay-emmc.ko ?

Is it a must? I saw few Kconfig files omit .ko.
I can fix this in next version.

> 
> > diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
> > index 233d530dadde..6566334e7b77 100644
> > --- a/drivers/phy/intel/Makefile
> > +++ b/drivers/phy/intel/Makefile
> > @@ -1,3 +1,4 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_PHY_INTEL_COMBO)  += phy-intel-combo.o
> >  obj-$(CONFIG_PHY_INTEL_EMMC)+= phy-intel-emmc.o
> > +obj-$(CONFIG_PHY_KEEMBAY_EMMC) += phy-keembay-
> emmc.o
> 
> here as well
> 
> > +/* eMMC/SD/SDIO core/phy configuration registers */
> > +#define PHY_CFG_0  0x24
> > +#define  SEL_DLY_TXCLK_MASKBIT(29)
> > +#define  SEL_DLY_TXCLK(x)  (((x) << 29) & SEL_DLY_TXCLK_MASK)
> > +#define  OTAP_DLY_ENA_MASK BIT(27)
> > +#define  OTAP_DLY_ENA(x)   (((x) << 27) & OTAP_DLY_ENA_MASK)
> > +#define  OTAP_DLY_SEL_MASK GENMASK(26, 23)
> > +#define  OTAP_DLY_SEL(x)   (((x) << 23) & OTAP_DLY_SEL_MASK)
> 
> why not a generic helper to do (x) << ffs(reg - 1) & reg ?
> You can skip defining for each register that way!

Is it something like this following?
#define maskval(mask, val) (((val) << (ffs(mask) - 1)) & mask)

> 
> --
> ~Vinod


Re: [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support

2020-07-12 Thread Vinod Koul
On 02-07-20, 08:09, Wan Ahmad Zainie wrote:
> Add support for eMMC PHY on Intel Keem Bay SoC.
> 
> Signed-off-by: Wan Ahmad Zainie 
> ---
>  drivers/phy/intel/Kconfig|  12 +
>  drivers/phy/intel/Makefile   |   1 +
>  drivers/phy/intel/phy-keembay-emmc.c | 314 +++
>  3 files changed, 327 insertions(+)
>  create mode 100644 drivers/phy/intel/phy-keembay-emmc.c
> 
> diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
> index 7b47682a4e0e..8ddda4fb95d2 100644
> --- a/drivers/phy/intel/Kconfig
> +++ b/drivers/phy/intel/Kconfig
> @@ -22,3 +22,15 @@ config PHY_INTEL_EMMC
>   select GENERIC_PHY
>   help
> Enable this to support the Intel EMMC PHY
> +
> +config PHY_KEEMBAY_EMMC

Pls keep this in alphabetical sort

> + tristate "Intel Keem Bay EMMC PHY driver"
> + depends on ARM64 || COMPILE_TEST

Intel and ARM64, aha, fun times!

> + depends on OF && HAS_IOMEM
> + select GENERIC_PHY
> + select REGMAP_MMIO
> + help
> +   Choose this option if you have an Intel Keem Bay SoC.
> +
> +   To compile this driver as a module, choose M here: the module
> +   will be called phy-keembay-emmc.

phy-keembay-emmc.ko ?

> diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
> index 233d530dadde..6566334e7b77 100644
> --- a/drivers/phy/intel/Makefile
> +++ b/drivers/phy/intel/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_PHY_INTEL_COMBO)+= phy-intel-combo.o
>  obj-$(CONFIG_PHY_INTEL_EMMC)+= phy-intel-emmc.o
> +obj-$(CONFIG_PHY_KEEMBAY_EMMC)   += phy-keembay-emmc.o

here as well

> +/* eMMC/SD/SDIO core/phy configuration registers */
> +#define PHY_CFG_00x24
> +#define  SEL_DLY_TXCLK_MASK  BIT(29)
> +#define  SEL_DLY_TXCLK(x)(((x) << 29) & SEL_DLY_TXCLK_MASK)
> +#define  OTAP_DLY_ENA_MASK   BIT(27)
> +#define  OTAP_DLY_ENA(x) (((x) << 27) & OTAP_DLY_ENA_MASK)
> +#define  OTAP_DLY_SEL_MASK   GENMASK(26, 23)
> +#define  OTAP_DLY_SEL(x) (((x) << 23) & OTAP_DLY_SEL_MASK)

why not a generic helper to do (x) << ffs(reg - 1) & reg ?
You can skip defining for each register that way!

-- 
~Vinod


[PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support

2020-07-01 Thread Wan Ahmad Zainie
Add support for eMMC PHY on Intel Keem Bay SoC.

Signed-off-by: Wan Ahmad Zainie 
---
 drivers/phy/intel/Kconfig|  12 +
 drivers/phy/intel/Makefile   |   1 +
 drivers/phy/intel/phy-keembay-emmc.c | 314 +++
 3 files changed, 327 insertions(+)
 create mode 100644 drivers/phy/intel/phy-keembay-emmc.c

diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
index 7b47682a4e0e..8ddda4fb95d2 100644
--- a/drivers/phy/intel/Kconfig
+++ b/drivers/phy/intel/Kconfig
@@ -22,3 +22,15 @@ config PHY_INTEL_EMMC
select GENERIC_PHY
help
  Enable this to support the Intel EMMC PHY
+
+config PHY_KEEMBAY_EMMC
+   tristate "Intel Keem Bay EMMC PHY driver"
+   depends on ARM64 || COMPILE_TEST
+   depends on OF && HAS_IOMEM
+   select GENERIC_PHY
+   select REGMAP_MMIO
+   help
+ Choose this option if you have an Intel Keem Bay SoC.
+
+ To compile this driver as a module, choose M here: the module
+ will be called phy-keembay-emmc.
diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
index 233d530dadde..6566334e7b77 100644
--- a/drivers/phy/intel/Makefile
+++ b/drivers/phy/intel/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PHY_INTEL_COMBO)  += phy-intel-combo.o
 obj-$(CONFIG_PHY_INTEL_EMMC)+= phy-intel-emmc.o
+obj-$(CONFIG_PHY_KEEMBAY_EMMC) += phy-keembay-emmc.o
diff --git a/drivers/phy/intel/phy-keembay-emmc.c 
b/drivers/phy/intel/phy-keembay-emmc.c
new file mode 100644
index ..66b5931a749a
--- /dev/null
+++ b/drivers/phy/intel/phy-keembay-emmc.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel Keem Bay eMMC PHY driver
+ * Copyright (C) 2020 Intel Corporation
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* eMMC/SD/SDIO core/phy configuration registers */
+#define PHY_CFG_0  0x24
+#define  SEL_DLY_TXCLK_MASKBIT(29)
+#define  SEL_DLY_TXCLK(x)  (((x) << 29) & SEL_DLY_TXCLK_MASK)
+#define  OTAP_DLY_ENA_MASK BIT(27)
+#define  OTAP_DLY_ENA(x)   (((x) << 27) & OTAP_DLY_ENA_MASK)
+#define  OTAP_DLY_SEL_MASK GENMASK(26, 23)
+#define  OTAP_DLY_SEL(x)   (((x) << 23) & OTAP_DLY_SEL_MASK)
+#define  DLL_EN_MASK   BIT(10)
+#define  DLL_EN(x) (((x) << 10) & DLL_EN_MASK)
+#define  PWR_DOWN_MASK BIT(0)
+#define  PWR_DOWN(x)   (((x) << 0) & PWR_DOWN_MASK)
+
+#define PHY_CFG_2  0x2c
+#define  SEL_FREQ_MASK GENMASK(12, 10)
+#define  SEL_FREQ(x)   (((x) << 10) & SEL_FREQ_MASK)
+
+#define PHY_STAT   0x40
+#define  CAL_DONE_MASK BIT(6)
+#define  IS_CALDONE(x) ((x) & CAL_DONE_MASK)
+#define  DLL_RDY_MASK  BIT(5)
+#define  IS_DLLRDY(x)  ((x) & DLL_RDY_MASK)
+
+/* From ACS_eMMC51_16nFFC_RO1100_Userguide_v1p0.pdf p17 */
+#define FREQSEL_200M_170M  0x0
+#define FREQSEL_170M_140M  0x1
+#define FREQSEL_140M_110M  0x2
+#define FREQSEL_110M_80M   0x3
+#define FREQSEL_80M_50M0x4
+
+struct keembay_emmc_phy {
+   struct regmap *syscfg;
+   struct clk *emmcclk;
+};
+
+static const struct regmap_config keembay_regmap_config = {
+   .reg_bits = 32,
+   .val_bits = 32,
+   .reg_stride = 4,
+};
+
+static int keembay_emmc_phy_power(struct phy *phy, bool on_off)
+{
+   struct keembay_emmc_phy *priv = phy_get_drvdata(phy);
+   unsigned int caldone;
+   unsigned int dllrdy;
+   unsigned int freqsel;
+   unsigned int mhz;
+   int ret;
+
+   /*
+* Keep phyctrl_pdb and phyctrl_endll low to allow
+* initialization of CALIO state M/C DFFs
+*/
+   ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, PWR_DOWN_MASK,
+PWR_DOWN(0));
+   if (ret) {
+   dev_err(&phy->dev, "CALIO power down bar failed: %d\n", ret);
+   return ret;
+   }
+
+   ret = regmap_update_bits(priv->syscfg, PHY_CFG_0, DLL_EN_MASK,
+DLL_EN(0));
+   if (ret) {
+   dev_err(&phy->dev, "turn off the dll failed: %d\n", ret);
+   return ret;
+   }
+
+   /* Already finish power off above */
+   if (!on_off)
+   return 0;
+
+   mhz = DIV_ROUND_CLOSEST(clk_get_rate(priv->emmcclk), 100);
+   if (mhz <= 200 && mhz >= 170)
+   freqsel = FREQSEL_200M_170M;
+   else if (mhz <= 170 && mhz >= 140)
+   freqsel = FREQSEL_170M_140M;
+   else if (mhz <= 140 && mhz >= 110)
+   freqsel = FREQSEL_140M_110M;
+   else if (mhz <= 110 && mhz >= 80)
+   freqsel = FREQSEL_110M_80M;
+   else if (mhz <= 80 && mhz >= 50)
+   freqsel = FREQSEL_80M_50M;
+   else
+   freqsel = 0x0;
+
+   if (mhz < 50 || mhz > 200)
+   dev_warn(&phy->dev, "U