[PATCH 0/3] Fix SPICC and ILI9486 drivers

[PATCH 0/3] Fix SPICC and ILI9486 drivers

[Carlo Caione]

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

The color corruption and the performance issues are actually due to two
different problems in the SPICC driver and in the ILI9486 tiny DRM driver.

We try to fix both in the same patcheset to be able to correctly use the SPI
panel again.

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-spi@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-amlogic@lists.infradead.org
Signed-off-by: Carlo Caione <ccaione@baylibre.com>

---
Carlo Caione (3):
      drm/tiny: rpi-lcd-35: Enable driver module autoloading
      drm/tiny: ili9486: Do not assume 8-bit only SPI controllers
      spi: meson-spicc: Lower CS between bursts

 drivers/gpu/drm/tiny/ili9486.c | 14 ++++++++++----
 drivers/spi/spi-meson-spicc.c  |  6 ++++++
 2 files changed, 16 insertions(+), 4 deletions(-)
---
base-commit: 15f3bff12cf6a888ec2ad39652828c60e6836b3d
change-id: 20221116-s905x_spi_ili9486-aed54ff3cb21

Best regards,
-- 
Carlo Caione

[PATCH 0/3] Fix SPICC and ILI9486 drivers

[Carlo Caione]

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

The color corruption and the performance issues are actually due to two
different problems in the SPICC driver and in the ILI9486 tiny DRM driver.

We try to fix both in the same patcheset to be able to correctly use the SPI
panel again.

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-spi@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-amlogic@lists.infradead.org
Signed-off-by: Carlo Caione <ccaione@baylibre.com>

---
Carlo Caione (3):
      drm/tiny: rpi-lcd-35: Enable driver module autoloading
      drm/tiny: ili9486: Do not assume 8-bit only SPI controllers
      spi: meson-spicc: Lower CS between bursts

 drivers/gpu/drm/tiny/ili9486.c | 14 ++++++++++----
 drivers/spi/spi-meson-spicc.c  |  6 ++++++
 2 files changed, 16 insertions(+), 4 deletions(-)
---
base-commit: 15f3bff12cf6a888ec2ad39652828c60e6836b3d
change-id: 20221116-s905x_spi_ili9486-aed54ff3cb21

Best regards,
-- 
Carlo Caione

[PATCH 0/3] Fix SPICC and ILI9486 drivers

[Carlo Caione]

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

The color corruption and the performance issues are actually due to two
different problems in the SPICC driver and in the ILI9486 tiny DRM driver.

We try to fix both in the same patcheset to be able to correctly use the SPI
panel again.

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-spi@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-amlogic@lists.infradead.org
Signed-off-by: Carlo Caione <ccaione@baylibre.com>

---
Carlo Caione (3):
      drm/tiny: rpi-lcd-35: Enable driver module autoloading
      drm/tiny: ili9486: Do not assume 8-bit only SPI controllers
      spi: meson-spicc: Lower CS between bursts

 drivers/gpu/drm/tiny/ili9486.c | 14 ++++++++++----
 drivers/spi/spi-meson-spicc.c  |  6 ++++++
 2 files changed, 16 insertions(+), 4 deletions(-)
---
base-commit: 15f3bff12cf6a888ec2ad39652828c60e6836b3d
change-id: 20221116-s905x_spi_ili9486-aed54ff3cb21

Best regards,
-- 
Carlo Caione


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/3] Fix SPICC and ILI9486 drivers

[Carlo Caione]

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

The color corruption and the performance issues are actually due to two
different problems in the SPICC driver and in the ILI9486 tiny DRM driver.

We try to fix both in the same patcheset to be able to correctly use the SPI
panel again.

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-spi@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-amlogic@lists.infradead.org
Signed-off-by: Carlo Caione <ccaione@baylibre.com>

---
Carlo Caione (3):
      drm/tiny: rpi-lcd-35: Enable driver module autoloading
      drm/tiny: ili9486: Do not assume 8-bit only SPI controllers
      spi: meson-spicc: Lower CS between bursts

 drivers/gpu/drm/tiny/ili9486.c | 14 ++++++++++----
 drivers/spi/spi-meson-spicc.c  |  6 ++++++
 2 files changed, 16 insertions(+), 4 deletions(-)
---
base-commit: 15f3bff12cf6a888ec2ad39652828c60e6836b3d
change-id: 20221116-s905x_spi_ili9486-aed54ff3cb21

Best regards,
-- 
Carlo Caione


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 1/3] drm/tiny: rpi-lcd-35: 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).

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

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

-- 
b4 0.10.1

[PATCH 1/3] drm/tiny: rpi-lcd-35: 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).

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

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

-- 
b4 0.10.1

[PATCH 1/3] drm/tiny: rpi-lcd-35: 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).

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

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

-- 
b4 0.10.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] drm/tiny: rpi-lcd-35: 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).

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

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

-- 
b4 0.10.1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Carlo Caione]

The ILI9486 driver is wrongly assuming that the SPI panel is interfaced
only with 8-bit SPI controllers and consequently that the pixel data
passed by the MIPI DBI subsystem are already swapped before being sent
over SPI using 8 bits-per-word.

This is not always true for all the SPI controllers.

Make the command function more general to not only support 8-bit only
SPI controllers and support sending un-swapped data over SPI using 16
bits-per-word when dealing with pixel data.

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 bd37dfe8dd05..4d80a413338f 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 2/3] drm/tiny: ili9486: Do not assume 8-bit only SPI controllers

[Carlo Caione]

The ILI9486 driver is wrongly assuming that the SPI panel is interfaced
only with 8-bit SPI controllers and consequently that the pixel data
passed by the MIPI DBI subsystem are already swapped before being sent
over SPI using 8 bits-per-word.

This is not always true for all the SPI controllers.

Make the command function more general to not only support 8-bit only
SPI controllers and support sending un-swapped data over SPI using 16
bits-per-word when dealing with pixel data.

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 bd37dfe8dd05..4d80a413338f 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 2/3] drm/tiny: ili9486: Do not assume 8-bit only SPI controllers

[Carlo Caione]

The ILI9486 driver is wrongly assuming that the SPI panel is interfaced
only with 8-bit SPI controllers and consequently that the pixel data
passed by the MIPI DBI subsystem are already swapped before being sent
over SPI using 8 bits-per-word.

This is not always true for all the SPI controllers.

Make the command function more general to not only support 8-bit only
SPI controllers and support sending un-swapped data over SPI using 16
bits-per-word when dealing with pixel data.

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 bd37dfe8dd05..4d80a413338f 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Carlo Caione]

The ILI9486 driver is wrongly assuming that the SPI panel is interfaced
only with 8-bit SPI controllers and consequently that the pixel data
passed by the MIPI DBI subsystem are already swapped before being sent
over SPI using 8 bits-per-word.

This is not always true for all the SPI controllers.

Make the command function more general to not only support 8-bit only
SPI controllers and support sending un-swapped data over SPI using 16
bits-per-word when dealing with pixel data.

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 bd37dfe8dd05..4d80a413338f 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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

