| Message ID | 20240430083730.134918-7-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for the LAN966x PCI device using a DT overlay | expand |
On Tue, Apr 30, 2024 at 10:37:15AM +0200, Herve Codina wrote: > Add the (optional) resets property. > The mscc-miim device is impacted by the switch reset especially when the > mscc-miim device is used as part of the LAN966x PCI device. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > --- > Documentation/devicetree/bindings/net/mscc,miim.yaml | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/mscc,miim.yaml b/Documentation/devicetree/bindings/net/mscc,miim.yaml > index 5b292e7c9e46..a8c92cec85a6 100644 > --- a/Documentation/devicetree/bindings/net/mscc,miim.yaml > +++ b/Documentation/devicetree/bindings/net/mscc,miim.yaml > @@ -38,6 +38,14 @@ properties: > > clock-frequency: true > > + resets: > + items: > + - description: Reset controller used for switch core reset (soft reset) A follow up to the comment on the next patch. I think it should be made clear in the patch and the binding, the aim is to reset the MDIO bus master, not the switch. It just happens that the MDIO bus master is within the domain of the switch core, and so the switch core reset also resets the MDIO bus master. Architecturally, this is important. I would not expect the MDIO driver to be resetting the switch, the switch driver should be doing that. But we have seen some odd Qualcomm patches where the MDIO driver has been doing things outside the scope of MDIO, playing with resets and clocks which are not directly related to the MDIO bus master. I want to avoid any confusion here, especially when Qualcomm tries again, and maybe points at this code. Andrew
Hi Andrew, On Tue, 30 Apr 2024 15:55:58 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > On Tue, Apr 30, 2024 at 10:37:15AM +0200, Herve Codina wrote: > > Add the (optional) resets property. > > The mscc-miim device is impacted by the switch reset especially when the > > mscc-miim device is used as part of the LAN966x PCI device. > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > --- > > Documentation/devicetree/bindings/net/mscc,miim.yaml | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/mscc,miim.yaml b/Documentation/devicetree/bindings/net/mscc,miim.yaml > > index 5b292e7c9e46..a8c92cec85a6 100644 > > --- a/Documentation/devicetree/bindings/net/mscc,miim.yaml > > +++ b/Documentation/devicetree/bindings/net/mscc,miim.yaml > > @@ -38,6 +38,14 @@ properties: > > > > clock-frequency: true > > > > + resets: > > + items: > > + - description: Reset controller used for switch core reset (soft reset) > > A follow up to the comment on the next patch. I think it should be > made clear in the patch and the binding, the aim is to reset the MDIO > bus master, not the switch. It just happens that the MDIO bus master > is within the domain of the switch core, and so the switch core reset > also resets the MDIO bus master. Exactly. > > Architecturally, this is important. I would not expect the MDIO driver > to be resetting the switch, the switch driver should be doing > that. But we have seen some odd Qualcomm patches where the MDIO driver > has been doing things outside the scope of MDIO, playing with resets > and clocks which are not directly related to the MDIO bus master. I > want to avoid any confusion here, especially when Qualcomm tries > again, and maybe points at this code. > Sure. We have the same construction with the pinctrl driver used in the LAN966x Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.yaml The reset name is 'switch' in the pinctrl binding. I can use the same description here as the one present in the pinctrl binding: description: Optional shared switch reset. and keep 'switch' as reset name here (consistent with pinctrl reset name). What do you think about that ? Best regards, Hervé
> We have the same construction with the pinctrl driver used in the LAN966x > Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.yaml > > The reset name is 'switch' in the pinctrl binding. > I can use the same description here as the one present in the pinctrl binding: > description: Optional shared switch reset. > and keep 'switch' as reset name here (consistent with pinctrl reset name). > > What do you think about that ? It would be good to document what it is shared with. So it seems to be the switch itself, pinctl and MDIO? Anything else? Andrew
Hi Andrew, On Tue, 30 Apr 2024 18:31:46 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > > We have the same construction with the pinctrl driver used in the LAN966x > > Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.yaml > > > > The reset name is 'switch' in the pinctrl binding. > > I can use the same description here as the one present in the pinctrl binding: > > description: Optional shared switch reset. > > and keep 'switch' as reset name here (consistent with pinctrl reset name). > > > > What do you think about that ? > > It would be good to document what it is shared with. So it seems to be > the switch itself, pinctl and MDIO? Anything else? > To be honest, I know that the GPIO controller (microchip,sparx5-sgpio) is impacted but I don't know if anything else is impacted by this reset. I can update the description with: description: Optional shared switch reset. This reset is shared with at least pinctrl, GPIO, MDIO and the switch itself. Does it sound better ? Best regards Hervé
On Thu, May 02, 2024 at 11:50:43AM +0200, Herve Codina wrote: > Hi Andrew, > > On Tue, 30 Apr 2024 18:31:46 +0200 > Andrew Lunn <andrew@lunn.ch> wrote: > > > > We have the same construction with the pinctrl driver used in the LAN966x > > > Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.yaml > > > > > > The reset name is 'switch' in the pinctrl binding. > > > I can use the same description here as the one present in the pinctrl binding: > > > description: Optional shared switch reset. > > > and keep 'switch' as reset name here (consistent with pinctrl reset name). > > > > > > What do you think about that ? > > > > It would be good to document what it is shared with. So it seems to be > > the switch itself, pinctl and MDIO? Anything else? > > > > To be honest, I know that the GPIO controller (microchip,sparx5-sgpio) is > impacted but I don't know if anything else is impacted by this reset. > I can update the description with: > description: > Optional shared switch reset. > This reset is shared with at least pinctrl, GPIO, MDIO and the switch > itself. > > Does it sound better ? $dayjob hat off, bindings hat on: If you don't know, can we get someone from Microchip (there's some and a list in CC) to figure it out? Cheers, Conor.
On Thu, May 02, 2024 at 11:31:00AM +0100, Conor Dooley wrote: > On Thu, May 02, 2024 at 11:50:43AM +0200, Herve Codina wrote: > > Hi Andrew, > > > > On Tue, 30 Apr 2024 18:31:46 +0200 > > Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > We have the same construction with the pinctrl driver used in the LAN966x > > > > Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.yaml > > > > > > > > The reset name is 'switch' in the pinctrl binding. > > > > I can use the same description here as the one present in the pinctrl binding: > > > > description: Optional shared switch reset. > > > > and keep 'switch' as reset name here (consistent with pinctrl reset name). > > > > > > > > What do you think about that ? > > > > > > It would be good to document what it is shared with. So it seems to be > > > the switch itself, pinctl and MDIO? Anything else? > > > > > > > To be honest, I know that the GPIO controller (microchip,sparx5-sgpio) is > > impacted but I don't know if anything else is impacted by this reset. > > I can update the description with: > > description: > > Optional shared switch reset. > > This reset is shared with at least pinctrl, GPIO, MDIO and the switch > > itself. > > > > Does it sound better ? > > $dayjob hat off, bindings hat on: If you don't know, can we get someone > from Microchip (there's some and a list in CC) to figure it out? That is probably a good idea, there is potential for hard to find bugs here, when a device gets an unexpected reset. Change the order things probe, or an unexpected EPRODE_DEFER could be interesting. Andrew
On 02/05/2024 14:26:36+0200, Andrew Lunn wrote: > On Thu, May 02, 2024 at 11:31:00AM +0100, Conor Dooley wrote: > > On Thu, May 02, 2024 at 11:50:43AM +0200, Herve Codina wrote: > > > Hi Andrew, > > > > > > On Tue, 30 Apr 2024 18:31:46 +0200 > > > Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > We have the same construction with the pinctrl driver used in the LAN966x > > > > > Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.yaml > > > > > > > > > > The reset name is 'switch' in the pinctrl binding. > > > > > I can use the same description here as the one present in the pinctrl binding: > > > > > description: Optional shared switch reset. > > > > > and keep 'switch' as reset name here (consistent with pinctrl reset name). > > > > > > > > > > What do you think about that ? > > > > > > > > It would be good to document what it is shared with. So it seems to be > > > > the switch itself, pinctl and MDIO? Anything else? > > > > > > > > > > To be honest, I know that the GPIO controller (microchip,sparx5-sgpio) is > > > impacted but I don't know if anything else is impacted by this reset. > > > I can update the description with: > > > description: > > > Optional shared switch reset. > > > This reset is shared with at least pinctrl, GPIO, MDIO and the switch > > > itself. > > > > > > Does it sound better ? > > > > $dayjob hat off, bindings hat on: If you don't know, can we get someone > > from Microchip (there's some and a list in CC) to figure it out? > > That is probably a good idea, there is potential for hard to find bugs > here, when a device gets an unexpected reset. Change the order things > probe, or an unexpected EPRODE_DEFER could be interesting. > The datasheet states: "The VCore system comprises all the blocks attached to the VCore Shared Bus (SBA), including the PCIe, DDR, frame DMA, SI slave, and MIIM slave blocks. The device includes all the blocks attached to the Switch Core Register Bus (CSR) including the VRAP slave. For more information about the VCore System blocks, see Figure 5-1." However, the reset driver protects the VCORE itself by setting bit 5. Everything else is going to be reset.
Hi, On Thu, 2 May 2024 15:22:09 +0200 Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > On 02/05/2024 14:26:36+0200, Andrew Lunn wrote: > > On Thu, May 02, 2024 at 11:31:00AM +0100, Conor Dooley wrote: > > > On Thu, May 02, 2024 at 11:50:43AM +0200, Herve Codina wrote: > > > > Hi Andrew, > > > > > > > > On Tue, 30 Apr 2024 18:31:46 +0200 > > > > Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > > > We have the same construction with the pinctrl driver used in the LAN966x > > > > > > Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.yaml > > > > > > > > > > > > The reset name is 'switch' in the pinctrl binding. > > > > > > I can use the same description here as the one present in the pinctrl binding: > > > > > > description: Optional shared switch reset. > > > > > > and keep 'switch' as reset name here (consistent with pinctrl reset name). > > > > > > > > > > > > What do you think about that ? > > > > > > > > > > It would be good to document what it is shared with. So it seems to be > > > > > the switch itself, pinctl and MDIO? Anything else? > > > > > > > > > > > > > To be honest, I know that the GPIO controller (microchip,sparx5-sgpio) is > > > > impacted but I don't know if anything else is impacted by this reset. > > > > I can update the description with: > > > > description: > > > > Optional shared switch reset. > > > > This reset is shared with at least pinctrl, GPIO, MDIO and the switch > > > > itself. > > > > > > > > Does it sound better ? > > > > > > $dayjob hat off, bindings hat on: If you don't know, can we get someone > > > from Microchip (there's some and a list in CC) to figure it out? > > > > That is probably a good idea, there is potential for hard to find bugs > > here, when a device gets an unexpected reset. Change the order things > > probe, or an unexpected EPRODE_DEFER could be interesting. > > > > > The datasheet states: > "The VCore system comprises all the blocks attached to the VCore Shared > Bus (SBA), including the PCIe, DDR, frame DMA, SI slave, and MIIM slave > blocks. The device includes all the blocks attached to the Switch Core > Register Bus (CSR) including the VRAP slave. For more information about > the VCore System blocks, see Figure 5-1." > > However, the reset driver protects the VCORE itself by setting bit 5. > Everything else is going to be reset. > Right, I will update the reset description with: description: Optional shared switch reset. This reset is shared with all blocks attached to the Switch Core Register Bus (CSR) including VRAP slave. Is that better ? Best regards, Hervé
diff --git a/Documentation/devicetree/bindings/net/mscc,miim.yaml b/Documentation/devicetree/bindings/net/mscc,miim.yaml index 5b292e7c9e46..a8c92cec85a6 100644 --- a/Documentation/devicetree/bindings/net/mscc,miim.yaml +++ b/Documentation/devicetree/bindings/net/mscc,miim.yaml @@ -38,6 +38,14 @@ properties: clock-frequency: true + resets: + items: + - description: Reset controller used for switch core reset (soft reset) + + reset-names: + items: + - const: switch + required: - compatible - reg
Add the (optional) resets property. The mscc-miim device is impacted by the switch reset especially when the mscc-miim device is used as part of the LAN966x PCI device. Signed-off-by: Herve Codina <herve.codina@bootlin.com> --- Documentation/devicetree/bindings/net/mscc,miim.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)