linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
@ 2018-03-23 15:05 Vicentiu Galanopulo
  2018-03-23 15:44 ` Andrew Lunn
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Vicentiu Galanopulo @ 2018-03-23 15:05 UTC (permalink / raw)
  To: netdev, linux-kernel, robh+dt, mark.rutland, davem, marcel, devicetree
  Cc: madalin.bucur, alexandru.marginean

Reason for this patch is that the Inphi PHY has a
vendor specific address space for accessing the
C45 MDIO registers - starting from 0x1e.

A search of the dev-addr property is done in of_mdiobus_register.
If the property is found in the PHY node,
of_mdiobus_register_static_phy is called. This is a
wrapper function for of_mdiobus_register_phy which finds the
device in package based on dev-addr and fills devices_addrs:
devices_addrs is a new field added to phy_c45_device_ids.
This new field will store the dev-addr property on the same
index where the device in package has been found.
In order to have dev-addr in get_phy_c45_ids(), mdio_c45_ids is
passed from of_mdio.c to phy_device.c as an external variable.
In get_phy_device a copy of the mdio_c45_ids is done over the
local c45_ids (wich are empty). After the copying, the c45_ids
will also contain the static device found from dev-addr.
Having dev-addr stored in devices_addrs, in get_phy_c45_ids(),
when probing the identifiers, dev-addr can be extracted from
devices_addrs and probed if devices_addrs[current_identifier]
is not 0.
This way changing the kernel API is avoided completely.

As a plus to this patch, num_ids in get_phy_c45_ids,
has the value 8 (ARRAY_SIZE(c45_ids->device_ids)),
but the u32 *devs can store 32 devices in the bitfield.
If a device is stored in *devs, in bits 32 to 9, it
will not be found. This is the reason for changing
in phy.h, the size of device_ids array.

Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
---
 Documentation/devicetree/bindings/net/phy.txt |  6 ++
 drivers/net/phy/phy_device.c                  | 22 +++++-
 drivers/of/of_mdio.c                          | 98 ++++++++++++++++++++++++++-
 include/linux/phy.h                           |  5 +-
 4 files changed, 125 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index d2169a5..82692e2 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -61,6 +61,11 @@ Optional Properties:
 - reset-deassert-us: Delay after the reset was deasserted in microseconds.
   If this property is missing the delay will be skipped.
 
+- dev-addr: If set, it indicates the device address of the PHY to be used
+  when accessing the C45 PHY registers over MDIO. It is used for vendor specific
+  register space addresses that do no conform to standard address for the MDIO
+  registers (e.g. MMD30)
+
 Example:
 
 ethernet-phy@0 {
@@ -72,4 +77,5 @@ ethernet-phy@0 {
 	reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
 	reset-assert-us = <1000>;
 	reset-deassert-us = <2000>;
+	dev-addr = <0x1e>;
 };
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b285323..f5051cf6 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -71,6 +71,7 @@ static void phy_mdio_device_remove(struct mdio_device *mdiodev)
 
 static struct phy_driver genphy_driver;
 extern struct phy_driver genphy_10g_driver;
+extern struct phy_c45_device_ids mdio_c45_ids;
 
 static LIST_HEAD(phy_fixup_list);
 static DEFINE_MUTEX(phy_fixup_lock);
@@ -457,7 +458,7 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
 static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 			   struct phy_c45_device_ids *c45_ids) {
 	int phy_reg;
-	int i, reg_addr;
+	int i, reg_addr, dev_addr;
 	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
 	u32 *devs = &c45_ids->devices_in_package;
 
@@ -493,13 +494,23 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 		if (!(c45_ids->devices_in_package & (1 << i)))
 			continue;
 
-		reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID1;
+		/* if c45_ids->devices_addrs for the current id is not 0,
+		 * then dev-addr was defined in the device tree node,
+		 * and the PHY as been seen as a valid device, and added,
+		 * in the package. In this case we can use the
+		 * dev-addr(c45_ids->devices_addrs[i]) to do the MDIO
+		 * reading of the PHY ID.
+		 */
+		dev_addr = !!c45_ids->devices_addrs[i] ?
+					c45_ids->devices_addrs[i] : i;
+
+		reg_addr = MII_ADDR_C45 | dev_addr << 16 | MII_PHYSID1;
 		phy_reg = mdiobus_read(bus, addr, reg_addr);
 		if (phy_reg < 0)
 			return -EIO;
 		c45_ids->device_ids[i] = (phy_reg & 0xffff) << 16;
 
-		reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID2;
+		reg_addr = MII_ADDR_C45 | dev_addr << 16 | MII_PHYSID2;
 		phy_reg = mdiobus_read(bus, addr, reg_addr);
 		if (phy_reg < 0)
 			return -EIO;
@@ -566,6 +577,11 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 	u32 phy_id = 0;
 	int r;
 
+	/* copy the external mdio_c45_ids (which may contain the id's found
+	 * by serching the device tree dev-addr property) to local c45_ids
+	 */
+	memcpy(&c45_ids, &mdio_c45_ids, sizeof(struct phy_c45_device_ids));
+
 	r = get_phy_id(bus, addr, &phy_id, is_c45, &c45_ids);
 	if (r)
 		return ERR_PTR(r);
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 8c0c927..cbc34f6 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -24,6 +24,8 @@
 
 #define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
 
+struct phy_c45_device_ids mdio_c45_ids = {0};
+
 MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
 MODULE_LICENSE("GPL");
 
@@ -190,6 +192,71 @@ static bool of_mdiobus_child_is_phy(struct device_node *child)
 	return false;
 }
 
+static void of_fill_c45ids_devs_addrs(u32 dev_addr)
+{
+	int i;
+	const int num_ids = ARRAY_SIZE(mdio_c45_ids.device_ids);
+
+	/* Search through all Device Identifiers and set
+	 * dev_addr in mdio_c45_ids.devices_addrs,
+	 * if the device bit is set in
+	 * mdio_c45_ids.devices_in_package
+	 */
+	for (i = 1; i < num_ids; i++) {
+		if (!(mdio_c45_ids.devices_in_package & (1 << i)))
+			continue;
+
+		mdio_c45_ids.devices_addrs[i] = dev_addr;
+		break;
+	}
+}
+
+static int of_find_devaddr_in_pkg(struct mii_bus *bus, u32 addr,
+				  u32 dev_addr)
+{
+	u32 *devs = &mdio_c45_ids.devices_in_package;
+	int phy_reg, reg_addr;
+
+	reg_addr = MII_ADDR_C45 | dev_addr << 16 | MDIO_DEVS2;
+	phy_reg = mdiobus_read(bus, addr, reg_addr);
+	if (phy_reg < 0)
+		return -EIO;
+
+	*devs = (phy_reg & 0xffff) << 16;
+
+	reg_addr = MII_ADDR_C45 | dev_addr << 16 | MDIO_DEVS1;
+	phy_reg = mdiobus_read(bus, addr, reg_addr);
+	if (phy_reg < 0)
+		return -EIO;
+
+	*devs |= (phy_reg & 0xffff);
+
+	return 0;
+}
+
+/*
+ * Finds the device in package and populates the mdio_c45_ids
+ * if any device is found at dev_addr address. After this
+ * the PHY is registered
+ */
+static int of_mdiobus_register_static_phy(struct mii_bus *mdio,
+					  struct device_node *child,
+					  u32 addr, u32 dev_addr)
+{
+	int dev_err = 0;
+
+	if (!dev_addr)
+		goto exit_register_phy;
+
+	dev_err = of_find_devaddr_in_pkg(mdio, addr, dev_addr);
+
+	if (!dev_err)
+		of_fill_c45ids_devs_addrs(dev_addr);
+
+exit_register_phy:
+	return of_mdiobus_register_phy(mdio, child, addr);
+}
+
 /**
  * of_mdiobus_register - Register mii_bus and create PHYs from the device tree
  * @mdio: pointer to mii_bus structure
@@ -202,7 +269,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 {
 	struct device_node *child;
 	bool scanphys = false;
+	bool dev_addr_found = true;
 	int addr, rc;
+	int dev_addr = 0;
+	int ret;
 
 	/* Do not continue if the node is disabled */
 	if (!of_device_is_available(np))
@@ -226,6 +296,14 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 
 	/* Loop over the child nodes and register a phy_device for each phy */
 	for_each_available_child_of_node(np, child) {
+		/* Check if dev-addr is set in the PHY node */
+		ret = of_property_read_u32(child, "dev-addr", &dev_addr);
+
+		if (ret < 0) {
+			/* either not set or invalid */
+			dev_addr_found = false;
+		}
+
 		addr = of_mdio_parse_addr(&mdio->dev, child);
 		if (addr < 0) {
 			scanphys = true;
@@ -233,7 +311,11 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 		}
 
 		if (of_mdiobus_child_is_phy(child))
-			rc = of_mdiobus_register_phy(mdio, child, addr);
+			if (dev_addr_found)
+				rc = of_mdiobus_register_static_phy(mdio, child,
+								    addr, dev_addr);
+			else
+				rc = of_mdiobus_register_phy(mdio, child, addr);
 		else
 			rc = of_mdiobus_register_device(mdio, child, addr);
 
@@ -248,8 +330,16 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 	if (!scanphys)
 		return 0;
 
+	/* reset device found variable */
+	dev_addr_found = true;
+
 	/* auto scan for PHYs with empty reg property */
 	for_each_available_child_of_node(np, child) {
+		/* Check if dev-addr is set in the PHY node,
+		 * for PHYs which don't have reg property set
+		 */
+		ret = of_property_read_u32(child, "dev-addr", &dev_addr);
+
 		/* Skip PHYs with reg property set */
 		if (of_find_property(child, "reg", NULL))
 			continue;
@@ -264,7 +354,11 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 				 child->name, addr);
 
 			if (of_mdiobus_child_is_phy(child)) {
-				rc = of_mdiobus_register_phy(mdio, child, addr);
+				if (dev_addr_found)
+					rc = of_mdiobus_register_static_phy(mdio, child,
+									    addr, dev_addr);
+				else
+					rc = of_mdiobus_register_phy(mdio, child, addr);
 				if (rc && rc != -ENODEV)
 					goto unregister;
 			}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5a9b175..161ad90 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -357,10 +357,13 @@ enum phy_state {
  * struct phy_c45_device_ids - 802.3-c45 Device Identifiers
  * @devices_in_package: Bit vector of devices present.
  * @device_ids: The device identifer for each present device.
+ * @devices_addrs: The devices addresses from the device tree
+ *		   for each present device.
  */
 struct phy_c45_device_ids {
 	u32 devices_in_package;
-	u32 device_ids[8];
+	u32 device_ids[32];
+	u32 devices_addrs[32];
 };
 
 /* phy_device: An instance of a PHY
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
  2018-03-23 15:05 [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up Vicentiu Galanopulo
@ 2018-03-23 15:44 ` Andrew Lunn
  2018-03-26 22:25 ` Rob Herring
  2018-03-26 22:44 ` Florian Fainelli
  2 siblings, 0 replies; 9+ messages in thread
