| Message ID | E1eRnWr-0006VS-1m@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 20, 2017 at 11:12:01PM +0000, Russell King wrote: > The 88e1545 PHY has its interrupts wired to the VF610, so we might as > well use them. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > This is certainly not correct, as all PHYs on this device share the > same interrupt line, but we can't specify the pinmux settings > individually on each PHY. How should this be handled? Hi Russell You could put it as a hog on the gpio controller node. However, i don't think that is much better. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Thu, Dec 21, 2017 at 12:12 AM, Russell King <rmk+kernel@armlinux.org.uk> wrote: > The 88e1545 PHY has its interrupts wired to the VF610, so we might as > well use them. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > This is certainly not correct, as all PHYs on this device share the > same interrupt line, but we can't specify the pinmux settings > individually on each PHY. How should this be handled? I do not know the details of the Marvell switch. Sorry for any possible misunderstandings below. What I did with the Realtek switch I was playing around with was to create a separate irqchip, also in the device tree, embedded inside the DSA switch, then referenced the IRQs from that chip as 0, 1 .. n. The patches are here: DTS: https://marc.info/?l=linux-netdev&m=150992420713391&w=2 Driver: https://marc.info/?l=linux-netdev&m=150992421113393&w=2 Note that this RFC is wrong: it assigns the IRQs to ports instead of PHYs, but the idea with an IRQchip inside the DSA is pretty solid IMO. (I will rewrite it using your method of a separate mdio bus node and phy-handle references.) Anyway I was inspired to this model from certain PCI bridges that contain an IRQ demuxer and thus instantiate an irqchip for this, that is then part of the bridge itself. Then for the pin control, I guess the irqchip inside the bridge should be the entity taking the IRQ from the GPIO-backed irq controller and also the pin control handle. As pin control handles are tied to Linux devices, that requires it to be a device proper though. I don't know if it's possible to properly spawn a device for this irqchip from the switch, but I guess it is what I would try. I hope this helps. Yours, Linus Walleij
On Thu, Dec 21, 2017 at 01:32:21PM +0100, Linus Walleij wrote: > On Thu, Dec 21, 2017 at 12:12 AM, Russell King > <rmk+kernel@armlinux.org.uk> wrote: > > > The 88e1545 PHY has its interrupts wired to the VF610, so we might as > > well use them. > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > --- > > This is certainly not correct, as all PHYs on this device share the > > same interrupt line, but we can't specify the pinmux settings > > individually on each PHY. How should this be handled? > > I do not know the details of the Marvell switch. Hi Linus The 88e1545 is a discreet quad PHY. It is connected to the switch, but not integrated into the switch. All its interrupt handling is done with a GPIO onto the Freescale processor, via a GPIO. There is nothing DSA related here at all with respect to the interrupt. It is just a normal GPIO interrupt. What is a bit odd is that it one shared interrupt for all four PHYs. What you described with an irqchip inside the switch is what we actually do for the internal PHYs on Marvell devices. And it is what i recommend for all DSA drivers. Expose standard IRQs, and let phylib use them in its normal way. Andrew
On Thu, Dec 21, 2017 at 02:40:58PM +0100, Andrew Lunn wrote: > On Thu, Dec 21, 2017 at 01:32:21PM +0100, Linus Walleij wrote: > > On Thu, Dec 21, 2017 at 12:12 AM, Russell King > > <rmk+kernel@armlinux.org.uk> wrote: > > > > > The 88e1545 PHY has its interrupts wired to the VF610, so we might as > > > well use them. > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > --- > > > This is certainly not correct, as all PHYs on this device share the > > > same interrupt line, but we can't specify the pinmux settings > > > individually on each PHY. How should this be handled? > > > > I do not know the details of the Marvell switch. > > Hi Linus > > The 88e1545 is a discreet quad PHY. It is connected to the switch, but > not integrated into the switch. All its interrupt handling is done > with a GPIO onto the Freescale processor, via a GPIO. There is nothing > DSA related here at all with respect to the interrupt. It is just a > normal GPIO interrupt. What is a bit odd is that it one shared > interrupt for all four PHYs. > > What you described with an irqchip inside the switch is what we > actually do for the internal PHYs on Marvell devices. And it is what i > recommend for all DSA drivers. Expose standard IRQs, and let phylib > use them in its normal way. ... and it has to be said that model doesn't work in this case, because, although there is the possibility to demux the interrupt any of the PHYs, you already need to be driving one of the PHYs. It's not an interrupt controller itself (there's no possibility to enable/disable individual interrupts from a PHY) so it doesn't make sense. What we have here is _really_ a shared interrupt between four separate devices, and we need a way to sanely describe resources shared between several device instances to pinmux. Unfortunately, it seems pinmux is designed around one device having exclusive use of a resource, which makes it hard to describe shared interrupts in DT. Given that DT should be a description of the hardware, and should be independent of the OS implementation, I'd say this is a pinmux bug, because pinmux gets in the way of describing the hardware correctly. ;)
On Thu, Dec 21, 2017 at 6:32 PM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > What we have here is _really_ a shared interrupt between four > separate devices, and we need a way to sanely describe resources > shared between several device instances to pinmux. Unfortunately, > it seems pinmux is designed around one device having exclusive use > of a resource, which makes it hard to describe shared interrupts in > DT. > > Given that DT should be a description of the hardware, and should be > independent of the OS implementation, I'd say this is a pinmux bug, > because pinmux gets in the way of describing the hardware correctly. > ;) Hm that would be annoying. But when I look at it I think it would actually work. Did you try just assigning the same pin control state to all the PHY's and see what happens? Just set pinctrl-names = "default"; pinctrl-0 = <&pinctrl_mv88e1545>; on all of them? I don't think DTC would complain at least. When I look at the driver subsystem, I don't see anything really stopping you from doing that and even have three devices selecting the same "default" pin control state. It seems it will just wind up three times in pinctrl_select_state() and the first time it calls the pin control driver to actually set it and the three other times it finds the state is already correct and returns success. So the way I read it actually several devices can reference the same pin control state. That is, unless there is something I missed. Which I often do... If it happens to work we should probably put a blurb in the DT binding that this is expected behaviour though. Yours, Linus Walleij
On Thu, Dec 21, 2017 at 11:53:47PM +0100, Linus Walleij wrote: > On Thu, Dec 21, 2017 at 6:32 PM, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > > What we have here is _really_ a shared interrupt between four > > separate devices, and we need a way to sanely describe resources > > shared between several device instances to pinmux. Unfortunately, > > it seems pinmux is designed around one device having exclusive use > > of a resource, which makes it hard to describe shared interrupts in > > DT. > > > > Given that DT should be a description of the hardware, and should be > > independent of the OS implementation, I'd say this is a pinmux bug, > > because pinmux gets in the way of describing the hardware correctly. > > ;) > > Hm that would be annoying. But when I look at it I think it would > actually work. Did you try just assigning the same pin control > state to all the PHY's and see what happens? > > Just set > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_mv88e1545>; > > on all of them? It was tried, DT was happy, but the kernel on boot complained because pinctrl objected, which caused the drivers to fail to bind: libphy: mdio: probed vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio@4!switch@0!mdio:00; cannot claim for !mdio-mux!mdio@4!switch@0!mdio:01 vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio@4!switch@0!mdio:01) status -22 vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545 on device 40048000.iomuxc Marvell 88E1545 !mdio-mux!mdio@4!switch@0!mdio:01: Error applying setting, reverse things back Marvell 88E1545: probe of !mdio-mux!mdio@4!switch@0!mdio:01 failed with error -22 vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio@4!switch@0!mdio:00; cannot claim for !mdio-mux!mdio@4!switch@0!mdio:02 vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio@4!switch@0!mdio:02) status -22 vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545 on device 40048000.iomuxc Marvell 88E1545 !mdio-mux!mdio@4!switch@0!mdio:02: Error applying setting, reverse things back Marvell 88E1545: probe of !mdio-mux!mdio@4!switch@0!mdio:02 failed with error -22
On 12/21/2017 04:14 PM, Russell King - ARM Linux wrote: > On Thu, Dec 21, 2017 at 11:53:47PM +0100, Linus Walleij wrote: >> On Thu, Dec 21, 2017 at 6:32 PM, Russell King - ARM Linux >> <linux@armlinux.org.uk> wrote: >> >>> What we have here is _really_ a shared interrupt between four >>> separate devices, and we need a way to sanely describe resources >>> shared between several device instances to pinmux. Unfortunately, >>> it seems pinmux is designed around one device having exclusive use >>> of a resource, which makes it hard to describe shared interrupts in >>> DT. >>> >>> Given that DT should be a description of the hardware, and should be >>> independent of the OS implementation, I'd say this is a pinmux bug, >>> because pinmux gets in the way of describing the hardware correctly. >>> ;) >> >> Hm that would be annoying. But when I look at it I think it would >> actually work. Did you try just assigning the same pin control >> state to all the PHY's and see what happens? >> >> Just set >> pinctrl-names = "default"; >> pinctrl-0 = <&pinctrl_mv88e1545>; >> >> on all of them? > > It was tried, DT was happy, but the kernel on boot complained because > pinctrl objected, which caused the drivers to fail to bind: > > libphy: mdio: probed > vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio@4!switch@0!mdio:00; cannot claim for !mdio-mux!mdio@4!switch@0!mdio:01 > vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio@4!switch@0!mdio:01) status -22 > vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545 on device 40048000.iomuxc > Marvell 88E1545 !mdio-mux!mdio@4!switch@0!mdio:01: Error applying setting, reverse things back > Marvell 88E1545: probe of !mdio-mux!mdio@4!switch@0!mdio:01 failed with error -22 > vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio@4!switch@0!mdio:00; cannot claim for !mdio-mux!mdio@4!switch@0!mdio:02 > vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio@4!switch@0!mdio:02) status -22 > vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545 on device 40048000.iomuxc > Marvell 88E1545 !mdio-mux!mdio@4!switch@0!mdio:02: Error applying setting, reverse things back > Marvell 88E1545: probe of !mdio-mux!mdio@4!switch@0!mdio:02 failed with error -22 > You could also see it another way, because this is a quad PHY in a single package, you could theoretically have a representation that exposes a node container for the 4 PHYs, and that container node requests the pinmux/pinctrl. Of course, this would not work with the MDIO code which would not go one level down, and would expect the PHYs to be at the same level as the container node... Oh well.
On Thu, Dec 21, 2017 at 04:20:34PM -0800, Florian Fainelli wrote: > On 12/21/2017 04:14 PM, Russell King - ARM Linux wrote: > > On Thu, Dec 21, 2017 at 11:53:47PM +0100, Linus Walleij wrote: > >> On Thu, Dec 21, 2017 at 6:32 PM, Russell King - ARM Linux > >> <linux@armlinux.org.uk> wrote: > >> > >>> What we have here is _really_ a shared interrupt between four > >>> separate devices, and we need a way to sanely describe resources > >>> shared between several device instances to pinmux. Unfortunately, > >>> it seems pinmux is designed around one device having exclusive use > >>> of a resource, which makes it hard to describe shared interrupts in > >>> DT. > >>> > >>> Given that DT should be a description of the hardware, and should be > >>> independent of the OS implementation, I'd say this is a pinmux bug, > >>> because pinmux gets in the way of describing the hardware correctly. > >>> ;) > >> > >> Hm that would be annoying. But when I look at it I think it would > >> actually work. Did you try just assigning the same pin control > >> state to all the PHY's and see what happens? > >> > >> Just set > >> pinctrl-names = "default"; > >> pinctrl-0 = <&pinctrl_mv88e1545>; > >> > >> on all of them? > > > > It was tried, DT was happy, but the kernel on boot complained because > > pinctrl objected, which caused the drivers to fail to bind: > > > > libphy: mdio: probed > > vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio@4!switch@0!mdio:00; cannot claim for !mdio-mux!mdio@4!switch@0!mdio:01 > > vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio@4!switch@0!mdio:01) status -22 > > vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545 on device 40048000.iomuxc > > Marvell 88E1545 !mdio-mux!mdio@4!switch@0!mdio:01: Error applying setting, reverse things back > > Marvell 88E1545: probe of !mdio-mux!mdio@4!switch@0!mdio:01 failed with error -22 > > vf610-pinctrl 40048000.iomuxc: pin VF610_PAD_PTB0 already requested by !mdio-mux!mdio@4!switch@0!mdio:00; cannot claim for !mdio-mux!mdio@4!switch@0!mdio:02 > > vf610-pinctrl 40048000.iomuxc: pin-22 (!mdio-mux!mdio@4!switch@0!mdio:02) status -22 > > vf610-pinctrl 40048000.iomuxc: could not request pin 22 (VF610_PAD_PTB0) from group pinctrl-mv88e1545 on device 40048000.iomuxc > > Marvell 88E1545 !mdio-mux!mdio@4!switch@0!mdio:02: Error applying setting, reverse things back > > Marvell 88E1545: probe of !mdio-mux!mdio@4!switch@0!mdio:02 failed with error -22 > > > > You could also see it another way, because this is a quad PHY in a > single package, you could theoretically have a representation that > exposes a node container for the 4 PHYs, and that container node > requests the pinmux/pinctrl. Of course, this would not work with the > MDIO code which would not go one level down, and would expect the PHYs > to be at the same level as the container node... It would actually - we have other devices that sit on buses that take several addresses, and we describe the "first" main device or use MFD for it. For example, in the case of the TDA998x HDMI encoder, these are two devices merged into one package - the HDMI encoder at one address, and a TDA9950 at another address. Both addresses are related, so if you tie the address configuration pins, the offset is added to both base addresses. We represent the TDA998x in DT, and have the TDA998x driver create a separate device itself for the TDA9950. What we could do for any multi-package PHY is describe the first PHY as a multi-package PHY in DT, extend the phy binding to include a PHY package index, and have the PHY driver create the MDIO devices for the other PHYs. Eg, switch { ... ports { port@0 { reg = <0>; label = "lan6"; phy-handle = <&switch2phy 0>; }; port@1 { reg = <1>; label = "lan6"; phy-handle = <&switch2phy 1>; }; port@2 { reg = <2>; label = "lan6"; phy-handle = <&switch2phy 2>; }; }; mdio { #address-cells = <1>; #size-cells = <0>; switch2phy: phy@0 { compatible = "marvell,88e1545", "ethernet-phy-ieee802.3-c22"; interrupt-parent = <&gpio0>; interrupts = <22 IRQ_TYPE_LEVEL_LOW>; reg = <0>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_mv88e1545>; }; }; }; The "marvell,88e1545" driver would be responsible for creating the PHY devices for the other MDIO bus addresses (iow 1 to 3.) This would be an accurate respresentation of the hardware in DT, probably more so than the trap we seem to have fallen into by describing the individual PHYs - which we've fallen into because that's how our current implementation requires us to describe them. Since DT is supposed to be a hardware description, I think the question we ought to ask is: if we were starting afresh, how would we describe these packages that contain multiple PHYs?
diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts index 782b69a3acdf..d6786c5d0109 100644 --- a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts +++ b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts @@ -312,12 +312,20 @@ #size-cells = <0>; switch2phy0: phy@0 { + interrupt-parent = <&gpio0>; + interrupts = <22 IRQ_TYPE_LEVEL_LOW>; reg = <0>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_mv88e1545>; }; switch2phy1: phy@1 { + interrupt-parent = <&gpio0>; + interrupts = <22 IRQ_TYPE_LEVEL_LOW>; reg = <1>; }; switch2phy2: phy@2 { + interrupt-parent = <&gpio0>; + interrupts = <22 IRQ_TYPE_LEVEL_LOW>; reg = <2>; }; }; @@ -488,6 +496,12 @@ >; }; + pinctrl_mv88e1545: pinctrl-mv88e1545 { + fsl,pins = < + VF610_PAD_PTB0__GPIO_22 0x219d + >; + }; + pinctrl_pca9554_22: pinctrl-pca95540-22 { fsl,pins = < VF610_PAD_PTB28__GPIO_98 0x219d
The 88e1545 PHY has its interrupts wired to the VF610, so we might as well use them. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- This is certainly not correct, as all PHYs on this device share the same interrupt line, but we can't specify the pinmux settings individually on each PHY. How should this be handled? --- arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 14 ++++++++++++++ 1 file changed, 14 insertions(+)