2020-01-31 15:36:39

by Calvin Johnson

[permalink] [raw]
Subject: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers

From: Marcin Wojtas <[email protected]>

This patch introduces fwnode helper for registering MDIO
bus, as well as one for finding the PHY, basing on its
firmware node pointer. Comparing to existing OF equivalent,
fwnode_mdiobus_register() does not support:
* deprecated bindings (device whitelist, nor the PHY ID embedded
in the compatible string)
* MDIO bus auto scanning

Signed-off-by: Marcin Wojtas <[email protected]>
Signed-off-by: Calvin Johnson <[email protected]>
---

drivers/net/phy/mdio_bus.c | 218 +++++++++++++++++++++++++++++++++++++
include/linux/mdio.h | 3 +
2 files changed, 221 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 229e480179ff..b1830ae2abd9 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,6 +22,7 @@
#include <linux/of_device.h>
#include <linux/of_mdio.h>
#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/reset.h>
@@ -725,6 +726,223 @@ static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
return 0;
}

+static int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+ struct fwnode_handle *child, u32 addr)
+{
+ struct phy_device *phy;
+ bool is_c45 = false;
+ int rc;
+
+ rc = fwnode_property_match_string(child, "compatible",
+ "ethernet-phy-ieee802.3-c45");
+ if (!rc)
+ is_c45 = true;
+
+ phy = get_phy_device(bus, addr, is_c45);
+ if (IS_ERR(phy))
+ return PTR_ERR(phy);
+
+ phy->irq = bus->irq[addr];
+
+ if (to_of_node(child)) {
+ rc = of_irq_get(to_of_node(child), 0);
+ if (rc == -EPROBE_DEFER) {
+ phy_device_free(phy);
+ return rc;
+ } else if (rc > 0) {
+ phy->irq = rc;
+ bus->irq[addr] = rc;
+ }
+ }
+
+ if (fwnode_property_read_bool(child, "broken-turn-around"))
+ bus->phy_ignore_ta_mask |= 1 << addr;
+
+ /* Associate the fwnode with the device structure so it
+ * can be looked up later.
+ */
+ 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(child);
+ return rc;
+ }
+
+ dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
+
+ return 0;
+}
+
+static int fwnode_mdiobus_register_device(struct mii_bus *bus,
+ struct fwnode_handle *child, u32 addr)
+{
+ struct mdio_device *mdiodev;
+ int rc;
+
+ mdiodev = mdio_device_create(bus, addr);
+ if (IS_ERR(mdiodev))
+ return PTR_ERR(mdiodev);
+
+ /* Associate the fwnode with the device structure so it
+ * can be looked up later.
+ */
+ mdiodev->dev.fwnode = child;
+
+ /* All data is now stored in the mdiodev struct; register it. */
+ rc = mdio_device_register(mdiodev);
+ if (rc) {
+ mdio_device_free(mdiodev);
+ fwnode_handle_put(child);
+ return rc;
+ }
+
+ dev_dbg(&bus->dev, "registered mdio device at address %i\n", addr);
+
+ return 0;
+}
+
+static int fwnode_mdio_parse_addr(struct device *dev,
+ const struct fwnode_handle *fwnode)
+{
+ u32 addr;
+ int ret;
+
+ ret = fwnode_property_read_u32(fwnode, "reg", &addr);
+ if (ret < 0) {
+ dev_err(dev, "PHY node has no 'reg' property\n");
+ return ret;
+ }
+
+ /* A PHY must have a reg property in the range [0-31] */
+ if (addr < 0 || addr >= PHY_MAX_ADDR) {
+ dev_err(dev, "PHY address %i is invalid\n", addr);
+ return -EINVAL;
+ }
+
+ return addr;
+}
+
+/**
+ * fwnode_mdiobus_child_is_phy - Return true if the child is a PHY node.
+ * It must either:
+ * o Compatible string of "ethernet-phy-ieee802.3-c45"
+ * o Compatible string of "ethernet-phy-ieee802.3-c22"
+ * Checking "compatible" property is done, in order to follow the DT binding.
+ */
+static bool fwnode_mdiobus_child_is_phy(struct fwnode_handle *child)
+{
+ int ret;
+
+ ret = fwnode_property_match_string(child, "compatible",
+ "ethernet-phy-ieee802.3-c45");
+ if (!ret)
+ return true;
+
+ ret = fwnode_property_match_string(child, "compatible",
+ "ethernet-phy-ieee802.3-c22");
+ if (!ret)
+ return true;
+
+ if (!fwnode_property_present(child, "compatible"))
+ return true;
+
+ return false;
+}
+
+/**
+ * fwnode_mdiobus_register - Register mii_bus and create PHYs from the fwnode
+ * @bus: pointer to mii_bus structure
+ * @fwnode: pointer to fwnode_handle of MDIO bus.
+ *
+ * This function registers the mii_bus structure and registers a phy_device
+ * for each child node of @fwnode.
+ */
+int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode)
+{
+ struct fwnode_handle *child;
+ int addr, rc;
+ int default_gpio_reset_delay_ms = 10;
+
+ /* Do not continue if the node is disabled */
+ if (!fwnode_device_is_available(fwnode))
+ return -ENODEV;
+
+ /* Mask out all PHYs from auto probing. Instead the PHYs listed in
+ * the firmware nodes are populated after the bus has been registered.
+ */
+ bus->phy_mask = ~0;
+
+ bus->dev.fwnode = fwnode;
+
+ /* Get bus level PHY reset GPIO details */
+ bus->reset_delay_us = default_gpio_reset_delay_ms;
+ fwnode_property_read_u32(fwnode, "reset-delay-us",
+ &bus->reset_delay_us);
+
+ /* Register the MDIO bus */
+ rc = mdiobus_register(bus);
+ if (rc)
+ return rc;
+
+ /* Loop over the child nodes and register a phy_device for each PHY */
+ fwnode_for_each_child_node(fwnode, child) {
+ addr = fwnode_mdio_parse_addr(&bus->dev, child);
+ if (addr < 0)
+ continue;
+
+ if (fwnode_mdiobus_child_is_phy(child))
+ rc = fwnode_mdiobus_register_phy(bus, child, addr);
+ else
+ rc = fwnode_mdiobus_register_device(bus, child, addr);
+ if (rc)
+ goto unregister;
+ }
+
+ return 0;
+
+unregister:
+ mdiobus_unregister(bus);
+
+ return rc;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register);
+
+/* Helper function for fwnode_phy_find_device */
+static int fwnode_phy_match(struct device *dev, const void *phy_fwnode)
+{
+ return dev->fwnode == phy_fwnode;
+}
+
+/**
+ * fwnode_phy_find_device - find the phy_device associated to fwnode
+ * @phy_fwnode: Pointer to the PHY's fwnode
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ */
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+ struct device *d;
+ struct mdio_device *mdiodev;
+
+ if (!phy_fwnode)
+ return NULL;
+
+ d = bus_find_device(&mdio_bus_type, NULL, phy_fwnode, fwnode_phy_match);
+ if (d) {
+ mdiodev = to_mdio_device(d);
+ if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
+ return to_phy_device(d);
+ put_device(d);
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL(fwnode_phy_find_device);
+
struct bus_type mdio_bus_type = {
.name = "mdio_bus",
.match = mdio_bus_match,
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index a7604248777b..5c600bb1183c 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -327,6 +327,9 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev);
bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr);

