mbox series

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

Message ID 20200702000934.3258-1-wan.ahmad.zainie.wan.mohamad@intel.com
Headers show
Series phy: intel: Add Keem Bay eMMC PHY support | expand

Message

Wan Ahmad Zainie July 2, 2020, 12:09 a.m. UTC 
Hi.

The first part is to document DT bindings for Keem Bay eMMC PHY.

The second is the driver file, loosely based on phy-rockchip-emmc.c
and phy-intel-emmc.c. The latter is not being reused as there are
quite a number of differences i.e. registers offset, supported clock
rates, bitfield to set.

The patch was tested with Keem Bay evaluation module board.

Thank you.

Best regards,
Zainie

Changes since resend v5:
- In keembay_emmc_phy_power(), remove redundant return ret;.

Resend v5:
- Include missing changes done for phy-keembay-emmc.c (See Changes since
  v3)

Changes since v4:
- In Kconfig file, add depends on ARM64 || COMPILE_TEST and
  depends on OF && HAS_IOMEM.
- In Kconfig file, reword the help section to be more verbose.

Changes since v3:
- Exit keembay_emmc_phy_power() with return ret;.
- In keembay_emmc_phy_init(), use PTR_ERR_OR_ZERO(...).
- In keembay_emmc_phy_probe(), devm_regmap_init_mmio(...) in single
  line.

Changes since v2:
- Modify DT example to use single cell for address and size.

Changes since v1:
- Rework phy-keembay-emmc.c to make it similar to phy-intel-emmc.c.
- Use regmap_mmio, and remove reference to intel,syscon.
- Use node name phy@....
- Update license i.e. use dual license.


Wan Ahmad Zainie (2):
  dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings
  phy: intel: Add Keem Bay eMMC PHY support

 .../bindings/phy/intel,keembay-emmc-phy.yaml  |  44 +++
 drivers/phy/intel/Kconfig                     |  12 +
 drivers/phy/intel/Makefile                    |   1 +
 drivers/phy/intel/phy-keembay-emmc.c          | 314 ++++++++++++++++++
 4 files changed, 371 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
 create mode 100644 drivers/phy/intel/phy-keembay-emmc.c

Comments

Vinod Koul July 13, 2020, 5:40 a.m. UTC | #1 
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 <wan.ahmad.zainie.wan.mohamad@intel.com>
> ---
>  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_0		0x24
> +#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!
Wan Ahmad Zainie July 16, 2020, 7:58 a.m. UTC | #2 
Hi Vinod.

Thanks for the review.

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Monday, July 13, 2020 1:40 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Cc: kishon@ti.com; robh+dt@kernel.org; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; MP, Sureshkumar
> <sureshkumar.mp@intel.com>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@intel.com>
> 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
> > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > ---
> >  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_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!

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

> 
> --
> ~Vinod