On Thu, Nov 17, 2022 at 09:47:40AM +0100, Carlo Caione wrote:
> The ILI9486 driver is wrongly assuming that the SPI panel is interfaced
> only with 8-bit SPI controllers and consequently that the pixel data
> passed by the MIPI DBI subsystem are already swapped before being sent
> over SPI using 8 bits-per-word.
> 
> This is not always true for all the SPI controllers.
> 
> Make the command function more general to not only support 8-bit only
> SPI controllers and support sending un-swapped data over SPI using 16
> bits-per-word when dealing with pixel data.

I don't understand what the commit log is saying here.  The
meson-spicc driver advertises support for 8 bit words, if the
driver is sending data formatted as a byte stream everything
should be fine.  It may be that there is some optimisation
available from taking advantage of the hardware's ability to
handle larger word sizes but there should be no data corruption
issue.

> +	/*
> +	 * Check whether pixel data bytes needs to be swapped or not
> +	 */
> +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
> +		bpw = 16;
> +

You should check the SPI controller compatibility here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

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

On Thu, Nov 17, 2022 at 09:47:40AM +0100, Carlo Caione wrote:
> The ILI9486 driver is wrongly assuming that the SPI panel is interfaced
> only with 8-bit SPI controllers and consequently that the pixel data
> passed by the MIPI DBI subsystem are already swapped before being sent
> over SPI using 8 bits-per-word.
> 
> This is not always true for all the SPI controllers.
> 
> Make the command function more general to not only support 8-bit only
> SPI controllers and support sending un-swapped data over SPI using 16
> bits-per-word when dealing with pixel data.

I don't understand what the commit log is saying here.  The
meson-spicc driver advertises support for 8 bit words, if the
driver is sending data formatted as a byte stream everything
should be fine.  It may be that there is some optimisation
available from taking advantage of the hardware's ability to
handle larger word sizes but there should be no data corruption
issue.

> +	/*
> +	 * Check whether pixel data bytes needs to be swapped or not
> +	 */
> +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
> +		bpw = 16;
> +

You should check the SPI controller compatibility here.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

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

On Thu, Nov 17, 2022 at 09:47:40AM +0100, Carlo Caione wrote:
> The ILI9486 driver is wrongly assuming that the SPI panel is interfaced
> only with 8-bit SPI controllers and consequently that the pixel data
> passed by the MIPI DBI subsystem are already swapped before being sent
> over SPI using 8 bits-per-word.
> 
> This is not always true for all the SPI controllers.
> 
> Make the command function more general to not only support 8-bit only
> SPI controllers and support sending un-swapped data over SPI using 16
> bits-per-word when dealing with pixel data.

I don't understand what the commit log is saying here.  The
meson-spicc driver advertises support for 8 bit words, if the
driver is sending data formatted as a byte stream everything
should be fine.  It may be that there is some optimisation
available from taking advantage of the hardware's ability to
handle larger word sizes but there should be no data corruption
issue.

> +	/*
> +	 * Check whether pixel data bytes needs to be swapped or not
> +	 */
> +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
> +		bpw = 16;
> +

You should check the SPI controller compatibility here.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

On Thu, Nov 17, 2022 at 09:47:40AM +0100, Carlo Caione wrote:
> The ILI9486 driver is wrongly assuming that the SPI panel is interfaced
> only with 8-bit SPI controllers and consequently that the pixel data
> passed by the MIPI DBI subsystem are already swapped before being sent
> over SPI using 8 bits-per-word.
> 
> This is not always true for all the SPI controllers.
> 
> Make the command function more general to not only support 8-bit only
> SPI controllers and support sending un-swapped data over SPI using 16
> bits-per-word when dealing with pixel data.

I don't understand what the commit log is saying here.  The
meson-spicc driver advertises support for 8 bit words, if the
driver is sending data formatted as a byte stream everything
should be fine.  It may be that there is some optimisation
available from taking advantage of the hardware's ability to
handle larger word sizes but there should be no data corruption
issue.

> +	/*
> +	 * Check whether pixel data bytes needs to be swapped or not
> +	 */
> +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
> +		bpw = 16;
> +

You should check the SPI controller compatibility here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Carlo Caione]

On 17/11/2022 12:09, Mark Brown wrote:

> I don't understand what the commit log is saying here.  The 
> meson-spicc driver advertises support for 8 bit words, if the driver 
> is sending data formatted as a byte stream everything should be fine.
> It may be that there is some optimisation available from taking 
> advantage of the hardware's ability to handle larger word sizes but 
> there should be no data corruption issue.


There is no data corruption but the 16-bit pixel data have per-pixel
bytes swapped: for example 0x55AD is sent instead of 0xAD55 and this is
causing the wrong color to be displayed on the panel.

The problem is that the current code is sending data with an hardcoded
bpw == 8 whether the data is swapped or not before the sending.

For 8-bit only controllers the data is swapped by the MIPI DBI code but
this is not true for controllers supporting 16-bit as well, but in both
cases we are sending the data out the same way with an 8 bpw.

So the same image is basically displayed differently whether the SPI
controller supports 16 bpw or not. I'm trying to fix this by sending
data with 16-bit bpw when the controller is supporting that.

Please note that this is what it is done also by mipi_dbi_typec3_command().


>> +	/* +	 * Check whether pixel data bytes needs to be swapped or not
>> +	 */ +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && 
>> !mipi->swap_bytes) +		bpw = 16; +
> 
> You should check the SPI controller compatibility here.

This is already done in mipi_dbi_spi_init() by using spi_is_bpw_supported().

Cheers,

--
Carlo Caione

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

[Carlo Caione]

On 17/11/2022 12:09, Mark Brown wrote:

> I don't understand what the commit log is saying here.  The 
> meson-spicc driver advertises support for 8 bit words, if the driver 
> is sending data formatted as a byte stream everything should be fine.
> It may be that there is some optimisation available from taking 
> advantage of the hardware's ability to handle larger word sizes but 
> there should be no data corruption issue.


There is no data corruption but the 16-bit pixel data have per-pixel
bytes swapped: for example 0x55AD is sent instead of 0xAD55 and this is
causing the wrong color to be displayed on the panel.

The problem is that the current code is sending data with an hardcoded
bpw == 8 whether the data is swapped or not before the sending.

For 8-bit only controllers the data is swapped by the MIPI DBI code but
this is not true for controllers supporting 16-bit as well, but in both
cases we are sending the data out the same way with an 8 bpw.

So the same image is basically displayed differently whether the SPI
controller supports 16 bpw or not. I'm trying to fix this by sending
data with 16-bit bpw when the controller is supporting that.

Please note that this is what it is done also by mipi_dbi_typec3_command().


>> +	/* +	 * Check whether pixel data bytes needs to be swapped or not
>> +	 */ +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && 
>> !mipi->swap_bytes) +		bpw = 16; +
> 
> You should check the SPI controller compatibility here.

This is already done in mipi_dbi_spi_init() by using spi_is_bpw_supported().

Cheers,

--
Carlo Caione

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

[Carlo Caione]

On 17/11/2022 12:09, Mark Brown wrote:

> I don't understand what the commit log is saying here.  The 
> meson-spicc driver advertises support for 8 bit words, if the driver 
> is sending data formatted as a byte stream everything should be fine.
> It may be that there is some optimisation available from taking 
> advantage of the hardware's ability to handle larger word sizes but 
> there should be no data corruption issue.


