Compatibility #17

lorforlinux · 2020-06-20T04:16:18Z

@RobertCNelson @jadonk, please review my code:)

jadonk

This is a nice pattern. I really look forward to seeing it for the other buses.

If you could keep the pinmux helper separate from the other stuff, that might be good in case it doesn't work.

Please fix your tabs.

jadonk · 2020-06-21T05:41:40Z

src/arm/am5729-beagleboneai.dts

@@ -265,1700 +267,9 @@
 		compatible = "ti,pruss-shmem";
 		reg = <0x4b280000 0x020000>;
 	};
-
-	cape-universal {


I thought you were going in the direction of adding config-pin to AM5729, though I wouldn't want to depend on it working.

Er, just realized these were just the GPIOs. Why remove them?

I didn't remove them just shifted them to another file src/arm/am572x-bone-common-univ.dtsi

jadonk · 2020-06-21T05:43:39Z

src/arm/bbb-bone-buses.dtsi

+};
+
+bone_uart_1: &uart1 {
+	status = "disabled";


Your indents look to not be set properly. Use tabs with a stop of 8.

jadonk · 2020-06-21T05:47:12Z

src/arm/bbb-bone-buses.dtsi

+
+    bone_uart_3_pins: pinmux_bone_uart_3_pins {
+		pinctrl-single,pins = <
+            P9_42A (PIN_OUTPUT_PULLUP | MUX_MODE1)


Tabs are off.

jadonk · 2020-06-21T05:49:43Z

src/arm/am5729-beagleboneai.dts

 };

 &dra7_pmx_core {
-    /************************/


This was a nice summary of information. Is the new organization harder to follow and more scattered? Can't the bone-bus.dtsi files simply reference these?

Yes, I was thinking of using phandles from here like pinmux_P8_03_default_pin then we will not require 2 separate files for bone-buses one would suffice and there will be no need for the helper header files for simple pin muxing.

no possibility to achieve this at this moment ^

Are these called phandles? Doesn't sound right to me.

I think the challenge in reusing these definitions is that not all header pins have only one ball/signal connected and that having 1 or 2 signals varies between AM335x (Black) and AM57x (AI) boards.

Are these called phandles? Doesn't sound right to me.

I think they are phandles, please correct me if you think i am wrong there.

I think the challenge in reusing these definitions is that not all header pins have only one ball/signal connected and that having 1 or 2 signals varies between AM335x (Black) and AM57x (AI) boards.

No that is not the case, the problem here is that peripheral name is different like uart1 on BBB and uart10 BBAI, the two-pin problem can be solved by using bone-pinmux-helper.

Are we closer now to using common symbols/phandles?

jadonk · 2020-06-21T06:01:28Z

src/arm/am572x-bone-common-univ.dtsi

+
+	/* P8_03  (ball AB8) gpio1_24 */
+	P8_03_default_pin: pinmux_P8_03_default_pin { pinctrl-single,pins = <
+		DRA7XX_CORE_IOPAD(0x379C, PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE14) >; };	/* mmc3_dat6.gpio1_24 */	


Shouldn't this use the new macro?

If you are happy with them I will implement them everywhere they can fit :)

The new macro seems handy. I just don't want to define the address entry in multiple locations. One place to be right or wrong and get fixed.

Okay, I will gradually shift things to use those macros.

jadonk · 2020-06-21T06:17:06Z

src/arm/bbai-bone-buses.dtsi

+	};
+
+	bone_uart_4_pins: pinmux_bone_uart_4_pins {
+        pinctrl-single,pins = <


Watch your tabs!

I am really very sorry, It looks fine on vscode here :(

src/arm/bbai-bone-buses.dtsi

jadonk · 2020-06-21T06:19:04Z

src/arm/bbai-bone-buses.dtsi

+            P9_13( PIN_OUTPUT_PULLUP | MUX_MODE4)	/* mcasp3_axr1.uart5_txd */
+            P9_11A( PIN_INPUT_PULLUP | MUX_MODE4)	/* mcasp3_axr0.uart5_rxd */
+            /* unused pins */
+			P9_11B( PIN_OUTPUT | MUX_MODE15)


Missing processor pin comment.

I skipped because IT was not in use. I will make sure everything is commented during code cleanup. I hope that's acceptable?

Comment should say something like "pin_xyz.Off" if it is off.

jadonk · 2020-06-21T06:19:18Z

src/arm/bbai-bone-buses.dtsi

+
+	bone_uart_5_pins: pinmux_bone_uart_5_pins {
+		pinctrl-single,pins = <
+			P8_37B( PIN_OUTPUT_PULLUP | MUX_MODE3)	/* uart8_txd */


Comment is missing the processor pin.

jadonk · 2020-06-21T06:21:57Z

src/arm/bbb-bone-buses.dtsi

+&am33xx_pinmux {
+bone_uart_1_pins: pinmux_bone_uart_1_pins {
+		pinctrl-single,pins = <
+			P9_24( PIN_OUTPUT_PULLUP | MUX_MODE0)


Having the consistent comments would be nice. pin.mode

Agreed 👍

jadonk · 2020-06-25T12:58:54Z

Can you rebase and provide fixes?

lorforlinux · 2020-06-25T16:06:49Z

@jadonk This is my first time rebasing.

lorforlinux · 2020-06-25T16:36:14Z

I accidentally rebased with the master branch but I had to rebase with v4.19.x-ti, I have reset the head to the last correct position and now it's saying that it's already up to date when I try to rebase with v4.19.x-ti. What should I do?

Edit: After rebasing with the master I also rebased with v4.19.x-ti, I am setting head to that position so that you can guide me better.

lorforlinux · 2020-06-25T16:57:46Z

hey, It looks okay now!

lorforlinux · 2020-06-26T21:34:23Z

@jadonk Hey, can you merge the changes please?

lorforlinux · 2020-06-27T06:34:56Z

I have added /bone/i2c code, everything has been tested on BBB and BBAI :)

The default properties are already set in the base files and we don't require these values here.

These nodes provides a way to make DT overlays compatible on both BBB and BBAI. For example, LCD cape and Motor cape uses these PWM and by uisng these nodes we can write one overlay to work on both BBB and BBAI.

The default properties are already set in the base files like dra7.dtsi and we don't require these values here.

These nodes provides a way to make DT overlays compatible on both BBB and BBAI. For example, LCD cape and Motor cape uses these PWM and by uisng these nodes we can write one overlay to work on both BBB and BBAI.

lorforlinux · 2020-07-13T15:24:49Z

src/arm/bbb-bone-buses.dtsi

+
+bone_pwmss_2: &epwmss2 {
+
+};


@jadonk @RobertCNelson I am not able to use symlinks with these nodes what could be the reason for that?
These nodes are used in beagleboard/bb.org-overlays#188 and I will also use them for 4D systems LCD cape DT overlay.

Is there even a udev rule for bone-style symlinks on pwms?

https://elinux.org/Beagleboard:BeagleBone_cape_interface_spec section on methodology need to be brought up-to-date on what is in https://github.com/beagleboard/customizations/ and planned to be in there.

Hmmm.... I guess even just https://github.com/beagleboard/customizations/blob/master/etc/udev/rules.d/10-of-symlink.rules should cover it. What is our method for debugging udev rules? Does the rule ever run on these? Why doesn't the pull-request include the symlink definitions?

lorforlinux · 2020-07-13T15:26:30Z

src/arm/bbb-bone-buses.dtsi

@@ -370,59 +370,41 @@

 // UARTs
 bone_uart_1: &uart1 {
-	status = "disabled";
-	pinctrl-names = "default";


I removed these because they serve no purpose as the settings are already done in the base files.

jadonk

Without a justification, I prefer the old way over macros.

jadonk · 2020-07-14T08:05:56Z

include/dt-bindings/board/am572x-bbai-pins.h

+/* P8_02               		GND */
+
+/* P8_03  (ball AB8) gpio1_24 */
+#define P8_03_DEFAULT       P8_03( PIN_OUTPUT_PULLDOWN | INPUT_EN | MUX_MODE14)	    /* mmc3_dat6.gpio1_24 */


Are tabs still off in your editor? Things don't seem to line up for me.

Are these defines somehow better than the definitions that could be referenced as symbols before? Is the point to save space in the .dtb? Please include your reasoning for the changes.

you are seeing an outdated version of the file.

jadonk · 2020-07-14T08:10:31Z

src/arm/am572x-bone-common-univ.dtsi

+	P8_03_default_pin: pinmux_P8_03_default_pin { pinctrl-single,pins = < P8_03_DEFAULT >; };
+	P8_03_gpio_pin: pinmux_P8_03_gpio_pin { pinctrl-single,pins = < P8_03_GPIO >; };
+	P8_03_gpio_pu_pin: pinmux_P8_03_gpio_pu_pin { pinctrl-single,pins = < P8_03_GPIO_PU >; };
+	P8_03_gpio_pd_pin: pinmux_P8_03_gpio_pd_pin { pinctrl-single,pins = < P8_03_GPIO_PD >; };


I agree to bring back the comment at the end of the line. I don't see the point of using macros here. It seems redundant and error-prone. I'd prefer things be in one place to fix. Besides, this means they can be referenced by overlays. If it is in macros, overlays wouldn't be able to use the symbols. This saves in the .dtb size, but I don't think that is a care-about.

jadonk

More change requests interspersed.

jadonk · 2020-07-14T08:41:41Z

include/dt-bindings/board/am335x-bbb-pins.h

@@ -0,0 +1,82 @@
+/*


As long as this is accurate, it seems reasonable to me to have 1 place that associates balls with cape header pins. I'll just be looking to make sure this association isn't needed in other places.

jadonk · 2020-07-14T08:42:41Z

include/dt-bindings/board/am572x-bbai-pins.h

+#ifndef _DT_BINDINGS_BOARD_AM5729_BBAI_PINS_H
+#define _DT_BINDINGS_BOARD_AM5729_BBAI_PINS_H
+
+#define P8_03(mode) DRA7XX_CORE_IOPAD(0x379C, mode)  /* AB8: P8.3: mmc3_dat6 */


I like having the ball number in the comment. The cape header pin definition in the comment is redundant.

jadonk · 2020-07-14T08:43:06Z

include/dt-bindings/board/am335x-bbb-pins.h

+#ifndef _DT_BINDINGS_BOARD_AM335X_BBB_PINS_H
+#define _DT_BINDINGS_BOARD_AM335X_BBB_PINS_H
+
+#define P8_03(mode) AM33XX_IOPAD(0x0818, mode)  /* gpmc_ad6 */


Please add the ball number to the comment. That is super useful.

jadonk · 2020-07-14T08:44:20Z

src/arm/am5729-beagleboneai.dts

-		pinctrl-names = "default";
-		pinctrl-0 = <&cape_pins_default>;
-	};
+	// cape_pins: cape_pins {


I understand doing this on work-in-progress, but please remove it for the pull request.

okay, I will remove the code.

jadonk · 2020-07-14T08:45:43Z

src/arm/am5729-beagleboneai.dts

 };

 &dra7_pmx_core {
-    /************************/


Are we closer now to using common symbols/phandles?

jadonk · 2020-07-14T08:57:49Z

src/arm/bbb-bone-buses.dtsi

+
+		led_P8_03 {
+			gpios = <&gpio1 6 0>;
+			status = "disabled";


Why is the status "reserved" for bbai?

Ohh this, I wanted to ask this to you. both "reserved" and "disabled" worked for me which should we choose?

From Deice tree specifications I found this:

"disabled" Indicates that the device is not presently operational, but it might become operational in the future (for example, something is not plugged in, or switched off). Refer to the device binding for details on what disabled means for a given device. "reserved" Indicates that the device is operational, but should not be used. Typically this is used for devices that are controlled by another software component, such as platform firmware.

I like the choice of "disabled".

jadonk · 2020-07-14T08:58:48Z

src/arm/bbb-bone-buses.dtsi

+
+// use only when bone_i2c_2 is not in use
+bone_i2c_2a: &i2c2 {
+	// symlink = "bone/i2c/2a";


Why is the symlink commented out?

the previous code was interfering with this, I updated it with the new code yesterday and things are not looking quite right for now. I am in the process of updating this code and DT overlays with it.

jadonk · 2020-07-14T09:00:44Z

src/arm/bbb-bone-buses.dtsi

+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	ti,pio-mode; /* disable dma when used as an overlay, dma gets stuck at 160 bits... */


I find it rather doubtful that this issue exists on both devices. Where did you copy and paste this from? Does it belong here or, more likely, in the device base-tree definition? If it specific to overlays, as is indicated, more information really is needed here.

I am sorry, this was just meant for testing purpose. I will fix this.
The code will be shifted to overlays just like we had for old overlays.

jadonk · 2020-07-14T09:03:29Z

src/arm/bbb-bone-buses.dtsi

+
+bone_pwmss_2: &epwmss2 {
+
+};


Is there even a udev rule for bone-style symlinks on pwms?

https://elinux.org/Beagleboard:BeagleBone_cape_interface_spec section on methodology need to be brought up-to-date on what is in https://github.com/beagleboard/customizations/ and planned to be in there.

jadonk · 2020-07-14T09:05:20Z

src/arm/bbb-bone-buses.dtsi

+
+bone_pwmss_2: &epwmss2 {
+
+};


Hmmm.... I guess even just https://github.com/beagleboard/customizations/blob/master/etc/udev/rules.d/10-of-symlink.rules should cover it. What is our method for debugging udev rules? Does the rule ever run on these? Why doesn't the pull-request include the symlink definitions?

I have removed the redundant code and changed the file name as discussed on irc.

I have added ball names and changed the name of file as discussed on IRC.

Created dummy nodes for unavailable bone buses to avoid FDT error!

These nodes are just to avoid FDT error while using src/arm/BONE-PWM0-00A0.dts

These nodes prevents FDT_ERROR to occur.

lorforlinux · 2020-08-16T11:38:41Z

@jadonk @RobertCNelson I am closing this PR as this is no longer required ;)
This code has been merged through another PR -> #18

jadonk

Looks good to me. Looks like it is already merged too. :-D

jadonk · 2020-08-18T15:00:37Z

src/arm/bbb-bone-buses.dtsi

+
+		led_P8_03 {
+			gpios = <&gpio1 6 0>;
+			status = "disabled";


I like the choice of "disabled".

add comments to am5729-beagleboneai.dts

63f5446

lorforlinux mentioned this pull request Jun 20, 2020

Bone uart beagleboard/bb.org-overlays#180

Open

jadonk reviewed Jun 21, 2020

View reviewed changes

lorforlinux force-pushed the compatibility branch from 81ca016 to f7f7275 Compare June 25, 2020 16:00

lorforlinux added 12 commits June 25, 2020 21:45

easy BBB pinmuxing

f019292

easy BBAI pinmuxing

88ace58

add bbb-bone-buses.dtsi

ef414f3

clean and add bbai-bone-buses.dtsi

369a235

add bone-pinmux-helper nodes

fcdd684

create BBAI /bone/<bus>/<#>

07dc849

create BBB /bone/<bus>/<#>

c42c59a

fix tabs

6280f47

fix spacing

2a7f9ee

fix tabs

558c2c3

add comments and fix tabs

86f270c

add comments and fix tabs

2c22378

lorforlinux force-pushed the compatibility branch 3 times, most recently from 7ebb04c to ec69013 Compare June 25, 2020 16:30

lorforlinux force-pushed the compatibility branch from ec69013 to 2c22378 Compare June 25, 2020 16:54

lorforlinux requested a review from jadonk June 26, 2020 23:01

lorforlinux added 3 commits June 27, 2020 12:00

fix symlink

83a5b91

add BBAI /bone/i2c

ab7d9bf

add BBB /bone/i2c

161efee

lorforlinux added 4 commits July 13, 2020 20:46

BBB: Remove status & pinctrl-names from bone_* nodes

c3d3011

The default properties are already set in the base files and we don't require these values here.

BBB: add PWM nodes

4552ec0

These nodes provides a way to make DT overlays compatible on both BBB and BBAI. For example, LCD cape and Motor cape uses these PWM and by uisng these nodes we can write one overlay to work on both BBB and BBAI.

BBAI: Remove status & pinctrl-names from bone_* nodes

a66a07a

The default properties are already set in the base files like dra7.dtsi and we don't require these values here.

BBAI: add PWM nodes

5551091

These nodes provides a way to make DT overlays compatible on both BBB and BBAI. For example, LCD cape and Motor cape uses these PWM and by uisng these nodes we can write one overlay to work on both BBB and BBAI.

lorforlinux mentioned this pull request Jul 13, 2020

BBB and BBAI compatible BBORG motorCape overlay beagleboard/bb.org-overlays#188

Open

lorforlinux commented Jul 13, 2020

View reviewed changes

lorforlinux mentioned this pull request Jul 13, 2020

fix BBB servo cape overlay beagleboard/bb.org-overlays#174

Open

jadonk requested changes Jul 14, 2020

View reviewed changes

lorforlinux added 9 commits July 15, 2020 22:04

add am5729-bone-pins.h

bfdaed3

I have removed the redundant code and changed the file name as discussed on irc.

add am335x-bone-pins.h

e22a7cb

I have added ball names and changed the name of file as discussed on IRC.

BBB: use pinmux macros

9982683

BBAI: cleanup base dts file

0566c2c

BBAI: updated with dummy nodes

21068c2

Created dummy nodes for unavailable bone buses to avoid FDT error!

BBAI: fix led, spi nodes & add CAN bus

861db41

BBB: fix spi nodes & add CAN bus

5a8715f

add bone bus PWM0

96385ac

BBAI: dummy nodes for bone bus pwm0

0091783

These nodes are just to avoid FDT error while using src/arm/BONE-PWM0-00A0.dts

lorforlinux mentioned this pull request Jul 21, 2020

bone bus PWM DT overlays beagleboard/bb.org-overlays#193

Open

lorforlinux added 7 commits July 22, 2020 02:29

remove symlinks for CAN & PWM

ab632f3

BBAI: Enabling timer PWM functionality

785fe6f

BBB: dummy timer nodes

56c68f5

These nodes prevents FDT_ERROR to occur.

BBAI: add Timer nodes

e40c15c

BBAI: add timer nodes and update spi nodes

61a7b0e

BBB: add timer nodes and update spi nodes

0411005

BBAI: add & update PRU nodes

97a6f0d

lorforlinux mentioned this pull request Aug 12, 2020

Cape compatibility layer for BeagleBone Black and BeagleBone AI #18

Merged

lorforlinux closed this Aug 16, 2020

jadonk reviewed Aug 18, 2020

View reviewed changes

src/arm/bbb-bone-buses.dtsi

led_P8_03 {

gpios = <&gpio1 6 0>;

status = "disabled";

Copy link

Member

jadonk Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the choice of "disabled".


		bone_pwmss_2: &epwmss2 {

		};


		bone_pwmss_2: &epwmss2 {

		};


		bone_pwmss_2: &epwmss2 {

		};

Compatibility #17

Compatibility #17

Conversation

lorforlinux commented Jun 20, 2020

jadonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadonk commented Jun 25, 2020

lorforlinux commented Jun 25, 2020

lorforlinux commented Jun 25, 2020 • edited Loading

lorforlinux commented Jun 25, 2020

lorforlinux commented Jun 26, 2020

lorforlinux commented Jun 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorforlinux commented Aug 16, 2020

jadonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorforlinux commented Jun 25, 2020 •

edited

Loading