| Message ID | 20240630073605.2164346-5-jacobe.zang@wesion.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add AP6275P wireless support | expand |
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)
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
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
>> 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
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
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 --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)