+int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode);
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
+
/**
* mdio_module_driver() - Helper macro for registering mdio drivers
*
--
2.17.1


2020-01-31 16:29:32

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers

On Fri, Jan 31, 2020 at 09:04:34PM +0530, Calvin Johnson wrote:
> From: Marcin Wojtas <[email protected]>
>
> This patch introduces fwnode helper for registering MDIO
> bus, as well as one for finding the PHY, basing on its
> firmware node pointer. Comparing to existing OF equivalent,
> fwnode_mdiobus_register() does not support:
> * deprecated bindings (device whitelist, nor the PHY ID embedded
> in the compatible string)
> * MDIO bus auto scanning
>
> Signed-off-by: Marcin Wojtas <[email protected]>
> Signed-off-by: Calvin Johnson <[email protected]>

Hi Calvin

This appears to but a cut and paste, follow by an intelligent
s/of/fwnode/g.

Did you make any attempt to consolidate the two implementations? It
seems like there should be some level of abstraction that hides away
the difference between DT properties, and DT properties stuffed into
ACPI tables?

Andrew

2020-02-03 11:19:31

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers

Hi Calvin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.5]
[cannot apply to driver-core/driver-core-testing net-next/master net/master linus/master sparc-next/master next-20200203]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Calvin-Johnson/ACPI-support-for-xgmac_mdio-and-dpaa2-mac-drivers/20200203-070754
base: d5226fa6dbae0569ee43ecfc08bdcd6770fc4755
reproduce: make htmldocs

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

Warning: The Sphinx 'sphinx_rtd_theme' HTML theme was not found. Make sure you have the theme installed to produce pretty HTML output. Falling back to the default theme.
WARNING: dot(1) not found, for better output quality install graphviz from http://www.graphviz.org
WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
include/linux/spi/spi.h:207: warning: Function parameter or member 'driver_override' not described in 'spi_device'
include/linux/spi/spi.h:650: warning: Function parameter or member 'irq_flags' not described in 'spi_controller'
drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c:1: warning: no structured comments found
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1: warning: no structured comments found
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:254: warning: Function parameter or member 'hdcp_workqueue' not described in 'amdgpu_display_manager'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'quotactl' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'quota_on' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'getprocattr' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'setprocattr' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'locked_down' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'perf_event_open' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'perf_event_alloc' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'perf_event_free' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'perf_event_read' not described in 'security_list_options'
include/linux/lsm_hooks.h:1830: warning: Function parameter or member 'perf_event_write' not described in 'security_list_options'
drivers/usb/typec/bus.c:1: warning: 'typec_altmode_unregister_driver' not found
drivers/usb/typec/bus.c:1: warning: 'typec_altmode_register_driver' not found
drivers/usb/typec/class.c:1: warning: 'typec_altmode_register_notifier' not found
drivers/usb/typec/class.c:1: warning: 'typec_altmode_unregister_notifier' not found
sound/soc/soc-core.c:2522: warning: Function parameter or member 'legacy_dai_naming' not described in 'snd_soc_register_dai'
include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints'
include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops'
include/linux/skbuff.h:888: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'list' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'vlan_present' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
include/linux/skbuff.h:888: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
include/net/sock.h:232: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
include/net/sock.h:232: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
include/net/sock.h:232: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
include/net/sock.h:232: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
include/net/sock.h:232: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
include/net/sock.h:232: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
include/net/sock.h:232: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
include/net/sock.h:232: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
include/net/sock.h:232: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
include/net/sock.h:232: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
include/net/sock.h:232: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
include/net/sock.h:514: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock'
include/net/sock.h:514: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
include/net/sock.h:514: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
include/net/sock.h:514: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock'
include/net/sock.h:514: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock'
include/net/sock.h:514: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock'
include/net/sock.h:514: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock'
include/net/sock.h:514: warning: Function parameter or member 'sk_bpf_storage' not described in 'sock'
include/net/sock.h:2459: warning: Function parameter or member 'tcp_rx_skb_cache_key' not described in 'DECLARE_STATIC_KEY_FALSE'
include/net/sock.h:2459: warning: Excess function parameter 'sk' description in 'DECLARE_STATIC_KEY_FALSE'
include/net/sock.h:2459: warning: Excess function parameter 'skb' description in 'DECLARE_STATIC_KEY_FALSE'
net/core/skbuff.c:5489: warning: Function parameter or member 'ethernet' not described in 'skb_mpls_push'
include/linux/netdevice.h:2082: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device'
include/linux/netdevice.h:2082: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device'
>> drivers/net/phy/mdio_bus.c:837: warning: Function parameter or member 'child' not described in 'fwnode_mdiobus_child_is_phy'
include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
drivers/infiniband/core/umem_odp.c:167: warning: Function parameter or member 'ops' not described in 'ib_umem_odp_alloc_child'
drivers/infiniband/core/umem_odp.c:217: warning: Function parameter or member 'ops' not described in 'ib_umem_odp_get'
drivers/infiniband/ulp/iser/iscsi_iser.h:401: warning: Function parameter or member 'all_list' not described in 'iser_fr_desc'
drivers/infiniband/ulp/iser/iscsi_iser.h:415: warning: Function parameter or member 'all_list' not described in 'iser_fr_pool'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:148: warning: Function parameter or member 'rsvd0' not described in 'opa_vesw_info'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:148: warning: Function parameter or member 'rsvd1' not described in 'opa_vesw_info'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:148: warning: Function parameter or member 'rsvd2' not described in 'opa_vesw_info'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:148: warning: Function parameter or member 'rsvd3' not described in 'opa_vesw_info'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:148: warning: Function parameter or member 'rsvd4' not described in 'opa_vesw_info'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:205: warning: Function parameter or member 'rsvd0' not described in 'opa_per_veswport_info'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:205: warning: Function parameter or member 'rsvd1' not described in 'opa_per_veswport_info'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:205: warning: Function parameter or member 'rsvd2' not described in 'opa_per_veswport_info'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:205: warning: Function parameter or member 'rsvd3' not described in 'opa_per_veswport_info'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:263: warning: Function parameter or member 'tbl_entries' not described in 'opa_veswport_mactable'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:342: warning: Function parameter or member 'reserved' not described in 'opa_veswport_summary_counters'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:394: warning: Function parameter or member 'rsvd0' not described in 'opa_veswport_error_counters'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:394: warning: Function parameter or member 'rsvd1' not described in 'opa_veswport_error_counters'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:394: warning: Function parameter or member 'rsvd2' not described in 'opa_veswport_error_counters'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:394: warning: Function parameter or member 'rsvd3' not described in 'opa_veswport_error_counters'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:394: warning: Function parameter or member 'rsvd4' not described in 'opa_veswport_error_counters'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:394: warning: Function parameter or member 'rsvd5' not described in 'opa_veswport_error_counters'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:394: warning: Function parameter or member 'rsvd6' not described in 'opa_veswport_error_counters'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:394: warning: Function parameter or member 'rsvd7' not described in 'opa_veswport_error_counters'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:394: warning: Function parameter or member 'rsvd8' not described in 'opa_veswport_error_counters'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:394: warning: Function parameter or member 'rsvd9' not described in 'opa_veswport_error_counters'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:460: warning: Function parameter or member 'reserved' not described in 'opa_vnic_vema_mad'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:485: warning: Function parameter or member 'reserved' not described in 'opa_vnic_notice_attr'
drivers/infiniband/ulp/opa_vnic/opa_vnic_encap.h:500: warning: Function parameter or member 'reserved' not described in 'opa_vnic_vema_mad_trap'
include/linux/input/sparse-keymap.h:43: warning: Function parameter or member 'sw' not described in 'key_entry'
include/drm/drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs'
include/drm/drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'
include/net/cfg80211.h:1189: warning: Function parameter or member 'txpwr' not described in 'station_parameters'
include/net/mac80211.h:4081: warning: Function parameter or member 'sta_set_txpwr' not described in 'ieee80211_ops'
include/net/mac80211.h:2036: warning: Function parameter or member 'txpwr' not described in 'ieee80211_sta'
include/linux/devfreq.h:187: warning: Function parameter or member 'last_status' not described in 'devfreq'
drivers/devfreq/devfreq.c:1818: warning: bad line: - Resource-managed devfreq_register_notifier()
drivers/devfreq/devfreq.c:1854: warning: bad line: - Resource-managed devfreq_unregister_notifier()
drivers/devfreq/devfreq-event.c:355: warning: Function parameter or member 'edev' not described in 'devfreq_event_remove_edev'
drivers/devfreq/devfreq-event.c:355: warning: Excess function parameter 'dev' description in 'devfreq_event_remove_edev'
Documentation/admin-guide/hw-vuln/tsx_async_abort.rst:142: WARNING: duplicate label virt_mechanism, other instance in Documentation/admin-guide/hw-vuln/mds.rst
Documentation/admin-guide/ras.rst:358: WARNING: Definition list ends without a blank line; unexpected unindent.
Documentation/admin-guide/ras.rst:358: WARNING: Definition list ends without a blank line; unexpected unindent.
Documentation/admin-guide/ras.rst:363: WARNING: Definition list ends without a blank line; unexpected unindent.
Documentation/admin-guide/ras.rst:363: WARNING: Definition list ends without a blank line; unexpected unindent.
Documentation/driver-api/driver-model/driver.rst:215: WARNING: Inline emphasis start-string without end-string.
Documentation/driver-api/driver-model/driver.rst:215: WARNING: Inline emphasis start-string without end-string.
include/uapi/linux/firewire-cdev.h:312: WARNING: Inline literal start-string without end-string.
drivers/firewire/core-transaction.c:606: WARNING: Inline strong start-string without end-string.
Documentation/usb/index.rst:5: WARNING: toctree contains reference to nonexisting document 'usb/rio'
Documentation/usb/index.rst:5: WARNING: toctree contains reference to nonexisting document 'usb/wusb-design-overview'
Documentation/x86/boot.rst:72: WARNING: Malformed table.
Text in column margin in table line 57.

vim +837 drivers/net/phy/mdio_bus.c

827
828 /**
829 * fwnode_mdiobus_child_is_phy - Return true if the child is a PHY node.
830 * It must either:
831 * o Compatible string of "ethernet-phy-ieee802.3-c45"
832 * o Compatible string of "ethernet-phy-ieee802.3-c22"
833 * Checking "compatible" property is done, in order to follow the DT binding.
834 */
835 static bool fwnode_mdiobus_child_is_phy(struct fwnode_handle *child)
836 {
> 837 int ret;
838
839 ret = fwnode_property_match_string(child, "compatible",
840 "ethernet-phy-ieee802.3-c45");
841 if (!ret)
842 return true;
843
844 ret = fwnode_property_match_string(child, "compatible",
845 "ethernet-phy-ieee802.3-c22");
846 if (!ret)
847 return true;
848
849 if (!fwnode_property_present(child, "compatible"))
850 return true;
851
852 return false;
853 }
854

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation


