| 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: <93cdd5c5-d54c-46c2-9055-5cd9cc79e2da@beagleboard.org>
Date: Thu, 27 Jun 2024 23:46:31 +0530
From: Ayush Singh <ayush@...gleboard.org>
To: Andrew Davis <afd@...com>, 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 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@...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.
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
>>
Powered by blists - more mailing lists