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

[Stefan Mavrodiev <stefan-kyXcfZUBQGPQT0dZR+AlfA@public.gmane.org>]

On 01/20/2018 08:08 AM, Chen-Yu Tsai wrote:
> On Fri, Jan 19, 2018 at 9:27 PM, Stefan Mavrodiev <stefan-kyXcfZUBQGPQT0dZR+AlfA@public.gmane.org> 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html