There is no data corruption but the 16-bit pixel data have per-pixel
bytes swapped: for example 0x55AD is sent instead of 0xAD55 and this is
causing the wrong color to be displayed on the panel.

The problem is that the current code is sending data with an hardcoded
bpw == 8 whether the data is swapped or not before the sending.

For 8-bit only controllers the data is swapped by the MIPI DBI code but
this is not true for controllers supporting 16-bit as well, but in both
cases we are sending the data out the same way with an 8 bpw.

So the same image is basically displayed differently whether the SPI
controller supports 16 bpw or not. I'm trying to fix this by sending
data with 16-bit bpw when the controller is supporting that.

Please note that this is what it is done also by mipi_dbi_typec3_command().


>> +	/* +	 * Check whether pixel data bytes needs to be swapped or not
>> +	 */ +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && 
>> !mipi->swap_bytes) +		bpw = 16; +
> 
> You should check the SPI controller compatibility here.

This is already done in mipi_dbi_spi_init() by using spi_is_bpw_supported().

Cheers,

--
Carlo Caione

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Carlo Caione]

On 17/11/2022 12:09, Mark Brown wrote:

> I don't understand what the commit log is saying here.  The 
> meson-spicc driver advertises support for 8 bit words, if the driver 
> is sending data formatted as a byte stream everything should be fine.
> It may be that there is some optimisation available from taking 
> advantage of the hardware's ability to handle larger word sizes but 
> there should be no data corruption issue.


There is no data corruption but the 16-bit pixel data have per-pixel
bytes swapped: for example 0x55AD is sent instead of 0xAD55 and this is
causing the wrong color to be displayed on the panel.

The problem is that the current code is sending data with an hardcoded
bpw == 8 whether the data is swapped or not before the sending.

For 8-bit only controllers the data is swapped by the MIPI DBI code but
this is not true for controllers supporting 16-bit as well, but in both
cases we are sending the data out the same way with an 8 bpw.

So the same image is basically displayed differently whether the SPI
controller supports 16 bpw or not. I'm trying to fix this by sending
data with 16-bit bpw when the controller is supporting that.

Please note that this is what it is done also by mipi_dbi_typec3_command().


>> +	/* +	 * Check whether pixel data bytes needs to be swapped or not
>> +	 */ +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && 
>> !mipi->swap_bytes) +		bpw = 16; +
> 
> You should check the SPI controller compatibility here.

This is already done in mipi_dbi_spi_init() by using spi_is_bpw_supported().

Cheers,

--
Carlo Caione

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]

On Thu, Nov 17, 2022 at 02:40:05PM +0100, Carlo Caione wrote:
> On 17/11/2022 12:09, Mark Brown wrote:

> > I don't understand what the commit log is saying here.  The meson-spicc
> > driver advertises support for 8 bit words, if the driver is sending data
> > formatted as a byte stream everything should be fine.
> > It may be that there is some optimisation available from taking
> > advantage of the hardware's ability to handle larger word sizes but
> > there should be no data corruption issue.

> There is no data corruption but the 16-bit pixel data have per-pixel
> bytes swapped: for example 0x55AD is sent instead of 0xAD55 and this is
> causing the wrong color to be displayed on the panel.

If the data is being unexpectedly byte swapped then clearly it is
being corrupted.  How is this byte swapping happening?  SPI
devices should default to doing 8 bit transfers, if things
randomly get put into anything other than 8 bit mode without the
client device explicitly asking for it then that seems really
bad.

> The problem is that the current code is sending data with an hardcoded
> bpw == 8 whether the data is swapped or not before the sending.

> For 8-bit only controllers the data is swapped by the MIPI DBI code but
> this is not true for controllers supporting 16-bit as well, but in both
> cases we are sending the data out the same way with an 8 bpw.

> So the same image is basically displayed differently whether the SPI
> controller supports 16 bpw or not. I'm trying to fix this by sending
> data with 16-bit bpw when the controller is supporting that.

So this is an issue in the MIPI DBI code where the interpretation
of the buffer passed in depends on both the a caller parameter
and the capabilities of the underlying SPI controller, meaning
that a driver can suddenly become buggy when used with a new
controller?  I can't really tell what the bits per word being
passed in along with the buffer is supposed to mean, I'd have
expected it to correspond to the format of the buffer but it
seems like perhaps the buffer is always formatted for 16 bits and
the callers are needing to pass in the capabilities of the
controller which is then also checked by the underlying code?
This all seems extremely confusing, I'm not surprised there's
bugs.

At the very least your changelog needs to express clearly what is
going on, the description doesn't appear to correspond to what
you're changing.

> Please note that this is what it is done also by mipi_dbi_typec3_command().

The core code does appear to have some checks for controller
capabilities...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

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

On Thu, Nov 17, 2022 at 02:40:05PM +0100, Carlo Caione wrote:
> On 17/11/2022 12:09, Mark Brown wrote:

> > I don't understand what the commit log is saying here.  The meson-spicc
> > driver advertises support for 8 bit words, if the driver is sending data
> > formatted as a byte stream everything should be fine.
> > It may be that there is some optimisation available from taking
> > advantage of the hardware's ability to handle larger word sizes but
> > there should be no data corruption issue.

> There is no data corruption but the 16-bit pixel data have per-pixel
> bytes swapped: for example 0x55AD is sent instead of 0xAD55 and this is
> causing the wrong color to be displayed on the panel.

If the data is being unexpectedly byte swapped then clearly it is
being corrupted.  How is this byte swapping happening?  SPI
devices should default to doing 8 bit transfers, if things
randomly get put into anything other than 8 bit mode without the
client device explicitly asking for it then that seems really
bad.

> The problem is that the current code is sending data with an hardcoded
> bpw == 8 whether the data is swapped or not before the sending.

> For 8-bit only controllers the data is swapped by the MIPI DBI code but
> this is not true for controllers supporting 16-bit as well, but in both
> cases we are sending the data out the same way with an 8 bpw.

> So the same image is basically displayed differently whether the SPI
> controller supports 16 bpw or not. I'm trying to fix this by sending
> data with 16-bit bpw when the controller is supporting that.

So this is an issue in the MIPI DBI code where the interpretation
of the buffer passed in depends on both the a caller parameter
and the capabilities of the underlying SPI controller, meaning
that a driver can suddenly become buggy when used with a new
controller?  I can't really tell what the bits per word being
passed in along with the buffer is supposed to mean, I'd have
expected it to correspond to the format of the buffer but it
seems like perhaps the buffer is always formatted for 16 bits and
the callers are needing to pass in the capabilities of the
controller which is then also checked by the underlying code?
This all seems extremely confusing, I'm not surprised there's
bugs.

At the very least your changelog needs to express clearly what is
going on, the description doesn't appear to correspond to what
you're changing.

> Please note that this is what it is done also by mipi_dbi_typec3_command().

The core code does appear to have some checks for controller
capabilities...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]

On Thu, Nov 17, 2022 at 02:40:05PM +0100, Carlo Caione wrote:
> On 17/11/2022 12:09, Mark Brown wrote:

> > I don't understand what the commit log is saying here.  The meson-spicc
> > driver advertises support for 8 bit words, if the driver is sending data
> > formatted as a byte stream everything should be fine.
> > It may be that there is some optimisation available from taking
> > advantage of the hardware's ability to handle larger word sizes but
> > there should be no data corruption issue.

