diff mbox series

[net-next,v9,8/9] net: txgbe: Implement phylink pcs

Message ID 20230524091722.522118-9-jiawenwu@trustnetic.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series TXGBE PHYLINK support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 5 maintainers not CCed: kuba@kernel.org piotr.raczynski@intel.com davem@davemloft.net pabeni@redhat.com edumazet@google.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: line length of 90 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiawen Wu May 24, 2023, 9:17 a.m. UTC
Register MDIO bus for PCS layer to use Synopsys designware XPCS, support
10GBASE-R interface to the controller.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/Kconfig          |  1 +
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    | 97 ++++++++++++++++++-
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  5 +
 3 files changed, 101 insertions(+), 2 deletions(-)

Comments

Andrew Lunn May 24, 2023, 12:53 p.m. UTC | #1
On Wed, May 24, 2023 at 05:17:21PM +0800, Jiawen Wu wrote:
> Register MDIO bus for PCS layer to use Synopsys designware XPCS, support
> 10GBASE-R interface to the controller.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Jakub Kicinski May 26, 2023, 4:14 a.m. UTC | #2
On Wed, 24 May 2023 17:17:21 +0800 Jiawen Wu wrote:
> +	ret = devm_mdiobus_register(&pdev->dev, mii_bus);
> +	if (ret)
> +		return ret;
> +
> +	mdiodev = mdio_device_create(mii_bus, 0);
> +	if (IS_ERR(mdiodev))
> +		return PTR_ERR(mdiodev);
> +
> +	xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER);
> +	if (IS_ERR(xpcs)) {
> +		mdio_device_free(mdiodev);
> +		return PTR_ERR(xpcs);
> +	}

How does the mdiodev get destroyed in case of success?
Seems like either freeing it in case of xpcs error is unnecessary 
or it needs to also be freed when xpcs is destroyed?
Jiawen Wu May 26, 2023, 6:21 a.m. UTC | #3
On Friday, May 26, 2023 12:14 PM, Jakub Kicinski wrote:
> On Wed, 24 May 2023 17:17:21 +0800 Jiawen Wu wrote:
> > +	ret = devm_mdiobus_register(&pdev->dev, mii_bus);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mdiodev = mdio_device_create(mii_bus, 0);
> > +	if (IS_ERR(mdiodev))
> > +		return PTR_ERR(mdiodev);
> > +
> > +	xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER);
> > +	if (IS_ERR(xpcs)) {
> > +		mdio_device_free(mdiodev);
> > +		return PTR_ERR(xpcs);
> > +	}
> 
> How does the mdiodev get destroyed in case of success?
> Seems like either freeing it in case of xpcs error is unnecessary
> or it needs to also be freed when xpcs is destroyed?

When xpcs is destroyed, that means mdiodev is no longer needed.
I think there is no need to free mdiodev in case of xpcs error,
since devm_* function leads to free it.
Russell King (Oracle) May 26, 2023, 8:43 a.m. UTC | #4
On Fri, May 26, 2023 at 02:21:23PM +0800, Jiawen Wu wrote:
> On Friday, May 26, 2023 12:14 PM, Jakub Kicinski wrote:
> > On Wed, 24 May 2023 17:17:21 +0800 Jiawen Wu wrote:
> > > +	ret = devm_mdiobus_register(&pdev->dev, mii_bus);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	mdiodev = mdio_device_create(mii_bus, 0);
> > > +	if (IS_ERR(mdiodev))
> > > +		return PTR_ERR(mdiodev);
> > > +
> > > +	xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER);
> > > +	if (IS_ERR(xpcs)) {
> > > +		mdio_device_free(mdiodev);
> > > +		return PTR_ERR(xpcs);
> > > +	}
> > 
> > How does the mdiodev get destroyed in case of success?
> > Seems like either freeing it in case of xpcs error is unnecessary
> > or it needs to also be freed when xpcs is destroyed?
> 
> When xpcs is destroyed, that means mdiodev is no longer needed.
> I think there is no need to free mdiodev in case of xpcs error,
> since devm_* function leads to free it.

