[PATCH v2 0/2] net: thunder: Add ACPI support.

[PATCH v2 0/2] net: thunder: Add ACPI support.

[David Daney]

From: David Daney <david.daney@cavium.com>

Change from v1:  Drop PHY binding part, use fwnode_property* APIs.

The first patch (1/2) rearranges the existing code a little with no
functional change to get ready for the second.  The second (2/2) does
the actual work of adding support to extract the needed information
from the ACPI tables.

David Daney (1):
  net, thunder, bgx: Add support to get MAC address from ACPI.

Robert Richter (1):
  net: thunder: Factor out DT specific code in BGX

 drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 134 +++++++++++++++++++---
 1 file changed, 120 insertions(+), 14 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] net: thunder: Factor out DT specific code in BGX

[David Daney]

From: Robert Richter <rrichter@redhat.com>

Separate DT code in preparation for follow-on ACPI integration.

Based on code from: Tomasz Nowicki <tomasz.nowicki@linaro.org>

Signed-off-by: Robert Richter <rrichter@cavium.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 48 +++++++++++++++++------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
index b961a89..615b2af 100644
--- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
+++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
@@ -835,18 +835,28 @@ static void bgx_get_qlm_mode(struct bgx *bgx)
 	}
 }
 
-static void bgx_init_of(struct bgx *bgx, struct device_node *np)
+#if IS_ENABLED(CONFIG_OF_MDIO)
+
+static int bgx_init_of_phy(struct bgx *bgx)
 {
+	struct device_node *np;
 	struct device_node *np_child;
 	u8 lmac = 0;
+	char bgx_sel[5];
+	const char *mac;
 
-	for_each_child_of_node(np, np_child) {
-		struct device_node *phy_np;
-		const char *mac;
+	/* Get BGX node from DT */
+	snprintf(bgx_sel, 5, "bgx%d", bgx->bgx_id);
+	np = of_find_node_by_name(NULL, bgx_sel);
+	if (!np)
+		return -ENODEV;
 
-		phy_np = of_parse_phandle(np_child, "phy-handle", 0);
-		if (phy_np)
-			bgx->lmac[lmac].phydev = of_phy_find_device(phy_np);
+	for_each_child_of_node(np, np_child) {
+		struct device_node *phy_np = of_parse_phandle(np_child,
+							      "phy-handle", 0);
+		if (!phy_np)
+			continue;
+		bgx->lmac[lmac].phydev = of_phy_find_device(phy_np);
 
 		mac = of_get_mac_address(np_child);
 		if (mac)
@@ -858,6 +868,21 @@ static void bgx_init_of(struct bgx *bgx, struct device_node *np)
 		if (lmac == MAX_LMAC_PER_BGX)
 			break;
 	}
+	return 0;
+}
+
+#else
+
+static int bgx_init_of_phy(struct bgx *bgx)
+{
+	return -ENODEV;
+}
+
+#endif /* CONFIG_OF_MDIO */
+
+static int bgx_init_phy(struct bgx *bgx)
+{
+	return bgx_init_of_phy(bgx);
 }
 
 static int bgx_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
@@ -865,8 +890,6 @@ static int bgx_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	int err;
 	struct device *dev = &pdev->dev;
 	struct bgx *bgx = NULL;
-	struct device_node *np;
-	char bgx_sel[5];
 	u8 lmac;
 
 	bgx = devm_kzalloc(dev, sizeof(*bgx), GFP_KERNEL);
@@ -902,10 +925,9 @@ static int bgx_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	bgx_vnic[bgx->bgx_id] = bgx;
 	bgx_get_qlm_mode(bgx);
 
-	snprintf(bgx_sel, 5, "bgx%d", bgx->bgx_id);
-	np = of_find_node_by_name(NULL, bgx_sel);
-	if (np)
-		bgx_init_of(bgx, np);
+	err = bgx_init_phy(bgx);
+	if (err)
+		goto err_enable;
 
 	bgx_init_hw(bgx);
 
-- 
1.9.1

[PATCH v2 2/2] net, thunder, bgx: Add support to get MAC address from ACPI.

[David Daney]

From: David Daney <david.daney@cavium.com>

Currently there is no way to get the MAC address in a firmware
independent manner, so set the MAC address of the device directly from
the ACPI tables.

The binding agrees with the proposed standard here:

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

Based on code from: Narinder Dhillon <ndhillon@cavium.com>
                    Tomasz Nowicki <tomasz.nowicki@linaro.org>
                    Robert Richter <rrichter@cavium.com>

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
Signed-off-by: Robert Richter <rrichter@cavium.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 86 ++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
index 615b2af..5e54186 100644
--- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
+++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
@@ -6,6 +6,7 @@
  * as published by the Free Software Foundation.
  */
 
+#include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/pci.h>
@@ -26,7 +27,7 @@
 struct lmac {
 	struct bgx		*bgx;
 	int			dmac;
-	unsigned char		mac[ETH_ALEN];
+	u8			mac[ETH_ALEN];
 	bool			link_up;
 	int			lmacid; /* ID within BGX */
 	int			lmacid_bd; /* ID on board */
@@ -835,6 +836,86 @@ static void bgx_get_qlm_mode(struct bgx *bgx)
 	}
 }
 
