| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <70f28343-6738-47f2-97b5-6afa96f1fbcc@ti.com>
Date: Thu, 27 Jun 2024 12:50:24 -0500
From: Andrew Davis <afd@...com>
To: Ayush Singh <ayush@...gleboard.org>, Mark Brown <broonie@...nel.org>,
Vaishnav M A <vaishnav@...gleboard.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Derek Kiernan <derek.kiernan@....com>,
Dragan Cvetic <dragan.cvetic@....com>, Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman
<gregkh@...uxfoundation.org>,
Nishanth Menon <nm@...com>, Vignesh Raghavendra
<vigneshr@...com>,
Tero Kristo <kristo@...nel.org>, Michael Walle
<mwalle@...nel.org>,
Andrew Lunn <andrew@...n.ch>, <jkridner@...gleboard.org>,
<robertcnelson@...gleboard.org>
CC: <linux-spi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v5 7/7] dts: ti: k3-am625-beagleplay: Add mikroBUS
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@...gleboard.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
>
Powered by blists - more mailing lists