If you are relying on the devm-ness of devm_mdiobus_register() then
it won't. Although mdiobus_unregister() walks bus->mdio_map[], I
think you are assuming that the mdio device you've created in
mdio_device_create() will be in that array. MDIO devices only get
added to that array when mdiobus_register_device() has been called,
which must only be called from mdio_device_register().

Please arrange to call mdio_device_free() prior to destroying the
XPCS in every case.
Jiawen Wu May 26, 2023, 9:01 a.m. UTC | #5
On Friday, May 26, 2023 4:43 PM, Russell King (Oracle) wrote:
> On Fri, May 26, 2023 at 02:21:23PM +0800, Jiawen Wu wrote:
> > On Friday, May 26, 2023 12:14 PM, Jakub Kicinski wrote:
> > > On Wed, 24 May 2023 17:17:21 +0800 Jiawen Wu wrote:
> > > > +	ret = devm_mdiobus_register(&pdev->dev, mii_bus);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	mdiodev = mdio_device_create(mii_bus, 0);
> > > > +	if (IS_ERR(mdiodev))
> > > > +		return PTR_ERR(mdiodev);
> > > > +
> > > > +	xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER);
> > > > +	if (IS_ERR(xpcs)) {
> > > > +		mdio_device_free(mdiodev);
> > > > +		return PTR_ERR(xpcs);
> > > > +	}
> > >
> > > How does the mdiodev get destroyed in case of success?
> > > Seems like either freeing it in case of xpcs error is unnecessary
> > > or it needs to also be freed when xpcs is destroyed?
> >
> > When xpcs is destroyed, that means mdiodev is no longer needed.
> > I think there is no need to free mdiodev in case of xpcs error,
> > since devm_* function leads to free it.
> 
> If you are relying on the devm-ness of devm_mdiobus_register() then
> it won't. Although mdiobus_unregister() walks bus->mdio_map[], I
> think you are assuming that the mdio device you've created in
> mdio_device_create() will be in that array. MDIO devices only get
> added to that array when mdiobus_register_device() has been called,
> which must only be called from mdio_device_register().
> 
> Please arrange to call mdio_device_free() prior to destroying the
> XPCS in every case.

Get it.
Russell King (Oracle) May 26, 2023, 9:07 a.m. UTC | #6
On Fri, May 26, 2023 at 05:01:49PM +0800, Jiawen Wu wrote:
> On Friday, May 26, 2023 4:43 PM, Russell King (Oracle) wrote:
> > On Fri, May 26, 2023 at 02:21:23PM +0800, Jiawen Wu wrote:
> > > On Friday, May 26, 2023 12:14 PM, Jakub Kicinski wrote:
> > > > On Wed, 24 May 2023 17:17:21 +0800 Jiawen Wu wrote:
> > > > > +	ret = devm_mdiobus_register(&pdev->dev, mii_bus);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	mdiodev = mdio_device_create(mii_bus, 0);
> > > > > +	if (IS_ERR(mdiodev))
> > > > > +		return PTR_ERR(mdiodev);
> > > > > +
> > > > > +	xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER);
> > > > > +	if (IS_ERR(xpcs)) {
> > > > > +		mdio_device_free(mdiodev);
> > > > > +		return PTR_ERR(xpcs);
> > > > > +	}
> > > >
> > > > How does the mdiodev get destroyed in case of success?
> > > > Seems like either freeing it in case of xpcs error is unnecessary
> > > > or it needs to also be freed when xpcs is destroyed?
> > >
> > > When xpcs is destroyed, that means mdiodev is no longer needed.
> > > I think there is no need to free mdiodev in case of xpcs error,
> > > since devm_* function leads to free it.
> > 
> > If you are relying on the devm-ness of devm_mdiobus_register() then
> > it won't. Although mdiobus_unregister() walks bus->mdio_map[], I
> > think you are assuming that the mdio device you've created in
> > mdio_device_create() will be in that array. MDIO devices only get
> > added to that array when mdiobus_register_device() has been called,
> > which must only be called from mdio_device_register().
> > 
> > Please arrange to call mdio_device_free() prior to destroying the
> > XPCS in every case.
> 
> Get it.

It seems this is becoming a pattern, so I think we need to solve it
differently. How about something like this, which means you only have
to care about calling xpcs_create_mdiodev() and xpcs_destroy() ?

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index b87c69c4cdd7..802222581feb 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1240,6 +1240,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
 	if (!xpcs)
 		return ERR_PTR(-ENOMEM);
 
