diff mbox series

[v2,1/5] arm64: dts: rockchip: Add AP6275P wireless support to Khadas Edge 2

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

Commit Message

Jacobe Zang June 24, 2024, 8:19 a.m. UTC
Khadas Edge2 uses the PCI-e Ampak AP6275P 2T2R Wi-Fi 6 module.

Co-developed-by: Muhammed Efe Cetin <efectn@protonmail.com>
Signed-off-by: Muhammed Efe Cetin <efectn@protonmail.com>
Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
---
 .../boot/dts/rockchip/rk3588s-khadas-edge2.dts   | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Krzysztof Kozlowski June 24, 2024, 11:52 a.m. UTC | #1
On 24/06/2024 10:19, Jacobe Zang wrote:
> Khadas Edge2 uses the PCI-e Ampak AP6275P 2T2R Wi-Fi 6 module.
> 
> Co-developed-by: Muhammed Efe Cetin <efectn@protonmail.com>
> Signed-off-by: Muhammed Efe Cetin <efectn@protonmail.com>
> Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com>
> ---
>  .../boot/dts/rockchip/rk3588s-khadas-edge2.dts   | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
> index 3b6286461a746..f674deb6f7da8 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
> @@ -356,6 +356,22 @@ &pcie2x1l2 {
>  	reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>;
>  	vpcie3v3-supply = <&vcc3v3_pcie_wl>;
>  	status = "okay";
> +
> +	pcie@0,0 {
> +		reg = <0x400000 0 0 0 0>;
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		ranges;
> +		device_type = "pci";
> +		bus-range = <0x40 0x4f>;

Isn't bus-range a property of PCI host bridge, so the parent? This is a
PCI device, right?

> +
> +		wifi: wifi@0,0 {

Binding does not say anything about this. Rockchip PCI controller is the
PCI host bridge, isn't it? Then the pci@0,0 is the child, so what is this?

> +			reg = <0x410000 0 0 0 0>;
> +			clocks = <&hym8563>;
> +			clock-names = "32k";

1. Bindings are before the users.
2. Where is the compatible? Are you sure this validates?


> +		};
> +	};
> +

No need for this blank line.

>  };
>  
>  &pwm11 {

Best regards,
Krzysztof
Jacobe Zang June 25, 2024, 8:04 a.m. UTC | #2
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
>> index 3b6286461a746..f674deb6f7da8 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
>> @@ -356,6 +356,22 @@ &pcie2x1l2 {
>>        reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>;
>>        vpcie3v3-supply = <&vcc3v3_pcie_wl>;
>>        status = "okay";
>> +
>> +     pcie@0,0 {
>> +             reg = <0x400000 0 0 0 0>;
>> +             #address-cells = <3>;
>> +             #size-cells = <2>;
>> +             ranges;
>> +             device_type = "pci";
>> +             bus-range = <0x40 0x4f>;
>
>Isn't bus-range a property of PCI host bridge, so the parent? This is a
>PCI device, right?
>
>> +
>> +             wifi: wifi@0,0 {
>
>Binding does not say anything about this. Rockchip PCI controller is the
>PCI host bridge, isn't it? Then the pci@0,0 is the child, so what is this?

The host bridge is the parent of pcie@0,0. And pcie@0,0 is Bridge1, so the
wifi@0,0 as a device under the Bridge1.

>
>> +                     reg = <0x410000 0 0 0 0>;
>> +                     clocks = <&hym8563>;
>> +                     clock-names = "32k";
>
>1. Bindings are before the users.
>2. Where is the compatible? Are you sure this validates?

Before, the compatible is "pci14e4,449d", but when I checkpatch the warning
said that "pci14e4" was not documented, so I remove the compatible which 
doesn't affect the Wi-Fi function. I have tried to add "pci14e4" to 
vendor-prefixes.yaml but was refused. So whether should I add the compatible 
with warning? 

References: 
https://lore.kernel.org/all/20220629155956.1138955-15-nfraprado@collabora.com/

---
Best Regards
Jacobe
Krzysztof Kozlowski July 1, 2024, 8:41 a.m. UTC | #3
On 25/06/2024 10:04, Jacobe Zang wrote:
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
>>> index 3b6286461a746..f674deb6f7da8 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
>>> @@ -356,6 +356,22 @@ &pcie2x1l2 {
>>>         reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>;
>>>         vpcie3v3-supply = <&vcc3v3_pcie_wl>;
>>>         status = "okay";
>>> +
>>> +     pcie@0,0 {
>>> +             reg = <0x400000 0 0 0 0>;
>>> +             #address-cells = <3>;
>>> +             #size-cells = <2>;
>>> +             ranges;
>>> +             device_type = "pci";
>>> +             bus-range = <0x40 0x4f>;
>>
>> Isn't bus-range a property of PCI host bridge, so the parent? This is a
>> PCI device, right?
>>
>>> +
>>> +             wifi: wifi@0,0 {
>>
>> Binding does not say anything about this. Rockchip PCI controller is the
>> PCI host bridge, isn't it? Then the pci@0,0 is the child, so what is this?
> 
> The host bridge is the parent of pcie@0,0. And pcie@0,0 is Bridge1, so the

Do you want to say Rockchip PCI is PCI-PCI bridge? Bindings do not allow it.

> wifi@0,0 as a device under the Bridge1.
> 
>>
>>> +                     reg = <0x410000 0 0 0 0>;
>>> +                     clocks = <&hym8563>;
>>> +                     clock-names = "32k";
>>
>> 1. Bindings are before the users.
>> 2. Where is the compatible? Are you sure this validates?
> 
> Before, the compatible is "pci14e4,449d", but when I checkpatch the warning
> said that "pci14e4" was not documented, so I remove the compatible which 
> doesn't affect the Wi-Fi function. I have tried to add "pci14e4" to 
> vendor-prefixes.yaml but was refused. So whether should I add the compatible 
> with warning? 

I talk about dtbs_check, not checkpatch. That checkpatch warning does
not matter, obviously.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
index 3b6286461a746..f674deb6f7da8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-khadas-edge2.dts
@@ -356,6 +356,22 @@  &pcie2x1l2 {
 	reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie_wl>;
 	status = "okay";
+
+	pcie@0,0 {
+		reg = <0x400000 0 0 0 0>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		ranges;
+		device_type = "pci";
+		bus-range = <0x40 0x4f>;
+
+		wifi: wifi@0,0 {
+			reg = <0x410000 0 0 0 0>;
+			clocks = <&hym8563>;
+			clock-names = "32k";
+		};
+	};
+
 };
 
 &pwm11 {