Add Support for the Geekworm MZP280 DPI Display

[macromorgan]

Please pull this request for the Raspberry Pi kernel. I'm working with Upstream for a corresponding patch to the mainline kernel as well.

https://lists.freedesktop.org/archives/dri-devel/2022-January/338550.html

[6by9]

Thanks for the PR. A few minor comments on the overlay.

[6by9]

Already defined in https://github.com/raspberrypi/linux/blob/rpi-5.10.y/arch/arm/boot/dts/bcm270x.dtsi#L235

[macromorgan]

Not sure how I missed that, but thank you. I can remove this node then at least when I resubmit.

[6by9]

This seems like an unrelated change as SPI is disabled by default so there should be no conflict. If someone does try enabling SPI, then it is right for the kernel to complain.

It also introduces overlay loading issues, as the overlay enabling SPI may be placed after this one, and it may reinstate the CS pins.

[macromorgan]

What would you advise I do? For this screen to work with the touchscreen I need SPI0 enabled, but I need the CS pins assigned to ALT2.

[6by9]

Overlay name hasn't been updated.

[macromorgan]

Dammit. Thanks, I'll fix.

[6by9]

I was looking at this yesterday. Having independent overlays for each and every display is going to become cumbersome. Adding overrides to vc4-kms-dpi-generic (or create one new one of vc4-kms-dpi for defined panels) would be more manageable.
vc4-kms-dpi-at056tn53v1 can be merged into -generic quite easily.
More complex displays (eg the Pimoroni HyperPixels that need spi-gpio for initialisation, and have touch controllers to configure too) would need to remain as separate overlays.

I wonder what @pelwell 's view is on that.

[pelwell]

I'm all for avoiding unnecessary duplication - whether that's by adding specialisations to the -generic case or by adding a -specific version for that purpose (name TBD) doesn't matter much, but the latter is marginally more appealing.

[macromorgan]

I'll be happy to remove this request if we can do it in a generic way. Honestly the only 2 (3) things that prevent it from working with the generic overlay today are:

1. There is no bus format defined. That can however be accomplished with 2 of the patches in this series and doesn't require the overlay at all to do. If this is the route you want to take I can resubmit this pull request with just the bus format changes to the VC4 driver and to the media headers.

2. I need to enable SPI0 but keep the SPI0 CS pins in ALT2 mode for the touchscreen to work. If I don't enable SPI0 there is no touchscreen, but if I don't keep the CS pins in ALT2 mode I get a horrible blue tint.

3-ish) This is kind of optional, but I should/could add a rotation property to the DPI generic overlay. The panel I'm submitting for in question is by default in a portrait orientation and is almost always rotated 270 degrees when used.

Let me know what you want me to do and I'll resubmit. Thank you.

[6by9]

• panel-simple.c got extended to take a bus-format property to allow the format to be specified when using the panel-dpi compatible.
  You've gone to the effort of getting a compatible merged upstream (congrats!), so you may as well use it, and that defines the bus format anyway. I'm fine with that. You need the patches adding that RGB565 mode to vc4_dpi.c anyway.

• Enabling SPI isn't done in this overlay, so changing chip selects shouldn't really be done in this overlay either. Which touch controller is it using? Is there an existing overlay for it that needs to be extended?

• I'm all for additional params if they make sense.

• I've had a quick chat with pelwell. Where we will be going will be a vc4-kms-dpi-panel overlay which takes an override to enable different defined panels (eg yours, and at056tn53v1).
  vc4-kms-dpi-generic will exist for providing a generic external timing/configuration.
  Both will include a new vc4-kms-dpi.dtsi which will define the base panel, dpi, and probably backlight properties.

It may take a day or so for me to find time to make the above changes. If you want to resubmit before those changes, then if you create the vc4-kms-dpi-panel-overlay.dts file with your panel as the only override, then we can do the shuffling of fragments around the place. If we merge your panel as vc4-kms-dpi-geekworm-mzp280, then it becomes an overhead that has to be carried forward to avoid breaking someone's system.

