| Message ID | 20200427132409.23664-1-calvin.johnson@oss.nxp.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce new APIs to support phylink and phy layers | expand |
On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote: > Following functions are defined: > phylink_fwnode_phy_connect() > phylink_device_phy_connect() > fwnode_phy_find_device() > device_phy_find_device() > fwnode_get_phy_node() > > First two help in connecting phy to phylink instance. > Next two help in finding a phy on a mdiobus. > Last one helps in getting phy_node from a fwnode. > > Changes in v2: > move phy code from base/property.c to net/phy/phy_device.c > replace acpi & of code to get phy-handle with fwnode_find_reference > replace of_ and acpi_ code with generic fwnode to get phy-handle. > > Calvin Johnson (3): > device property: Introduce phy related fwnode functions > net: phy: alphabetically sort header includes > phylink: Introduce phylink_fwnode_phy_connect() Thanks for this, but there's more work that needs to be done here. I also think that we must have an ack from ACPI people before this can be accepted - you are in effect proposing a new way for representing PHYs in ACPI. > > drivers/net/phy/phy_device.c | 83 ++++++++++++++++++++++++++++++------ > drivers/net/phy/phylink.c | 68 +++++++++++++++++++++++++++++ > include/linux/phy.h | 3 ++ > include/linux/phylink.h | 6 +++ > 4 files changed, 146 insertions(+), 14 deletions(-) > > -- > 2.17.1 > >
On Mon, Apr 27, 2020 at 02:58:20PM +0100, Russell King - ARM Linux admin wrote: > On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote: > > Following functions are defined: > > phylink_fwnode_phy_connect() > > phylink_device_phy_connect() > > fwnode_phy_find_device() > > device_phy_find_device() > > fwnode_get_phy_node() > > > > First two help in connecting phy to phylink instance. > > Next two help in finding a phy on a mdiobus. > > Last one helps in getting phy_node from a fwnode. > > > > Changes in v2: > > move phy code from base/property.c to net/phy/phy_device.c > > replace acpi & of code to get phy-handle with fwnode_find_reference > > replace of_ and acpi_ code with generic fwnode to get phy-handle. > > > > Calvin Johnson (3): > > device property: Introduce phy related fwnode functions > > net: phy: alphabetically sort header includes > > phylink: Introduce phylink_fwnode_phy_connect() > > Thanks for this, but there's more work that needs to be done here. I > also think that we must have an ack from ACPI people before this can be > accepted - you are in effect proposing a new way for representing PHYs > in ACPI. Thanks for your review. Agree that we need an ack from ACPI people. However, I don't think it is a completely new way as similar acpi approach to get phy-handle is already in place. Please see this: https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L832 Please let me know, if you see more work than the ones you pointed out in your review comments on previous patches. Thanks Calvin
On Mon, Apr 27, 2020 at 08:02:38PM +0530, Calvin Johnson wrote: > On Mon, Apr 27, 2020 at 02:58:20PM +0100, Russell King - ARM Linux admin wrote: > > On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote: > > > Following functions are defined: > > > phylink_fwnode_phy_connect() > > > phylink_device_phy_connect() > > > fwnode_phy_find_device() > > > device_phy_find_device() > > > fwnode_get_phy_node() > > > > > > First two help in connecting phy to phylink instance. > > > Next two help in finding a phy on a mdiobus. > > > Last one helps in getting phy_node from a fwnode. > > > > > > Changes in v2: > > > move phy code from base/property.c to net/phy/phy_device.c > > > replace acpi & of code to get phy-handle with fwnode_find_reference > > > replace of_ and acpi_ code with generic fwnode to get phy-handle. > > > > > > Calvin Johnson (3): > > > device property: Introduce phy related fwnode functions > > > net: phy: alphabetically sort header includes > > > phylink: Introduce phylink_fwnode_phy_connect() > > > > Thanks for this, but there's more work that needs to be done here. I > > also think that we must have an ack from ACPI people before this can be > > accepted - you are in effect proposing a new way for representing PHYs > > in ACPI. > > Thanks for your review. > > Agree that we need an ack from ACPI people. > However, I don't think it is a completely new way as similar acpi approach to > get phy-handle is already in place. > Please see this: > https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L832 That was added by: commit 8089a96f601bdfe3e1b41d14bb703aafaf1b8f34 Author: Iyappan Subramanian <isubramanian@apm.com> Date: Mon Jul 25 17:12:41 2016 -0700 drivers: net: xgene: Add backward compatibility This patch adds xgene_enet_check_phy_hanlde() function that checks whether MDIO driver is probed successfully and sets pdata->mdio_driver to true. If MDIO driver is not probed, ethernet driver falls back to backward compatibility mode. Since enum xgene_enet_cmd is used by MDIO driver, removing this from ethernet driver. Signed-off-by: Iyappan Subramanian <isubramanian@apm.com> Tested-by: Fushen Chen <fchen@apm.com> Tested-by: Toan Le <toanle@apm.com> Signed-off-by: David S. Miller <davem@davemloft.net> The commit message says nothing about adding ACPI stuff, and searching the 'net for the posting of this patch seems to suggest that it wasn't obviously copied to any ACPI people: https://lists.openwall.net/netdev/2016/07/26/11 Annoyingly, searching for: "drivers: net: xgene: Add backward compatibility" site:lore.kernel.org doesn't find it on lore, so can't get the full headers and therefore addresses. So, yes, there's another driver using it, but the ACPI folk probably never got a look-in on that instance. Even if they had been copied, the patch description is probably sufficiently poor that they wouldn't have read the patch. I'd say there's questions over whether ACPI people will find this an acceptable approach. Given that your patch moves this from one driver to a subsystem thing, it needs to be ratified by ACPI people, because it's effectively becoming a standardised way to represent a PHY in ACPI.
On Mon, Apr 27, 2020 at 03:48:07PM +0100, Russell King - ARM Linux admin wrote: > On Mon, Apr 27, 2020 at 08:02:38PM +0530, Calvin Johnson wrote: > > On Mon, Apr 27, 2020 at 02:58:20PM +0100, Russell King - ARM Linux admin wrote: > > > On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote: > > > > Following functions are defined: > > > > phylink_fwnode_phy_connect() > > > > phylink_device_phy_connect() > > > > fwnode_phy_find_device() > > > > device_phy_find_device() > > > > fwnode_get_phy_node() > > > > > > > > First two help in connecting phy to phylink instance. > > > > Next two help in finding a phy on a mdiobus. > > > > Last one helps in getting phy_node from a fwnode. > > > > > > > > Changes in v2: > > > > move phy code from base/property.c to net/phy/phy_device.c > > > > replace acpi & of code to get phy-handle with fwnode_find_reference > > > > replace of_ and acpi_ code with generic fwnode to get phy-handle. > > > > > > > > Calvin Johnson (3): > > > > device property: Introduce phy related fwnode functions > > > > net: phy: alphabetically sort header includes > > > > phylink: Introduce phylink_fwnode_phy_connect() > > > > > > Thanks for this, but there's more work that needs to be done here. I > > > also think that we must have an ack from ACPI people before this can be > > > accepted - you are in effect proposing a new way for representing PHYs > > > in ACPI. > > > > Thanks for your review. > > > > Agree that we need an ack from ACPI people. > > However, I don't think it is a completely new way as similar acpi approach to > > get phy-handle is already in place. > > Please see this: > > https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L832 > > That was added by: > > commit 8089a96f601bdfe3e1b41d14bb703aafaf1b8f34 > Author: Iyappan Subramanian <isubramanian@apm.com> > Date: Mon Jul 25 17:12:41 2016 -0700 > > drivers: net: xgene: Add backward compatibility > > This patch adds xgene_enet_check_phy_hanlde() function that checks whether > MDIO driver is probed successfully and sets pdata->mdio_driver to true. > If MDIO driver is not probed, ethernet driver falls back to backward > compatibility mode. > > Since enum xgene_enet_cmd is used by MDIO driver, removing this from > ethernet driver. > > Signed-off-by: Iyappan Subramanian <isubramanian@apm.com> > Tested-by: Fushen Chen <fchen@apm.com> > Tested-by: Toan Le <toanle@apm.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > The commit message says nothing about adding ACPI stuff, and searching > the 'net for the posting of this patch seems to suggest that it wasn't > obviously copied to any ACPI people: > > https://lists.openwall.net/netdev/2016/07/26/11 > > Annoyingly, searching for: > > "drivers: net: xgene: Add backward compatibility" site:lore.kernel.org > > doesn't find it on lore, so can't get the full headers and therefore > addresses. > > So, yes, there's another driver using it, but the ACPI folk probably > never got a look-in on that instance. Even if they had been copied, > the patch description is probably sufficiently poor that they wouldn't > have read the patch. > > I'd say there's questions over whether ACPI people will find this an > acceptable approach. > > Given that your patch moves this from one driver to a subsystem thing, > it needs to be ratified by ACPI people, because it's effectively > becoming a standardised way to represent a PHY in ACPI. > Thanks for digging deep. Makes sense to me. Will wait for ACPI response. Regards Calvin
On Mon, Apr 27, 2020 at 03:48:07PM +0100, Russell King - ARM Linux admin wrote: > On Mon, Apr 27, 2020 at 08:02:38PM +0530, Calvin Johnson wrote: > > On Mon, Apr 27, 2020 at 02:58:20PM +0100, Russell King - ARM Linux admin wrote: > > > On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote: > > > > Following functions are defined: > > > > phylink_fwnode_phy_connect() > > > > phylink_device_phy_connect() > > > > fwnode_phy_find_device() > > > > device_phy_find_device() > > > > fwnode_get_phy_node() > > > > > > > > First two help in connecting phy to phylink instance. > > > > Next two help in finding a phy on a mdiobus. > > > > Last one helps in getting phy_node from a fwnode. > > > > > > > > Changes in v2: > > > > move phy code from base/property.c to net/phy/phy_device.c > > > > replace acpi & of code to get phy-handle with fwnode_find_reference > > > > replace of_ and acpi_ code with generic fwnode to get phy-handle. > > > > > > > > Calvin Johnson (3): > > > > device property: Introduce phy related fwnode functions > > > > net: phy: alphabetically sort header includes > > > > phylink: Introduce phylink_fwnode_phy_connect() > > > > > > Thanks for this, but there's more work that needs to be done here. I > > > also think that we must have an ack from ACPI people before this can be > > > accepted - you are in effect proposing a new way for representing PHYs > > > in ACPI. > > > > Thanks for your review. > > > > Agree that we need an ack from ACPI people. > > However, I don't think it is a completely new way as similar acpi approach to > > get phy-handle is already in place. > > Please see this: > > https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L832 > > That was added by: > > commit 8089a96f601bdfe3e1b41d14bb703aafaf1b8f34 > Author: Iyappan Subramanian <isubramanian@apm.com> > Date: Mon Jul 25 17:12:41 2016 -0700 > > drivers: net: xgene: Add backward compatibility > > This patch adds xgene_enet_check_phy_hanlde() function that checks whether > MDIO driver is probed successfully and sets pdata->mdio_driver to true. > If MDIO driver is not probed, ethernet driver falls back to backward > compatibility mode. > > Since enum xgene_enet_cmd is used by MDIO driver, removing this from > ethernet driver. > > Signed-off-by: Iyappan Subramanian <isubramanian@apm.com> > Tested-by: Fushen Chen <fchen@apm.com> > Tested-by: Toan Le <toanle@apm.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > The commit message says nothing about adding ACPI stuff, and searching > the 'net for the posting of this patch seems to suggest that it wasn't > obviously copied to any ACPI people: > > https://lists.openwall.net/netdev/2016/07/26/11 > > Annoyingly, searching for: > > "drivers: net: xgene: Add backward compatibility" site:lore.kernel.org > > doesn't find it on lore, so can't get the full headers and therefore > addresses. > > So, yes, there's another driver using it, but the ACPI folk probably > never got a look-in on that instance. Even if they had been copied, > the patch description is probably sufficiently poor that they wouldn't > have read the patch. > > I'd say there's questions over whether ACPI people will find this an > acceptable approach. > > Given that your patch moves this from one driver to a subsystem thing, > it needs to be ratified by ACPI people, because it's effectively > becoming a standardised way to represent a PHY in ACPI. How can we get attention/response from ACPI people? I've now added ACPI maintainers in the To list. Thanks Calvin
On Wed, Apr 29, 2020 at 7:38 AM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > On Mon, Apr 27, 2020 at 03:48:07PM +0100, Russell King - ARM Linux admin wrote: > > On Mon, Apr 27, 2020 at 08:02:38PM +0530, Calvin Johnson wrote: > > > On Mon, Apr 27, 2020 at 02:58:20PM +0100, Russell King - ARM Linux admin wrote: > > > > On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote: > > > > > Following functions are defined: > > > > > phylink_fwnode_phy_connect() > > > > > phylink_device_phy_connect() > > > > > fwnode_phy_find_device() > > > > > device_phy_find_device() > > > > > fwnode_get_phy_node() > > > > > > > > > > First two help in connecting phy to phylink instance. > > > > > Next two help in finding a phy on a mdiobus. > > > > > Last one helps in getting phy_node from a fwnode. > > > > > > > > > > Changes in v2: > > > > > move phy code from base/property.c to net/phy/phy_device.c > > > > > replace acpi & of code to get phy-handle with fwnode_find_reference > > > > > replace of_ and acpi_ code with generic fwnode to get phy-handle. > > > > > > > > > > Calvin Johnson (3): > > > > > device property: Introduce phy related fwnode functions > > > > > net: phy: alphabetically sort header includes > > > > > phylink: Introduce phylink_fwnode_phy_connect() > > > > > > > > Thanks for this, but there's more work that needs to be done here. I > > > > also think that we must have an ack from ACPI people before this can be > > > > accepted - you are in effect proposing a new way for representing PHYs > > > > in ACPI. > > > > > > Thanks for your review. > > > > > > Agree that we need an ack from ACPI people. > > > However, I don't think it is a completely new way as similar acpi approach to > > > get phy-handle is already in place. > > > Please see this: > > > https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c#L832 > > > > That was added by: > > > > commit 8089a96f601bdfe3e1b41d14bb703aafaf1b8f34 > > Author: Iyappan Subramanian <isubramanian@apm.com> > > Date: Mon Jul 25 17:12:41 2016 -0700 > > > > drivers: net: xgene: Add backward compatibility > > > > This patch adds xgene_enet_check_phy_hanlde() function that checks whether > > MDIO driver is probed successfully and sets pdata->mdio_driver to true. > > If MDIO driver is not probed, ethernet driver falls back to backward > > compatibility mode. > > > > Since enum xgene_enet_cmd is used by MDIO driver, removing this from > > ethernet driver. > > > > Signed-off-by: Iyappan Subramanian <isubramanian@apm.com> > > Tested-by: Fushen Chen <fchen@apm.com> > > Tested-by: Toan Le <toanle@apm.com> > > Signed-off-by: David S. Miller <davem@davemloft.net> > > > > The commit message says nothing about adding ACPI stuff, and searching > > the 'net for the posting of this patch seems to suggest that it wasn't > > obviously copied to any ACPI people: > > > > https://lists.openwall.net/netdev/2016/07/26/11 > > > > Annoyingly, searching for: > > > > "drivers: net: xgene: Add backward compatibility" site:lore.kernel.org > > > > doesn't find it on lore, so can't get the full headers and therefore > > addresses. > > > > So, yes, there's another driver using it, but the ACPI folk probably > > never got a look-in on that instance. Even if they had been copied, > > the patch description is probably sufficiently poor that they wouldn't > > have read the patch. > > > > I'd say there's questions over whether ACPI people will find this an > > acceptable approach. > > > > Given that your patch moves this from one driver to a subsystem thing, > > it needs to be ratified by ACPI people, because it's effectively > > becoming a standardised way to represent a PHY in ACPI. > > How can we get attention/response from ACPI people? This is in my queue, but the processing of this has been slow for a while, sorry about that. If you have a new version of the series, please submit it, otherwise ping me in a couple of days if I don't respond to the patches in the meantime. Thanks!
On Mon, Apr 27, 2020 at 06:54:06PM +0530, Calvin Johnson wrote: Hi Russell, others, > Following functions are defined: > phylink_fwnode_phy_connect() > phylink_device_phy_connect() > fwnode_phy_find_device() > device_phy_find_device() > fwnode_get_phy_node() > > First two help in connecting phy to phylink instance. > Next two help in finding a phy on a mdiobus. > Last one helps in getting phy_node from a fwnode. > > Changes in v2: > move phy code from base/property.c to net/phy/phy_device.c > replace acpi & of code to get phy-handle with fwnode_find_reference > replace of_ and acpi_ code with generic fwnode to get phy-handle. > > Calvin Johnson (3): > device property: Introduce phy related fwnode functions > net: phy: alphabetically sort header includes > phylink: Introduce phylink_fwnode_phy_connect() > > drivers/net/phy/phy_device.c | 83 ++++++++++++++++++++++++++++++------ > drivers/net/phy/phylink.c | 68 +++++++++++++++++++++++++++++ > include/linux/phy.h | 3 ++ > include/linux/phylink.h | 6 +++ > 4 files changed, 146 insertions(+), 14 deletions(-) I've a new patch introducing fwnode_mdiobus_register_phy and fwnode_get_phy_id. Can I introduce it in v3 of this patchset or do I need to send it separately? Please advice. Thanks Calvin
Hi Rafael, On Wed, Apr 29, 2020 at 12:26:12PM +0200, Rafael J. Wysocki wrote: > On Wed, Apr 29, 2020 at 7:38 AM Calvin Johnson > <calvin.johnson@oss.nxp.com> wrote: > > <snip> > > > So, yes, there's another driver using it, but the ACPI folk probably > > > never got a look-in on that instance. Even if they had been copied, > > > the patch description is probably sufficiently poor that they wouldn't > > > have read the patch. > > > > > > I'd say there's questions over whether ACPI people will find this an > > > acceptable approach. > > > > > > Given that your patch moves this from one driver to a subsystem thing, > > > it needs to be ratified by ACPI people, because it's effectively > > > becoming a standardised way to represent a PHY in ACPI. > > > > How can we get attention/response from ACPI people? > > This is in my queue, but the processing of this has been slow for a > while, sorry about that. > > If you have a new version of the series, please submit it, otherwise > ping me in a couple of days if I don't respond to the patches in the > meantime. I've posted v3 of the patchset. Can you please review? Thanks Calvin