| Message ID | 20240731061132.703368-5-jacobe.zang@wesion.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AP6275P wireless support | expand |
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@xff.cz> > Signed-off-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.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. > + > /* 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. Best regards, Alexey
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@xff.cz> > > Signed-off-by: Ondrej Jirman <megi@xff.cz> > > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.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); } Regards, Arend
On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel <arend.vanspriel@broadcom.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@xff.cz> > > > Signed-off-by: Ondrej Jirman <megi@xff.cz> > > > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.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? Best, Alexey
On 7/31/2024 2:01 PM, Alexey Charkov wrote: > On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel > <arend.vanspriel@broadcom.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@xff.cz> >>> > Signed-off-by: Ondrej Jirman <megi@xff.cz> >>> > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.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. Regards, Arend
On Wed, Jul 31, 2024 at 3:32 PM Arend van Spriel <arend.vanspriel@broadcom.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@broadcom.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@xff.cz> > >>> > Signed-off-by: Ondrej Jirman <megi@xff.cz> > >>> > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.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 the existing void returns should get appropriate errno's. And the functions prototypes should be updated along the call chain. And then it would still only work after pwrseq is added to ensure that power and wake signals are applied correctly along with this clock, as Sebastian pointed out in the other thread :) Which really prompts a question: should this clock be added to the PCIe driver and the respective DT binding in the first place, or should it instead be claimed by pwrseq, leaving brcmfmac alone? Best regards, Alexey
>>On 7/31/2024 2:01 PM, Alexey Charkov wrote: >>> On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel >>> <arend.vanspriel@broadcom.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@xff.cz> >>>>> > Signed-off-by: Ondrej Jirman <megi@xff.cz> >>>>> > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.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. > the existing void returns should get appropriate errno's. And the > functions prototypes should be updated along the call chain. And then > it would still only work after pwrseq is added to ensure that power > and wake signals are applied correctly along with this clock, as > Sebastian pointed out in the other thread :) > > Which really prompts a question: should this clock be added to the > PCIe driver and the respective DT binding in the first place, or > should it instead be claimed by pwrseq, leaving brcmfmac alone? --- Best Regards Jacobe
On Thu, Aug 1, 2024 at 6:53 AM Jacobe Zang <jacobe.zang@wesion.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@broadcom.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@xff.cz> > >>>>> > Signed-off-by: Ondrej Jirman <megi@xff.cz> > >>>>> > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.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
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; + /* 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; + } err = brcmf_of_get_country_codes(dev, settings); if (err)