+	mdio_device_get(mdiodev);
 	xpcs->mdiodev = mdiodev;
 
 	xpcs_id = xpcs_get_id(xpcs);
@@ -1272,6 +1273,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
 	ret = -ENODEV;
 
 out:
+	mdio_device_put(mdiodev);
 	kfree(xpcs);
 
 	return ERR_PTR(ret);
@@ -1280,8 +1282,33 @@ EXPORT_SYMBOL_GPL(xpcs_create);
 
 void xpcs_destroy(struct dw_xpcs *xpcs)
 {
+	mdio_device_put(mdiodev);
 	kfree(xpcs);
 }
 EXPORT_SYMBOL_GPL(xpcs_destroy);
 
+struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
+				    phy_interface_t interface)
+{
+	struct mdio_device *mdiodev;
+	struct dw_xpcs *xpcs;
+
+	mdiodev = mdio_device_create(bus, addr);
+	if (IS_ERR(mdiodev))
+		return ERR_CAST(mdiodev);
+
+	xpcs = xpcs_create(mdiodev, interface);
+
+	/* xpcs_create() has taken a refcount on the mdiodev if it was
+	 * successful. If xpcs_create() fails, this will free the mdio
+	 * device here. In any case, we don't need to hold our reference
+	 * anymore, and putting it here will allow mdio_device_put() in
+	 * xpcs_destroy() to automatically free the mdio device.
+	 */
+	mdio_device_put(mdiodev);
+
+	return xpcs;
+}
+EXPORT_SYMBOL_GPL(xpcs_create_mdiodev);
+
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 1d7d550bbf1a..537b62330c90 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -108,6 +108,16 @@ int mdio_driver_register(struct mdio_driver *drv);
 void mdio_driver_unregister(struct mdio_driver *drv);
 int mdio_device_bus_match(struct device *dev, struct device_driver *drv);
 
