| Message ID | e7a699fb-f7aa-3a3e-625f-7a2c512da5f9@sord.co.jp |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | net: phy: adin: add support for inverting the link status output signal | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Wed, May 31, 2023 at 10:57:55PM +0900, Atsushi Nemoto wrote: > The ADIN1200/ADIN1300 supports inverting the link status output signal > on the LINK_ST pin. Please could you add an explanation to the commit message why you would want to do this? Is this because there is an LED connected to it, but its polarity is inverted? Andrew
On 2023/05/31 23:35, Andrew Lunn wrote: >> The ADIN1200/ADIN1300 supports inverting the link status output signal >> on the LINK_ST pin. > > Please could you add an explanation to the commit message why you > would want to do this? Is this because there is an LED connected to > it, but its polarity is inverted? Yes, the LINK_ST pin of AD1200/1300 is active-high but our custom board designer connected it to an LED as an active-low signal. I'd like to change the description as follows: --- The ADIN1200/ADIN1300 supports inverting the link status output signal on the LINK_ST pin. The LINK_ST is active-high by default. This can be inverted by the GE_LNK_STAT_INV_REG register, meaning that link up is indicated by setting LINK_ST low. Add support for selecting this feature via device-tree properties. --- Atsushi Nemoto
On Thu, Jun 01, 2023 at 09:29:01AM +0900, Atsushi Nemoto wrote: > On 2023/05/31 23:35, Andrew Lunn wrote: > >> The ADIN1200/ADIN1300 supports inverting the link status output signal > >> on the LINK_ST pin. > > > > Please could you add an explanation to the commit message why you > > would want to do this? Is this because there is an LED connected to > > it, but its polarity is inverted? > > Yes, the LINK_ST pin of AD1200/1300 is active-high but our custom > board designer connected it to an LED as an active-low signal. O.K. LED is the magic word here. And the fact that there is nothing specific to this PHY. Being able to specify the polarity of an output to an LED is pretty common. Please take a look at: ocumentation/devicetree/bindings/net/ethernet-phy.yaml where it describes LEDs. Please add a generic DT property there for everybody to use. See if the LED subsystem has a name for such a property. There is sufficient code in net-next to allow LED0 to be controlled in a limited way. There are more patches coming soon which will give you much more control. Using LINK_ST to control an LED is not so easy to represent in the current code because it appears you don't have manual control of the LED, i.e. forced on/off. But you can offload functions when the new code lands. Andrew
On 2023/06/01 21:28, Andrew Lunn wrote: >>> Please could you add an explanation to the commit message why you >>> would want to do this? Is this because there is an LED connected to >>> it, but its polarity is inverted? >> >> Yes, the LINK_ST pin of AD1200/1300 is active-high but our custom >> board designer connected it to an LED as an active-low signal. > > O.K. LED is the magic word here. And the fact that there is nothing > specific to this PHY. Being able to specify the polarity of an output > to an LED is pretty common. > > Please take a look at: > > ocumentation/devicetree/bindings/net/ethernet-phy.yaml Thanks for your advice. But in this case, LINK_ST is also connected to MII_RXLINK pin of the MAC module in SoC. MII_RXLINK also expects low-active signal input. (though I only wrote about LED, sorry) AD1200/AD1200 have another LED_0 pin and it should be appropriate for the LED subsystem. So I still like to use this device specific property device. --- Atsushi Nemoto
> But in this case, LINK_ST is also connected to MII_RXLINK pin of > the MAC module in SoC. MII_RXLINK also expects low-active signal input. > (though I only wrote about LED, sorry) This is why the Commit message should contain the answer to 'Why?'. The code tells us 'What', but without knowing the 'Why', it is hard to say if you are doing the right or wrong thing. O.K. so the signal is also connected to the SoC. Why? This is very unusual. The MAC does not really care if there is link or not, it just sends a bitstream to the PHY. phylib will be monitoring the PHY and when it detects the link is down it will tell the MAC via the adjust_link callback. What SoC is this? > AD1200/AD1200 have another LED_0 pin and it should be appropriate for > the LED subsystem. Agreed. Andrew
On 2023/06/01 22:36, Andrew Lunn wrote: >> But in this case, LINK_ST is also connected to MII_RXLINK pin of >> the MAC module in SoC. MII_RXLINK also expects low-active signal input. >> (though I only wrote about LED, sorry) > > This is why the Commit message should contain the answer to 'Why?'. > The code tells us 'What', but without knowing the 'Why', it is hard to > say if you are doing the right or wrong thing. Yes, then how about this? --- The LINK_ST is active-high by default. This can be inverted by the GE_LNK_STAT_INV_REG register, meaning that link up is indicated by setting LINK_ST low. LINK_ST is not a generic LED signal but a dedicated signal for the link status. So use device specific propery instead of a LED subsystem. --- > O.K. so the signal is also connected to the SoC. Why? This is very > unusual. The MAC does not really care if there is link or not, it just > sends a bitstream to the PHY. phylib will be monitoring the PHY and > when it detects the link is down it will tell the MAC via the > adjust_link callback. > > What SoC is this? PRU in TI Sitara SoC. This is actually a Software MAC controlled by TI firmware. So unusual. Please look at a block diagram in this page: https://software-dl.ti.com/processor-sdk-linux/esd/docs/latest/linux/Foundational_Components/PRU-ICSS/Linux_Drivers/PRU-ICSS_Ethernet.html I don't know why it needs MII_RXLINK signal... --- Atsushi Nemoto
On Thu, Jun 01, 2023 at 11:17:52PM +0900, Atsushi Nemoto wrote: > On 2023/06/01 22:36, Andrew Lunn wrote: > >> But in this case, LINK_ST is also connected to MII_RXLINK pin of > >> the MAC module in SoC. MII_RXLINK also expects low-active signal input. > >> (though I only wrote about LED, sorry) > > > > This is why the Commit message should contain the answer to 'Why?'. > > The code tells us 'What', but without knowing the 'Why', it is hard to > > say if you are doing the right or wrong thing. > > Yes, then how about this? > --- > The LINK_ST is active-high by default. This can be inverted by the > GE_LNK_STAT_INV_REG register, meaning that link up is indicated by > setting LINK_ST low. O.K. So far. But the rest still does not explain why. So i suggest adding.... One application of this PHY is connecting it to the PRU in TI Sitara SoC. This MAC requires a hardware indication of link, rather than the usual phylib callback via adjust link. And this input is active-low. Add a property to indicate that LINK_ST be configured active low. > Please look at a block diagram in this page: > https://software-dl.ti.com/processor-sdk-linux/esd/docs/latest/linux/Foundational_Components/PRU-ICSS/Linux_Drivers/PRU-ICSS_Ethernet.html We are now getting close to having all the pieces of the puzzle to decide if this is the right or wrong way to do this. This appears to be the 'Vendor Crap' driver. You are patching mainline here, so it is the mainline PRU driver we care about: https://lore.kernel.org/netdev/20230424053233.2338782-1-danishanwar@ti.com/T/ Looking at the device tree binding, it uses the standard phy-handle, phy-mode properties. There is also emac_adjust_link() which is used by phylib to tell the MAC driver the link is down. So you now need to convince me this change is actually needed in mainline. Thanks Andrew
On 2023/06/01 23:45, Andrew Lunn wrote: > We are now getting close to having all the pieces of the puzzle to > decide if this is the right or wrong way to do this. > > This appears to be the 'Vendor Crap' driver. You are patching mainline > here, so it is the mainline PRU driver we care about: > > https://lore.kernel.org/netdev/20230424053233.2338782-1-danishanwar@ti.com/T/ > > Looking at the device tree binding, it uses the standard phy-handle, > phy-mode properties. There is also emac_adjust_link() which is used by > phylib to tell the MAC driver the link is down. > > So you now need to convince me this change is actually needed in > mainline. Looking for purpose of MII_RXLINK signal, it seems like just for EtherCAT's "enhanced link detection" feature. refer: https://software-dl.ti.com/processor-industrial-sw/esd/docs/indsw/EtherCAT_Slave/PRU_ICSS_EtherCAT_firmware_API_guide.html#pru-icss-mdio-host-side-apis So maybe standard linux driver can ignore this. Then, what really needed is controlling just an LED, i.e. could be done using the LED subsystem as your advice. I will try it if I use this combination of devices in the future. Please drop this patch series for now. --- Atsushi Nemoto
diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml index 929cf8c0b0fd..cd4a1461da1f 100644 --- a/Documentation/devicetree/bindings/net/adi,adin.yaml +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml @@ -52,6 +52,11 @@ properties: description: Enable 25MHz reference clock output on CLK25_REF pin. type: boolean + adi,link-stat-inv: + description: Enable the link status output signal on the LINK_ST pin to be + inverted, meaning that link up is indicated by setting LINK_ST low. + type: boolean + unevaluatedProperties: false examples:
The ADIN1200/ADIN1300 supports inverting the link status output signal on the LINK_ST pin. Add support for selecting this feature via device-tree properties. Signed-off-by: Atsushi Nemoto <atsushi.nemoto@sord.co.jp> --- Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++ 1 file changed, 5 insertions(+)