+#ifdef CONFIG_ACPI
+
+static int acpi_get_mac_address(struct acpi_device *adev, u8 *dst)
+{
+	u8 mac[ETH_ALEN];
+	int ret;
+
+	ret = fwnode_property_read_u8_array(acpi_fwnode_handle(adev),
+					    "mac-address", mac, ETH_ALEN);
+	if (ret)
+		goto out;
+
+	if (!is_valid_ether_addr(mac)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	memcpy(dst, mac, ETH_ALEN);
+out:
+	return ret;
+}
+
+/* Currently only sets the MAC address. */
+static acpi_status bgx_acpi_register_phy(acpi_handle handle,
+					 u32 lvl, void *context, void **rv)
+{
+	struct bgx *bgx = context;
+	struct acpi_device *adev;
+
+	if (acpi_bus_get_device(handle, &adev))
+		goto out;
+
+	acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
+
+	SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
+
+	bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
+out:
+	bgx->lmac_count++;
+	return AE_OK;
+}
+
+static acpi_status bgx_acpi_match_id(acpi_handle handle, u32 lvl,
+				     void *context, void **ret_val)
+{
+	struct acpi_buffer string = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct bgx *bgx = context;
+	char bgx_sel[5];
+
+	snprintf(bgx_sel, 5, "BGX%d", bgx->bgx_id);
+	if (ACPI_FAILURE(acpi_get_name(handle, ACPI_SINGLE_NAME, &string))) {
+		pr_warn("Invalid link device\n");
+		return AE_OK;
+	}
+
+	if (strncmp(string.pointer, bgx_sel, 4))
+		return AE_OK;
+
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+			    bgx_acpi_register_phy, NULL, bgx, NULL);
+
+	kfree(string.pointer);
+	return AE_CTRL_TERMINATE;
+}
+
+static int bgx_init_acpi_phy(struct bgx *bgx)
+{
+	acpi_get_devices(NULL, bgx_acpi_match_id, bgx, (void **)NULL);
+	return 0;
+}
+
+#else
+
+static int bgx_init_acpi_phy(struct bgx *bgx)
+{
+	return -ENODEV;
+}
+
+#endif /* CONFIG_ACPI */
+
 #if IS_ENABLED(CONFIG_OF_MDIO)
 
 static int bgx_init_of_phy(struct bgx *bgx)
@@ -882,6 +963,9 @@ static int bgx_init_of_phy(struct bgx *bgx)
 
 static int bgx_init_phy(struct bgx *bgx)
 {
+	if (!acpi_disabled)
+		return bgx_init_acpi_phy(bgx);
+
 	return bgx_init_of_phy(bgx);
 }
 
-- 
1.9.1

Re: [PATCH v2 0/2] net: thunder: Add ACPI support.

[David Miller]

From: David Daney <ddaney.cavm@gmail.com>
Date: Mon, 10 Aug 2015 17:58:35 -0700

> Change from v1:  Drop PHY binding part, use fwnode_property* APIs.
> 
> The first patch (1/2) rearranges the existing code a little with no
> functional change to get ready for the second.  The second (2/2) does
> the actual work of adding support to extract the needed information
> from the ACPI tables.

Series applied.

In the future it might be better structured to try and get the OF
node, and if that fails then try and use the ACPI method to obtain
these values.

Re: [PATCH v2 0/2] net: thunder: Add ACPI support.

[David Daney]

On 08/11/2015 11:49 AM, David Miller wrote:
> From: David Daney <ddaney.cavm@gmail.com>
> Date: Mon, 10 Aug 2015 17:58:35 -0700
>
>> Change from v1:  Drop PHY binding part, use fwnode_property* APIs.
>>
>> The first patch (1/2) rearranges the existing code a little with no
>> functional change to get ready for the second.  The second (2/2) does
>> the actual work of adding support to extract the needed information
>> from the ACPI tables.
>
> Series applied.

Thank you very much.

>
> In the future it might be better structured to try and get the OF
> node, and if that fails then try and use the ACPI method to obtain
> these values.