Attachments:
(No filename) (18.81 kB)
.config.gz (7.10 kB)
Download all attachments

2020-02-05 07:12:58

by Calvin Johnson

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Friday, January 31, 2020 9:58 PM

<snip>

> On Fri, Jan 31, 2020 at 09:04:34PM +0530, Calvin Johnson wrote:
> > From: Marcin Wojtas <[email protected]>
> >
> > This patch introduces fwnode helper for registering MDIO bus, as well
> > as one for finding the PHY, basing on its firmware node pointer.
> > Comparing to existing OF equivalent,
> > fwnode_mdiobus_register() does not support:
> > * deprecated bindings (device whitelist, nor the PHY ID embedded
> > in the compatible string)
> > * MDIO bus auto scanning
> >
> > Signed-off-by: Marcin Wojtas <[email protected]>
> > Signed-off-by: Calvin Johnson <[email protected]>
>
> Hi Calvin
> no
> This appears to but a cut and paste, follow by an intelligent s/of/fwnode/g.

In this patchset, I tried to reuse Marcin's patch which was posted on 2017/12/18.
https://lkml.org/lkml/2017/12/18/211
With my patch([v1,2/7] mdio_bus: modify fwnode phy related functions), I've made
modifications to this(v1,1/7) patch to adapt to the changes in the kernel.