> There is no data corruption but the 16-bit pixel data have per-pixel
> bytes swapped: for example 0x55AD is sent instead of 0xAD55 and this is
> causing the wrong color to be displayed on the panel.

If the data is being unexpectedly byte swapped then clearly it is
being corrupted.  How is this byte swapping happening?  SPI
devices should default to doing 8 bit transfers, if things
randomly get put into anything other than 8 bit mode without the
client device explicitly asking for it then that seems really
bad.

> The problem is that the current code is sending data with an hardcoded
> bpw == 8 whether the data is swapped or not before the sending.

> For 8-bit only controllers the data is swapped by the MIPI DBI code but
> this is not true for controllers supporting 16-bit as well, but in both
> cases we are sending the data out the same way with an 8 bpw.

> So the same image is basically displayed differently whether the SPI
> controller supports 16 bpw or not. I'm trying to fix this by sending
> data with 16-bit bpw when the controller is supporting that.

So this is an issue in the MIPI DBI code where the interpretation
of the buffer passed in depends on both the a caller parameter
and the capabilities of the underlying SPI controller, meaning
that a driver can suddenly become buggy when used with a new
controller?  I can't really tell what the bits per word being
passed in along with the buffer is supposed to mean, I'd have
expected it to correspond to the format of the buffer but it
seems like perhaps the buffer is always formatted for 16 bits and
the callers are needing to pass in the capabilities of the
controller which is then also checked by the underlying code?
This all seems extremely confusing, I'm not surprised there's
bugs.

At the very least your changelog needs to express clearly what is
going on, the description doesn't appear to correspond to what
you're changing.

> Please note that this is what it is done also by mipi_dbi_typec3_command().

The core code does appear to have some checks for controller
capabilities...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

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

On Thu, Nov 17, 2022 at 02:40:05PM +0100, Carlo Caione wrote:
> On 17/11/2022 12:09, Mark Brown wrote:

> > I don't understand what the commit log is saying here.  The meson-spicc
> > driver advertises support for 8 bit words, if the driver is sending data
> > formatted as a byte stream everything should be fine.
> > It may be that there is some optimisation available from taking
> > advantage of the hardware's ability to handle larger word sizes but
> > there should be no data corruption issue.

> There is no data corruption but the 16-bit pixel data have per-pixel
> bytes swapped: for example 0x55AD is sent instead of 0xAD55 and this is
> causing the wrong color to be displayed on the panel.

If the data is being unexpectedly byte swapped then clearly it is
being corrupted.  How is this byte swapping happening?  SPI
devices should default to doing 8 bit transfers, if things
randomly get put into anything other than 8 bit mode without the
client device explicitly asking for it then that seems really
bad.

> The problem is that the current code is sending data with an hardcoded
> bpw == 8 whether the data is swapped or not before the sending.

> For 8-bit only controllers the data is swapped by the MIPI DBI code but
> this is not true for controllers supporting 16-bit as well, but in both
> cases we are sending the data out the same way with an 8 bpw.

> So the same image is basically displayed differently whether the SPI
> controller supports 16 bpw or not. I'm trying to fix this by sending
> data with 16-bit bpw when the controller is supporting that.

So this is an issue in the MIPI DBI code where the interpretation
of the buffer passed in depends on both the a caller parameter
and the capabilities of the underlying SPI controller, meaning
that a driver can suddenly become buggy when used with a new
controller?  I can't really tell what the bits per word being
passed in along with the buffer is supposed to mean, I'd have
expected it to correspond to the format of the buffer but it
seems like perhaps the buffer is always formatted for 16 bits and
the callers are needing to pass in the capabilities of the
controller which is then also checked by the underlying code?
This all seems extremely confusing, I'm not surprised there's
bugs.

At the very least your changelog needs to express clearly what is
going on, the description doesn't appear to correspond to what
you're changing.

> Please note that this is what it is done also by mipi_dbi_typec3_command().

The core code does appear to have some checks for controller
capabilities...

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Carlo Caione]

On 17/11/2022 15:59, Mark Brown wrote:

> So this is an issue in the MIPI DBI code where the interpretation of 
> the buffer passed in depends on both the a caller parameter and the 
> capabilities of the underlying SPI controller, meaning that a driver 
> can suddenly become buggy when used with a new controller?

The MIPI DBI code is fine, in fact it is doing the correct thing in the
mipi_dbi_typec3_command() function. The problem is that the ILI9486
driver is hijacking that function installing its own hook that is wrong.

> I can't really tell what the bits per word being passed in along
> with the buffer is supposed to mean, I'd have expected it to
> correspond to the format of the buffer but it seems like perhaps the
> buffer is always formatted for 16 bits and the callers are needing to
> pass in the capabilities of the controller which is then also checked
> by the underlying code? This all seems extremely confusing, I'm not 
> surprised there's bugs.

Correct, the buffer (pixel data) is always formatted for 16 bits and the
bpw is to indicate how this data should be put on the bus (according to
the controller capability).

If the controller is only capable of 8-bit transfers, the 16-bit data
needs to be swapped to account for the big endian bus, while this is not
needed if the controller already supports 16-bit transfers.

The decision to swap the data or not is taken in the MIPI DBI code by
probing the controller capabilities, so if the controller only supports
8-bit the data is swapped, otherwise it is not.

The problem arrives when your controller does support 16-bits, so your
data is not swapped, but you still put the data on the bus with 8-bit
transfers.

> At the very least your changelog needs to express clearly what is 
> going on, the description doesn't appear to correspond to what
> you're changing.

Gotcha, I'll try to clarify that in the next revision.

Thanks,

--
Carlo Caione

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

[Carlo Caione]

On 17/11/2022 15:59, Mark Brown wrote:

> So this is an issue in the MIPI DBI code where the interpretation of 
> the buffer passed in depends on both the a caller parameter and the 
> capabilities of the underlying SPI controller, meaning that a driver 
> can suddenly become buggy when used with a new controller?

The MIPI DBI code is fine, in fact it is doing the correct thing in the
mipi_dbi_typec3_command() function. The problem is that the ILI9486
driver is hijacking that function installing its own hook that is wrong.

> I can't really tell what the bits per word being passed in along
> with the buffer is supposed to mean, I'd have expected it to
> correspond to the format of the buffer but it seems like perhaps the
> buffer is always formatted for 16 bits and the callers are needing to
> pass in the capabilities of the controller which is then also checked
> by the underlying code? This all seems extremely confusing, I'm not 
> surprised there's bugs.

Correct, the buffer (pixel data) is always formatted for 16 bits and the
bpw is to indicate how this data should be put on the bus (according to
the controller capability).

If the controller is only capable of 8-bit transfers, the 16-bit data
needs to be swapped to account for the big endian bus, while this is not
needed if the controller already supports 16-bit transfers.

The decision to swap the data or not is taken in the MIPI DBI code by
probing the controller capabilities, so if the controller only supports
8-bit the data is swapped, otherwise it is not.

The problem arrives when your controller does support 16-bits, so your
data is not swapped, but you still put the data on the bus with 8-bit
transfers.

> At the very least your changelog needs to express clearly what is 
> going on, the description doesn't appear to correspond to what
> you're changing.

Gotcha, I'll try to clarify that in the next revision.

