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@xxxxxxxxxxxxx> 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 = <®_cldo4>;
>> > + vqmmc-supply = <®_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
