Re: [PATCH v3 2/4] arm64: dts: allwinner: h700: Add RG35XX 2024 DTS

["Ryan Walklin" <ryan@testtoast.com>]

On Thu, 25 Apr 2024, at 1:58 PM, Chris Morgan wrote:
> On Thu, Apr 25, 2024 at 01:25:59AM +0100, Andre Przywara wrote:
>> On Wed, 24 Apr 2024 23:09:45 +1200
>> Ryan Walklin <ryan@testtoast.com> wrote:
>> 
>> Hi Ryan (and Chris),
>> 
>> many thanks for the changes, that looks really close now. Only a few
>> smaller comments this time.
>> 
Thanks both for the review.

>> > +&mmc2 {
>> > +	vmmc-supply = <&reg_cldo4>;
>> > +	vqmmc-supply = <&reg_aldo1>;
>> 
>> This is now fixed to 1.8V, which doesn't look right. Either it's not
>> the right regulator, or you should extend its range to cover 3.3V as
>> well.
>
> The IO is fixed at 1.8v (both the SDIO pins and the UART1 pins for
> bluetooth). If you raise this regulator too high the system becomes
> unstable.
>
Ideally LV signalling would work for UHS but unsure if that is achievable. FWIW the vendor BSP does refer to the vqmmc supply being ALDO1 but allows a range up to 3.5v. Will test out a 3.3v max and confirm it is unstable.


>> > +&r_rsb {
>> > +   status = "okay";
>> 
>> This is indented with spaces, not a tab.
Fixed, ta.

>> > +		regulators {
>> > +			reg_dcdc1: dcdc1 {
>> > +				regulator-always-on;
>> > +				regulator-boot-on;
>> 
>> boot-on doesn't make much sense here: that allows it to be turned off,
>> which we don't want. Also the binding documentation in regulator.yaml
>> says that it's only intended "where software cannot read the state of
>> the regulator", which is not true here.
>> regulator-always-on is all we need - technically speaking not even
>> that, since cpu0 is a consumer, but we need to play safe here.

Thanks for the explanation, this and others fixed.

>> > +			reg_aldo3: aldo3 {
>> > +				regulator-always-on;
>> > +				regulator-min-microvolt = <1800000>;
>> > +				regulator-max-microvolt = <1800000>;
>> > +				regulator-name = "axp717-aldo3";
>> 
>> So do we know for sure that's critical? And do we have any clue what
>> this supplies?
>> There is AVCC, VCC_HDMI, VCC_TV, VCC_RTC, all at 1.8V. The middle two
>> are not critical.
>> 
Unsure currently, but can try with the HDMI patchset and see if I can identify VCC_HDMI at least. At least one of the audio-codec-connected regulators is presumably AVCC for the amp.

>> > +			};
>> > +
>> > +			reg_aldo4: aldo4 { /* 5096000.codec */
>> > +				regulator-always-on;
>> 
>> Is that necessary? What happens if that is turned off? Looks like only
>> the WiFi and potentially audio is affected? I think it can go then,
>> also pg-supply would reference it, so it would effectively be enabled
>> anyways.
>
> I think this does something critical, as in my testing tinkering with
> this regulator or turning it off locks up the system.

Agreed, unclear what else it is powering but at least the G-bank of GPIOs and also possibly VCC18_DRAM for the DRAM controller?


>> > +			reg_cldo1: cldo1 { /* 5096000.codec */
>> > +				/* unused */
>> > +				regulator-min-microvolt = <3300000>;
>> > +				regulator-max-microvolt = <3300000>;
>> 
>> Looks a bit odd to have an "unused" comment, but also a voltage range
>> specified. Judging from the comment this might be supplying some audio
>> circuitry, which we don't need at the moment?

Thanks, have removed the range for now.

Ryan