diff mbox series

[v3,4/5] wifi: brcmfmac: Add optional lpo clock enable support

Message ID 20240630073605.2164346-5-jacobe.zang@wesion.com
State New
Headers show
Series Add AP6275P wireless support | expand

Commit Message

Jacobe Zang June 30, 2024, 7:36 a.m. UTC
WiFi modules often require 32kHz clock to function. Add support to
enable the clock to PCIe driver.

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/pcie.c    | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Stefan Wahren June 30, 2024, 8:43 a.m. UTC | #1
Hi,

Am 30.06.24 um 09:36 schrieb Jacobe Zang:
> WiFi modules often require 32kHz clock to function. Add support to
> enable the clock to PCIe driver.
the low power clock is independent from the host interface like PCIe. So
the clock handling should move to the common code. Sorry, not i cannot
give a good suggestion, what's the best place for this.
>
> 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/pcie.c    | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index 06698a714b523..e84f562fc91b8 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -3,6 +3,7 @@
>    * Copyright (c) 2014 Broadcom Corporation
>    */
>
> +#include <linux/clk.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/firmware.h>
> @@ -2411,6 +2412,7 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   	struct brcmf_pciedev *pcie_bus_dev;
>   	struct brcmf_core *core;
>   	struct brcmf_bus *bus;
> +	struct clk *clk;
>
>   	if (!id) {
>   		id = pci_match_id(brcmf_pcie_devid_table, pdev);
> @@ -2422,6 +2424,14 @@ brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>
>   	brcmf_dbg(PCIE, "Enter %x:%x\n", pdev->vendor, pdev->device);
>
> +	clk = devm_clk_get_optional_enabled(&pdev->dev, "lpo");
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +	if (clk) {
> +		brcmf_dbg(PCIE, "enabling 32kHz clock\n", pdev->vendor, pdev->device);
> +		clk_set_rate(clk, 32768);
> +	}
> +
>   	ret = -ENOMEM;
>   	devinfo = kzalloc(sizeof(*devinfo), GFP_KERNEL);
>   	if (devinfo == NULL)
Chen-Yu Tsai June 30, 2024, 9:15 a.m. UTC | #2
On Sun, Jun 30, 2024 at 5:10 PM Jacobe Zang <jacobe.zang@wesion.com> wrote:
>
> Hi Stefan,
>
> >> WiFi modules often require 32kHz clock to function. Add support to
> >> enable the clock to PCIe driver.
> > the low power clock is independent from the host interface like PCIe. So
> > the clock handling should move to the common code. Sorry, not i cannot
> > give a good suggestion, what's the best place for this.
>
> I think the clock is used by the PCIe device so enable it in this file. Also I checked
> use of clock which in spi[0] or sdio[0] device was enabled similarly to this.
>
> [0] https://lore.kernel.org/all/20210806081229.721731-4-claudiu.beznea@microchip.com/

You're looking at the wrong driver. For brcmfmac, the lpo clock is toggled
by the MMC pwrseq code. And for the Bluetooth side (where it really matters)
for UARTs, it is in drivers/bluetooth/hci_bcm.c. and documented in the
binding Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml


ChenYu
Arend Van Spriel June 30, 2024, 10:51 a.m. UTC | #3
On June 30, 2024 11:54:43 AM Stefan Wahren <wahrenst@gmx.net> wrote:

> Am 30.06.24 um 11:15 schrieb Chen-Yu Tsai:
>> On Sun, Jun 30, 2024 at 5:10 PM Jacobe Zang <jacobe.zang@wesion.com> wrote:
>>> Hi Stefan,
>>>
>>>>> WiFi modules often require 32kHz clock to function. Add support to
>>>>> enable the clock to PCIe driver.
>>>> the low power clock is independent from the host interface like PCIe. So
>>>> the clock handling should move to the common code. Sorry, not i cannot
>>>> give a good suggestion, what's the best place for this.
>>> I think the clock is used by the PCIe device so enable it in this file. 
>>> Also I checked
>>> use of clock which in spi[0] or sdio[0] device was enabled similarly to this.
>>>
>>> [0] 
>>> https://lore.kernel.org/all/20210806081229.721731-4-claudiu.beznea@microchip.com/
>> You're looking at the wrong driver. For brcmfmac, the lpo clock is toggled
>> by the MMC pwrseq code. And for the Bluetooth side (where it really matters)
>> for UARTs, it is in drivers/bluetooth/hci_bcm.c. and documented in the
>> binding Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml
> Thanks for clarifying. So this change handles the PCIe case without
> bluetooth. For USB the clock control doesn't make sense.
>
> Sorry for the noise

So someone could end up with both wifi and bt LPO clock defined in DTS 
file. Not sure if that can be expressed and validated in device tree, but 
at the least there should be a fair warning in both binding files that 
there can be only one!

The LPO clock matters to the chip. It is not specific to the BT part. The 
clock is important for the power-up cycle. The timing difference WL_REG_ON 
and BT_REG_ON is expressed in LPO clock cycles.

Regards,
Arend
Jacobe Zang July 1, 2024, 10:57 a.m. UTC | #4
>> Am 30.06.24 um 11:15 schrieb Chen-Yu Tsai:
>>> On Sun, Jun 30, 2024 at 5:10 PM Jacobe Zang <jacobe.zang@wesion.com> wrote:
>>>> Hi Stefan,
>>>>
>>>>>> WiFi modules often require 32kHz clock to function. Add support to
>>>>>> enable the clock to PCIe driver.
>>>>> the low power clock is independent from the host interface like PCIe. So
>>>>> the clock handling should move to the common code. Sorry, not i cannot
>>>>> give a good suggestion, what's the best place for this.
>>>> I think the clock is used by the PCIe device so enable it in this file.
>>>> Also I checked
>>>> use of clock which in spi[0] or sdio[0] device was enabled similarly to this.
>>>>
>>>> [0]
>>>> https://lore.kernel.org/all/20210806081229.721731-4-claudiu.beznea@microchip.com/
>>> You're looking at the wrong driver. For brcmfmac, the lpo clock is toggled
>>> by the MMC pwrseq code. And for the Bluetooth side (where it really matters)
>>> for UARTs, it is in drivers/bluetooth/hci_bcm.c. and documented in the
>>> binding Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml

As ChenYu said, hci_bcm.c has handled for bt clock. But it seemed that clock has
never handled in brcmfmac wifi driver. So I wonder does everyone all enable the
clock in BT node not wifi?

>> Thanks for clarifying. So this change handles the PCIe case without
>> bluetooth. For USB the clock control doesn't make sense.
>>
>> Sorry for the noise
>
> So someone could end up with both wifi and bt LPO clock defined in DTS
> file. Not sure if that can be expressed and validated in device tree, but
> at the least there should be a fair warning in both binding files that
> there can be only one!
>
> The LPO clock matters to the chip. It is not specific to the BT part. The

I also think that it is necessary to handle the clock in wifi driver, though.

> clock is important for the power-up cycle. The timing difference WL_REG_ON
> and BT_REG_ON is expressed in LPO clock cycles.

---
Best Regards
Jacobe
kernel test robot July 1, 2024, 12:03 p.m. UTC | #5
Hi Jacobe,

kernel test robot noticed the following build warnings:

[auto build test WARNING on wireless-next/main]
[also build test WARNING on wireless/main rockchip/for-next robh/for-next linus/master v6.10-rc6 next-20240628]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacobe-Zang/dt-bindings-net-wireless-brcm4329-fmac-add-pci14e4-449d/20240630-221025
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link:    https://lore.kernel.org/r/20240630073605.2164346-5-jacobe.zang%40wesion.com
patch subject: [PATCH v3 4/5] wifi: brcmfmac: Add optional lpo clock enable support
config: riscv-allyesconfig
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 326ba38a991250a8587a399a260b0f7af2c9166a)
reproduce (this is a W=1 build):

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407011914.7gDVanTu-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c:10:
   In file included from include/linux/pci.h:38:
   In file included from include/linux/interrupt.h:21:
   In file included from arch/riscv/include/asm/sections.h:9:
   In file included from include/linux/mm.h:2258:
   include/linux/vmstat.h:500:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     500 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     501 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:507:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     507 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     508 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:519:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     519 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     520 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:528:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     528 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     529 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c:2426:45: warning: data argument not used by format string [-Wformat-extra-args]
    2426 |                 brcmf_dbg(PCIE, "enabling 32kHz clock\n", pdev->vendor, pdev->device);
         |                                 ~~~~~~~~~~~~~~~~~~~~~~~~  ^
   drivers/net/wireless/broadcom/brcm80211/brcmfmac/debug.h:77:14: note: expanded from macro 'brcmf_dbg'
      77 |                     fmt, ##__VA_ARGS__);                        \
         |                     ~~~    ^
   6 warnings generated.


