RE: [PATCH v5 19/19] dt-bindings: usb: intel,keembay-dwc3: Validate DWC3 sub-node
Hi Serge.
> -Original Message-
> From: Serge Semin
> Sent: Saturday, December 5, 2020 11:24 PM
> To: Nyman, Mathias ; Felipe Balbi
> ; Krzysztof Kozlowski ; Greg Kroah-
> Hartman ; Rob Herring
> ; Chunfeng Yun ;
> Wan Mohamad, Wan Ahmad Zainie
>
> Cc: Serge Semin ; Serge Semin
> ; Alexey Malahov
> ; Pavel Parkhomenko
> ; Andy Gross
> ; Bjorn Andersson ;
> Manu Gautam ; Roger Quadros
> ; Lad Prabhakar lad...@bp.renesas.com>; Yoshihiro Shimoda
> ; narmstrong
> ; Kevin Hilman ;
> Martin Blumenstingl ; linux-arm-
> ker...@lists.infradead.org; linux-snps-...@lists.infradead.org; linux-
> m...@vger.kernel.org; linuxppc-...@lists.ozlabs.org; linux-
> u...@vger.kernel.org; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH v5 19/19] dt-bindings: usb: intel,keembay-dwc3: Validate
> DWC3 sub-node
>
> Intel Keem Bay DWC3 compatible DT nodes are supposed to have a DWC
> USB3 compatible sub-node to describe a fully functioning USB interface. Let's
> use the available DWC USB3 DT schema to validate the Qualcomm DWC3 sub-
> nodes.
>
> Note since the generic DWC USB3 DT node is supposed to be named as
> generic USB HCD ("^usb(@.*)?") one we have to accordingly fix the sub-
> nodes name regexp and fix the DT node example.
>
> Signed-off-by: Serge Semin
LGTM. With minor change to fix the typo above, Qualcomm to Intel
Keem Bay,
Acked-by: Wan Ahmad Zainie
>
> ---
>
> Changelog v5:
> - This is a new patch created for the new Intel Keem Bay bindings file,
> which has been added just recently.
> ---
> .../devicetree/bindings/usb/intel,keembay-dwc3.yaml | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/intel,keembay-
> dwc3.yaml b/Documentation/devicetree/bindings/usb/intel,keembay-
> dwc3.yaml
> index dd32c10ce6c7..43b91ab62004 100644
> --- a/Documentation/devicetree/bindings/usb/intel,keembay-dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/intel,keembay-dwc3.yaml
> @@ -34,11 +34,8 @@ properties:
> # Required child node:
>
> patternProperties:
> - "^dwc3@[0-9a-f]+$":
> -type: object
> -description:
> - A child node must exist to represent the core DWC3 IP block.
> - The content of the node is defined in dwc3.txt.
> + "^usb@[0-9a-f]+$":
> +$ref: snps,dwc3.yaml#
>
> required:
>- compatible
> @@ -68,7 +65,7 @@ examples:
>#address-cells = <1>;
>#size-cells = <1>;
>
> - dwc3@3400 {
> + usb@3400 {
> compatible = "snps,dwc3";
> reg = <0x3400 0x1>;
> interrupts = ;
> --
> 2.29.2
Best regards,
Zainie
RE: [PATCH v4 0/2] phy: intel: Add Keem Bay USB PHY support
Thanks Vinod. > -Original Message- > From: Vinod Koul > Sent: Monday, November 30, 2020 6:02 PM > To: Wan Mohamad, Wan Ahmad Zainie > > Cc: kis...@ti.com; robh...@kernel.org; linux-kernel@vger.kernel.org; > devicet...@vger.kernel.org; andriy.shevche...@linux.intel.com; > mgr...@linux.intel.com; Raja Subramanian, Lakshmi Bai > > Subject: Re: [PATCH v4 0/2] phy: intel: Add Keem Bay USB PHY support > > On 16-11-20, 20:08, Wan Ahmad Zainie wrote: > > Hi. > > > > Intel Keem Bay USB subsystem incorporates DesignWare USB3.1 > > controller, an USB3.1 (Gen1/2) PHY and an USB2.0 PHY. It is a Dual > > Role Device (DRD), operating as either a USB host or a USB device. > > Applied all, thanks > > -- > ~Vinod
RE: [PATCH v3 2/2] phy: intel: Add Keem Bay USB PHY support
Hi Andy.
> -Original Message-
> From: Andy Shevchenko
> Sent: Thursday, November 12, 2020 10:29 PM
> To: Wan Mohamad, Wan Ahmad Zainie
>
> Cc: kis...@ti.com; vk...@kernel.org; robh...@kernel.org; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org;
> mgr...@linux.intel.com; Raja Subramanian, Lakshmi Bai
>
> Subject: Re: [PATCH v3 2/2] phy: intel: Add Keem Bay USB PHY support
>
> On Thu, Nov 12, 2020 at 05:58:21PM +0800, Wan Ahmad Zainie wrote:
> > Add support for USB PHY on Intel Keem Bay SoC.
>
> Any elaboration here? What is this PHY (USB2 or USB3 or?.. etc)?
Yes, I can elaborate this.
>
> ...
>
> > +config PHY_INTEL_KEEMBAY_USB
> > + tristate "Intel Keem Bay USB PHY driver"
>
> > + depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)
>
> It seems other drivers that are not using ARM specific calls moved to
>
> depends on ARCH_KEEMBAY || COMPILE_TEST
I will fix in v4.
>
> > + depends on HAS_IOMEM
> > + select GENERIC_PHY
> > + select REGMAP_MMIO
>
> ...
>
> > +#define USS_CPR_MASK 0x7f
>
> GENMASK() ?
I will fix in v4.
>
> ...
>
> > +static const struct regmap_config keembay_regmap_config = {
> > + .reg_bits = 32,
> > + .val_bits = 32,
> > + .reg_stride = 4,
>
> .max_register?
It is optional. But yes, I can add,
.max_register = USS_USB_TIEOFFS_CONSTANTS_REG1
>
> > +};
>
> --
> With Best Regards,
> Andy Shevchenko
>
Best regards,
Zainie
RE: [PATCH v2 2/2] phy: intel: Add Keem Bay USB PHY support
Hi Andy. Thanks for the review and sorry for the late reply. > -Original Message- > From: Andy Shevchenko > Sent: Monday, November 9, 2020 7:41 PM > To: Wan Mohamad, Wan Ahmad Zainie > > Cc: kis...@ti.com; vk...@kernel.org; robh...@kernel.org; linux- > ker...@vger.kernel.org; devicet...@vger.kernel.org; > mgr...@linux.intel.com; Raja Subramanian, Lakshmi Bai > > Subject: Re: [PATCH v2 2/2] phy: intel: Add Keem Bay USB PHY support > > On Mon, Nov 09, 2020 at 11:16:54AM +0800, Wan Ahmad Zainie wrote: > > Add support for USB PHY on Intel Keem Bay SoC. > > ... > > > +config PHY_INTEL_KEEMBAY_USB > > + tristate "Intel Keem Bay USB PHY driver" > > + depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST) > > > + depends on OF && HAS_IOMEM > > Do you really need dependency to OF (yes, I see that it will be not > functional, > but still can be compile tested)? No, I can drop OF. I will remove in v3. > > > + 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-usb.ko. > > ... > > > +#include > > +#include > > +#include > > +#include > > +#include > > > +#include > > No evidence of anything being used in this code. > mod_devicetable.h is missed, though. I will fix in v3. Remove of.h and add mod_devicetable.h. > > > +#include > > +#include > > +#include > > ... > > > + usleep_range(30, 50); > > Why 30-50? I take this value from boot firmware. There is a delay of 30us after clearing IDDQ_enable bit. I believe the purpose is to ensure all analog blocks are powered up. > > ... > > > + usleep_range(20, 50); > > Why these numbers? In Keem Bay data book, under USB initialization section, there is step that there must be a minimum 20us wait after clock enable, before bringing PHYs out of reset. 50 is the value that I picked randomly. Is usleep_range(20, 20) Better? > > ... > > > + usleep_range(2, 10); > > Ditto. Under the same section above, there is a step for 2us wait. I believe it is for register write to go through. > > ... > > > + usleep_range(20, 50); > > Ditto. Under the same section above, there is a step to wait 20us after setting SRAM load bit, before release the controller reset. I will add comment for those 4 delay above. Before I proceed with v3, I would like to know if I should use udelay(), instead of usleep_range()? I refer to https://www.kernel.org/doc/Documentation/timers/timers-howto.txt. > > > ... > > > + struct device_node *np = dev->of_node; > > It's being used only once and it doesn't bring any benefit. I will fix in v3. Use dev->of_node directly. > > -- > With Best Regards, > Andy Shevchenko > Best regards, Zainie
RE: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings
Resend the reply. > -Original Message- > From: Vinod Koul > Sent: Tuesday, September 1, 2020 1:52 PM > To: Wan Mohamad, Wan Ahmad Zainie > > Cc: kis...@ti.com; robh...@kernel.org; Shevchenko, Andriy > ; eswara.k...@linux.intel.com; > vadivel.muruganx.ramuthe...@linux.intel.com; Raja Subramanian, Lakshmi > Bai ; linux- > ker...@vger.kernel.org; devicet...@vger.kernel.org > Subject: Re: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC > PHY bindings > > On 01-09-20, 04:58, Wan Mohamad, Wan Ahmad Zainie wrote: > > > > > @@ -0,0 +1,44 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2 > > > > +--- > > > > +$id: "http://devicetree.org/schemas/phy/intel,keembay-emmc- > > > phy.yaml#" > > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"; > > > > + > > > > +title: Intel Keem Bay eMMC PHY bindings > > > > > > This seems same as > > > Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml, > why > > > not add a new compatible in lgm binding, or did I miss a difference? > > > > AFAIK, LGM make use of syscon node, whilst KMB does not. > > And LGM and KMB belongs to different SoC family. So, I prefer them to > > be in separate file. > > > > Having said that, with few changes in wordings in title and > > description, I think we can make it generic and can be used across few > products. > > The bindings seems quite similar. We can have two drivers loaded using two > compatible but binding description can be made same Noted. I can make the change i.e. add Keem Bay compatible string in lgm binding document and drop Keem Bay binding document. Rob and Vadivel, is there any objection? If not, I will proceed with v9 in the next one or two days. > > -- > ~Vinod
RE: [PATCH v8 1/3] phy: intel: Rename phy-intel to phy-intel-lgm
> -Original Message-
> From: Andy Shevchenko
> Sent: Tuesday, September 1, 2020 4:11 PM
> To: Wan Mohamad, Wan Ahmad Zainie
>
> Cc: kis...@ti.com; vk...@kernel.org; robh...@kernel.org; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org;
> vadivel.muruganx.ramuthe...@linux.intel.com;
> eswara.k...@linux.intel.com; Raja Subramanian, Lakshmi Bai
>
> Subject: Re: [PATCH v8 1/3] phy: intel: Rename phy-intel to phy-intel-lgm
>
> On Tue, Sep 01, 2020 at 12:41:59PM +0800, Wan Ahmad Zainie wrote:
> > Rename phy-intel-{combo,emmc}.c to phy-intel-lgm-{combo,emmc}.c to
> > make drivers/phy/intel directory more generic for future use.
> >
> > Signed-off-by: Wan Ahmad Zainie
> >
>
> > Reviewed-by: Ramuthevar Vadivel Murugan
> >
>
> This shall be one line.
I will fix this in v9.
>
> > ---
> > drivers/phy/intel/Kconfig | 10 +-
> > drivers/phy/intel/Makefile | 4 ++--
> > .../intel/{phy-intel-combo.c => phy-intel-lgm-combo.c} | 0
> > .../intel/{phy-intel-emmc.c => phy-intel-lgm-emmc.c} | 0
> > 4 files changed, 7 insertions(+), 7 deletions(-) rename
> > drivers/phy/intel/{phy-intel-combo.c => phy-intel-lgm-combo.c} (100%)
> > rename drivers/phy/intel/{phy-intel-emmc.c => phy-intel-lgm-emmc.c}
> > (100%)
> >
> > diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
> > index 7b47682a4e0e..db8586c3eed8 100644
> > --- a/drivers/phy/intel/Kconfig
> > +++ b/drivers/phy/intel/Kconfig
> > @@ -1,9 +1,9 @@
> > # SPDX-License-Identifier: GPL-2.0
> > #
> > -# Phy drivers for Intel Lightning Mountain(LGM) platform
> > +# Phy drivers for Intel platforms
> > #
> > -config PHY_INTEL_COMBO
> > - bool "Intel ComboPHY driver"
> > +config PHY_INTEL_LGM_COMBO
> > + bool "Intel Lightning Mountain ComboPHY driver"
> > depends on X86 || COMPILE_TEST
> > depends on OF && HAS_IOMEM
> > select MFD_SYSCON
> > @@ -16,8 +16,8 @@ config PHY_INTEL_COMBO
> > chipsets which provides PHYs for various controllers, EMAC,
> > SATA and PCIe.
> >
> > -config PHY_INTEL_EMMC
> > - tristate "Intel EMMC PHY driver"
> > +config PHY_INTEL_LGM_EMMC
> > + tristate "Intel Lightning Mountain EMMC PHY driver"
> > depends on X86 || COMPILE_TEST
> > select GENERIC_PHY
> > help
> > diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
> > index 233d530dadde..662385d0a366 100644
> > --- a/drivers/phy/intel/Makefile
> > +++ b/drivers/phy/intel/Makefile
> > @@ -1,3 +1,3 @@
> > # 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_INTEL_LGM_COMBO) += phy-intel-lgm-combo.o
> > +obj-$(CONFIG_PHY_INTEL_LGM_EMMC) += phy-intel-lgm-emmc.o
> > diff --git a/drivers/phy/intel/phy-intel-combo.c
> > b/drivers/phy/intel/phy-intel-lgm-combo.c
> > similarity index 100%
> > rename from drivers/phy/intel/phy-intel-combo.c
> > rename to drivers/phy/intel/phy-intel-lgm-combo.c
> > diff --git a/drivers/phy/intel/phy-intel-emmc.c
> > b/drivers/phy/intel/phy-intel-lgm-emmc.c
> > similarity index 100%
> > rename from drivers/phy/intel/phy-intel-emmc.c
> > rename to drivers/phy/intel/phy-intel-lgm-emmc.c
> > --
> > 2.17.1
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
RE: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings
> -Original Message-
> From: Vinod Koul
> Sent: Monday, August 31, 2020 5:10 PM
> To: Wan Mohamad, Wan Ahmad Zainie
>
> Cc: kis...@ti.com; robh...@kernel.org; Shevchenko, Andriy
> ; eswara.k...@linux.intel.com;
> vadivel.muruganx.ramuthe...@linux.intel.com; Raja Subramanian, Lakshmi
> Bai ; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org
> Subject: Re: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC
> PHY bindings
>
> On 21-08-20, 19:37, Wan Ahmad Zainie wrote:
> > Binding description for Intel Keem Bay eMMC PHY.
> >
> > Signed-off-by: Wan Ahmad Zainie
> >
> > Reviewed-by: Rob Herring
> > ---
> > .../bindings/phy/intel,keembay-emmc-phy.yaml | 44
> > +++
> > 1 file changed, 44 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/phy/intel,keembay-emmc-
> phy.yaml
> > b/Documentation/devicetree/bindings/phy/intel,keembay-emmc-
> phy.yaml
> > new file mode 100644
> > index ..4cbbd3887c13
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/intel,keembay-emmc-
> phy.yam
> > +++ l
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/phy/intel,keembay-emmc-
> phy.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> > +
> > +title: Intel Keem Bay eMMC PHY bindings
>
> This seems same as
> Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml, why
> not add a new compatible in lgm binding, or did I miss a difference?
AFAIK, LGM make use of syscon node, whilst KMB does not.
And LGM and KMB belongs to different SoC family. So, I prefer them to
be in separate file.
Having said that, with few changes in wordings in title and description,
I think we can make it generic and can be used across few products.
>
> > +
> > +maintainers:
> > + - Wan Ahmad Zainie
> > +
> > +properties:
> > + compatible:
> > +const: intel,keembay-emmc-phy
> > +
> > + reg:
> > +maxItems: 1
> > +
> > + clocks:
> > +maxItems: 1
> > +
> > + clock-names:
> > +items:
> > + - const: emmcclk
> > +
> > + "#phy-cells":
> > +const: 0
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - "#phy-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +phy@2029 {
> > + compatible = "intel,keembay-emmc-phy";
> > + reg = <0x2029 0x54>;
> > + clocks = <&emmc>;
> > + clock-names = "emmcclk";
> > + #phy-cells = <0>;
> > +};
> > --
> > 2.17.1
>
> --
> ~Vinod
RE: [PATCH v7 3/3] phy: intel: Add Keem Bay eMMC PHY support
Hi Vinod. Thanks for the review. > -Original Message- > From: Vinod Koul > Sent: Monday, August 31, 2020 5:20 PM > To: Wan Mohamad, Wan Ahmad Zainie > > Cc: kis...@ti.com; robh...@kernel.org; Shevchenko, Andriy > ; eswara.k...@linux.intel.com; > vadivel.muruganx.ramuthe...@linux.intel.com; Raja Subramanian, Lakshmi > Bai ; linux- > ker...@vger.kernel.org; devicet...@vger.kernel.org > Subject: Re: [PATCH v7 3/3] phy: intel: Add Keem Bay eMMC PHY support > > On 21-08-20, 19:37, Wan Ahmad Zainie wrote: > > > +/* 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 > > + > > +#define maskval(mask, val) (((val) << (ffs(mask) - 1)) & mask) > > Kernel has a macro do this for you, please use FIELD_PREP instead of I have updated to v8, to remove this macro and use FIELD_PREP. I also add changes based on Andy's comments. > > your own macro > -- > ~Vinod Best regards, Zainie
RE: [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support
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: [RESEND v5 2/2] phy: intel: Add Keem Bay eMMC PHY support
> -Original Message-
> From: Shevchenko, Andriy
> Sent: Wednesday, June 17, 2020 10:01 PM
> To: Wan Mohamad, Wan Ahmad Zainie
>
> Cc: kis...@ti.com; vk...@kernel.org; robh...@kernel.org; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org; Hunter, Adrian
>
> Subject: Re: [RESEND v5 2/2] phy: intel: Add Keem Bay eMMC PHY support
>
> On Wed, Jun 17, 2020 at 07:32:52AM +0800, Wan Ahmad Zainie wrote:
> > Add support for eMMC PHY on Intel Keem Bay SoC.
>
> ...
>
> > + ret = regmap_read_poll_timeout(priv->syscfg, PHY_STAT,
> > + dllrdy, IS_DLLRDY(dllrdy),
> > + 0, 50 * USEC_PER_MSEC);
> > + if (ret) {
> > + dev_err(&phy->dev, "dllrdy failed, ret=%d\n", ret);
> > + return ret;
> > + }
> > +
> > + return ret;
> > +}
>
> This has no changes.
>
> Are you sure the version is correct?
Ah. Sorry Andy. I misunderstood your earlier review comment.
You mean return inside the last if(...) should be discarded.
Kishon, should I send another version, or is it acceptable?
>
> --
> With Best Regards,
> Andy Shevchenko
>
RE: [PATCH v5 2/2] phy: intel: Add Keem Bay eMMC PHY support
> -Original Message-
> From: Shevchenko, Andriy
> Sent: Wednesday, June 17, 2020 12:44 AM
> To: Wan Mohamad, Wan Ahmad Zainie
>
> Cc: kis...@ti.com; vk...@kernel.org; robh...@kernel.org; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org; Hunter, Adrian
>
> Subject: Re: [PATCH v5 2/2] phy: intel: Add Keem Bay eMMC PHY support
>
> On Tue, Jun 16, 2020 at 10:38:18PM +0800, Wan Ahmad Zainie wrote:
> > Add support for eMMC PHY on Intel Keem Bay SoC.
>
> ...
>
> > + ret = regmap_read_poll_timeout(priv->syscfg, PHY_STAT,
> > + dllrdy, IS_DLLRDY(dllrdy),
> > + 0, 50 * USEC_PER_MSEC);
> > + if (ret) {
> > + dev_err(&phy->dev, "dllrdy failed, ret=%d\n", ret);
>
> > + return ret;
> > + }
> > +
> > + return ret;
>
> return ret;
>
> (Since it's only one minor issue, it's up to maintainers to decide if new
> version is needed)
This was addressed in v4, together with another 2 changes.
I was careless when modifying Kconfig in my older working branch.
I resend v5.
>
> --
> With Best Regards,
> Andy Shevchenko
>
RE: [PATCH v3 2/2] phy: intel: Add Keem Bay eMMC PHY support
> -Original Message-
> From: Shevchenko, Andriy
> Sent: Monday, June 8, 2020 6:08 PM
> To: Wan Mohamad, Wan Ahmad Zainie
>
> Cc: kis...@ti.com; vk...@kernel.org; robh...@kernel.org; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org; Hunter, Adrian
>
> Subject: Re: [PATCH v3 2/2] phy: intel: Add Keem Bay eMMC PHY support
>
> On Mon, Jun 08, 2020 at 04:15:01PM +0800, Wan Ahmad Zainie wrote:
> > Add support for eMMC PHY on Intel Keem Bay SoC.
>
> I think I commented on something already.
I don't recall any. May be on other similar patches.
I will wait for few days before sending next revision, if it is okay with you.
>
> ...
>
> > + if (ret) {
> > + dev_err(&phy->dev, "dllrdy failed, ret=%d\n", ret);
>
> > + return ret;
> > + }
> > +
> > + return 0;
>
> return ret;
I will change in next revision.
>
> ...
>
> > + if (IS_ERR(priv->emmcclk)) {
> > + dev_err(&phy->dev, "ERROR: getting emmcclk\n");
>
> > + return PTR_ERR(priv->emmcclk);
> > + }
> > +
> > + return 0;
>
> return PTR_ERR_OR_ZERO(...);
To clarify, I should replace the block above with,
return PTR_ERR_OR_ZERO(priv->emmcclk);
>
> ...
>
> > + priv->syscfg = devm_regmap_init_mmio(dev, base,
> > +&keembay_regmap_config);
>
> One line.
I will change in next revision.
>
> --
> With Best Regards,
> Andy Shevchenko
>
RE: [PATCH 1/2] dt-bindings: phy: intel: Add documentation for Keem Bay eMMC PHY
> -Original Message-
> From: Rob Herring
> Sent: Tuesday, March 31, 2020 4:23 AM
> To: Wan Mohamad, Wan Ahmad Zainie
>
> Cc: kis...@ti.com; mark.rutl...@arm.com; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH 1/2] dt-bindings: phy: intel: Add documentation for
> Keem Bay eMMC PHY
>
> On Mon, Mar 16, 2020 at 06:37:25PM +0800, Wan Ahmad Zainie wrote:
> > Document Intel Keem Bay eMMC PHY DT bindings.
> >
> > Signed-off-by: Wan Ahmad Zainie
>
> > ---
> > .../bindings/phy/intel,keembay-emmc-phy.yaml | 57
> +++
> > 1 file changed, 57 insertions(+)
> > create mode 100644
> Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/phy/intel,keembay-
> emmc-phy.yaml b/Documentation/devicetree/bindings/phy/intel,keembay-
> emmc-phy.yaml
> > new file mode 100644
> > index ..af1d62fc8323
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/intel,keembay-emmc-
> phy.yaml
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
>
> Dual license new bindings:
>
> (GPL-2.0-only OR BSD-2-Clause)
Will change in v2.
>
> > +# Copyright 2020 Intel Corporation
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/phy/intel,keembay-emmc-
> phy.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
> > +
> > +title: Intel Keem Bay eMMC PHY
> > +
> > +maintainers:
> > + - Wan Ahmad Zainie
> > +
> > +properties:
> > + compatible:
> > +enum:
> > + - intel,keembay-emmc-phy
> > +
> > + reg:
> > +maxItems: 1
> > +
> > + clocks:
> > +maxItems: 1
> > +
> > + clock-names:
> > +items:
> > + - const: emmcclk
> > +
> > + intel,syscon:
> > +$ref: '/schemas/types.yaml#/definitions/phandle'
>
> Make this binding a child of the syscon and get rid of this.
>
> > +description:
> > + A phandle to a syscon device used to access core/phy configuration
> > + registers.
> > +
> > + "#phy-cells":
> > +const: 0
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - intel,syscon
> > + - "#phy-cells"
> > +
> > +examples:
> > + - |
> > +mmc_phy_syscon: syscon@2029 {
> > + compatible = "simple-mfd", "syscon";
> > + reg = <0x0 0x2029 0x0 0x54>;
> > +};
> > +
> > +emmc_phy: mmc_phy@2029 {
>
> phy@...
Will change in v2.
>
> > + compatible = "intel,keembay-emmc-phy";
> > + reg = <0x0 0x2029 0x0 0x54>;
>
> Here you have overlapping register regions. Don't do that.
>
> Given they are the same size, why do you need the syscon at all?
In v2, the driver will use regmap_mmio. With that, can remove
intel,syscon. I will send out once reviewed internally.
>
> > + clocks = <&mmc>;
> > + clock-names = "emmcclk";
> > + intel,syscon = <&mmc_phy_syscon>;
> > + #phy-cells = <0>;
> > +};
> > --
> > 2.17.1
> >
