Message-ID: <CABjd4YwCFpPerXRaR=6zd-61wDE6nH7_s0C6jMRhA4x0L6guLg@mail.gmail.com>
Date: Thu, 1 Aug 2024 10:57:31 +0300
From: Alexey Charkov <alchark@...il.com>
To: Jacobe Zang <jacobe.zang@...ion.com>
Cc: Arend van Spriel <arend.vanspriel@...adcom.com>, "robh@...nel.org" <robh@...nel.org>, 
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "heiko@...ech.de" <heiko@...ech.de>, 
	"kvalo@...nel.org" <kvalo@...nel.org>, "davem@...emloft.net" <davem@...emloft.net>, 
	"edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>, 
	"pabeni@...hat.com" <pabeni@...hat.com>, "conor+dt@...nel.org" <conor+dt@...nel.org>, 
	"efectn@...tonmail.com" <efectn@...tonmail.com>, "dsimic@...jaro.org" <dsimic@...jaro.org>, 
	"jagan@...eble.ai" <jagan@...eble.ai>, 
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, 
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, 
	"linux-rockchip@...ts.infradead.org" <linux-rockchip@...ts.infradead.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "arend@...adcom.com" <arend@...adcom.com>, 
	"linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "megi@....cz" <megi@....cz>, 
	"duoming@....edu.cn" <duoming@....edu.cn>, "bhelgaas@...gle.com" <bhelgaas@...gle.com>, 
	"minipli@...ecurity.net" <minipli@...ecurity.net>, 
	"brcm80211@...ts.linux.dev" <brcm80211@...ts.linux.dev>, 
	"brcm80211-dev-list.pdl@...adcom.com" <brcm80211-dev-list.pdl@...adcom.com>, Nick Xie <nick@...das.com>
Subject: Re: [PATCH v6 4/5] wifi: brcmfmac: Add optional lpo clock enable support

On Thu, Aug 1, 2024 at 6:53 AM Jacobe Zang <jacobe.zang@...ion.com> wrote:
>
> >>On 7/31/2024 2:01 PM, Alexey Charkov wrote:
> >>> On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel
> >>> <arend.vanspriel@...adcom.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@....cz>
> >>>>>   > Signed-off-by: Ondrej Jirman <megi@....cz>
> >>>>>   > Signed-off-by: Jacobe Zang <jacobe.zang@...ion.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.
> >
> > 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.

SDIO devices might also want to enable a clock, so I think wrapping
the drive strength and interrupts handling into an if statement and
putting the clock-related stuff right after it (but not in the else
block) is better.

Best regards,
Alexey

Powered by blists - more mailing lists