[PATCH v3 0/3] Make ILI9486 driver working with 16-bits SPI controllers

[PATCH v3 0/3] Make ILI9486 driver working with 16-bits SPI controllers

[Carlo Caione]

This patchset is trying to fix problems seen on S905X boards when interfacing
with an ILI9486 equipped SPI panel.

To: Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com>
To: David Airlie <airlied@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
To: Mark Brown <broonie@kernel.org>
To: Neil Armstrong <neil.armstrong@linaro.org>
To: Kevin Hilman <khilman@baylibre.com>
To: Jerome Brunet <jbrunet@baylibre.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-amlogic@lists.infradead.org
Signed-off-by: Carlo Caione <ccaione@baylibre.com>

---
Changes in v3:
- Added trailers
- Added new patch to use drm_aperture_remove_framebuffers()
- Link to v2: https://lore.kernel.org/r/20221116-s905x_spi_ili9486-v2-0-084c6e3cd930@baylibre.com

Changes in v2:
- Removed SPICC patch
- Reworked commit message
- Link to v1: https://lore.kernel.org/r/20221116-s905x_spi_ili9486-v1-0-630401cb62d5@baylibre.com

---
Carlo Caione (3):
      drm/tiny: ili9486: Enable driver module autoloading
      drm/tiny: ili9486: Do not assume 8-bit only SPI controllers
      drm/tiny: ili9486: remove conflicting framebuffers

 drivers/gpu/drm/tiny/ili9486.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)
---
base-commit: bce9332220bd677d83b19d21502776ad555a0e73
change-id: 20221116-s905x_spi_ili9486-aed54ff3cb21

Best regards,
-- 
Carlo Caione

[PATCH v3 1/3] drm/tiny: ili9486: Enable driver module autoloading

[Carlo Caione]

SPI devices use the spi_device_id for module autoloading even on
systems using device tree.

Add the spi_device_id entry to enable autoloading for the 3.5inch RPi
Display (rpi-lcd-35 and piscreen).

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 drivers/gpu/drm/tiny/ili9486.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
index c80028bb1d11..2ca7c59771f3 100644
--- a/drivers/gpu/drm/tiny/ili9486.c
+++ b/drivers/gpu/drm/tiny/ili9486.c
@@ -183,6 +183,8 @@ MODULE_DEVICE_TABLE(of, ili9486_of_match);
 
 static const struct spi_device_id ili9486_id[] = {
 	{ "ili9486", 0 },
+	{ "rpi-lcd-35", 0 },
+	{ "piscreen", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, ili9486_id);

-- 
b4 0.10.1

[PATCH v3 2/3] drm/tiny: ili9486: Do not assume 8-bit only SPI controllers

[Carlo Caione]

The pixel data for the ILI9486 is always 16-bits wide and it must be
sent over the SPI bus. When the controller is only able to deal with
8-bit transfers, this 16-bits data needs to be swapped before the
sending to account for the big endian bus, this is on the contrary not
needed when the SPI controller already supports 16-bits transfers.

The decision about swapping the pixel data or not is taken in the MIPI
DBI code by probing the controller capabilities: if the controller only
suppors 8-bit transfers the data is swapped, otherwise it is not.

This swapping/non-swapping is relying on the assumption that when the
controller does support 16-bit transactions then the data is sent
unswapped in 16-bits-per-word over SPI.

The problem with the ILI9486 driver is that it is forcing 8-bit
transactions also for controllers supporting 16-bits, violating the
assumption and corrupting the pixel data.

Align the driver to what is done in the MIPI DBI code by adjusting the
transfer size to the maximum allowed by the SPI controller.

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 drivers/gpu/drm/tiny/ili9486.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
index 2ca7c59771f3..14a9e6ad2d15 100644
--- a/drivers/gpu/drm/tiny/ili9486.c
+++ b/drivers/gpu/drm/tiny/ili9486.c
@@ -43,6 +43,7 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
 			     size_t num)
 {
 	struct spi_device *spi = mipi->spi;
+	unsigned int bpw = 8;
 	void *data = par;
 	u32 speed_hz;
 	int i, ret;
@@ -56,8 +57,6 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
 	 * The displays are Raspberry Pi HATs and connected to the 8-bit only
 	 * SPI controller, so 16-bit command and parameters need byte swapping
 	 * before being transferred as 8-bit on the big endian SPI bus.
-	 * Pixel data bytes have already been swapped before this function is
-	 * called.
 	 */
 	buf[0] = cpu_to_be16(*cmd);
 	gpiod_set_value_cansleep(mipi->dc, 0);
@@ -71,12 +70,18 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
 		for (i = 0; i < num; i++)
 			buf[i] = cpu_to_be16(par[i]);
 		num *= 2;
-		speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
 		data = buf;
 	}
 
+	/*
+	 * Check whether pixel data bytes needs to be swapped or not
+	 */
+	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
+		bpw = 16;
+
 	gpiod_set_value_cansleep(mipi->dc, 1);
-	ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, data, num);
+	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
+	ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw, data, num);
  free:
 	kfree(buf);
 

-- 
b4 0.10.1

[PATCH v3 3/3] drm/tiny: ili9486: remove conflicting framebuffers

[Carlo Caione]

For platforms using simplefb / efifb, call
drm_aperture_remove_framebuffers() to remove the conflicting
framebuffer.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 drivers/gpu/drm/tiny/ili9486.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
index 14a9e6ad2d15..6fd4d42437fd 100644
--- a/drivers/gpu/drm/tiny/ili9486.c
+++ b/drivers/gpu/drm/tiny/ili9486.c
@@ -14,6 +14,7 @@
 
 #include <video/mipi_display.h>
 
