From: David Daney <[email protected]>
Hook up PHYs, and get MAC address from ACPI for the thunder driver.
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 for ACPI binding.
Robert Richter (1):
net: thunder: Factor out DT specific code in BGX
drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 183 ++++++++++++++++++++--
1 file changed, 169 insertions(+), 14 deletions(-)
--
1.9.1
From: Robert Richter <[email protected]>
Separate DT code in preparation for follow-on ACPI integration.
Based on code from: Tomasz Nowicki <[email protected]>
Signed-off-by: Robert Richter <[email protected]>
Signed-off-by: David Daney <[email protected]>
---
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
From: David Daney <[email protected]>
Find out which PHYs belong to which BGX instance in the ACPI way.
Set the MAC address of the device as provided by ACPI tables. This is
similar to the implementation for devicetree in
of_get_mac_address(). The table is searched for the device property
entries "mac-address", "local-mac-address" and "address" in that
order. The address is provided in a u64 variable and must contain a
valid 6 bytes-len mac addr.
Based on code from: Narinder Dhillon <[email protected]>
Tomasz Nowicki <[email protected]>
Robert Richter <[email protected]>
Signed-off-by: Tomasz Nowicki <[email protected]>
Signed-off-by: Robert Richter <[email protected]>
Signed-off-by: David Daney <[email protected]>
---
drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 137 +++++++++++++++++++++-
1 file changed, 135 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
index 615b2af..2056583 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,133 @@ static void bgx_get_qlm_mode(struct bgx *bgx)
}
}
+#ifdef CONFIG_ACPI
+
+static int bgx_match_phy_id(struct device *dev, void *data)
+{
+ struct phy_device *phydev = to_phy_device(dev);
+ u32 *phy_id = data;
+
+ if (phydev->addr == *phy_id)
+ return 1;
+
+ return 0;
+}
+
+static const char * const addr_propnames[] = {
+ "mac-address",
+ "local-mac-address",
+ "address",
+};
+
+static int acpi_get_mac_address(struct acpi_device *adev, u8 *dst)
+{
+ const union acpi_object *prop;
+ u64 mac_val;
+ u8 mac[ETH_ALEN];
+ int i, j;
+ int ret;
+
+ for (i = 0; i < ARRAY_SIZE(addr_propnames); i++) {
+ ret = acpi_dev_get_property(adev, addr_propnames[i],
+ ACPI_TYPE_INTEGER, &prop);
+ if (ret)
+ continue;
+
+ mac_val = prop->integer.value;
+
+ if (mac_val & (~0ULL << 48))
+ continue; /* more than 6 bytes */
+
+ for (j = 0; j < ARRAY_SIZE(mac); j++)
+ mac[j] = (u8)(mac_val >> (8 * j));
+ if (!is_valid_ether_addr(mac))
+ continue;
+
+ memcpy(dst, mac, ETH_ALEN);
+
+ return 0;
+ }
+
+ return ret ? ret : -EINVAL;
+}
+
+static acpi_status bgx_acpi_register_phy(acpi_handle handle,
+ u32 lvl, void *context, void **rv)
+{
+ struct acpi_reference_args args;
+ const union acpi_object *prop;
+ struct bgx *bgx = context;
+ struct acpi_device *adev;
+ struct device *phy_dev;
+ u32 phy_id;
+
+ if (acpi_bus_get_device(handle, &adev))
+ goto out;
+
+ SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
+
+ acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
+
+ bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
+
+ if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
+ goto out;
+
+ if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
+ goto out;
+
+ phy_id = prop->integer.value;
+
+ phy_dev = bus_find_device(&mdio_bus_type, NULL, (void *)&phy_id,
+ bgx_match_phy_id);
+ if (!phy_dev)
+ goto out;
+
+ bgx->lmac[bgx->lmac_count].phydev = to_phy_device(phy_dev);
+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,7 +1010,12 @@ static int bgx_init_of_phy(struct bgx *bgx)
static int bgx_init_phy(struct bgx *bgx)
{
- return bgx_init_of_phy(bgx);
+ int err = bgx_init_of_phy(bgx);
+
+ if (err != -ENODEV)
+ return err;
+
+ return bgx_init_acpi_phy(bgx);
}
static int bgx_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
--
1.9.1
On 07.08.2015 02:33, David Daney wrote:
> From: David Daney <[email protected]>
>
> Find out which PHYs belong to which BGX instance in the ACPI way.
>
> Set the MAC address of the device as provided by ACPI tables. This is
> similar to the implementation for devicetree in
> of_get_mac_address(). The table is searched for the device property
> entries "mac-address", "local-mac-address" and "address" in that
> order. The address is provided in a u64 variable and must contain a
> valid 6 bytes-len mac addr.
>
> Based on code from: Narinder Dhillon <[email protected]>
> Tomasz Nowicki <[email protected]>
> Robert Richter <[email protected]>
>
> Signed-off-by: Tomasz Nowicki <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
> Signed-off-by: David Daney <[email protected]>
> ---
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 137 +++++++++++++++++++++-
> 1 file changed, 135 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
> index 615b2af..2056583 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,133 @@ static void bgx_get_qlm_mode(struct bgx *bgx)
> }
> }
>
> +#ifdef CONFIG_ACPI
> +
> +static int bgx_match_phy_id(struct device *dev, void *data)
> +{
> + struct phy_device *phydev = to_phy_device(dev);
> + u32 *phy_id = data;
> +
> + if (phydev->addr == *phy_id)
> + return 1;
> +
> + return 0;
> +}
> +
> +static const char * const addr_propnames[] = {
> + "mac-address",
> + "local-mac-address",
> + "address",
> +};
> +
> +static int acpi_get_mac_address(struct acpi_device *adev, u8 *dst)
> +{
> + const union acpi_object *prop;
> + u64 mac_val;
> + u8 mac[ETH_ALEN];
> + int i, j;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(addr_propnames); i++) {
> + ret = acpi_dev_get_property(adev, addr_propnames[i],
> + ACPI_TYPE_INTEGER, &prop);
> + if (ret)
> + continue;
> +
> + mac_val = prop->integer.value;
> +
> + if (mac_val & (~0ULL << 48))
> + continue; /* more than 6 bytes */
> +
> + for (j = 0; j < ARRAY_SIZE(mac); j++)
> + mac[j] = (u8)(mac_val >> (8 * j));
> + if (!is_valid_ether_addr(mac))
> + continue;
> +
> + memcpy(dst, mac, ETH_ALEN);
> +
> + return 0;
> + }
> +
> + return ret ? ret : -EINVAL;
> +}
> +
> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
> + u32 lvl, void *context, void **rv)
> +{
> + struct acpi_reference_args args;
> + const union acpi_object *prop;
> + struct bgx *bgx = context;
> + struct acpi_device *adev;
> + struct device *phy_dev;
> + u32 phy_id;
> +
> + if (acpi_bus_get_device(handle, &adev))
> + goto out;
> +
> + SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
> +
> + acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
> +
> + bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
> +
> + if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
> + goto out;
> +
> + if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
> + goto out;
> +
> + phy_id = prop->integer.value;
> +
> + phy_dev = bus_find_device(&mdio_bus_type, NULL, (void *)&phy_id,
> + bgx_match_phy_id);
> + if (!phy_dev)
> + goto out;
> +
> + bgx->lmac[bgx->lmac_count].phydev = to_phy_device(phy_dev);
> +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,7 +1010,12 @@ static int bgx_init_of_phy(struct bgx *bgx)
>
> static int bgx_init_phy(struct bgx *bgx)
> {
> - return bgx_init_of_phy(bgx);
> + int err = bgx_init_of_phy(bgx);
> +
> + if (err != -ENODEV)
> + return err;
> +
> + return bgx_init_acpi_phy(bgx);
> }
>
If kernel can work with DT and ACPI (both compiled in), it should take
only one path instead of probing DT and ACPI sequentially. How about:
@@ -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 = acpi_disabled ? bgx_init_of_phy(bgx) : bgx_init_acpi_phy(bgx);
+ if (err)
+ goto err_enable;
bgx_init_hw(bgx);
Regards,
Tomasz
On 07.08.15 10:09:04, Tomasz Nowicki wrote:
> On 07.08.2015 02:33, David Daney wrote:
...
> >+#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,7 +1010,12 @@ static int bgx_init_of_phy(struct bgx *bgx)
> >
> > static int bgx_init_phy(struct bgx *bgx)
> > {
> >- return bgx_init_of_phy(bgx);
> >+ int err = bgx_init_of_phy(bgx);
> >+
> >+ if (err != -ENODEV)
> >+ return err;
> >+
> >+ return bgx_init_acpi_phy(bgx);
> > }
> >
>
> If kernel can work with DT and ACPI (both compiled in), it should take only
> one path instead of probing DT and ACPI sequentially. How about:
>
> @@ -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 = acpi_disabled ? bgx_init_of_phy(bgx) : bgx_init_acpi_phy(bgx);
> + if (err)
> + goto err_enable;
>
> bgx_init_hw(bgx);
I would not pollute bgx_probe() with acpi and dt specifics, and instead
keep bgx_init_phy(). The typical design pattern for this is:
static int bgx_init_phy(struct bgx *bgx)
{
#ifdef CONFIG_ACPI
if (!acpi_disabled)
return bgx_init_acpi_phy(bgx);
#endif
return bgx_init_of_phy(bgx);
}
This adds acpi runtime detection (acpi=no), does not call dt code in
case of acpi, and saves the #else for bgx_init_acpi_phy().
-Robert
On 07.08.2015 12:43, Robert Richter wrote:
> On 07.08.15 10:09:04, Tomasz Nowicki wrote:
>> On 07.08.2015 02:33, David Daney wrote:
>
> ...
>
>>> +#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,7 +1010,12 @@ static int bgx_init_of_phy(struct bgx *bgx)
>>>
>>> static int bgx_init_phy(struct bgx *bgx)
>>> {
>>> - return bgx_init_of_phy(bgx);
>>> + int err = bgx_init_of_phy(bgx);
>>> +
>>> + if (err != -ENODEV)
>>> + return err;
>>> +
>>> + return bgx_init_acpi_phy(bgx);
>>> }
>>>
>>
>> If kernel can work with DT and ACPI (both compiled in), it should take only
>> one path instead of probing DT and ACPI sequentially. How about:
>>
>> @@ -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 = acpi_disabled ? bgx_init_of_phy(bgx) : bgx_init_acpi_phy(bgx);
>> + if (err)
>> + goto err_enable;
>>
>> bgx_init_hw(bgx);
>
> I would not pollute bgx_probe() with acpi and dt specifics, and instead
> keep bgx_init_phy(). The typical design pattern for this is:
>
> static int bgx_init_phy(struct bgx *bgx)
> {
> #ifdef CONFIG_ACPI
> if (!acpi_disabled)
> return bgx_init_acpi_phy(bgx);
> #endif
> return bgx_init_of_phy(bgx);
> }
>
> This adds acpi runtime detection (acpi=no), does not call dt code in
> case of acpi, and saves the #else for bgx_init_acpi_phy().
>
I am fine with keeping it in bgx_init_phy(), however we can drop there
#ifdefs since both of bgx_init_{acpi,of}_phy calls have empty stub for
!ACPI and !OF case. Like that:
static int bgx_init_phy(struct bgx *bgx)
{
if (!acpi_disabled)
return bgx_init_acpi_phy(bgx);
else
return bgx_init_of_phy(bgx);
}
Tomasz
On 07.08.15 12:52:41, Tomasz Nowicki wrote:
> On 07.08.2015 12:43, Robert Richter wrote:
> >On 07.08.15 10:09:04, Tomasz Nowicki wrote:
> >>On 07.08.2015 02:33, David Daney wrote:
> >
> >...
> >
> >>>+#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,7 +1010,12 @@ static int bgx_init_of_phy(struct bgx *bgx)
> >>>
> >>> static int bgx_init_phy(struct bgx *bgx)
> >>> {
> >>>- return bgx_init_of_phy(bgx);
> >>>+ int err = bgx_init_of_phy(bgx);
> >>>+
> >>>+ if (err != -ENODEV)
> >>>+ return err;
> >>>+
> >>>+ return bgx_init_acpi_phy(bgx);
> >>> }
> >>>
> >>
> >>If kernel can work with DT and ACPI (both compiled in), it should take only
> >>one path instead of probing DT and ACPI sequentially. How about:
> >>
> >>@@ -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 = acpi_disabled ? bgx_init_of_phy(bgx) : bgx_init_acpi_phy(bgx);
> >>+ if (err)
> >>+ goto err_enable;
> >>
> >> bgx_init_hw(bgx);
> >
> >I would not pollute bgx_probe() with acpi and dt specifics, and instead
> >keep bgx_init_phy(). The typical design pattern for this is:
> >
> >static int bgx_init_phy(struct bgx *bgx)
> >{
> >#ifdef CONFIG_ACPI
> > if (!acpi_disabled)
> > return bgx_init_acpi_phy(bgx);
> >#endif
> > return bgx_init_of_phy(bgx);
> >}
> >
> >This adds acpi runtime detection (acpi=no), does not call dt code in
> >case of acpi, and saves the #else for bgx_init_acpi_phy().
> >
>
> I am fine with keeping it in bgx_init_phy(), however we can drop there
> #ifdefs since both of bgx_init_{acpi,of}_phy calls have empty stub for !ACPI
> and !OF case. Like that:
>
> static int bgx_init_phy(struct bgx *bgx)
> {
>
> if (!acpi_disabled)
> return bgx_init_acpi_phy(bgx);
> else
> return bgx_init_of_phy(bgx);
> }
As said, keeping it in #ifdefs makes the empty stub function for !acpi
obsolete, which makes the code smaller and better readable. This style
is common practice in the kernel. Apart from that, the 'else' should
be dropped as it is useless.
-Robert
On 07.08.2015 13:56, Robert Richter wrote:
> On 07.08.15 12:52:41, Tomasz Nowicki wrote:
>> On 07.08.2015 12:43, Robert Richter wrote:
>>> On 07.08.15 10:09:04, Tomasz Nowicki wrote:
>>>> On 07.08.2015 02:33, David Daney wrote:
>>>
>>> ...
>>>
>>>>> +#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,7 +1010,12 @@ static int bgx_init_of_phy(struct bgx *bgx)
>>>>>
>>>>> static int bgx_init_phy(struct bgx *bgx)
>>>>> {
>>>>> - return bgx_init_of_phy(bgx);
>>>>> + int err = bgx_init_of_phy(bgx);
>>>>> +
>>>>> + if (err != -ENODEV)
>>>>> + return err;
>>>>> +
>>>>> + return bgx_init_acpi_phy(bgx);
>>>>> }
>>>>>
>>>>
>>>> If kernel can work with DT and ACPI (both compiled in), it should take only
>>>> one path instead of probing DT and ACPI sequentially. How about:
>>>>
>>>> @@ -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 = acpi_disabled ? bgx_init_of_phy(bgx) : bgx_init_acpi_phy(bgx);
>>>> + if (err)
>>>> + goto err_enable;
>>>>
>>>> bgx_init_hw(bgx);
>>>
>>> I would not pollute bgx_probe() with acpi and dt specifics, and instead
>>> keep bgx_init_phy(). The typical design pattern for this is:
>>>
>>> static int bgx_init_phy(struct bgx *bgx)
>>> {
>>> #ifdef CONFIG_ACPI
>>> if (!acpi_disabled)
>>> return bgx_init_acpi_phy(bgx);
>>> #endif
>>> return bgx_init_of_phy(bgx);
>>> }
>>>
>>> This adds acpi runtime detection (acpi=no), does not call dt code in
>>> case of acpi, and saves the #else for bgx_init_acpi_phy().
>>>
>>
>> I am fine with keeping it in bgx_init_phy(), however we can drop there
>> #ifdefs since both of bgx_init_{acpi,of}_phy calls have empty stub for !ACPI
>> and !OF case. Like that:
>>
>> static int bgx_init_phy(struct bgx *bgx)
>> {
>>
>> if (!acpi_disabled)
>> return bgx_init_acpi_phy(bgx);
>> else
>> return bgx_init_of_phy(bgx);
>> }
>
> As said, keeping it in #ifdefs makes the empty stub function for !acpi
> obsolete, which makes the code smaller and better readable. This style
> is common practice in the kernel. Apart from that, the 'else' should
> be dropped as it is useless.
>
I would't say the code is better readable (bgx_init_of_phy has still two
stubs) but this is just cosmetic, my point was to use run time detection
using acpi_disabled.
Tomasz
On Fri, Aug 07, 2015 at 01:33:10AM +0100, David Daney wrote:
> From: David Daney <[email protected]>
>
> Find out which PHYs belong to which BGX instance in the ACPI way.
>
> Set the MAC address of the device as provided by ACPI tables. This is
> similar to the implementation for devicetree in
> of_get_mac_address(). The table is searched for the device property
> entries "mac-address", "local-mac-address" and "address" in that
> order. The address is provided in a u64 variable and must contain a
> valid 6 bytes-len mac addr.
>
> Based on code from: Narinder Dhillon <[email protected]>
> Tomasz Nowicki <[email protected]>
> Robert Richter <[email protected]>
>
> Signed-off-by: Tomasz Nowicki <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
> Signed-off-by: David Daney <[email protected]>
> ---
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 137 +++++++++++++++++++++-
> 1 file changed, 135 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
> index 615b2af..2056583 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,133 @@ static void bgx_get_qlm_mode(struct bgx *bgx)
> }
> }
>
> +#ifdef CONFIG_ACPI
> +
> +static int bgx_match_phy_id(struct device *dev, void *data)
> +{
> + struct phy_device *phydev = to_phy_device(dev);
> + u32 *phy_id = data;
> +
> + if (phydev->addr == *phy_id)
> + return 1;
> +
> + return 0;
> +}
> +
> +static const char * const addr_propnames[] = {
> + "mac-address",
> + "local-mac-address",
> + "address",
> +};
If these are going to be generally necessary, then we should get them
adopted as standardised _DSD properties (ideally just one of them).
[...]
> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
> + u32 lvl, void *context, void **rv)
> +{
> + struct acpi_reference_args args;
> + const union acpi_object *prop;
> + struct bgx *bgx = context;
> + struct acpi_device *adev;
> + struct device *phy_dev;
> + u32 phy_id;
> +
> + if (acpi_bus_get_device(handle, &adev))
> + goto out;
> +
> + SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
> +
> + acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
> +
> + bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
> +
> + if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
> + goto out;
> +
> + if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
> + goto out;
Likewise for any inter-device properties, so that we can actually handle
them in a generic fashion, and avoid / learn from the mistakes we've
already handled with DT.
Mark.
On Thu, Aug 06, 2015 at 05:33:10PM -0700, David Daney wrote:
> From: David Daney <[email protected]>
>
> Find out which PHYs belong to which BGX instance in the ACPI way.
>
> Set the MAC address of the device as provided by ACPI tables. This is
> similar to the implementation for devicetree in
> of_get_mac_address(). The table is searched for the device property
> entries "mac-address", "local-mac-address" and "address" in that
> order. The address is provided in a u64 variable and must contain a
> valid 6 bytes-len mac addr.
>
> Based on code from: Narinder Dhillon <[email protected]>
> Tomasz Nowicki <[email protected]>
> Robert Richter <[email protected]>
>
> Signed-off-by: Tomasz Nowicki <[email protected]>
> Signed-off-by: Robert Richter <[email protected]>
> Signed-off-by: David Daney <[email protected]>
> ---
> drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 137 +++++++++++++++++++++-
> 1 file changed, 135 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
> index 615b2af..2056583 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,133 @@ static void bgx_get_qlm_mode(struct bgx *bgx)
> }
> }
>
> +#ifdef CONFIG_ACPI
> +
> +static int bgx_match_phy_id(struct device *dev, void *data)
> +{
> + struct phy_device *phydev = to_phy_device(dev);
> + u32 *phy_id = data;
> +
> + if (phydev->addr == *phy_id)
> + return 1;
> +
> + return 0;
> +}
> +
> +static const char * const addr_propnames[] = {
> + "mac-address",
> + "local-mac-address",
> + "address",
> +};
> +
> +static int acpi_get_mac_address(struct acpi_device *adev, u8 *dst)
> +{
> + const union acpi_object *prop;
> + u64 mac_val;
> + u8 mac[ETH_ALEN];
> + int i, j;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(addr_propnames); i++) {
> + ret = acpi_dev_get_property(adev, addr_propnames[i],
> + ACPI_TYPE_INTEGER, &prop);
Shouldn't this be trying to use device_property_read_* API and making
the DT/ACPI path the same where possible?
Graeme
> + if (ret)
> + continue;
> +
> + mac_val = prop->integer.value;
> +
> + if (mac_val & (~0ULL << 48))
> + continue; /* more than 6 bytes */
> +
> + for (j = 0; j < ARRAY_SIZE(mac); j++)
> + mac[j] = (u8)(mac_val >> (8 * j));
> + if (!is_valid_ether_addr(mac))
> + continue;
> +
> + memcpy(dst, mac, ETH_ALEN);
> +
> + return 0;
> + }
> +
> + return ret ? ret : -EINVAL;
> +}
> +
> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
> + u32 lvl, void *context, void **rv)
> +{
> + struct acpi_reference_args args;
> + const union acpi_object *prop;
> + struct bgx *bgx = context;
> + struct acpi_device *adev;
> + struct device *phy_dev;
> + u32 phy_id;
> +
> + if (acpi_bus_get_device(handle, &adev))
> + goto out;
> +
> + SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
> +
> + acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
> +
> + bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
> +
> + if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
> + goto out;
> +
> + if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
> + goto out;
> +
> + phy_id = prop->integer.value;
> +
> + phy_dev = bus_find_device(&mdio_bus_type, NULL, (void *)&phy_id,
> + bgx_match_phy_id);
> + if (!phy_dev)
> + goto out;
> +
> + bgx->lmac[bgx->lmac_count].phydev = to_phy_device(phy_dev);
> +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,7 +1010,12 @@ static int bgx_init_of_phy(struct bgx *bgx)
>
> static int bgx_init_phy(struct bgx *bgx)
> {
> - return bgx_init_of_phy(bgx);
> + int err = bgx_init_of_phy(bgx);
> +
> + if (err != -ENODEV)
> + return err;
> +
> + return bgx_init_acpi_phy(bgx);
> }
>
> static int bgx_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/07/2015 05:42 AM, Tomasz Nowicki wrote:
> On 07.08.2015 13:56, Robert Richter wrote:
>> On 07.08.15 12:52:41, Tomasz Nowicki wrote:
[...]
>>>>
>>>> I would not pollute bgx_probe() with acpi and dt specifics, and instead
>>>> keep bgx_init_phy(). The typical design pattern for this is:
>>>>
>>>> static int bgx_init_phy(struct bgx *bgx)
>>>> {
>>>> #ifdef CONFIG_ACPI
>>>> if (!acpi_disabled)
>>>> return bgx_init_acpi_phy(bgx);
>>>> #endif
>>>> return bgx_init_of_phy(bgx);
>>>> }
>>>>
>>>> This adds acpi runtime detection (acpi=no), does not call dt code in
>>>> case of acpi, and saves the #else for bgx_init_acpi_phy().
>>>>
>>>
>>> I am fine with keeping it in bgx_init_phy(), however we can drop there
>>> #ifdefs since both of bgx_init_{acpi,of}_phy calls have empty stub
>>> for !ACPI
>>> and !OF case. Like that:
>>>
>>> static int bgx_init_phy(struct bgx *bgx)
>>> {
>>>
>>> if (!acpi_disabled)
>>> return bgx_init_acpi_phy(bgx);
>>> else
>>> return bgx_init_of_phy(bgx);
>>> }
>>
>> As said, keeping it in #ifdefs makes the empty stub function for !acpi
>> obsolete, which makes the code smaller and better readable. This style
>> is common practice in the kernel. Apart from that, the 'else' should
>> be dropped as it is useless.
>>
>
> I would't say the code is better readable (bgx_init_of_phy has still two
> stubs) but this is just cosmetic, my point was to use run time detection
> using acpi_disabled.
>
Thanks Tomasz and Robert for the input. Because of this, it seems that
another version of the patch will be necessary. In the interests of
code clarity and asthetics, we will go with the code without the
#ifdefs, and rely on the compiler to optimize away any dead code.
David Daney
> Tomasz
On 08/07/2015 07:01 AM, Mark Rutland wrote:
> On Fri, Aug 07, 2015 at 01:33:10AM +0100, David Daney wrote:
>> From: David Daney <[email protected]>
>>
>> Find out which PHYs belong to which BGX instance in the ACPI way.
>>
>> Set the MAC address of the device as provided by ACPI tables. This is
>> similar to the implementation for devicetree in
>> of_get_mac_address(). The table is searched for the device property
>> entries "mac-address", "local-mac-address" and "address" in that
>> order. The address is provided in a u64 variable and must contain a
>> valid 6 bytes-len mac addr.
>>
>> Based on code from: Narinder Dhillon <[email protected]>
>> Tomasz Nowicki <[email protected]>
>> Robert Richter <[email protected]>
>>
>> Signed-off-by: Tomasz Nowicki <[email protected]>
>> Signed-off-by: Robert Richter <[email protected]>
>> Signed-off-by: David Daney <[email protected]>
>> ---
>> drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 137 +++++++++++++++++++++-
>> 1 file changed, 135 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
>> index 615b2af..2056583 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,133 @@ static void bgx_get_qlm_mode(struct bgx *bgx)
>> }
>> }
>>
>> +#ifdef CONFIG_ACPI
>> +
>> +static int bgx_match_phy_id(struct device *dev, void *data)
>> +{
>> + struct phy_device *phydev = to_phy_device(dev);
>> + u32 *phy_id = data;
>> +
>> + if (phydev->addr == *phy_id)
>> + return 1;
>> +
>> + return 0;
>> +}
>> +
>> +static const char * const addr_propnames[] = {
>> + "mac-address",
>> + "local-mac-address",
>> + "address",
>> +};
>
> If these are going to be generally necessary, then we should get them
> adopted as standardised _DSD properties (ideally just one of them).
As far as I can tell, and please correct me if I am wrong, ACPI-6.0
doesn't contemplate MAC addresses.
Today we are using "mac-address", which is an Integer containing the MAC
address in its lowest order 48 bits in Little-Endian byte order.
The hardware and ACPI tables are here today, and we would like to
support it. If some future ACPI specification specifies a standard way
to do this, we will probably adapt the code to do this in a standard manner.
>
> [...]
>
>> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
>> + u32 lvl, void *context, void **rv)
>> +{
>> + struct acpi_reference_args args;
>> + const union acpi_object *prop;
>> + struct bgx *bgx = context;
>> + struct acpi_device *adev;
>> + struct device *phy_dev;
>> + u32 phy_id;
>> +
>> + if (acpi_bus_get_device(handle, &adev))
>> + goto out;
>> +
>> + SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
>> +
>> + acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
>> +
>> + bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
>> +
>> + if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
>> + goto out;
>> +
>> + if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
>> + goto out;
>
> Likewise for any inter-device properties, so that we can actually handle
> them in a generic fashion, and avoid / learn from the mistakes we've
> already handled with DT.
This is the fallacy of the ACPI is superior to DT argument. The
specification of PHY topology and MAC addresses is well standardized in
DT, there is no question about what the proper way to specify it is.
Under ACPI, it is the Wild West, there is no specification, so each
system design is forced to invent something, and everybody comes up with
an incompatible implementation.
That said, this is all specific to our BGX device, so anything we do
doesn't break other devices.
David Daney
[Correcting the devicetree list address, which I typo'd in my original
reply]
> >> +static const char * const addr_propnames[] = {
> >> + "mac-address",
> >> + "local-mac-address",
> >> + "address",
> >> +};
> >
> > If these are going to be generally necessary, then we should get them
> > adopted as standardised _DSD properties (ideally just one of them).
>
> As far as I can tell, and please correct me if I am wrong, ACPI-6.0
> doesn't contemplate MAC addresses.
>
> Today we are using "mac-address", which is an Integer containing the MAC
> address in its lowest order 48 bits in Little-Endian byte order.
>
> The hardware and ACPI tables are here today, and we would like to
> support it. If some future ACPI specification specifies a standard way
> to do this, we will probably adapt the code to do this in a standard manner.
>
>
> >
> > [...]
> >
> >> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
> >> + u32 lvl, void *context, void **rv)
> >> +{
> >> + struct acpi_reference_args args;
> >> + const union acpi_object *prop;
> >> + struct bgx *bgx = context;
> >> + struct acpi_device *adev;
> >> + struct device *phy_dev;
> >> + u32 phy_id;
> >> +
> >> + if (acpi_bus_get_device(handle, &adev))
> >> + goto out;
> >> +
> >> + SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
> >> +
> >> + acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
> >> +
> >> + bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
> >> +
> >> + if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
> >> + goto out;
> >> +
> >> + if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
> >> + goto out;
> >
> > Likewise for any inter-device properties, so that we can actually handle
> > them in a generic fashion, and avoid / learn from the mistakes we've
> > already handled with DT.
>
> This is the fallacy of the ACPI is superior to DT argument. The
> specification of PHY topology and MAC addresses is well standardized in
> DT, there is no question about what the proper way to specify it is.
> Under ACPI, it is the Wild West, there is no specification, so each
> system design is forced to invent something, and everybody comes up with
> an incompatible implementation.
Indeed.
If ACPI is going to handle it, it should handle it properly. I really
don't see the point in bodging properties together in a less standard
manner than DT, especially for inter-device relationships.
Doing so is painful for _everyone_, and it's extremely unlikely that
other ACPI-aware OSs will actually support these custom descriptions,
making this Linux-specific, and breaking the rationale for using ACPI in
the first place -- a standard that says "just do non-standard stuff" is
not a usable standard.
For intra-device properties, we should standardise what we can, but
vendor-specific stuff is ok -- this can be self-contained within a
driver.
For inter-device relationships ACPI _must_ gain a better model of
componentised devices. It's simply unworkable otherwise, and as you
point out it's fallacious to say that because ACPI is being used that
something is magically industry standard, portable, etc.
This is not your problem in particular; the entire handling of _DSD so
far is a joke IMO.
Thanks,
Mark.
[Correcting the devicetree list address, which I typo'd in my original
reply]
[resending to _really_ correct the address, apologies for the spam]
> >> +static const char * const addr_propnames[] = {
> >> + "mac-address",
> >> + "local-mac-address",
> >> + "address",
> >> +};
> >
> > If these are going to be generally necessary, then we should get them
> > adopted as standardised _DSD properties (ideally just one of them).
>
> As far as I can tell, and please correct me if I am wrong, ACPI-6.0
> doesn't contemplate MAC addresses.
>
> Today we are using "mac-address", which is an Integer containing the MAC
> address in its lowest order 48 bits in Little-Endian byte order.
>
> The hardware and ACPI tables are here today, and we would like to
> support it. If some future ACPI specification specifies a standard way
> to do this, we will probably adapt the code to do this in a standard manner.
>
>
> >
> > [...]
> >
> >> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
> >> + u32 lvl, void *context, void **rv)
> >> +{
> >> + struct acpi_reference_args args;
> >> + const union acpi_object *prop;
> >> + struct bgx *bgx = context;
> >> + struct acpi_device *adev;
> >> + struct device *phy_dev;
> >> + u32 phy_id;
> >> +
> >> + if (acpi_bus_get_device(handle, &adev))
> >> + goto out;
> >> +
> >> + SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
> >> +
> >> + acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
> >> +
> >> + bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
> >> +
> >> + if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
> >> + goto out;
> >> +
> >> + if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
> >> + goto out;
> >
> > Likewise for any inter-device properties, so that we can actually handle
> > them in a generic fashion, and avoid / learn from the mistakes we've
> > already handled with DT.
>
> This is the fallacy of the ACPI is superior to DT argument. The
> specification of PHY topology and MAC addresses is well standardized in
> DT, there is no question about what the proper way to specify it is.
> Under ACPI, it is the Wild West, there is no specification, so each
> system design is forced to invent something, and everybody comes up with
> an incompatible implementation.
Indeed.
If ACPI is going to handle it, it should handle it properly. I really
don't see the point in bodging properties together in a less standard
manner than DT, especially for inter-device relationships.
Doing so is painful for _everyone_, and it's extremely unlikely that
other ACPI-aware OSs will actually support these custom descriptions,
making this Linux-specific, and breaking the rationale for using ACPI in
the first place -- a standard that says "just do non-standard stuff" is
not a usable standard.
For intra-device properties, we should standardise what we can, but
vendor-specific stuff is ok -- this can be self-contained within a
driver.
For inter-device relationships ACPI _must_ gain a better model of
componentised devices. It's simply unworkable otherwise, and as you
point out it's fallacious to say that because ACPI is being used that
something is magically industry standard, portable, etc.
This is not your problem in particular; the entire handling of _DSD so
far is a joke IMO.
Thanks,
Mark.
On 08/07/2015 07:54 AM, Graeme Gregory wrote:
> On Thu, Aug 06, 2015 at 05:33:10PM -0700, David Daney wrote:
>> From: David Daney <[email protected]>
>>
>> Find out which PHYs belong to which BGX instance in the ACPI way.
>>
>> Set the MAC address of the device as provided by ACPI tables. This is
>> similar to the implementation for devicetree in
>> of_get_mac_address(). The table is searched for the device property
>> entries "mac-address", "local-mac-address" and "address" in that
>> order. The address is provided in a u64 variable and must contain a
>> valid 6 bytes-len mac addr.
>>
>> Based on code from: Narinder Dhillon <[email protected]>
>> Tomasz Nowicki <[email protected]>
>> Robert Richter <[email protected]>
>>
>> Signed-off-by: Tomasz Nowicki <[email protected]>
>> Signed-off-by: Robert Richter <[email protected]>
>> Signed-off-by: David Daney <[email protected]>
>> ---
>> drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 137 +++++++++++++++++++++-
>> 1 file changed, 135 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
>> index 615b2af..2056583 100644
>> --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
>> +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
[...]
>> +
>> +static int acpi_get_mac_address(struct acpi_device *adev, u8 *dst)
>> +{
>> + const union acpi_object *prop;
>> + u64 mac_val;
>> + u8 mac[ETH_ALEN];
>> + int i, j;
>> + int ret;
>> +
>> + for (i = 0; i < ARRAY_SIZE(addr_propnames); i++) {
>> + ret = acpi_dev_get_property(adev, addr_propnames[i],
>> + ACPI_TYPE_INTEGER, &prop);
>
> Shouldn't this be trying to use device_property_read_* API and making
> the DT/ACPI path the same where possible?
>
Ideally, something like you suggest would be possible. However, there
are a couple of problems trying to do it in the kernel as it exists today:
1) There is no 'struct device *' here, so device_property_read_* is not
applicable.
2) There is no standard ACPI binding for MAC addresses, so it is
impossible to create a hypothetical fw_get_mac_address(), which would be
analogous to of_get_mac_address().
Other e-mail threads have suggested that the path to an elegant solution
is to inter-mix a bunch of calls to acpi_dev_get_property*() and
fwnode_property_read*() as to use these more generic
fwnode_property_read*() functions whereever possible. I rejected this
approach as it seems cleaner to me to consistently use a single set of APIs.
Thanks,
David Daney
> Graeme
>
>> + if (ret)
>> + continue;
>> +
>> + mac_val = prop->integer.value;
>> +
>> + if (mac_val & (~0ULL << 48))
>> + continue; /* more than 6 bytes */
>> +
>> + for (j = 0; j < ARRAY_SIZE(mac); j++)
>> + mac[j] = (u8)(mac_val >> (8 * j));
>> + if (!is_valid_ether_addr(mac))
>> + continue;
>> +
>> + memcpy(dst, mac, ETH_ALEN);
>> +
>> + return 0;
>> + }
>> +
>> + return ret ? ret : -EINVAL;
>> +}
>> +
>> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
>> + u32 lvl, void *context, void **rv)
>> +{
>> + struct acpi_reference_args args;
>> + const union acpi_object *prop;
>> + struct bgx *bgx = context;
>> + struct acpi_device *adev;
>> + struct device *phy_dev;
>> + u32 phy_id;
>> +
>> + if (acpi_bus_get_device(handle, &adev))
>> + goto out;
>> +
>> + SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
>> +
>> + acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
>> +
>> + bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
>> +
>> + if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
>> + goto out;
>> +
>> + if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
>> + goto out;
>> +
>> + phy_id = prop->integer.value;
>> +
>> + phy_dev = bus_find_device(&mdio_bus_type, NULL, (void *)&phy_id,
>> + bgx_match_phy_id);
>> + if (!phy_dev)
>> + goto out;
>> +
>> + bgx->lmac[bgx->lmac_count].phydev = to_phy_device(phy_dev);
>> +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,7 +1010,12 @@ static int bgx_init_of_phy(struct bgx *bgx)
>>
>> static int bgx_init_phy(struct bgx *bgx)
>> {
>> - return bgx_init_of_phy(bgx);
>> + int err = bgx_init_of_phy(bgx);
>> +
>> + if (err != -ENODEV)
>> + return err;
>> +
>> + return bgx_init_acpi_phy(bgx);
>> }
>>
>> static int bgx_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark,
On Fri, Aug 7, 2015 at 7:51 PM, Mark Rutland <[email protected]> wrote:
> [Correcting the devicetree list address, which I typo'd in my original
> reply]
>
>> >> +static const char * const addr_propnames[] = {
>> >> + "mac-address",
>> >> + "local-mac-address",
>> >> + "address",
>> >> +};
>> >
>> > If these are going to be generally necessary, then we should get them
>> > adopted as standardised _DSD properties (ideally just one of them).
>>
>> As far as I can tell, and please correct me if I am wrong, ACPI-6.0
>> doesn't contemplate MAC addresses.
>>
>> Today we are using "mac-address", which is an Integer containing the MAC
>> address in its lowest order 48 bits in Little-Endian byte order.
>>
>> The hardware and ACPI tables are here today, and we would like to
>> support it. If some future ACPI specification specifies a standard way
>> to do this, we will probably adapt the code to do this in a standard manner.
>>
>>
>> >
>> > [...]
>> >
>> >> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
>> >> + u32 lvl, void *context, void **rv)
>> >> +{
>> >> + struct acpi_reference_args args;
>> >> + const union acpi_object *prop;
>> >> + struct bgx *bgx = context;
>> >> + struct acpi_device *adev;
>> >> + struct device *phy_dev;
>> >> + u32 phy_id;
>> >> +
>> >> + if (acpi_bus_get_device(handle, &adev))
>> >> + goto out;
>> >> +
>> >> + SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
>> >> +
>> >> + acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
>> >> +
>> >> + bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
>> >> +
>> >> + if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
>> >> + goto out;
>> >> +
>> >> + if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
>> >> + goto out;
>> >
>> > Likewise for any inter-device properties, so that we can actually handle
>> > them in a generic fashion, and avoid / learn from the mistakes we've
>> > already handled with DT.
>>
>> This is the fallacy of the ACPI is superior to DT argument. The
>> specification of PHY topology and MAC addresses is well standardized in
>> DT, there is no question about what the proper way to specify it is.
>> Under ACPI, it is the Wild West, there is no specification, so each
>> system design is forced to invent something, and everybody comes up with
>> an incompatible implementation.
>
> Indeed.
>
> If ACPI is going to handle it, it should handle it properly. I really
> don't see the point in bodging properties together in a less standard
> manner than DT, especially for inter-device relationships.
>
> Doing so is painful for _everyone_, and it's extremely unlikely that
> other ACPI-aware OSs will actually support these custom descriptions,
> making this Linux-specific, and breaking the rationale for using ACPI in
> the first place -- a standard that says "just do non-standard stuff" is
> not a usable standard.
>
> For intra-device properties, we should standardise what we can, but
> vendor-specific stuff is ok -- this can be self-contained within a
> driver.
>
> For inter-device relationships ACPI _must_ gain a better model of
> componentised devices. It's simply unworkable otherwise, and as you
> point out it's fallacious to say that because ACPI is being used that
> something is magically industry standard, portable, etc.
>
> This is not your problem in particular; the entire handling of _DSD so
> far is a joke IMO.
It is actually useful to people as far as I can say.
Also, if somebody is going to use properties with ACPI, why whould
they use a different set of properties with DT?
Wouldn't it be more reasonable to use the same set in both cases?
Thanks,
Rafael
On 08/07/2015 05:05 PM, Rafael J. Wysocki wrote:
> Hi Mark,
>
> On Fri, Aug 7, 2015 at 7:51 PM, Mark Rutland <[email protected]> wrote:
>> [Correcting the devicetree list address, which I typo'd in my original
>> reply]
>>
>>>>> +static const char * const addr_propnames[] = {
>>>>> + "mac-address",
>>>>> + "local-mac-address",
>>>>> + "address",
>>>>> +};
>>>>
>>>> If these are going to be generally necessary, then we should get them
>>>> adopted as standardised _DSD properties (ideally just one of them).
>>>
>>> As far as I can tell, and please correct me if I am wrong, ACPI-6.0
>>> doesn't contemplate MAC addresses.
>>>
>>> Today we are using "mac-address", which is an Integer containing the MAC
>>> address in its lowest order 48 bits in Little-Endian byte order.
>>>
>>> The hardware and ACPI tables are here today, and we would like to
>>> support it. If some future ACPI specification specifies a standard way
>>> to do this, we will probably adapt the code to do this in a standard manner.
>>>
>>>
>>>>
>>>> [...]
>>>>
>>>>> +static acpi_status bgx_acpi_register_phy(acpi_handle handle,
>>>>> + u32 lvl, void *context, void **rv)
>>>>> +{
>>>>> + struct acpi_reference_args args;
>>>>> + const union acpi_object *prop;
>>>>> + struct bgx *bgx = context;
>>>>> + struct acpi_device *adev;
>>>>> + struct device *phy_dev;
>>>>> + u32 phy_id;
>>>>> +
>>>>> + if (acpi_bus_get_device(handle, &adev))
>>>>> + goto out;
>>>>> +
>>>>> + SET_NETDEV_DEV(&bgx->lmac[bgx->lmac_count].netdev, &bgx->pdev->dev);
>>>>> +
>>>>> + acpi_get_mac_address(adev, bgx->lmac[bgx->lmac_count].mac);
>>>>> +
>>>>> + bgx->lmac[bgx->lmac_count].lmacid = bgx->lmac_count;
>>>>> +
>>>>> + if (acpi_dev_get_property_reference(adev, "phy-handle", 0, &args))
>>>>> + goto out;
>>>>> +
>>>>> + if (acpi_dev_get_property(args.adev, "phy-channel", ACPI_TYPE_INTEGER, &prop))
>>>>> + goto out;
>>>>
>>>> Likewise for any inter-device properties, so that we can actually handle
>>>> them in a generic fashion, and avoid / learn from the mistakes we've
>>>> already handled with DT.
>>>
>>> This is the fallacy of the ACPI is superior to DT argument. The
>>> specification of PHY topology and MAC addresses is well standardized in
>>> DT, there is no question about what the proper way to specify it is.
>>> Under ACPI, it is the Wild West, there is no specification, so each
>>> system design is forced to invent something, and everybody comes up with
>>> an incompatible implementation.
>>
>> Indeed.
>>
>> If ACPI is going to handle it, it should handle it properly. I really
>> don't see the point in bodging properties together in a less standard
>> manner than DT, especially for inter-device relationships.
>>
>> Doing so is painful for _everyone_, and it's extremely unlikely that
>> other ACPI-aware OSs will actually support these custom descriptions,
>> making this Linux-specific, and breaking the rationale for using ACPI in
>> the first place -- a standard that says "just do non-standard stuff" is
>> not a usable standard.
>>
>> For intra-device properties, we should standardise what we can, but
>> vendor-specific stuff is ok -- this can be self-contained within a
>> driver.
>>
>> For inter-device relationships ACPI _must_ gain a better model of
>> componentised devices. It's simply unworkable otherwise, and as you
>> point out it's fallacious to say that because ACPI is being used that
>> something is magically industry standard, portable, etc.
>>
>> This is not your problem in particular; the entire handling of _DSD so
>> far is a joke IMO.
>
> It is actually useful to people as far as I can say.
>
> Also, if somebody is going to use properties with ACPI, why whould
> they use a different set of properties with DT?
>
> Wouldn't it be more reasonable to use the same set in both cases?
Yes, but there is still quite a bit of leeway to screw things up.
FWIW: http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf
This actually seems to have been adopted by the UEFI people as a
"Standard", I am not sure where a record of this is kept though.
So, we are changing our firmware to use this standard (which is quite
similar the the DT with respect to MAC addresses).
Thanks,
David Daney
Hi David,
On Sat, Aug 8, 2015 at 2:11 AM, David Daney <[email protected]> wrote:
> On 08/07/2015 05:05 PM, Rafael J. Wysocki wrote:
[cut]
>>
>> It is actually useful to people as far as I can say.
>>
>> Also, if somebody is going to use properties with ACPI, why whould
>> they use a different set of properties with DT?
>>
>> Wouldn't it be more reasonable to use the same set in both cases?
>
>
> Yes, but there is still quite a bit of leeway to screw things up.
That I have to agree with, unfortunately.
On the other hand, this is a fairly new concept and we need to gain
some experience with it to be able to come up with best practices and
so on. Cases like yours are really helpful here.
> FWIW: http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf
>
> This actually seems to have been adopted by the UEFI people as a
> "Standard", I am not sure where a record of this is kept though.
Work on this is in progress, but far from completion. Essentially,
what's needed is more pressure from vendors who want to use properties
in their firmware.
> So, we are changing our firmware to use this standard (which is quite
> similar the the DT with respect to MAC addresses).
Cool. :-)
Thanks,
Rafael
Hi David,
On Fri, Aug 7, 2015 at 8:14 PM, David Daney <[email protected]> wrote:
> On 08/07/2015 07:54 AM, Graeme Gregory wrote:
>>
>> On Thu, Aug 06, 2015 at 05:33:10PM -0700, David Daney wrote:
>>>
>>> From: David Daney <[email protected]>
>>>
>>> Find out which PHYs belong to which BGX instance in the ACPI way.
>>>
>>> Set the MAC address of the device as provided by ACPI tables. This is
>>> similar to the implementation for devicetree in
>>> of_get_mac_address(). The table is searched for the device property
>>> entries "mac-address", "local-mac-address" and "address" in that
>>> order. The address is provided in a u64 variable and must contain a
>>> valid 6 bytes-len mac addr.
>>>
>>> Based on code from: Narinder Dhillon <[email protected]>
>>> Tomasz Nowicki <[email protected]>
>>> Robert Richter <[email protected]>
>>>
>>> Signed-off-by: Tomasz Nowicki <[email protected]>
>>> Signed-off-by: Robert Richter <[email protected]>
>>> Signed-off-by: David Daney <[email protected]>
>>> ---
>>> drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 137
>>> +++++++++++++++++++++-
>>> 1 file changed, 135 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
>>> b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
>>> index 615b2af..2056583 100644
>>> --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
>>> +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
>
> [...]
>>>
>>> +
>>> +static int acpi_get_mac_address(struct acpi_device *adev, u8 *dst)
>>> +{
>>> + const union acpi_object *prop;
>>> + u64 mac_val;
>>> + u8 mac[ETH_ALEN];
>>> + int i, j;
>>> + int ret;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(addr_propnames); i++) {
>>> + ret = acpi_dev_get_property(adev, addr_propnames[i],
>>> + ACPI_TYPE_INTEGER, &prop);
>>
>>
>> Shouldn't this be trying to use device_property_read_* API and making
>> the DT/ACPI path the same where possible?
>>
>
> Ideally, something like you suggest would be possible. However, there are a
> couple of problems trying to do it in the kernel as it exists today:
>
> 1) There is no 'struct device *' here, so device_property_read_* is not
> applicable.
>
> 2) There is no standard ACPI binding for MAC addresses, so it is impossible
> to create a hypothetical fw_get_mac_address(), which would be analogous to
> of_get_mac_address().
>
> Other e-mail threads have suggested that the path to an elegant solution is
> to inter-mix a bunch of calls to acpi_dev_get_property*() and
> fwnode_property_read*() as to use these more generic fwnode_property_read*()
> functions whereever possible. I rejected this approach as it seems cleaner
> to me to consistently use a single set of APIs.
Actually, that wasn't my intention.
I wanted to say that once you'd got an ACPI device pointer (struct
acpi_device), you could easly convert it to a struct fwnode_handle
pointer and operate that going forward when accessing properties.
That at least would help with the properties that do not differ
between DT and ACPI.
Thanks,
Rafael
On Friday 07 August 2015 12:43:20 Robert Richter wrote:
>
> I would not pollute bgx_probe() with acpi and dt specifics, and instead
> keep bgx_init_phy(). The typical design pattern for this is:
>
> static int bgx_init_phy(struct bgx *bgx)
> {
> #ifdef CONFIG_ACPI
> if (!acpi_disabled)
> return bgx_init_acpi_phy(bgx);
> #endif
> return bgx_init_of_phy(bgx);
> }
>
> This adds acpi runtime detection (acpi=no), does not call dt code in
> case of acpi, and saves the #else for bgx_init_acpi_phy().
>
What you should really do is to use the same function for both,
using the generic device properties API. If that is not possible,
explain in a comment why not.
Aside from that, if you do have to use compile-time conditionals,
use 'if (IS_ENABLED(CONFIG_ACPI) && !acpi_disabled)' instead of
#ifdef, for readability. The compiler will produce the same binary,
but also give helpful warnings about incorrect code that you don't
get with #ifdef.
Arnd