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

[Jacobe Zang]

>>On 7/31/2024 2:01 PM, Alexey Charkov wrote:
>>> On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel
>>> <arend.vanspriel@xxxxxxxxxxxx> 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@xxxxxx>
>>>>>   > Signed-off-by: Ondrej Jirman <megi@xxxxxx>
>>>>>   > Signed-off-by: Jacobe Zang <jacobe.zang@xxxxxxxxxx>
>>>>>   > ---
>>>>>   >  .../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.
> 
> Indeed. Perhaps something along the lines of:
> 
>        clk = devm_clk_get_optional_enabled(dev, "lpo");
>        if (!IS_ERR_OR_NULL(clk)) {
>                brcmf_dbg(INFO, "enabling 32kHz clock\n");
>                return clk_set_rate(clk, 32768);
>        } else {
>                return PTR_ERR_OR_ZERO(clk);
>        }
> 
> ... which should then go at the very end of brcmf_of_probe. And all of

But before end of brcmf_of_probe is to set interrupt configuration which
wifi chip connect via sdio. Like this:
```
	if (bus_type != BRCMF_BUSTYPE_SDIO)
		return;

	if (of_property_read_u32(np, "brcm,drive-strength", &val) == 0)
		sdio->drive_strength = val;

	/* make sure there are interrupts defined in the node */
	if (!of_property_present(np, "interrupts"))
		return;

	irq = irq_of_parse_and_map(np, 0);
	if (!irq) {
		brcmf_err("interrupt could not be mapped\n");
		return;
	}
	irqf = irqd_get_trigger_type(irq_get_irq_data(irq));

	sdio->oob_irq_supported = true;
	sdio->oob_irq_nr = irq;
	sdio->oob_irq_flags = irqf;
```
So I think the interrupt should be set in the if statement while
bus_type==BRCMF_BUSTYPE_SDIO, and add else statement
to enable clock(or simply put it at the end as Alexey said). And
can also use else-if statement to deal with
bus_type == BRCMF_BUSTYPE_USB or PCIE in the future.

> the existing void returns should get appropriate errno's. And the
> functions prototypes should be updated along the call chain. And then
> it would still only work after pwrseq is added to ensure that power
> and wake signals are applied correctly along with this clock, as
> Sebastian pointed out in the other thread :)
> 
> Which really prompts a question: should this clock be added to the
> PCIe driver and the respective DT binding in the first place, or
> should it instead be claimed by pwrseq, leaving brcmfmac alone?

---
Best Regards
Jacobe