netdev - RE: [RFC,net-next,v1, 1/1] net: stmmac: Enable SERDES power up/down sequence

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 Apr 2020 14:23:09 +0000
From:   "Voon, Weifeng" <weifeng.voon@...el.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>,
        "David S . Miller" <davem@...emloft.net>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Giuseppe Cavallaro" <peppe.cavallaro@...com>,
        Andrew Lunn <andrew@...n.ch>,
        "Alexandre Torgue" <alexandre.torgue@...com>,
        "Ong, Boon Leong" <boon.leong.ong@...el.com>
Subject: RE: [RFC,net-next,v1, 1/1] net: stmmac: Enable SERDES power up/down
 sequence

> > > > > This patch is to enable Intel SERDES power up/down sequence. The
> > > > > SERDES converts 8/10 bits data to SGMII signal. Below is an
> > > > > example of HW configuration for SGMII mode. The SERDES is
> > > > > located in the PHY IF in the diagram below.
> > > > >
> > > > > <-----------------GBE Controller---------->|<--External PHY
> > > > > chip-->
> > > > > +----------+         +----+            +---+           +--------
> --
> > +
> > > > > |   EQoS   | <-GMII->| DW | < ------ > |PHY| <-SGMII-> |
> External
> > |
> > > > > |   MAC    |         |xPCS|            |IF |           | PHY
> > |
> > > > > +----------+         +----+            +---+           +--------
> --
> > +
> > > > >        ^               ^                 ^                ^
> > > > >        |               |                 |                |
> > > > >        +---------------------MDIO-------------------------+
> > > > >
> > > > > PHY IF configuration and status registers are accessible through
> > > > > mdio address 0x15 which is defined as intel_adhoc_addr. During
> > > > > D0, The driver will need to power up PHY IF by changing the
> > > > > power
> > state to P0.
> > > > > Likewise, for D3, the driver sets PHY IF power state to P3.
> > > >
> > > > I don't think this is the right approach.
> > > >
> > > > You could just add a new "mdio-intel-serdes" to phy/ folder just
> > > > like I did with XPCS because this is mostly related with PHY
> > > > settings rather than EQoS.
> > > I am taking this approach to put it in stmmac folder rather than phy
> > > folder as a generic mdio-intel-serdes as this is a specific Intel
> > > serdes architecture which would only pair with DW EQos and DW xPCS
> HW.
> > > Since this serdes will not able to pair other MAC or other non-Intel
> > > platform, I would like you to reconsider this approach. I am open
> > > for
> > discussion.
> > > Thanks Jose for the fast response.
> >
> > OK, then I think we should use the BSP init/exit functions that are
> > already available for platform setups (.init and .exit callback of
> > plat_stmmacenet_data struct). We just need to extend this to PCI based
> > setups.
> >
> > You can take a look at stmmac_platform.c and check what's done.
> > Basically:
> > 	- Call priv->plat->init() at probe() and resume()
> > 	- Call priv->plat->exit() at remove() and suspend()
> >
> I have 2 concern if using the suggested BSP init/exit function.
> 1. Serdes is configured through MDIO bus. But the mdio bus register only
> happens in stmmac_dvr_probe() in stmmac_main.c.
> 
> 2. All tx/rx packets requires serdes to be in the correct power state.
> If the driver power-down before stopping all the dma, it will cause tx
> queue timeout as packets are not able to be transmitted out. Hence, the
> serdes cannot be power-down before calling the stmmac_dvr_remove(). The
> stmmac_dvr_remove() will unregister the mdio bus. So, the
> driver cannot powerdown the serdes after the stmmac_dvr_remove() too.

I went through the code again, I understand that your intention is to keep the
platform specific setup in its own file and keep the main dwmac logic clean.
But, I did not see any way to separate this SERDES configuration from the 
stmmac_main logic cleanly. 
Hope to get more ideas and discussion. Thanks.

Weifeng

> 
> Regards,
> Weifeng
> 
> > ---
> > Thanks,
> > Jose Miguel Abreu