| Message ID | 20240630073605.2164346-5-jacobe.zang@wesion.com (mailing list archive) |
|---|---|
| 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)
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/ --- Best Regards Jacobe
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
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 > > > 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
On June 30, 2024 9:36:37 AM Jacobe Zang <jacobe.zang@wesion.com> wrote: > WiFi modules often require 32kHz clock to function. Add support to > enable the clock to PCIe driver. Another remark. This clock handling code should be done in of.c where we handle all device tree stuff. > 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(+)
>> 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
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)