| Message ID | 20200131153440.20870-2-calvin.johnson@nxp.com |
|---|---|
| State | Deferred |
| Delegated to: | David Miller |
| Headers | show |
| Series | ACPI support for xgmac_mdio and dpaa2-mac drivers. | expand |
On Fri, Jan 31, 2020 at 09:04:34PM +0530, Calvin Johnson wrote: > From: Marcin Wojtas <mw@semihalf.com> > > 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 <mw@semihalf.com> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> 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
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 <lkp@intel.com> 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/kbuild-all@lists.01.org Intel Corporation
Hi Andrew, > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > 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 <mw@semihalf.com> > > > > 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 <mw@semihalf.com> > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > 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
Hi, On 1/31/20 9:34 AM, Calvin Johnson wrote: > From: Marcin Wojtas <mw@semihalf.com> > > 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 <mw@semihalf.com> > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > --- > > 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 > * >
Hi Jeremy, > -----Original Message----- > From: Jeremy Linton <jeremy.linton@arm.com> > 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&data=02%7C01%7Ccalvin.johnson%40nxp.com%7Cf16350b8314 > b4992063008d7ab4f6486%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1 > %7C637166229795374486&sdata=zcXu%2Fu%2Fxw5%2Ff7eJd%2FledR% > 2FgnabvFcCUtOfwTXtMoDBI%3D&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
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
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
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
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 *