| Message ID | cover.b921b010b6d6bde1c11e69551ae38f3b2818645b.1536916714.git-series.quentin.schulz@bootlin.com |
|---|---|
| Headers | show |
| Series | add support for VSC8584 and VSC8574 Microsemi quad-port PHYs | expand |
> Most of the init sequence of a PHY of the package is common to all PHYs > in the package, thus we use the SMI broadcast feature which enables us > to propagate a write in one register of one PHY to all PHYs in the > package. Hi Quinten Could you say a bit more about the broadcast. Does the SMI broadcast go to all PHY everywhere on an MDIO bus, or only all PHYs within one package? I'm just thinking about the case you need two of these packages to cover 8 switch ports. Thanks Andrew
Hi Andrew, On Fri, Sep 14, 2018 at 03:18:46PM +0200, Andrew Lunn wrote: > > Most of the init sequence of a PHY of the package is common to all PHYs > > in the package, thus we use the SMI broadcast feature which enables us > > to propagate a write in one register of one PHY to all PHYs in the > > package. > > Hi Quinten > > Could you say a bit more about the broadcast. Does the SMI broadcast > go to all PHY everywhere on an MDIO bus, or only all PHYs within one > package? I'm just thinking about the case you need two of these > packages to cover 8 switch ports. > Ah sorry, that wasn't very explicit. That's a feature on the PHY side so my wildest guess is that it wouldn't impact any other PHY outside of this package. Affecting any other PHY on the bus is counter-intuitive to me but I'll ask the HW engineers for confirmation. Thanks, Quentin
Hi, On 14/09/2018 11:44:26+0200, Quentin Schulz wrote: > In order to use GPIO4 as a GPIO, we need to mux it in this mode so let's > declare a new pinctrl DT node for it. > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > --- > arch/mips/boot/dts/mscc/ocelot.dtsi | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/mips/boot/dts/mscc/ocelot.dtsi b/arch/mips/boot/dts/mscc/ocelot.dtsi > index 8ce317c..b5c4c74 100644 > --- a/arch/mips/boot/dts/mscc/ocelot.dtsi > +++ b/arch/mips/boot/dts/mscc/ocelot.dtsi > @@ -182,6 +182,11 @@ > interrupts = <13>; > #interrupt-cells = <2>; > > + gpio4: gpio4 { > + pins = "GPIO_4"; > + function = "gpio"; > + }; > + For a GPIO, I would do that in the board dts because it is not used directly in the dtsi. > uart_pins: uart-pins { > pins = "GPIO_6", "GPIO_7"; > function = "uart"; > -- > git-series 0.9.1
On 14/09/2018 11:44:27+0200, Quentin Schulz wrote: > The Ocelot PCB120 evaluation board is different from the PCB123 in that > it has 4 external VSC8584 (or VSC8574) PHYs. > > It uses the SoC's second MDIO bus for external PHYs which have a > reversed address on the bus (i.e. PHY4 is on address 3, PHY5 is on > address 2, PHY6 on 1 and PHY7 on 0). > > Here is how the PHYs are connected to the switch ports: > port 0: phy0 (internal) > port 1: phy1 (internal) > port 2: phy2 (internal) > port 3: phy3 (internal) > port 4: phy7 > port 5: phy4 > port 6: phy6 > port 9: phy5 > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > arch/mips/boot/dts/mscc/Makefile | 2 +- > arch/mips/boot/dts/mscc/ocelot_pcb120.dts | 100 +++++++++++++++++++++++- > 2 files changed, 101 insertions(+), 1 deletion(-) > create mode 100644 arch/mips/boot/dts/mscc/ocelot_pcb120.dts > > diff --git a/arch/mips/boot/dts/mscc/Makefile b/arch/mips/boot/dts/mscc/Makefile > index 9a9bb7e..ec6f5b2 100644 > --- a/arch/mips/boot/dts/mscc/Makefile > +++ b/arch/mips/boot/dts/mscc/Makefile > @@ -1,3 +1,3 @@ > -dtb-$(CONFIG_MSCC_OCELOT) += ocelot_pcb123.dtb > +dtb-$(CONFIG_MSCC_OCELOT) += ocelot_pcb123.dtb ocelot_pcb120.dtb > > obj-$(CONFIG_BUILTIN_DTB) += $(addsuffix .o, $(dtb-y)) > diff --git a/arch/mips/boot/dts/mscc/ocelot_pcb120.dts b/arch/mips/boot/dts/mscc/ocelot_pcb120.dts > new file mode 100644 > index 0000000..8eb03a5 > --- /dev/null > +++ b/arch/mips/boot/dts/mscc/ocelot_pcb120.dts > @@ -0,0 +1,100 @@ > +// SPDX-License-Identifier: (GPL-2.0 OR MIT) > +/* Copyright (c) 2017 Microsemi Corporation */ > + > +/dts-v1/; > + > +#include <dt-bindings/interrupt-controller/irq.h> > +#include <dt-bindings/phy/phy-ocelot-serdes.h> > +#include "ocelot.dtsi" > + > +/ { > + compatible = "mscc,ocelot-pcb120", "mscc,ocelot"; > + > + chosen { > + stdout-path = "serial0:115200n8"; > + }; > + > + memory@0 { > + device_type = "memory"; > + reg = <0x0 0x0e000000>; > + }; > +}; > + > +&mdio0 { > + status = "okay"; > +}; > + > +&mdio1 { > + status = "okay"; > + pinctrl-names = "default"; > + pinctrl-0 = <&miim1>, <&gpio4>; > + > + phy7: ethernet-phy@0 { > + reg = <0>; > + interrupts = <4 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-parent = <&gpio>; > + }; > + phy6: ethernet-phy@1 { > + reg = <1>; > + interrupts = <4 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-parent = <&gpio>; > + }; > + phy5: ethernet-phy@2 { > + reg = <2>; > + interrupts = <4 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-parent = <&gpio>; > + }; > + phy4: ethernet-phy@3 { > + reg = <3>; > + interrupts = <4 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-parent = <&gpio>; > + }; > +}; > + > +&port0 { > + phy-handle = <&phy0>; > +}; > + > +&port1 { > + phy-handle = <&phy1>; > +}; > + > +&port2 { > + phy-handle = <&phy2>; > +}; > + > +&port3 { > + phy-handle = <&phy3>; > +}; > + > +&port4 { > + phy-handle = <&phy7>; > + phy-mode = "sgmii"; > + phys = <&serdes 4 SERDES1G_2>; > +}; > + > +&port5 { > + phy-handle = <&phy4>; > + phy-mode = "sgmii"; > + phys = <&serdes 5 SERDES1G_5>; > +}; > + > +&port6 { > + phy-handle = <&phy6>; > + phy-mode = "sgmii"; > + phys = <&serdes 6 SERDES1G_3>; > +}; > + > +&port9 { > + phy-handle = <&phy5>; > + phy-mode = "sgmii"; > + phys = <&serdes 9 SERDES1G_4>; > +}; > + > +&uart0 { > + status = "okay"; > +}; > + > +&uart2 { > + status = "okay"; > +}; > -- > git-series 0.9.1
On 14/09/2018 11:44:28+0200, Quentin Schulz wrote: > PCB120 and PCB123 are both development boards based on Microsemi Ocelot > so let's use the same fitImage for both. > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > arch/mips/generic/Kconfig | 6 +-- > arch/mips/generic/Platform | 2 +- > arch/mips/generic/board-ocelot.its.S | 40 ++++++++++++++++++++++- > arch/mips/generic/board-ocelot_pcb123.its.S | 23 +------------- > 4 files changed, 44 insertions(+), 27 deletions(-) > create mode 100644 arch/mips/generic/board-ocelot.its.S > delete mode 100644 arch/mips/generic/board-ocelot_pcb123.its.S > > diff --git a/arch/mips/generic/Kconfig b/arch/mips/generic/Kconfig > index 08e33c6..fd60198 100644 > --- a/arch/mips/generic/Kconfig > +++ b/arch/mips/generic/Kconfig > @@ -65,11 +65,11 @@ config FIT_IMAGE_FDT_XILFPGA > Enable this to include the FDT for the MIPSfpga platform > from Imagination Technologies in the FIT kernel image. > > -config FIT_IMAGE_FDT_OCELOT_PCB123 > - bool "Include FDT for Microsemi Ocelot PCB123" > +config FIT_IMAGE_FDT_OCELOT > + bool "Include FDT for Microsemi Ocelot development platforms" > select MSCC_OCELOT > help > - Enable this to include the FDT for the Ocelot PCB123 platform > + Enable this to include the FDT for the Ocelot development platforms > from Microsemi in the FIT kernel image. > This requires u-boot on the platform. > > diff --git a/arch/mips/generic/Platform b/arch/mips/generic/Platform > index 879cb80..eaa19d1 100644 > --- a/arch/mips/generic/Platform > +++ b/arch/mips/generic/Platform > @@ -16,5 +16,5 @@ all-$(CONFIG_MIPS_GENERIC) := vmlinux.gz.itb > its-y := vmlinux.its.S > its-$(CONFIG_FIT_IMAGE_FDT_BOSTON) += board-boston.its.S > its-$(CONFIG_FIT_IMAGE_FDT_NI169445) += board-ni169445.its.S > -its-$(CONFIG_FIT_IMAGE_FDT_OCELOT_PCB123) += board-ocelot_pcb123.its.S > +its-$(CONFIG_FIT_IMAGE_FDT_OCELOT) += board-ocelot.its.S > its-$(CONFIG_FIT_IMAGE_FDT_XILFPGA) += board-xilfpga.its.S > diff --git a/arch/mips/generic/board-ocelot.its.S b/arch/mips/generic/board-ocelot.its.S > new file mode 100644 > index 0000000..3da2398 > --- /dev/null > +++ b/arch/mips/generic/board-ocelot.its.S > @@ -0,0 +1,40 @@ > +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */ > +/ { > + images { > + fdt@ocelot_pcb123 { > + description = "MSCC Ocelot PCB123 Device Tree"; > + data = /incbin/("boot/dts/mscc/ocelot_pcb123.dtb"); > + type = "flat_dt"; > + arch = "mips"; > + compression = "none"; > + hash@0 { > + algo = "sha1"; > + }; > + }; > + > + fdt@ocelot_pcb120 { > + description = "MSCC Ocelot PCB120 Device Tree"; > + data = /incbin/("boot/dts/mscc/ocelot_pcb120.dtb"); > + type = "flat_dt"; > + arch = "mips"; > + compression = "none"; > + hash@0 { > + algo = "sha1"; > + }; > + }; > + }; > + > + configurations { > + conf@ocelot_pcb123 { > + description = "Ocelot Linux kernel"; > + kernel = "kernel@0"; > + fdt = "fdt@ocelot_pcb123"; > + }; > + > + conf@ocelot_pcb120 { > + description = "Ocelot Linux kernel"; > + kernel = "kernel@0"; > + fdt = "fdt@ocelot_pcb120"; > + }; > + }; > +}; > diff --git a/arch/mips/generic/board-ocelot_pcb123.its.S b/arch/mips/generic/board-ocelot_pcb123.its.S > deleted file mode 100644 > index 5a7d5e1..0000000 > --- a/arch/mips/generic/board-ocelot_pcb123.its.S > +++ /dev/null > @@ -1,23 +0,0 @@ > -/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */ > -/ { > - images { > - fdt@ocelot_pcb123 { > - description = "MSCC Ocelot PCB123 Device Tree"; > - data = /incbin/("boot/dts/mscc/ocelot_pcb123.dtb"); > - type = "flat_dt"; > - arch = "mips"; > - compression = "none"; > - hash@0 { > - algo = "sha1"; > - }; > - }; > - }; > - > - configurations { > - conf@ocelot_pcb123 { > - description = "Ocelot Linux kernel"; > - kernel = "kernel@0"; > - fdt = "fdt@ocelot_pcb123"; > - }; > - }; > -}; > -- > git-series 0.9.1
Hi Alexandre, On Fri, Sep 14, 2018 at 04:54:46PM +0200, Alexandre Belloni wrote: > Hi, > > On 14/09/2018 11:44:26+0200, Quentin Schulz wrote: > > In order to use GPIO4 as a GPIO, we need to mux it in this mode so let's > > declare a new pinctrl DT node for it. > > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > --- > > arch/mips/boot/dts/mscc/ocelot.dtsi | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/arch/mips/boot/dts/mscc/ocelot.dtsi b/arch/mips/boot/dts/mscc/ocelot.dtsi > > index 8ce317c..b5c4c74 100644 > > --- a/arch/mips/boot/dts/mscc/ocelot.dtsi > > +++ b/arch/mips/boot/dts/mscc/ocelot.dtsi > > @@ -182,6 +182,11 @@ > > interrupts = <13>; > > #interrupt-cells = <2>; > > > > + gpio4: gpio4 { > > + pins = "GPIO_4"; > > + function = "gpio"; > > + }; > > + > > For a GPIO, I would do that in the board dts because it is not used > directly in the dtsi. > And the day we've two boards using this pinctrl we move it to a dtsi. Is that the plan? Thanks, Quentin
Hi Andrew, On Fri, Sep 14, 2018 at 03:29:30PM +0200, Quentin Schulz wrote: > Hi Andrew, > > On Fri, Sep 14, 2018 at 03:18:46PM +0200, Andrew Lunn wrote: > > > Most of the init sequence of a PHY of the package is common to all PHYs > > > in the package, thus we use the SMI broadcast feature which enables us > > > to propagate a write in one register of one PHY to all PHYs in the > > > package. > > > > Hi Quinten > > > > Could you say a bit more about the broadcast. Does the SMI broadcast > > go to all PHY everywhere on an MDIO bus, or only all PHYs within one > > package? I'm just thinking about the case you need two of these > > packages to cover 8 switch ports. > > > > Ah sorry, that wasn't very explicit. That's a feature on the PHY side so > my wildest guess is that it wouldn't impact any other PHY outside of > this package. Affecting any other PHY on the bus is counter-intuitive to > me but I'll ask the HW engineers for confirmation. > Confirmed by HW engineers, it only impacts PHYs in the same package. Quentin
> Confirmed by HW engineers, it only impacts PHYs in the same package.
Hi Quentin
Thanks for checking. As you said, it would be counter intuitive,
meaning a lot of confusion if it actually did happen.
Maybe you can add "in package" before broadcast in the commit message
and the code comments.
Andrew
On Fri, Sep 14, 2018 at 06:26:38PM +0200, Quentin Schulz wrote: > Hi Alexandre, > > On Fri, Sep 14, 2018 at 04:54:46PM +0200, Alexandre Belloni wrote: > > Hi, > > > > On 14/09/2018 11:44:26+0200, Quentin Schulz wrote: > > > In order to use GPIO4 as a GPIO, we need to mux it in this mode so let's > > > declare a new pinctrl DT node for it. > > > > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > > --- > > > arch/mips/boot/dts/mscc/ocelot.dtsi | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/arch/mips/boot/dts/mscc/ocelot.dtsi b/arch/mips/boot/dts/mscc/ocelot.dtsi > > > index 8ce317c..b5c4c74 100644 > > > --- a/arch/mips/boot/dts/mscc/ocelot.dtsi > > > +++ b/arch/mips/boot/dts/mscc/ocelot.dtsi > > > @@ -182,6 +182,11 @@ > > > interrupts = <13>; > > > #interrupt-cells = <2>; > > > > > > + gpio4: gpio4 { > > > + pins = "GPIO_4"; > > > + function = "gpio"; > > > + }; > > > + > > > > For a GPIO, I would do that in the board dts because it is not used > > directly in the dtsi. > > > > And the day we've two boards using this pinctrl we move it to a dtsi. Is > that the plan? Hi Quentin gpio4 appears to be pretty arbitrary. Could a different design use a different gpio? It me, this seems like a board property. Andrew
> struct vsc8531_private { > int rate_magic; > u16 supp_led_modes; > @@ -181,6 +354,7 @@ struct vsc8531_private { > struct vsc85xx_hw_stat *hw_stats; > u64 *stats; > int nstats; > + bool pkg_init; > +/* bus->mdio_lock should be locked when using this function */ > +static int vsc8584_cmd(struct mii_bus *bus, int phy, u16 val) > +{ > + unsigned long deadline; > + u16 reg_val; > + > + __mdiobus_write(bus, phy, MSCC_EXT_PAGE_ACCESS, > + MSCC_PHY_PAGE_EXTENDED_GPIO); > + > + __mdiobus_write(bus, phy, MSCC_PHY_PROC_CMD, PROC_CMD_NCOMPLETED | val); Hi Quentin All the __mdiobus_write() look a bit ugly. Maybe add bus and base_addr to the vsc8531_private structure. Then add helpers phy_write_base_phy(priv, reg, val) and phy_read_base_phy(priv, reg). You could also add in: if (unlikely(!mutex_is_locked(&priv->bus->mdio_lock))) { dev_err(bus->dev, "MDIO bus lock not held!\n"); dump_stack(); } Having such code in the mv88e6xxx driver has found a few bugs for me. Andrew
On 09/14/2018 02:44 AM, Quentin Schulz wrote: > Part of the config init is common between the VSC8584 and the VSC8574, > so to prepare the upcoming support for VSC8574, separate config_init > PHY-specific code to config_pre_init function which is set in the probe > function of the PHY and used in config_init. > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > --- > drivers/net/phy/mscc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c > index b450489..69cc3cf 100644 > --- a/drivers/net/phy/mscc.c > +++ b/drivers/net/phy/mscc.c > @@ -355,6 +355,7 @@ struct vsc8531_private { > u64 *stats; > int nstats; > bool pkg_init; > + int (*config_pre_init)(struct mii_bus *bus, int phy); Is not this overkill given that you have a reference to the phy_device, you could check for the for phy_id to know which exact type you have and call the appropriate pre_init function? unsigned int phy might be more appropriate.
On 14/09/2018 18:26:38+0200, Quentin Schulz wrote: > Hi Alexandre, > > On Fri, Sep 14, 2018 at 04:54:46PM +0200, Alexandre Belloni wrote: > > Hi, > > > > On 14/09/2018 11:44:26+0200, Quentin Schulz wrote: > > > In order to use GPIO4 as a GPIO, we need to mux it in this mode so let's > > > declare a new pinctrl DT node for it. > > > > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > > --- > > > arch/mips/boot/dts/mscc/ocelot.dtsi | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/arch/mips/boot/dts/mscc/ocelot.dtsi b/arch/mips/boot/dts/mscc/ocelot.dtsi > > > index 8ce317c..b5c4c74 100644 > > > --- a/arch/mips/boot/dts/mscc/ocelot.dtsi > > > +++ b/arch/mips/boot/dts/mscc/ocelot.dtsi > > > @@ -182,6 +182,11 @@ > > > interrupts = <13>; > > > #interrupt-cells = <2>; > > > > > > + gpio4: gpio4 { > > > + pins = "GPIO_4"; > > > + function = "gpio"; > > > + }; > > > + > > > > For a GPIO, I would do that in the board dts because it is not used > > directly in the dtsi. > > > > And the day we've two boards using this pinctrl we move it to a dtsi. Is > that the plan? > Not really, at least not for gpios. I've included the pinctrl for the uart, i2c and spi because they are the only option if you are to use those peripherals. Else, I've would have left the pinctrl to the board file. From my point of view, the gpios are too board specific to be in a soc dtsi.
On 09/14/2018 02:44 AM, Quentin Schulz wrote: > The VSC8574 PHY is a 4-ports PHY that is 10/100/1000BASE-T, 100BASE-FX, > 1000BASE-X and triple-speed copper SFP capable, can communicate with > the MAC via SGMII, QSGMII or 1000BASE-X, supports WOL, downshifting and > can set the blinking pattern of each of its 4 LEDs, supports SyncE as > well as HP Auto-MDIX detection. > > This adds support for 10/100/1000BASE-T, SGMII/QSGMII link with the MAC, > WOL, downshifting, HP Auto-MDIX detection and blinking pattern for its 4 > LEDs. > > The VSC8574 has also an internal Intel 8051 microcontroller whose > firmware needs to be patched when the PHY is reset. If the 8051's > firmware has the expected CRC, its patching can be skipped. The > microcontroller can be accessed from any port of the PHY, though the CRC > function can only be done through the PHY that is the base PHY of the > package (internal address 0) due to a limitation of the firmware. > > The GPIO register bank is a set of registers that are common to all PHYs > in the package. So any modification in any register of this bank affects > all PHYs of the package. > > If the PHYs haven't been reset before booting the Linux kernel and were > configured to use interrupts for e.g. link status updates, it is > required to clear the interrupts mask register of all PHYs before being > able to use interrupts with any PHY. The first PHY of the package that > will be init will take care of clearing all PHYs interrupts mask > registers. Thus, we need to keep track of the init sequence in the > package, if it's already been done or if it's to be done. > > Most of the init sequence of a PHY of the package is common to all PHYs > in the package, thus we use the SMI broadcast feature which enables us > to propagate a write in one register of one PHY to all PHYs in the > package. > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > --- [snip] > + reg = __mdiobus_read(bus, phy, MSCC_PHY_TEST_PAGE_8); > + reg |= 0x8000; Having a define would be nice here? This looks like a write enable? > + __mdiobus_write(bus, phy, MSCC_PHY_TEST_PAGE_8, reg); > + > + __mdiobus_write(bus, phy, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_TR); > + > + vsc8584_csr_write(bus, phy, 0x8fae, 0x000401bd); Just make this an array of address + value pairs and blast it to the PHY, having them be inlined here is both error prone and does not scale well at all. [snip] > + vsc8584_csr_write(bus, phy, 0x84a8, 0x00000000); > + vsc8584_csr_write(bus, phy, 0x84aa, 0x00000000); > + vsc8584_csr_write(bus, phy, 0x84ae, 0x007df7dd); > + vsc8584_csr_write(bus, phy, 0x84b0, 0x006d95d4); > + vsc8584_csr_write(bus, phy, 0x84b2, 0x00492410); Likewise [snip]
Just as a drive-by comment this seems vaguely related to the Vitesse DSA switch I merged in drivers/net/dsa/vitesse-vsc73xx.c The VSC* product name handily gives away the origin in Vitesse's product line. The VSC73xx also have the 8051 CPU and internal RAM, but are accessed (typically) over SPI, and AFAICT this thing is talking over MDIO. The Vitesse 73xx however also supports a WAN port and VLANs which makes it significantly different, falling into switch class I guess. These VSC85*4's does have an SPI interface as well, according to the data sheet but I assume your target boards don't even connect it? When it comes to 8051 code we have quite a lot of this in the kernel these days, I suspect the 8051 snippets in this code could be disassembled and put into linux-firmware in source form, but that is maybe a bit overly ambitious. We have done that for a few USB to serial controllers using the EzUSB 8051 though: https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/keyspan_pda https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/usbdux These can rebuild their firmware using the as31 assembler. https://github.com/nitsky/as31 Yours, Linus Walleij
Hi Andrew, On Fri, Sep 14, 2018 at 07:02:21PM +0200, Andrew Lunn wrote: > On Fri, Sep 14, 2018 at 06:26:38PM +0200, Quentin Schulz wrote: > > Hi Alexandre, > > > > On Fri, Sep 14, 2018 at 04:54:46PM +0200, Alexandre Belloni wrote: > > > Hi, > > > > > > On 14/09/2018 11:44:26+0200, Quentin Schulz wrote: > > > > In order to use GPIO4 as a GPIO, we need to mux it in this mode so let's > > > > declare a new pinctrl DT node for it. > > > > > > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > > > --- > > > > arch/mips/boot/dts/mscc/ocelot.dtsi | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/arch/mips/boot/dts/mscc/ocelot.dtsi b/arch/mips/boot/dts/mscc/ocelot.dtsi > > > > index 8ce317c..b5c4c74 100644 > > > > --- a/arch/mips/boot/dts/mscc/ocelot.dtsi > > > > +++ b/arch/mips/boot/dts/mscc/ocelot.dtsi > > > > @@ -182,6 +182,11 @@ > > > > interrupts = <13>; > > > > #interrupt-cells = <2>; > > > > > > > > + gpio4: gpio4 { > > > > + pins = "GPIO_4"; > > > > + function = "gpio"; > > > > + }; > > > > + > > > > > > For a GPIO, I would do that in the board dts because it is not used > > > directly in the dtsi. > > > > > > > And the day we've two boards using this pinctrl we move it to a dtsi. Is > > that the plan? > > Hi Quentin > > gpio4 appears to be pretty arbitrary. Could a different design use a > different gpio? It me, this seems like a board property. > Right now, I don't see why it couldn't be. Quentin
Hi Alexandre, On Fri, Sep 14, 2018 at 08:02:22PM +0200, Alexandre Belloni wrote: > On 14/09/2018 18:26:38+0200, Quentin Schulz wrote: > > Hi Alexandre, > > > > On Fri, Sep 14, 2018 at 04:54:46PM +0200, Alexandre Belloni wrote: > > > Hi, > > > > > > On 14/09/2018 11:44:26+0200, Quentin Schulz wrote: > > > > In order to use GPIO4 as a GPIO, we need to mux it in this mode so let's > > > > declare a new pinctrl DT node for it. > > > > > > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > > > --- > > > > arch/mips/boot/dts/mscc/ocelot.dtsi | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/arch/mips/boot/dts/mscc/ocelot.dtsi b/arch/mips/boot/dts/mscc/ocelot.dtsi > > > > index 8ce317c..b5c4c74 100644 > > > > --- a/arch/mips/boot/dts/mscc/ocelot.dtsi > > > > +++ b/arch/mips/boot/dts/mscc/ocelot.dtsi > > > > @@ -182,6 +182,11 @@ > > > > interrupts = <13>; > > > > #interrupt-cells = <2>; > > > > > > > > + gpio4: gpio4 { > > > > + pins = "GPIO_4"; > > > > + function = "gpio"; > > > > + }; > > > > + > > > > > > For a GPIO, I would do that in the board dts because it is not used > > > directly in the dtsi. > > > > > > > And the day we've two boards using this pinctrl we move it to a dtsi. Is > > that the plan? > > > > Not really, at least not for gpios. I've included the pinctrl for the > uart, i2c and spi because they are the only option if you are to use > those peripherals. Else, I've would have left the pinctrl to the board > file. From my point of view, the gpios are too board specific to be in a > soc dtsi. > Understood, will move it to the board file. Thanks, Quentin
Hi Florian, On Fri, Sep 14, 2018 at 10:57:08AM -0700, Florian Fainelli wrote: > On 09/14/2018 02:44 AM, Quentin Schulz wrote: > > Part of the config init is common between the VSC8584 and the VSC8574, > > so to prepare the upcoming support for VSC8574, separate config_init > > PHY-specific code to config_pre_init function which is set in the probe > > function of the PHY and used in config_init. > > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > --- > > drivers/net/phy/mscc.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c > > index b450489..69cc3cf 100644 > > --- a/drivers/net/phy/mscc.c > > +++ b/drivers/net/phy/mscc.c > > @@ -355,6 +355,7 @@ struct vsc8531_private { > > u64 *stats; > > int nstats; > > bool pkg_init; > > + int (*config_pre_init)(struct mii_bus *bus, int phy); > > Is not this overkill given that you have a reference to the phy_device, > you could check for the for phy_id to know which exact type you have and > call the appropriate pre_init function? > Agreed. It just seemed "cleaner" to me to set config_pre_init in separate probe functions which are pretty straightforward and short. Thus not complexifying an already-not-so-straightforward config_init function. Anyway, I'll use the phy_id as suggested so that we don't overload the vsc8531_private structure (since it's also only called once, in config_init). Thanks, Quentin
Hi Andrew, On Fri, Sep 14, 2018 at 07:27:54PM +0200, Andrew Lunn wrote: > > > struct vsc8531_private { > > int rate_magic; > > u16 supp_led_modes; > > @@ -181,6 +354,7 @@ struct vsc8531_private { > > struct vsc85xx_hw_stat *hw_stats; > > u64 *stats; > > int nstats; > > + bool pkg_init; > > > +/* bus->mdio_lock should be locked when using this function */ > > +static int vsc8584_cmd(struct mii_bus *bus, int phy, u16 val) > > +{ > > + unsigned long deadline; > > + u16 reg_val; > > + > > + __mdiobus_write(bus, phy, MSCC_EXT_PAGE_ACCESS, > > + MSCC_PHY_PAGE_EXTENDED_GPIO); > > + > > + __mdiobus_write(bus, phy, MSCC_PHY_PROC_CMD, PROC_CMD_NCOMPLETED | val); > > Hi Quentin > > All the __mdiobus_write() look a bit ugly. I agree :) > Maybe add bus and base_addr > to the vsc8531_private structure. Then add helpers > phy_write_base_phy(priv, reg, val) and phy_read_base_phy(priv, reg). > ACK. > You could also add in: > > if (unlikely(!mutex_is_locked(&priv->bus->mdio_lock))) { > dev_err(bus->dev, "MDIO bus lock not held!\n"); > dump_stack(); > } > > Having such code in the mv88e6xxx driver has found a few bugs for me. > ACK. Thanks, Quentin
Hi Andrew, On Fri, Sep 14, 2018 at 06:58:24PM +0200, Andrew Lunn wrote: > > Confirmed by HW engineers, it only impacts PHYs in the same package. > > Hi Quentin > > Thanks for checking. As you said, it would be counter intuitive, > meaning a lot of confusion if it actually did happen. > > Maybe you can add "in package" before broadcast in the commit message > and the code comments. > ACK. Quentin
Hi Florian, On Fri, Sep 14, 2018 at 01:26:06PM -0700, Florian Fainelli wrote: > On 09/14/2018 02:44 AM, Quentin Schulz wrote: > > The VSC8574 PHY is a 4-ports PHY that is 10/100/1000BASE-T, 100BASE-FX, > > 1000BASE-X and triple-speed copper SFP capable, can communicate with > > the MAC via SGMII, QSGMII or 1000BASE-X, supports WOL, downshifting and > > can set the blinking pattern of each of its 4 LEDs, supports SyncE as > > well as HP Auto-MDIX detection. > > > > This adds support for 10/100/1000BASE-T, SGMII/QSGMII link with the MAC, > > WOL, downshifting, HP Auto-MDIX detection and blinking pattern for its 4 > > LEDs. > > > > The VSC8574 has also an internal Intel 8051 microcontroller whose > > firmware needs to be patched when the PHY is reset. If the 8051's > > firmware has the expected CRC, its patching can be skipped. The > > microcontroller can be accessed from any port of the PHY, though the CRC > > function can only be done through the PHY that is the base PHY of the > > package (internal address 0) due to a limitation of the firmware. > > > > The GPIO register bank is a set of registers that are common to all PHYs > > in the package. So any modification in any register of this bank affects > > all PHYs of the package. > > > > If the PHYs haven't been reset before booting the Linux kernel and were > > configured to use interrupts for e.g. link status updates, it is > > required to clear the interrupts mask register of all PHYs before being > > able to use interrupts with any PHY. The first PHY of the package that > > will be init will take care of clearing all PHYs interrupts mask > > registers. Thus, we need to keep track of the init sequence in the > > package, if it's already been done or if it's to be done. > > > > Most of the init sequence of a PHY of the package is common to all PHYs > > in the package, thus we use the SMI broadcast feature which enables us > > to propagate a write in one register of one PHY to all PHYs in the > > package. > > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > --- > > [snip] > > > + reg = __mdiobus_read(bus, phy, MSCC_PHY_TEST_PAGE_8); > > + reg |= 0x8000; > > Having a define would be nice here? This looks like a write enable? > I had asked for the meaning of this bit in this register before but we couldn't find documentation for it. I'll ask again and let you know. > > + __mdiobus_write(bus, phy, MSCC_PHY_TEST_PAGE_8, reg); > > + > > + __mdiobus_write(bus, phy, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_TR); > > + > > + vsc8584_csr_write(bus, phy, 0x8fae, 0x000401bd); > > Just make this an array of address + value pairs and blast it to the > PHY, having them be inlined here is both error prone and does not scale > well at all. Right. Turned out it was a great idea as the below values were mistakenly adding 0x8000 to the register (which was fine since in vsc8584_csr_write does a 0x8000 | reg). Thanks, Quentin
Hi Florian, On Fri, Sep 14, 2018 at 01:26:06PM -0700, Florian Fainelli wrote: > On 09/14/2018 02:44 AM, Quentin Schulz wrote: > > The VSC8574 PHY is a 4-ports PHY that is 10/100/1000BASE-T, 100BASE-FX, > > 1000BASE-X and triple-speed copper SFP capable, can communicate with > > the MAC via SGMII, QSGMII or 1000BASE-X, supports WOL, downshifting and > > can set the blinking pattern of each of its 4 LEDs, supports SyncE as > > well as HP Auto-MDIX detection. > > > > This adds support for 10/100/1000BASE-T, SGMII/QSGMII link with the MAC, > > WOL, downshifting, HP Auto-MDIX detection and blinking pattern for its 4 > > LEDs. > > > > The VSC8574 has also an internal Intel 8051 microcontroller whose > > firmware needs to be patched when the PHY is reset. If the 8051's > > firmware has the expected CRC, its patching can be skipped. The > > microcontroller can be accessed from any port of the PHY, though the CRC > > function can only be done through the PHY that is the base PHY of the > > package (internal address 0) due to a limitation of the firmware. > > > > The GPIO register bank is a set of registers that are common to all PHYs > > in the package. So any modification in any register of this bank affects > > all PHYs of the package. > > > > If the PHYs haven't been reset before booting the Linux kernel and were > > configured to use interrupts for e.g. link status updates, it is > > required to clear the interrupts mask register of all PHYs before being > > able to use interrupts with any PHY. The first PHY of the package that > > will be init will take care of clearing all PHYs interrupts mask > > registers. Thus, we need to keep track of the init sequence in the > > package, if it's already been done or if it's to be done. > > > > Most of the init sequence of a PHY of the package is common to all PHYs > > in the package, thus we use the SMI broadcast feature which enables us > > to propagate a write in one register of one PHY to all PHYs in the > > package. > > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > --- > > [snip] > > > + reg = __mdiobus_read(bus, phy, MSCC_PHY_TEST_PAGE_8); > > + reg |= 0x8000; > > Having a define would be nice here? This looks like a write enable? > Same as for the SerDes muxing patch series, I'm sending a new version without the suggested change. I've requested some information on this bit but can't guarantee I'll get anything (and if anything, worthy). I'll try not to forget to add a constant if and when I get a clue on what this bit does. Thanks, Quentin