Re: [PATCH v6 4/5] wifi: brcmfmac: Add optional lpo clock enable support

Arend van Spriel
2024-07-31T12:32:32Z

On 7/31/2024 2:01 PM, Alexey Charkov wrote:
> On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>>
>> On 7/31/2024 12:16 PM, Alexey Charkov wrote:
>>> Hi Jacobe,
>>>
>>>
>>> On 31/07/2024 9:11 am, Jacobe Zang wrote:
>>> > WiFi modules often require 32kHz clock to function. Add support to
>>> > enable the clock to PCIe driver and move "brcm,bcm4329-fmac" check
>>> > to the top of brcmf_of_probe
>>> >
>>> > Co-developed-by: Ondrej Jirman <megi@xff.cz>
>>> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
>>> > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
>>> > ---
>>> > .../net/wireless/broadcom/brcm80211/brcmfmac/of.c | 12 +++++++++++-
>>> > 1 file changed, 11 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>> > index e406e11481a62..7e0a2ad5c7c8a 100644
>>> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>> > @@ -6,6 +6,7 @@
>>> > #include <linux/of.h>
>>> > #include <linux/of_irq.h>
>>> > #include <linux/of_net.h>
>>> > +#include <linux/clk.h>
>>> >
>>> > #include <defs.h>
>>> > #include "debug.h"
>>> > @@ -70,12 +71,16 @@ void brcmf_of_probe(struct device *dev, enum
>>> brcmf_bus_type bus_type,
>>> > {
>>> > struct brcmfmac_sdio_pd *sdio = &settings->bus.sdio;
>>> > struct device_node *root, *np = dev->of_node;
>>> > + struct clk *clk;
>>> > const char *prop;
>>> > int irq;
>>> > int err;
>>> > u32 irqf;
>>> > u32 val;
>>> >
>>> > + if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
>>> > + return;
>>>
>>> Did you test this? The DTS patch you sent as part of this series doesn't
>>> list "brcm,bcm4329-fmac" in the compatible, so this will probably return
>>> right here, skipping over the rest of your patch.
>>>
>>> Please test before resending, both with and without the driver for the
>>> Bluetooth part of the chip (since it also touches clocks).
>>>
>>> You are also changing the behavior for other systems by putting this
>>> check further up the probe path, which might break things for no reason.
>>> Better put your clk-related addition below where this check was
>>> originally, rather than reorder stuff you don't have to reorder.
>>
>> That was upon my suggestion. That check was originally at the top of the
>> function, but people added stuff before that. I agree that makes the
>> compatible "brcm,brcm4329-fmac" required which is what the textual
>> binding stated before the switch to YAML was made:
>>
>> """
>> Broadcom BCM43xx Fullmac wireless SDIO devices
>>
>> This node provides properties for controlling the Broadcom wireless
>> device. The
>> node is expected to be specified as a child node to the SDIO controller that
>> connects the device to the system.
>>
>> Required properties:
>>
>> - compatible : Should be "brcm,bcm4329-fmac".
>> """
>>
>> Not sure whether this is still true for YAML version (poor YAML reading
>> skills ;-) ), but it should as the switch from textual to YAML should
>> not have changed the bindings specification.
>>
>>> > +
>>> > /* Apple ARM64 platforms have their own idea of board type,
>>> passed in
>>> > * via the device tree. They also have an antenna SKU parameter
>>> > */
>>> > @@ -113,8 +118,13 @@ void brcmf_of_probe(struct device *dev, enum
>>> brcmf_bus_type bus_type,
>>> > of_node_put(root);
>>> > }
>>> >
>>> > - if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
>>> > + clk = devm_clk_get_optional_enabled(dev, "lpo");
>>> > + if (!IS_ERR_OR_NULL(clk)) {
>>> > + brcmf_dbg(INFO, "enabling 32kHz clock\n");
>>> > + clk_set_rate(clk, 32768);
>>> > + } else {
>>> > return;
>>>
>>> Why return here? If the clock is optional, a lot of systems will not
>>> have it - that shouldn't prevent the driver from probing. And you are
>>> still not handling the -EPROBE_DEFER case which was mentioned on your
>>> previous submission.
>>
>> Right. The else statement above could/should be:
>>
>> } else if (clk && PTR_ERR(clk) == -EPROBE_DEFER) {
>> return PTR_ERR(clk);
>> }
>
> ... plus change the function prototype to return int and propagate
> that error code through brcmf_get_module_param to brcmf_pcie_probe's
> return value. I guess checking clk for NULL is also redundant in this
> case?

Only wanted to give the suggestion to get started. Propagating the
return value seemed obvious to me, but you are absolutely right.
PTR_ERR(NULL) will probably be something else than -EPROBE_DEFER but it
seems odd to me. Maybe PTR_ERR_OR_ZERO(clk) is a better option here.

Regards,
Arend