[macromorgan]

No worries, I'll wait for your update before I revise what I'm doing. I'm in no big hurry and the stuff I pushed upstream is still under review. Again, the biggest piece I need is the bus format definition. Without it I either have to use the fkms overlay or recompile my kernel; while with the proper bus format definition I only need a device tree overlay.

The touch controller is using the ads7846 overlay. SPI is enabled in that overlay but it looks like there's no SPI pinctrl overrides done in it. (I'm using "dtoverlay=ads7846,penirq=27,swapxy=1,xmin=200,xmax=3850,ymin=200,ymax=3850" in my config.txt). Again it's not so much that I'm changing the chip selects so much as completely disabling them and repurposing the pins to ALT2. While I see overlays that change the chip select pins (such as spi0-1cs-overlay.dts) I don't see an overlay that enables SPI0 but disables the SPI0 CS pins. Any thoughts on how to support that (and thus support this panel)?

[pelwell]

Unfortunately this doesn't do what you think - overlays can't delete properties or nodes.

[pelwell]

How is SPI0 supposed to function without chip selects? I'm not sure that's going to be possible now that hardware chip-select support has effectively been removed.

[macromorgan]

Presumably it's hard coded as 0? I honestly don't know.

edit: After looking at my GPIO configuration, I can't seem to find any pin that currently works as a CS anyway.
For my current setup I have GPIOs 0-8, 12-17, and 20-24 in ALT2 mode. GPIOs 9-11 are set up as SPI (MISO/MOSI/SCLK). GPIO 27 is set up as an input, it appears to control the touchscreen IRQ. While GPIO 18 is not used on my revision of the screen there are newer revisions of this geekworm panel that use it to control the backlight. That leaves GPIO 19, 25 and 26 which don't appear to do anything (to the display or the touchscreen) when toggled between 1, 0, in or out. Toggling pin 27 from input to output does disable the touchscreen, but I suppose that's to be expected.

Thank you.

[6by9]

#4860 is the basic change I was thinking of, however it does still need verifying.

[macromorgan]

I can test it out.

What do we do about the SPI0 though for this one? Could I just override the SPI0 CS GPIO to a nonsense value like 255?

[pelwell]

There is a recognised way to run SPI without using a chip select, but it seems to rely on passing the SPI_NO_CS flag with the transfer.

What does this overlay do for you (I named it spi0-0cs):

/dts-v1/;
/plugin/;


/ {
	compatible = "brcm,bcm2835";

	fragment@0 {
		target = <&spi0_cs_pins>;
		frag0: __overlay__ {
			brcm,pins;
		};
	};

	fragment@1 {
		target = <&spi0>;
		frag1: __overlay__ {
			cs-gpios;
			status = "okay";
		};
	};

	fragment@2 {
		target = <&spidev1>;
		__overlay__ {
			status = "disabled";
		};
	};

	fragment@3 {
		target = <&spi0_pins>;
		__dormant__ {
			brcm,pins = <10 11>;
		};
	};

	__overrides__ {
		no_miso = <0>,"=3";
	};
};

[pelwell]

There is another way - using the gpio-fsm overlay to create software GPIO that the SPI subsystem can waggle to its heart's content. I'll try and hack that into the overlay above.

[macromorgan]

Using spi0-0cs and removing the SPI stuff from the geekworm overlay causes the panel to work better than it ever did (no flashing or discoloration during boot).

:-)

[pelwell]

Cool. Here's the gpio_fsm version anyway, should you be interested:

/dts-v1/;
/plugin/;


