Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: iefc945ae:Fastmail
Message-ID: <54e14c57-e30b-5a39-00ca-1dcc7b352844@tom-fitzhenry.me.uk>
Date:   Sun, 2 Oct 2022 20:33:55 +1100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [PATCH] arm64: dts: rockchip: add BT/wifi nodes to Pinephone Pro
Content-Language: en-US
To:     =?UTF-8?Q?Ond=c5=99ej_Jirman?= <megi@xff.cz>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Heiko Stuebner <heiko@sntech.de>, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
        phone-devel@vger.kernel.org
References: <20220906124713.1683587-1-tom@tom-fitzhenry.me.uk>
 <20220906133539.6ghjlzbs2ozgsa7v@core>
From:   Tom Fitzhenry <tom@tom-fitzhenry.me.uk>
In-Reply-To: <20220906133539.6ghjlzbs2ozgsa7v@core>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Ondřej,

Thanks for the review.

On 6/9/22 23:35, Ondřej Jirman wrote:
>> +	/* Power sequence for SDIO WiFi module */
>> +	sdio_pwrseq: sdio-pwrseq {
>> +		compatible = "mmc-pwrseq-simple";
>> +		clocks = <&rk818 1>;
>> +		clock-names = "ext_clock";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&wifi_enable_h_pin>;
>> +		post-power-on-delay-ms = <100>;
>> +		power-off-delay-us = <500000>;
> 
> Do we really need such long delays? Almost no boards in rockchip/ use such
> delays at all, and if they do they don't usually use power off delay.

I have checked the datasheet, and updated the delays accordingly with 
explanatory comments. This is applied in v2.

>>   &sdmmc {
> 
> see below
> 
>> @@ -380,6 +414,20 @@ &sdmmc {
>>   	status = "okay";
>>   };
>>   
>> +&sdio0 {
> 
> sd'i'o0 comes before 'm' in the alphabet.

Done. :)

> 
>> +	bus-width = <4>;
>> +	cap-sd-highspeed;
>> +	cap-sdio-irq;
>> +	disable-wp;
>> +	keep-power-in-suspend;
>> +	mmc-pwrseq = <&sdio_pwrseq>;
>> +	non-removable;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&sdio0_bus4 &sdio0_cmd &sdio0_clk>;
>> +	sd-uhs-sdr104;
>> +	status = "okay";
> 
> It might also be good to add the wifi node, and hookup the interrupt line and
> pinctrls, so that WoW works, while you're at it.
> 
> See eg. https://elixir.bootlin.com/linux/v5.19.7/source/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4b-plus.dts#L30
> 
> Looks like WIFI_HOST_WAKE_L is hooked to GPIO4_D0/PCIE_CLKREQnB_u according
> to the schematic. Let's hope GPIO4_D will consider 1.8V as high, because SoC
> GPIO4_D is in 3.0V domain and VDDIO of wifi chip is 1.8V.

As discussed off-list but included here for posterity, I'll leave this 
out of the DT for now, until we know the GPIO that the firmware is 
expecting.

Regards,
Tom