Thanks,

--
Carlo Caione

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

[Carlo Caione]

On 17/11/2022 15:59, Mark Brown wrote:

> So this is an issue in the MIPI DBI code where the interpretation of 
> the buffer passed in depends on both the a caller parameter and the 
> capabilities of the underlying SPI controller, meaning that a driver 
> can suddenly become buggy when used with a new controller?

The MIPI DBI code is fine, in fact it is doing the correct thing in the
mipi_dbi_typec3_command() function. The problem is that the ILI9486
driver is hijacking that function installing its own hook that is wrong.

> I can't really tell what the bits per word being passed in along
> with the buffer is supposed to mean, I'd have expected it to
> correspond to the format of the buffer but it seems like perhaps the
> buffer is always formatted for 16 bits and the callers are needing to
> pass in the capabilities of the controller which is then also checked
> by the underlying code? This all seems extremely confusing, I'm not 
> surprised there's bugs.

Correct, the buffer (pixel data) is always formatted for 16 bits and the
bpw is to indicate how this data should be put on the bus (according to
the controller capability).

If the controller is only capable of 8-bit transfers, the 16-bit data
needs to be swapped to account for the big endian bus, while this is not
needed if the controller already supports 16-bit transfers.

The decision to swap the data or not is taken in the MIPI DBI code by
probing the controller capabilities, so if the controller only supports
8-bit the data is swapped, otherwise it is not.

The problem arrives when your controller does support 16-bits, so your
data is not swapped, but you still put the data on the bus with 8-bit
transfers.

> At the very least your changelog needs to express clearly what is 
> going on, the description doesn't appear to correspond to what
> you're changing.

Gotcha, I'll try to clarify that in the next revision.

Thanks,

--
Carlo Caione

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Carlo Caione]

On 17/11/2022 15:59, Mark Brown wrote:

> So this is an issue in the MIPI DBI code where the interpretation of 
> the buffer passed in depends on both the a caller parameter and the 
> capabilities of the underlying SPI controller, meaning that a driver 
> can suddenly become buggy when used with a new controller?

The MIPI DBI code is fine, in fact it is doing the correct thing in the
mipi_dbi_typec3_command() function. The problem is that the ILI9486
driver is hijacking that function installing its own hook that is wrong.

> I can't really tell what the bits per word being passed in along
> with the buffer is supposed to mean, I'd have expected it to
> correspond to the format of the buffer but it seems like perhaps the
> buffer is always formatted for 16 bits and the callers are needing to
> pass in the capabilities of the controller which is then also checked
> by the underlying code? This all seems extremely confusing, I'm not 
> surprised there's bugs.

Correct, the buffer (pixel data) is always formatted for 16 bits and the
bpw is to indicate how this data should be put on the bus (according to
the controller capability).

If the controller is only capable of 8-bit transfers, the 16-bit data
needs to be swapped to account for the big endian bus, while this is not
needed if the controller already supports 16-bit transfers.

The decision to swap the data or not is taken in the MIPI DBI code by
probing the controller capabilities, so if the controller only supports
8-bit the data is swapped, otherwise it is not.

The problem arrives when your controller does support 16-bits, so your
data is not swapped, but you still put the data on the bus with 8-bit
transfers.

> At the very least your changelog needs to express clearly what is 
> going on, the description doesn't appear to correspond to what
> you're changing.

Gotcha, I'll try to clarify that in the next revision.

Thanks,

--
Carlo Caione

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

On Fri, Nov 18, 2022 at 11:36:27AM +0100, Carlo Caione wrote:
> On 17/11/2022 15:59, Mark Brown wrote:

> > So this is an issue in the MIPI DBI code where the interpretation of the
> > buffer passed in depends on both the a caller parameter and the
> > capabilities of the underlying SPI controller, meaning that a driver can
> > suddenly become buggy when used with a new controller?

> The MIPI DBI code is fine, in fact it is doing the correct thing in the
> mipi_dbi_typec3_command() function. The problem is that the ILI9486
> driver is hijacking that function installing its own hook that is wrong.

Ah, I see - it's causing confusion because it peers into the
internals of the underlying code.

> The problem arrives when your controller does support 16-bits, so your
> data is not swapped, but you still put the data on the bus with 8-bit
> transfers.

Why would you need to use 8 bit transfers if the controller
supports 16 bits?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

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

On Fri, Nov 18, 2022 at 11:36:27AM +0100, Carlo Caione wrote:
> On 17/11/2022 15:59, Mark Brown wrote:

> > So this is an issue in the MIPI DBI code where the interpretation of the
> > buffer passed in depends on both the a caller parameter and the
> > capabilities of the underlying SPI controller, meaning that a driver can
> > suddenly become buggy when used with a new controller?

> The MIPI DBI code is fine, in fact it is doing the correct thing in the
> mipi_dbi_typec3_command() function. The problem is that the ILI9486
> driver is hijacking that function installing its own hook that is wrong.

Ah, I see - it's causing confusion because it peers into the
internals of the underlying code.

> The problem arrives when your controller does support 16-bits, so your
> data is not swapped, but you still put the data on the bus with 8-bit
> transfers.

Why would you need to use 8 bit transfers if the controller
supports 16 bits?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Brown]

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

On Fri, Nov 18, 2022 at 11:36:27AM +0100, Carlo Caione wrote:
> On 17/11/2022 15:59, Mark Brown wrote:

> > So this is an issue in the MIPI DBI code where the interpretation of the
> > buffer passed in depends on both the a caller parameter and the
> > capabilities of the underlying SPI controller, meaning that a driver can
> > suddenly become buggy when used with a new controller?

> The MIPI DBI code is fine, in fact it is doing the correct thing in the
> mipi_dbi_typec3_command() function. The problem is that the ILI9486
> driver is hijacking that function installing its own hook that is wrong.

Ah, I see - it's causing confusion because it peers into the
internals of the underlying code.

> The problem arrives when your controller does support 16-bits, so your
> data is not swapped, but you still put the data on the bus with 8-bit
> transfers.

Why would you need to use 8 bit transfers if the controller
supports 16 bits?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

On Fri, Nov 18, 2022 at 11:36:27AM +0100, Carlo Caione wrote:
> On 17/11/2022 15:59, Mark Brown wrote:

> > So this is an issue in the MIPI DBI code where the interpretation of the
> > buffer passed in depends on both the a caller parameter and the
> > capabilities of the underlying SPI controller, meaning that a driver can
> > suddenly become buggy when used with a new controller?

> The MIPI DBI code is fine, in fact it is doing the correct thing in the
> mipi_dbi_typec3_command() function. The problem is that the ILI9486
> driver is hijacking that function installing its own hook that is wrong.

Ah, I see - it's causing confusion because it peers into the
internals of the underlying code.

> The problem arrives when your controller does support 16-bits, so your
> data is not swapped, but you still put the data on the bus with 8-bit
> transfers.

Why would you need to use 8 bit transfers if the controller
supports 16 bits?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Carlo Caione]

On 18/11/2022 16:44, Mark Brown wrote:

>> The problem arrives when your controller does support 16-bits, so 
>> your data is not swapped, but you still put the data on the bus 
>> with 8-bit transfers.
> 
> Why would you need to use 8 bit transfers if the controller supports
>  16 bits?