> Did you make any attempt to consolidate the two implementations? It
> seems like there should be some level of abstraction that hides away the
> difference between DT properties, and DT properties stuffed into ACPI
> tables?

Yes attempt is to consolidate DT and ACPI into fwnode. Sure, I'll revisit the patch
and try to work on your recommendation.

Thanks
Calvin

2020-02-06 21:58:28

by Jeremy Linton

[permalink] [raw]
Subject: Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers

Hi,

On 1/31/20 9:34 AM, Calvin Johnson wrote:
> From: Marcin Wojtas <[email protected]>
>
> This patch introduces fwnode helper for registering MDIO
> bus, as well as one for finding the PHY, basing on its
> firmware node pointer. Comparing to existing OF equivalent,
> fwnode_mdiobus_register() does not support:
> * deprecated bindings (device whitelist, nor the PHY ID embedded
> in the compatible string)
> * MDIO bus auto scanning
>
> Signed-off-by: Marcin Wojtas <[email protected]>
> Signed-off-by: Calvin Johnson <[email protected]>
> ---
>
> drivers/net/phy/mdio_bus.c | 218 +++++++++++++++++++++++++++++++++++++
> include/linux/mdio.h | 3 +
> 2 files changed, 221 insertions(+)
>
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 229e480179ff..b1830ae2abd9 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -22,6 +22,7 @@
> #include <linux/of_device.h>
> #include <linux/of_mdio.h>
> #include <linux/of_gpio.h>
> +#include <linux/of_irq.h>
> #include <linux/netdevice.h>
> #include <linux/etherdevice.h>
> #include <linux/reset.h>
> @@ -725,6 +726,223 @@ static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
> return 0;
> }
>
> +static int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> + struct fwnode_handle *child, u32 addr)
> +{
> + struct phy_device *phy;
> + bool is_c45 = false;
> + int rc;
> +
> + rc = fwnode_property_match_string(child, "compatible",
> + "ethernet-phy-ieee802.3-c45");
> + if (!rc)
> + is_c45 = true;
> +
> + phy = get_phy_device(bus, addr, is_c45);
> + if (IS_ERR(phy))
> + return PTR_ERR(phy);
> +
> + phy->irq = bus->irq[addr];
> +
> + if (to_of_node(child)) {
> + rc = of_irq_get(to_of_node(child), 0);
> + if (rc == -EPROBE_DEFER) {
> + phy_device_free(phy);
> + return rc;
> + } else if (rc > 0) {
> + phy->irq = rc;
> + bus->irq[addr] = rc;
> + }
> + }
> +
> + if (fwnode_property_read_bool(child, "broken-turn-around"))
> + bus->phy_ignore_ta_mask |= 1 << addr;
> +
> + /* Associate the fwnode with the device structure so it
> + * can be looked up later.
> + */
> + 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(child);
> + return rc;
> + }
> +
> + dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> +
> + return 0;
> +}
> +
> +static int fwnode_mdiobus_register_device(struct mii_bus *bus,
> + struct fwnode_handle *child, u32 addr)
> +{
> + struct mdio_device *mdiodev;
> + int rc;
> +
> + mdiodev = mdio_device_create(bus, addr);
> + if (IS_ERR(mdiodev))
> + return PTR_ERR(mdiodev);
> +
> + /* Associate the fwnode with the device structure so it
> + * can be looked up later.
> + */
> + mdiodev->dev.fwnode = child;
> +
> + /* All data is now stored in the mdiodev struct; register it. */
> + rc = mdio_device_register(mdiodev);
> + if (rc) {
> + mdio_device_free(mdiodev);
> + fwnode_handle_put(child);
> + return rc;
> + }
> +
> + dev_dbg(&bus->dev, "registered mdio device at address %i\n", addr);
> +
> + return 0;
> +}
> +
> +static int fwnode_mdio_parse_addr(struct device *dev,
> + const struct fwnode_handle *fwnode)
> +{
> + u32 addr;
> + int ret;
> +
> + ret = fwnode_property_read_u32(fwnode, "reg", &addr);
> + if (ret < 0) {
> + dev_err(dev, "PHY node has no 'reg' property\n");
> + return ret;
> + }
> +
> + /* A PHY must have a reg property in the range [0-31] */
> + if (addr < 0 || addr >= PHY_MAX_ADDR) {
> + dev_err(dev, "PHY address %i is invalid\n", addr);
> + return -EINVAL;
> + }
> +
> + return addr;
> +}

