stm32h7: dsi: ltdc: clock: PLL3: clock not set up correctly or side effect

[dariis]

Bug or Not

https://lists.zephyrproject.org/g/devel/topic/95406281#8795
Thanks to @erwango for the fast response.

Describe the bug
Using the DSI host on stm32h747i-disco with a custom display needs additional initialisation of the PLL3.
I'm not sure if I'm hunting a ghost, because I still have other issues with the display (init sequence).

Bad Case

To Reproduce
Vanilla display_stm32_ltdc.c Zephyr v3.2.0.
stm32h747i_disco_m7.overlay:

&pll3 {
	status = "okay";
	clocks = <&clk_hse>;
	div-m = <5>;
	mul-n = <132>;
	div-p = <2>;
	div-q = <2>;
	div-r = <24>; /* 27.5 MHz */
};

showing color bars with HAL_DSI_PatternGeneratorStart(&hdsi, 0, 1)

Expected behavior
Show color bars.

Logs and console output
Colors do not match.

Better Case

To Reproduce
patched display_stm32_ltdc.c

diff --git a/drivers/display/display_stm32_ltdc.c b/drivers/display/display_stm32_ltdc.c
index 840c276e28..04d387cf37 100644
--- a/drivers/display/display_stm32_ltdc.c
+++ b/drivers/display/display_stm32_ltdc.c
@@ -280,7 +280,18 @@ static int stm32_ltdc_init(const struct device *dev)
                LOG_ERR("clock control device not ready");
                return -ENODEV;
        }
-
+#if 1
+       RCC_PeriphCLKInitTypeDef  PeriphClkInitStruct;
+
+       PeriphClkInitStruct.PeriphClockSelection   = RCC_PERIPHCLK_LTDC;
+       PeriphClkInitStruct.PLL3.PLL3M      = DT_PROP(DT_NODELABEL(pll3), div_m);
+       PeriphClkInitStruct.PLL3.PLL3N      = DT_PROP(DT_NODELABEL(pll3), mul_n);
+       PeriphClkInitStruct.PLL3.PLL3P      = DT_PROP_OR(DT_NODELABEL(pll3), div_p, 1);
+       PeriphClkInitStruct.PLL3.PLL3Q      = DT_PROP_OR(DT_NODELABEL(pll3), div_q, 1);
+       PeriphClkInitStruct.PLL3.PLL3R      = DT_PROP_OR(DT_NODELABEL(pll3), div_r, 1);
+       PeriphClkInitStruct.PLL3.PLL3FRACN  = 0U;
+       HAL_RCCEx_PeriphCLKConfig(&PeriphClkInitStruct);
+#endif
        /* Turn on LTDC peripheral clock */
        err = clock_control_on(DEVICE_DT_GET(STM32_CLOCK_CONTROL_NODE),
                                (clock_control_subsys_t) &config->pclken);

same overlay and showing color bars as above.

Expected behavior
Show color bars.

Logs and console output
stm32h7 DSI host experts will see that the R and B colors are flipped, but the colors match.

Comments

Environment (please complete the following information):

• OS: Linux
• Zephyr SDK 15.1
• v3.2.0

Additional context
RCC register diff from gdb x/1024xw 0x58024400:

< 0x58024420:     0x00000040      0x00000000      0x00520052      0x01ff0808
---
> 0x58024420:     0x00000040      0x00000000      0x00520052      0x01ff0bd8

Set RCC_PeriphCLKInitTypeDef PeriphClkInitStruct = {0}; to zero gives the following diff:

< 0x58024420:     0x00000040      0x00000000      0x00520052      0x01ff0808
---
> 0x58024420:     0x00000040      0x00000000      0x00520052      0x01ff0108

rcc 0x2C: 0x01ff0808 = Bad Case

rcc 0x2C: 0x01ff0108, 0x01ff0bd8 = Better Case

Could this be the problem?
Or is the problem more likely at the display init sequence?
Or at the LTDC and DSI init order?

---