Our current approach, as you can see in the patch, is the opposite.  If 
ACPI is being used, prefer that over the OF device tree.

You seem to be recommending precedence for OF.  It should be consistent 
across all drivers/sub-systems, so do you really think that OF before 
ACPI is the way to go?

Thanks,
David Daney

Re: [PATCH v2 0/2] net: thunder: Add ACPI support.

[Robert Richter]

On 11.08.15 13:04:55, David Daney wrote:
> >In the future it might be better structured to try and get the OF
> >node, and if that fails then try and use the ACPI method to obtain
> >these values.
> 
> Our current approach, as you can see in the patch, is the opposite.  If ACPI
> is being used, prefer that over the OF device tree.
> 
> You seem to be recommending precedence for OF.  It should be consistent
> across all drivers/sub-systems, so do you really think that OF before ACPI
> is the way to go?

If ACPI is enabled then no OF function may be called at all.

With !ACPI or acpi=no kernel parameter, then acpi_disabled is set and
no ACPI function should be called. It always falls back to and only
uses OF/devicetree in this case.

So there is now way to try devicetree first and then use acpi or vice
versa. There is no mixup using acpi or devicetree with the same boot,
either one or the other.

-Robert

Re: [PATCH v2 0/2] net: thunder: Add ACPI support.

[David Miller]

From: David Daney <ddaney@caviumnetworks.com>
Date: Tue, 11 Aug 2015 13:04:55 -0700

> You seem to be recommending precedence for OF.  It should be
> consistent across all drivers/sub-systems, so do you really think
> that OF before ACPI is the way to go?

I just think it's more hackish to test acpi_disabled than to
simply see if the matching OF node even exists.

If ACPI is enabled, no OF node will be found.

It could just be my preference for such things.

I really wish it just fell out from the probing method, but
we're using PCI for that.

Re: [PATCH v2 0/2] net: thunder: Add ACPI support.

[David Miller]

From: Robert Richter <robert.richter@caviumnetworks.com>
Date: Tue, 11 Aug 2015 22:12:37 +0200

> On 11.08.15 13:04:55, David Daney wrote:
>> >In the future it might be better structured to try and get the OF
>> >node, and if that fails then try and use the ACPI method to obtain
>> >these values.
>> 
>> Our current approach, as you can see in the patch, is the opposite.  If ACPI
>> is being used, prefer that over the OF device tree.
>> 
>> You seem to be recommending precedence for OF.  It should be consistent
>> across all drivers/sub-systems, so do you really think that OF before ACPI
>> is the way to go?
> 
> If ACPI is enabled then no OF function may be called at all.

That makes no sense to me at all.

If ACPI is enabled, the OF routines should return no nodes etc.

Re: [PATCH v2 0/2] net: thunder: Add ACPI support.

[Catalin Marinas]

On Tue, Aug 11, 2015 at 01:04:55PM -0700, David Daney wrote:
> On 08/11/2015 11:49 AM, David Miller wrote:
> >From: David Daney <ddaney.cavm@gmail.com>
> >Date: Mon, 10 Aug 2015 17:58:35 -0700
> >
> >>Change from v1:  Drop PHY binding part, use fwnode_property* APIs.
> >>
> >>The first patch (1/2) rearranges the existing code a little with no
> >>functional change to get ready for the second.  The second (2/2) does
> >>the actual work of adding support to extract the needed information
> >>from the ACPI tables.
> >
> >Series applied.
> 
> Thank you very much.
> 
> >In the future it might be better structured to try and get the OF
> >node, and if that fails then try and use the ACPI method to obtain
> >these values.
> 
> Our current approach, as you can see in the patch, is the opposite.  If ACPI
> is being used, prefer that over the OF device tree.
> 
> You seem to be recommending precedence for OF.  It should be consistent
> across all drivers/sub-systems, so do you really think that OF before ACPI
> is the way to go?

On arm64 (unless you use a vendor kernel), DT takes precedence over ACPI
if both arm provided to the kernel (and it's a fair assumption given
that ACPI on ARM is still in the early days). You could also force ACPI
with acpi=force on the kernel cmd line and the arch code will not
unflatten the DT even if it is provided, therefore is_of_node(fwnode)
returning false.

I haven't looked at your driver in detail but something like AMD's
xgbe_probe() uses a single function for both DT and ACPI with
device_property_read_*() functions getting the information from the
correct place in either case. The ACPI vs DT precedence is handled by
the arch boot code, we never mix the two and confuse the drivers.

-- 
Catalin

Re: [PATCH v2 0/2] net: thunder: Add ACPI support.

[David Daney]