From: Andrew Lunn @ 2018-03-23 15:44 UTC (permalink / raw)
  To: Vicentiu Galanopulo
  Cc: netdev, linux-kernel, robh+dt, mark.rutland, davem, marcel,
	devicetree, madalin.bucur, alexandru.marginean

> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -24,6 +24,8 @@
>  
>  #define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
>  
> +struct phy_c45_device_ids mdio_c45_ids = {0};

You do know that Linux is multi-threaded. It could be probing two MDIO
busses at once.

	Andrew

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
  2018-03-23 15:05 [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up Vicentiu Galanopulo
  2018-03-23 15:44 ` Andrew Lunn
@ 2018-03-26 22:25 ` Rob Herring
  2018-03-27  8:10   ` Vicenţiu Galanopulo
  2018-03-26 22:44 ` Florian Fainelli
  2 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2018-03-26 22:25 UTC (permalink / raw)
  To: Vicentiu Galanopulo
  Cc: netdev, linux-kernel, mark.rutland, davem, marcel, devicetree,
	madalin.bucur, alexandru.marginean

On Fri, Mar 23, 2018 at 10:05:22AM -0500, Vicentiu Galanopulo wrote:
> Reason for this patch is that the Inphi PHY has a
> vendor specific address space for accessing the
> C45 MDIO registers - starting from 0x1e.
> 
> A search of the dev-addr property is done in of_mdiobus_register.
> If the property is found in the PHY node,
> of_mdiobus_register_static_phy is called. This is a
> wrapper function for of_mdiobus_register_phy which finds the
> device in package based on dev-addr and fills devices_addrs:
> devices_addrs is a new field added to phy_c45_device_ids.
> This new field will store the dev-addr property on the same
> index where the device in package has been found.
> In order to have dev-addr in get_phy_c45_ids(), mdio_c45_ids is
> passed from of_mdio.c to phy_device.c as an external variable.
> In get_phy_device a copy of the mdio_c45_ids is done over the
> local c45_ids (wich are empty). After the copying, the c45_ids
> will also contain the static device found from dev-addr.
> Having dev-addr stored in devices_addrs, in get_phy_c45_ids(),
> when probing the identifiers, dev-addr can be extracted from
> devices_addrs and probed if devices_addrs[current_identifier]
> is not 0.
> This way changing the kernel API is avoided completely.
> 
> As a plus to this patch, num_ids in get_phy_c45_ids,
> has the value 8 (ARRAY_SIZE(c45_ids->device_ids)),
> but the u32 *devs can store 32 devices in the bitfield.
> If a device is stored in *devs, in bits 32 to 9, it
> will not be found. This is the reason for changing
> in phy.h, the size of device_ids array.
> 
> Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
> ---
>  Documentation/devicetree/bindings/net/phy.txt |  6 ++

Please split bindings to separate patch.

>  drivers/net/phy/phy_device.c                  | 22 +++++-
>  drivers/of/of_mdio.c                          | 98 ++++++++++++++++++++++++++-
>  include/linux/phy.h                           |  5 +-
>  4 files changed, 125 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> index d2169a5..82692e2 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -61,6 +61,11 @@ Optional Properties:
>  - reset-deassert-us: Delay after the reset was deasserted in microseconds.
>    If this property is missing the delay will be skipped.
>  
> +- dev-addr: If set, it indicates the device address of the PHY to be used
> +  when accessing the C45 PHY registers over MDIO. It is used for vendor specific
> +  register space addresses that do no conform to standard address for the MDIO
> +  registers (e.g. MMD30)

This is a 2nd MDIO address, right? Can't you just append this to reg 
property?

Rob

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
  2018-03-23 15:05 [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up Vicentiu Galanopulo
  2018-03-23 15:44 ` Andrew Lunn
  2018-03-26 22:25 ` Rob Herring
@ 2018-03-26 22:44 ` Florian Fainelli
  2018-03-27 10:02   ` Vicenţiu Galanopulo
  2 siblings, 1 reply; 9+ messages in thread
From: Florian Fainelli @ 2018-03-26 22:44 UTC (permalink / raw)
  To: vicentiu.galanopulo, netdev, linux-kernel, robh+dt, mark.rutland,
	davem, marcel, devicetree
  Cc: madalin.bucur, alexandru.marginean

On 03/23/2018 08:05 AM, Vicentiu Galanopulo wrote:
> Reason for this patch is that the Inphi PHY has a
> vendor specific address space for accessing the
> C45 MDIO registers - starting from 0x1e.
> 
> A search of the dev-addr property is done in of_mdiobus_register.
> If the property is found in the PHY node,
> of_mdiobus_register_static_phy is called. This is a
> wrapper function for of_mdiobus_register_phy which finds the
> device in package based on dev-addr and fills devices_addrs:
> devices_addrs is a new field added to phy_c45_device_ids.
> This new field will store the dev-addr property on the same
> index where the device in package has been found.
> In order to have dev-addr in get_phy_c45_ids(), mdio_c45_ids is
> passed from of_mdio.c to phy_device.c as an external variable.
> In get_phy_device a copy of the mdio_c45_ids is done over the
> local c45_ids (wich are empty). After the copying, the c45_ids
> will also contain the static device found from dev-addr.
> Having dev-addr stored in devices_addrs, in get_phy_c45_ids(),
> when probing the identifiers, dev-addr can be extracted from
> devices_addrs and probed if devices_addrs[current_identifier]
> is not 0.
> This way changing the kernel API is avoided completely.
> 
> As a plus to this patch, num_ids in get_phy_c45_ids,
> has the value 8 (ARRAY_SIZE(c45_ids->device_ids)),
> but the u32 *devs can store 32 devices in the bitfield.
> If a device is stored in *devs, in bits 32 to 9, it
> will not be found. This is the reason for changing
> in phy.h, the size of device_ids array.
> 
> Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
> ---

Correct me if I am completely misunderstanding  the problem, but have
you considered doing the following:

- if all you need to is to replace instances of loops that do:

        if (phydev->is_c45) {
                for (i = 1; i < num_ids; i++) {
                        if (!(phydev->c45_ids.devices_in_package & (1 <<
i)))
                                continue;

with one that starts at dev-addr, as specified by Device Tree, then I
suspect there is an easier way to do what you want rather than your
fairly intrusive patch to do that:

- patch of_mdiobus_register_phy() to lookup both the c45 compatible
string as well as fetch the "dev-addr" property

- provide a PHY Device Tree node that has its OUI as a compatible string
(see of_get_phy_id() for details), or if it has a specified 'dev-addr'
property, use that in lieu of the default get_phy_device() logic

- pass both to phy_device_create() and eventually introduce a helper
function that lets you populate the c45_ids structure

Then for each function that does the loop above, as long as you have a
phydev reference, you can have phydev->dev_addr = 0x1e be where to start
from, if it is 0, then start at 1 (like it currently is). If you don't
have a phy device reference, which would be get_phy_c45_ids() then just
make sure you don't call that function :)

>  struct phy_c45_device_ids {
>  	u32 devices_in_package;
> -	u32 device_ids[8];
> +	u32 device_ids[32];
> +	u32 devices_addrs[32];
>  };

This looks like a fix in itself, so it is worth a separate patch.
-- 
Florian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
  2018-03-26 22:25 ` Rob Herring
@ 2018-03-27  8:10   ` Vicenţiu Galanopulo
  2018-03-27 14:24     ` Andrew Lunn
  0 siblings, 1 reply; 9+ messages in thread
From: Vicenţiu Galanopulo @ 2018-03-27  8:10 UTC (permalink / raw)
  To: Rob Herring
  Cc: netdev, linux-kernel, mark.rutland, davem, marcel, devicetree,
	Madalin-cristian Bucur, Alexandru Marginean



> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Tuesday, March 27, 2018 1:25 AM
> To: Vicenţiu Galanopulo <vicentiu.galanopulo@nxp.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> mark.rutland@arm.com; davem@davemloft.net; marcel@holtmann.org;
> devicetree@vger.kernel.org; Madalin-cristian Bucur <madalin.bucur@nxp.com>;
> Alexandru Marginean <alexandru.marginean@nxp.com>
> Subject: Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr
> and dev-addr code check-up
> 
> On Fri, Mar 23, 2018 at 10:05:22AM -0500, Vicentiu Galanopulo wrote:
> > Reason for this patch is that the Inphi PHY has a vendor specific
> > address space for accessing the
> > C45 MDIO registers - starting from 0x1e.
> >
> > A search of the dev-addr property is done in of_mdiobus_register.
> > If the property is found in the PHY node,
> > of_mdiobus_register_static_phy is called. This is a wrapper function
> > for of_mdiobus_register_phy which finds the device in package based on
> > dev-addr and fills devices_addrs:
> > devices_addrs is a new field added to phy_c45_device_ids.
> > This new field will store the dev-addr property on the same index
> > where the device in package has been found.
> > In order to have dev-addr in get_phy_c45_ids(), mdio_c45_ids is passed
> > from of_mdio.c to phy_device.c as an external variable.
> > In get_phy_device a copy of the mdio_c45_ids is done over the local
> > c45_ids (wich are empty). After the copying, the c45_ids will also
> > contain the static device found from dev-addr.
> > Having dev-addr stored in devices_addrs, in get_phy_c45_ids(), when
> > probing the identifiers, dev-addr can be extracted from devices_addrs
> > and probed if devices_addrs[current_identifier] is not 0.
> > This way changing the kernel API is avoided completely.
> >
> > As a plus to this patch, num_ids in get_phy_c45_ids, has the value 8
> > (ARRAY_SIZE(c45_ids->device_ids)),
> > but the u32 *devs can store 32 devices in the bitfield.
> > If a device is stored in *devs, in bits 32 to 9, it will not be found.
> > This is the reason for changing in phy.h, the size of device_ids
> > array.
> >
> > Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/net/phy.txt |  6 ++
> 
> Please split bindings to separate patch.

 
Thanks Rob for your comments. I was considering doing that after I reach a more stable, non-RFC version of the patch. I also added a v3, before
your comments, I think... so in the next one, v4, I will split the binding to a separate patch.


> 
> >  drivers/net/phy/phy_device.c                  | 22 +++++-
> >  drivers/of/of_mdio.c                          | 98 ++++++++++++++++++++++++++-
> >  include/linux/phy.h                           |  5 +-
> >  4 files changed, 125 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/phy.txt
> > b/Documentation/devicetree/bindings/net/phy.txt
> > index d2169a5..82692e2 100644
> > --- a/Documentation/devicetree/bindings/net/phy.txt
> > +++ b/Documentation/devicetree/bindings/net/phy.txt
> > @@ -61,6 +61,11 @@ Optional Properties:
> >  - reset-deassert-us: Delay after the reset was deasserted in microseconds.
> >    If this property is missing the delay will be skipped.
> >
> > +- dev-addr: If set, it indicates the device address of the PHY to be
> > +used
> > +  when accessing the C45 PHY registers over MDIO. It is used for
> > +vendor specific
> > +  register space addresses that do no conform to standard address for
> > +the MDIO
> > +  registers (e.g. MMD30)
> 
> This is a 2nd MDIO address, right? Can't you just append this to reg property?
> 
> Rob

Yes, it is a 2nd MDIO address which is coming from the PHY vendor. This address cannot be obtained by querying the MDIO bus, it's specified in the PHY datasheet. The current kernel implementation was relying on the fact that this address is in the decimal 0 to 7 range. That worked for the PHYs which already have a kernel driver, but for the new Inphi PHY, which I'm trying to add, it didn't.  
If I were to append the dev-addr address to the reg property,  I would have to fit two 32bit variable into a single one... in my opinion the code is clearer having the two addresses as distinct variables.... And so far, the comments from Andrew or Florian didn't go in this direction..  

Vicentiu

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
  2018-03-26 22:44 ` Florian Fainelli
@ 2018-03-27 10:02   ` Vicenţiu Galanopulo
  0 siblings, 0 replies; 9+ messages in thread
From: Vicenţiu Galanopulo @ 2018-03-27 10:02 UTC (permalink / raw)
  To: Florian Fainelli, netdev, linux-kernel, robh+dt, mark.rutland,
	davem, marcel, devicetree
  Cc: Madalin-cristian Bucur, Alexandru Marginean



> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Tuesday, March 27, 2018 1:45 AM
> To: Vicenţiu Galanopulo <vicentiu.galanopulo@nxp.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; robh+dt@kernel.org;
> mark.rutland@arm.com; davem@davemloft.net; marcel@holtmann.org;
> devicetree@vger.kernel.org
> Cc: Madalin-cristian Bucur <madalin.bucur@nxp.com>; Alexandru Marginean
> <alexandru.marginean@nxp.com>
> Subject: Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr
> and dev-addr code check-up
> 
> On 03/23/2018 08:05 AM, Vicentiu Galanopulo wrote:
> > Reason for this patch is that the Inphi PHY has a vendor specific
> > address space for accessing the
> > C45 MDIO registers - starting from 0x1e.
> >
> > A search of the dev-addr property is done in of_mdiobus_register.
> > If the property is found in the PHY node,
> > of_mdiobus_register_static_phy is called. This is a wrapper function
> > for of_mdiobus_register_phy which finds the device in package based on
> > dev-addr and fills devices_addrs:
> > devices_addrs is a new field added to phy_c45_device_ids.
> > This new field will store the dev-addr property on the same index
> > where the device in package has been found.
> > In order to have dev-addr in get_phy_c45_ids(), mdio_c45_ids is passed
> > from of_mdio.c to phy_device.c as an external variable.
> > In get_phy_device a copy of the mdio_c45_ids is done over the local
> > c45_ids (wich are empty). After the copying, the c45_ids will also
> > contain the static device found from dev-addr.
> > Having dev-addr stored in devices_addrs, in get_phy_c45_ids(), when
> > probing the identifiers, dev-addr can be extracted from devices_addrs
> > and probed if devices_addrs[current_identifier] is not 0.
> > This way changing the kernel API is avoided completely.
> >
> > As a plus to this patch, num_ids in get_phy_c45_ids, has the value 8
> > (ARRAY_SIZE(c45_ids->device_ids)),
> > but the u32 *devs can store 32 devices in the bitfield.
> > If a device is stored in *devs, in bits 32 to 9, it will not be found.
> > This is the reason for changing in phy.h, the size of device_ids
> > array.
> >
> > Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
> > ---
> 
> Correct me if I am completely misunderstanding  the problem, but have you
> considered doing the following:
> 
> - if all you need to is to replace instances of loops that do:
> 
>         if (phydev->is_c45) {
>                 for (i = 1; i < num_ids; i++) {
>                         if (!(phydev->c45_ids.devices_in_package & (1 <<
> i)))
>                                 continue;
> 
> with one that starts at dev-addr, as specified by Device Tree, then I suspect
> there is an easier way to do what you want rather than your fairly intrusive
> patch to do that:
> 


Sorry for the lengthy comment and sorry if this is redundant, I'm just trying to explain 
best that I can my patch: 
The loop goes through the devices_in_packages, and where it finds a bit that is set, it
continues to get the PHY device ID.
But, to have devices_in_package with those bits set, a previous querying of the 
MDIO_DEVS2 and MDIO_DEVS1 is necessary. And in the MDIO bus query,
dev-addr, from the device tree,  is part of the whole register address:
reg_addr = MII_ADDR_C45 | dev_addr << 16 | MDIO_DEVS2; 

Andrew suggested in his first comment that the device tree lookup could be done
in of_mdiobus_register(),  probably because of the loop which already checks the
<reg> property of the PHY.
My understanding of his comments was that I could propagate, as a parameter,
dev-addr, down the hierarchy call of_mdiobus_register_phy()-> get_phy_device()->
get_phy_id()->get_phy_c45_ids()->get_phy_c45_devs_in_pkg(dev_addr param existed here).
This is where the loop you're mentioning needed some altering, because the loop index is
actually the dev_addr parameter from get_phy_c45_devs_in_pkg(), and it's from 1 to 7 (num_ids = 8)

This worked for the other PHYs which had dev-addr in this range, but it doesn't work for the INPHI,
which has dev_add = 30 (0x1e).
After I did the extension of the  device_ids from 8 to 32 to match 
the devs (u32 *devs = &c45_ids->devices_in_package;)  in get_phy_c45_ids() :
 -	u32 device_ids[8];
 +	u32 device_ids[32];

I realized that dev_addr for other PHY vendors could be larger than 31 (just a coincidence that 
for INPHI is 30 and it fits), and that dev-addr should be a separate parameter, that still 
has to match the bit position from *devs (&c45_ids->devices_in_package) 

So, I didn't had to change the loop to start from dev-addr, just to let it check if the bit is set in *devs (as before), 
and if that bit corresponds to the INPHI PHY (dev-addr has been set in the PHY device tree node), use
dev-addr in getting the PHY ID.

The loop start index has to remain the same because other PHY vendors have dev-addr 1 to 7, and 
PHY discovering succeeds.

For other issues that I had with the above solution (plus Andrew's comment about the SMP), I uploaded 
a v3... probably slightly before you made this comment. Please have look at it, and paste the below 
comments if they still apply. I was not able to match them with my latest patch...

Thanks,
Vicentiu

> - patch of_mdiobus_register_phy() to lookup both the c45 compatible string as
> well as fetch the "dev-addr" property
> 
> - provide a PHY Device Tree node that has its OUI as a compatible string (see
> of_get_phy_id() for details), or if it has a specified 'dev-addr'
> property, use that in lieu of the default get_phy_device() logic
> 
> - pass both to phy_device_create() and eventually introduce a helper function
> that lets you populate the c45_ids structure
> 
> Then for each function that does the loop above, as long as you have a phydev
> reference, you can have phydev->dev_addr = 0x1e be where to start from, if it is
> 0, then start at 1 (like it currently is). If you don't have a phy device reference,
> which would be get_phy_c45_ids() then just make sure you don't call that
> function :)
> 
> >  struct phy_c45_device_ids {
> >  	u32 devices_in_package;
> > -	u32 device_ids[8];
> > +	u32 device_ids[32];
> > +	u32 devices_addrs[32];
> >  };
> 
> This looks like a fix in itself, so it is worth a separate patch.
> --
> Florian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
  2018-03-27  8:10   ` Vicenţiu Galanopulo
@ 2018-03-27 14:24     ` Andrew Lunn
  2018-03-28 14:31       ` Rob Herring
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Lunn @ 2018-03-27 14:24 UTC (permalink / raw)
  To: Vicenţiu Galanopulo
  Cc: Rob Herring, netdev, linux-kernel, mark.rutland, davem, marcel,
	devicetree, Madalin-cristian Bucur, Alexandru Marginean

> > This is a 2nd MDIO address, right? Can't you just append this to reg property?

Hi Rob

It is a sub address.

There are two different MDIO addressing schemes. Clause 22 allowed for
32 different addresses on an MDIO bus. Clause 45 extended that. You
have the existing 32 addresses for a package. However, within a
package, you can have 32 devices.

You are supposed to be able to look are registers inside the package,
and it will tell you which devices in the packages are in use. You can
then look at those devices and figure out what they are using ID
registers.

However some vendors get this wrong, they don't fill in the devices in
package information. So the generic probe code never finds them. We
need to pass it a hint, go looking at this specific device in the
package.

You can mix Clause 22 and Clause 45 on the same bus. Does DT allow two
different reg formats to be used at once? Can we have some reg
properties with a single value, and some with two values? I thought
#address-cells = <1> means there should be a single address in reg.

	Andrew

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
  2018-03-27 14:24     ` Andrew Lunn
@ 2018-03-28 14:31       ` Rob Herring
  2018-03-28 15:27         ` Andrew Lunn
  0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2018-03-28 14:31 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Vicenţiu Galanopulo, netdev, linux-kernel, mark.rutland,
	davem, marcel, devicetree, Madalin-cristian Bucur,
	Alexandru Marginean

On Tue, Mar 27, 2018 at 9:24 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > This is a 2nd MDIO address, right? Can't you just append this to reg property?
>
> Hi Rob
>
> It is a sub address.
>
> There are two different MDIO addressing schemes. Clause 22 allowed for
> 32 different addresses on an MDIO bus. Clause 45 extended that. You
> have the existing 32 addresses for a package. However, within a
> package, you can have 32 devices.

Sounds similar to functions in PCI land (which are part of reg address).

> You are supposed to be able to look are registers inside the package,
> and it will tell you which devices in the packages are in use. You can
> then look at those devices and figure out what they are using ID
> registers.
>
> However some vendors get this wrong, they don't fill in the devices in
> package information. So the generic probe code never finds them. We
> need to pass it a hint, go looking at this specific device in the
> package.

If this is a rare case and something future devices should get right,
then I'm more inclined to use 'dev-addr' rather than extending reg.

> You can mix Clause 22 and Clause 45 on the same bus. Does DT allow two
> different reg formats to be used at once? Can we have some reg
> properties with a single value, and some with two values? I thought
> #address-cells = <1> means there should be a single address in reg.

#address-cells is how many cells (aka u32) it takes to store an
address, not how many addresses you have. The number of addresses is
(sizeof(reg) / 4) / (#address-cells + #size-cells). So yes, you can
have different number of addresses for each device, but you can't have
different sizes of addresses (e.g. 32-bit and 64-bit) in one bus. But
I think in this case it would logically be 1 address with 2 cells
because it is the port and device together that make up the address.
If you did that, you'd have to define how to express a clause 22
device with 2 cells. You could either set a high bit in the first cell
to indicate clause 45 address or use an illegal device address in the
2nd cell.

However, the MDIO core would need to handle 2 address cells. If more
than 1 address cell is an error currently, that causes a compatibility
problem with new dtb and older kernels (but could be addressed with
stable updates).

Rob

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
  2018-03-28 14:31       ` Rob Herring
@ 2018-03-28 15:27         ` Andrew Lunn
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Lunn @ 2018-03-28 15:27 UTC (permalink / raw)
  To: Rob Herring
  Cc: Vicenţiu Galanopulo, netdev, linux-kernel, mark.rutland,
	davem, marcel, devicetree, Madalin-cristian Bucur,
	Alexandru Marginean

> If this is a rare case and something future devices should get right,
> then I'm more inclined to use 'dev-addr' rather than extending reg.

Hi Rob

The sample size is a bit small at the moment to know how rare it is.
I think we have 6 C45 devices so far, and two get this wrong. C45 is
mostly used for PHY device with > 1Gbps. There are not so many of
these yet.

I would also prefer to use 'dev-addr'.

  Andrew

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-03-28 15:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-23 15:05 [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up Vicentiu Galanopulo
2018-03-23 15:44 ` Andrew Lunn
2018-03-26 22:25 ` Rob Herring
2018-03-27  8:10   ` Vicenţiu Galanopulo
2018-03-27 14:24     ` Andrew Lunn
2018-03-28 14:31       ` Rob Herring
2018-03-28 15:27         ` Andrew Lunn
2018-03-26 22:44 ` Florian Fainelli
2018-03-27 10:02   ` Vicenţiu Galanopulo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).