I'm lost...
I appreciate every comment, critics and hints.

[erwango]

Just a hint that will help you reading registers in gdb: https://github.com/1udo6arre/svd-tools.
It could also be used to dump the full register in good/bad cases and then compare dump files.

[erwango]

@dariis Since this is not an actual zephyr bug (DSI not supported) I'll move this to discussion, but github remains a good place to discuss.
Then I'm not quit eclear on your set up: DSI and LTDC are 2 differents IPs, so I'm a bit surprised to see that you're able to achieve somewhat functional DSI using LTDC driver. Am I missing something ?

[erwango]

@dariis I've just seen you pushed DSI driver #53044. A big thanks.
Does it means that you solved issues mentioned in this discussion ?

[dariis]

I wrapped the driver to Zephyr by adding something like that to the otm8009a.c:

#define DT_DRV_COMPAT orisetech_otm8009a

#include <zephyr/kernel.h>
#include <zephyr/drivers/display.h>
#include <zephyr/drivers/mipi_dsi.h>
#include <zephyr/drivers/gpio.h>
#include <zephyr/logging/log.h>

LOG_MODULE_REGISTER(otm8009a, CONFIG_DISPLAY_LOG_LEVEL);

struct otm8009a_config {
	const struct device *mipi_dsi;
	const struct gpio_dt_spec reset;
	const struct gpio_dt_spec backlight;
	uint8_t num_of_lanes;
	uint8_t pixel_format;
	uint16_t width;
	uint16_t height;
	uint8_t channel;
	bool is_landscape;
};

const struct device *dsi;

static int32_t DSI_IO_Write(uint16_t ChannelNbr, uint16_t Reg, uint8_t *pData, uint16_t Size)
{
	int ret;

	ret = mipi_dsi_dcs_write(dsi, ChannelNbr, Reg, pData, Size);

	return ret == Size ? OTM8009A_OK : OTM8009A_ERROR;
}

static int32_t DSI_IO_Read(uint16_t ChannelNbr, uint16_t Reg, uint8_t *pData, uint16_t Size)
{
	int ret;

	ret = mipi_dsi_dcs_read(dsi, ChannelNbr, Reg, pData, Size);

	return ret == Size ? OTM8009A_OK : OTM8009A_ERROR;
}

static int32_t _OTM8009A_Probe(uint32_t ColorCoding, uint32_t Orientation)
{
	int32_t ret;
	uint32_t id;
	OTM8009A_IO_t IOCtx;
	static OTM8009A_Object_t OTM8009AObj;

	IOCtx.Address = 0;
	IOCtx.GetTick = (OTM8009A_GetTick_Func)k_uptime_get_32;
	IOCtx.WriteReg = DSI_IO_Write;
	IOCtx.ReadReg = DSI_IO_Read;

	if (OTM8009A_RegisterBusIO(&OTM8009AObj, &IOCtx)) {
		ret = HAL_ERROR;
	} else {
		if (OTM8009A_ReadID(&OTM8009AObj, &id)) {
			ret = HAL_ERROR;
		} else {
			LOG_DBG("ID 0x%x (should 0x%x)", id, OTM8009A_ID);
			if (OTM8009A_LCD_Driver.Init(&OTM8009AObj, ColorCoding, Orientation)) {
				ret = HAL_ERROR;
			} else {
				ret = HAL_OK;
			}
		}
	}

	return ret;
}

