From: Calvin Johnson <hidden> Date: 2021-01-12 13:42:36
This patch set provides ACPI support to DPAA2 network drivers.
It also introduces new fwnode based APIs to support phylink and phy
layers
Following functions are defined:
phylink_fwnode_phy_connect()
fwnode_mdiobus_register_phy()
fwnode_mdiobus_register()
fwnode_get_phy_id()
fwnode_phy_find_device()
device_phy_find_device()
fwnode_get_phy_node()
fwnode_mdio_find_device()
fwnode_get_id()
First one helps in connecting phy to phylink instance.
Next three helps in getting phy_id and registering phy to mdiobus
Next two help in finding a phy on a mdiobus.
Next one helps in getting phy_node from a fwnode.
Last one is used to get fwnode ID.
Corresponding OF functions are refactored.
END
Changes in v3:
- Add more info on legacy DT properties "phy" and "phy-device"
- Redefine fwnode_phy_find_device() to follow of_phy_find_device()
- Use traditional comparison pattern
- Use GENMASK
- Modified to retrieve reg property value for ACPI as well
- Resolved compilation issue with CONFIG_ACPI = n
- Added more info into documentation
- Use acpi_mdiobus_register()
- Avoid unnecessary line removal
- Remove unused inclusion of acpi.h
Changes in v2:
- Updated with more description in document
- use reverse christmas tree ordering for local variables
- Refactor OF functions to use fwnode functions
Calvin Johnson (15):
Documentation: ACPI: DSD: Document MDIO PHY
net: phy: Introduce fwnode_mdio_find_device()
net: phy: Introduce phy related fwnode functions
of: mdio: Refactor of_phy_find_device()
net: phy: Introduce fwnode_get_phy_id()
of: mdio: Refactor of_get_phy_id()
net: mdiobus: Introduce fwnode_mdiobus_register_phy()
of: mdio: Refactor of_mdiobus_register_phy()
device property: Introduce fwnode_get_id()
net: mdio: Add ACPI support code for mdio
net: mdiobus: Introduce fwnode_mdiobus_register()
net/fsl: Use fwnode_mdiobus_register()
phylink: introduce phylink_fwnode_phy_connect()
net: phylink: Refactor phylink_of_phy_connect()
net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++
MAINTAINERS | 1 +
drivers/base/property.c | 33 +++++
.../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 87 +++++++-----
drivers/net/ethernet/freescale/xgmac_mdio.c | 12 +-
drivers/net/mdio/Kconfig | 7 +
drivers/net/mdio/Makefile | 1 +
drivers/net/mdio/acpi_mdio.c | 49 +++++++
drivers/net/mdio/of_mdio.c | 79 +----------
drivers/net/phy/mdio_bus.c | 87 ++++++++++++
drivers/net/phy/phy_device.c | 106 ++++++++++++++
drivers/net/phy/phylink.c | 49 ++++---
include/linux/acpi_mdio.h | 27 ++++
include/linux/mdio.h | 2 +
include/linux/of_mdio.h | 6 +-
include/linux/phy.h | 32 +++++
include/linux/phylink.h | 3 +
include/linux/property.h | 1 +
18 files changed, 579 insertions(+), 132 deletions(-)
create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
create mode 100644 drivers/net/mdio/acpi_mdio.c
create mode 100644 include/linux/acpi_mdio.h
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Calvin Johnson <hidden> Date: 2021-01-12 13:42:36
Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
provide them to be connected to MAC.
Describe properties "phy-handle" and "phy-mode".
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3: None
Changes in v2:
- Updated with more description in document
Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++
1 file changed, 129 insertions(+)
create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
@@ -0,0 +1,129 @@+.. SPDX-License-Identifier: GPL-2.0++=========================+MDIO bus and PHYs in ACPI+=========================++The PHYs on an MDIO bus [1] are probed and registered using+fwnode_mdiobus_register_phy().+Later, for connecting these PHYs to MAC, the PHYs registered on the+mdiobus have to be referenced.++UUID given below should be used as mentioned in the "Device Properties+UUID For _DSD" [2] document.+- UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301++This document introduces two _DSD properties that are to be used+for PHYs on the MDIO bus.[3]++phy-handle+----------+For each MAC node, a device property "phy-handle" is used to reference+the PHY that is registered on an MDIO bus. This is mandatory for+network interfaces that have PHYs connected to MAC via MDIO bus.++During the MDIO bus driver initialization, PHYs on this bus are probed+using the _ADR object as shown below and are registered on the mdio bus.++::+ Scope(\_SB.MDI0)+ {+ Device(PHY1) {+ Name (_ADR, 0x1)+ } // end of PHY1++ Device(PHY2) {+ Name (_ADR, 0x2)+ } // end of PHY2+ }++Later, during the MAC driver initialization, the registered PHY devices+have to be retrieved from the mdio bus. For this, MAC driver needs+reference to the previously registered PHYs which are provided+using reference to the device as {\_SB.MDI0.PHY1}.++phy-mode+--------+The "phy-mode" _DSD property is used to describe the connection to+the PHY. The valid values for "phy-mode" are defined in [4].+++An ASL example of this is shown below.++DSDT entry for MDIO node+------------------------+The MDIO bus has an SoC component(mdio controller) and a platform+component(PHYs on the mdiobus).++a) Silicon Component+This node describes the MDIO controller,MDI0+--------------------------------------------+::+ Scope(_SB)+ {+ Device(MDI0) {+ Name(_HID, "NXP0006")+ Name(_CCA, 1)+ Name(_UID, 0)+ Name(_CRS, ResourceTemplate() {+ Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)+ Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)+ {+ MDI0_IT+ }+ }) // end of _CRS for MDI0+ } // end of MDI0+ }++b) Platform Component+This node defines the PHYs that are connected to the MDIO bus, MDI0+-------------------------------------------------------------------+::+ Scope(\_SB.MDI0)+ {+ Device(PHY1) {+ Name (_ADR, 0x1)+ } // end of PHY1++ Device(PHY2) {+ Name (_ADR, 0x2)+ } // end of PHY2+ }+++Below are the MAC nodes where PHY nodes are referenced.+phy-mode and phy-handle are used as explained earlier.+------------------------------------------------------+::+ Scope(\_SB.MCE0.PR17)+ {+ Name (_DSD, Package () {+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),+ Package () {+ Package (2) {"phy-mode", "rgmii-id"},+ Package (2) {"phy-handle", \_SB.MDI0.PHY1}+ }+ })+ }++ Scope(\_SB.MCE0.PR18)+ {+ Name (_DSD, Package () {+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),+ Package () {+ Package (2) {"phy-mode", "rgmii-id"},+ Package (2) {"phy-handle", \_SB.MDI0.PHY2}}+ }+ })+ }++References+==========++[1] Documentation/networking/phy.rst++[2] https://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf++[3] Documentation/firmware-guide/acpi/DSD-properties-rules.rst++[4] Documentation/devicetree/bindings/net/ethernet-controller.yaml
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Calvin Johnson <hidden> Date: 2021-01-12 13:43:10
Define fwnode_phy_find_device() to iterate an mdiobus and find the
phy device of the provided phy fwnode. Additionally define
device_phy_find_device() to find phy device of provided device.
Define fwnode_get_phy_node() to get phy_node using named reference.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3:
- Add more info on legacy DT properties "phy" and "phy-device"
- Redefine fwnode_phy_find_device() to follow of_phy_find_device()
Changes in v2:
- use reverse christmas tree ordering for local variables
drivers/net/phy/phy_device.c | 62 ++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 20 ++++++++++++
2 files changed, 82 insertions(+)
@@ -2852,6 +2853,67 @@ struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode)}EXPORT_SYMBOL(fwnode_mdio_find_device);+/**+*fwnode_phy_find_device-Forprovidedphy_fwnode,findphy_device.+*+*@phy_fwnode:Pointertothephy'sfwnode.+*+*Ifsuccessful,returnsapointertothephy_devicewiththeembedded+*structdevicerefcountincrementedbyone,orNULLonfailure.+*/+structphy_device*fwnode_phy_find_device(structfwnode_handle*phy_fwnode)+{+structmdio_device*mdiodev;++mdiodev=fwnode_mdio_find_device(phy_fwnode);+if(!mdiodev)+returnNULL;++if(mdiodev->flags&MDIO_DEVICE_FLAG_PHY)+returnto_phy_device(&mdiodev->dev);++put_device(&mdiodev->dev);++returnNULL;+}+EXPORT_SYMBOL(fwnode_phy_find_device);++/**+*device_phy_find_device-Forthegivendevice,getthephy_device+*@dev:Pointertothegivendevice+*+*Referreturnconditionsoffwnode_phy_find_device().+*/+structphy_device*device_phy_find_device(structdevice*dev)+{+returnfwnode_phy_find_device(dev_fwnode(dev));+}+EXPORT_SYMBOL_GPL(device_phy_find_device);++/**+*fwnode_get_phy_node-Getthephy_nodeusingthenamedreference.+*@fwnode:Pointertofwnodefromwhichphy_nodehastobeobtained.+*+*Referreturnconditionsoffwnode_find_reference().+*ForACPI,only"phy-handle"issupported.LegacyDTproperties"phy"+*and"phy-device"arenotsupportedinACPI.DTsupportsallthethree+*namedreferencestothephynode.+*/+structfwnode_handle*fwnode_get_phy_node(structfwnode_handle*fwnode)+{+structfwnode_handle*phy_node;++/* Only phy-handle is used for ACPI */+phy_node=fwnode_find_reference(fwnode,"phy-handle",0);+if(is_acpi_node(fwnode)||!IS_ERR(phy_node))+returnphy_node;+phy_node=fwnode_find_reference(fwnode,"phy",0);+if(IS_ERR(phy_node))+phy_node=fwnode_find_reference(fwnode,"phy-device",0);+returnphy_node;+}+EXPORT_SYMBOL_GPL(fwnode_get_phy_node);+/***phy_probe-probeandinitaPHYdevice*@dev:devicetoprobeandinit
From: Calvin Johnson <hidden> Date: 2021-01-12 13:43:10
With the introduction of fwnode_get_phy_id(), refactor of_get_phy_id()
to use fwnode equivalent.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3: None
Changes in v2: None
drivers/net/mdio/of_mdio.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
From: Calvin Johnson <hidden> Date: 2021-01-12 13:43:11
Introduce fwnode_mdiobus_register_phy() to register PHYs on the
mdiobus. From the compatible string, identify whether the PHY is
c45 and based on this create a PHY device instance which is
registered on the mdiobus.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3: None
Changes in v2: None
drivers/net/mdio/of_mdio.c | 3 +-
drivers/net/phy/mdio_bus.c | 67 ++++++++++++++++++++++++++++++++++++++
include/linux/mdio.h | 2 ++
include/linux/of_mdio.h | 6 +++-
4 files changed, 76 insertions(+), 2 deletions(-)
@@ -106,6 +107,72 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev)}EXPORT_SYMBOL(mdiobus_unregister_device);+intfwnode_mdiobus_register_phy(structmii_bus*bus,+structfwnode_handle*child,u32addr)+{+structmii_timestamper*mii_ts;+structphy_device*phy;+boolis_c45=false;+u32phy_id;+intrc;++if(is_of_node(child)){+mii_ts=of_find_mii_timestamper(to_of_node(child));+if(IS_ERR(mii_ts))+returnPTR_ERR(mii_ts);+}++rc=fwnode_property_match_string(child,"compatible","ethernet-phy-ieee802.3-c45");+if(rc>=0)+is_c45=true;++if(is_c45||fwnode_get_phy_id(child,&phy_id))+phy=get_phy_device(bus,addr,is_c45);+else+phy=phy_device_create(bus,addr,phy_id,0,NULL);+if(IS_ERR(phy)){+if(mii_ts&&is_of_node(child))+unregister_mii_timestamper(mii_ts);+returnPTR_ERR(phy);+}++if(is_acpi_node(child)){+phy->irq=bus->irq[addr];++/* Associate the fwnode with the device structure so it+*canbelookeduplater.+*/+phy->mdio.dev.fwnode=child;++/* All data is now stored in the phy struct, so register it */+rc=phy_device_register(phy);+if(rc){+phy_device_free(phy);+fwnode_handle_put(phy->mdio.dev.fwnode);+returnrc;+}++dev_dbg(&bus->dev,"registered phy at address %i\n",addr);+}elseif(is_of_node(child)){+rc=of_mdiobus_phy_device_register(bus,phy,to_of_node(child),addr);+if(rc){+if(mii_ts)+unregister_mii_timestamper(mii_ts);+phy_device_free(phy);+returnrc;+}++/* phy->mii_ts may already be defined by the PHY driver. A+*mii_timestamperprobedviathedevicetreewillstillhave+*precedence.+*/+if(mii_ts)+phy->mii_ts=mii_ts;+}+return0;+}+EXPORT_SYMBOL(fwnode_mdiobus_register_phy);+structphy_device*mdiobus_get_phy(structmii_bus*bus,intaddr){structmdio_device*mdiodev=bus->mdio_map[addr];
From: Calvin Johnson <hidden> Date: 2021-01-12 13:43:12
Define fwnode_mdio_find_device() to get a pointer to the
mdio_device from fwnode passed to the function.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3: None
Changes in v2: None
drivers/net/mdio/of_mdio.c | 11 +----------
drivers/net/phy/phy_device.c | 23 +++++++++++++++++++++++
include/linux/phy.h | 6 ++++++
3 files changed, 30 insertions(+), 10 deletions(-)
@@ -98,45 +98,7 @@ EXPORT_SYMBOL(of_mdiobus_phy_device_register);staticintof_mdiobus_register_phy(structmii_bus*mdio,structdevice_node*child,u32addr){-structmii_timestamper*mii_ts;-structphy_device*phy;-boolis_c45;-intrc;-u32phy_id;--mii_ts=of_find_mii_timestamper(child);-if(IS_ERR(mii_ts))-returnPTR_ERR(mii_ts);--is_c45=of_device_is_compatible(child,-"ethernet-phy-ieee802.3-c45");--if(!is_c45&&!of_get_phy_id(child,&phy_id))-phy=phy_device_create(mdio,addr,phy_id,0,NULL);-else-phy=get_phy_device(mdio,addr,is_c45);-if(IS_ERR(phy)){-if(mii_ts)-unregister_mii_timestamper(mii_ts);-returnPTR_ERR(phy);-}--rc=of_mdiobus_phy_device_register(mdio,phy,child,addr);-if(rc){-if(mii_ts)-unregister_mii_timestamper(mii_ts);-phy_device_free(phy);-returnrc;-}--/* phy->mii_ts may already be defined by the PHY driver. A-*mii_timestamperprobedviathedevicetreewillstillhave-*precedence.-*/-if(mii_ts)-phy->mii_ts=mii_ts;--return0;+returnfwnode_mdiobus_register_phy(mdio,of_fwnode_handle(child),addr);}staticintof_mdiobus_register_device(structmii_bus*mdio,
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Calvin Johnson <hidden> Date: 2021-01-12 13:43:54
Extract phy_id from compatible string. This will be used by
fwnode_mdiobus_register_phy() to create phy device using the
phy_id.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3:
- Use traditional comparison pattern
- Use GENMASK
Changes in v2: None
drivers/net/phy/phy_device.c | 21 +++++++++++++++++++++
include/linux/phy.h | 5 +++++
2 files changed, 26 insertions(+)
@@ -846,6 +846,27 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)return0;}+/* Extract the phy ID from the compatible string of the form+*ethernet-phy-idAAAA.BBBB.+*/+intfwnode_get_phy_id(structfwnode_handle*fwnode,u32*phy_id)+{+unsignedintupper,lower;+constchar*cp;+intret;++ret=fwnode_property_read_string(fwnode,"compatible",&cp);+if(ret)+returnret;++if(sscanf(cp,"ethernet-phy-id%4x.%4x",&upper,&lower)!=2)+return-EINVAL;++*phy_id=((upper&GENMASK(15,0))<<16)|(lower&GENMASK(15,0));+return0;+}+EXPORT_SYMBOL(fwnode_get_phy_id);+/***get_phy_device-readsthespecifiedPHYdeviceandreturnsits@phy_device*struct
From: Calvin Johnson <hidden> Date: 2021-01-12 13:43:55
Using fwnode_get_id(), get the reg property value for DT node
or get the _ADR object value for ACPI node.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3:
- Modified to retrieve reg property value for ACPI as well
- Resolved compilation issue with CONFIG_ACPI = n
- Added more info into documentation
Changes in v2: None
drivers/base/property.c | 33 +++++++++++++++++++++++++++++++++
include/linux/property.h | 1 +
2 files changed, 34 insertions(+)
@@ -2,6 +2,7 @@# Makefile for Linux MDIO bus driversobj-$(CONFIG_OF_MDIO)+=of_mdio.o+obj-$(CONFIG_ACPI_MDIO)+=acpi_mdio.oobj-$(CONFIG_MDIO_ASPEED)+=mdio-aspeed.oobj-$(CONFIG_MDIO_BCM_IPROC)+=mdio-bcm-iproc.o
@@ -0,0 +1,49 @@+// SPDX-License-Identifier: GPL-2.0-only+/*+*ACPIhelpersfortheMDIO(EthernetPHY)API+*+*ThisfileprovideshelperfunctionsforextractingPHYdeviceinformation+*outoftheACPIASLandusingittopopulateanmii_bus.+*/++#include<linux/acpi.h>+#include<linux/acpi_mdio.h>++/**+*acpi_mdiobus_register-Registermii_busandcreatePHYsfromtheACPIASL.+*+*@mdio:pointertomii_busstructure+*@fwnode:pointertofwnodeofMDIObus.+*+*Thisfunctionregistersthemii_busstructureandregistersaphy_device+*foreachchildnodeof@fwnode.+*/+intacpi_mdiobus_register(structmii_bus*mdio,structfwnode_handle*fwnode)+{+structfwnode_handle*child;+u32addr;+intret;++/* Mask out all PHYs from auto probing. */+mdio->phy_mask=~0;+ret=mdiobus_register(mdio);+if(ret)+returnret;++mdio->dev.fwnode=fwnode;+/* Loop over the child nodes and register a phy_device for each PHY */+fwnode_for_each_child_node(fwnode,child){+ret=fwnode_get_id(child,&addr);++if(addr>=PHY_MAX_ADDR)+continue;++ret=fwnode_mdiobus_register_phy(mdio,child,addr);+if(ret==-ENODEV)+dev_err(&mdio->dev,+"MDIO device at address %d is missing.\n",+addr);+}+return0;+}+EXPORT_SYMBOL(acpi_mdiobus_register);
From: Calvin Johnson <hidden> Date: 2021-01-12 13:44:26
Introduce fwnode_mdiobus_register() to register PHYs on the mdiobus.
If the fwnode is DT node, then call of_mdiobus_register().
If it is an ACPI node, then call acpi_mdiobus_register().
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3:
- Use acpi_mdiobus_register()
Changes in v2: None
drivers/net/phy/mdio_bus.c | 20 ++++++++++++++++++++
include/linux/phy.h | 1 +
2 files changed, 21 insertions(+)
From: Calvin Johnson <hidden> Date: 2021-01-12 13:44:27
Define phylink_fwnode_phy_connect() to connect phy specified by
a fwnode to a phylink instance.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3: None
Changes in v2: None
drivers/net/phy/phylink.c | 54 +++++++++++++++++++++++++++++++++++++++
include/linux/phylink.h | 3 +++
2 files changed, 57 insertions(+)
@@ -1120,6 +1121,59 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,}EXPORT_SYMBOL_GPL(phylink_of_phy_connect);+/**+*phylink_fwnode_phy_connect()-connectthePHYspecifiedinthefwnode.+*@pl:apointertoa&structphylinkreturnedfromphylink_create()+*@fwnode:apointertoa&structfwnode_handle.+*@flags:PHY-specificflagstocommunicatetothePHYdevicedriver+*+*Connectthephyspecified@fwnodetothephylinkinstancespecified+*by@pl.+*+*Returns0onsuccessoranegativeerrno.+*/+intphylink_fwnode_phy_connect(structphylink*pl,+structfwnode_handle*fwnode,+u32flags)+{+structfwnode_handle*phy_fwnode;+structphy_device*phy_dev;+intret;++if(is_of_node(fwnode)){+/* Fixed links and 802.3z are handled without needing a PHY */+if(pl->cfg_link_an_mode==MLO_AN_FIXED||+(pl->cfg_link_an_mode==MLO_AN_INBAND&&+phy_interface_mode_is_8023z(pl->link_interface)))+return0;+}++phy_fwnode=fwnode_get_phy_node(fwnode);+if(IS_ERR(phy_fwnode)){+if(pl->cfg_link_an_mode==MLO_AN_PHY)+return-ENODEV;+return0;+}++phy_dev=fwnode_phy_find_device(phy_fwnode);+/* We're done with the phy_node handle */+fwnode_handle_put(phy_fwnode);+if(!phy_dev)+return-ENODEV;++ret=phy_attach_direct(pl->netdev,phy_dev,flags,+pl->link_interface);+if(ret)+returnret;++ret=phylink_bringup_phy(pl,phy_dev,pl->link_config.interface);+if(ret)+phy_detach(phy_dev);++returnret;+}+EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect);+/***phylink_disconnect_phy()-disconnectanyPHYattachedtothephylink*instance.
From: Calvin Johnson <hidden> Date: 2021-01-12 13:44:27
fwnode_mdiobus_register() internally takes care of both DT
and ACPI cases to register mdiobus. Replace existing
of_mdiobus_register() with fwnode_mdiobus_register().
Note: For both ACPI and DT cases, endianness of MDIO controller
need to be specified using "little-endian" property.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3:
- Avoid unnecessary line removal
- Remove unused inclusion of acpi.h
Changes in v2: None
drivers/net/ethernet/freescale/xgmac_mdio.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
@@ -243,10 +244,9 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum)staticintxgmac_mdio_probe(structplatform_device*pdev){-structdevice_node*np=pdev->dev.of_node;-structmii_bus*bus;-structresource*res;structmdio_fsl_priv*priv;+structresource*res;+structmii_bus*bus;intret;/* In DPAA-1, MDIO is one of the many FMan sub-devices. The FMan
@@ -279,13 +279,15 @@ static int xgmac_mdio_probe(struct platform_device *pdev)gotoerr_ioremap;}+/* For both ACPI and DT cases, endianness of MDIO controller+*needtobespecifiedusing"little-endian"property.+*/priv->is_little_endian=device_property_read_bool(&pdev->dev,"little-endian");priv->has_a011043=device_property_read_bool(&pdev->dev,"fsl,erratum-a011043");--ret=of_mdiobus_register(bus,np);+ret=fwnode_mdiobus_register(bus,pdev->dev.fwnode);if(ret){dev_err(&pdev->dev,"cannot register MDIO bus\n");gotoerr_registration;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -1080,44 +1080,7 @@ EXPORT_SYMBOL_GPL(phylink_connect_phy);intphylink_of_phy_connect(structphylink*pl,structdevice_node*dn,u32flags){-structdevice_node*phy_node;-structphy_device*phy_dev;-intret;--/* Fixed links and 802.3z are handled without needing a PHY */-if(pl->cfg_link_an_mode==MLO_AN_FIXED||-(pl->cfg_link_an_mode==MLO_AN_INBAND&&-phy_interface_mode_is_8023z(pl->link_interface)))-return0;--phy_node=of_parse_phandle(dn,"phy-handle",0);-if(!phy_node)-phy_node=of_parse_phandle(dn,"phy",0);-if(!phy_node)-phy_node=of_parse_phandle(dn,"phy-device",0);--if(!phy_node){-if(pl->cfg_link_an_mode==MLO_AN_PHY)-return-ENODEV;-return0;-}--phy_dev=of_phy_find_device(phy_node);-/* We're done with the phy_node handle */-of_node_put(phy_node);-if(!phy_dev)-return-ENODEV;--ret=phy_attach_direct(pl->netdev,phy_dev,flags,-pl->link_interface);-if(ret)-returnret;--ret=phylink_bringup_phy(pl,phy_dev,pl->link_config.interface);-if(ret)-phy_detach(phy_dev);--returnret;+returnphylink_fwnode_phy_connect(pl,of_fwnode_handle(dn),flags);}EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Calvin Johnson <hidden> Date: 2021-01-12 13:45:21
Modify dpaa2_mac_connect() to support ACPI along with DT.
Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
DT or ACPI.
Replace of_get_phy_mode with fwnode_get_phy_mode to get
phy-mode for a dpmac_node.
Use helper function phylink_fwnode_phy_connect() to find phy_dev and
connect to mac->phylink.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3: None
Changes in v2:
- Refactor OF functions to use fwnode functions
.../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 87 +++++++++++--------
1 file changed, 50 insertions(+), 37 deletions(-)
@@ -34,39 +37,47 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode)return0;}-/* Caller must call of_node_put on the returned value */-staticstructdevice_node*dpaa2_mac_get_node(u16dpmac_id)+staticstructfwnode_handle*dpaa2_mac_get_node(structdevice*dev,+u16dpmac_id){-structdevice_node*dpmacs,*dpmac=NULL;-u32id;+structdevice_node*dpmacs=NULL;+structfwnode_handle*parent,*child=NULL;interr;+u32id;-dpmacs=of_find_node_by_name(NULL,"dpmacs");-if(!dpmacs)-returnNULL;+if(is_of_node(dev->parent->fwnode)){+dpmacs=of_find_node_by_name(NULL,"dpmacs");+if(!dpmacs)+returnNULL;+parent=of_fwnode_handle(dpmacs);+}elseif(is_acpi_node(dev->parent->fwnode)){+parent=dev->parent->fwnode;+}-while((dpmac=of_get_next_child(dpmacs,dpmac))!=NULL){-err=of_property_read_u32(dpmac,"reg",&id);-if(err)+fwnode_for_each_child_node(parent,child){+err=fwnode_get_id(child,&id);+if(err){continue;-if(id==dpmac_id)-break;+}elseif(id==dpmac_id){+if(is_of_node(dev->parent->fwnode))+of_node_put(dpmacs);+returnchild;+}}--of_node_put(dpmacs);--returndpmac;+if(is_of_node(dev->parent->fwnode))+of_node_put(dpmacs);+returnNULL;}-staticintdpaa2_mac_get_if_mode(structdevice_node*node,+staticintdpaa2_mac_get_if_mode(structfwnode_handle*dpmac_node,structdpmac_attrattr){phy_interface_tif_mode;interr;-err=of_get_phy_mode(node,&if_mode);-if(!err)-returnif_mode;+err=fwnode_get_phy_mode(dpmac_node);+if(err>0)+returnerr;err=phy_mode(attr.eth_if,&if_mode);if(!err)
@@ -221,26 +232,27 @@ static const struct phylink_mac_ops dpaa2_mac_phylink_ops = {};staticintdpaa2_pcs_create(structdpaa2_mac*mac,-structdevice_node*dpmac_node,intid)+structfwnode_handle*dpmac_node,+intid){structmdio_device*mdiodev;-structdevice_node*node;+structfwnode_handle*node;-node=of_parse_phandle(dpmac_node,"pcs-handle",0);-if(!node){+node=fwnode_find_reference(dpmac_node,"pcs-handle",0);+if(IS_ERR(node)){/* do not error out on old DTS files */netdev_warn(mac->net_dev,"pcs-handle node not found\n");return0;}-if(!of_device_is_available(node)){+if(!of_device_is_available(to_of_node(node))){netdev_err(mac->net_dev,"pcs-handle node not available\n");-of_node_put(node);+of_node_put(to_of_node(node));return-ENODEV;}-mdiodev=of_mdio_find_device(node);-of_node_put(node);+mdiodev=fwnode_mdio_find_device(node);+fwnode_handle_put(node);if(!mdiodev)return-EPROBE_DEFER;
From: Andy Shevchenko <hidden> Date: 2021-01-12 15:49:06
On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
[off-list ref] wrote:
quoted hunk
Using fwnode_get_id(), get the reg property value for DT node
or get the _ADR object value for ACPI node.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3:
- Modified to retrieve reg property value for ACPI as well
- Resolved compilation issue with CONFIG_ACPI = n
- Added more info into documentation
Changes in v2: None
drivers/base/property.c | 33 +++++++++++++++++++++++++++++++++
include/linux/property.h | 1 +
2 files changed, 34 insertions(+)
From: Andy Shevchenko <hidden> Date: 2021-01-12 15:52:14
On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
[off-list ref] wrote:
Introduce fwnode_mdiobus_register() to register PHYs on the mdiobus.
If the fwnode is DT node, then call of_mdiobus_register().
If it is an ACPI node, then call acpi_mdiobus_register().
...
+/**
+ * fwnode_mdiobus_register - Register mii_bus and create PHYs from fwnode
+ * @mdio: pointer to mii_bus structure
+ * @fwnode: pointer to fwnode of MDIO bus.
+ *
+ * This function returns of_mdiobus_register() for DT and
+ * acpi_mdiobus_register() for ACPI.
+ */
+int fwnode_mdiobus_register(struct mii_bus *mdio, struct fwnode_handle *fwnode)
+{
+ if (is_of_node(fwnode))
+ return of_mdiobus_register(mdio, to_of_node(fwnode));
+ else if (is_acpi_node(fwnode))
From: Andy Shevchenko <hidden> Date: 2021-01-12 15:53:28
On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson
[off-list ref] wrote:
fwnode_mdiobus_register() internally takes care of both DT
and ACPI cases to register mdiobus. Replace existing
of_mdiobus_register() with fwnode_mdiobus_register().
Note: For both ACPI and DT cases, endianness of MDIO controller
need to be specified using "little-endian" property.
+ /* For both ACPI and DT cases, endianness of MDIO controller
+ * need to be specified using "little-endian" property.
From: Andy Shevchenko <hidden> Date: 2021-01-12 15:57:09
On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson
[off-list ref] wrote:
Refactor phylink_of_phy_connect() to use phylink_fwnode_phy_connect().
Same Q as per previous patch. If it's indeed a bug in the existing
code, should be fixed in a separate patch
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
[off-list ref] wrote:
quoted hunk
Using fwnode_get_id(), get the reg property value for DT node
or get the _ADR object value for ACPI node.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3:
- Modified to retrieve reg property value for ACPI as well
- Resolved compilation issue with CONFIG_ACPI = n
- Added more info into documentation
Changes in v2: None
drivers/base/property.c | 33 +++++++++++++++++++++++++++++++++
include/linux/property.h | 1 +
2 files changed, 34 insertions(+)
Please don't do it this way. The whole point of fwnode_operations is
to avoid conditional stuff at the fwnode level. Also ACPI and DT
aren't mutually exclusive if I'm not mistaken.
Also, can you CC me on the entire series please? I want to reply to
some of your other patches too. Most of the fwnode changes don't seem
right. fwnode is lower level that the device-driver framework. Making
it aware of busses like mdio, etc doesn't sound right. Also, there's
already get_dev_from_fwnode() which is a much more efficient way to
look up/get a device from a fwnode instead of looping through a bus.
-Saravana
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Andy Shevchenko <hidden> Date: 2021-01-12 18:03:51
On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
[off-list ref] wrote:
quoted
Using fwnode_get_id(), get the reg property value for DT node
or get the _ADR object value for ACPI node.
...
quoted
+/**
+ * fwnode_get_id - Get the id of a fwnode.
+ * @fwnode: firmware node
+ * @id: id of the fwnode
+ *
+ * This function provides the id of a fwnode which can be either
+ * DT or ACPI node. For ACPI, "reg" property value, if present will
+ * be provided or else _ADR value will be provided.
+ * Returns 0 on success or a negative errno.
+ */
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
+{
+#ifdef CONFIG_ACPI
+ unsigned long long adr;
+ acpi_status status;
+#endif
+ int ret;
+
+ ret = fwnode_property_read_u32(fwnode, "reg", id);
+ if (!(ret && is_acpi_node(fwnode)))
+ return ret;
+
+#ifdef CONFIG_ACPI
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+ METHOD_NAME__ADR, NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+ *id = (u32)adr;
+#endif
+ return 0;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_id);
Please don't do it this way. The whole point of fwnode_operations is
to avoid conditional stuff at the fwnode level.
Not fully true. We have non-POD getters that are conditional. Moreover,
we have additional layer of Primary / Secondary fwnodes on top of that.
The caller of fwnode API is indeed agnostic, but under the hood it differs by
the definition (obviously due to natural differences between ACPI and DT and
whatever else might come in the future.
Also ACPI and DT
aren't mutually exclusive if I'm not mistaken.
That's why we try 'reg' property for both cases first.
is_acpi_fwnode() conditional is that what I don't like though.
...
fwnode is lower level that the device-driver framework.
Agree.
Making
it aware of busses like mdio, etc doesn't sound right.
Disagree. Conceptually resource providers can be quite different and fwnode API
*is* LCM for them.
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Calvin Johnson <hidden> Date: 2021-01-15 10:48:26
Hi,
On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
[off-list ref] wrote:
quoted
Using fwnode_get_id(), get the reg property value for DT node
or get the _ADR object value for ACPI node.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3:
- Modified to retrieve reg property value for ACPI as well
- Resolved compilation issue with CONFIG_ACPI = n
- Added more info into documentation
Changes in v2: None
drivers/base/property.c | 33 +++++++++++++++++++++++++++++++++
include/linux/property.h | 1 +
2 files changed, 34 insertions(+)
Please don't do it this way. The whole point of fwnode_operations is
to avoid conditional stuff at the fwnode level. Also ACPI and DT
aren't mutually exclusive if I'm not mistaken.
Also, can you CC me on the entire series please? I want to reply to
some of your other patches too. Most of the fwnode changes don't seem
right. fwnode is lower level that the device-driver framework. Making
it aware of busses like mdio, etc doesn't sound right. Also, there's
already get_dev_from_fwnode() which is a much more efficient way to
look up/get a device from a fwnode instead of looping through a bus.
Thanks for reviewing the patch. I'll add you in the upcoming v4 series.
There is lot of history into patch series. It would be helpful, if you can
search for related submissions in the past and get some background.
Regards
Calvin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Randy Dunlap <hidden> Date: 2021-01-18 22:14:10
Hi,
On 1/12/21 5:40 AM, Calvin Johnson wrote:
quoted hunk
Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
provide them to be connected to MAC.
Describe properties "phy-handle" and "phy-mode".
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3: None
Changes in v2:
- Updated with more description in document
Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++
1 file changed, 129 insertions(+)
create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
@@ -0,0 +1,129 @@+.. SPDX-License-Identifier: GPL-2.0++=========================+MDIO bus and PHYs in ACPI+=========================++The PHYs on an MDIO bus [1] are probed and registered using+fwnode_mdiobus_register_phy().+Later, for connecting these PHYs to MAC, the PHYs registered on the+mdiobus have to be referenced.++UUID given below should be used as mentioned in the "Device Properties
The UUID given below
+UUID For _DSD" [2] document.
+ - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301
+
+This document introduces two _DSD properties that are to be used
+for PHYs on the MDIO bus.[3]
+
+phy-handle
+----------
+For each MAC node, a device property "phy-handle" is used to reference
+the PHY that is registered on an MDIO bus. This is mandatory for
+network interfaces that have PHYs connected to MAC via MDIO bus.
+
+During the MDIO bus driver initialization, PHYs on this bus are probed
+using the _ADR object as shown below and are registered on the mdio bus.
s/mdio/MDIO/ (please be consistent, as 3 lines above)
+
+::
+ Scope(\_SB.MDI0)
+ {
+ Device(PHY1) {
+ Name (_ADR, 0x1)
+ } // end of PHY1
+
+ Device(PHY2) {
+ Name (_ADR, 0x2)
+ } // end of PHY2
+ }
+
+Later, during the MAC driver initialization, the registered PHY devices
+have to be retrieved from the mdio bus. For this, MAC driver needs
ditto.
+reference to the previously registered PHYs which are provided
+using reference to the device as {\_SB.MDI0.PHY1}.
+
+phy-mode
+--------
+The "phy-mode" _DSD property is used to describe the connection to
+the PHY. The valid values for "phy-mode" are defined in [4].
+
+
+An ASL example of this is shown below.
+
+DSDT entry for MDIO node
+------------------------
+The MDIO bus has an SoC component(mdio controller) and a platform
component (MDIO controller)
+component(PHYs on the mdiobus).
component (PHYs
+
+a) Silicon Component
+This node describes the MDIO controller,MDI0
controller, MDI0
+--------------------------------------------
and then one more '-', please.
+::
+ Scope(_SB)
+ {
+ Device(MDI0) {
+ Name(_HID, "NXP0006")
+ Name(_CCA, 1)
+ Name(_UID, 0)
+ Name(_CRS, ResourceTemplate() {
+ Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
+ Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
+ {
+ MDI0_IT
+ }
+ }) // end of _CRS for MDI0
+ } // end of MDI0
+ }
+
+b) Platform Component
+This node defines the PHYs that are connected to the MDIO bus, MDI0
+-------------------------------------------------------------------
[deletia]
thanks.
--
~Randy
You can't do anything without having to do something else first.
-- Belefant's Law
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Calvin Johnson <hidden> Date: 2021-01-19 04:11:17
On Tue, Jan 12, 2021 at 05:57:01PM +0200, Andy Shevchenko wrote:
On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson
[off-list ref] wrote:
quoted
Refactor phylink_of_phy_connect() to use phylink_fwnode_phy_connect().
Same Q as per previous patch. If it's indeed a bug in the existing
code, should be fixed in a separate patch
Sorry, I didn't get what you meant. This patch just refactors
phylink_of_phy_connect(). There is no bug fixing.
Regards
Calvin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-01-20 18:27:28
On Tue, Jan 12, 2021 at 4:47 PM Andy Shevchenko
[off-list ref] wrote:
On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
[off-list ref] wrote:
quoted
Using fwnode_get_id(), get the reg property value for DT node
or get the _ADR object value for ACPI node.
Signed-off-by: Calvin Johnson <redacted>
---
Changes in v3:
- Modified to retrieve reg property value for ACPI as well
- Resolved compilation issue with CONFIG_ACPI = n
- Added more info into documentation
Changes in v2: None
drivers/base/property.c | 33 +++++++++++++++++++++++++++++++++
include/linux/property.h | 1 +
2 files changed, 34 insertions(+)
Shouldn't be
return 0;
#else
return -EINVAL;
#endif
?
Yes, it's a theoretical case when is_acpi_node() returns true when
CONFIG_ACPI=n.
How so? is_acpi_node() is defined as a static inline returning false then.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-01-20 18:52:22
On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
[off-list ref] wrote:
On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
quoted
On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
[off-list ref] wrote:
quoted
Using fwnode_get_id(), get the reg property value for DT node
or get the _ADR object value for ACPI node.
...
quoted
quoted
+/**
+ * fwnode_get_id - Get the id of a fwnode.
+ * @fwnode: firmware node
+ * @id: id of the fwnode
+ *
+ * This function provides the id of a fwnode which can be either
+ * DT or ACPI node. For ACPI, "reg" property value, if present will
+ * be provided or else _ADR value will be provided.
+ * Returns 0 on success or a negative errno.
+ */
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
+{
+#ifdef CONFIG_ACPI
+ unsigned long long adr;
+ acpi_status status;
+#endif
+ int ret;
+
+ ret = fwnode_property_read_u32(fwnode, "reg", id);
+ if (!(ret && is_acpi_node(fwnode)))
+ return ret;
+
+#ifdef CONFIG_ACPI
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+ METHOD_NAME__ADR, NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+ *id = (u32)adr;
+#endif
+ return 0;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_id);
quoted
Please don't do it this way. The whole point of fwnode_operations is
to avoid conditional stuff at the fwnode level.
Not fully true. We have non-POD getters that are conditional. Moreover,
we have additional layer of Primary / Secondary fwnodes on top of that.
The caller of fwnode API is indeed agnostic, but under the hood it differs by
the definition (obviously due to natural differences between ACPI and DT and
whatever else might come in the future.
quoted
Also ACPI and DT
aren't mutually exclusive if I'm not mistaken.
That's why we try 'reg' property for both cases first.
is_acpi_fwnode() conditional is that what I don't like though.
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-01-20 19:14:34
On Wed, Jan 20, 2021 at 7:44 PM Andy Shevchenko
[off-list ref] wrote:
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 4:47 PM Andy Shevchenko
[off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
[off-list ref] wrote:
...
quoted
quoted
quoted
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
+{
+#ifdef CONFIG_ACPI
+ unsigned long long adr;
+ acpi_status status;
+#endif
+ int ret;
+
+ ret = fwnode_property_read_u32(fwnode, "reg", id);
+ if (!(ret && is_acpi_node(fwnode)))
+ return ret;
+
+#ifdef CONFIG_ACPI
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+ METHOD_NAME__ADR, NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+ *id = (u32)adr;
Shouldn't be
return 0;
#else
return -EINVAL;
#endif
?
Yes, it's a theoretical case when is_acpi_node() returns true when
CONFIG_ACPI=n.
How so? is_acpi_node() is defined as a static inline returning false then.
I understand that, that's why it's pure theoretical when, for example,
the semantics is changed. But I believe it's unlucky to happen.
Changing the definition of it for CONFIG_ACPI=n would be a regression
given the current usage of it.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Andy Shevchenko <hidden> Date: 2021-01-20 19:17:40
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki [off-list ref] wrote:
On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
[off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
quoted
On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
[off-list ref] wrote:
...
quoted
quoted
quoted
+ ret = fwnode_property_read_u32(fwnode, "reg", id);
+ if (!(ret && is_acpi_node(fwnode)))
+ return ret;
+
+#ifdef CONFIG_ACPI
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+ METHOD_NAME__ADR, NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+ *id = (u32)adr;
+#endif
+ return 0;
quoted
quoted
Also ACPI and DT
aren't mutually exclusive if I'm not mistaken.
That's why we try 'reg' property for both cases first.
is_acpi_fwnode() conditional is that what I don't like though.
I'm not sure what you mean here, care to elaborate?
I meant is_acpi_node(fwnode) in the conditional.
I think it's redundant and we can simple do something like this:
if (ret) {
#ifdef ACPI
...
#else
return ret;
#endif
}
return 0;
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-01-20 19:17:40
On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
[off-list ref] wrote:
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
[off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
quoted
On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
[off-list ref] wrote:
...
quoted
quoted
quoted
quoted
+ ret = fwnode_property_read_u32(fwnode, "reg", id);
+ if (!(ret && is_acpi_node(fwnode)))
+ return ret;
+
+#ifdef CONFIG_ACPI
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+ METHOD_NAME__ADR, NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+ *id = (u32)adr;
+#endif
+ return 0;
quoted
quoted
quoted
Also ACPI and DT
aren't mutually exclusive if I'm not mistaken.
That's why we try 'reg' property for both cases first.
is_acpi_fwnode() conditional is that what I don't like though.
I'm not sure what you mean here, care to elaborate?
I meant is_acpi_node(fwnode) in the conditional.
I think it's redundant and we can simple do something like this:
if (ret) {
#ifdef ACPI
...
#else
return ret;
#endif
}
return 0;
--
From: Andy Shevchenko <hidden> Date: 2021-01-20 19:32:18
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki [off-list ref] wrote:
On Tue, Jan 12, 2021 at 4:47 PM Andy Shevchenko
[off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 3:42 PM Calvin Johnson
[off-list ref] wrote:
...
quoted
quoted
+int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
+{
+#ifdef CONFIG_ACPI
+ unsigned long long adr;
+ acpi_status status;
+#endif
+ int ret;
+
+ ret = fwnode_property_read_u32(fwnode, "reg", id);
+ if (!(ret && is_acpi_node(fwnode)))
+ return ret;
+
+#ifdef CONFIG_ACPI
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+ METHOD_NAME__ADR, NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+ *id = (u32)adr;
Shouldn't be
return 0;
#else
return -EINVAL;
#endif
?
Yes, it's a theoretical case when is_acpi_node() returns true when
CONFIG_ACPI=n.
How so? is_acpi_node() is defined as a static inline returning false then.
I understand that, that's why it's pure theoretical when, for example,
the semantics is changed. But I believe it's unlucky to happen.
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki [off-list ref] wrote:
On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
[off-list ref] wrote:
quoted
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
[off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
quoted
On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
[off-list ref] wrote:
...
quoted
quoted
quoted
quoted
+ ret = fwnode_property_read_u32(fwnode, "reg", id);
+ if (!(ret && is_acpi_node(fwnode)))
+ return ret;
+
+#ifdef CONFIG_ACPI
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+ METHOD_NAME__ADR, NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+ *id = (u32)adr;
+#endif
+ return 0;
quoted
quoted
quoted
Also ACPI and DT
aren't mutually exclusive if I'm not mistaken.
That's why we try 'reg' property for both cases first.
is_acpi_fwnode() conditional is that what I don't like though.
I'm not sure what you mean here, care to elaborate?
I meant is_acpi_node(fwnode) in the conditional.
I think it's redundant and we can simple do something like this:
if (ret) {
#ifdef ACPI
...
#else
return ret;
#endif
}
return 0;
--
Right, that should work. And I'd prefer it too.
Rafael,
I'd rather this new function be an ops instead of a bunch of #ifdef or
if (acpi) checks. Thoughts?
-Saravana
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-01-22 16:46:24
On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan [off-list ref] wrote:
On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki [off-list ref] wrote:
quoted
On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
[off-list ref] wrote:
quoted
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
[off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
quoted
On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
[off-list ref] wrote:
...
quoted
quoted
quoted
quoted
+ ret = fwnode_property_read_u32(fwnode, "reg", id);
+ if (!(ret && is_acpi_node(fwnode)))
+ return ret;
+
+#ifdef CONFIG_ACPI
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+ METHOD_NAME__ADR, NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+ *id = (u32)adr;
+#endif
+ return 0;
quoted
quoted
quoted
Also ACPI and DT
aren't mutually exclusive if I'm not mistaken.
That's why we try 'reg' property for both cases first.
is_acpi_fwnode() conditional is that what I don't like though.
I'm not sure what you mean here, care to elaborate?
I meant is_acpi_node(fwnode) in the conditional.
I think it's redundant and we can simple do something like this:
if (ret) {
#ifdef ACPI
...
#else
return ret;
#endif
}
return 0;
--
Right, that should work. And I'd prefer it too.
Rafael,
I'd rather this new function be an ops instead of a bunch of #ifdef or
if (acpi) checks. Thoughts?
Well, it looks more like a helper function than like an op and I'm not
even sure how many potential users of it will expect that _ADR should
be evaluated in the absence of the "reg" property.
It's just that the "reg" property happens to be kind of an _ADR
equivalent in this particular binding AFAICS.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki [off-list ref] wrote:
On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan [off-list ref] wrote:
quoted
On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki [off-list ref] wrote:
quoted
On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko
[off-list ref] wrote:
quoted
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki [off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko
[off-list ref] wrote:
quoted
On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
quoted
On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
[off-list ref] wrote:
...
quoted
quoted
quoted
quoted
+ ret = fwnode_property_read_u32(fwnode, "reg", id);
+ if (!(ret && is_acpi_node(fwnode)))
+ return ret;
+
+#ifdef CONFIG_ACPI
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
+ METHOD_NAME__ADR, NULL, &adr);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+ *id = (u32)adr;
+#endif
+ return 0;
quoted
quoted
quoted
Also ACPI and DT
aren't mutually exclusive if I'm not mistaken.
That's why we try 'reg' property for both cases first.
is_acpi_fwnode() conditional is that what I don't like though.
I'm not sure what you mean here, care to elaborate?
I meant is_acpi_node(fwnode) in the conditional.
I think it's redundant and we can simple do something like this:
if (ret) {
#ifdef ACPI
...
#else
return ret;
#endif
}
return 0;
--
Right, that should work. And I'd prefer it too.
Rafael,
I'd rather this new function be an ops instead of a bunch of #ifdef or
if (acpi) checks. Thoughts?
Well, it looks more like a helper function than like an op and I'm not
even sure how many potential users of it will expect that _ADR should
be evaluated in the absence of the "reg" property.
It's just that the "reg" property happens to be kind of an _ADR
equivalent in this particular binding AFAICS.
I agree it is not clear how useful this helper function is going to be.
But in general, to me, any time the wrapper/helper functions in
drivers/base/property.c need to do something like this:
if (ACPI)
ACPI specific code
if (OF)
OF specific code
I think the code should be pushed to the fwnode ops. That's one of the
main point of fwnode. So that firmware specific stuff is done by
firmware specific code. Also, when adding support for new firmware,
it's pretty clear what support the firmware needs to implement.
Instead of having to go fix up a bunch of code all over the place.
So fwnode_ops->get_id() would be the OP ACPI and OF would implement.
And then we can have a wrapper in drivers/base/property.c.
-Saravana
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Andy Shevchenko <hidden> Date: 2021-01-22 21:07:54
On Fri, Jan 22, 2021 at 10:59 PM Saravana Kannan [off-list ref] wrote:
On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki [off-list ref] wrote:
quoted
On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan [off-list ref] wrote:
quoted
On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki [off-list ref] wrote:
quoted
quoted
I'd rather this new function be an ops instead of a bunch of #ifdef or
if (acpi) checks. Thoughts?
Well, it looks more like a helper function than like an op and I'm not
even sure how many potential users of it will expect that _ADR should
be evaluated in the absence of the "reg" property.
It's just that the "reg" property happens to be kind of an _ADR
equivalent in this particular binding AFAICS.
I agree it is not clear how useful this helper function is going to be.
But in general, to me, any time the wrapper/helper functions in
drivers/base/property.c need to do something like this:
if (ACPI)
ACPI specific code
if (OF)
OF specific code
I think the code should be pushed to the fwnode ops. That's one of the
main point of fwnode. So that firmware specific stuff is done by
firmware specific code. Also, when adding support for new firmware,
it's pretty clear what support the firmware needs to implement.
Instead of having to go fix up a bunch of code all over the place.
Wishful thinking.
In the very case of GPIO it's related to framework using headers local
to framework. Are you suggesting to open its guts to the entire wild
world?
I don't think it's a good idea. You see, here we have different
layering POD types, which are natural and quite low level that ops
suits best for them and quite different resource types like GPIO. And
the latter is closer to certain framework rather than to POD handling
cases.
So fwnode_ops->get_id() would be the OP ACPI and OF would implement.
And then we can have a wrapper in drivers/base/property.c.
On Fri, Jan 22, 2021 at 1:05 PM Andy Shevchenko
[off-list ref] wrote:
On Fri, Jan 22, 2021 at 10:59 PM Saravana Kannan [off-list ref] wrote:
quoted
On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki [off-list ref] wrote:
quoted
On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan [off-list ref] wrote:
quoted
On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki [off-list ref] wrote:
quoted
quoted
quoted
I'd rather this new function be an ops instead of a bunch of #ifdef or
if (acpi) checks. Thoughts?
Well, it looks more like a helper function than like an op and I'm not
even sure how many potential users of it will expect that _ADR should
be evaluated in the absence of the "reg" property.
It's just that the "reg" property happens to be kind of an _ADR
equivalent in this particular binding AFAICS.
I agree it is not clear how useful this helper function is going to be.
But in general, to me, any time the wrapper/helper functions in
drivers/base/property.c need to do something like this:
if (ACPI)
ACPI specific code
if (OF)
OF specific code
I think the code should be pushed to the fwnode ops. That's one of the
main point of fwnode. So that firmware specific stuff is done by
firmware specific code. Also, when adding support for new firmware,
it's pretty clear what support the firmware needs to implement.
Instead of having to go fix up a bunch of code all over the place.
Wishful thinking.
In the very case of GPIO it's related to framework using headers local
to framework. Are you suggesting to open its guts to the entire wild
world?
What are you even talking about? How is whatever you are saying
remotely related to getting an "id" for a fwnode?
I don't think it's a good idea. You see, here we have different
layering POD types,
POD?
which are natural and quite low level that ops
suits best for them and quite different resource types like GPIO. And
the latter is closer to certain framework rather than to POD handling
cases.