Please roast my new PocketBeagle device tree overlay

[Tom Stepleton]

After three years of avoiding it I've finally decided to fight the dragon of writing my own device tree overlay. It's for a PocketBeagle application that uses both PRUs to talk to 13 pins: 8 GPIOs, 3 pruins, and 2 pruouts. I'm happy to say that I've succeeded in getting one that works!

But I made it with a heap of conjecture and experimentation, and I'm pretty sure it stinks. It would be a big favour for you to tell me all the ways it's lousy. It doesn't use the New Syntax, but I'm OK with that --- I guess I'm more worried about future proofing, brittleness, and things that are actually wrong (and only working because I'm getting lucky).

If it matters, I'm running on 10.0 IOT. (I can't use 10.3 for (happily-soon-eliminated) Reasons.)

Without further adieu, I give you: pb-cameo-aphid.dts. It's a bit long for this message, so I've uploaded it here:

http://mg-1.uk:31132/pb-cameo-aphid.dts.txt

Thanks for any insights,

--Tom

[Tom Stepleton]

I've come up with a minimal working version of the file now for my application (appended below), and rather than see if anyone's up for a general review like I did earlier, maybe it would be easier to post some more specific questions here. I would love to understand better what the parts of this device tree overlay are doing, so if anyone has any insights, I'd be very grateful to hear them.

1. I compile my device tree overlay this way: is it correct?
         cpp -I/opt/source/bb.org-overlays/include MY-DT-OVERLAY.dts -P -o tmp.dts
         dtc -@ -I dts -O dtb -o MY-DT-OVERLAY.dtbo tmp.dts
   Is it right to use the C preprocessor in this way? I note that the preprocessor is substituting a value for __TIMESTAMP__ in fragment@0 --- is this correct, or should __TIMESTAMP__ be left un-substituted so that dtc can see it?
   
   
2. fragment@1 reserves some header pins so that the pinmux helper doesn't use them. Is this necessary? Is there a reason that PB-eqep2.dts doesn't reserve the resources it uses? (I studied a lot of examples in the BB.org overlays and this stood out to me.)
   
   
3. fragment@2 --- is it the case that this fragment is just defining a pinmux configuration, and that the configuration actually gets put to use elsewhere?
   
   
4. fragment@3 --- is it correct that this fragment does both of the following things:
   1. enable the PRUs somehow?
   2. somehow makes the PRU system configuration put the fragment@2 pinmux configuration into effect?
      
      Is there anything else that this fragment does? Does it make the pins available to the PRUs in some way beyond just configuring the pinmux? (In other words, why does the pruss fragment have pinctrl-names and pinctrl-0 parameters?)
      
      
5. Potentially answering my Question #2, I think that PB-eqep2.dts is basically doing things in a different way --- it's turning on some on-chip peripherals and then configuring the pinmux with some new symbols, but in a way that would still allow you to use config-pins to reuse the header pins for something else. (My fragment@1 does not allow this.) Is that interpretation correct? Could I have taken this approach somehow, or is it better to set up pins for a "PRU-heavy" application using the method I'm using here?
   
   
6. In the pruss fragment for the Replicape, I found some additional labeling of pins. What's that all about? I don't need it for my application, but as that stuff uses custom names, I've found it hard to search for what's going on.
   
   
7. Finally, just in case you happened to notice --- did anything look weird or wrong in my overlay file below?
   

Thanks as ever,

--Tom

/dts-v1/;
/plugin/;

/* This header file doesn't exist on our pocketbeagle, so we replicate
 * necessary symbols manually. */
/* #include <dt-bindings/board/am335x-pb-base.h> */
#define PB_P1_02 0x08e4
#define PB_P1_29 0x09ac
#define PB_P1_30 0x0974
#define PB_P1_31 0x09a0
#define PB_P1_33 0x0994
#define PB_P1_36 0x0990
#define PB_P2_09 0x0984
#define PB_P2_24 0x0830
#define PB_P2_28 0x09a8
#define PB_P2_30 0x099c
#define PB_P2_32 0x0998
#define PB_P2_34 0x09a4
#define PB_P2_35 0x08e0

#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/pinctrl/am33xx.h>