+#include <drm/drm_aperture.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_helper.h>
@@ -238,6 +239,10 @@ static int ili9486_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	ret = drm_aperture_remove_framebuffers(false, &ili9486_driver);
+	if (ret)
+		return ret;
+
 	drm_mode_config_reset(drm);
 
 	ret = drm_dev_register(drm, 0);

-- 
b4 0.10.1

Re: [PATCH v3 3/3] drm/tiny: ili9486: remove conflicting framebuffers

[Neil Armstrong]

Hi Carlo,

On 06/12/2022 09:34, Carlo Caione wrote:
> For platforms using simplefb / efifb, call
> drm_aperture_remove_framebuffers() to remove the conflicting
> framebuffer.

Conflicting framebuffer on the SPI display ? How is that possible ?

The meson_drm should already do this, no ?

Neil

> 
> Signed-off-by: Carlo Caione <ccaione@baylibre.com>
> ---
>   drivers/gpu/drm/tiny/ili9486.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
> index 14a9e6ad2d15..6fd4d42437fd 100644
> --- a/drivers/gpu/drm/tiny/ili9486.c
> +++ b/drivers/gpu/drm/tiny/ili9486.c
> @@ -14,6 +14,7 @@
>   
>   #include <video/mipi_display.h>
>   
> +#include <drm/drm_aperture.h>
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_drv.h>
>   #include <drm/drm_fb_helper.h>
> @@ -238,6 +239,10 @@ static int ili9486_probe(struct spi_device *spi)
>   	if (ret)
>   		return ret;
>   
> +	ret = drm_aperture_remove_framebuffers(false, &ili9486_driver);
> +	if (ret)
> +		return ret;
> +
>   	drm_mode_config_reset(drm);
>   
>   	ret = drm_dev_register(drm, 0);
> 

Re: [PATCH v3 3/3] drm/tiny: ili9486: remove conflicting framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1817 bytes --]

Hi

Am 06.12.22 um 10:41 schrieb Neil Armstrong:
> Hi Carlo,
> 
> On 06/12/2022 09:34, Carlo Caione wrote:
>> For platforms using simplefb / efifb, call
>> drm_aperture_remove_framebuffers() to remove the conflicting
>> framebuffer.
> 
> Conflicting framebuffer on the SPI display ? How is that possible ?

Calling drm_aperture_remove_framebuffers() is only required if the 
graphics card may have been pre-initialized by the system, such as a 
VGA-compatible card on a PC.

Could the SPI display have been initialized by the firmware? If not, the 
call should be left out.

Best regards
Thomas

> 
> The meson_drm should already do this, no ?
> 
> Neil
> 
>>
>> Signed-off-by: Carlo Caione <ccaione@baylibre.com>
>> ---
>>   drivers/gpu/drm/tiny/ili9486.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tiny/ili9486.c 
>> b/drivers/gpu/drm/tiny/ili9486.c
>> index 14a9e6ad2d15..6fd4d42437fd 100644
>> --- a/drivers/gpu/drm/tiny/ili9486.c
>> +++ b/drivers/gpu/drm/tiny/ili9486.c
>> @@ -14,6 +14,7 @@
>>   #include <video/mipi_display.h>
>> +#include <drm/drm_aperture.h>
>>   #include <drm/drm_atomic_helper.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_fb_helper.h>
>> @@ -238,6 +239,10 @@ static int ili9486_probe(struct spi_device *spi)
>>       if (ret)
>>           return ret;
>> +    ret = drm_aperture_remove_framebuffers(false, &ili9486_driver);
>> +    if (ret)
>> +        return ret;
>> +
>>       drm_mode_config_reset(drm);
>>       ret = drm_dev_register(drm, 0);
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3 3/3] drm/tiny: ili9486: remove conflicting framebuffers

[Carlo Caione]

On 06/12/2022 10:52, Thomas Zimmermann wrote:

>> Conflicting framebuffer on the SPI display ? How is that possible ?
> 
> Calling drm_aperture_remove_framebuffers() is only required if the 
> graphics card may have been pre-initialized by the system, such as a 
> VGA-compatible card on a PC.
> 
> Could the SPI display have been initialized by the firmware? If not, the 
> call should be left out.

What's happening on this board is that the builtin simpledrm driver is 
creating fb0 backed by the framebuffer prepared by u-boot / grub, and 
this the framebuffer being used by fbcon at early boot.

When the ILI9486 DRM driver is probed later during boot a second 
framebuffer is created (fb1) and when fb0 is destroyed, fbcon still 
remains attached to a non-existent framebuffer, so the user is left in 
the dark.

What this patch is doing is that when the ILI driver is probed, fb0 is 
destroyed and a new DRM-backed fb0 is created by the ILI DRM driver that 
can be used by fbcon, so the user can correctly see the console on the 
SPI display.

Cheers,

-- 
Carlo Caione

Re: [PATCH v3 3/3] drm/tiny: ili9486: remove conflicting framebuffers

[Javier Martinez Canillas]

On 12/6/22 10:52, Thomas Zimmermann wrote:
> Hi
> 
> Am 06.12.22 um 10:41 schrieb Neil Armstrong:
>> Hi Carlo,
>>
>> On 06/12/2022 09:34, Carlo Caione wrote:
>>> For platforms using simplefb / efifb, call
>>> drm_aperture_remove_framebuffers() to remove the conflicting
>>> framebuffer.
>>
>> Conflicting framebuffer on the SPI display ? How is that possible ?
> 
> Calling drm_aperture_remove_framebuffers() is only required if the 
> graphics card may have been pre-initialized by the system, such as a 
> VGA-compatible card on a PC.
> 
> Could the SPI display have been initialized by the firmware? If not, the 
> call should be left out.
>

Agree.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat