diff mbox series

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

Message ID 20240730033053.4092132-3-jacobe.zang@wesion.com
State Superseded
Headers show
Series Add AP6275P wireless support | expand

Commit Message

Jacobe Zang July 30, 2024, 3:30 a.m. UTC
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@broadcom.com>
Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
---
 .../bindings/net/wireless/brcm,bcm4329-fmac.yaml          | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Krzysztof Kozlowski July 30, 2024, 6:03 a.m. UTC | #1
On 30/07/2024 05:30, Jacobe Zang 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@broadcom.com>
> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Krzysztof Kozlowski July 30, 2024, 9:01 a.m. UTC | #2
On 30/07/2024 08:37, Arend Van Spriel wrote:
> + Linus W
> 
> On July 30, 2024 5:31:15 AM Jacobe Zang <jacobe.zang@wesion.com> 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@broadcom.com>
>> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
>> ---
>> .../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.

Best regards,
Krzysztof
Jacobe Zang July 30, 2024, 10 a.m. UTC | #3
>> On 30/07/2024 08:37, Arend Van Spriel wrote:
>>> + Linus W
>>>
>>> On July 30, 2024 5:31:15 AM Jacobe Zang <jacobe.zang@wesion.com> 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@broadcom.com>
>>>> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
>>>> ---
>>>> .../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?

I have enabled the same clock both in bluetooth and wifi just now, they worked
well. Maybe this make sense?

---
Best Regards
Jacobe
Krzysztof Kozlowski July 30, 2024, 10:18 a.m. UTC | #4
On 30/07/2024 11:52, Arend Van Spriel wrote:
> On July 30, 2024 11:01:43 AM Krzysztof Kozlowski <krzk@kernel.org> 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@wesion.com> 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@broadcom.com>
>>>> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
>>>> ---
>>>> .../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
about hardware - does this node, which represents PCI interface of the
chip, has/uses the clocks?

Best regards,
Krzysztof
Arend Van Spriel July 30, 2024, 11:16 a.m. UTC | #5
On July 30, 2024 12:18:20 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 30/07/2024 11:52, Arend Van Spriel wrote:
>> On July 30, 2024 11:01:43 AM Krzysztof Kozlowski <krzk@kernel.org> 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@wesion.com> 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@broadcom.com>
>>>>> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
>>>>> ---
>>>>> .../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.

Regards,
Arend

> Best regards,
> Krzysztof
Sebastian Reichel July 30, 2024, 5:38 p.m. UTC | #6
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@kernel.org> wrote:
> 
> > On 30/07/2024 11:52, Arend Van Spriel wrote:
> > > On July 30, 2024 11:01:43 AM Krzysztof Kozlowski <krzk@kernel.org> 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@wesion.com> 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@broadcom.com>
> > > > > > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
> > > > > > ---
> > > > > > .../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
Arend Van Spriel July 31, 2024, 12:57 p.m. UTC | #7
On 7/30/2024 7:38 PM, Sebastian Reichel wrote:
> 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@kernel.org> wrote:
>>
>>> On 30/07/2024 11:52, Arend Van Spriel wrote:
>>>> On July 30, 2024 11:01:43 AM Krzysztof Kozlowski <krzk@kernel.org> 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@wesion.com> 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@broadcom.com>
>>>>>>> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
>>>>>>> ---
>>>>>>> .../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?).

That does not really make sense. There is no relation between the LPO 
clock and the PCIe clocks so 1) being a requirement for probing the 
device looks odd. It also does not match past experience when I assisted 
Andy Green in getting this module up and running almost two years ago.

"""
On 11/9/22 18:26, Arend Van Spriel wrote:
 > On November 8, 2022 11:48:22 AM Andy Green <andy@warmcat.com> wrote:
 >> Hi -
 >>
 >> I'm trying to bring up AP6275 support on 6.1-rc4... I have tried a 
forward-ported sdk broadcom driver from the 5.10 based soc sdk, and the 
mainline brcm fullmac driver.
 >
 > Do you have a reference to the SDK? For what SoC?

Hi Arend -

It's the OOT broadcom driver that came with the latest (Sept 2022) 
vendor SDK for RK3588, from Rockchip.  Their evb has an AP6275 onboard.

PCIe generally is working on this (eg, for NVMe in the PCIe 4-lane slot) 
and for network, and the PCIe part seems OK when I hack in a gpio 
regulator to hold up the module enable gpio.
"""

So regarding 2) and 3) I agree with you.

Regards,
Arend
Sebastian Reichel July 31, 2024, 1:54 p.m. UTC | #8
Hi,

On Wed, Jul 31, 2024 at 02:57:37PM GMT, Arend van Spriel wrote:
> On 7/30/2024 7:38 PM, Sebastian Reichel wrote:
> > 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@kernel.org> wrote:
> > > 
> > > > On 30/07/2024 11:52, Arend Van Spriel wrote:
> > > > > On July 30, 2024 11:01:43 AM Krzysztof Kozlowski <krzk@kernel.org> 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@wesion.com> 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@broadcom.com>
> > > > > > > > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
> > > > > > > > ---
> > > > > > > > .../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?).
> 
> That does not really make sense. There is no relation between the LPO clock
> and the PCIe clocks so 1) being a requirement for probing the device looks
> odd. It also does not match past experience when I assisted Andy Green in
> getting this module up and running almost two years ago.

Well, first of all I can easily reproduce this on my RK3588 EVB1. I
intentionally ignore any bluetooth bits to avoid cross-effects from
bluetooth enabling any clocks / regulators / GPIOs and make sure the
RTC output clock is disabled at boot time (i.e. boot once without
any reference to the RTC clock and without 'clk_ignore_unused'
kernel argument). When booting up like this the WLAN device is not
visible in 'lspci' despite the WL_REG_ON GPIO being hogged. If I
additionally hack the RTC output clock to be enabled the WLAN device
becomes visible in 'lspci'.

The datasheet fully explains this:

https://www.lcsc.com/datasheet/lcsc_datasheet_2203281730_AMPAK-Tech-AP6275P_C2984107.pdf

PDF Page 23/24 (20/21 in the footer) has the Host Interface Timing
Diagram. WL_REG_ON should only be enabled after 2 cycles from LPO.
That means with LPO being disabled WL_REG_ON cannot be enabled. I'm
pretty sure WL_REG_ON means WLAN_REGULATOR_ON, so the logic is not
powered. On page 27 (24 in the footer) there is also a PCIe Power-On
Timing diagram, which shows that WL_REG_ON must be enabled before
the PCIe refclk is enabled.

So there is a specific power up sequence, which must be followed.

Greetings,

-- Sebastian
Arend Van Spriel July 31, 2024, 3:12 p.m. UTC | #9
On July 31, 2024 3:54:52 PM Sebastian Reichel 
<sebastian.reichel@collabora.com> wrote:

> Hi,
>
> On Wed, Jul 31, 2024 at 02:57:37PM GMT, Arend van Spriel wrote:
>> On 7/30/2024 7:38 PM, Sebastian Reichel wrote:
>>> 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@kernel.org> wrote:
>>>>
>>>>> On 30/07/2024 11:52, Arend Van Spriel wrote:
>>>>>> On July 30, 2024 11:01:43 AM Krzysztof Kozlowski <krzk@kernel.org> 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@wesion.com> 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@broadcom.com>
>>>>>>> > > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
>>>>>>> > > ---
>>>>>>> > > .../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?).
>>
>> That does not really make sense. There is no relation between the LPO clock
>> and the PCIe clocks so 1) being a requirement for probing the device looks
>> odd. It also does not match past experience when I assisted Andy Green in
>> getting this module up and running almost two years ago.
>
> Well, first of all I can easily reproduce this on my RK3588 EVB1. I
> intentionally ignore any bluetooth bits to avoid cross-effects from
> bluetooth enabling any clocks / regulators / GPIOs and make sure the
> RTC output clock is disabled at boot time (i.e. boot once without
> any reference to the RTC clock and without 'clk_ignore_unused'
> kernel argument). When booting up like this the WLAN device is not
> visible in 'lspci' despite the WL_REG_ON GPIO being hogged. If I
> additionally hack the RTC output clock to be enabled the WLAN device
> becomes visible in 'lspci'.
>
> The datasheet fully explains this:
>
> https://www.lcsc.com/datasheet/lcsc_datasheet_2203281730_AMPAK-Tech-AP6275P_C2984107.pdf
>
> PDF Page 23/24 (20/21 in the footer) has the Host Interface Timing
> Diagram. WL_REG_ON should only be enabled after 2 cycles from LPO.
> That means with LPO being disabled WL_REG_ON cannot be enabled. I'm
> pretty sure WL_REG_ON means WLAN_REGULATOR_ON, so the logic is not
> powered. On page 27 (24 in the footer) there is also a PCIe Power-On
> Timing diagram, which shows that WL_REG_ON must be enabled before
> the PCIe refclk is enabled.
>
> So there is a specific power up sequence, which must be followed.

The chip also has an (less accurate) internal LPO so the 32khz sleep clock 
in the diagram does not have to be an external clock. Maybe Ampak 
bootstrapped the chip to disable the internal clock. Dunno.

What Andy needed back then to get firmware running was a change in the 
nvram file to force using the internal LPO, but the device was already 
visible on the PCIe bus.

Regards,
Arend
Sebastian Reichel July 31, 2024, 5:50 p.m. UTC | #10
Hi,

On Wed, Jul 31, 2024 at 05:12:43PM GMT, Arend Van Spriel wrote:
> On July 31, 2024 3:54:52 PM Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> > On Wed, Jul 31, 2024 at 02:57:37PM GMT, Arend van Spriel wrote:
> > > On 7/30/2024 7:38 PM, Sebastian Reichel wrote:
> > > > 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@kernel.org> wrote:
> > > > > 
> > > > > > On 30/07/2024 11:52, Arend Van Spriel wrote:
> > > > > > > On July 30, 2024 11:01:43 AM Krzysztof Kozlowski <krzk@kernel.org> 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@wesion.com> 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@broadcom.com>
> > > > > > > > > > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
> > > > > > > > > > ---
> > > > > > > > > > .../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?).
> > > 
> > > That does not really make sense. There is no relation between the LPO clock
> > > and the PCIe clocks so 1) being a requirement for probing the device looks
> > > odd. It also does not match past experience when I assisted Andy Green in
> > > getting this module up and running almost two years ago.
> > 
> > Well, first of all I can easily reproduce this on my RK3588 EVB1. I
> > intentionally ignore any bluetooth bits to avoid cross-effects from
> > bluetooth enabling any clocks / regulators / GPIOs and make sure the
> > RTC output clock is disabled at boot time (i.e. boot once without
> > any reference to the RTC clock and without 'clk_ignore_unused'
> > kernel argument). When booting up like this the WLAN device is not
> > visible in 'lspci' despite the WL_REG_ON GPIO being hogged. If I
> > additionally hack the RTC output clock to be enabled the WLAN device
> > becomes visible in 'lspci'.
> > 
> > The datasheet fully explains this:
> > 
> > https://www.lcsc.com/datasheet/lcsc_datasheet_2203281730_AMPAK-Tech-AP6275P_C2984107.pdf
> > 
> > PDF Page 23/24 (20/21 in the footer) has the Host Interface Timing
> > Diagram. WL_REG_ON should only be enabled after 2 cycles from LPO.
> > That means with LPO being disabled WL_REG_ON cannot be enabled. I'm
> > pretty sure WL_REG_ON means WLAN_REGULATOR_ON, so the logic is not
> > powered. On page 27 (24 in the footer) there is also a PCIe Power-On
> > Timing diagram, which shows that WL_REG_ON must be enabled before
> > the PCIe refclk is enabled.
> > 
> > So there is a specific power up sequence, which must be followed.
> 
> The chip also has an (less accurate) internal LPO so the 32khz sleep clock
> in the diagram does not have to be an external clock. Maybe Ampak
> bootstrapped the chip to disable the internal clock. Dunno.
> 
> What Andy needed back then to get firmware running was a change in the nvram
> file to force using the internal LPO, but the device was already visible on
> the PCIe bus.

mh, I just tested again and I can no longer reproduce the LPO
requirement for PCIe detection. Maybe it was something else all
along (I did most of my tests quite some time ago).
Sorry for the noise.

-- Sebastian
Arend Van Spriel July 31, 2024, 6:27 p.m. UTC | #11
On 7/31/2024 7:50 PM, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Jul 31, 2024 at 05:12:43PM GMT, Arend Van Spriel wrote:
>> On July 31, 2024 3:54:52 PM Sebastian Reichel
>> <sebastian.reichel@collabora.com> wrote:
>>> On Wed, Jul 31, 2024 at 02:57:37PM GMT, Arend van Spriel wrote:
>>>> On 7/30/2024 7:38 PM, Sebastian Reichel wrote:
>>>>> 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@kernel.org> wrote:
>>>>>>
>>>>>>> On 30/07/2024 11:52, Arend Van Spriel wrote:
>>>>>>>> On July 30, 2024 11:01:43 AM Krzysztof Kozlowski <krzk@kernel.org> 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@wesion.com> 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@broadcom.com>
>>>>>>>>>>> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
>>>>>>>>>>> ---
>>>>>>>>>>> .../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?).
>>>>
>>>> That does not really make sense. There is no relation between the LPO clock
>>>> and the PCIe clocks so 1) being a requirement for probing the device looks
>>>> odd. It also does not match past experience when I assisted Andy Green in
>>>> getting this module up and running almost two years ago.
>>>
>>> Well, first of all I can easily reproduce this on my RK3588 EVB1. I
>>> intentionally ignore any bluetooth bits to avoid cross-effects from
>>> bluetooth enabling any clocks / regulators / GPIOs and make sure the
>>> RTC output clock is disabled at boot time (i.e. boot once without
>>> any reference to the RTC clock and without 'clk_ignore_unused'
>>> kernel argument). When booting up like this the WLAN device is not
>>> visible in 'lspci' despite the WL_REG_ON GPIO being hogged. If I
>>> additionally hack the RTC output clock to be enabled the WLAN device
>>> becomes visible in 'lspci'.
>>>
>>> The datasheet fully explains this:
>>>
>>> https://www.lcsc.com/datasheet/lcsc_datasheet_2203281730_AMPAK-Tech-AP6275P_C2984107.pdf
>>>
>>> PDF Page 23/24 (20/21 in the footer) has the Host Interface Timing
>>> Diagram. WL_REG_ON should only be enabled after 2 cycles from LPO.
>>> That means with LPO being disabled WL_REG_ON cannot be enabled. I'm
>>> pretty sure WL_REG_ON means WLAN_REGULATOR_ON, so the logic is not
>>> powered. On page 27 (24 in the footer) there is also a PCIe Power-On
>>> Timing diagram, which shows that WL_REG_ON must be enabled before
>>> the PCIe refclk is enabled.
>>>
>>> So there is a specific power up sequence, which must be followed.
>>
>> The chip also has an (less accurate) internal LPO so the 32khz sleep clock
>> in the diagram does not have to be an external clock. Maybe Ampak
>> bootstrapped the chip to disable the internal clock. Dunno.
>>
>> What Andy needed back then to get firmware running was a change in the nvram
>> file to force using the internal LPO, but the device was already visible on
>> the PCIe bus.
> 
> mh, I just tested again and I can no longer reproduce the LPO
> requirement for PCIe detection. Maybe it was something else all
> along (I did most of my tests quite some time ago).
> Sorry for the noise.

Hi Sebastian,

Thanks for letting it know.

Regards,
Arend
diff mbox series

Patch

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
+
 required:
   - compatible
   - reg