+static inline void mdio_device_get(struct mdio_device *mdiodev)
+{
+	get_device(&mdiodev->dev);
+}
+
+static inline void mdio_device_put(struct mdio_device *mdiodev)
+{
+	mdio_device_free(mdiodev);
+}
+
 static inline bool mdio_phy_id_is_c45(int phy_id)
 {
 	return (phy_id & MDIO_PHY_ID_C45) && !(phy_id & ~MDIO_PHY_ID_C45_MASK);
Jiawen Wu May 26, 2023, 9:22 a.m. UTC | #7
On Friday, May 26, 2023 5:07 PM, Russell King (Oracle) wrote:
> On Fri, May 26, 2023 at 05:01:49PM +0800, Jiawen Wu wrote:
> > On Friday, May 26, 2023 4:43 PM, Russell King (Oracle) wrote:
> > > On Fri, May 26, 2023 at 02:21:23PM +0800, Jiawen Wu wrote:
> > > > On Friday, May 26, 2023 12:14 PM, Jakub Kicinski wrote:
> > > > > On Wed, 24 May 2023 17:17:21 +0800 Jiawen Wu wrote:
> > > > > > +	ret = devm_mdiobus_register(&pdev->dev, mii_bus);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	mdiodev = mdio_device_create(mii_bus, 0);
> > > > > > +	if (IS_ERR(mdiodev))
> > > > > > +		return PTR_ERR(mdiodev);
> > > > > > +
> > > > > > +	xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER);
> > > > > > +	if (IS_ERR(xpcs)) {
> > > > > > +		mdio_device_free(mdiodev);
> > > > > > +		return PTR_ERR(xpcs);
> > > > > > +	}
> > > > >
> > > > > How does the mdiodev get destroyed in case of success?
> > > > > Seems like either freeing it in case of xpcs error is unnecessary
> > > > > or it needs to also be freed when xpcs is destroyed?
> > > >
> > > > When xpcs is destroyed, that means mdiodev is no longer needed.
> > > > I think there is no need to free mdiodev in case of xpcs error,
> > > > since devm_* function leads to free it.
> > >
> > > If you are relying on the devm-ness of devm_mdiobus_register() then
> > > it won't. Although mdiobus_unregister() walks bus->mdio_map[], I
> > > think you are assuming that the mdio device you've created in
> > > mdio_device_create() will be in that array. MDIO devices only get
> > > added to that array when mdiobus_register_device() has been called,
> > > which must only be called from mdio_device_register().
> > >
> > > Please arrange to call mdio_device_free() prior to destroying the
> > > XPCS in every case.
> >
> > Get it.
> 
> It seems this is becoming a pattern, so I think we need to solve it
> differently. How about something like this, which means you only have
> to care about calling xpcs_create_mdiodev() and xpcs_destroy() ?
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index b87c69c4cdd7..802222581feb 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -1240,6 +1240,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
>  	if (!xpcs)
>  		return ERR_PTR(-ENOMEM);
> 
> +	mdio_device_get(mdiodev);
>  	xpcs->mdiodev = mdiodev;
> 
>  	xpcs_id = xpcs_get_id(xpcs);
> @@ -1272,6 +1273,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
>  	ret = -ENODEV;
> 
>  out:
> +	mdio_device_put(mdiodev);
>  	kfree(xpcs);
> 
>  	return ERR_PTR(ret);
> @@ -1280,8 +1282,33 @@ EXPORT_SYMBOL_GPL(xpcs_create);
> 
>  void xpcs_destroy(struct dw_xpcs *xpcs)
>  {
> +	mdio_device_put(mdiodev);
>  	kfree(xpcs);
>  }
>  EXPORT_SYMBOL_GPL(xpcs_destroy);
> 
> +struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
> +				    phy_interface_t interface)
> +{
> +	struct mdio_device *mdiodev;
> +	struct dw_xpcs *xpcs;
> +
> +	mdiodev = mdio_device_create(bus, addr);
> +	if (IS_ERR(mdiodev))
> +		return ERR_CAST(mdiodev);
> +
> +	xpcs = xpcs_create(mdiodev, interface);
> +
> +	/* xpcs_create() has taken a refcount on the mdiodev if it was
> +	 * successful. If xpcs_create() fails, this will free the mdio
> +	 * device here. In any case, we don't need to hold our reference
> +	 * anymore, and putting it here will allow mdio_device_put() in
> +	 * xpcs_destroy() to automatically free the mdio device.
> +	 */
> +	mdio_device_put(mdiodev);
> +
> +	return xpcs;
> +}
> +EXPORT_SYMBOL_GPL(xpcs_create_mdiodev);
> +
>  MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mdio.h b/include/linux/mdio.h
> index 1d7d550bbf1a..537b62330c90 100644
> --- a/include/linux/mdio.h
> +++ b/include/linux/mdio.h
> @@ -108,6 +108,16 @@ int mdio_driver_register(struct mdio_driver *drv);
>  void mdio_driver_unregister(struct mdio_driver *drv);
>  int mdio_device_bus_match(struct device *dev, struct device_driver *drv);
> 
> +static inline void mdio_device_get(struct mdio_device *mdiodev)
> +{
> +	get_device(&mdiodev->dev);
> +}
> +
> +static inline void mdio_device_put(struct mdio_device *mdiodev)
> +{
> +	mdio_device_free(mdiodev);
> +}
> +
>  static inline bool mdio_phy_id_is_c45(int phy_id)
>  {
>  	return (phy_id & MDIO_PHY_ID_C45) && !(phy_id & ~MDIO_PHY_ID_C45_MASK);

Looks great, it can eliminate to create mdiodev in the ethernet driver, this device
only be used in xpcs.
Russell King (Oracle) May 26, 2023, 9:37 a.m. UTC | #8
On Fri, May 26, 2023 at 05:22:29PM +0800, Jiawen Wu wrote:
> On Friday, May 26, 2023 5:07 PM, Russell King (Oracle) wrote:
> > On Fri, May 26, 2023 at 05:01:49PM +0800, Jiawen Wu wrote:
> > > On Friday, May 26, 2023 4:43 PM, Russell King (Oracle) wrote:
> > > > On Fri, May 26, 2023 at 02:21:23PM +0800, Jiawen Wu wrote:
> > > > > On Friday, May 26, 2023 12:14 PM, Jakub Kicinski wrote:
> > > > > > On Wed, 24 May 2023 17:17:21 +0800 Jiawen Wu wrote:
> > > > > > > +	ret = devm_mdiobus_register(&pdev->dev, mii_bus);
> > > > > > > +	if (ret)
> > > > > > > +		return ret;
> > > > > > > +
> > > > > > > +	mdiodev = mdio_device_create(mii_bus, 0);
> > > > > > > +	if (IS_ERR(mdiodev))
> > > > > > > +		return PTR_ERR(mdiodev);
> > > > > > > +
> > > > > > > +	xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER);
> > > > > > > +	if (IS_ERR(xpcs)) {
> > > > > > > +		mdio_device_free(mdiodev);
> > > > > > > +		return PTR_ERR(xpcs);
> > > > > > > +	}
> > > > > >
> > > > > > How does the mdiodev get destroyed in case of success?
> > > > > > Seems like either freeing it in case of xpcs error is unnecessary
> > > > > > or it needs to also be freed when xpcs is destroyed?
> > > > >
> > > > > When xpcs is destroyed, that means mdiodev is no longer needed.
> > > > > I think there is no need to free mdiodev in case of xpcs error,
> > > > > since devm_* function leads to free it.
> > > >
> > > > If you are relying on the devm-ness of devm_mdiobus_register() then
> > > > it won't. Although mdiobus_unregister() walks bus->mdio_map[], I
> > > > think you are assuming that the mdio device you've created in
> > > > mdio_device_create() will be in that array. MDIO devices only get
> > > > added to that array when mdiobus_register_device() has been called,
> > > > which must only be called from mdio_device_register().
> > > >
> > > > Please arrange to call mdio_device_free() prior to destroying the
> > > > XPCS in every case.
> > >
> > > Get it.
> > 
> > It seems this is becoming a pattern, so I think we need to solve it
> > differently. How about something like this, which means you only have
> > to care about calling xpcs_create_mdiodev() and xpcs_destroy() ?
> > 
> > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> > index b87c69c4cdd7..802222581feb 100644
> > --- a/drivers/net/pcs/pcs-xpcs.c
> > +++ b/drivers/net/pcs/pcs-xpcs.c
> > @@ -1240,6 +1240,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> >  	if (!xpcs)
> >  		return ERR_PTR(-ENOMEM);
> > 
> > +	mdio_device_get(mdiodev);
> >  	xpcs->mdiodev = mdiodev;
> > 
> >  	xpcs_id = xpcs_get_id(xpcs);
> > @@ -1272,6 +1273,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> >  	ret = -ENODEV;
> > 
> >  out:
> > +	mdio_device_put(mdiodev);
> >  	kfree(xpcs);
> > 
> >  	return ERR_PTR(ret);
> > @@ -1280,8 +1282,33 @@ EXPORT_SYMBOL_GPL(xpcs_create);
> > 
> >  void xpcs_destroy(struct dw_xpcs *xpcs)
> >  {
> > +	mdio_device_put(mdiodev);
> >  	kfree(xpcs);
> >  }
> >  EXPORT_SYMBOL_GPL(xpcs_destroy);
> > 
> > +struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr,
> > +				    phy_interface_t interface)
> > +{
> > +	struct mdio_device *mdiodev;
> > +	struct dw_xpcs *xpcs;
> > +
> > +	mdiodev = mdio_device_create(bus, addr);
> > +	if (IS_ERR(mdiodev))
> > +		return ERR_CAST(mdiodev);
> > +
> > +	xpcs = xpcs_create(mdiodev, interface);
> > +
> > +	/* xpcs_create() has taken a refcount on the mdiodev if it was
> > +	 * successful. If xpcs_create() fails, this will free the mdio
> > +	 * device here. In any case, we don't need to hold our reference
> > +	 * anymore, and putting it here will allow mdio_device_put() in
> > +	 * xpcs_destroy() to automatically free the mdio device.
> > +	 */
> > +	mdio_device_put(mdiodev);
> > +
> > +	return xpcs;
> > +}
> > +EXPORT_SYMBOL_GPL(xpcs_create_mdiodev);
> > +
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/mdio.h b/include/linux/mdio.h
> > index 1d7d550bbf1a..537b62330c90 100644
> > --- a/include/linux/mdio.h
> > +++ b/include/linux/mdio.h
> > @@ -108,6 +108,16 @@ int mdio_driver_register(struct mdio_driver *drv);
> >  void mdio_driver_unregister(struct mdio_driver *drv);
> >  int mdio_device_bus_match(struct device *dev, struct device_driver *drv);
> > 
> > +static inline void mdio_device_get(struct mdio_device *mdiodev)
> > +{
> > +	get_device(&mdiodev->dev);
> > +}
> > +
> > +static inline void mdio_device_put(struct mdio_device *mdiodev)
> > +{
> > +	mdio_device_free(mdiodev);
> > +}
> > +
> >  static inline bool mdio_phy_id_is_c45(int phy_id)
> >  {
> >  	return (phy_id & MDIO_PHY_ID_C45) && !(phy_id & ~MDIO_PHY_ID_C45_MASK);
> 
> Looks great, it can eliminate to create mdiodev in the ethernet driver, this device
> only be used in xpcs.

I'm just creating a patch series for both xpcs and lynx, which this
morning have had patches identifying similar problems with creation
and destruction.
Russell King (Oracle) May 26, 2023, 10:42 a.m. UTC | #9
On Fri, May 26, 2023 at 10:37:04AM +0100, Russell King (Oracle) wrote:
> I'm just creating a patch series for both xpcs and lynx, which this
> morning have had patches identifying similar problems with creation
> and destruction.

https://lore.kernel.org/all/ZHCGZ8IgAAwr8bla@shell.armlinux.org.uk/
Jiawen Wu May 29, 2023, 1:57 a.m. UTC | #10
On Friday, May 26, 2023 6:42 PM, Russell King (Oracle) wrote:
> On Fri, May 26, 2023 at 10:37:04AM +0100, Russell King (Oracle) wrote:
> > I'm just creating a patch series for both xpcs and lynx, which this
> > morning have had patches identifying similar problems with creation
> > and destruction.
> 
> https://lore.kernel.org/all/ZHCGZ8IgAAwr8bla@shell.armlinux.org.uk/

OK, I will send the updated patches after this patch series merged.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig
index 73f4492928c0..f3fb273e6fd0 100644
--- a/drivers/net/ethernet/wangxun/Kconfig
+++ b/drivers/net/ethernet/wangxun/Kconfig
@@ -45,6 +45,7 @@  config TXGBE
 	select GPIOLIB
 	select REGMAP
 	select COMMON_CLK
+	select PCS_XPCS
 	select LIBWX
 	select SFP
 	help
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index 6aba22a4fc5e..d2a6f8ca78e7 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -8,6 +8,8 @@ 
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
 #include <linux/regmap.h>
+#include <linux/pcs/pcs-xpcs.h>
+#include <linux/mdio.h>
 #include <linux/i2c.h>
 #include <linux/pci.h>
 
@@ -77,6 +79,88 @@  static int txgbe_swnodes_register(struct txgbe *txgbe)
 	return software_node_register_node_group(nodes->group);
 }
 
