lkml.org 
[lkml]   [2021]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 3/3] arm64: dts: meson-sm1: add Banana PI BPI-M5 board dts
    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 :-)

    On Thu, Apr 29, 2021 at 7:04 PM Neil Armstrong <narmstrong@baylibre.com> 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)?

    > + 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

    > + 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

    > + 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?

    > + 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)

    [...]
    > +&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

    > +&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

    > +&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


    Best regards,
    Martin

    \
     
     \ /
      Last update: 2021-04-29 23:33    [W:3.178 / U:0.700 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site