vim +2426 drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c

  2400	
  2401	static int
  2402	brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  2403	{
  2404		int ret;
  2405		struct brcmf_fw_request *fwreq;
  2406		struct brcmf_pciedev_info *devinfo;
  2407		struct brcmf_pciedev *pcie_bus_dev;
  2408		struct brcmf_core *core;
  2409		struct brcmf_bus *bus;
  2410		struct clk *clk;
  2411	
  2412		if (!id) {
  2413			id = pci_match_id(brcmf_pcie_devid_table, pdev);
  2414			if (!id) {
  2415				pci_err(pdev, "Error could not find pci_device_id for %x:%x\n", pdev->vendor, pdev->device);
  2416				return -ENODEV;
  2417			}
  2418		}
  2419	
  2420		brcmf_dbg(PCIE, "Enter %x:%x\n", pdev->vendor, pdev->device);
  2421	
  2422		clk = devm_clk_get_optional_enabled(&pdev->dev, "lpo");
  2423		if (IS_ERR(clk))
  2424			return PTR_ERR(clk);
  2425		if (clk) {
> 2426			brcmf_dbg(PCIE, "enabling 32kHz clock\n", pdev->vendor, pdev->device);
  2427			clk_set_rate(clk, 32768);
  2428		}
  2429	
  2430		ret = -ENOMEM;
  2431		devinfo = kzalloc(sizeof(*devinfo), GFP_KERNEL);
  2432		if (devinfo == NULL)
  2433			return ret;
  2434	
  2435		devinfo->pdev = pdev;
  2436		pcie_bus_dev = NULL;
  2437		devinfo->ci = brcmf_chip_attach(devinfo, pdev->device,
  2438						&brcmf_pcie_buscore_ops);
  2439		if (IS_ERR(devinfo->ci)) {
  2440			ret = PTR_ERR(devinfo->ci);
  2441			devinfo->ci = NULL;
  2442			goto fail;
  2443		}
  2444	
  2445		core = brcmf_chip_get_core(devinfo->ci, BCMA_CORE_PCIE2);
  2446		if (core->rev >= 64)
  2447			devinfo->reginfo = &brcmf_reginfo_64;
  2448		else
  2449			devinfo->reginfo = &brcmf_reginfo_default;
  2450	
  2451		pcie_bus_dev = kzalloc(sizeof(*pcie_bus_dev), GFP_KERNEL);
  2452		if (pcie_bus_dev == NULL) {
  2453			ret = -ENOMEM;
  2454			goto fail;
  2455		}
  2456	
  2457		devinfo->settings = brcmf_get_module_param(&devinfo->pdev->dev,
  2458							   BRCMF_BUSTYPE_PCIE,
  2459							   devinfo->ci->chip,
  2460							   devinfo->ci->chiprev);
  2461		if (!devinfo->settings) {
  2462			ret = -ENOMEM;
  2463			goto fail;
  2464		}
  2465	
  2466		bus = kzalloc(sizeof(*bus), GFP_KERNEL);
  2467		if (!bus) {
  2468			ret = -ENOMEM;
  2469			goto fail;
  2470		}
  2471		bus->msgbuf = kzalloc(sizeof(*bus->msgbuf), GFP_KERNEL);
  2472		if (!bus->msgbuf) {
  2473			ret = -ENOMEM;
  2474			kfree(bus);
  2475			goto fail;
  2476		}
  2477	
  2478		/* hook it all together. */
  2479		pcie_bus_dev->devinfo = devinfo;
  2480		pcie_bus_dev->bus = bus;
  2481		bus->dev = &pdev->dev;
  2482		bus->bus_priv.pcie = pcie_bus_dev;
  2483		bus->ops = &brcmf_pcie_bus_ops;
  2484		bus->proto_type = BRCMF_PROTO_MSGBUF;
  2485		bus->fwvid = id->driver_data;
  2486		bus->chip = devinfo->coreid;
  2487		bus->wowl_supported = pci_pme_capable(pdev, PCI_D3hot);
  2488		dev_set_drvdata(&pdev->dev, bus);
  2489	
  2490		ret = brcmf_alloc(&devinfo->pdev->dev, devinfo->settings);
  2491		if (ret)
  2492			goto fail_bus;
  2493	
  2494		ret = brcmf_pcie_read_otp(devinfo);
  2495		if (ret) {
  2496			brcmf_err(bus, "failed to parse OTP\n");
  2497			goto fail_brcmf;
  2498		}
  2499
Arend Van Spriel July 2, 2024, 7:20 p.m. UTC | #6
On 6/30/2024 9:36 AM, Jacobe Zang wrote:
> WiFi modules often require 32kHz clock to function. Add support to
> enable the clock to PCIe driver.

you can add my....

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> 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/pcie.c    | 10 ++++++++++

These changes should really go into of.c.

>   1 file changed, 10 insertions(+)
diff mbox series

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 06698a714b523..e84f562fc91b8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -3,6 +3,7 @@ 
  * Copyright (c) 2014 Broadcom Corporation
  */
 
+#include <linux/clk.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/firmware.h>
@@ -2411,6 +2412,7 @@  brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct brcmf_pciedev *pcie_bus_dev;
 	struct brcmf_core *core;
 	struct brcmf_bus *bus;
+	struct clk *clk;
 
 	if (!id) {
 		id = pci_match_id(brcmf_pcie_devid_table, pdev);
@@ -2422,6 +2424,14 @@  brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	brcmf_dbg(PCIE, "Enter %x:%x\n", pdev->vendor, pdev->device);
 
+	clk = devm_clk_get_optional_enabled(&pdev->dev, "lpo");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	if (clk) {
+		brcmf_dbg(PCIE, "enabling 32kHz clock\n", pdev->vendor, pdev->device);
+		clk_set_rate(clk, 32768);
+	}
+
 	ret = -ENOMEM;
 	devinfo = kzalloc(sizeof(*devinfo), GFP_KERNEL);
 	if (devinfo == NULL)