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

From: Stefan Mavrodiev
Date: Tue Jan 23 2018 - 09:04:30 EST


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