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

[Alexey Charkov]

On 07/08/2024 2:17 pm, Arend van Spriel wrote:
> On 8/7/2024 1:10 AM, Alexey Charkov wrote:
>> Hi Jacobe,
>>
>> On 05/08/2024 10:34 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. Change function prototypes from void
>>> to int and add appropriate errno's for return values that will be
>>> send to bus when error occurred.
>>>
>>> Co-developed-by: Ondrej Jirman <megi at xff.cz>
>>> Signed-off-by: Ondrej Jirman <megi at xff.cz>
>>> Co-developed-by: Arend van Spriel <arend.vanspriel at broadcom.com>
>>> Signed-off-by: Arend van Spriel <arend.vanspriel at broadcom.com>
>>> Signed-off-by: Jacobe Zang <jacobe.zang at wesion.com>
>>> ---
>>>   .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  4 +++
>>>   .../broadcom/brcm80211/brcmfmac/common.c      |  6 +++-
>>>   .../wireless/broadcom/brcm80211/brcmfmac/of.c | 28 +++++++++++++------
>>>   .../wireless/broadcom/brcm80211/brcmfmac/of.h |  9 +++---
>>>   .../broadcom/brcm80211/brcmfmac/pcie.c        |  3 ++
>>>   .../broadcom/brcm80211/brcmfmac/sdio.c        | 18 ++++++++----
>>>   .../broadcom/brcm80211/brcmfmac/usb.c         |  3 ++
>>>   7 files changed, 52 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/
>>> bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>>> index 13391c2d82aae..ee3ca85c4a47b 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>>> @@ -951,6 +951,10 @@ int brcmf_sdiod_probe(struct brcmf_sdio_dev
>>> *sdiodev)
>>>           ret = -ENODEV;
>>>           goto out;
>>>       }
>>> +    if (IS_ERR(sdiodev->bus)) {
>>> +        ret = PTR_ERR(sdiodev->bus);
>>> +        goto out;
>>> +    }
>>
>> Maybe return -ENODEV error pointer instead of NULL from
>> brcmf_sdio_probe as the default for the fail path? Then you can
>> condense these two checks into one
>
> Sound reasonable.
>
>>>       brcmf_sdiod_host_fixup(sdiodev->func2->card->host);
>>>   out:
>>>       if (ret)
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/
>>> common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>>> index b24faae35873d..6c5d26f9b7661 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>>> @@ -561,8 +561,12 @@ struct brcmf_mp_device
>>> *brcmf_get_module_param(struct device *dev,
>>>       if (!found) {
>>>           /* 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);
>>> +        i = brcmf_of_probe(dev, bus_type, settings);
>>> +        if (i < 0) {
>>> +            kfree(settings);
>>> +            settings = ERR_PTR(i);
>>> +        }
>>
>> This looks wrong. First, you're calling brcmf_of_probe twice. Second,
>> if either DMI or ACPI probe successfully but OF doesn't, then you
>> return an error code instead of success, and also overwrite settings
>> with an error pointer thus rendering both brcmf_dmi_probe and
>> brcmf_acpi_probe useless
>
> Twice? it is removed and added few lines below.

Indeed, time to change glasses :) Didn't see the minus sign

> It does change the order
> so that may not be best thing to do here. We actually only want to
> handle the scenario where the clock resources are not yet available, ie.
> when -EPROBE_DEFER is returned because that error value is taken into
> account by the bus driver and tries to bind the driver again later.

Maybe we then do something like the following, which would retain the
old behavior but pass -EPROBE_DEFER on to the bus:

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index b24faae35873d..6c5d26f9b7661 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -561,8 +561,12 @@ struct brcmf_mp_device
*brcmf_get_module_param(struct device *dev,
        if (!found) {
                /* No platform data for this device, try OF and DMI data */
                brcmf_dmi_probe(settings, chip, chiprev);
-               brcmf_of_probe(dev, bus_type, settings);
+               if (brcmf_of_probe(dev, bus_type, settings) == -EPROBE_DEFER)
+                       return ERR_PTR(-EPROBE_DEFER);
                brcmf_acpi_probe(dev, bus_type, settings);
        }
        return settings;
 }

>>>       }
>>>       return settings;
>>>   }
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/
>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
>>> index e406e11481a62..5f61363fb5d0e 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"
>>> @@ -65,17 +66,21 @@ static int brcmf_of_get_country_codes(struct
>>> device *dev,
>>>       return 0;
>>>   }
>>> -void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
>>> -            struct brcmf_mp_device *settings)
>>> +int brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
>>> +           struct brcmf_mp_device *settings)
>>>   {
>>>       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 0;
>>> +
>>
>> return 0 implies this function has completed successfully, while in
>> this case it's obviously returned early due to not finding the correct
>> device in DT. -ENODEV perhaps?
>
> This was a void function so returning 0 retains the behavior as before,
> which is important to keep in mind here.
>
> This function will be called if the platform has CONFIG_OF enabled.
> However, that does not mean that on every platform there is a node
> defined for the struct device being probed. That is fine if it does not
> require any DT properties to be functional. Hence we bail out here
> without an error.

Fair enough, thanks for the explanation!

Best regards,
Alexey