No idea why this driver is forcing 8-bit transfers when the controller
supports 16-bits (this is what this patch is fixing).

My theory is that this driver was written with the Raspberry Pi HATs in
mind and (AFAICT) the RPi has an 8-bit only SPI controller so the driver
author didn't bother with anything different.

--
Carlo Caione

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

[Carlo Caione]

On 18/11/2022 16:44, Mark Brown wrote:

>> The problem arrives when your controller does support 16-bits, so 
>> your data is not swapped, but you still put the data on the bus 
>> with 8-bit transfers.
> 
> Why would you need to use 8 bit transfers if the controller supports
>  16 bits?

No idea why this driver is forcing 8-bit transfers when the controller
supports 16-bits (this is what this patch is fixing).

My theory is that this driver was written with the Raspberry Pi HATs in
mind and (AFAICT) the RPi has an 8-bit only SPI controller so the driver
author didn't bother with anything different.

--
Carlo Caione

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

[Carlo Caione]

On 18/11/2022 16:44, Mark Brown wrote:

>> The problem arrives when your controller does support 16-bits, so 
>> your data is not swapped, but you still put the data on the bus 
>> with 8-bit transfers.
> 
> Why would you need to use 8 bit transfers if the controller supports
>  16 bits?

No idea why this driver is forcing 8-bit transfers when the controller
supports 16-bits (this is what this patch is fixing).

My theory is that this driver was written with the Raspberry Pi HATs in
mind and (AFAICT) the RPi has an 8-bit only SPI controller so the driver
author didn't bother with anything different.

--
Carlo Caione


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Carlo Caione]

On 18/11/2022 16:44, Mark Brown wrote:

>> The problem arrives when your controller does support 16-bits, so 
>> your data is not swapped, but you still put the data on the bus 
>> with 8-bit transfers.
> 
> Why would you need to use 8 bit transfers if the controller supports
>  16 bits?

No idea why this driver is forcing 8-bit transfers when the controller
supports 16-bits (this is what this patch is fixing).

My theory is that this driver was written with the Raspberry Pi HATs in
mind and (AFAICT) the RPi has an 8-bit only SPI controller so the driver
author didn't bother with anything different.

--
Carlo Caione


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Carlo Caione]

On some hardware (reproduced on S905X) when a large payload is
transmitted over SPI in bursts at the end of each burst, the clock line
briefly fluctuates creating spurious clock transitions that are being
recognised by the connected device as a genuine pulses, creating an
offset in the data being transmitted.

