Re: [PATCH v5 3/4] arm64: dts: rockchip: Add devicetree for the FriendlyElec NanoPi R28S

[安容]

Hi Jonas,

Thanks for the review, and for going through the schematic.

>> +  aliases {
>> +          mmc2 = &sdio0;
>
> This looks to be missing an serial2 alias:
>
>     serial2 = &uart2;

Added in v6.

>> +          pinctrl-0 = <&usr_btn>;
>
> This is named gpio4_b2_usr_btn in the schematics.

You are right, the net is GPIO4_B2_USR_BTN. v6 renames the label to
gpio4_b2_usr_btn and the node to gpio4-b2-usr-btn. The other pin
configuration labels (led1, led2, led_sys, pwr_wf, pcie_eth_perstb)
is already match their nets.

> This is missing nodes for bluetooth:
>
>     bluetooth {
>         bt_wake_host: bt-wake-host {
[...]
> Suggest you also add host_wake_wl:
[...]
> and wl_wake_host:

I checked all four against the schematic and the pins are right:
HOST_WAKE_WL on GPIO1_A6, WL_WAKE_HOST on GPIO1_A7, HOST_WAKE_BT on
GPIO1_B4, BT_WAKE_HOST on GPIO1_B5, all of them wired to the module on
sheet 16.

I still left them out. The AIC8800D80 has no upstream driver, so there
is no wifi@1 node and no bluetooth node under &uart2 to reference these
pin configurations from, and a pinctrl state that nothing references is
never applied. I would rather add them in the patch that adds those
nodes where they can be tested. If you want them in now for the NEO3
Plus work, point it out so and I will add them in the next revision.

>> +&sdio0 {
>
> We can likely be explicit and declare:
>
>     no-mmc;
>     no-sd;
>
> This is missing the vmmc-supply used.
>
>     vmmc-supply = <&vcc_3v3>;

They are both added in v6. The module's VBAT comes from VCC_3V3 through a ferrite
bead and VDDIO from VCC_1V8, which matches the vqmmc-supply that was
already there.

>> +&uart2 {
>
> We can likely declare dma-names here:
>
>     dma-names = "tx", "rx";

Added in v6.

>> +  dr_mode = "peripheral";
>
> Why do you disable dual role here? Is it not possible to have it work in
> host mode? E.g. using a USB hub with power pass-thru?

The port is wired as a device only, see sheet 15:

- Both CC pins have a 5.1k pull-down, R3811 and R3812, so the port always
  presents itself as a UFP. There is no CC controller and no Rp that
  could advertise a DFP.
- USB20_OTG0_ID is not connected, R3922 is /NC, and the sheet has a
  "Slave Mode" note right next to it.
- VBUS is an input. It goes through the input protection into
  VCC5V0_SYS, which powers the whole board. Nothing can source 5V onto
  the connector.

So behind a powered hub the board is powered, but it still cannot
present itself as a host to the other side. v6 keeps
dr_mode = "peripheral" and states the above in a comment above it.

Your comments on the other two patches are in v6 as well: the USB nodes
stay in the board devicetrees, and the binding lists the R28S and the
Zero2 in a single entry. The coding style patch is dropped -- Diederik,
for the record, only 18 of the ~2900 "status" properties under
arch/arm64/boot/dts/rockchip are preceded by a blank line, so the series
now follows what the rockchip devicetrees do instead.

Looking forward to the NEO3 Plus series, and enjoy the R28S once it
arrives.

Regards,
AnRong