+static int txgbe_pcs_read(struct mii_bus *bus, int addr, int devnum, int regnum)
+{
+	struct wx *wx  = bus->priv;
+	u32 offset, val;
+
+	if (addr)
+		return -EOPNOTSUPP;
+
+	offset = devnum << 16 | regnum;
+
+	/* Set the LAN port indicator to IDA_ADDR */
+	wr32(wx, TXGBE_XPCS_IDA_ADDR, offset);
+
+	/* Read the data from IDA_DATA register */
+	val = rd32(wx, TXGBE_XPCS_IDA_DATA);
+
+	return (u16)val;
+}
+
+static int txgbe_pcs_write(struct mii_bus *bus, int addr, int devnum, int regnum, u16 val)
+{
+	struct wx *wx = bus->priv;
+	u32 offset;
+
+	if (addr)
+		return -EOPNOTSUPP;
+
+	offset = devnum << 16 | regnum;
+
+	/* Set the LAN port indicator to IDA_ADDR */
+	wr32(wx, TXGBE_XPCS_IDA_ADDR, offset);
+
+	/* Write the data to IDA_DATA register */
+	wr32(wx, TXGBE_XPCS_IDA_DATA, val);
+
+	return 0;
+}
+
+static int txgbe_mdio_pcs_init(struct txgbe *txgbe)
+{
+	struct mdio_device *mdiodev;
+	struct mii_bus *mii_bus;
+	struct dw_xpcs *xpcs;
+	struct pci_dev *pdev;
+	struct wx *wx;
+	int ret = 0;
+
+	wx = txgbe->wx;
+	pdev = wx->pdev;
+
+	mii_bus = devm_mdiobus_alloc(&pdev->dev);
+	if (!mii_bus)
+		return -ENOMEM;
+
+	mii_bus->name = "txgbe_pcs_mdio_bus";
+	mii_bus->read_c45 = &txgbe_pcs_read;
+	mii_bus->write_c45 = &txgbe_pcs_write;
+	mii_bus->parent = &pdev->dev;
+	mii_bus->phy_mask = ~0;
+	mii_bus->priv = wx;
+	snprintf(mii_bus->id, MII_BUS_ID_SIZE, "txgbe_pcs-%x",
+		 (pdev->bus->number << 8) | pdev->devfn);
+
+	ret = devm_mdiobus_register(&pdev->dev, mii_bus);
+	if (ret)
+		return ret;
+
+	mdiodev = mdio_device_create(mii_bus, 0);
+	if (IS_ERR(mdiodev))
+		return PTR_ERR(mdiodev);
+
+	xpcs = xpcs_create(mdiodev, PHY_INTERFACE_MODE_10GBASER);
+	if (IS_ERR(xpcs)) {
+		mdio_device_free(mdiodev);
+		return PTR_ERR(xpcs);
+	}
+
+	txgbe->xpcs = xpcs;
+
+	return 0;
+}
+
 static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct wx *wx = gpiochip_get_data(chip);
@@ -429,16 +513,22 @@  int txgbe_init_phy(struct txgbe *txgbe)
 		return ret;
 	}
 
+	ret = txgbe_mdio_pcs_init(txgbe);
+	if (ret) {
+		wx_err(txgbe->wx, "failed to init mdio pcs: %d\n", ret);
+		goto err_unregister_swnode;
+	}
+
 	ret = txgbe_gpio_init(txgbe);
 	if (ret) {
 		wx_err(txgbe->wx, "failed to init gpio\n");
-		goto err_unregister_swnode;
+		goto err_destroy_xpcs;
 	}
 
 	ret = txgbe_clock_register(txgbe);
 	if (ret) {
 		wx_err(txgbe->wx, "failed to register clock: %d\n", ret);
-		goto err_unregister_swnode;
+		goto err_destroy_xpcs;
 	}
 
 	ret = txgbe_i2c_register(txgbe);
@@ -460,6 +550,8 @@  int txgbe_init_phy(struct txgbe *txgbe)
 err_unregister_clk:
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
+err_destroy_xpcs:
+	xpcs_destroy(txgbe->xpcs);
 err_unregister_swnode:
 	software_node_unregister_node_group(txgbe->nodes.group);
 
@@ -472,5 +564,6 @@  void txgbe_remove_phy(struct txgbe *txgbe)
 	platform_device_unregister(txgbe->i2c_dev);
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
+	xpcs_destroy(txgbe->xpcs);
 	software_node_unregister_node_group(txgbe->nodes.group);
 }
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 6c903e4517c7..d1f62f62c28c 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -80,6 +80,10 @@ 
 /* I2C registers */
 #define TXGBE_I2C_BASE                          0x14900
 
+/************************************** ETH PHY ******************************/
+#define TXGBE_XPCS_IDA_ADDR                     0x13000
+#define TXGBE_XPCS_IDA_DATA                     0x13004
+
 /* Part Number String Length */
 #define TXGBE_PBANUM_LENGTH                     32
 
@@ -171,6 +175,7 @@  struct txgbe_nodes {
 struct txgbe {
 	struct wx *wx;
 	struct txgbe_nodes nodes;
+	struct dw_xpcs *xpcs;
 	struct platform_device *sfp_dev;
 	struct platform_device *i2c_dev;
 	struct clk_lookup *clock;