Almost assuredly this is wrong, the _ADR method exists to identify a
device on its parent bus. The DT reg property shouldn't be used like
this in an ACPI enviroment.

Further, there are a number of other dt bindings in this set that seem
inappropriate in common/shared ACPI code paths. That is because AFAIK
the _DSD methods are there to provide device implementation specific
behaviors, not as standardized methods for a generic classes of devices.
Its vaguly the equivlant of the "vendor,xxxx" properties in DT.

This has been a discussion point on/off for a while with any attempt to
publicly specify/standardize for all OS vendors what they might find
encoded in a DSD property. The few year old "WORK_IN_PROGRESS" link on
the uefi page has a few suggested ones

https://uefi.org/sites/default/files/resources/nic-request-v2.pdf

Anyway, the use of phy-handle with a reference to the phy device on a
shared MDIO is a techically workable solution to the problem brought up
in the RPI/ACPI thread as well. OTOH, it suffers from the use of DSD and
at a minimum should probably be added to the document linked above if
its found to be the best way to handle this. Although, in the common
case of a mdio bus, matching acpi described devices with their
discovered counterparts (note the device() defintion in the spec
19.6.30) only to define DSD refrences is a bit overkill.

Put another way, while seemingly not nessiary if a bus can be probed, a
nic/device->mdio->phy can be described in the normal ACPI heirarchy with
only appropriatly nested CRS and ADR resources. Its the shared nature of
the MDIO bus that causes problems.



