| Message ID | 20211004191527.1610759-7-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for Xilinx PCS | expand |
On Mon, Oct 04, 2021 at 03:15:17PM -0400, Sean Anderson wrote: > This adds a function to set the PCS only if there is not one currently > set. The intention here is to allow MAC drivers to have a "default" PCS > (such as an internal one) which may be used when one has not been set > via the device tree. This allows for backwards compatibility for cases > where a PCS was automatically attached if necessary. I'm not sure I entirely like this approach. Why can't the network driver check for the pcs-handle property and avoid using its "default" if present?
On Tue, Oct 05, 2021 at 10:51:38AM +0100, Russell King (Oracle) wrote: > On Mon, Oct 04, 2021 at 03:15:17PM -0400, Sean Anderson wrote: > > This adds a function to set the PCS only if there is not one currently > > set. The intention here is to allow MAC drivers to have a "default" PCS > > (such as an internal one) which may be used when one has not been set > > via the device tree. This allows for backwards compatibility for cases > > where a PCS was automatically attached if necessary. > > I'm not sure I entirely like this approach. Why can't the network > driver check for the pcs-handle property and avoid using its > "default" if present? And that would also be in line with ethernet/freescale/dpaa2/dpaa2-mac.c: node = fwnode_find_reference(dpmac_node, "pcs-handle", 0); We need a uniform meaning of pcs-handle. And dpaa2-mac.c has set the precedent that the MAC uses it, not phylink. That can however be changed, if it make sense, but both users should do the same. Andrew
On 10/5/21 9:43 AM, Andrew Lunn wrote: > On Tue, Oct 05, 2021 at 10:51:38AM +0100, Russell King (Oracle) wrote: >> On Mon, Oct 04, 2021 at 03:15:17PM -0400, Sean Anderson wrote: >> > This adds a function to set the PCS only if there is not one currently >> > set. The intention here is to allow MAC drivers to have a "default" PCS >> > (such as an internal one) which may be used when one has not been set >> > via the device tree. This allows for backwards compatibility for cases >> > where a PCS was automatically attached if necessary. >> >> I'm not sure I entirely like this approach. Why can't the network >> driver check for the pcs-handle property and avoid using its >> "default" if present? > > And that would also be in line with > > ethernet/freescale/dpaa2/dpaa2-mac.c: node = fwnode_find_reference(dpmac_node, "pcs-handle", 0); > > We need a uniform meaning of pcs-handle. And dpaa2-mac.c has set the > precedent that the MAC uses it, not phylink. That can however be > changed, if it make sense, but both users should do the same. The tricky bit here happens when drivers set the PCS in mac_prepare() depending on the interface. So you have to remember during probe() whether you are supposed to set the PCS for later. I would like to leave more of this to phylink to ensure that the process is done in a uniform way. --Sean
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 046fdac3597d..f82dc0f87f40 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -871,6 +871,32 @@ int phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs) } EXPORT_SYMBOL_GPL(phylink_set_pcs); +/** + * phylink_set_pcs_weak() - optionally 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 only if there is no currently-bound PCS. This + * may be called to set a "default" PCS, such as an internal PCS which was + * previously handled by the MAC driver directly. Otherwise, this function + * behaves like phylink_set_pcs(); + * + * Context: may sleep. + * Return: 1 if the PCS was set, 0 if it was not, or -errno on failure. + */ +int phylink_set_pcs_weak(struct phylink *pl, struct phylink_pcs *pcs) +{ + int ret; + + if (!pl->pcs) { + ret = phylink_set_pcs(pl, pcs); + return ret ? ret : 1; + } + + return 0; +} +EXPORT_SYMBOL_GPL(phylink_set_pcs_weak); + static struct phylink_pcs *phylink_find_pcs(struct fwnode_handle *fwnode) { struct phylink_pcs *pcs; diff --git a/include/linux/phylink.h b/include/linux/phylink.h index d60756b36ad3..bd0ce707d098 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -442,6 +442,7 @@ void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode, int phylink_register_pcs(struct phylink_pcs *pcs); void phylink_unregister_pcs(struct phylink_pcs *pcs); int phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs); +int phylink_set_pcs_weak(struct phylink *pl, struct phylink_pcs *pcs); struct phylink *phylink_create(struct phylink_config *, struct fwnode_handle *, phy_interface_t iface,
This adds a function to set the PCS only if there is not one currently set. The intention here is to allow MAC drivers to have a "default" PCS (such as an internal one) which may be used when one has not been set via the device tree. This allows for backwards compatibility for cases where a PCS was automatically attached if necessary. Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- drivers/net/phy/phylink.c | 26 ++++++++++++++++++++++++++ include/linux/phylink.h | 1 + 2 files changed, 27 insertions(+)