| Message ID | 20220104072658.69756-17-marcan@marcan.st (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcmfmac: Support Apple T2 and M1 platforms | expand |
On 1/4/2022 8:26 AM, Hector Martin wrote: > On DT platforms, the module-instance and antenna-sku-info properties > are passed in the DT. On ACPI platforms, module-instance is passed via > the analogous Apple device property mechanism, while the antenna SKU > info is instead obtained via an ACPI method that grabs it from > non-volatile storage. > > Add support for this, to allow proper firmware selection on Apple > platforms. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/Makefile | 2 + > .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ > .../broadcom/brcm80211/brcmfmac/common.c | 1 + > .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ > 4 files changed, 59 insertions(+) > create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile > index 13c13504a6e8..19009eb9db93 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile > @@ -47,3 +47,5 @@ brcmfmac-$(CONFIG_OF) += \ > of.o > brcmfmac-$(CONFIG_DMI) += \ > dmi.o > +brcmfmac-$(CONFIG_ACPI) += \ > + acpi.o > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > new file mode 100644 > index 000000000000..2b1a4448b291 > --- /dev/null > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > @@ -0,0 +1,47 @@ > +// SPDX-License-Identifier: ISC > +/* > + * Copyright The Asahi Linux Contributors > + */ Common format for copyright statement (in this folder) seems to be: Copyright (c) <YEAR> <COPYRIGHT_HOLDER> Regards, Arend
On 2022/01/04 19:21, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> On DT platforms, the module-instance and antenna-sku-info properties >> are passed in the DT. On ACPI platforms, module-instance is passed via >> the analogous Apple device property mechanism, while the antenna SKU >> info is instead obtained via an ACPI method that grabs it from >> non-volatile storage. >> >> Add support for this, to allow proper firmware selection on Apple >> platforms. >> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/Makefile | 2 + >> .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ >> .../broadcom/brcm80211/brcmfmac/common.c | 1 + >> .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ >> 4 files changed, 59 insertions(+) >> create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >> index 13c13504a6e8..19009eb9db93 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >> @@ -47,3 +47,5 @@ brcmfmac-$(CONFIG_OF) += \ >> of.o >> brcmfmac-$(CONFIG_DMI) += \ >> dmi.o >> +brcmfmac-$(CONFIG_ACPI) += \ >> + acpi.o >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> new file mode 100644 >> index 000000000000..2b1a4448b291 >> --- /dev/null >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> @@ -0,0 +1,47 @@ >> +// SPDX-License-Identifier: ISC >> +/* >> + * Copyright The Asahi Linux Contributors >> + */ > > Common format for copyright statement (in this folder) seems to be: > > Copyright (c) <YEAR> <COPYRIGHT_HOLDER> > > Regards, > Arend I get this every time I submit a patch to a new subsystem :-) This is based on this best practice: https://www.linuxfoundation.org/blog/copyright-notices-in-open-source-software-projects/ Basically, the year format quickly becomes outdated and is rather useless, and listing specific names also ends up missing every subsequent contributor, so more general copyright statements work better for this kind of use case. In the end we all know git history is the proper record of copyright status. I don't have a super strong opinion here, but we've been trying to standardize on this format for contributions coming from our subproject, and it feels more useful than a random contributor's name with a date 5 years in the past :)
On 1/4/2022 8:26 AM, Hector Martin wrote: > On DT platforms, the module-instance and antenna-sku-info properties > are passed in the DT. On ACPI platforms, module-instance is passed via > the analogous Apple device property mechanism, while the antenna SKU > info is instead obtained via an ACPI method that grabs it from > non-volatile storage. > > Add support for this, to allow proper firmware selection on Apple > platforms. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/Makefile | 2 + > .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ > .../broadcom/brcm80211/brcmfmac/common.c | 1 + > .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ > 4 files changed, 59 insertions(+) > create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c [...] > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > new file mode 100644 > index 000000000000..2b1a4448b291 > --- /dev/null > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > @@ -0,0 +1,47 @@ > +// SPDX-License-Identifier: ISC > +/* > + * Copyright The Asahi Linux Contributors > + */ > + > +#include <linux/acpi.h> > +#include "debug.h" > +#include "core.h" > +#include "common.h" > + > +void brcmf_acpi_probe(struct device *dev, enum brcmf_bus_type bus_type, > + struct brcmf_mp_device *settings) > +{ > + acpi_status status; > + const union acpi_object *o; > + struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL}; > + struct acpi_device *adev = ACPI_COMPANION(dev); > + > + if (!adev) > + return; > + > + if (!ACPI_FAILURE(acpi_dev_get_property(adev, "module-instance", > + ACPI_TYPE_STRING, &o))) { > + brcmf_dbg(INFO, "ACPI module-instance=%s\n", o->string.pointer); > + settings->board_type = devm_kasprintf(dev, GFP_KERNEL, > + "apple,%s", > + o->string.pointer); > + } else { > + brcmf_dbg(INFO, "No ACPI module-instance\n"); Do you need to obtain the antenna-sku when there is no module-instance? > + } > + > + status = acpi_evaluate_object(adev->handle, "RWCV", NULL, &buf); Can you clarify what the above does? What does the "RWCV" mean? > + o = buf.pointer; > + if (!ACPI_FAILURE(status) && o && o->type == ACPI_TYPE_BUFFER && > + o->buffer.length >= 2) { > + char *antenna_sku = devm_kzalloc(dev, 3, GFP_KERNEL); > + > + memcpy(antenna_sku, o->buffer.pointer, 2); > + brcmf_dbg(INFO, "ACPI RWCV data=%*phN antenna-sku=%s\n", > + (int)o->buffer.length, o->buffer.pointer, > + antenna_sku); > + > + settings->antenna_sku = antenna_sku; > + } else { > + brcmf_dbg(INFO, "No ACPI antenna-sku\n"); > + } > +}
On 2022/01/10 18:11, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> On DT platforms, the module-instance and antenna-sku-info properties >> are passed in the DT. On ACPI platforms, module-instance is passed via >> the analogous Apple device property mechanism, while the antenna SKU >> info is instead obtained via an ACPI method that grabs it from >> non-volatile storage. >> >> Add support for this, to allow proper firmware selection on Apple >> platforms. >> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/Makefile | 2 + >> .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ >> .../broadcom/brcm80211/brcmfmac/common.c | 1 + >> .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ >> 4 files changed, 59 insertions(+) >> create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > > [...] > >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> new file mode 100644 >> index 000000000000..2b1a4448b291 >> --- /dev/null >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> @@ -0,0 +1,47 @@ >> +// SPDX-License-Identifier: ISC >> +/* >> + * Copyright The Asahi Linux Contributors >> + */ >> + >> +#include <linux/acpi.h> >> +#include "debug.h" >> +#include "core.h" >> +#include "common.h" >> + >> +void brcmf_acpi_probe(struct device *dev, enum brcmf_bus_type bus_type, >> + struct brcmf_mp_device *settings) >> +{ >> + acpi_status status; >> + const union acpi_object *o; >> + struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL}; >> + struct acpi_device *adev = ACPI_COMPANION(dev); >> + >> + if (!adev) >> + return; >> + >> + if (!ACPI_FAILURE(acpi_dev_get_property(adev, "module-instance", >> + ACPI_TYPE_STRING, &o))) { >> + brcmf_dbg(INFO, "ACPI module-instance=%s\n", o->string.pointer); >> + settings->board_type = devm_kasprintf(dev, GFP_KERNEL, >> + "apple,%s", >> + o->string.pointer); >> + } else { >> + brcmf_dbg(INFO, "No ACPI module-instance\n"); > > Do you need to obtain the antenna-sku when there is no module-instance? In principle I don't think any machines would have antenna-sku and no module-instance, though the firmware selection will still work without it (it'll just end up using the DMI machine name instead). > >> + } >> + >> + status = acpi_evaluate_object(adev->handle, "RWCV", NULL, &buf); > > Can you clarify what the above does? What does the "RWCV" mean? No idea what it *means* :-) What it is, though, is the ACPI method name to get the antenna-sku.
On 1/10/2022 12:07 PM, Hector Martin wrote: > On 2022/01/10 18:11, Arend van Spriel wrote: >> On 1/4/2022 8:26 AM, Hector Martin wrote: >>> On DT platforms, the module-instance and antenna-sku-info properties >>> are passed in the DT. On ACPI platforms, module-instance is passed via >>> the analogous Apple device property mechanism, while the antenna SKU >>> info is instead obtained via an ACPI method that grabs it from >>> non-volatile storage. >>> >>> Add support for this, to allow proper firmware selection on Apple >>> platforms. >>> >>> Signed-off-by: Hector Martin <marcan@marcan.st> >>> --- >>> .../broadcom/brcm80211/brcmfmac/Makefile | 2 + >>> .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ >>> .../broadcom/brcm80211/brcmfmac/common.c | 1 + >>> .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ >>> 4 files changed, 59 insertions(+) >>> create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> >> [...] >> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >>> new file mode 100644 >>> index 000000000000..2b1a4448b291 >>> --- /dev/null >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >>> @@ -0,0 +1,47 @@ >>> +// SPDX-License-Identifier: ISC >>> +/* >>> + * Copyright The Asahi Linux Contributors >>> + */ >>> + >>> +#include <linux/acpi.h> >>> +#include "debug.h" >>> +#include "core.h" >>> +#include "common.h" >>> + >>> +void brcmf_acpi_probe(struct device *dev, enum brcmf_bus_type bus_type, >>> + struct brcmf_mp_device *settings) >>> +{ >>> + acpi_status status; >>> + const union acpi_object *o; >>> + struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL}; >>> + struct acpi_device *adev = ACPI_COMPANION(dev); >>> + >>> + if (!adev) >>> + return; >>> + >>> + if (!ACPI_FAILURE(acpi_dev_get_property(adev, "module-instance", >>> + ACPI_TYPE_STRING, &o))) { >>> + brcmf_dbg(INFO, "ACPI module-instance=%s\n", o->string.pointer); >>> + settings->board_type = devm_kasprintf(dev, GFP_KERNEL, >>> + "apple,%s", >>> + o->string.pointer); >>> + } else { >>> + brcmf_dbg(INFO, "No ACPI module-instance\n"); >> >> Do you need to obtain the antenna-sku when there is no module-instance? > > In principle I don't think any machines would have antenna-sku and no > module-instance, though the firmware selection will still work without > it (it'll just end up using the DMI machine name instead). Right. That was my assumption as well. I would bail out here and skip obtaining the antenna-sku. >> >>> + } >>> + >>> + status = acpi_evaluate_object(adev->handle, "RWCV", NULL, &buf); >> >> Can you clarify what the above does? What does the "RWCV" mean? > > No idea what it *means* :-) > > What it is, though, is the ACPI method name to get the antenna-sku. Wow. So much for meaning-full naming ;-)
Hector Martin <marcan@marcan.st> writes: > On 2022/01/04 19:21, Arend van Spriel wrote: >> On 1/4/2022 8:26 AM, Hector Martin wrote: >>> On DT platforms, the module-instance and antenna-sku-info properties >>> are passed in the DT. On ACPI platforms, module-instance is passed via >>> the analogous Apple device property mechanism, while the antenna SKU >>> info is instead obtained via an ACPI method that grabs it from >>> non-volatile storage. >>> >>> Add support for this, to allow proper firmware selection on Apple >>> platforms. >>> >>> Signed-off-by: Hector Martin <marcan@marcan.st> >>> --- >>> .../broadcom/brcm80211/brcmfmac/Makefile | 2 + >>> .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ >>> .../broadcom/brcm80211/brcmfmac/common.c | 1 + >>> .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ >>> 4 files changed, 59 insertions(+) >>> create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >>> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >>> index 13c13504a6e8..19009eb9db93 100644 >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >>> @@ -47,3 +47,5 @@ brcmfmac-$(CONFIG_OF) += \ >>> of.o >>> brcmfmac-$(CONFIG_DMI) += \ >>> dmi.o >>> +brcmfmac-$(CONFIG_ACPI) += \ >>> + acpi.o >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >>> new file mode 100644 >>> index 000000000000..2b1a4448b291 >>> --- /dev/null >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >>> @@ -0,0 +1,47 @@ >>> +// SPDX-License-Identifier: ISC >>> +/* >>> + * Copyright The Asahi Linux Contributors >>> + */ >> >> Common format for copyright statement (in this folder) seems to be: >> >> Copyright (c) <YEAR> <COPYRIGHT_HOLDER> >> >> Regards, >> Arend > > I get this every time I submit a patch to a new subsystem :-) > > This is based on this best practice: > > https://www.linuxfoundation.org/blog/copyright-notices-in-open-source-software-projects/ I didn't know about this recommendation, thanks. > Basically, the year format quickly becomes outdated and is rather > useless, and listing specific names also ends up missing every > subsequent contributor, so more general copyright statements work better > for this kind of use case. In the end we all know git history is the > proper record of copyright status. > > I don't have a super strong opinion here, but we've been trying to > standardize on this format for contributions coming from our subproject, > and it feels more useful than a random contributor's name with a date 5 > years in the past :) If LF is fine with this approach, then it's good enough also for me. So at least from my point of view no need to make any changes.
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile index 13c13504a6e8..19009eb9db93 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile @@ -47,3 +47,5 @@ brcmfmac-$(CONFIG_OF) += \ of.o brcmfmac-$(CONFIG_DMI) += \ dmi.o +brcmfmac-$(CONFIG_ACPI) += \ + acpi.o diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c new file mode 100644 index 000000000000..2b1a4448b291 --- /dev/null +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c @@ -0,0 +1,47 @@ +// SPDX-License-Identifier: ISC +/* + * Copyright The Asahi Linux Contributors + */ + +#include <linux/acpi.h> +#include "debug.h" +#include "core.h" +#include "common.h" + +void brcmf_acpi_probe(struct device *dev, enum brcmf_bus_type bus_type, + struct brcmf_mp_device *settings) +{ + acpi_status status; + const union acpi_object *o; + struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL}; + struct acpi_device *adev = ACPI_COMPANION(dev); + + if (!adev) + return; + + if (!ACPI_FAILURE(acpi_dev_get_property(adev, "module-instance", + ACPI_TYPE_STRING, &o))) { + brcmf_dbg(INFO, "ACPI module-instance=%s\n", o->string.pointer); + settings->board_type = devm_kasprintf(dev, GFP_KERNEL, + "apple,%s", + o->string.pointer); + } else { + brcmf_dbg(INFO, "No ACPI module-instance\n"); + } + + status = acpi_evaluate_object(adev->handle, "RWCV", NULL, &buf); + o = buf.pointer; + if (!ACPI_FAILURE(status) && o && o->type == ACPI_TYPE_BUFFER && + o->buffer.length >= 2) { + char *antenna_sku = devm_kzalloc(dev, 3, GFP_KERNEL); + + memcpy(antenna_sku, o->buffer.pointer, 2); + brcmf_dbg(INFO, "ACPI RWCV data=%*phN antenna-sku=%s\n", + (int)o->buffer.length, o->buffer.pointer, + antenna_sku); + + settings->antenna_sku = antenna_sku; + } else { + brcmf_dbg(INFO, "No ACPI antenna-sku\n"); + } +} diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c index b8ed851129b4..c84c48e49fde 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c @@ -437,6 +437,7 @@ struct brcmf_mp_device *brcmf_get_module_param(struct device *dev, /* No platform data for this device, try OF and DMI data */ brcmf_dmi_probe(settings, chip, chiprev); brcmf_of_probe(dev, bus_type, settings); + brcmf_acpi_probe(dev, bus_type, settings); } return settings; } diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h index d4aa25d646fe..a88c4a9310f3 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h @@ -73,6 +73,15 @@ static inline void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev) {} #endif +#ifdef CONFIG_ACPI +void brcmf_acpi_probe(struct device *dev, enum brcmf_bus_type bus_type, + struct brcmf_mp_device *settings); +#else +static inline void brcmf_acpi_probe(struct device *dev, + enum brcmf_bus_type bus_type, + struct brcmf_mp_device *settings) {} +#endif + u8 brcmf_map_prio_to_prec(void *cfg, u8 prio); u8 brcmf_map_prio_to_aci(void *cfg, u8 prio);
On DT platforms, the module-instance and antenna-sku-info properties are passed in the DT. On ACPI platforms, module-instance is passed via the analogous Apple device property mechanism, while the antenna SKU info is instead obtained via an ACPI method that grabs it from non-volatile storage. Add support for this, to allow proper firmware selection on Apple platforms. Signed-off-by: Hector Martin <marcan@marcan.st> --- .../broadcom/brcm80211/brcmfmac/Makefile | 2 + .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ .../broadcom/brcm80211/brcmfmac/common.c | 1 + .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ 4 files changed, 59 insertions(+) create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c