Re: [PATCH 3/3] arm64: dts: meson-sm1: add Banana PI BPI-M5 board dts

[Neil Armstrong]

Hi Martin,

On 29/04/2021 23:30, Martin Blumenstingl wrote:
> Hi Neil,
> 
> thanks for adding support for the BPI-M5!
> I had this on my TODO-list for a long time but didn't have enough time
> so far. so it's great to see this patch now :-)
Same, so it's nearly done now !

> 
> On Thu, Apr 29, 2021 at 7:04 PM Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
> [...]
>> +       adc_keys {
>> +               compatible = "adc-keys";
>> +               io-channels = <&saradc 2>;
>> +               io-channel-names = "buttons";
>> +               keyup-threshold-microvolt = <1800000>;
>> +
>> +               button-onoff {
> maybe just "key" (as you used below for SW1)?

Fixed

> 
>> +                       label = "SW3";
>> +                       linux,code = <BTN_3>;
>> +                       press-threshold-microvolt = <1700000>;
>> +               };
>> +       };
> 
>> +       /* TOFIX: handle CVBS_DET on SARADC channel 0 */
> it's great to see that after many years at least some boards finally
> can detect whether the CVBS connector is plugged it

Yes, it's also available on Odroid-N2(+), so now no excuse for not supporting it !

> 
>> +       memory@0 {
>> +               device_type = "memory";
>> +               reg = <0x0 0x0 0x0 0x40000000>;
>> +       };
> note to self: u-boot will update this from 1GiB to 4GiB
> 
>> +       flash_1v8: regulator-flash_1v8 {
>> +               compatible = "regulator-fixed";
>> +               regulator-name = "FLASH_1V8";
> BPI-M5-SCH-V10-Release.pdf schematics are calling this EMMC_1V8. I
> suggest to use that name so it's easier to compare this with the
> schematics in the future

Done

> 
>> +               regulator-min-microvolt = <1800000>;
>> +               regulator-max-microvolt = <1800000>;
>> +               vin-supply = <&vddao_3v3>;
>> +               regulator-always-on;
>> +       };
>> +
>> +       dc_in: regulator-dc_in {
>> +               compatible = "regulator-fixed";
>> +               regulator-name = "5V";
> maybe use "DC_IN" here as well so the node-name matches what we see in
> userspace/kernel logs?

Done

> 
>> +       vddcpu: regulator-vddcpu {
>> +               /*
>> +                * SY8120B1ABC DC/DC Regulator.
>> +                */
>> +               compatible = "pwm-regulator";
>> +
>> +               regulator-name = "VDDCPU";
>> +               regulator-min-microvolt = <721000>;
>> +               regulator-max-microvolt = <1022000>;
> the vendor .dts has:
>   regulator-min-microvolt = <690000>;
>   regulator-max-microvolt = <1050000>;
> which also matches meson-sm1-sei610.dts (which uses the same regulator IC)

Done, thanks

> 
> [...]
>> +&ethmac {
>> +       pinctrl-0 = <&eth_pins>, <&eth_rgmii_pins>;
>> +       pinctrl-names = "default";
>> +       status = "okay";
>> +       phy-mode = "rgmii";
>> +       phy-handle = <&external_phy>;
>> +       amlogic,tx-delay-ns = <2>;
> I haven't checked their u-boot code but some of the newer Amlogic BSPs
> seem to let the PHY add the TX delay (which is also what the netdev
> maintainers recommend)
> that can be achieved by setting:
>   phy-mode = "rgmii-txid";
> and deleting the "amlogic,tx-delay-ns" property

Done, but what about when we use mainline U-boot here ?

> 
>> +&gpio {
>> +       gpio-line-names =
>> +               /* GPIOZ */
>> +               "", "", "", "", "", "", "", "",
> GPIOZ_0 to GPIOZ_15:
> - ETH_MDIO
> - ETH_MDC
> - ETH_RXCLK
> - ETH_RX_DV
> - ETH_RXD0
> - ETH_RXD1
> - ETH_RXD2
> - ETH_RXD3
> - ETH_TXCLK
> - ETH_TXEN
> - ETH_TXD0
> - ETH_TXD1
> - ETH_TXD2
> - ETH_TXD3
> - ETH_INTR
> - ETH_NRST
> 
>> +               /* GPIOH */
>> +               "", "", "", "", "",
> GPIOH_0 to GPIOH_4:
> - HDMI_SDA
> - HDMI_SCL
> - HDMI_HPD
> - HDMI_CEC
> - VL-RST_N
> 
>> +               "CON1-P36", /* GPIOH_5 */
> GPIOH_6 to GPIOH_8:
> - VL-PWREN
> - WiFi_3V3_1V8
> - TFLASH_VDD_EN
> 
>> +               /* BOOT */
>> +               "", "", "", "", "", "", "", "",
>> +               "", "", "", "", "", "", "", "",
> BOOT_0 to BOOT_13:
> - eMMC_D0
> - eMMC_D1
> - eMMC_D2
> - eMMC_D3
> - eMMC_D4
> - eMMC_D5
> - eMMC_D6
> - eMMC_D7
> - eMMC_CLK
> - (BOOT_9 is unused)
> - eMMC_CMD
> - (BOOT_11 is unused)
> - eMMC_RST#
> - eMMC_DS
> 
>> +               /* GPIOC */
>> +               "", "", "", "", "", "", "", "",
> GPIOC_0 to GPIOC_7:
> - SD_D0_B
> - SD_D1_B
> - SD_D2_B
> - SD_D3_B
> - SD_CLK_B
> - SD_CMD_B
> - CARD_EN_DET
> - (GPIOC_7 is unused)
> 
> 
>> +&gpio_ao {
>> +       gpio-line-names =
>> +               /* GPIOAO */
>> +               "DEBUG TX", /* GPIOAO_0 */
>> +               "DEBUG RX", /* GPIOAO_1 */
>> +               "SYS_LED2", /* GPIOAO_2 */
>> +               "UPDATE_KEY", /* GPIOAO_3 */
>> +               "CON1-P40", /* GPIOAO_4 */
>> +               "",
> GPIOAO_5 is IR_IN
> 
>> +               "TF_3V3N_1V8_EN", /* GPIOAO_6 */
>> +               "CON1-P35", /* GPIOAO_7 */
>> +               "CON1-P12", /* GPIOAO_8 */
>> +               "CON1-P37", /* GPIOAO_9 */
>> +               "CON1-P38", /* GPIOAO_10 */
>> +               "SYS_LED", /* GPIOAO_11 */
>> +               /* GPIOE */
>> +               "", "", "";
> GPIOE_0 to GPIOOE_2:
> - VDDEE_PWM
> - VDDCPU_PWM
> - TF_PWR_EN

I was to lazy to do that, so thanks !

> 
>> +&usb2_phy0 {
>> +       phy-supply = <&dc_in>;
>> +};
>> +
>> +&usb2_phy1 {
>> +       /* Enable the hub which is connected to this port */
>> +       phy-supply = <&vl_pwr_en>;
>> +};
> I think technically we'd need to use AVDD18_USB here (that said, I
> chose the same approach as you are using here before..)
> USB bus devices (AFAIK) still need to be detected before any
> device-tree properties can be applied and there's (again AFAIK) still
> no pwrseq concept.
> so I guess for now this is the way to go as there's enough comments
> about it in your patch

yeah we'll see if there is proper pwrseq method to fix that (and the hub reset)

Thanks for the review,

Neil

> 
> 
> Best regards,
> Martin
>