Message-ID: <20240826234452.0d015548@minigeek.lan>
Date: Mon, 26 Aug 2024 23:44:52 +0100
From: Andre Przywara <andre.przywara@....com>
To: Kryštof Černý <cleverline1mc@...il.com>
Cc: wens@...e.org, Krzysztof Kozlowski <krzk@...nel.org>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jernej Skrabec <jernej.skrabec@...il.com>, Samuel
 Holland <samuel@...lland.org>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH PATCH] arm64: dts: sunxi: nanopi-neo-plus2: Add pio
 regulators

On Sat, 24 Aug 2024 16:24:41 +0200
Kryštof Černý <cleverline1mc@...il.com> wrote:

Hi Kryštof,

many thanks for taking care of those warnings and sending a patch,
that's the right way of fixing things and I wish more people would
actually do that!

I also checked the schematics and the H5 datasheet, so:

> Yes, you are right with vcc-pd, I misunderstood it, thank you. Let me 
> explain and ask about the rest:
> VDD_SYS_3.3V = reg_vcc3v3
> Groups PA, PE, PF are powered from vcc-io, according to Allwinner H5 
> datasheet, Vcc-io is connected to VDD_SYS_3.3V, just like mmc0. Should 
> those be set or omitted?

Yes, you can set those supplies to reg_vcc3v3.
It seems like most of the boards's I/O (expect PD) is actually 3.3V,
driven from that one discrete regulator, which is correctly described
as reg_vcc3v3. This isn't very clear in the DT (it looks like a "dummy"
regulator), which is what probably triggered Krzysztof's comment.
To make this more obvious, please change the regulator description as
seen in sun50i-h618-longanpi-3h.dts: There should be one 5V regulator,
as the external power input, from the MicroUSB port. Every other
regulator should be "chained" to that, via the vin-supply property.
And add a comment mentioning that it's a discrete regulator, maybe
even mentioning the chip name (SY8089A).

> vcc-pc (ball G15), which is labeled as vcc_io2 (probably wrong), which 
> is also connected to VDD_SYS_3.3V.

As Chen-Yu mentioned, VCC-PC is not mentioned in the schematics, but it
must be 3.3V: there is a pull-up for PC7, to 3.3V, also there is no
1.8V regulator, so the eMMC must work with 3.3V, supported by the
missing vqmmc-supply property for mmc2.
 
> vcc-pd (ball J15) is bonded to VDD_PHY_2.5V, which is always on and made 
> from VDD_SYS_3.3V, so I should make a new fixed regulator of name 
> "reg_gmac_2v5"? Mainline eth driver does not implement this pin, so it 
> would be used only for the pio.

Yes, please have another "regulator-fixed", feeding of reg_vcc3v3, and
mention it's a discrete RT9193.

> vcc-pg (ball H7) is also VDD_SYS_3.3V.

Yes, that's the same reg_vcc3v3.

> While PL is supplied from vcc-rtc (vcc_rtc, ball K6), it is connected 
> directly to the VDD_SYS_3.3V too, should this be the same regulator or a 
> new one or omitted too?

Since they are directly wired together, it's indeed the same reg_vcc3v3
regulator.

> Do you agree with my findings? I hope they are 100% now. If so, I will 
> try to make V2 with a new fixed regulator of 2.5V for the PD.
> 
> Many thanks for your replies, I will do better next time.

No worries, except for the VCC-PD at 2.5V this was actually all correct.
So well done, especially for a first try!

Cheers,
Andre


> 
> Dne 24. 08. 24 v 14:34 Chen-Yu Tsai napsal(a):
> > Hi,
> >
> > On Sat, Aug 24, 2024 at 5:08 PM Kryštof Černý <cleverline1mc@...il.com> wrote:  
> >> I am sorry if the message is wrong, this is my first patch ever sent to
> >> the Linux kernel. I have checked the schematic of the board and it
> >> shares the same power line with mmc0, so I assumed I can use the same
> >> regulator. Thanks for your feedback and I would be glad for your further
> >> response.  
> > So some of the pin groups do have dedicated supplies, and should thus be
> > described, but not all of them. The schematic only shows dedicated
> > supplies for PD and PG pingroups. So just add those. PD supply is from
> > 2.5V ethernet PHY I/O regulator supply, so you would need to add that
> > as well.
> >
> > The datasheet also mentions a separate supply pin for PC pingroup, but
> > it is not shown in the schematic. I would just omit that.
> >
> > And as Krzysztof mentioned, device tree changes should be to model
> > the hardware, not to work around some operating system warning. At
> > least most of the time that is. So your commit message should also
> > be about fixing the description or providing more detail, and not
> > about the operating system.
> >
> >
> > ChenYu
> >  
> >> Dne 24. 08. 24 v 9:40 Krzysztof Kozlowski napsal(a):  
> >>> On 24/08/2024 09:09, Kryštof Černý wrote:  
> >>>> The board does not have a dedicated regulator for pio and r_pio,
> >>>> but this fixes the kernel warning about dummy regulators being used.
> >>>> Tested on the actual board.
> >>>>  
> >>> Judging by commit msg these are not correct regulators. Please do not
> >>> add incorrect hardware description to silence some warnings coming from
> >>> OS. Either you need proper (correct) hardware description or fix the
> >>> problem other way, assuming there is anything to fix in the first place.
> >>>
> >>> Best regards,
> >>> Krzysztof
> >>>  
> 

Powered by blists - more mailing lists