Re: [PATCH v5 2/5] dt-bindings: net: wireless: brcm4329-fmac: add clock description for AP6275P

[Sebastian Reichel]

Hi,

On Tue, Jul 30, 2024 at 01:16:57PM GMT, Arend Van Spriel wrote:
> On July 30, 2024 12:18:20 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> 
> > On 30/07/2024 11:52, Arend Van Spriel wrote:
> > > On July 30, 2024 11:01:43 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> > > 
> > > > On 30/07/2024 08:37, Arend Van Spriel wrote:
> > > > > + Linus W
> > > > > 
> > > > > On July 30, 2024 5:31:15 AM Jacobe Zang <jacobe.zang@xxxxxxxxxx> wrote:
> > > > > 
> > > > > > Not only AP6275P Wi-Fi device but also all Broadcom wireless devices allow
> > > > > > external low power clock input. In DTS the clock as an optional choice in
> > > > > > the absence of an internal clock.
> > > > > > 
> > > > > > Reviewed-by: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx>
> > > > > > Signed-off-by: Jacobe Zang <jacobe.zang@xxxxxxxxxx>
> > > > > > ---
> > > > > > .../bindings/net/wireless/brcm,bcm4329-fmac.yaml          | 8 ++++++++
> > > > > > 1 file changed, 8 insertions(+)
> > > > > > 
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
> > > > > > b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
> > > > > > index 2c2093c77ec9a..a3607d55ef367 100644
> > > > > > --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
> > > > > > @@ -122,6 +122,14 @@ properties:
> > > > > > NVRAM. This would normally be filled in by the bootloader from platform
> > > > > > configuration data.
> > > > > > 
> > > > > > +  clocks:
> > > > > > +    items:
> > > > > > +      - description: External Low Power Clock input (32.768KHz)
> > > > > > +
> > > > > > +  clock-names:
> > > > > > +    items:
> > > > > > +      - const: lpo
> > > > > > +
> > > > > 
> > > > > We still have an issue that this clock input is also present in the
> > > > > bindings specification broadcom-bluetooth.yaml (not in bluetooth
> > > > > subfolder). This clock is actually a chip resource. What happens if both
> > > > > are defined and both wifi and bt drivers try to enable this clock? Can this
> > > > > be expressed in yaml or can we only put a textual warning in the property
> > > > > descriptions?
> > > > 
> > > > Just like all clocks, what would happen? It will be enabled.
> > > 
> > > Oh, wow! Cool stuff. But seriously is it not a problem to have two entities
> > > controlling one and the same clock? Is this use-case taken into account by
> > > the clock framework?
> > 
> > Yes, it is handled correctly. That's a basic use-case, handled by CCF
> > since some years (~12?). Anyway, whatever OS is doing (or not doing)
> > with the clocks is independent of the bindings here. The question is
> 
> Agree. Probably the bindings would not be the place to document this if it
> would be an issue.
> 
> > about hardware - does this node, which represents PCI interface of the
> > chip, has/uses the clocks.
> 
> The schematics I found for the wifi module and the khadas edge platform show
> these are indeed wired to the chip.

I have a Rockchip RK3588 Evaluation Board on my desk, which uses the
same WLAN AP6275P module. I think I already commented on a prior
version of this series: The LPO clock is needed to make the PCIe
device visible on the bus. That means this series only works if the
clock has already been running. Otherwise the PCIe driver will never
be probed. To become visible the devices requires:

1. The LPO clock to be enabled
2. Power to be applied
3. The WL_EN gpio to be configured correctly

If one of the above is not met, the device will not even appear in
'lspci'. I believe the binding needs to take into consideration, that
pwrseq is needed for the PCIe side. Fortuantely the heavy lifting of
creating the proper infrastructure for this has already been done by
Bartosz Golaszewski for Qualcomm WLAN chips. What is missing is a
pwrseq driver for the Broadcom chip (or this specific module?).

Greetings,

-- Sebastian

Attachment: signature.asc
Description: PGP signature