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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DM6PR11MB37212AA1A79526A55A90D812DD7F0@DM6PR11MB3721.namprd11.prod.outlook.com>
Date:   Thu, 16 Jul 2020 07:58:15 +0000
From:   "Wan Mohamad, Wan Ahmad Zainie" 
        <wan.ahmad.zainie.wan.mohamad@...el.com>
To:     Vinod Koul <vkoul@...nel.org>
CC:     "kishon@...com" <kishon@...com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "Shevchenko, Andriy" <andriy.shevchenko@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "MP, Sureshkumar" <sureshkumar.mp@...el.com>,
        "Raja Subramanian, Lakshmi Bai" 
        <lakshmi.bai.raja.subramanian@...el.com>
Subject: RE: [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support

Hi Vinod.

Thanks for the review.

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