| Message ID | 20181007173823.21590-5-richardcochran@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Peer to Peer One-Step time stamping | expand |
On 10/07/18 10:38, Richard Cochran wrote: > When parsing a PHY node, register its time stamper, if any, and attach > the instance to the PHY device. > > Signed-off-by: Richard Cochran <richardcochran@gmail.com> > --- > drivers/net/phy/phy_device.c | 3 +++ > drivers/of/of_mdio.c | 26 ++++++++++++++++++++++++++ > 2 files changed, 29 insertions(+) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index a454432d166f..c24bce9b7270 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -833,6 +833,9 @@ EXPORT_SYMBOL(phy_device_register); > */ > void phy_device_remove(struct phy_device *phydev) > { > + if (phydev->mii_ts) > + unregister_mii_timestamper(phydev->mii_ts); > + > device_del(&phydev->mdio.dev); > > /* Assert the reset signal */ > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index f76c10ecc616..7699f167e4a9 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -44,14 +44,38 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id) > return -EINVAL; > } > > +struct mii_timestamper *of_find_mii_timestamper(struct device_node *node) > +{ > + struct of_phandle_args args; > + unsigned int port = 0; > + int err; > + > + err = of_parse_phandle_with_args(node, "timestamper", > + "#phandle-cells", 0, &args); There appears to be a binding document missing to describe what a timerstamper provider is. Using a more specific name than "#phandle-cells" is preferred when dealing with specific devices, e.g: interrupt-controller/#interrupt-cells clocks/#clock-cells etc. So I would go with #timestamp-cells here, and define what the cell sie and format should be in a separate "dt-bindings" prefixed patch that the Device Tree folks can also comment on. > + if (err == -ENOENT) > + return NULL; > + else if (err) > + return ERR_PTR(err); > + > + if (args.args_count >= 1) > + port = args.args[0]; If it's greater than one, than it is an error, and it should be flagged as such. The idea looks good though, should of_find_mii_timestamper() somehow be made conditional to CONFIG_PTP and we should have a stub for when it is disabled? > + > + return register_mii_timestamper(args.np, port); > +} > + > static int of_mdiobus_register_phy(struct mii_bus *mdio, > struct device_node *child, u32 addr) > { > + struct mii_timestamper *mii_ts; > struct phy_device *phy; > bool is_c45; > int rc; > u32 phy_id; > > + mii_ts = of_find_mii_timestamper(child); > + if (IS_ERR(mii_ts)) > + return PTR_ERR(mii_ts); > + > is_c45 = of_device_is_compatible(child, > "ethernet-phy-ieee802.3-c45"); > > @@ -97,6 +121,8 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, > return rc; > } > > + phy->mii_ts = mii_ts; > + > dev_dbg(&mdio->dev, "registered phy %s at address %i\n", > child->name, addr); > return 0; >
> > + if (err == -ENOENT) > > + return NULL; > > + else if (err) > > + return ERR_PTR(err); > > + > > + if (args.args_count >= 1) > > + port = args.args[0]; > > If it's greater than one, than it is an error, and it should be flagged > as such. > > The idea looks good though, should of_find_mii_timestamper() somehow be > made conditional to CONFIG_PTP and we should have a stub for when it is > disabled? Hi Florian There already is a stub. But register return -EOPNOTSUPP. > > + > > + return register_mii_timestamper(args.np, port); So this returns EOPNOTUP > > +} > > + > > static int of_mdiobus_register_phy(struct mii_bus *mdio, > > struct device_node *child, u32 addr) > > { > > + struct mii_timestamper *mii_ts; > > struct phy_device *phy; > > bool is_c45; > > int rc; > > u32 phy_id; > > > > + mii_ts = of_find_mii_timestamper(child); > > + if (IS_ERR(mii_ts)) > > + return PTR_ERR(mii_ts); > > + and this returns EOPNOPTSUPP, so the PHY is not registered :-( Andrew
On Sun, Oct 07, 2018 at 10:38:22AM -0700, Richard Cochran wrote: > When parsing a PHY node, register its time stamper, if any, and attach > the instance to the PHY device. Hi Richard This does look a lot better. Thanks for making the changes. Andrew
On Sun, Oct 07, 2018 at 11:14:38AM -0700, Florian Fainelli wrote: > There appears to be a binding document missing to describe what a > timerstamper provider is. Using a more specific name than > "#phandle-cells" is preferred when dealing with specific devices, e.g: > > interrupt-controller/#interrupt-cells > clocks/#clock-cells Sure. > So I would go with #timestamp-cells here, and define what the cell sie > and format should be in a separate "dt-bindings" prefixed patch that the > Device Tree folks can also comment on. I documented this in the last patch. I didn't see any example in our device tree that explains a "reference" like this that is not connected to a specific node type. > > > + if (err == -ENOENT) > > + return NULL; > > + else if (err) > > + return ERR_PTR(err); > > + > > + if (args.args_count >= 1) > > + port = args.args[0]; > > If it's greater than one, than it is an error, and it should be flagged > as such. I wanted to allow specific MII time stamping drivers to use one than one value in the future, should the need arise. > The idea looks good though, should of_find_mii_timestamper() somehow be > made conditional to CONFIG_PTP and we should have a stub for when it is > disabled? Do you mean CONFIG_NETWORK_PHY_TIMESTAMPING ? There is a stub for that. Thanks, Richard
On Sun, Oct 07, 2018 at 08:17:54PM +0200, Andrew Lunn wrote: > > > + if (err == -ENOENT) > > > + return NULL; > > > + else if (err) > > > + return ERR_PTR(err); > > > + > > > + if (args.args_count >= 1) > > > + port = args.args[0]; > > > > If it's greater than one, than it is an error, and it should be flagged > > as such. > > > > The idea looks good though, should of_find_mii_timestamper() somehow be > > made conditional to CONFIG_PTP and we should have a stub for when it is > > disabled? > > Hi Florian > > There already is a stub. But register return -EOPNOTSUPP. The stub returns NULL... > > > + return register_mii_timestamper(args.np, port); > > So this returns EOPNOTUP NULL... > > > static int of_mdiobus_register_phy(struct mii_bus *mdio, > > > struct device_node *child, u32 addr) > > > { > > > + struct mii_timestamper *mii_ts; > > > struct phy_device *phy; > > > bool is_c45; > > > int rc; > > > u32 phy_id; > > > > > > + mii_ts = of_find_mii_timestamper(child); > > > + if (IS_ERR(mii_ts)) > > > + return PTR_ERR(mii_ts); > > > + > > and this returns EOPNOPTSUPP, so the PHY is not registered :-( and the phydev.mii_ts field is then set to NULL. Thanks, Richard
On Sun, Oct 07, 2018 at 12:26:27PM -0700, Richard Cochran wrote: > On Sun, Oct 07, 2018 at 08:17:54PM +0200, Andrew Lunn wrote: > > > > + if (err == -ENOENT) > > > > + return NULL; > > > > + else if (err) > > > > + return ERR_PTR(err); > > > > + > > > > + if (args.args_count >= 1) > > > > + port = args.args[0]; > > > > > > If it's greater than one, than it is an error, and it should be flagged > > > as such. > > > > > > The idea looks good though, should of_find_mii_timestamper() somehow be > > > made conditional to CONFIG_PTP and we should have a stub for when it is > > > disabled? > > > > Hi Florian > > > > There already is a stub. But register return -EOPNOTSUPP. > > The stub returns NULL... Ah, sorry, it is register_mii_tstamp_controller() which return -EOPNOTSUP. Andrew
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index a454432d166f..c24bce9b7270 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -833,6 +833,9 @@ EXPORT_SYMBOL(phy_device_register); */ void phy_device_remove(struct phy_device *phydev) { + if (phydev->mii_ts) + unregister_mii_timestamper(phydev->mii_ts); + device_del(&phydev->mdio.dev); /* Assert the reset signal */ diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index f76c10ecc616..7699f167e4a9 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -44,14 +44,38 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id) return -EINVAL; } +struct mii_timestamper *of_find_mii_timestamper(struct device_node *node) +{ + struct of_phandle_args args; + unsigned int port = 0; + int err; + + err = of_parse_phandle_with_args(node, "timestamper", + "#phandle-cells", 0, &args); + if (err == -ENOENT) + return NULL; + else if (err) + return ERR_PTR(err); + + if (args.args_count >= 1) + port = args.args[0]; + + return register_mii_timestamper(args.np, port); +} + static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child, u32 addr) { + struct mii_timestamper *mii_ts; struct phy_device *phy; bool is_c45; int rc; u32 phy_id; + mii_ts = of_find_mii_timestamper(child); + if (IS_ERR(mii_ts)) + return PTR_ERR(mii_ts); + is_c45 = of_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"); @@ -97,6 +121,8 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, return rc; } + phy->mii_ts = mii_ts; + dev_dbg(&mdio->dev, "registered phy %s at address %i\n", child->name, addr); return 0;
When parsing a PHY node, register its time stamper, if any, and attach the instance to the PHY device. Signed-off-by: Richard Cochran <richardcochran@gmail.com> --- drivers/net/phy/phy_device.c | 3 +++ drivers/of/of_mdio.c | 26 ++++++++++++++++++++++++++ 2 files changed, 29 insertions(+)