Re: [draft2 PATCH] ARM: dts: sun8i: r40: add devicetree for FETA40i-C/OKA40i-C

[Ivan Uvarov <i.uvarov@cognitivepilot.com>]

On Wed, 3 Mar 2021 13:08:34 +0000
Andre Przywara <andre.przywara@arm.com> wrote:

> On Tue, 2 Mar 2021 13:54:15 +0300
> Ivan Uvarov <i.uvarov@cognitivepilot.com> wrote:
> 
> Hi Ivan,
> 
> many thanks for spending the time in piecing this together and caring
> about upstreaming!
> 

Many more thanks to you for helping me out with the process. I wouldn't
have gotten anywhere near this far without your help.

> 
> Should be a TAB here.
> 
> And I don't know if this is just my client, but someone mangled equal
> signs into "=3D" everywhere :-(
> I am trying to ignore this for now ...
> Can you force pure text email in Thunderbird? Or use git send-email?
> 

I was using Thunderbird's plain text mode when composing this; looks
like it imposes its own modifications anyway. I've switched to
Claws now, hopefully it'll handle the next iteration more
gracefully.


> > +&reg_dldo1 {
> > +	regulator-always-on;
> > +	regulator-min-microvolt =3D <3300000>;
> > +	regulator-max-microvolt =3D <3300000>;
> > +	regulator-name =3D "vcc-wifi-io";
> > +};
> > +
> > +&reg_dldo4 {
> > +	regulator-always-on;  
> 
> Does this really need to be always on? If it's just for SATA, I'd
> expect this to be enabled by the driver when needed? As you correctly
> do in the ahci node below.
I'm not sure, to be honest. I haven't tested without it. I should
probably try to.

> > +	regulator-min-microvolt =3D <2500000>;
> > +	regulator-max-microvolt =3D <2500000>;
> > +	regulator-name =3D "vdd2v5-sata";  
> 
> Does this end prematurely here?

It does. Looks like I deleted the last line of the file somehow while
creating the patch. 


> > +
> > +&mmc0 {
> > +	vmmc-supply =3D <&reg_dcdc1>;
> > +	vqmmc-supply =3D <&reg_dcdc1>;
> > +	bus-width =3D <4>;
> > +	cd-gpios =3D <&pio 8 11 GPIO_ACTIVE_LOW>; // PI11
> > +	status =3D "okay";
> > +};
> > +
> > +&mmc1 {  
> 
> So this is the SDIO connector on the board, right? Which is just a set
> of header pins? 
Yes, that's it.

> Not sure we should have it in here, then.
OK.
 
> What about MMC3, which is apparently connected to a microSD slot (TF
> card in the schematic)?

I never needed it, so never added it. Probably should.

> 
> > +	regulator-min-microvolt =3D <3300000>;
> > +	regulator-max-microvolt =3D <3300000>;
> > +	regulator-name =3D "vcc-wifi";
> > +};
> > +
> > +&reg_dldo3 { // possibly unneeded  
> 
> If it's unneeded, you can drop it. If it turns out be needed later on,
> we can always add it. Did the board work without it?
> Did you try Wifi? We should not add untested features.

WiFi does work without any hiccups on my board with this DTS. I haven't
tested with the regulator disabled.

> > +	// regulator-always-on;
> > +	regulator-min-microvolt =3D <3300000>;
> > +	regulator-max-microvolt =3D <3300000>;
> > +	regulator-name =3D "vcc-wifi-2";
> > +};
> > +
> > +&reg_eldo2 {
> > +	regulator-always-on;  
> 
> This shouldn't be always on. I guess this AXP pin is connected to the
> VDD-SATA pin on the SoC, so it just drives the integrated SATA PHY.
> Which probably means that this node belong into the SoM .dtsi, as the
> connection is on the SoM. You just reference it from the ahci node
> here.
> 
> > +	regulator-min-microvolt =3D <1200000>;
> > +	regulator-max-microvolt =3D <1200000>;
> > +	regulator-name =3D "vdd1v2-sata";
> > +};
> > +
> > +&reg_eldo3 {
> > +	regulator-always-on;  
> 
> Again, why always on? Do you know where this is connected to?
> If this is VCC_PE on the SoC, then I wonder if this should be moved to
> the SoM .dtsi, with a range between 1.8V and 3.3V, since those are
> valid input voltages for the VCC_PE pin. And then here you overwrite
> this with 2.8V, as this is apparently used for the CSI connector on
> the board, which seems to work on 2.8V.
> But I am not sure if this is too much, and just having this node here
> is sufficient.

I'll look into both of those.

> > +	regulator-min-microvolt =3D <2800000>;
> > +	regulator-max-microvolt =3D <2800000>;
> > +	regulator-name =3D "vcc-pe";
> > +};
> > +
> > +&tcon_tv0 {
> > +	status =3D "okay";
> > +};
> > +
> > +&uart0 {
> > +	pinctrl-names =3D "default";
> > +	pinctrl-0 =3D <&uart0_pb_pins>;
> > +	status =3D "okay";
> > +};
> > +
> > +&uart3 {  
> 
> Please add uart3 to the aliases section then.

Sure. 

I think I'll post a new version of the patch tomorrow, which should fix
formatting and address the issues you've raised that can be fixed
immediately without much further testing. It will also include the
other uarts that are available on the devboard (2,4 and 7).


Thank you for your assistance once more, you've been tremendously
helpful.