/ {
	compatible = "brcm,bcm2835";

	fragment@0 {
		target = <&spi0_cs_pins>;
		frag0: __overlay__ {
			brcm,pins;
		};
	};

	fragment@1 {
		target = <&spi0>;
		__overlay__ {
			cs-gpios = <&fake_cs 0 0>;
			status = "okay";
		};
	};

	fragment@2 {
		target = <&spidev1>;
		__overlay__ {
			status = "disabled";
		};
	};

	fragment@3 {
		target = <&spi0_pins>;
		__dormant__ {
			brcm,pins = <10 11>;
		};
	};

	fragment@4 {
		target-path = "/";
		__overlay__ {
			fake_cs: fake_cs {
				compatible = "rpi,gpio-fsm";

				debug = <0>;
				gpio-controller;
				#gpio-cells = <2>;
				num-swgpios = <1>;
				gpio-line-names = "fake_cs";

				idle {
					start_state;
					shutdown_state;
				};
			};
	       };
        };

	__overrides__ {
		no_miso = <0>,"=3";
		fsm_debug = <&fake_cs>,"debug:0";
	};
};

[macromorgan]

gpio_fsm also works fine. Not as good as the 0cs one (gpio_fsm blinks during boot-up whereas the other one leaves the screen up the whole time), but it's no big deal at all and for all I know some sort of order-of-operations thing I need to solve myself.

[macromorgan]

#4860 is the basic change I was thinking of, however it does still need verifying.

Can this be made to set an override on the pinctrl-0 value for the DPI node? Also if I'm reading it, is it only allowing you to override part of the compatible string on the vc4-kms-dpi-panel overlay? (meaning I could replace the at056tn53v1 part but not the innolux part)

Thank you.

[6by9]

#4860 is the basic change I was thinking of, however it does still need verifying.

Can this be made to set an override on the pinctrl-0 value for the DPI node? Also if I'm reading it, is it only allowing you to override part of the compatible string on the vc4-kms-dpi-panel overlay? (meaning I could replace the at056tn53v1 part but not the innolux part)

The overrides section enables other fragments from earlier in the file. I've just updated it so that it includes an override for the Adafruit Kippah with 7inch panel as well.

	__overrides__ {
		at056tn53v1 = <0>, "+0+90";
		kippah-7inch = <0>, "+1+91";
	};

So at056tn53v1 enables fragment 0 to set the compatible string, and fragment 90 to choose dpi_18bit_cpadhi_gpio0.
kippah-7inch enables fragment 1 to set the compatible string, and fragment 91 to choose dpi_18bit_gpio0.

If your panel simply needs a compatible and DPI mode setting, then it follows the same pattern.
If you need to add a backlight, then it follows the same format as the overrides in vc4-kms-dpi.dtsi.

I'll let Phil comment on the PR as in theory we can set the compatible directly from the override without a fragment. You could then have (if I've got it right), at056tn53v1 = <&panel>,"compatible=innolux,at056tn53v1", <&dpi>,"pinctrl-0=",<&dpi_18bit_cpadhi_gpio0>
I think the current solution is preferable, and more in keeping with things like i2c-rtc, but I'm not sure.

[macromorgan]

Okay, tested vc4-kms-dpi-panel-overlay by adding the following:

    fragment@2 {
            target = <&panel>;
            __dormant__  {
                    compatible = "geekworm,mzp280", "simple-panel";
            };
    };

    __overrides__ {
            at056tn53v1 = <0>, "+0+90";
            mzp280 = <0>, "+2+93";
    };

I then added the following to my config.txt after removing all the other "geekworm" related stuff.
dtoverlay=vc4-kms-dpi-panel,mzp280

It seems to work just fine for me (albeit without rotation).

edit: Once the vc4-kms-dpi-panel-overlay stuff is done if you want I can rebase this patch. Also, I can add rotation support. Note that at least in my experience rotation support generates some dmesg warnings currently.
WARNING: CPU: 2 PID: 186 at drivers/gpu/drm/drm_mode_object.c:45 drm_mode_object_add+0x94/0xa0 [drm]
WARNING: CPU: 2 PID: 186 at drivers/gpu/drm/drm_mode_object.c:242 drm_object_attach_property+0x78/0xc0 [drm]

