Re: [PATCH v3 2/2] arm64: dts: allwinner: a64: Add WiFi/BT header on SoPine

[Dragan Simic <dsimic@manjaro.org>]

Hello Chen-Yu,

On 2025-04-20 03:59, Chen-Yu Tsai wrote:
> On Sun, Apr 20, 2025 at 3:57 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> 
>> On 2025-04-19 21:26, Peter Robinson wrote:
>> > On Sat, 19 Apr 2025 at 19:25, Dragan Simic <dsimic@manjaro.org> wrote:
>> >> On 2025-04-19 18:00, Peter Robinson wrote:
>> >>> This adds all the pin mappings on the WiFi/BT header on
>> >>> the SoPine baseboard/A64-LTS. They're disabled by default
>> >>> as the modules don't ship by default. This includes, where
>> >>> they haven't been already, UART1 for BT and mmc1 for WiFi.
>> >>
>> >> The patch subject should be improved a bit, to include
>> >> "Baseboard" as well.  Having just "SoPine" is a bit too
>> >> vague, and it should actually be written as "SOPINE"
>> >> at all places in the prose.
>> >>
>> >>> Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
>> >>> ---
>> >>> .../allwinner/sun50i-a64-sopine-baseboard.dts | 25
>> >> +++++++++++++++++++
>> >>> 1 file changed, 25 insertions(+)
>> >>>
>> >>> diff --git
>> >>> a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
>> >>> b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
>> >>> index be2347c8f267..64d696f110ee 100644
>> >>> ---
>> >> a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
>> >>> +++
>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
>> >>> @@ -42,6 +42,11 @@ reg_vcc1v8: vcc1v8 {
>> >>> regulator-min-microvolt = <1800000>;
>> >>> regulator-max-microvolt = <1800000>;
>> >>> };
>> >>> +
>> >>> +     wifi_pwrseq: pwrseq {
>> >>> +             compatible = "mmc-pwrseq-simple";
>> >>> +             reset-gpios = <&r_pio 0 2 GPIO_ACTIVE_LOW>; /* PL2
>> >> */
>> >>> +     };
>> >>
>> >> Is there a reason why the status of this node isn't set to
>> >> "disabled"?  Or even better, why don't we move this node
>> >> entirely into the proposed DT overlay?
>> >>
>> >> The required reset procedure actually depends on what's
>> >> found on the add-on module, so it should belong to the DT
>> >> overlay that defines the add-on module.
>> >
>> > But the pin itself is pinned out through the header on the device.
>> 
>> Good point.  On second thought, the right approach should be
>> to reserve the reset pin through pinctrl additions to the
>> board dts files, but to move the "mmc-pwrseq-simple" nodes
>> to the proposed add-on overlays.

Actually, s/overlays/overlay/

>> 
>> You can have a look at rk3399-pinebook-pro.dts for an example
>> of such a pinctrl definition and its use.
> 
> For sunxi, we don't add pinctrl nodes for GPIO usage.

I see, thanks for the note.  In that case, I'd still suggest
that the "mmc-pwrseq-simple" nodes are moved to the proposed
add-on overlay, because that's where they belong to.

In theory, some add-on module may actually not need resetting,
so defining that should belong to what defines the specific
add-on module, which is the add-on overlay.

>> >>> };
>> >>>
>> >>> &ac_power_supply {
>> >>> @@ -103,6 +108,18 @@ ext_rgmii_phy: ethernet-phy@1 {
>> >>> };
>> >>> };
>> >>>
>> >>> +/* On Wifi/BT connector */
>> >>> +&mmc1 {
>> >>> +     pinctrl-names = "default";
>> >>> +     pinctrl-0 = <&mmc1_pins>;
>> >>> +     vmmc-supply = <&reg_dldo4>;
>> >>> +     vqmmc-supply = <&reg_eldo1>;
>> >>> +     mmc-pwrseq = <&wifi_pwrseq>;
>> >>
>> >> Of course, the "mmc-pwrseq" property would then also be moved
>> >> to the DT overlay that defines the add-on module.
>> >>
>> >>> +     bus-width = <4>;
>> >>> +     non-removable;
>> >>> +     status = "disabled";
>> >>> +};
>> >>> +
>> >>> &mmc2 {
>> >>> pinctrl-names = "default";
>> >>> pinctrl-0 = <&mmc2_pins>;
>> >>> @@ -175,6 +192,14 @@ &uart0 {
>> >>> status = "okay";
>> >>> };
>> >>>
>> >>> +/* On Wifi/BT connector, with RTS/CTS */
>> >>> +&uart1 {
>> >>> +     pinctrl-names = "default";
>> >>> +     pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
>> >>> +     uart-has-rtscts;
>> >>> +     status = "disabled";
>> >>> +};
>> >>> +
>> >>> /* On Pi-2 connector */
>> >>> &uart2 {
>> >>> pinctrl-names = "default";