Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS

[Ayush Singh <ayush@beagleboard.org>]

On 6/27/24 22:37, Andrew Davis wrote:
> On 6/27/24 11:26 AM, Ayush Singh wrote:
>> DONOTMERGE
>>
>> Add mikroBUS connector and some mikroBUS boards support for Beagleplay.
>> The mikroBUS boards node should probably be moved to a more appropriate
>> location but I am not quite sure where it should go since it is not
>> dependent on specific arch.
>>
>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>>   arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 94 
>> +++++++++++++++++++++++---
>>   1 file changed, 86 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts 
>> b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>> index 70de288d728e..3f3cd70345c4 100644
>> --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>> +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
>> @@ -38,6 +38,7 @@ aliases {
>>           serial2 = &main_uart0;
>>           usb0 = &usb0;
>>           usb1 = &usb1;
>> +        mikrobus0 = &mikrobus0;
>>       };
>>         chosen {
>> @@ -227,6 +228,56 @@ simple-audio-card,codec {
>>           };
>>       };
>>   +    mikrobus0: mikrobus-connector {
>> +        compatible = "mikrobus-connector";
>> +        pinctrl-names = "default", "pwm_default", "pwm_gpio",
>> +                "uart_default", "uart_gpio", "i2c_default",
>> +                "i2c_gpio", "spi_default", "spi_gpio";
>> +        pinctrl-0 = <&mikrobus_gpio_pins_default>;
>> +        pinctrl-1 = <&mikrobus_pwm_pins_default>;
>> +        pinctrl-2 = <&mikrobus_pwm_pins_gpio>;
>> +        pinctrl-3 = <&mikrobus_uart_pins_default>;
>> +        pinctrl-4 = <&mikrobus_uart_pins_gpio>;
>> +        pinctrl-5 = <&mikrobus_i2c_pins_default>;
>> +        pinctrl-6 = <&mikrobus_i2c_pins_gpio>;
>> +        pinctrl-7 = <&mikrobus_spi_pins_default>;
>> +        pinctrl-8 = <&mikrobus_spi_pins_gpio>;
>> +
>> +        mikrobus-gpio-names = "pwm", "int", "rx", "tx", "scl", "sda",
>> +                      "mosi", "miso", "sck", "cs", "rst", "an";
>> +        mikrobus-gpios = <&main_gpio1 11 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 9 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 24 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 25 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 22 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 23 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 7 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 8 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 14 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 13 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 12 GPIO_ACTIVE_HIGH>,
>> +                 <&main_gpio1 10 GPIO_ACTIVE_HIGH>;
>> +
>> +        spi-controller = <&main_spi2>;
>> +        spi-cs = <0>;
>> +        spi-cs-names = "default";
>> +
>> +        board = <&lsm6dsl_click>;
>> +    };
>> +
>> +    mikrobus_boards {
>> +        thermo_click: thermo-click {
>> +            compatible = "maxim,max31855k", "mikrobus-spi";
>
> I might be missing something, but your solution cannot possibly be
> to list every click board that could be connected (all 1500+ of them)
> to every mikroBUS connector on every device's DT file..


I think you missed something. `mikrobus-boards` is not a child node of 
`mikrobus0`. See the `board` property in `mikrobus0`. That is what 
selects the board attached to the connector.

The `mikcrobus-boards` node itself should be moved to some independent 
location and included from a system that wants to support mikrobus 
boards. The connector will only have a phandle to the board (or boards 
in case a single mikroBUS board has 1 i2c and 1 spi sensor or some 
combination).


>
> Each click board should have a single DTSO overlay file to describe the
> click board, one per click board total. And then that overlay should
> apply cleanly to any device that has a mikroBUS interface.


Yes, that is the goal.


>
> Which means you have not completely solved the fundamental problem of
> abstracting the mikroBUS connector in DT. Each of these click device 
> child
> nodes has to be under the parent connector node. Which means a phandle
> to the parent node, which is not generically named. For instance
> if my board has 2 connectors, I would have mikrobus0 and mikrobus1,
> the click board's overlay would look like this:
>
> /dts-v1/;
> /plugin/;
>
> &mikrobus0 {
>     status = "okay";
>
>     mikrobus_board {
>         thermo-click {
>             compatible = "maxim,max31855k", "mikrobus-spi";
>             spi-max-frequency = <1000000>;
>             pinctrl-apply = "spi_default";
>         };
>     };
> };


No, it will look as follows:

```

&mikrobus0 {
     status = "okay";

     board = <&thermo-click>;

};


&mikrobus_board {
         thermo-click {
             compatible = "maxim,max31855k", "mikrobus-spi";
             spi-max-frequency = <1000000>;
             pinctrl-apply = "spi_default";
         };
   };

```


>
> I think this solution is almost there, but once you solve the above
> issue, we could just apply the right overlay for our attached click
> board ahead of time and not need the mikroBUS bus driver at all.


Well, the driver is still needed because some things cannot be done 
generically in dt. Eg:

1. SPI chipselect. Each connector will have different chipselect number 
mapped to CS pin. In fact a mikrobus board might use other pins like RST 
as chipselect as well.

2. Using pins other than their intended purpose like GPIO.


>
> Andrew
>
>> +            spi-max-frequency = <1000000>;
>> +            pinctrl-apply = "spi_default";
>> +        };
>> +
>> +        lsm6dsl_click: lsm6dsl-click {
>> +            compatible = "st,lsm6ds3", "mikrobus-spi";
>> +            spi-max-frequency = <1000000>;
>> +            pinctrl-apply = "spi_default";
>> +        };
>> +    };
>>   };
>>     &main_pmx0 {
>> @@ -387,6 +438,18 @@ AM62X_IOPAD(0x01f0, PIN_OUTPUT, 5) /* (A18) 
>> EXT_REFCLK1.CLKOUT0 */
>>           >;
>>       };
>>   +    mikrobus_pwm_pins_default: mikrobus-pwm-default-pins {
>> +        pinctrl-single,pins = <
>> +            AM62X_IOPAD(0x01a4, PIN_INPUT, 2) /* (B20) 
>> MCASP0_ACLKX.ECAP2_IN_APWM_OUT */
>> +        >;
>> +    };
>> +
>> +    mikrobus_pwm_pins_gpio: mikrobus-pwm-gpio-pins {
>> +        pinctrl-single,pins = <
>> +            AM62X_IOPAD(0x01a4, PIN_INPUT, 7) /* (B20) 
>> MCASP0_ACLKX.GPIO1_11 */
>> +        >;
>> +    };
>> +
>>       mikrobus_i2c_pins_default: mikrobus-i2c-default-pins {
>>           pinctrl-single,pins = <
>>               AM62X_IOPAD(0x01d0, PIN_INPUT_PULLUP, 2) /* (A15) 
>> UART0_CTSn.I2C3_SCL */
>> @@ -394,6 +457,13 @@ AM62X_IOPAD(0x01d4, PIN_INPUT_PULLUP, 2) /* 
>> (B15) UART0_RTSn.I2C3_SDA */
>>           >;
>>       };
>>   +    mikrobus_i2c_pins_gpio: mikrobus-i2c-gpio-pins {
>> +        pinctrl-single,pins = <
>> +            AM62X_IOPAD(0x01d0, PIN_INPUT, 7) /* (A15) 
>> UART0_CTSn.GPIO1_22 */
>> +            AM62X_IOPAD(0x01d4, PIN_INPUT, 7) /* (B15) 
>> UART0_RTSn.GPIO1_23 */
>> +        >;
>> +    };
>> +
>>       mikrobus_uart_pins_default: mikrobus-uart-default-pins {
>>           pinctrl-single,pins = <
>>               AM62X_IOPAD(0x01d8, PIN_INPUT, 1) /* (C15) 
>> MCAN0_TX.UART5_RXD */
>> @@ -401,6 +471,13 @@ AM62X_IOPAD(0x01dc, PIN_OUTPUT, 1) /* (E15) 
>> MCAN0_RX.UART5_TXD */
>>           >;
>>       };
>>   +    mikrobus_uart_pins_gpio: mikrobus-uart-gpio-pins {
>> +        pinctrl-single,pins = <
>> +            AM62X_IOPAD(0x01d8, PIN_INPUT, 7) /* (C15) 
>> MCAN0_TX.GPIO1_24 */
>> +            AM62X_IOPAD(0x01dc, PIN_INPUT, 7) /* (E15) 
>> MCAN0_RX.GPIO1_25 */
>> +        >;
>> +    };
>> +
>>       mikrobus_spi_pins_default: mikrobus-spi-default-pins {
>>           pinctrl-single,pins = <
>>               AM62X_IOPAD(0x01b0, PIN_INPUT, 1) /* (A20) 
>> MCASP0_ACLKR.SPI2_CLK */
>> @@ -410,6 +487,15 @@ AM62X_IOPAD(0x0198, PIN_INPUT, 1) /* (A19) 
>> MCASP0_AXR2.SPI2_D1 */
>>           >;
>>       };
>>   +    mikrobus_spi_pins_gpio: mikrobus-spi-gpio-pins {
>> +        pinctrl-single,pins = <
>> +            AM62X_IOPAD(0x0194, PIN_INPUT, 7) /* (B19) 
>> MCASP0_AXR3.GPIO1_7 */
>> +            AM62X_IOPAD(0x0198, PIN_INPUT, 7) /* (A19) 
>> MCASP0_AXR2.GPIO1_8 */
>> +            AM62X_IOPAD(0x01ac, PIN_INPUT, 7) /* (E19) 
>> MCASP0_AFSR.GPIO1_13 */
>> +            AM62X_IOPAD(0x01b0, PIN_INPUT, 7) /* (A20) 
>> MCASP0_ACLKR.GPIO1_14 */
>> +        >;
>> +    };
>> +
>>       mikrobus_gpio_pins_default: mikrobus-gpio-default-pins {
>>           bootph-all;
>>           pinctrl-single,pins = <
>> @@ -630,8 +716,6 @@ &main_gpio0 {
>>     &main_gpio1 {
>>       bootph-all;
>> -    pinctrl-names = "default";
>> -    pinctrl-0 = <&mikrobus_gpio_pins_default>;
>>       gpio-line-names = "", "", "", "", "",            /* 0-4 */
>>           "SPE_RSTN", "SPE_INTN", "MIKROBUS_GPIO1_7",    /* 5-7 */
>>           "MIKROBUS_GPIO1_8", "MIKROBUS_GPIO1_9",        /* 8-9 */
>> @@ -804,15 +888,11 @@ it66121_out: endpoint {
>>   };
>>     &main_i2c3 {
>> -    pinctrl-names = "default";
>> -    pinctrl-0 = <&mikrobus_i2c_pins_default>;
>>       clock-frequency = <400000>;
>>       status = "okay";
>>   };
>>     &main_spi2 {
>> -    pinctrl-names = "default";
>> -    pinctrl-0 = <&mikrobus_spi_pins_default>;
>>       status = "okay";
>>   };
>>   @@ -876,8 +956,6 @@ &main_uart1 {
>>   };
>>     &main_uart5 {
>> -    pinctrl-names = "default";
>> -    pinctrl-0 = <&mikrobus_uart_pins_default>;
>>       status = "okay";
>>   };
>>

Ayush Singh