[6by9]

I'll add the rotation option to my PR and test. It should be possible to make your fragment

    fragment@2 {
            target = <&panel>;
            __dormant__  {
                    compatible = "geekworm,mzp280", "simple-panel";
                    rotation = <270>;
         };
    };

to set the default, and then we can have a generic override (probably wants to go in vc4-kms-dpi.dtsi) to allow user configuration of any panel using panel-simple.

The two WARNS are a little concerning as they imply a locking or sequencing issue.

WARN_ON(!dev->driver->load && dev->registered && !obj_free_cb);

WARN_ON(!dev->driver->load &&
			connector->registration_state == DRM_CONNECTOR_REGISTERED);

It looks like it's drm_connector_set_panel_orientation being called only from panel_simple_get_modes, but the property should be created in probe before the panel is registered with drm_panel_add.
I think that's only a couple of lines in probe to add the initial property, but all users of of_drm_get_panel_orientation and drm_connector_set_panel_orientation are reading from OF in probe and setting in get_modes, so they'll all have the same issue. I need to check out where the connector is created, as the orientation is a property of the connector and not the panel.
Slightly amusing that no one else has noticed it in mainline as yet as this has been in the kernel since 5.10 with torvalds/linux@5759c96

[6by9]

#4860 is now merged, so that gives the nice framework to add extra DPI panels into.

#4862 sorts out the WARNs from the rotation property.

[macromorgan]

I'll remove the last commit from this pull request then (creating the overlay), and then submit a new pull request for the changes to the vc4-kms-dpi-panel-overlay.dts file. Will this pull be acceptable without the last commit?

Thank you.

[macromorgan]

I'll add the rotation option to my PR and test. It should be possible to make your fragment

    fragment@2 {
            target = <&panel>;
            __dormant__  {
                    compatible = "geekworm,mzp280", "simple-panel";
                    rotation = <270>;
         };
    };

to set the default, and then we can have a generic override (probably wants to go in vc4-kms-dpi.dtsi) to allow user configuration of any panel using panel-simple.

The two WARNS are a little concerning as they imply a locking or sequencing issue.

WARN_ON(!dev->driver->load && dev->registered && !obj_free_cb);

WARN_ON(!dev->driver->load &&
			connector->registration_state == DRM_CONNECTOR_REGISTERED);

It looks like it's drm_connector_set_panel_orientation being called only from panel_simple_get_modes, but the property should be created in probe before the panel is registered with drm_panel_add. I think that's only a couple of lines in probe to add the initial property, but all users of of_drm_get_panel_orientation and drm_connector_set_panel_orientation are reading from OF in probe and setting in get_modes, so they'll all have the same issue. I need to check out where the connector is created, as the orientation is a property of the connector and not the panel. Slightly amusing that no one else has noticed it in mainline as yet as this has been in the kernel since 5.10 with torvalds/linux@5759c96

Ohh I've noticed it, I just haven't had time to debug it yet (thank you for doing this). I know at least the Odroid Go Advance on the 5.17 kernel gets this warning, and I've seen additional devices on the Allwinner A64 get this as well.

[6by9]

I'm good with that.
The first 6 patches are all the ones submitted upstream, and AFAICT they should be acceptable there.

[6by9]

Ohh I've noticed it, I just haven't had time to debug it yet (thank you for doing this). I know at least the Odroid Go Advance on the 5.17 kernel gets this warning, and I've seen additional devices on the Allwinner A64 get this as well.

I'm waiting for Maxime to look at #4862, and he'll then hopefully upstream it for me (I like having a mainline dev on hand to deal with upstream! It also means that he does the review to reduce any holdups)

[macromorgan]

I'm good with that. The first 6 patches are all the ones submitted upstream, and AFAICT they should be acceptable there.

Done, I removed the overlay and will add that to the new generic one that just got merged.