static int otm8009a_probe(const struct device *dev)
{
	const struct otm8009a_config *config = dev->config;
	struct mipi_dsi_device mdev;
	int ret;

	dsi = config->mipi_dsi;
	mdev.data_lanes = config->num_of_lanes;
	mdev.pixfmt = config->pixel_format;

	if (config->reset.port) {
		/* NOTE: reset display, mandatory for mipi config */
		ret = gpio_pin_configure_dt(&config->reset, GPIO_OUTPUT_INACTIVE);
		if (ret < 0) {
			LOG_ERR("Configuration of display on/off control GPIO failed");
			return ret;
		}
		k_msleep(20); /* display dependent */
		ret = gpio_pin_configure_dt(&config->reset, GPIO_OUTPUT_ACTIVE);
		if (ret < 0) {
			LOG_ERR("Configuration of display on/off control GPIO failed");
			return ret;
		}
		k_msleep(10); /* display dependent */
	}

	if (config->backlight.port != NULL) {
		ret = gpio_pin_configure_dt(&config->backlight, GPIO_OUTPUT_ACTIVE);
		if (ret < 0) {
			LOG_ERR("Could not configure bl GPIO (%d)", ret);
			return ret;
		}
	}

	ret = mipi_dsi_attach(config->mipi_dsi, config->channel, &mdev);
	if (ret < 0) {
		LOG_ERR("Could not attach to MIPI-DSI host");
		return ret;
	}

	ret = _OTM8009A_Probe(config->pixel_format, config->is_landscape);
	if (ret != HAL_OK) {
		return ret;
	}

	return 0;
}

#define OTM8009A_DEVICE(id)							\
	static const struct otm8009a_config otm8009a_config_##id = {		\
		.mipi_dsi = DEVICE_DT_GET(DT_INST_BUS(id)),			\
		.reset = GPIO_DT_SPEC_INST_GET_OR(id, reset_gpios, {0}),	\
		.backlight = GPIO_DT_SPEC_INST_GET_OR(id, bl_gpios, {0}),	\
		.num_of_lanes = DT_INST_PROP_BY_IDX(id, data_lanes, 0),		\
		.width = DT_INST_PROP(id, width),				\
		.height = DT_INST_PROP(id, height),				\
		.channel = DT_INST_REG_ADDR(id),				\
		.is_landscape = DT_INST_PROP(id, is_landscape),			\
	};									\
	DEVICE_DT_INST_DEFINE(id, &otm8009a_probe, NULL, NULL,			\
			      &otm8009a_config_##id, POST_KERNEL,		\
			      CONFIG_APPLICATION_INIT_PRIORITY,			\
			      NULL);

DT_INST_FOREACH_STATUS_OKAY(OTM8009A_DEVICE)

An option could be that i rewrite the otm8009a driver, but I am not familiar with the licensing if there would be an issue.
But this will take some time.
Is it more clear now?

[erwango]

Much clearer, thanks.
This code has been contributed under BSD-3 Clause, so you are authorized to take it, modify it and redistribute it (eg change license to Apache 2.0), as long as:
A] You retain ST copyright
B] You retain BSD-3 clauses. So in the driver, you should mention that the file was taken from [ST repo] under BSD-3 clause.

Though, I'm not quite sure if B] is ok in Zephyr. @carlescufi maybe you can confirm ?

[dariis]

@carlescufi
would this be accepted:

/*
 * Copyright (c) 2022 bytes at work AG
 * based on STM32Cube BSP OTM8009A driver under BSD 3-Clause
 *
 * SPDX-License-Identifier: Apache-2.0
 *
 ******************************************************************************
 * Copyright (c) 2015 STMicroelectronics.
 * All rights reserved.
 *
 * This software component is licensed by ST under BSD 3-Clause license,
 * the "License"; You may not use this file except in compliance with the
 * License. You may obtain a copy of the License at:
 *                        opensource.org/licenses/BSD-3-Clause
 *
 ******************************************************************************
 */

[erwango]

My understanding is that it would be ok if:

• you first merge the file as is
• add a commit to switch the licence (while keeping the copyright and a link to the initial file)

[carlescufi]

In order to be able to do something like this, because you are effectively importing 3rd-party code to Zephyr, you need to follow the procedure outlined in this page:
https://docs.zephyrproject.org/latest/contribute/external.html

[dariis]

Try to get it upstream gave me more insight and solved my problem.
Thanks to all of you, specially @erwango @carlescufi @gmarull

Hint: In my case LVGL does not work with RGB888 only with RGB565 or ARGB8888!