Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Jose Abreu <Jose.Abreu@synopsys.com>
To:     "Voon, Weifeng" <weifeng.voon@intel.com>,
        "David S . Miller" <davem@davemloft.net>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>
CC:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Giuseppe Cavallaro" <peppe.cavallaro@st.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Alexandre Torgue <alexandre.torgue@st.com>,
        "Ong, Boon Leong" <boon.leong.ong@intel.com>
Subject: RE: [RFC,net-next,v1, 1/1] net: stmmac: Enable SERDES power up/down
 sequence
Thread-Topic: [RFC,net-next,v1, 1/1] net: stmmac: Enable SERDES power up/down
 sequence
Thread-Index: AQHWAE03JZEV2gP9CkiKXaQcDq7OzahVzGeAgAAJs4CAAAleQIAAdfeAgA4MGYCAATPU0A==
Date:   Thu, 2 Apr 2020 08:47:19 +0000
Message-ID: <BN8PR12MB326674C44D4D44C3D2B86C73D3C60@BN8PR12MB3266.namprd12.prod.outlook.com>
References: <20200322132342.2687-1-weifeng.voon@intel.com>
 <20200322132342.2687-2-weifeng.voon@intel.com>
 <BN8PR12MB3266ACFFA4808A133BB72F9DD3F00@BN8PR12MB3266.namprd12.prod.outlook.com>
 <BYAPR11MB27575EF05D65A8AA9AF4128488F00@BYAPR11MB2757.namprd11.prod.outlook.com>
 <BN8PR12MB326606DE1FEB055B7361A939D3F00@BN8PR12MB3266.namprd12.prod.outlook.com>
 <BYAPR11MB2757B80101035B9A599E357B88F00@BYAPR11MB2757.namprd11.prod.outlook.com>
 <BYAPR11MB3125359A86ECE3A845595E3888C90@BYAPR11MB3125.namprd11.prod.outlook.com>
In-Reply-To: <BYAPR11MB3125359A86ECE3A845595E3888C90@BYAPR11MB3125.namprd11.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: synopsys.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 1c101dfb-54be-4a6f-c844-08d7d6e2740e
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Apr 2020 08:47:19.3024
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: c33c9f88-1eb7-4099-9700-16013fd9e8aa
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: /vYqAOI8RIoPbiUv/qqNf+lol7fIpwzMZt/8Tlwur30/k8QzPOsjk6B9/rSczfJyg8l/LXfGJbVp/O4x5DuVDA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR12MB3091
X-OriginatorOrg: synopsys.com
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Voon, Weifeng <weifeng.voon@intel.com>
Date: Apr/01/2020, 15:23:09 (UTC+00:00)

> > > > > > This patch is to enable Intel SERDES power up/down sequence. Th=
e
> > > > > > 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 throug=
h
> > > > > > 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 ph=
y
> > > > 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-Inte=
l
> > > > 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 base=
d
> > > 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 onl=
y
> > happens in stmmac_dvr_probe() in stmmac_main.c.
> >=20
> > 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.
>=20
> I went through the code again, I understand that your intention is to kee=
p the
> platform specific setup in its own file and keep the main dwmac logic cle=
an.
> But, I did not see any way to separate this SERDES configuration from the=
=20
> stmmac_main logic cleanly.=20
> Hope to get more ideas and discussion. Thanks.

Sorry for the late reply!

Yes, that's exactly my intention. Maybe we can add a new set of callbacks=20
for BSP. Something like ->start(), ->stop() ?

---
Thanks,
Jose Miguel Abreu