| Message ID | 20240627-mikrobus-scratch-spi-v5-7-9e6c148bf5f0@beagleboard.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | misc: Add mikroBUS driver | expand |
On 21:56-20240627, Ayush Singh wrote: > DONOTMERGE ^^ might be better off in the diffstat and explain why DONOT MERGE :) [...] > + 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 */ > + >; > + }; we could potentially get rid of these if we get the gpio-ranges correct on pinctrl? I have not gotten around to am62x yet - but see this: https://lore.kernel.org/linux-arm-kernel/20240627162539.691223-1-nm@ti.com/T/#t [...]
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.. 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. 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"; }; }; }; 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. 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"; > }; > >
On 6/27/24 22:12, Nishanth Menon wrote: > On 21:56-20240627, Ayush Singh wrote: >> DONOTMERGE > ^^ might be better off in the diffstat and explain why DONOT MERGE :) > [...] There are 2 reasons for DONOTMERGE 1. Mikrobus boards config should not live here. It is supposed to be independent of the system and arch. 2. I am going to be cleaning up and sending this once at least the dt-bindings are approved. I have included it in the patch to provide some idea about how it will supposedly look like (and anyone who might want to test this stuff). I will try to remember to add it in diffstat in the future. >> + 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 */ >> + >; >> + }; > we could potentially get rid of these if we get the gpio-ranges correct > on pinctrl? I have not gotten around to am62x yet - but see this: > > https://lore.kernel.org/linux-arm-kernel/20240627162539.691223-1-nm@ti.com/T/#t > > [...] Thanks. Will look into this before sending an actual patch for beagleplay dts as well. Ayush Singh
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
On Thu Jun 27, 2024 at 7:07 PM CEST, Andrew Davis wrote: > > + 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.. > > 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. > > 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"; > }; > }; > }; If there should only be one DT overlay per click board, how would you apply that to to different connectors? > 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. The bus driver would still be needed to do the enumeration of the children, no? And it could make the chip select translations etc. So you can use the normal bindings of these devices. -michael
On 6/27/24 22:51, Michael Walle wrote: > On Thu Jun 27, 2024 at 7:07 PM CEST, Andrew Davis wrote: >>> + 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.. >> >> 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. >> >> 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"; >> }; >> }; >> }; > If there should only be one DT overlay per click board, how would > you apply that to to different connectors? See my other two replies for more context: https://lore.kernel.org/linux-arm-kernel/cef08d49-a462-4167-8b9d-bf09e8aac92f@beagleboard.org/ https://lore.kernel.org/linux-arm-kernel/e0f9754e-4d84-4ab4-82a4-34cb12800927@beagleboard.org/ My ideal design is that most mikroBUS board configs will be defined in a `dtsi` file which can be included by any system with mikroBUS support. This file might look as follows: ``` /dts-v1/; /plugin/; / { mikrobus_boards { thermo_click: thermo-click { compatible = "maxim,max31855k", "mikrobus-spi"; 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"; }; }; }; ``` And the board overlay will look as follows: ``` &conector { board = <&lsm6dsl_click>; }; ``` I do have an experimental configfs entry that passes the mikroBUS board id(s) and creates and applies the dt changeset to the connector dynamically. > >> 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. > The bus driver would still be needed to do the enumeration of the > children, no? And it could make the chip select translations etc. So > you can use the normal bindings of these devices. > > -michael If static dt was the only method to detect the board, then it would be fine. But boards with 1-wire-eeprom can allow for for dynamic detection if the overlay can be system agnostic. To make the dt system agnostic, it should be possible for the board to specify the following information: 1. If a pin is supposed to perform its normal function (UART TX should actually do UART TX), or if it should work as say GPIO (Eg, using RST pin as SPI chipselect). 2. Which pin(s) are the SPI chipselect. 3. If a particular pin needs to be pulled high or low for the board to function, etc. So while most of the normal properties of SPI devices can be reused, there is a need to introduce new properties for additional information. In the previous patches, MikroBUS manifest was being used for this purpose, but for a full dt driver, the device tree needs to be extended to specify these extra properties that are not relevant to the non-mikrobus variant of the device. Ayush Singh
On 6/27/24 12:16 PM, Ayush Singh wrote: > > 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. > That seems even worse.. That means the board file needs to know about the attached board, which is not how DT works. It describes hardware in a hierarchical/acyclic graph. For instance, take an I2C device, its node is a child of the I2C bus, and has phandle pointers to the IRQ it uses (or whatever else provider it needs). What you have here is like the I2C bus node phandle pointing to the connected child devices. > 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). > How about providing the full/final example then (this series should be marked as RFC as it is now has missing parts). Move the click board node into a DTSO file and put that in a common location (click boards are common to all boards right, so lets say in drivers/of/click for now just for the RFC). > >> >> 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 { ^^^ So same issue, what if I want to attach this click board to the second mikrobus connector on my board (i.e. mikrobus1), I'd have to modify the overlay.. Or have an overlay for every possible connector instance number. > 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. > Then these are two things you'll need to solve. We can add these functions to the base DT/OF support if they are generic problems (which they are, other connectors/daughterboards have this same issue, RPi header, Seeed Grove connector, Sparkfun QWIIC headers, etc..). Andrew > >> >> 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 >
On 6/27/24 23:20, Andrew Davis wrote: > On 6/27/24 12:16 PM, Ayush Singh wrote: >> >> 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. >> > > That seems even worse.. That means the board file needs to know about the > attached board, which is not how DT works. It describes hardware in a > hierarchical/acyclic graph. For instance, take an I2C device, its node > is a child of the I2C bus, and has phandle pointers to the IRQ it uses > (or whatever else provider it needs). What you have here is like the > I2C bus node phandle pointing to the connected child devices. > >> 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). >> > > How about providing the full/final example then (this series should be > marked > as RFC as it is now has missing parts). Move the click board node into > a DTSO > file and put that in a common location (click boards are common to all > boards > right, so lets say in drivers/of/click for now just for the RFC). > >> >>> >>> 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 { > > ^^^ > So same issue, what if I want to attach this click board > to the second mikrobus connector on my board (i.e. mikrobus1), > I'd have to modify the overlay.. Or have an overlay for every > possible connector instance number. The plan is to have a sysfs `new_device` and `delete_device` entry like I2C has where the board name is passed. The driver can then create a dt changeset apply to live tree. In the current dt, the changeset is to add a `board` property with the phandle of a board (if found). Can you suggest how something similar will be possible if the board node is a child of the connector node? Maybe it is possible to take a generic dt overlay and change the name of parent node on it or something? > >> 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. >> > > Then these are two things you'll need to solve. We can add > these functions to the base DT/OF support if they are generic > problems (which they are, other connectors/daughterboards have > this same issue, RPi header, Seeed Grove connector, Sparkfun QWIIC > headers, etc..). > > Andrew > >> >>> >>> 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 >>
> Can you suggest how something similar will be possible if the board node is > a child of the connector node? Maybe it is possible to take a generic dt > overlay and change the name of parent node on it or something? Maybe that answers my question on an earlier patch. So the parent node of the overlay is embedded inside the overlay? Making a guess without looking at the code... The code loading an overlay must first parse the overlay and find that node name. It then must somehow traverses the graph, and find that node. It then must apply the overlay at that point. Could you not take this code apart, so you can pass it the name of the node you want to apply the overlay to, rather than read it from the overlay itself? The connector should known its own node in the graph. So it can find the overlay, load it, and then ask for it to be applied over itself. Andrew
On Thu, Jun 27, 2024 at 09:56:17PM +0530, Ayush Singh wrote: > + mikrobus_boards { > + thermo_click: thermo-click { > + compatible = "maxim,max31855k", "mikrobus-spi"; > + 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"; So every single device that could possibly go on a mikrobus board will need a new binding (or significant binding modifications)? tbh, I was expecting something where the mikrobus connector looks like what "spi-mux" does. Ditto "i2c-mux" for the i2c component.
On Thu, Jun 27, 2024 at 11:46:31PM +0530, Ayush Singh wrote: > > On 6/27/24 23:20, Andrew Davis wrote: > > On 6/27/24 12:16 PM, Ayush Singh wrote: > > > > > > 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. > > > > > > > That seems even worse.. That means the board file needs to know about the > > attached board, which is not how DT works. It describes hardware in a > > hierarchical/acyclic graph. For instance, take an I2C device, its node > > is a child of the I2C bus, and has phandle pointers to the IRQ it uses > > (or whatever else provider it needs). What you have here is like the > > I2C bus node phandle pointing to the connected child devices. > > > > > 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). > > > > > > > How about providing the full/final example then (this series should be > > marked > > as RFC as it is now has missing parts). Move the click board node into a > > DTSO > > file and put that in a common location (click boards are common to all > > boards > > right, so lets say in drivers/of/click for now just for the RFC). > > > > > > > > > > > > > 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 { > > > > ^^^ > > So same issue, what if I want to attach this click board > > to the second mikrobus connector on my board (i.e. mikrobus1), > > I'd have to modify the overlay.. Or have an overlay for every > > possible connector instance number. > > > The plan is to have a sysfs `new_device` and `delete_device` entry like I2C > has where the board name is passed. The driver can then create a dt > changeset apply to live tree. In the current dt, the changeset is to add a > `board` property with the phandle of a board (if found). > > Can you suggest how something similar will be possible if the board node is > a child of the connector node? Maybe it is possible to take a generic dt > overlay and change the name of parent node on it or something? You need to describe the problem(s) to solve first, and then a solution. You're just giving us a solution to review. Let's start with you have an add-on board and an overlay for that board. No one wants N overlays for N base board for a single physical board. One board, one overlay. Any solution must provide that. Who knows when a board is connected and what board it is? Each one could be the OS or the user. Worst case is nothing is detected and it is just the user knows "I've connected board X to connector A" and has to tell the OS that. Or the OS can detect a board and figure out what board via EEPROM with no user intervention. In between is OS detects a board, but needs the user to say which one. The detecting everything case is easy. The connector driver knows which connector it is and which overlay. You just need to define how you select the overlay file based on EEPROM data. The others will need some sort of interface for the user to provide the information. Rob
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"; + 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"; };
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(-)