On 08/12/2015 08:23 AM, Catalin Marinas wrote:
> On Tue, Aug 11, 2015 at 01:04:55PM -0700, David Daney wrote:
>> On 08/11/2015 11:49 AM, David Miller wrote:
>>> From: David Daney <ddaney.cavm@gmail.com>
>>> Date: Mon, 10 Aug 2015 17:58:35 -0700
>>>
>>>> Change from v1:  Drop PHY binding part, use fwnode_property* APIs.
>>>>
>>>> The first patch (1/2) rearranges the existing code a little with no
>>>> functional change to get ready for the second.  The second (2/2) does
>>>> the actual work of adding support to extract the needed information
>>> >from the ACPI tables.
>>>
>>> Series applied.
>>
>> Thank you very much.
>>
>>> In the future it might be better structured to try and get the OF
>>> node, and if that fails then try and use the ACPI method to obtain
>>> these values.
>>
>> Our current approach, as you can see in the patch, is the opposite.  If ACPI
>> is being used, prefer that over the OF device tree.
>>
>> You seem to be recommending precedence for OF.  It should be consistent
>> across all drivers/sub-systems, so do you really think that OF before ACPI
>> is the way to go?
>
> On arm64 (unless you use a vendor kernel), DT takes precedence over ACPI
> if both arm provided to the kernel (and it's a fair assumption given
> that ACPI on ARM is still in the early days). You could also force ACPI
> with acpi=force on the kernel cmd line and the arch code will not
> unflatten the DT even if it is provided, therefore is_of_node(fwnode)
> returning false.
>
> I haven't looked at your driver in detail but something like AMD's
> xgbe_probe() uses a single function for both DT and ACPI with
> device_property_read_*() functions getting the information from the
> correct place in either case. The ACPI vs DT precedence is handled by
> the arch boot code, we never mix the two and confuse the drivers.
>

My long term plan is to create something like 
firmware_get_mac_address(), that would encapsulate  of_get_mac_address() 
and the ACPI equivalent.

Same for firmware_phy_find_device().

These would function as you suggest, but lacking this infrastructure, we 
implemented this patch set instead.

Thanks,
David Daney

Re: [PATCH v2 0/2] net: thunder: Add ACPI support.

[Hanjun Guo]

On 08/12/2015 11:36 PM, David Daney wrote:
> On 08/12/2015 08:23 AM, Catalin Marinas wrote:
>> On Tue, Aug 11, 2015 at 01:04:55PM -0700, David Daney wrote:
>>> On 08/11/2015 11:49 AM, David Miller wrote:
>>>> From: David Daney <ddaney.cavm@gmail.com>
>>>> Date: Mon, 10 Aug 2015 17:58:35 -0700
>>>>
>>>>> Change from v1:  Drop PHY binding part, use fwnode_property* APIs.
>>>>>
>>>>> The first patch (1/2) rearranges the existing code a little with no
>>>>> functional change to get ready for the second.  The second (2/2) does
>>>>> the actual work of adding support to extract the needed information
>>>> >from the ACPI tables.
>>>>
>>>> Series applied.
>>>
>>> Thank you very much.
>>>
>>>> In the future it might be better structured to try and get the OF
>>>> node, and if that fails then try and use the ACPI method to obtain
>>>> these values.
>>>
>>> Our current approach, as you can see in the patch, is the opposite.
>>> If ACPI
>>> is being used, prefer that over the OF device tree.
>>>
>>> You seem to be recommending precedence for OF.  It should be consistent
>>> across all drivers/sub-systems, so do you really think that OF before
>>> ACPI
>>> is the way to go?
>>
>> On arm64 (unless you use a vendor kernel), DT takes precedence over ACPI
>> if both arm provided to the kernel (and it's a fair assumption given
>> that ACPI on ARM is still in the early days). You could also force ACPI
>> with acpi=force on the kernel cmd line and the arch code will not
>> unflatten the DT even if it is provided, therefore is_of_node(fwnode)
>> returning false.

Yes. on the other hand, if no DT is provided, will try ACPI even
if no acpi=force on the kernel cmd line.

>>
>> I haven't looked at your driver in detail but something like AMD's
>> xgbe_probe() uses a single function for both DT and ACPI with
>> device_property_read_*() functions getting the information from the
>> correct place in either case. The ACPI vs DT precedence is handled by
>> the arch boot code, we never mix the two and confuse the drivers.
>>
>
> My long term plan is to create something like
> firmware_get_mac_address(), that would encapsulate  of_get_mac_address()
> and the ACPI equivalent.
>
> Same for firmware_phy_find_device().

I'm very keen on seeing that happens :)

Thanks
Hanjun