| Message ID | 20200621225451.12435-6-ioana.ciornei@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: add Lynx PCS MDIO module | expand |
On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote: > In order to support split PCS using PHYLINK properly, we need to add a > phylink_pcs_ops structure. > > Note that a DSA driver that wants to use these needs to implement all 4 > of them: the DSA core checks the presence of these 4 function pointers > in dsa_switch_ops and only then does it add a PCS to PHYLINK. This is > done in order to preserve compatibility with drivers that have not yet > been converted, or don't need, a split PCS setup. > > Also, when pcs_get_state() and pcs_an_restart() are present, their mac > counterparts (mac_pcs_get_state(), mac_an_restart()) will no longer get > called, as can be seen in phylink.c. I don't like this at all, it means we've got all this useless layering, and that layering will force similar layering veneers into other parts of the kernel (such as the DPAA2 MAC driver, when we eventually come to re-use pcs-lynx there.) I don't think we need that - I think we can get to a position where pcs-lynx is called requesting that it bind to phylink as the PCS, and it calls phylink_add_pcs() directly, which means we do not end up with veneers in DSA nor in the DPAA2 MAC driver - they just need to call the pcs-lynx initialisation function with the phylink instance for it to attach to. Yes, that requires phylink_add_pcs() to change slightly, and for there to be a PCS private pointer, as I have previously stated. At the moment, changing that is really easy - the phylink PCS backend has no in-tree users at the moment. So there is no reason not to get this correct right now.
On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux admin wrote: > On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote: > > In order to support split PCS using PHYLINK properly, we need to add a > > phylink_pcs_ops structure. > > > > Note that a DSA driver that wants to use these needs to implement all 4 > > of them: the DSA core checks the presence of these 4 function pointers > > in dsa_switch_ops and only then does it add a PCS to PHYLINK. This is > > done in order to preserve compatibility with drivers that have not yet > > been converted, or don't need, a split PCS setup. > > > > Also, when pcs_get_state() and pcs_an_restart() are present, their mac > > counterparts (mac_pcs_get_state(), mac_an_restart()) will no longer get > > called, as can be seen in phylink.c. > > I don't like this at all, it means we've got all this useless layering, > and that layering will force similar layering veneers into other parts > of the kernel (such as the DPAA2 MAC driver, when we eventually come to > re-use pcs-lynx there.) > > I don't think we need that - I think we can get to a position where > pcs-lynx is called requesting that it bind to phylink as the PCS, and > it calls phylink_add_pcs() directly, which means we do not end up with > veneers in DSA nor in the DPAA2 MAC driver - they just need to call > the pcs-lynx initialisation function with the phylink instance for it > to attach to. > > Yes, that requires phylink_add_pcs() to change slightly, and for there > to be a PCS private pointer, as I have previously stated. At the > moment, changing that is really easy - the phylink PCS backend has no > in-tree users at the moment. So there is no reason not to get this > correct right now. How about something like this? I don't want to embed a mdio_device inside struct phylink_pcs because that would not be appropriate for some potential users (e.g., Marvell 88E6xxx DSA drivers, Marvell NETA, PP2, etc.) From: Russell King <rmk+kernel@armlinux.org.uk> Subject: [PATCH net-next] net: phylink: add struct phylink_pcs Add a way for MAC PCS to have private data while keeping independence from struct phylink_config, which is used for the MAC itself. We need this independence as we will have stand-alone code for PCS that is independent of the MAC. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phylink.c | 23 +++++++++++++++------- include/linux/phylink.h | 41 ++++++++++++++++++++++++++------------- 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 7ce787c227b3..37ed7455652a 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -43,6 +43,7 @@ struct phylink { const struct phylink_mac_ops *mac_ops; const struct phylink_pcs_ops *pcs_ops; struct phylink_config *config; + struct phylink_pcs *pcs; struct device *dev; unsigned int old_link_state:1; @@ -431,7 +432,7 @@ static void phylink_mac_pcs_an_restart(struct phylink *pl) if (pl->link_config.an_enabled && phy_interface_mode_is_8023z(pl->link_config.interface)) { if (pl->pcs_ops) - pl->pcs_ops->pcs_an_restart(pl->config); + pl->pcs_ops->pcs_an_restart(pl->pcs); else pl->mac_ops->mac_an_restart(pl->config); } @@ -442,7 +443,7 @@ static void phylink_pcs_config(struct phylink *pl, bool force_restart, { bool restart = force_restart; - if (pl->pcs_ops && pl->pcs_ops->pcs_config(pl->config, + if (pl->pcs_ops && pl->pcs_ops->pcs_config(pl->pcs, pl->cur_link_an_mode, state->interface, state->advertising)) @@ -468,7 +469,7 @@ static void phylink_mac_pcs_get_state(struct phylink *pl, state->link = 1; if (pl->pcs_ops) - pl->pcs_ops->pcs_get_state(pl->config, state); + pl->pcs_ops->pcs_get_state(pl->pcs, state); else pl->mac_ops->mac_pcs_get_state(pl->config, state); } @@ -539,7 +540,7 @@ static void phylink_link_up(struct phylink *pl, pl->cur_interface = link_state.interface; if (pl->pcs_ops && pl->pcs_ops->pcs_link_up) - pl->pcs_ops->pcs_link_up(pl->config, pl->cur_link_an_mode, + pl->pcs_ops->pcs_link_up(pl->pcs, pl->cur_link_an_mode, pl->cur_interface, link_state.speed, link_state.duplex); @@ -777,11 +778,19 @@ struct phylink *phylink_create(struct phylink_config *config, } EXPORT_SYMBOL_GPL(phylink_create); -void phylink_add_pcs(struct phylink *pl, const struct phylink_pcs_ops *ops) +/** + * phylink_set_pcs() - set the current PCS for phylink to use + * @pl: a pointer to a &struct phylink returned from phylink_create() + * @pcs: a pointer to the &struct phylink_pcs + * + * Bind the MAC PCS to phylink. + */ +void phylink_set_pcs(struct phylink *pl, const struct phylink_pcs *pcs) { - pl->pcs_ops = ops; + pl->pcs = pcs; + pl->pcs_ops = pcs->ops; } -EXPORT_SYMBOL_GPL(phylink_add_pcs); +EXPORT_SYMBOL_GPL(phylink_set_pcs); /** * phylink_destroy() - cleanup and destroy the phylink instance diff --git a/include/linux/phylink.h b/include/linux/phylink.h index cc5b452a184e..77e20d48577f 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -273,6 +273,19 @@ void mac_link_up(struct phylink_config *config, struct phy_device *phy, int speed, int duplex, bool tx_pause, bool rx_pause); #endif +struct phylink_pcs_ops; + +/** + * struct phylink_pcs - PHYLINK PCS instance + * @ops: a pointer to the &struct phylink_pcs_ops structure + * + * This structure is designed to be embedded within the PCS private data, + * and will be passed between phylink and the PCS. + */ +struct phylink_pcs { + const struct phylink_pcs_ops *ops; +}; + /** * struct phylink_pcs_ops - MAC PCS operations structure. * @pcs_get_state: read the current MAC PCS link state from the hardware. @@ -282,12 +295,12 @@ void mac_link_up(struct phylink_config *config, struct phy_device *phy, * (where necessary). */ struct phylink_pcs_ops { - void (*pcs_get_state)(struct phylink_config *config, + void (*pcs_get_state)(struct phylink_pcs *pcs, struct phylink_link_state *state); - int (*pcs_config)(struct phylink_config *config, unsigned int mode, + int (*pcs_config)(struct phylink_pcs *pcs, unsigned int mode, phy_interface_t interface, const unsigned long *advertising); - void (*pcs_an_restart)(struct phylink_config *config); + void (*pcs_an_restart)(struct phylink_pcs *pcs); void (*pcs_link_up)(struct phylink_config *config, unsigned int mode, phy_interface_t interface, int speed, int duplex); }; @@ -295,7 +308,7 @@ struct phylink_pcs_ops { #if 0 /* For kernel-doc purposes only. */ /** * pcs_get_state() - Read the current inband link state from the hardware - * @config: a pointer to a &struct phylink_config. + * @pcs: a pointer to a &struct phylink_pcs. * @state: a pointer to a &struct phylink_link_state. * * Read the current inband link state from the MAC PCS, reporting the @@ -308,12 +321,12 @@ struct phylink_pcs_ops { * When present, this overrides mac_pcs_get_state() in &struct * phylink_mac_ops. */ -void pcs_get_state(struct phylink_config *config, +void pcs_get_state(struct phylink_pcs *pcs, struct phylink_link_state *state); /** * pcs_config() - Configure the PCS mode and advertisement - * @config: a pointer to a &struct phylink_config. + * @pcs: a pointer to a &struct phylink_pcs. * @mode: one of %MLO_AN_FIXED, %MLO_AN_PHY, %MLO_AN_INBAND. * @interface: interface mode to be used * @advertising: adertisement ethtool link mode mask @@ -331,21 +344,21 @@ void pcs_get_state(struct phylink_config *config, * * For most 10GBASE-R, there is no advertisement. */ -int (*pcs_config)(struct phylink_config *config, unsigned int mode, - phy_interface_t interface, const unsigned long *advertising); +int pcs_config(struct phylink_pcs *pcs, unsigned int mode, + phy_interface_t interface, const unsigned long *advertising); /** * pcs_an_restart() - restart 802.3z BaseX autonegotiation - * @config: a pointer to a &struct phylink_config. + * @pcs: a pointer to a &struct phylink_pcs. * * When PCS ops are present, this overrides mac_an_restart() in &struct * phylink_mac_ops. */ -void (*pcs_an_restart)(struct phylink_config *config); +void pcs_an_restart(struct phylink_pcs *pcs); /** * pcs_link_up() - program the PCS for the resolved link configuration - * @config: a pointer to a &struct phylink_config. + * @pcs: a pointer to a &struct phylink_pcs. * @mode: link autonegotiation mode * @interface: link &typedef phy_interface_t mode * @speed: link speed @@ -356,14 +369,14 @@ void (*pcs_an_restart)(struct phylink_config *config); * mode without in-band AN needs to be manually configured for the link * and duplex setting. Otherwise, this should be a no-op. */ -void (*pcs_link_up)(struct phylink_config *config, unsigned int mode, - phy_interface_t interface, int speed, int duplex); +void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode, + phy_interface_t interface, int speed, int duplex); #endif struct phylink *phylink_create(struct phylink_config *, struct fwnode_handle *, phy_interface_t iface, const struct phylink_mac_ops *mac_ops); -void phylink_add_pcs(struct phylink *, const struct phylink_pcs_ops *ops); +void phylink_set_pcs(struct phylink *, struct phylink_pcs *pcs); void phylink_destroy(struct phylink *); int phylink_connect_phy(struct phylink *, struct phy_device *);
On Mon, Jun 22, 2020 at 12:10:57PM +0100, Russell King - ARM Linux admin wrote: > On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux admin wrote: > > On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote: > > > In order to support split PCS using PHYLINK properly, we need to add a > > > phylink_pcs_ops structure. > > > > > > Note that a DSA driver that wants to use these needs to implement all 4 > > > of them: the DSA core checks the presence of these 4 function pointers > > > in dsa_switch_ops and only then does it add a PCS to PHYLINK. This is > > > done in order to preserve compatibility with drivers that have not yet > > > been converted, or don't need, a split PCS setup. > > > > > > Also, when pcs_get_state() and pcs_an_restart() are present, their mac > > > counterparts (mac_pcs_get_state(), mac_an_restart()) will no longer get > > > called, as can be seen in phylink.c. > > > > I don't like this at all, it means we've got all this useless layering, > > and that layering will force similar layering veneers into other parts > > of the kernel (such as the DPAA2 MAC driver, when we eventually come to > > re-use pcs-lynx there.) > > > > I don't think we need that - I think we can get to a position where > > pcs-lynx is called requesting that it bind to phylink as the PCS, and > > it calls phylink_add_pcs() directly, which means we do not end up with > > veneers in DSA nor in the DPAA2 MAC driver - they just need to call > > the pcs-lynx initialisation function with the phylink instance for it > > to attach to. > > > > Yes, that requires phylink_add_pcs() to change slightly, and for there > > to be a PCS private pointer, as I have previously stated. At the > > moment, changing that is really easy - the phylink PCS backend has no > > in-tree users at the moment. So there is no reason not to get this > > correct right now. > > How about something like this? I don't want to embed a mdio_device > inside struct phylink_pcs because that would not be appropriate for > some potential users (e.g., Marvell 88E6xxx DSA drivers, Marvell > NETA, PP2, etc.) Just to be clear, I'm expecting discussion on this approach, rather than a patch series appearing - I've already tweaked this patch slightly, adding a "poll" option to cater for the "pcs_poll" facility that was introduced into the phylink_config structure - which I think makes more sense here, as it's all part of the PCS. I'd like there to be some concensus _before_ I go through the users I have in my tree converting them to this, rather than converting them, and then having to convert them to something else. So, if we can agree on what this should look like, then I feel it would make the development process a lot easier for everyone concerned.
> Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops > > On Mon, Jun 22, 2020 at 12:10:57PM +0100, Russell King - ARM Linux admin > wrote: > > On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux admin > wrote: > > > On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote: > > > > In order to support split PCS using PHYLINK properly, we need to > > > > add a phylink_pcs_ops structure. > > > > > > > > Note that a DSA driver that wants to use these needs to implement > > > > all 4 of them: the DSA core checks the presence of these 4 > > > > function pointers in dsa_switch_ops and only then does it add a > > > > PCS to PHYLINK. This is done in order to preserve compatibility > > > > with drivers that have not yet been converted, or don't need, a split PCS > setup. > > > > > > > > Also, when pcs_get_state() and pcs_an_restart() are present, their > > > > mac counterparts (mac_pcs_get_state(), mac_an_restart()) will no > > > > longer get called, as can be seen in phylink.c. > > > > > > I don't like this at all, it means we've got all this useless > > > layering, and that layering will force similar layering veneers into > > > other parts of the kernel (such as the DPAA2 MAC driver, when we > > > eventually come to re-use pcs-lynx there.) > > > The veneers that you are talking about are one phylink_pcs_ops structure and 4 functions that call lynx_pcs_* subsequently. We have the same thing for the MAC operations. Also, the "veneers" in DSA are just how it works, and I don't want to change its structure without a really good reason and without a green light from DSA maintainers. > > > I don't think we need that - I think we can get to a position where > > > pcs-lynx is called requesting that it bind to phylink as the PCS, > > > and it calls phylink_add_pcs() directly, which means we do not end > > > up with veneers in DSA nor in the DPAA2 MAC driver - they just need > > > to call the pcs-lynx initialisation function with the phylink > > > instance for it to attach to. What I am most concerned about is that by passing the PCS ops directly to the PCS module we would lose any ability to apply SoC specific quirks at runtime such as errata workarounds. On the other hand, I am not sure what is the concrete benefit of doing it your way. I understand that for a PHY device the MAC is not involved in the call path but in the case of the PCS the expectation is that it's tightly coupled in the silicon and not plug-and-play. - Ioana > > > > > > Yes, that requires phylink_add_pcs() to change slightly, and for > > > there to be a PCS private pointer, as I have previously stated. At > > > the moment, changing that is really easy - the phylink PCS backend > > > has no in-tree users at the moment. So there is no reason not to > > > get this correct right now. > > > > How about something like this? I don't want to embed a mdio_device > > inside struct phylink_pcs because that would not be appropriate for > > some potential users (e.g., Marvell 88E6xxx DSA drivers, Marvell NETA, > > PP2, etc.) > > Just to be clear, I'm expecting discussion on this approach, rather than a patch > series appearing - I've already tweaked this patch slightly, adding a "poll" option > to cater for the "pcs_poll" facility that was introduced into the phylink_config > structure - which I think makes more sense here, as it's all part of the PCS. > > I'd like there to be some concensus _before_ I go through the users I have in my > tree converting them to this, rather than converting them, and then having to > convert them to something else. > > So, if we can agree on what this should look like, then I feel it would make the > development process a lot easier for everyone concerned. >
On Mon, Jun 22, 2020 at 12:35:06PM +0000, Ioana Ciornei wrote: > > > Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops > > > > On Mon, Jun 22, 2020 at 12:10:57PM +0100, Russell King - ARM Linux admin > > wrote: > > > On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux admin > > wrote: > > > > On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote: > > > > > In order to support split PCS using PHYLINK properly, we need to > > > > > add a phylink_pcs_ops structure. > > > > > > > > > > Note that a DSA driver that wants to use these needs to implement > > > > > all 4 of them: the DSA core checks the presence of these 4 > > > > > function pointers in dsa_switch_ops and only then does it add a > > > > > PCS to PHYLINK. This is done in order to preserve compatibility > > > > > with drivers that have not yet been converted, or don't need, a split PCS > > setup. > > > > > > > > > > Also, when pcs_get_state() and pcs_an_restart() are present, their > > > > > mac counterparts (mac_pcs_get_state(), mac_an_restart()) will no > > > > > longer get called, as can be seen in phylink.c. > > > > > > > > I don't like this at all, it means we've got all this useless > > > > layering, and that layering will force similar layering veneers into > > > > other parts of the kernel (such as the DPAA2 MAC driver, when we > > > > eventually come to re-use pcs-lynx there.) > > > > > > The veneers that you are talking about are one phylink_pcs_ops structure > and 4 functions that call lynx_pcs_* subsequently. We have the same thing > for the MAC operations. > > Also, the "veneers" in DSA are just how it works, and I don't want to change > its structure without a really good reason and without a green light from > DSA maintainers. Right, but we're talking about hardware that is common not only in DSA but elsewhere - and we already deal with that outside of DSA with PHYs. So, what I'm proposing is really nothing new for DSA. > > > > I don't think we need that - I think we can get to a position where > > > > pcs-lynx is called requesting that it bind to phylink as the PCS, > > > > and it calls phylink_add_pcs() directly, which means we do not end > > > > up with veneers in DSA nor in the DPAA2 MAC driver - they just need > > > > to call the pcs-lynx initialisation function with the phylink > > > > instance for it to attach to. > > What I am most concerned about is that by passing the PCS ops directly to the > PCS module we would lose any ability to apply SoC specific quirks at runtime > such as errata workarounds. Do you know what those errata would be? I'm only aware of A-011118 in the LX2160A which I don't believe will impact this code. I don't have visibility of Ocelot/Felix. > On the other hand, I am not sure what is the concrete benefit of doing > it your way. I understand that for a PHY device the MAC is not involved > in the call path but in the case of the PCS the expectation is that it's > tightly coupled in the silicon and not plug-and-play. The advantage is less lines of code to maintain, and a more efficient and understandable code structure. I would much rather start off simple and then augment rather than start off with unnecessary complexity and then get stuck with it while not really needing it.
> Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops > > On Mon, Jun 22, 2020 at 12:35:06PM +0000, Ioana Ciornei wrote: > > > > > Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for > > > phylink_pcs_ops > > > > > > On Mon, Jun 22, 2020 at 12:10:57PM +0100, Russell King - ARM Linux > > > admin > > > wrote: > > > > On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux > > > > admin > > > wrote: > > > > > On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote: > > > > > > In order to support split PCS using PHYLINK properly, we need > > > > > > to add a phylink_pcs_ops structure. > > > > > > > > > > > > Note that a DSA driver that wants to use these needs to > > > > > > implement all 4 of them: the DSA core checks the presence of > > > > > > these 4 function pointers in dsa_switch_ops and only then does > > > > > > it add a PCS to PHYLINK. This is done in order to preserve > > > > > > compatibility with drivers that have not yet been converted, > > > > > > or don't need, a split PCS > > > setup. > > > > > > > > > > > > Also, when pcs_get_state() and pcs_an_restart() are present, > > > > > > their mac counterparts (mac_pcs_get_state(), mac_an_restart()) > > > > > > will no longer get called, as can be seen in phylink.c. > > > > > > > > > > I don't like this at all, it means we've got all this useless > > > > > layering, and that layering will force similar layering veneers > > > > > into other parts of the kernel (such as the DPAA2 MAC driver, > > > > > when we eventually come to re-use pcs-lynx there.) > > > > > > > > > The veneers that you are talking about are one phylink_pcs_ops > > structure and 4 functions that call lynx_pcs_* subsequently. We have > > the same thing for the MAC operations. > > > > Also, the "veneers" in DSA are just how it works, and I don't want to > > change its structure without a really good reason and without a green > > light from DSA maintainers. > > Right, but we're talking about hardware that is common not only in DSA but > elsewhere - and we already deal with that outside of DSA with PHYs. I said before why the PHY use case is different from a PCS tightly coupled inside the SoC. > So, what I'm proposing is really nothing new for DSA. > > > > > > I don't think we need that - I think we can get to a position > > > > > where pcs-lynx is called requesting that it bind to phylink as > > > > > the PCS, and it calls phylink_add_pcs() directly, which means we > > > > > do not end up with veneers in DSA nor in the DPAA2 MAC driver - > > > > > they just need to call the pcs-lynx initialisation function with > > > > > the phylink instance for it to attach to. > > > > What I am most concerned about is that by passing the PCS ops directly > > to the PCS module we would lose any ability to apply SoC specific > > quirks at runtime such as errata workarounds. > > Do you know what those errata would be? I'm only aware of A-011118 in the > LX2160A which I don't believe will impact this code. I don't have visibility of > Ocelot/Felix. I was mainly looking at this from a software architecture perspective, not having any explicit erratum in mind. > > > On the other hand, I am not sure what is the concrete benefit of doing > > it your way. I understand that for a PHY device the MAC is not > > involved in the call path but in the case of the PCS the expectation > > is that it's tightly coupled in the silicon and not plug-and-play. > > The advantage is less lines of code to maintain, and a more efficient and > understandable code structure. I would much rather start off simple and then > augment rather than start off with unnecessary complexity and then get stuck > with it while not really needing it. > I am not going to argue ad infinitum on this. If you think keeping the phylink_pcs_ops structure outside is the better approach then let's take that route. I will go through your feedback on the actual Lynx module and respond/make the necessary adjustments. Beside this, what should be our next move? Will you submit the new method of working with the phylink_pcs_ops structure? Ioana
On Mon, Jun 22, 2020 at 01:51:55PM +0000, Ioana Ciornei wrote: > > Right, but we're talking about hardware that is common not only in DSA but > > elsewhere - and we already deal with that outside of DSA with PHYs. > > I said before why the PHY use case is different from a PCS tightly > coupled inside the SoC. Yes, however, I'm responding to your point about whether DSA maintainers would be happy with it, so I've used the already existing example with phylib to illustrate that this already happens in DSA-land. I was not meaning to refer to your point about the PCS being tightly coupled inside the SoC. That should've been clear by the comment immediately below: > > So, what I'm proposing is really nothing new for DSA. > > Do you know what those errata would be? I'm only aware of A-011118 in the > > LX2160A which I don't believe will impact this code. I don't have visibility of > > Ocelot/Felix. > > I was mainly looking at this from a software architecture perspective, > not having any explicit erratum in mind. Ok. > > > On the other hand, I am not sure what is the concrete benefit of doing > > > it your way. I understand that for a PHY device the MAC is not > > > involved in the call path but in the case of the PCS the expectation > > > is that it's tightly coupled in the silicon and not plug-and-play. > > > > The advantage is less lines of code to maintain, and a more efficient and > > understandable code structure. I would much rather start off simple and then > > augment rather than start off with unnecessary complexity and then get stuck > > with it while not really needing it. > > > > I am not going to argue ad infinitum on this. If you think keeping the > phylink_pcs_ops structure outside is the better approach then let's > take that route. > > I will go through your feedback on the actual Lynx module and respond/make the > necessary adjustments. > > Beside this, what should be our next move? Will you submit the new method of > working with the phylink_pcs_ops structure? I do think it is the best approach _at the moment_ given what we know at this time - without over-designing for situations that we don't know. So, I'll take this as tentative agreement, and I'll begin work on converting my local users, testing, and then sending a finished patch.
diff --git a/include/net/dsa.h b/include/net/dsa.h index 50389772c597..09aa36198f4b 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -446,6 +446,18 @@ struct dsa_switch_ops { bool tx_pause, bool rx_pause); void (*phylink_fixed_state)(struct dsa_switch *ds, int port, struct phylink_link_state *state); + void (*phylink_pcs_get_state)(struct dsa_switch *ds, int port, + struct phylink_link_state *state); + int (*phylink_pcs_config)(struct dsa_switch *ds, int port, + unsigned int mode, + phy_interface_t interface, + const unsigned long *advertising); + void (*phylink_pcs_an_restart)(struct dsa_switch *ds, int port); + void (*phylink_pcs_link_up)(struct dsa_switch *ds, int port, + unsigned int mode, + phy_interface_t interface, int speed, + int duplex); + /* * ethtool hardware statistics. */ diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index adecf73bd608..de8e11796178 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -169,6 +169,7 @@ int dsa_port_vid_del(struct dsa_port *dp, u16 vid); int dsa_port_link_register_of(struct dsa_port *dp); void dsa_port_link_unregister_of(struct dsa_port *dp); extern const struct phylink_mac_ops dsa_port_phylink_mac_ops; +extern const struct phylink_pcs_ops dsa_port_phylink_pcs_ops; /* slave.c */ extern const struct dsa_device_ops notag_netdev_ops; diff --git a/net/dsa/port.c b/net/dsa/port.c index e23ece229c7e..a2b0460d2593 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -592,6 +592,52 @@ const struct phylink_mac_ops dsa_port_phylink_mac_ops = { .mac_link_up = dsa_port_phylink_mac_link_up, }; +static void dsa_port_pcs_get_state(struct phylink_config *config, + struct phylink_link_state *state) +{ + struct dsa_port *dp = container_of(config, struct dsa_port, pl_config); + struct dsa_switch *ds = dp->ds; + + ds->ops->phylink_pcs_get_state(ds, dp->index, state); +} + +static void dsa_port_pcs_an_restart(struct phylink_config *config) +{ + struct dsa_port *dp = container_of(config, struct dsa_port, pl_config); + struct dsa_switch *ds = dp->ds; + + ds->ops->phylink_pcs_an_restart(ds, dp->index); +} + +static int dsa_port_pcs_config(struct phylink_config *config, + unsigned int mode, phy_interface_t interface, + const unsigned long *advertising) +{ + struct dsa_port *dp = container_of(config, struct dsa_port, pl_config); + struct dsa_switch *ds = dp->ds; + + return ds->ops->phylink_pcs_config(ds, dp->index, mode, interface, + advertising); +} + +static void dsa_port_pcs_link_up(struct phylink_config *config, + unsigned int mode, phy_interface_t interface, + int speed, int duplex) +{ + struct dsa_port *dp = container_of(config, struct dsa_port, pl_config); + struct dsa_switch *ds = dp->ds; + + ds->ops->phylink_pcs_link_up(ds, dp->index, mode, interface, speed, + duplex); +} + +const struct phylink_pcs_ops dsa_port_phylink_pcs_ops = { + .pcs_get_state = dsa_port_pcs_get_state, + .pcs_config = dsa_port_pcs_config, + .pcs_an_restart = dsa_port_pcs_an_restart, + .pcs_link_up = dsa_port_pcs_link_up, +}; + static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable) { struct dsa_switch *ds = dp->ds; diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 4c7f086a047b..8856e70f6a06 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1650,6 +1650,12 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev) if (ds->ops->get_phy_flags) phy_flags = ds->ops->get_phy_flags(ds, dp->index); + if (ds->ops->phylink_pcs_get_state && + ds->ops->phylink_pcs_an_restart && + ds->ops->phylink_pcs_config && + ds->ops->phylink_pcs_link_up) + phylink_add_pcs(dp->pl, &dsa_port_phylink_pcs_ops); + ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags); if (ret == -ENODEV && ds->slave_mii_bus) { /* We could not connect to a designated PHY or SFP, so try to
In order to support split PCS using PHYLINK properly, we need to add a phylink_pcs_ops structure. Note that a DSA driver that wants to use these needs to implement all 4 of them: the DSA core checks the presence of these 4 function pointers in dsa_switch_ops and only then does it add a PCS to PHYLINK. This is done in order to preserve compatibility with drivers that have not yet been converted, or don't need, a split PCS setup. Also, when pcs_get_state() and pcs_an_restart() are present, their mac counterparts (mac_pcs_get_state(), mac_an_restart()) will no longer get called, as can be seen in phylink.c. Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- Changes in v3: * patch added include/net/dsa.h | 12 ++++++++++++ net/dsa/dsa_priv.h | 1 + net/dsa/port.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ net/dsa/slave.c | 6 ++++++ 4 files changed, 65 insertions(+)