Lower the GPIO CS between bursts to avoid the clock being interpreted as
valid.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 drivers/spi/spi-meson-spicc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
index d47f2623a60f..af8d74b53519 100644
--- a/drivers/spi/spi-meson-spicc.c
+++ b/drivers/spi/spi-meson-spicc.c
@@ -291,6 +291,10 @@ static inline void meson_spicc_setup_burst(struct meson_spicc_device *spicc)
 static irqreturn_t meson_spicc_irq(int irq, void *data)
 {
 	struct meson_spicc_device *spicc = (void *) data;
+	struct spi_device *spi_dev;
+
+	spi_dev = spicc->message->spi;
+	gpiod_set_value(spi_dev->cs_gpiod, 0);
 
 	writel_bits_relaxed(SPICC_TC, SPICC_TC, spicc->base + SPICC_STATREG);
 
@@ -309,6 +313,8 @@ static irqreturn_t meson_spicc_irq(int irq, void *data)
 	/* Setup burst */
 	meson_spicc_setup_burst(spicc);
 
+	gpiod_set_value(spi_dev->cs_gpiod, 1);
+
 	/* Start burst */
 	writel_bits_relaxed(SPICC_XCH, SPICC_XCH, spicc->base + SPICC_CONREG);
 

-- 
b4 0.10.1

[PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Carlo Caione]

On some hardware (reproduced on S905X) when a large payload is
transmitted over SPI in bursts at the end of each burst, the clock line
briefly fluctuates creating spurious clock transitions that are being
recognised by the connected device as a genuine pulses, creating an
offset in the data being transmitted.

Lower the GPIO CS between bursts to avoid the clock being interpreted as
valid.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 drivers/spi/spi-meson-spicc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
index d47f2623a60f..af8d74b53519 100644
--- a/drivers/spi/spi-meson-spicc.c
+++ b/drivers/spi/spi-meson-spicc.c
@@ -291,6 +291,10 @@ static inline void meson_spicc_setup_burst(struct meson_spicc_device *spicc)
 static irqreturn_t meson_spicc_irq(int irq, void *data)
 {
 	struct meson_spicc_device *spicc = (void *) data;
+	struct spi_device *spi_dev;
+
+	spi_dev = spicc->message->spi;
+	gpiod_set_value(spi_dev->cs_gpiod, 0);
 
 	writel_bits_relaxed(SPICC_TC, SPICC_TC, spicc->base + SPICC_STATREG);
 
@@ -309,6 +313,8 @@ static irqreturn_t meson_spicc_irq(int irq, void *data)
 	/* Setup burst */
 	meson_spicc_setup_burst(spicc);
 
+	gpiod_set_value(spi_dev->cs_gpiod, 1);
+
 	/* Start burst */
 	writel_bits_relaxed(SPICC_XCH, SPICC_XCH, spicc->base + SPICC_CONREG);
 

-- 
b4 0.10.1

[PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Carlo Caione]

On some hardware (reproduced on S905X) when a large payload is
transmitted over SPI in bursts at the end of each burst, the clock line
briefly fluctuates creating spurious clock transitions that are being
recognised by the connected device as a genuine pulses, creating an
offset in the data being transmitted.

Lower the GPIO CS between bursts to avoid the clock being interpreted as
valid.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 drivers/spi/spi-meson-spicc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
index d47f2623a60f..af8d74b53519 100644
--- a/drivers/spi/spi-meson-spicc.c
+++ b/drivers/spi/spi-meson-spicc.c
@@ -291,6 +291,10 @@ static inline void meson_spicc_setup_burst(struct meson_spicc_device *spicc)
 static irqreturn_t meson_spicc_irq(int irq, void *data)
 {
 	struct meson_spicc_device *spicc = (void *) data;
+	struct spi_device *spi_dev;
+
+	spi_dev = spicc->message->spi;
+	gpiod_set_value(spi_dev->cs_gpiod, 0);
 
 	writel_bits_relaxed(SPICC_TC, SPICC_TC, spicc->base + SPICC_STATREG);
 
@@ -309,6 +313,8 @@ static irqreturn_t meson_spicc_irq(int irq, void *data)
 	/* Setup burst */
 	meson_spicc_setup_burst(spicc);
 
+	gpiod_set_value(spi_dev->cs_gpiod, 1);
+
 	/* Start burst */
 	writel_bits_relaxed(SPICC_XCH, SPICC_XCH, spicc->base + SPICC_CONREG);
 

-- 
b4 0.10.1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Carlo Caione]

On some hardware (reproduced on S905X) when a large payload is
transmitted over SPI in bursts at the end of each burst, the clock line
briefly fluctuates creating spurious clock transitions that are being
recognised by the connected device as a genuine pulses, creating an
offset in the data being transmitted.

Lower the GPIO CS between bursts to avoid the clock being interpreted as
valid.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
 drivers/spi/spi-meson-spicc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
index d47f2623a60f..af8d74b53519 100644
--- a/drivers/spi/spi-meson-spicc.c
+++ b/drivers/spi/spi-meson-spicc.c
@@ -291,6 +291,10 @@ static inline void meson_spicc_setup_burst(struct meson_spicc_device *spicc)
 static irqreturn_t meson_spicc_irq(int irq, void *data)
 {
 	struct meson_spicc_device *spicc = (void *) data;
+	struct spi_device *spi_dev;
+
+	spi_dev = spicc->message->spi;
+	gpiod_set_value(spi_dev->cs_gpiod, 0);
 
 	writel_bits_relaxed(SPICC_TC, SPICC_TC, spicc->base + SPICC_STATREG);
 
@@ -309,6 +313,8 @@ static irqreturn_t meson_spicc_irq(int irq, void *data)
 	/* Setup burst */
 	meson_spicc_setup_burst(spicc);
 
+	gpiod_set_value(spi_dev->cs_gpiod, 1);
+
 	/* Start burst */
 	writel_bits_relaxed(SPICC_XCH, SPICC_XCH, spicc->base + SPICC_CONREG);
 

-- 
b4 0.10.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Neil Armstrong]

Hi,

On 17/11/2022 09:47, Carlo Caione wrote:
> On some hardware (reproduced on S905X) when a large payload is
> transmitted over SPI in bursts at the end of each burst, the clock line
> briefly fluctuates creating spurious clock transitions that are being
> recognised by the connected device as a genuine pulses, creating an
> offset in the data being transmitted.
> 
> Lower the GPIO CS between bursts to avoid the clock being interpreted as
> valid.

I'm afraid this will actually break SPI NORs for example where CS actually splits
transactions.

Isn't Amjad change enough ? The CLK pull-up should avoid this.

If it's not the case, then it's an HW issue and the CLK line pull-up is too weak and an
external pull should then be added.

> 
> Signed-off-by: Carlo Caione <ccaione@baylibre.com>
> ---
>   drivers/spi/spi-meson-spicc.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> index d47f2623a60f..af8d74b53519 100644
> --- a/drivers/spi/spi-meson-spicc.c
> +++ b/drivers/spi/spi-meson-spicc.c
> @@ -291,6 +291,10 @@ static inline void meson_spicc_setup_burst(struct meson_spicc_device *spicc)
>   static irqreturn_t meson_spicc_irq(int irq, void *data)
>   {
>   	struct meson_spicc_device *spicc = (void *) data;
> +	struct spi_device *spi_dev;
> +
> +	spi_dev = spicc->message->spi;
> +	gpiod_set_value(spi_dev->cs_gpiod, 0);
>   
>   	writel_bits_relaxed(SPICC_TC, SPICC_TC, spicc->base + SPICC_STATREG);
>   
> @@ -309,6 +313,8 @@ static irqreturn_t meson_spicc_irq(int irq, void *data)
>   	/* Setup burst */
>   	meson_spicc_setup_burst(spicc);
>   
> +	gpiod_set_value(spi_dev->cs_gpiod, 1);
> +
>   	/* Start burst */
>   	writel_bits_relaxed(SPICC_XCH, SPICC_XCH, spicc->base + SPICC_CONREG);
>   
> 

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Neil Armstrong]

Hi,

On 17/11/2022 09:47, Carlo Caione wrote:
> On some hardware (reproduced on S905X) when a large payload is
> transmitted over SPI in bursts at the end of each burst, the clock line
> briefly fluctuates creating spurious clock transitions that are being
> recognised by the connected device as a genuine pulses, creating an
> offset in the data being transmitted.
> 
> Lower the GPIO CS between bursts to avoid the clock being interpreted as
> valid.

I'm afraid this will actually break SPI NORs for example where CS actually splits
transactions.

Isn't Amjad change enough ? The CLK pull-up should avoid this.

If it's not the case, then it's an HW issue and the CLK line pull-up is too weak and an
external pull should then be added.

> 
> Signed-off-by: Carlo Caione <ccaione@baylibre.com>
> ---
>   drivers/spi/spi-meson-spicc.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> index d47f2623a60f..af8d74b53519 100644
> --- a/drivers/spi/spi-meson-spicc.c
> +++ b/drivers/spi/spi-meson-spicc.c
> @@ -291,6 +291,10 @@ static inline void meson_spicc_setup_burst(struct meson_spicc_device *spicc)
>   static irqreturn_t meson_spicc_irq(int irq, void *data)
>   {
>   	struct meson_spicc_device *spicc = (void *) data;
> +	struct spi_device *spi_dev;
> +
> +	spi_dev = spicc->message->spi;
> +	gpiod_set_value(spi_dev->cs_gpiod, 0);
>   
>   	writel_bits_relaxed(SPICC_TC, SPICC_TC, spicc->base + SPICC_STATREG);
>   
> @@ -309,6 +313,8 @@ static irqreturn_t meson_spicc_irq(int irq, void *data)
>   	/* Setup burst */
>   	meson_spicc_setup_burst(spicc);
>   
> +	gpiod_set_value(spi_dev->cs_gpiod, 1);
> +
>   	/* Start burst */
>   	writel_bits_relaxed(SPICC_XCH, SPICC_XCH, spicc->base + SPICC_CONREG);
>   
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Neil Armstrong]

Hi,

On 17/11/2022 09:47, Carlo Caione wrote:
> On some hardware (reproduced on S905X) when a large payload is
> transmitted over SPI in bursts at the end of each burst, the clock line
> briefly fluctuates creating spurious clock transitions that are being
> recognised by the connected device as a genuine pulses, creating an
> offset in the data being transmitted.
> 
> Lower the GPIO CS between bursts to avoid the clock being interpreted as
> valid.

I'm afraid this will actually break SPI NORs for example where CS actually splits
transactions.

Isn't Amjad change enough ? The CLK pull-up should avoid this.

If it's not the case, then it's an HW issue and the CLK line pull-up is too weak and an
external pull should then be added.

> 
> Signed-off-by: Carlo Caione <ccaione@baylibre.com>
> ---
>   drivers/spi/spi-meson-spicc.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> index d47f2623a60f..af8d74b53519 100644
> --- a/drivers/spi/spi-meson-spicc.c
> +++ b/drivers/spi/spi-meson-spicc.c
> @@ -291,6 +291,10 @@ static inline void meson_spicc_setup_burst(struct meson_spicc_device *spicc)
>   static irqreturn_t meson_spicc_irq(int irq, void *data)
>   {
>   	struct meson_spicc_device *spicc = (void *) data;
> +	struct spi_device *spi_dev;
> +
> +	spi_dev = spicc->message->spi;
> +	gpiod_set_value(spi_dev->cs_gpiod, 0);
>   
>   	writel_bits_relaxed(SPICC_TC, SPICC_TC, spicc->base + SPICC_STATREG);
>   
> @@ -309,6 +313,8 @@ static irqreturn_t meson_spicc_irq(int irq, void *data)
>   	/* Setup burst */
>   	meson_spicc_setup_burst(spicc);
>   
> +	gpiod_set_value(spi_dev->cs_gpiod, 1);
> +
>   	/* Start burst */
>   	writel_bits_relaxed(SPICC_XCH, SPICC_XCH, spicc->base + SPICC_CONREG);
>   
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Neil Armstrong]

Hi,

On 17/11/2022 09:47, Carlo Caione wrote:
> On some hardware (reproduced on S905X) when a large payload is
> transmitted over SPI in bursts at the end of each burst, the clock line
> briefly fluctuates creating spurious clock transitions that are being
> recognised by the connected device as a genuine pulses, creating an
> offset in the data being transmitted.
> 
> Lower the GPIO CS between bursts to avoid the clock being interpreted as
> valid.

I'm afraid this will actually break SPI NORs for example where CS actually splits
transactions.

Isn't Amjad change enough ? The CLK pull-up should avoid this.

If it's not the case, then it's an HW issue and the CLK line pull-up is too weak and an
external pull should then be added.

> 
> Signed-off-by: Carlo Caione <ccaione@baylibre.com>
> ---
>   drivers/spi/spi-meson-spicc.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> index d47f2623a60f..af8d74b53519 100644
> --- a/drivers/spi/spi-meson-spicc.c
> +++ b/drivers/spi/spi-meson-spicc.c
> @@ -291,6 +291,10 @@ static inline void meson_spicc_setup_burst(struct meson_spicc_device *spicc)
>   static irqreturn_t meson_spicc_irq(int irq, void *data)
>   {
>   	struct meson_spicc_device *spicc = (void *) data;
> +	struct spi_device *spi_dev;
> +
> +	spi_dev = spicc->message->spi;
> +	gpiod_set_value(spi_dev->cs_gpiod, 0);
>   
>   	writel_bits_relaxed(SPICC_TC, SPICC_TC, spicc->base + SPICC_STATREG);
>   
> @@ -309,6 +313,8 @@ static irqreturn_t meson_spicc_irq(int irq, void *data)
>   	/* Setup burst */
>   	meson_spicc_setup_burst(spicc);
>   
> +	gpiod_set_value(spi_dev->cs_gpiod, 1);
> +
>   	/* Start burst */
>   	writel_bits_relaxed(SPICC_XCH, SPICC_XCH, spicc->base + SPICC_CONREG);
>   
> 

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Carlo Caione]

On 17/11/2022 09:54, Neil Armstrong wrote:

> I'm afraid this will actually break SPI NORs for example where CS 
> actually splits transactions.
> 
> Isn't Amjad change enough ? The CLK pull-up should avoid this.

It looks like it is not enough unfortunately.

> If it's not the case, then it's an HW issue and the CLK line pull-up 
> is too weak and an external pull should then be added.
	
Alright, I'll drop this patch in the next respin if needed.

Thanks,

--
Carlo Caione

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Carlo Caione]

On 17/11/2022 09:54, Neil Armstrong wrote:

> I'm afraid this will actually break SPI NORs for example where CS 
> actually splits transactions.
> 
> Isn't Amjad change enough ? The CLK pull-up should avoid this.

It looks like it is not enough unfortunately.

> If it's not the case, then it's an HW issue and the CLK line pull-up 
> is too weak and an external pull should then be added.
	
Alright, I'll drop this patch in the next respin if needed.

Thanks,

--
Carlo Caione

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Carlo Caione]

