Re: [PATCH 1/1] ARM: dts: sunxi: Add Olimex A20-SOM204-EVB board

[Stefan Mavrodiev]

On 01/20/2018 08:08 AM, Chen-Yu Tsai wrote:
> On Fri, Jan 19, 2018 at 9:27 PM, Stefan Mavrodiev <stefan@xxxxxxxxxx> wrote:
> > On 01/18/2018 04:28 PM, Chen-Yu Tsai wrote:
> > > On Thu, Jan 18, 2018 at 6:07 PM, Maxime Ripard
> > > <maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> > > > Hi!
> > > >
> > > > On Mon, Jan 15, 2018 at 12:07:34PM +0200, Stefan Mavrodiev wrote:
> > > > > > > +/dts-v1/;
> > > > > > > +#include "sun7i-a20.dtsi"
> > > > > > > +#include "sunxi-common-regulators.dtsi"
> > > > > > > +
> > > > > > > +
> > > > > > > +#include <dt-bindings/gpio/gpio.h>
> > > > > > > +#include <dt-bindings/interrupt-controller/irq.h>
> > > > > > > +#include <dt-bindings/pwm/pwm.h>
> > > > > > > +
> > > > > > > +/ {
> > > > > > > + model = "Olimex A20-SOM204-EVB";
> > > > > > > + compatible = "olimex,a20-olimex-som204-evb", "allwinner,sun7i-a20";
> > > > > > > +
> > > > > > > + aliases {
> > > > > > > +         serial0 = &uart0;
> > > > > > > +         serial1 = &uart4;
> > > > > > > +         serial2 = &uart7;
> > > > > > > +         spi0 = &spi1;
> > > > > > > +         spi1 = &spi2;
> > > > > > > +         ethernet1 = &rtl8723bs;
> > > > > > ethernet1? if there's a single network interface, it should be
> > > > > > ethernet0.
> > > > > I think this will conflict the gmac alias defined in sun7i-a20.dtsi:
> > > > >
> > > > > aliases {
> > > > >       ethernet0 = &gmac;
> > > > > };
> > > > We have that? That's bad, but you're right :)
> > > >
> > > > > > > +         stat {
> > > > > > > +                 label = "a20-som204:green:stat";
> > > > > > > +                 gpios = <&pio 8 0 GPIO_ACTIVE_HIGH>;
> > > > > > > +                 default-state = "on";
> > > > > > > +         };
> > > > > > > +
> > > > > > > +         led1 {
> > > > > > > +                 label = "a20-som204-evb:green:led1";
> > > > > > > +                 gpios = <&pio 8 10 GPIO_ACTIVE_HIGH>;
> > > > > > > +                 default-state = "on";
> > > > > > > +         };
> > > > > > > +
> > > > > > > +         led2 {
> > > > > > > +                 label = "a20-som204-evb:yellow:led2";
> > > > > > > +                 gpios = <&pio 8 11 GPIO_ACTIVE_HIGH>;
> > > > > > > +                 default-state = "on";
> > > > > > > +         };
> > > > > > You don't have the same prefix between stat and led1/led2. I'm fine
> > > > > > with both, but you should be consistent :)
> > > > > STAT led is on the SOM204 module, while led1/2 on the EVB. Thats why
> > > > > they have different prefix.
> > > > Still, the user and the system will see it as a single board, and the
> > > > documentation states that it should be the board name. I'm not quite
> > > > sure what a good rule would be here. Have you looked at how other
> > > > boards dealt with it? Chen-Yu, any opinion on this?
> > > Follow the bindings, I guess? I don't think we (sunxi) have dealt
> > > with modules that have LEDs or anything that needs to be named after
> > > the board.
> > >
> > > On a related topic, I don't know if you (Stefan / Olimex) want to split
> > > this into a .dtsi file for the SoM, and a .dts file for the EVB. It might
> > > help your customers?
> > I'm not sure this will be good ideal. We will have one EVB with all
> > possible peripheries. On the other hand, we are planning 3-4 different
> > SOM204 modules (A20, A64, RK....). I think this will make the dtsi
> > incompatible.
> Yes. That was what I mentioned in the second half of my reply.
>
> > Maybe if there is one dtsi for the base SOM204 module (one for each arch)
> > and
> > multiple dts for boards with additional features. But this will generate
> > 10-20
> > dts files. I think this will be better handled using overlays in the uboot.
> OK. I'm guessing there's the possibility that some pins or GPIOs get muxed
> to different functions depending on what base board is used? How would
> you list them, if you only had one .dts file, say for the EVB? Clearly
> the SoM cannot work by itself, so it probably doesn't get its own .dts
> file.
Yes, the SoM cannot work by itself. I'm thinking to follow the current 
practice:
ÂÂÂ - One dts for base board + evb
ÂÂÂ - One dts for the above + eMMC.

There is also possibility (a real one) some periphery to work with one 
SoM, and
other - not. For example A20-SOM204 or A64-SOM204 doesn't have PCIe 
support, but
RKxxxx-SOM204 will.

On second re-read of the comments:
> > +};
> > +
> > +&ahci {
> > +	target-supply = <&reg_ahci_5v>;
> You should use the regulators you defined in your PMIC there.
The power comes from the DC jack not from PCIM. In this case, is this OK?

> > +&usb_otg {
> > +	dr_mode = "otg";
> > +	status = "okay";
> > +};
> > +
> > +&usb_power_supply {
> > +	status = "okay";
> > +};
> > +
> > +&usbphy {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&usb0_id_detect_pin>,
> > +		    <&usb0_vbus_detect_pin>;
> > +	usb0_id_det-gpio = <&pio 7 4 GPIO_ACTIVE_HIGH>; /* PH4 */
> > +	usb0_vbus_det-gpio = <&pio 7 5 GPIO_ACTIVE_HIGH>; /* PH5 */
> > +	usb0_vbus_power-supply = <&usb_power_supply>;
> > +	usb0_vbus-supply = <&reg_usb0_vbus>;
> > +	usb1_vbus-supply = <&reg_usb1_vbus>;
> > +	usb2_vbus-supply = <&reg_usb2_vbus>;
> You should also use one of the PMIC regulators here.
Same here. Power comes from DC jack, not PMIC.

Regards,
Stefan Mavrodiev

> > About the leds, I'm ok to be named after full board name (a20-som204-evb).
> Cool.
>
> ChenYu
>
> > > I've tried it previously, and it helps in some ways
> > > when you're matching the files to the schematics. But it is confusing
> > > when you want the big picture. On the other hand, this is not going to
> > > help with supporting different modules on the same baseboard, as the
> > > routing, peripherals and labels likely won't match up. Just my two cents.
> > >
> > > ChenYu
> > Regards,
> > Stefan Mavrodiev