RE: Re: linux-next: Tree for Mar 31 (drivers/phy/marvell/phy-mvebu-cp110-utmi.o)
Hi, Randy, > -Original Message- > From: Randy Dunlap > Sent: Wednesday, March 31, 2021 18:28 > To: Stephen Rothwell ; Linux Next Mailing List n...@vger.kernel.org> > Cc: Linux Kernel Mailing List ; Kostya > Porotchkin ; net...@vger.kernel.org > Subject: [EXT] Re: linux-next: Tree for Mar 31 (drivers/phy/marvell/phy-mvebu- > cp110-utmi.o) > > > on i386: > > ld: drivers/phy/marvell/phy-mvebu-cp110-utmi.o: in function > `mvebu_cp110_utmi_phy_probe': > phy-mvebu-cp110-utmi.c:(.text+0x152): undefined reference to > `of_usb_get_dr_mode_by_phy' > [KP] This driver depends on ARCH_MVEBU (arm64). How it happens that it is included in i386 builds? Regards Kosta > > Full randconfig file is attached. > > -- > ~Randy > Reported-by: Randy Dunlap
RE: [PATCH v3 4/5] arch/arm64: dts: add support for Marvell CP110 UTMI driver
This one sent by mistake, please ignore it.
There is another v3 4/5 that has a correct name.
Sorry for the mess.
Kosta
> -Original Message-
> From: kos...@marvell.com
> Sent: Sunday, March 7, 2021 18:34
> To: linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Cc: lkund...@v3.sk; li...@armlinux.org.uk;
> sebastian.hesselba...@gmail.com; gregory.clem...@bootlin.com;
> and...@lunn.ch; robh...@kernel.org; vk...@kernel.org; kis...@ti.com;
> miquel.ray...@bootlin.com; m...@semihalf.com; j...@semihalf.com; Nadav
> Haklai ; Stefan Chulski ; Ben
> Peled ; Kostya Porotchkin
> Subject: [PATCH v3 4/5] arch/arm64: dts: add support for Marvell CP110 UTMI
> driver
>
> From: Konstantin Porotchkin
>
> Add support for Marvell CP110 UTMI PHY in a common DTSI
>
> Signed-off-by: Konstantin Porotchkin
> ---
> arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
> b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
> index 64179a372ecf..49f9d2cd8619 100644
> --- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
> @@ -285,6 +285,25 @@
> };
> };
>
> + CP11X_LABEL(utmi): utmi@58 {
> + compatible = "marvell,cp110-utmi-phy";
> + reg = <0x58 0x2000>;
> + marvell,system-controller =
> <_LABEL(syscon0)>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> +
> + CP11X_LABEL(utmi0): usb-phy@0 {
> + reg = <0>;
> + #phy-cells = <0>;
> + };
> +
> + CP11X_LABEL(utmi1): usb-phy@1 {
> + reg = <1>;
> + #phy-cells = <0>;
> + };
> + };
> +
> CP11X_LABEL(usb3_0): usb@50 {
> compatible = "marvell,armada-8k-xhci",
> "generic-xhci";
> --
> 2.17.1
RE: [EXT] Re: [PATCH v2 01/12] fix: arm64: dts: replace wrong regulator on ap emmc
> -- > On Wed, Feb 10, 2021 at 04:09:38PM +0200, kos...@marvell.com wrote: > > From: Konstantin Porotchkin > > > > Replace wrong regulator in AP0 eMMC definition on MacchiatoBIN board > > with 3.3V regulator. > > The MacchiatoBIN board has no 1.8V regulator connected to AP0 eMMC > > (ap0_sdhci0) interface. > > There seems to be some variability between Macchiatobin versions according > to the schematics. > > The VDDO_H supply is connected to the eMMC VCCQ pins, and is also > connected to the AP_VDDO_H pins. It is wired to the 1.8V regulator on rev 1.1 > schematics, but hard-wired to the 3.3V regulator on rev 1.3 schematics. > > This needs clarification from SolidRun before the patch can be accepted - was > VDDO_H ever wired to the 1.8V regulator on production hardware? > [KP] I will try to find a relevant contact in SolidRun for get this issue clarified. Kosta > -- > RMK's Patch system: https://urldefense.proofpoint.com/v2/url?u=https- > 3A__www.armlinux.org.uk_developer_patches_=DwIBAg=nKjWec2b6R0 > mOyPaz7xtfQ=- > N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o=7JAOlRdnL- > 42trjLrz_DOgZhvVuW8Skolb3bL-wJ6lo=9IB3Lxht5IQHTINpyLfX- > KC8AmqqHn0cCSSuQuTvfkE= > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
RE: [EXT] Re: [PATCH 02/11] dts: mvebu: Update A8K AP806 SDHCI settings
Hello, Russell,
I agree that this patch needs rework.
I will definitely do it and issue a new version.
> On Wed, Feb 03, 2021 at 02:50:45PM +0000, Kostya Porotchkin wrote:
> > [KP] So for older systems this "slow mode" parameter could be set on the
> board level.
> > When it is set in ap80x,dtsi file it downgrades all systems to HS-SDR52,
> > even
> if they support HS400 on AP side.
> > MacchiatoBIN AP eMMC is connected to 3.3v regulator and has "no-1-8-v"
> flag set, so it should remain in low speed anyway.
>
> Your reasoning does not make sense.
>
> The ap80x.dtsi file does not specify "marvell,xenon-phy-slow-mode".
> It is not specified at this level. It is already specified at board level.
[KP] it does. In current armada-ap80x.dtsi File this specification is on row
260:
ap_sdhci0: sdhci@6e {
compatible = "marvell,armada-ap806-sdhci";
reg = <0x6e 0x300>;
interrupts = ;
clock-names = "core";
clocks = <_clk 4>;
dma-coherent;
marvell,xenon-phy-slow-mode;
status = "disabled";
};
So I would like to remove this row.
> Given that Macchiatobin will still use slow mode, why remove the
> marvell,xenon-phy-slow-mode property from this file?
[KP] Agree, I will keep this property in Macchiatobin DTS file.
>
> Also, if you're upgrading ap80x.dtsi to use a bus-width of 8, why keep the
> bus-
> width specifier of 8 in the board files?
[KP] The bus width is updated in A8040 DB DTS. This board utilizes 8-bit
interface.
The armada-ap80x.dtsi file does not specifies the bus width since it is
board-specific.
>
> This patch just doesn't make sense, and your responses to our points seem to
> add to the confusion.
[KP] I am sorry about it. Hope my last response clarifies it.
Kosta
>
> --
> RMK's Patch system: https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__www.armlinux.org.uk_developer_patches_=DwIBAg=nKjWec2b6R0
> mOyPaz7xtfQ=-
> N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o=V27OOcgNqKN2
> WrlW2YFvHm_D_dXoP44wPd5zyOKvEBk=o3OrmStt1ZuXVNlYklTV_b1wY35
> NvPPrdLqwGgtxRZU=
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
RE: [EXT] Re: [PATCH 03/11] dts: mvebu: Add pin control definitions for SDIO interafce
> > > > + sdhci_pins: sdhi-pins {
> > >
> > > sdhi-pins ?
> > >
> > [KP] You mean to replace the underline with dash?
>
> I think he would like a c added in the correct place.
>
[KP] Ahh, now I see it. Thank you, Andrew!
Kosta
> Andrew
RE: [EXT] Re: [PATCH 02/11] dts: mvebu: Update A8K AP806 SDHCI settings
> -Original Message- > From: linux-arm-kernel On > Behalf Of Kostya Porotchkin > Sent: Wednesday, February 3, 2021 16:51 > To: Russell King - ARM Linux admin > Cc: devicet...@vger.kernel.org; Baruch Siach ; > and...@lunn.ch; j...@semihalf.com; gregory.clem...@bootlin.com; linux- > ker...@vger.kernel.org; Nadav Haklai ; > robh...@kernel.org; Stefan Chulski ; > m...@semihalf.com; Ben Peled ; linux-arm- > ker...@lists.infradead.org; sebastian.hesselba...@gmail.com > Subject: RE: [EXT] Re: [PATCH 02/11] dts: mvebu: Update A8K AP806 SDHCI > settings [KP] > > > > Hi Konstantin, > > > > > > > > On Wed, Feb 03 2021, kos...@marvell.com wrote: > > > > > From: Konstantin Porotchkin > > > > > > > > > > Update the settings for AP806 SDHCI interface according to > > > > > latest Xenon drivers changes. > > > > > - no need to select the PHY slow mode anymore > > > > > > > > Why? Has anything changed since the introduction of > > > > marvell,xenon-phy-slow- mode? > > > [KP] AP806 B0, AP807 and later do not need the "slow mode" set by > > > the > > default. > > > The HWE-7296210 errata is not applicable to these components and > > > they are able to run AP SDHCI in HS400 8-bit mode. > > > > So what about all those people, such as me, who have A0 silicon on > > their Macchiatobin boards? > > > > You can't just go around removing DT properties like this. > > > [KP] So for older systems this "slow mode" parameter could be set on the > board level. > When it is set in ap80x,dtsi file it downgrades all systems to HS-SDR52, even > if > they support HS400 on AP side. > MacchiatoBIN AP eMMC is connected to 3.3v regulator and has "no-1-8-v" flag > set, so it should remain in low speed anyway. [KP] I also forgot to mention this code piece in Xenon driver: /* Disable HS200 on Armada AP806 */ if (priv->hw_version == XENON_AP806) host->quirks2 |= SDHCI_QUIRK2_BROKEN_HS200; > > > -- > > RMK's Patch system: https://urldefense.proofpoint.com/v2/url?u=https- > > > 3A__www.armlinux.org.uk_developer_patches_=DwIBAg=nKjWec2b6R0 > > mOyPaz7xtfQ=- > > > N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o=yMC9YPQXZUm > > > QPwlD7KCTVoVTPXCTQwTXD2yVsAo6sxA=OuBO2QArzHvV4k_vsNZdmSoDX > > rL4Q_voTqxrlYU6KKE= > > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > https://urldefense.proofpoint.com/v2/url?u=http- > 3A__lists.infradead.org_mailman_listinfo_linux-2Darm- > 2Dkernel=DwICAg=nKjWec2b6R0mOyPaz7xtfQ=- > N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o=8f- > 1fnISJVHCS4gZTeFgRPXGPiwBevUsFbmYDBmkRWM=RBlEEUXG0sOmZHsQ > Omurf018V8kSE_IMZR7bDLVJ0FA=
RE: [EXT] Re: [PATCH 02/11] dts: mvebu: Update A8K AP806 SDHCI settings
> -Original Message- > From: Russell King - ARM Linux admin > Sent: Wednesday, February 3, 2021 16:39 > To: Kostya Porotchkin > Cc: Baruch Siach ; linux-kernel@vger.kernel.org; > devicet...@vger.kernel.org; and...@lunn.ch; j...@semihalf.com; > gregory.clem...@bootlin.com; Nadav Haklai ; > robh...@kernel.org; Stefan Chulski ; > m...@semihalf.com; Ben Peled ; > sebastian.hesselba...@gmail.com; linux-arm-ker...@lists.infradead.org > Subject: Re: [EXT] Re: [PATCH 02/11] dts: mvebu: Update A8K AP806 SDHCI > settings > > On Wed, Feb 03, 2021 at 02:37:22PM +, Kostya Porotchkin wrote: > > Hi, Baruch, > > > > > -Original Message- > > > From: Baruch Siach > > > Sent: Wednesday, February 3, 2021 15:59 > > > To: Kostya Porotchkin > > > Cc: linux-kernel@vger.kernel.org; devicet...@vger.kernel.org; > > > and...@lunn.ch; j...@semihalf.com; gregory.clem...@bootlin.com; > > > li...@armlinux.org.uk; Nadav Haklai ; > > > robh...@kernel.org; Stefan Chulski ; > > > m...@semihalf.com; Ben Peled ; > > > sebastian.hesselba...@gmail.com; > > > linux-arm-ker...@lists.infradead.org > > > Subject: [EXT] Re: [PATCH 02/11] dts: mvebu: Update A8K AP806 SDHCI > > > settings > > > > > > External Email > > > > > > > > > -- > > > Hi Konstantin, > > > > > > On Wed, Feb 03 2021, kos...@marvell.com wrote: > > > > From: Konstantin Porotchkin > > > > > > > > Update the settings for AP806 SDHCI interface according to latest > > > > Xenon drivers changes. > > > > - no need to select the PHY slow mode anymore > > > > > > Why? Has anything changed since the introduction of > > > marvell,xenon-phy-slow- mode? > > [KP] AP806 B0, AP807 and later do not need the "slow mode" set by the > default. > > The HWE-7296210 errata is not applicable to these components and they > > are able to run AP SDHCI in HS400 8-bit mode. > > So what about all those people, such as me, who have A0 silicon on their > Macchiatobin boards? > > You can't just go around removing DT properties like this. > [KP] So for older systems this "slow mode" parameter could be set on the board level. When it is set in ap80x,dtsi file it downgrades all systems to HS-SDR52, even if they support HS400 on AP side. MacchiatoBIN AP eMMC is connected to 3.3v regulator and has "no-1-8-v" flag set, so it should remain in low speed anyway. > -- > RMK's Patch system: https://urldefense.proofpoint.com/v2/url?u=https- > 3A__www.armlinux.org.uk_developer_patches_=DwIBAg=nKjWec2b6R0 > mOyPaz7xtfQ=- > N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o=yMC9YPQXZUm > QPwlD7KCTVoVTPXCTQwTXD2yVsAo6sxA=OuBO2QArzHvV4k_vsNZdmSoDX > rL4Q_voTqxrlYU6KKE= > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
RE: [EXT] Re: [PATCH 03/11] dts: mvebu: Add pin control definitions for SDIO interafce
Hu, Russel,
> -Original Message-
> From: Russell King - ARM Linux admin
> Sent: Wednesday, February 3, 2021 16:28
> To: Kostya Porotchkin
> Cc: linux-kernel@vger.kernel.org; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; robh...@kernel.org;
> sebastian.hesselba...@gmail.com; gregory.clem...@bootlin.com;
> and...@lunn.ch; m...@semihalf.com; j...@semihalf.com; Nadav Haklai
> ; Stefan Chulski ; Ben Peled
>
> Subject: [EXT] Re: [PATCH 03/11] dts: mvebu: Add pin control definitions for
> SDIO interafce
>
> External Email
>
> --
> On Wed, Feb 03, 2021 at 03:31:30PM +0200, kos...@marvell.com wrote:
> > From: Konstantin Porotchkin
> >
> > Add SDIO mode pin control configration for CP0 on A8K DB.
> >
> > Signed-off-by: Konstantin Porotchkin
> > ---
> > arch/arm64/boot/dts/marvell/armada-70x0.dtsi | 6 ++
> > arch/arm64/boot/dts/marvell/armada-80x0.dtsi | 6 ++
> > 2 files changed, 12 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/marvell/armada-70x0.dtsi
> > b/arch/arm64/boot/dts/marvell/armada-70x0.dtsi
> > index 293403a1a333..179218774ba9 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-70x0.dtsi
> > +++ b/arch/arm64/boot/dts/marvell/armada-70x0.dtsi
> > @@ -47,6 +47,12 @@
> > cp0_pinctrl: pinctrl {
> > compatible = "marvell,armada-7k-pinctrl";
> >
> > + sdhci_pins: sdhi-pins {
>
> sdhi-pins ?
>
[KP] You mean to replace the underline with dash?
Will do it in the next version, no problem.
> > + marvell,pins = "mpp56", "mpp57", "mpp58",
> > + "mpp59", "mpp60", "mpp61", "mpp62";
> > + marvell,function = "sdio";
> > + };
> > +
> > nand_pins: nand-pins {
> > marvell,pins =
> > "mpp15", "mpp16", "mpp17", "mpp18", diff --git
> > a/arch/arm64/boot/dts/marvell/armada-80x0.dtsi
> > b/arch/arm64/boot/dts/marvell/armada-80x0.dtsi
> > index ee67c70bf02e..64100ae204da 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-80x0.dtsi
> > +++ b/arch/arm64/boot/dts/marvell/armada-80x0.dtsi
> > @@ -70,6 +70,12 @@
> > _syscon0 {
> > cp0_pinctrl: pinctrl {
> > compatible = "marvell,armada-8k-cpm-pinctrl";
> > +
> > + sdhci_pins: sdhi-pins {
>
> sdhi-pins ?
>
> > + marvell,pins = "mpp56", "mpp57", "mpp58",
> > + "mpp59", "mpp60", "mpp61", "mpp62";
> > + marvell,function = "sdio";
> > + };
> > };
> > };
> >
> > --
> > 2.17.1
> >
> >
>
> --
> RMK's Patch system: https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__www.armlinux.org.uk_developer_patches_=DwIBAg=nKjWec2b6R0
> mOyPaz7xtfQ=-
> N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o=wUA0mnqioCngi
> HLZcn2iOBuiWLQtawWb1yfozx_80C4=_yolpLSRiJi4CnA-
> iEzpbF5r77VBdLcM6pouXxTdupk=
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
RE: [EXT] Re: [PATCH 03/11] dts: mvebu: Add pin control definitions for SDIO interafce
> -Original Message-
> From: Baruch Siach
> Sent: Wednesday, February 3, 2021 16:01
> To: Kostya Porotchkin
> Cc: linux-kernel@vger.kernel.org; devicet...@vger.kernel.org;
> and...@lunn.ch; j...@semihalf.com; gregory.clem...@bootlin.com;
> li...@armlinux.org.uk; Nadav Haklai ;
> robh...@kernel.org; Stefan Chulski ;
> m...@semihalf.com; Ben Peled ;
> sebastian.hesselba...@gmail.com; linux-arm-ker...@lists.infradead.org
> Subject: [EXT] Re: [PATCH 03/11] dts: mvebu: Add pin control definitions for
> SDIO interafce
>
> External Email
>
> --
> Hi Konstantin,
>
> On Wed, Feb 03 2021, kos...@marvell.com wrote:
> > From: Konstantin Porotchkin
> >
> > Add SDIO mode pin control configration for CP0 on A8K DB.
>
> This patch does not touch the A8K DB device-tree file.
>
[KP] Right, it changes the SoC DTSI. I missed it when ported the patch.
Will fix in the next version
Kosta
> baruch
>
> >
> > Signed-off-by: Konstantin Porotchkin
> > ---
> > arch/arm64/boot/dts/marvell/armada-70x0.dtsi | 6 ++
> > arch/arm64/boot/dts/marvell/armada-80x0.dtsi | 6 ++
> > 2 files changed, 12 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/marvell/armada-70x0.dtsi
> > b/arch/arm64/boot/dts/marvell/armada-70x0.dtsi
> > index 293403a1a333..179218774ba9 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-70x0.dtsi
> > +++ b/arch/arm64/boot/dts/marvell/armada-70x0.dtsi
> > @@ -47,6 +47,12 @@
> > cp0_pinctrl: pinctrl {
> > compatible = "marvell,armada-7k-pinctrl";
> >
> > + sdhci_pins: sdhi-pins {
> > + marvell,pins = "mpp56", "mpp57", "mpp58",
> > + "mpp59", "mpp60", "mpp61", "mpp62";
> > + marvell,function = "sdio";
> > + };
> > +
> > nand_pins: nand-pins {
> > marvell,pins =
> > "mpp15", "mpp16", "mpp17", "mpp18", diff --git
> > a/arch/arm64/boot/dts/marvell/armada-80x0.dtsi
> > b/arch/arm64/boot/dts/marvell/armada-80x0.dtsi
> > index ee67c70bf02e..64100ae204da 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-80x0.dtsi
> > +++ b/arch/arm64/boot/dts/marvell/armada-80x0.dtsi
> > @@ -70,6 +70,12 @@
> > _syscon0 {
> > cp0_pinctrl: pinctrl {
> > compatible = "marvell,armada-8k-cpm-pinctrl";
> > +
> > + sdhci_pins: sdhi-pins {
> > + marvell,pins = "mpp56", "mpp57", "mpp58",
> > + "mpp59", "mpp60", "mpp61", "mpp62";
> > + marvell,function = "sdio";
> > + };
> > };
> > };
>
>
> --
> ~. .~ Tk Open Systems
> =}ooO--U--Ooo{=
>- bar...@tkos.co.il - tel: +972.52.368.4656,
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__www.tkos.co.il=DwIBAg=nKjWec2b6R0mOyPaz7xtfQ=-
> N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o=8Kz0ddezxxG41
> 9tiQOva_I9GUi6QZw9Pa6tRxYugqQw=Ky8dBlut-daLt2-
> 0j3BIwiBEBAVzKi8e9oJetRIzuPA= -
RE: [EXT] Re: [PATCH 02/11] dts: mvebu: Update A8K AP806 SDHCI settings
Hi, Baruch,
> -Original Message-
> From: Baruch Siach
> Sent: Wednesday, February 3, 2021 15:59
> To: Kostya Porotchkin
> Cc: linux-kernel@vger.kernel.org; devicet...@vger.kernel.org;
> and...@lunn.ch; j...@semihalf.com; gregory.clem...@bootlin.com;
> li...@armlinux.org.uk; Nadav Haklai ;
> robh...@kernel.org; Stefan Chulski ;
> m...@semihalf.com; Ben Peled ;
> sebastian.hesselba...@gmail.com; linux-arm-ker...@lists.infradead.org
> Subject: [EXT] Re: [PATCH 02/11] dts: mvebu: Update A8K AP806 SDHCI
> settings
>
> External Email
>
> --
> Hi Konstantin,
>
> On Wed, Feb 03 2021, kos...@marvell.com wrote:
> > From: Konstantin Porotchkin
> >
> > Update the settings for AP806 SDHCI interface according to latest
> > Xenon drivers changes.
> > - no need to select the PHY slow mode anymore
>
> Why? Has anything changed since the introduction of marvell,xenon-phy-slow-
> mode?
[KP] AP806 B0, AP807 and later do not need the "slow mode" set by the default.
The HWE-7296210 errata is not applicable to these components and they are able
to run AP SDHCI in HS400 8-bit mode.
Kosta
>
> baruch
>
> > -* Not stable in HS modes - phy needs "more calibration", so add
> > -* the "slow-mode" and disable SDR104, SDR50 and DDR50 modes.
> > -*/
> > - marvell,xenon-phy-slow-mode;
> > no-1-8-v;
> > no-sd;
> > no-sdio;
> > diff --git a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
> > b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
> > index 12e477f1aeb9..edd6131a0587 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
> > +++ b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
> > @@ -257,7 +257,6 @@
> > clock-names = "core";
> > clocks = <_clk 4>;
> > dma-coherent;
> > - marvell,xenon-phy-slow-mode;
> > status = "disabled";
> > };
>
>
> --
> ~. .~ Tk Open Systems
> =}ooO--U--Ooo{=
>- bar...@tkos.co.il - tel: +972.52.368.4656,
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__www.tkos.co.il=DwIBAg=nKjWec2b6R0mOyPaz7xtfQ=-
> N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o=F567knLB6kbyr-
> BZRqLFLJgXHENu41578OHxsKQQ-
> sw=eXEBLtBC3CwWIF9XFbHrgSgASz4aMgHi5W1uuXTgdQ4= -
RE: [PATCH 2/4] devicetree/bindings: add support for CP110 UTMI driver
Hi, Lubomir,
Thank you for your review!
> On Wed, Jan 27, 2021 at 01:27:17PM +0200, kos...@marvell.com wrote:
> > From: Konstantin Porotchkin
> >
> > Add DTS binding for Marvell CP110 UTMI driver
> >
> > Signed-off-by: Konstantin Porotchkin
>
> Any chance you could convert the document to YAML so that it could be used
> for automatic validation?
>
[KP] I believe it is possible, but probably should be done by a separate patch.
Here I am extending the existing documentation.
> > ---
> > Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt | 69
> > ++--
> > 1 file changed, 63 insertions(+), 6 deletions(-)
...
> > Required Properties:
> >
> > - compatible: Should be one of:
> > * "marvell,a3700-utmi-host-phy" for the PHY connected to
> > - the USB2 host-only controller.
> > + the USB2 host-only controller (for Armada3700 only).
> > * "marvell,a3700-utmi-otg-phy" for the PHY connected to
> > - the USB3 and USB2 OTG capable controller.
> > + the USB3 and USB2 OTG capable controller (for Armada3700 only.
> > + * "marvell,cp110-utmi-phy" (for Armada 7k/8k or CN913x only).
> > - reg: PHY IP register range.
> > - marvell,usb-misc-reg: handle on the "USB miscellaneous registers" shared
> > region covering registers related to both the host
> > - controller and the PHY.
> > -- #phy-cells: Standard property (Documentation: phy-bindings.txt) Should be
> 0.
> > + controller and the PHY (for Armada3700 only).
> > +- marvell,system-controller: should contain a phandle to the system
> > +controller node (for Armada 7k/8k or CN913x only)
>
> I guess this is okay, but referring to a syscon is done in a multitude ways
> across
> various other bindings; with the most popular being just:
>
> syscon = <>;
>
> Perhaps consider doing that?
[KP] I was not sure that I can use a generic name inside the PHY entry
if it is not defined as part of the generic PHY properties.
I just did not see a good example of such in PHY bindings documentation.
If it is legal, I can change this entry name to just "syscon".
>
> > +- #phy-cells: Standard property (Documentation: phy-bindings.txt.
> > + Should be 0 (for Armada3700 only).
> > +
> > +
> > +Required properties (child nodes, for Armada 7k/8k/CN913x only):
> > +
> > +- reg: UTMI PHY port ID (0 or 1).
> > +- #phy-cells : Should be 0.
> > +
> > +
> > +Optional Properties (child nodes, for Armada 7k/8k/CN913x only)::
> >
> > +- marvell,cp110-utmi-device-mode: request the driver to connect the UTMI
> PHY
> > + port to USB device controller.
>
> Do you need a separate property for this? Could the driver look at "dr_mode"
> property of the USB controller node to see if it's supposed to be in
> device/peripheral mode?
[KP] Yes, it seems I missed this option. I will try to change the code to
support it in version 2.
>
> > Example:
> >
> > +Armada3700
> > usb2_utmi_host_phy: phy@5f000 {
> > compatible = "marvell,armada-3700-utmi-host-phy";
> > reg = <0x5f000 0x800>;
> > @@ -36,3 +67,29 @@ Example:
> > --
> > 2.17.1
> >
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__lists.infradead.org_mailman_listinfo_linux-2Darm-
> 2Dkernel=DwICAg=nKjWec2b6R0mOyPaz7xtfQ=-
> N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o=_ZOAKZShBT3Qj
> uT3RZIld2HoLnlvv6gkbHW9gSvEfI4=ggCBpvhDLJ8M6-
> Q41qbt8GRxryUo_mHxLMkUl8Ao5mA=
RE: [PATCH 1/4] drivers: phy: add support for Armada CP110 UTMI PHY
Hello, Lubomir,
Thank you for your review!
> > +static void mvebu_cp110_utmi_port_setup(struct mvebu_cp110_utmi_port
> > +*port) {
> > + u32 reg;
> > +
> > + /*
> > +* Setup PLL. 40MHz clock used to be the default, being 25MHz now.
> > +* See the functional specification for details.
>
> I tried to, couldn't find it. Perhaps you could elaborate more here, or
> include a
> link in the header.
[KP] This is the frequency of a quartz resonator connected to
REFCLK_XIN/REFCLK_XOUT SoC pins.
The default crystal frequency used now at all Marvell reference platforms is
25MHz.
The default out of reset state register values are however have settings for
40MHz crystal used at the time of RTL code freeze.
I will mention this fact in the second patch version.
>
> > + /* PLL Power down if all UTMI PHYs are down */
> > + regmap_clear_bits(utmi->syscon, SYSCON_USB_CFG_REG,
> > +USB_CFG_PLL_MASK);
> > +
> > + return 0;
> > +}
> > +
> > +static int mvebu_cp110_utmi_phy_power_on(struct phy *phy) {
> > + struct mvebu_cp110_utmi_port *port = phy_get_drvdata(phy);
> > + struct mvebu_cp110_utmi *utmi = port->priv;
> > + struct device *dev = >dev;
> > + int ret;
> > + u32 reg;
> > +
> > + /* It is necessary to power off UTMI before configuration */
> > + ret = mvebu_cp110_utmi_phy_power_off(phy);
>
> mvebu_cp110_utmi_phy_power_off() also sometimes, but not always, shuts
> down the PLL. Is that necessary? I guess all you care about is the bit in
> UTMI_PHY_CFG_PU_MASK?
[KP] Not sure I fully understand the question. Both UTMI PHYs are using the
same dedicated PLL source.
I am trying to save the power to shutting down this PLL when both PHY ports are
down.
>
> > +
> > + ret = of_property_read_u32(child, "reg", );
> > + if (ret < 0) {
> > + dev_err(dev, "missing 'reg' property (%d)\n", ret);
>
> A nit: the property is not necessarily missing -- it could be malformed (with
> ret
> of -ENODATA or -EOVERFLOW).
>
> Also, perhaps you want to log the name of node that has problems ("%pOF",
> child); also in the error messages below.
[KP] OK, will do it in the second patch version.
>
> > + continue;
> > + }
> > +
> > + if (val >= UTMI_PHY_PORTS) {
> > + dev_err(dev, "invalid 'reg' property\n");
> > + continue;
> > + }
>
> Perhaps you could just squelch those two warnings together:
>
>
> if (ret || val >= UTMI_PHY_PORTS) {
> dev_err(dev, "invalid 'reg' property on %pOF\n", child);
> continue;
> }
>
[KP] Yes, it will definitely be better, thank you.
> > --
> > 2.17.1
> >
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__lists.infradead.org_mailman_listinfo_linux-2Darm-
> 2Dkernel=DwICAg=nKjWec2b6R0mOyPaz7xtfQ=-
> N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o=YFT4I6c34R7m8
> 6d4PlkWJFgC3qPXYkofB_VPJDgQVSA=Hf5wCKTcwKa3Mx-
> L7ZXEX4MPsfaMtPDu87RltnvXa90=