On 17/11/2022 09:54, Neil Armstrong wrote:

> I'm afraid this will actually break SPI NORs for example where CS 
> actually splits transactions.
> 
> Isn't Amjad change enough ? The CLK pull-up should avoid this.

It looks like it is not enough unfortunately.

> If it's not the case, then it's an HW issue and the CLK line pull-up 
> is too weak and an external pull should then be added.
	
Alright, I'll drop this patch in the next respin if needed.

Thanks,

--
Carlo Caione

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Carlo Caione]

On 17/11/2022 09:54, Neil Armstrong wrote:

> I'm afraid this will actually break SPI NORs for example where CS 
> actually splits transactions.
> 
> Isn't Amjad change enough ? The CLK pull-up should avoid this.

It looks like it is not enough unfortunately.

> If it's not the case, then it's an HW issue and the CLK line pull-up 
> is too weak and an external pull should then be added.
	
Alright, I'll drop this patch in the next respin if needed.

Thanks,

--
Carlo Caione

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Thu, Nov 17, 2022 at 09:47:41AM +0100, Carlo Caione wrote:
> On some hardware (reproduced on S905X) when a large payload is
> transmitted over SPI in bursts at the end of each burst, the clock line
> briefly fluctuates creating spurious clock transitions that are being
> recognised by the connected device as a genuine pulses, creating an
> offset in the data being transmitted.

> Lower the GPIO CS between bursts to avoid the clock being interpreted as
> valid.

This is just plain broken, *many* SPI devices attach meaning to
chip select edges - for example register writes will typically
have the register address followed by one or more register values
for sequential registers.  Bouncing chip select in the middle of
transfer will corrupt data.  If the device can't handle larger
transfers it needs to advertise this limit and refuse to handle
them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Mark Brown]

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

On Thu, Nov 17, 2022 at 09:47:41AM +0100, Carlo Caione wrote:
> On some hardware (reproduced on S905X) when a large payload is
> transmitted over SPI in bursts at the end of each burst, the clock line
> briefly fluctuates creating spurious clock transitions that are being
> recognised by the connected device as a genuine pulses, creating an
> offset in the data being transmitted.

> Lower the GPIO CS between bursts to avoid the clock being interpreted as
> valid.

This is just plain broken, *many* SPI devices attach meaning to
chip select edges - for example register writes will typically
have the register address followed by one or more register values
for sequential registers.  Bouncing chip select in the middle of
transfer will corrupt data.  If the device can't handle larger
transfers it needs to advertise this limit and refuse to handle
them.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Mark Brown]

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

On Thu, Nov 17, 2022 at 09:47:41AM +0100, Carlo Caione wrote:
> On some hardware (reproduced on S905X) when a large payload is
> transmitted over SPI in bursts at the end of each burst, the clock line
> briefly fluctuates creating spurious clock transitions that are being
> recognised by the connected device as a genuine pulses, creating an
> offset in the data being transmitted.

> Lower the GPIO CS between bursts to avoid the clock being interpreted as
> valid.

This is just plain broken, *many* SPI devices attach meaning to
chip select edges - for example register writes will typically
have the register address followed by one or more register values
for sequential registers.  Bouncing chip select in the middle of
transfer will corrupt data.  If the device can't handle larger
transfers it needs to advertise this limit and refuse to handle
them.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 3/3] spi: meson-spicc: Lower CS between bursts

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Thu, Nov 17, 2022 at 09:47:41AM +0100, Carlo Caione wrote:
> On some hardware (reproduced on S905X) when a large payload is
> transmitted over SPI in bursts at the end of each burst, the clock line
> briefly fluctuates creating spurious clock transitions that are being
> recognised by the connected device as a genuine pulses, creating an
> offset in the data being transmitted.

> Lower the GPIO CS between bursts to avoid the clock being interpreted as
> valid.

This is just plain broken, *many* SPI devices attach meaning to
chip select edges - for example register writes will typically
have the register address followed by one or more register values
for sequential registers.  Bouncing chip select in the middle of
transfer will corrupt data.  If the device can't handle larger
transfers it needs to advertise this limit and refuse to handle
them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]