> +
> +/**
> + * fwnode_mdiobus_child_is_phy - Return true if the child is a PHY node.
> + * It must either:
> + * o Compatible string of "ethernet-phy-ieee802.3-c45"
> + * o Compatible string of "ethernet-phy-ieee802.3-c22"
> + * Checking "compatible" property is done, in order to follow the DT binding.
> + */
> +static bool fwnode_mdiobus_child_is_phy(struct fwnode_handle *child)
> +{
> + int ret;
> +
> + ret = fwnode_property_match_string(child, "compatible",
> + "ethernet-phy-ieee802.3-c45");
> + if (!ret)
> + return true;
> +
> + ret = fwnode_property_match_string(child, "compatible",
> + "ethernet-phy-ieee802.3-c22");
> + if (!ret)
> + return true;
> +
> + if (!fwnode_property_present(child, "compatible"))
> + return true;
> +
> + return false;
> +}
> +
> +/**
> + * fwnode_mdiobus_register - Register mii_bus and create PHYs from the fwnode
> + * @bus: pointer to mii_bus structure
> + * @fwnode: pointer to fwnode_handle of MDIO bus.
> + *
> + * This function registers the mii_bus structure and registers a phy_device
> + * for each child node of @fwnode.
> + */
> +int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode)
> +{
> + struct fwnode_handle *child;
> + int addr, rc;
> + int default_gpio_reset_delay_ms = 10;
> +
> + /* Do not continue if the node is disabled */
> + if (!fwnode_device_is_available(fwnode))
> + return -ENODEV;
> +
> + /* Mask out all PHYs from auto probing. Instead the PHYs listed in
> + * the firmware nodes are populated after the bus has been registered.
> + */
> + bus->phy_mask = ~0;
> +
> + bus->dev.fwnode = fwnode;
> +
> + /* Get bus level PHY reset GPIO details */
> + bus->reset_delay_us = default_gpio_reset_delay_ms;
> + fwnode_property_read_u32(fwnode, "reset-delay-us",
> + &bus->reset_delay_us);
> +
> + /* Register the MDIO bus */
> + rc = mdiobus_register(bus);
> + if (rc)
> + return rc;
> +
> + /* Loop over the child nodes and register a phy_device for each PHY */
> + fwnode_for_each_child_node(fwnode, child) {
> + addr = fwnode_mdio_parse_addr(&bus->dev, child);
> + if (addr < 0)
> + continue;
> +
> + if (fwnode_mdiobus_child_is_phy(child))
> + rc = fwnode_mdiobus_register_phy(bus, child, addr);
> + else
> + rc = fwnode_mdiobus_register_device(bus, child, addr);
> + if (rc)
> + goto unregister;
> + }
> +
> + return 0;
> +
> +unregister:
> + mdiobus_unregister(bus);
> +
> + return rc;
> +}
> +EXPORT_SYMBOL(fwnode_mdiobus_register);
> +
> +/* Helper function for fwnode_phy_find_device */
> +static int fwnode_phy_match(struct device *dev, const void *phy_fwnode)
> +{
> + return dev->fwnode == phy_fwnode;
> +}
> +
> +/**
> + * fwnode_phy_find_device - find the phy_device associated to fwnode
> + * @phy_fwnode: Pointer to the PHY's fwnode
> + *
> + * If successful, returns a pointer to the phy_device with the embedded
> + * struct device refcount incremented by one, or NULL on failure.
> + */
> +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> +{
> + struct device *d;
> + struct mdio_device *mdiodev;
> +
> + if (!phy_fwnode)
> + return NULL;
> +
> + d = bus_find_device(&mdio_bus_type, NULL, phy_fwnode, fwnode_phy_match);
> + if (d) {
> + mdiodev = to_mdio_device(d);
> + if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
> + return to_phy_device(d);
> + put_device(d);
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL(fwnode_phy_find_device);
> +
> struct bus_type mdio_bus_type = {
> .name = "mdio_bus",
> .match = mdio_bus_match,
> diff --git a/include/linux/mdio.h b/include/linux/mdio.h
> index a7604248777b..5c600bb1183c 100644
> --- a/include/linux/mdio.h
> +++ b/include/linux/mdio.h
> @@ -327,6 +327,9 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev);
> bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
> struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr);
>
> +int fwnode_mdiobus_register(struct mii_bus *bus, struct fwnode_handle *fwnode);
> +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
> +
> /**
> * mdio_module_driver() - Helper macro for registering mdio drivers
> *
>

2020-02-07 09:44:10

by Calvin Johnson

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers

Hi Jeremy,

> -----Original Message-----
> From: Jeremy Linton <[email protected]>
> Sent: Wednesday, February 5, 2020 7:48 PM

<snip>

> > +static int fwnode_mdio_parse_addr(struct device *dev,
> > + const struct fwnode_handle *fwnode) {
> > + u32 addr;
> > + int ret;
> > +
> > + ret = fwnode_property_read_u32(fwnode, "reg", &addr);
> > + if (ret < 0) {
> > + dev_err(dev, "PHY node has no 'reg' property\n");
> > + return ret;
> > + }
> > +
> > + /* A PHY must have a reg property in the range [0-31] */
> > + if (addr < 0 || addr >= PHY_MAX_ADDR) {
> > + dev_err(dev, "PHY address %i is invalid\n", addr);
> > + return -EINVAL;
> > + }
> > +
> > + return addr;
> > +}
>
> Almost assuredly this is wrong, the _ADR method exists to identify a device
> on its parent bus. The DT reg property shouldn't be used like this in an ACPI
> enviroment.
>
> Further, there are a number of other dt bindings in this set that seem
> inappropriate in common/shared ACPI code paths. That is because AFAIK the
> _DSD methods are there to provide device implementation specific
> behaviors, not as standardized methods for a generic classes of devices.
> Its vaguly the equivlant of the "vendor,xxxx" properties in DT.
>
> This has been a discussion point on/off for a while with any attempt to
> publicly specify/standardize for all OS vendors what they might find encoded
> in a DSD property. The few year old "WORK_IN_PROGRESS" link on the uefi
> page has a few suggested ones
>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fuefi.
> org%2Fsites%2Fdefault%2Ffiles%2Fresources%2Fnic-request-
> v2.pdf&amp;data=02%7C01%7Ccalvin.johnson%40nxp.com%7Cf16350b8314
> b4992063008d7ab4f6486%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1
> %7C637166229795374486&amp;sdata=zcXu%2Fu%2Fxw5%2Ff7eJd%2FledR%
> 2FgnabvFcCUtOfwTXtMoDBI%3D&amp;reserved=0
>
> Anyway, the use of phy-handle with a reference to the phy device on a
> shared MDIO is a techically workable solution to the problem brought up in
> the RPI/ACPI thread as well. OTOH, it suffers from the use of DSD and at a
> minimum should probably be added to the document linked above if its
> found to be the best way to handle this. Although, in the common case of a
> mdio bus, matching acpi described devices with their discovered
> counterparts (note the device() defintion in the spec
> 19.6.30) only to define DSD refrences is a bit overkill.
>
> Put another way, while seemingly not nessiary if a bus can be probed, a
> nic/device->mdio->phy can be described in the normal ACPI heirarchy with
> only appropriatly nested CRS and ADR resources. Its the shared nature of the
> MDIO bus that causes problems.

Thanks! I'll definitely consider your suggestions along with the others received earlier.

While I do more study on this, thought of forwarding DSTD tables used by this patch-set.
https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platforms/tree/Platform/NXP/LX2160aRdbPkg/AcpiTables/Dsdt/Mdio.asl?h=LX2160_UEFI_ACPI_EAR1
https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platforms/tree/Platform/NXP/LX2160aRdbPkg/AcpiTables/Dsdt/Mc.asl?h=LX2160_UEFI_ACPI_EAR1

Regards
Calvin

2020-03-17 11:37:39

by Calvin Johnson

[permalink] [raw]
Subject: Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers

Hi,

On Fri, Jan 31, 2020 at 09:04:34PM +0530, Calvin Johnson wrote:

<snip>

> +/**
> + * fwnode_mdiobus_child_is_phy - Return true if the child is a PHY node.
> + * It must either:
> + * o Compatible string of "ethernet-phy-ieee802.3-c45"
> + * o Compatible string of "ethernet-phy-ieee802.3-c22"
> + * Checking "compatible" property is done, in order to follow the DT binding.
> + */
> +static bool fwnode_mdiobus_child_is_phy(struct fwnode_handle *child)
> +{
> + int ret;
> +
> + ret = fwnode_property_match_string(child, "compatible",
> + "ethernet-phy-ieee802.3-c45");
> + if (!ret)
> + return true;
> +
> + ret = fwnode_property_match_string(child, "compatible",
> + "ethernet-phy-ieee802.3-c22");
> + if (!ret)
> + return true;
> +
> + if (!fwnode_property_present(child, "compatible"))
> + return true;
> +
> + return false;
> +}

Can we use _CID in ACPI to get the compatible string? Is there any other method
to handle this kind of situation where we would like to pass C45 or C22 info to
the mdiobus driver?

Thanks
Calvin

2020-03-17 14:05:40

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers

On Tue, Mar 17, 2020 at 05:06:50PM +0530, Calvin Johnson wrote:
> Hi,
>
> On Fri, Jan 31, 2020 at 09:04:34PM +0530, Calvin Johnson wrote:
>
> <snip>
>
> > +/**
> > + * fwnode_mdiobus_child_is_phy - Return true if the child is a PHY node.
> > + * It must either:
> > + * o Compatible string of "ethernet-phy-ieee802.3-c45"
> > + * o Compatible string of "ethernet-phy-ieee802.3-c22"
> > + * Checking "compatible" property is done, in order to follow the DT binding.
> > + */
> > +static bool fwnode_mdiobus_child_is_phy(struct fwnode_handle *child)
> > +{
> > + int ret;
> > +
> > + ret = fwnode_property_match_string(child, "compatible",
> > + "ethernet-phy-ieee802.3-c45");
> > + if (!ret)
> > + return true;
> > +
> > + ret = fwnode_property_match_string(child, "compatible",
> > + "ethernet-phy-ieee802.3-c22");
> > + if (!ret)
> > + return true;
> > +
> > + if (!fwnode_property_present(child, "compatible"))
> > + return true;
> > +
> > + return false;
> > +}
>
> Can we use _CID in ACPI to get the compatible string? Is there any other method
> to handle this kind of situation where we would like to pass C45 or C22 info to
> the mdiobus driver?

Hi Calvin

Is there any defacto standardised way to stuff this device tree
property into ACPI? It is one of the key properties, so either there
is one standard way, or lots of variants because nobody can be
bothered to go to the ACPI standardisation body and get it formalised.

Andrew

2020-03-18 06:04:33

by Calvin Johnson

[permalink] [raw]
Subject: Re: [PATCH v1 1/7] mdio_bus: Introduce fwnode MDIO helpers

Hi Andrew,

On Tue, Mar 17, 2020 at 03:04:26PM +0100, Andrew Lunn wrote:
> On Tue, Mar 17, 2020 at 05:06:50PM +0530, Calvin Johnson wrote:
> > Hi,
> >
> > On Fri, Jan 31, 2020 at 09:04:34PM +0530, Calvin Johnson wrote:
> >
> > <snip>
> >
> > > +/**
> > > + * fwnode_mdiobus_child_is_phy - Return true if the child is a PHY node.
> > > + * It must either:
> > > + * o Compatible string of "ethernet-phy-ieee802.3-c45"
> > > + * o Compatible string of "ethernet-phy-ieee802.3-c22"
> > > + * Checking "compatible" property is done, in order to follow the DT binding.
> > > + */
> > > +static bool fwnode_mdiobus_child_is_phy(struct fwnode_handle *child)
> > > +{
> > > + int ret;
> > > +
> > > + ret = fwnode_property_match_string(child, "compatible",
> > > + "ethernet-phy-ieee802.3-c45");
> > > + if (!ret)
> > > + return true;
> > > +
> > > + ret = fwnode_property_match_string(child, "compatible",
> > > + "ethernet-phy-ieee802.3-c22");
> > > + if (!ret)
> > > + return true;
> > > +
> > > + if (!fwnode_property_present(child, "compatible"))
> > > + return true;
> > > +
> > > + return false;
> > > +}
> >
> > Can we use _CID in ACPI to get the compatible string? Is there any other method
> > to handle this kind of situation where we would like to pass C45 or C22 info to
> > the mdiobus driver?
>
> Hi Calvin
>
> Is there any defacto standardised way to stuff this device tree
> property into ACPI? It is one of the key properties, so either there
> is one standard way, or lots of variants because nobody can be
> bothered to go to the ACPI standardisation body and get it formalised.

_DSD package is used to stuff this kind of DT property. IMO, this is not the
standard way as C22 and C45 are key properties for MDIO.

Eg usage of _DSD:
https://source.codeaurora.org/external/qoriq/qoriq-components/edk2-platforms/tree/Platform/NXP/LX2160aRdbPkg/AcpiTables/Dsdt/Mdio.asl?h=LX2160_UEFI_ACPI_EAR1

Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () {"reg", 5},
Package () {"phy-addr", 5},
Package () {"compatible", "ethernet-phy-ieee802.3-c45"}
}
})

Ideally, MDIO bus should be part of the ACPI spec.
Maybe this property can be included in:
https://uefi.org/sites/default/files/resources/nic-request-v2.pdf

I'm still looking for a better approach than _DSD till ACPI spec defines it.

Regards
Calvin