/ {
        /* For showing loaded overlays at /proc/device-tree/chosen/overlays/ */
        fragment@0 {
                target-path = "/";
                __overlay__ {
                        chosen {
                                overlays {
                                        PB-CAMEO-APHID = __TIMESTAMP__;
                                };
                        };
                };
        };

        /* Reserve these header pins so the pinmux helpers don't claim them. */
        fragment@1 {
                target = <&ocp>;
                __overlay__ {
                        P1_02_pinmux { status = "disabled"; };
                        P1_29_pinmux { status = "disabled"; };
                        P1_30_pinmux { status = "disabled"; };
                        P1_31_pinmux { status = "disabled"; };
                        P1_33_pinmux { status = "disabled"; };
                        P1_36_pinmux { status = "disabled"; };
                        P2_09_pinmux { status = "disabled"; };
                        P2_24_pinmux { status = "disabled"; };
                        P2_28_pinmux { status = "disabled"; };
                        P2_30_pinmux { status = "disabled"; };
                        P2_32_pinmux { status = "disabled"; };
                        P2_34_pinmux { status = "disabled"; };
                        P2_35_pinmux { status = "disabled"; };
                };
        };

        /* Introduce a pinmux configuration for Cameo/Aphid. */
        fragment@2 {
                target = <&am33xx_pinmux>;
                __overlay__ {

                        /* Here is our new pinmux configuration. It's called
                         * "pinmux_aphd_pins", and you can find the files that
                         * correspond to this declaration at
                         * /sys/firmware/devicetree/base/ocp/l4_wkup@44c00000/
                         *     scm@210000/pinmux@800/pinmux_aphd_pins
                         *
                         * The symbol "aphd_pins" can then be used to refer to
                         * this configuration elsewhere in this overlay. */
                        aphd_pins: pinmux_aphd_pins {
                                pinctrl-single,pins = <

                                        /* Ordinary GPIO pins. Even though these
                                         * pins are accessible to the ARM, the
                                         * only part of the Cameo/Aphid software
                                         * that touches them are the PRU
                                         * firmware programs. The ARM tells the
                                         * PRUs what to do over rpmsg. */

                                        AM33XX_IOPAD(PB_P1_29, PIN_INPUT | MUX_MODE7 )
                                        AM33XX_IOPAD(PB_P1_31, PIN_INPUT | MUX_MODE7 )
                                        AM33XX_IOPAD(PB_P1_33, PIN_INPUT | MUX_MODE7 )
                                        AM33XX_IOPAD(PB_P1_36, PIN_INPUT | MUX_MODE7 )
                                        AM33XX_IOPAD(PB_P2_28, PIN_INPUT | MUX_MODE7 )
                                        AM33XX_IOPAD(PB_P2_30, PIN_INPUT | MUX_MODE7 )
                                        AM33XX_IOPAD(PB_P2_32, PIN_INPUT | MUX_MODE7 )
                                        AM33XX_IOPAD(PB_P2_34, PIN_INPUT | MUX_MODE7 )

                                        /* PRU-exclusive pins. Note that all are
                                         * declared "inputs" even though the
                                         * latter two are PRU outputs --- IIRC,
                                         * from the ARM's perspective, they're
                                         * inputs. */

                                        /* Input pins */
                                        AM33XX_IOPAD(PB_P1_02, PIN_INPUT | MUX_MODE6 )
                                        AM33XX_IOPAD(PB_P1_30, PIN_INPUT | MUX_MODE6 )
                                        AM33XX_IOPAD(PB_P2_09, PIN_INPUT | MUX_MODE6 )
                                        /* Output pins: note 2.24 is unusual for
                                         * doing input in Mode 6. */
                                        AM33XX_IOPAD(PB_P2_24, INPUT_EN | MUX_MODE6 )
                                        AM33XX_IOPAD(PB_P2_35, INPUT_EN | MUX_MODE5 )
                                >;
                        };
                };
        };

        /* Required for use of the PRUs. */
        fragment@3 {
                target = <&pruss>;
                __overlay__ {
                        pinctrl-names = "default";
                        pinctrl-0 = <&aphd_pins>;
                        status = "okay";
                };
        };
};