Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 1/1] ARM: dts: sunxi: Add Olimex A20-SOM204-EVB board
To:     Chen-Yu Tsai <wens@csie.org>, Stefan Mavrodiev <stefan@olimex.com>
Cc:     Maxime Ripard <maxime.ripard@free-electrons.com>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Russell King <linux@armlinux.org.uk>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "moderated list:ARM PORT" <linux-arm-kernel@lists.infradead.org>,
        open list <linux-kernel@vger.kernel.org>,
        linux-sunxi <linux-sunxi@googlegroups.com>
References: <1515747666-6597-1-git-send-email-stefan@olimex.com>
 <20180115095031.pzsgrp3tdmemotrs@flea.lan>
 <1b9667a4-75de-4ece-069d-7ec33b7e2f8d@olimex.com>
 <20180118100745.y62adzvfnrzqkagd@flea.lan>
 <CAGb2v66U9ziFhU5n6haTA3LGNX7BavmfXiiCmvebtqaMxcN0-w@mail.gmail.com>
 <548d57b7-4db1-a9ee-369e-9067f981162d@olimex.com>
 <CAGb2v67BARS-kOH06DzkYH-_7h_18yrEpc0c_diXVvRxZPn_4Q@mail.gmail.com>
From:   Stefan Mavrodiev <stefan@olimex.com>
Message-ID: <bfe19092-a43f-be80-0ee4-c76d45208c24@olimex.com>
Date:   Tue, 23 Jan 2018 16:04:14 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <CAGb2v67BARS-kOH06DzkYH-_7h_18yrEpc0c_diXVvRxZPn_4Q@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 01/20/2018 08:08 AM, Chen-Yu Tsai wrote:
> On Fri, Jan 19, 2018 at 9:27 PM, Stefan Mavrodiev <stefan@olimex.com> 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@free-electrons.com> 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