[PATCH v3 00/13] Add imx8mp video support
In order to display a boot picture or an error message, the i.MX8MP display pipeline must be enabled. The SoC has support for various interfaces (LVDS, HDMI, DSI). The one supported in this series is the standard 4-lane LVDS output. The minimal setup is thus composed of: * An LCD InterFace (LCDIF) with an AXI/APB interface, generating a pixel stream * One LVDS Display Bridge (LDB), also named pixel mapper, which receives the pixel stream and route it to one or two (possibly combined) LVDS displays. * All necessary clocks and power controls coming from the MEDIAMIX control block.
Patch 1 adds a very useful helper to the core in order to grab devices through endpoints instead of being limited to phandles. Video pipelines being often described using graphs endpoints, the introduced helper is used several times in the serires (there are 3 LCDIF, one of them being connected to the LDB, itself having 2 ports).
Patch 2 is a fix which is necessary for this series to work properly, but is way broader than just this use case. In practice, when assigned clocks are defined in the device tree, the clock uclass tries to assign the parents first and then sets them to the correct frequency. This only works if the parents have been enabled themselves. Otherwise we end-up with a non-clocked parent. I believe this is not the intended behavior in general, but more importantly on the i.MX8MP, there are "clock slices" which have pre-requisites in order to be modified and selecting an ungated parent is one of them.
All the other patches progressively build support for the whole video pipeline. Regarding the LCDIF driver, there is already a similar driver for older i.MX SoCs but I didn't manage to get it to work. It was written more than a decade ago while device-model, clocks and others were not yet generically supported. Thus, numerous ad-hoc solutions were implemented, which no longer fit today's requirements. I preferred to add a new "clean" driver instead of modifying the existing one because of the too high risk of breaking these platforms. Once proper clocks/power-domain descriptions will be added to them they might be converted (and tested) to work with the "new" implementation, but going the opposite way felt drawback.
--- Changes in v3: - Fix a clock name in the imx8mp clock driver (mismatch between Linux and U-Boot). - Fixed the Azure tests with extra changes in the test files and sandbox configurations: https://dev.azure.com/u-boot/u-boot/_build/results?buildId=10296&view=re... - Link to v2: https://lore.kernel.org/r/20241205-ge-mainline-display-support-v2-0-4683bb43...
Changes in v2: - Add ISP clocks to avoid harmless errors when shutting down the pipeline. - Add power domain refcounting to avoid shutting down a power domain already disabled. This is useful when a device points several times to the same power domain in the DT. - Ensure the mediamix driver frees all the power domains it got a reference on. - Add an in-tree user as requested by Fabio: the EVK. - Use the same Kconfig prompts as in Linux. - Add sandbox tests for uclass_get_device_by_endpoint(). - Enclosed media clocks definitions within #if CONFIG_IS_ENABLED() guards to avoid bloating the SPL and configurations without video support, as Adam and Fabio advised. - Disable the enabled clocks in the remove path of the mediamix block driver. - Fix many typos. - Fix checkpatch warnings. - Rebase on next. - Collect tags - Link to v1: https://lore.kernel.org/r/20240910101344.110633-1-miquel.raynal@bootlin.com
--- Miquel Raynal (13): dm: doc: Fix example dm: core: Add a helper to retrieve devices through graph endpoints sandbox: Add a fake DSI controller and link it to the panel test: dm: test-fdt: Add checks for uclass_get_device_by_endpoint() power-domain: Add refcounting clk: Ensure the parent clocks are enabled while reparenting clk: imx8mp: Add media related clocks imx: power-domain: Describe the i.MX8 MEDIAMIX domain imx: power-domain: Add support for the MEDIAMIX control block video: imx: Fix Makefile in order to be able to add other imx drivers video: imx: Add LDB driver video: imx: Add LCDIF driver imx8mp_evk: Enable display support
arch/sandbox/dts/test.dts | 23 ++ configs/imx8mp_evk_defconfig | 7 + configs/sandbox64_defconfig | 1 + configs/sandbox_flattree_defconfig | 1 + doc/develop/driver-model/design.rst | 2 +- drivers/clk/clk-uclass.c | 21 +- drivers/clk/imx/clk-imx8mp.c | 69 +++++ drivers/core/uclass.c | 62 +++++ drivers/firmware/scmi/sandbox-scmi_devices.c | 1 + drivers/power/domain/Kconfig | 7 + drivers/power/domain/Makefile | 1 + drivers/power/domain/imx8m-power-domain.c | 17 ++ drivers/power/domain/imx8mp-mediamix.c | 208 +++++++++++++++ drivers/power/domain/power-domain-uclass.c | 37 ++- drivers/power/domain/sandbox-power-domain-test.c | 1 + drivers/video/Makefile | 2 +- drivers/video/imx/Kconfig | 9 + drivers/video/imx/Makefile | 4 +- drivers/video/imx/lcdif.c | 314 +++++++++++++++++++++++ drivers/video/imx/ldb.c | 251 ++++++++++++++++++ include/dm/uclass.h | 21 ++ include/power-domain.h | 4 +- test/dm/power-domain.c | 2 +- test/dm/test-fdt.c | 20 +- 24 files changed, 1072 insertions(+), 13 deletions(-) --- base-commit: c498b6cace9e0de45ea12146312f5a6f43a03a9b change-id: 20241205-ge-mainline-display-support-00cf60fbd2cc
Best regards,
`.priv_data_size` does not exist. I believe the actual structure member was supposed to be `.priv_auto`.
Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- doc/develop/driver-model/design.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/doc/develop/driver-model/design.rst b/doc/develop/driver-model/design.rst index 8c2c81d7ac9a1f59af67db012c78ed5be276805b..ce914dae342388d770a17e74c72e423ba5c34204 100644 --- a/doc/develop/driver-model/design.rst +++ b/doc/develop/driver-model/design.rst @@ -312,7 +312,7 @@ drivers/demo/demo-shape.c): .name = "demo_shape_drv", .id = UCLASS_DEMO, .ops = &shape_ops, - .priv_data_size = sizeof(struct shape_data), + .priv_auto = sizeof(struct shape_data), };
There are already several helpers to find a udevice based on its position in a device tree, like getting a child or a node pointed by a phandle, but there was no support for graph endpoints, which are very common in display pipelines.
Add a new helper, named uclass_get_device_by_endpoint() which enters the child graph reprensentation, looks for a specific port, then follows the remote endpoint, and finally retrieves the first parent of the given uclass_id.
This is a very handy and straightforward way to get a bridge or a panel handle.
Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/core/uclass.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/dm/uclass.h | 21 +++++++++++++++++ 2 files changed, 83 insertions(+)
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index f846a35d6b2537bab56bca158cedfb134b37f477..c9c5debbe7cd9ec44f95dc80d3199ee11521e997 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -582,6 +582,68 @@ int uclass_get_device_by_phandle(enum uclass_id id, struct udevice *parent, ret = uclass_find_device_by_phandle(id, parent, name, &dev); return uclass_get_device_tail(dev, ret, devp); } + +int uclass_get_device_by_endpoint(enum uclass_id class_id, struct udevice *dev, + u32 ep_idx, struct udevice **devp) +{ + ofnode port = ofnode_null(), ep, remote_ep; + struct udevice *target = NULL; + u32 remote_phandle; + int ret; + + if (!ep_idx) + port = dev_read_subnode(dev, "port"); + + if (!ofnode_valid(port)) { + char port_name[32]; + + port = dev_read_subnode(dev, "ports"); + if (!ofnode_valid(port)) { + debug("%s: no 'port'/'ports' subnode\n", + dev_read_name(dev)); + return -EINVAL; + } + + snprintf(port_name, sizeof(port_name), "port@%x", ep_idx); + port = ofnode_find_subnode(port, port_name); + if (!ofnode_valid(port)) { + debug("%s: no 'port@%x' subnode\n", + dev_read_name(dev), ep_idx); + return -EINVAL; + } + } + + ep = ofnode_find_subnode(port, "endpoint"); + if (!ofnode_valid(ep)) { + debug("%s: no 'endpoint' in %s subnode\n", + ofnode_get_name(port), dev_read_name(dev)); + return -EINVAL; + } + + ret = ofnode_read_u32(ep, "remote-endpoint", &remote_phandle); + if (ret) + return ret; + + remote_ep = ofnode_get_by_phandle(remote_phandle); + if (!ofnode_valid(remote_ep)) + return -EINVAL; + + while (ofnode_valid(remote_ep)) { + remote_ep = ofnode_get_parent(remote_ep); + debug("trying subnode: %s\n", ofnode_get_name(remote_ep)); + if (!ofnode_valid(remote_ep)) { + debug("%s: no more remote devices\n", + ofnode_get_name(remote_ep)); + return -ENODEV; + } + + ret = uclass_find_device_by_ofnode(class_id, remote_ep, &target); + if (!ret) + break; + }; + + return uclass_get_device_tail(target, ret, devp); +} #endif
/* diff --git a/include/dm/uclass.h b/include/dm/uclass.h index c279304092352200d5297ad2d17f527173ec506b..6b76a9a588e70f487c2ce5398526d98b4547e2a7 100644 --- a/include/dm/uclass.h +++ b/include/dm/uclass.h @@ -333,6 +333,27 @@ int uclass_get_device_by_phandle(enum uclass_id id, struct udevice *parent, int uclass_get_device_by_driver(enum uclass_id id, const struct driver *drv, struct udevice **devp);
+/** + * uclass_get_device_by_endpoint() - Get a uclass device for a remote endpoint + * + * This searches through the parents of the specified remote endpoint + * for the first device matching the uclass. Said otherwise, this helper + * goes through the graph (endpoint) representation and searches for + * matching devices. Endpoints can be subnodes of the "port" node or + * subnodes of ports identified with a reg property, themselves in a + * "ports" container. + * + * The device is probed to activate it ready for use. + * + * @class_id: uclass ID to look up + * @dev: Device to start from + * @ep_idx: Index of the endpoint to follow, 0 if there is none. + * @target: Returns pointer to the first device matching the expected uclass. + * Return: 0 if OK, -ve on error + */ +int uclass_get_device_by_endpoint(enum uclass_id class_id, struct udevice *dev, + u32 ep_idx, struct udevice **target); + /** * uclass_first_device() - Get the first device in a uclass *
Use a graph endpoint representation for that in order to test a new core DM helper going through these properties.
Display pipelines typically include graph endpoints, but the helper is generic and would work with any device type.
Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- arch/sandbox/dts/test.dts | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 36cfbf213e4cfe17d9ca98d6e40f7caf95631254..f7d4d9824454207f2d4650739e43a36e4936ef9e 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -295,6 +295,23 @@
dsi_host: dsi_host { compatible = "sandbox,dsi-host"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + unused_ep: endpoint { + }; + }; + + port@1 { + reg = <1>; + dispc_out1: endpoint { + remote-endpoint = <&panel_to_dispc>; + }; + }; + }; };
a-test { @@ -1359,6 +1376,12 @@ panel { compatible = "simple-panel"; backlight = <&backlight 0 100>; + + port { + panel_to_dispc: endpoint { + remote-endpoint = <&dispc_out1>; + }; + }; };
scsi {
This is a new DM core helper. There is now a graph endpoint representation in the sandbox test DTS, so we can just use it to verify the helper proper behavior.
Suggested-by: Simon Glass sjg@chromium.org Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- configs/sandbox64_defconfig | 1 + configs/sandbox_flattree_defconfig | 1 + test/dm/test-fdt.c | 20 +++++++++++++++++++- 3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig index 7960b2ef42e7ceee2a3646888ae159cdde9b59e0..359c12b88c904c995ca63bf3c17324c8eb02257a 100644 --- a/configs/sandbox64_defconfig +++ b/configs/sandbox64_defconfig @@ -256,6 +256,7 @@ CONFIG_CONSOLE_TRUETYPE=y CONFIG_CONSOLE_TRUETYPE_CANTORAONE=y CONFIG_I2C_EDID=y CONFIG_VIDEO_SANDBOX_SDL=y +CONFIG_VIDEO_DSI_HOST_SANDBOX=y CONFIG_OSD=y CONFIG_SANDBOX_OSD=y CONFIG_BMP_16BPP=y diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig index 563093dd8a426f98cdcfa342655bebb82dbefe86..f08dfef9056c4a2c86650f43298b3f506fae8d4c 100644 --- a/configs/sandbox_flattree_defconfig +++ b/configs/sandbox_flattree_defconfig @@ -220,6 +220,7 @@ CONFIG_CONSOLE_TRUETYPE=y CONFIG_CONSOLE_TRUETYPE_CANTORAONE=y CONFIG_I2C_EDID=y CONFIG_VIDEO_SANDBOX_SDL=y +CONFIG_VIDEO_DSI_HOST_SANDBOX=y CONFIG_OSD=y CONFIG_SANDBOX_OSD=y CONFIG_BMP_16BPP=y diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c index af8cd617108ac2421f7275948be000a7cbaebda0..e3c8c07066a00d55b2319e8f028b8ce9906efe28 100644 --- a/test/dm/test-fdt.c +++ b/test/dm/test-fdt.c @@ -792,7 +792,7 @@ DM_TEST(dm_test_fdt_disable_enable_by_path, UTF_SCAN_PDATA | UTF_SCAN_FDT); /* Test a few uclass phandle functions */ static int dm_test_fdt_phandle(struct unit_test_state *uts) { - struct udevice *back, *dev, *dev2; + struct udevice *back, *dispc, *panel, *dev, *dev2;
ut_assertok(uclass_find_first_device(UCLASS_PANEL_BACKLIGHT, &back)); ut_assertnonnull(back); @@ -807,6 +807,24 @@ static int dm_test_fdt_phandle(struct unit_test_state *uts) "power-supply", &dev2)); ut_asserteq_ptr(dev, dev2);
+ /* Test uclass_get_device_by_endpoint() */ + ut_assertok(uclass_find_device_by_name(UCLASS_DSI_HOST, "dsi_host", &dispc)); + ut_assertnonnull(dispc); + ut_asserteq(0, device_active(dispc)); + ut_assertok(uclass_find_device_by_name(UCLASS_PANEL, "panel", &panel)); + ut_assertnonnull(panel); + ut_asserteq(0, device_active(panel)); + + ut_asserteq(-ENODEV, uclass_get_device_by_endpoint(UCLASS_PANEL_BACKLIGHT, dispc, 1, &dev)); + ut_asserteq(-EINVAL, uclass_get_device_by_endpoint(UCLASS_PANEL, dispc, 0, &dev)); + ut_assertok(uclass_get_device_by_endpoint(UCLASS_PANEL, dispc, 1, &dev)); + ut_asserteq_ptr(panel, dev); + + ut_asserteq(-ENODEV, uclass_get_device_by_endpoint(UCLASS_PANEL_BACKLIGHT, panel, 0, &dev2)); + ut_asserteq(-EINVAL, uclass_get_device_by_endpoint(UCLASS_DSI_HOST, panel, 1, &dev2)); + ut_assertok(uclass_get_device_by_endpoint(UCLASS_DSI_HOST, panel, 0, &dev2)); + ut_asserteq_ptr(dispc, dev2); + return 0; } DM_TEST(dm_test_fdt_phandle, UTF_SCAN_PDATA | UTF_SCAN_FDT);
It is very surprising that such an uclass, specifically designed to handle resources that may be shared by different devices, is not keeping the count of the number of times a power domain has been enabled/disabled to avoid shutting it down unexpectedly or disabling it several times.
Doing this causes troubles on eg. i.MX8MP because disabling power domains can be done in recursive loops were the same power domain disabled up to 4 times in a row. PGCs seem to have tight FSM internal timings to respect and it is easy to produce a race condition that puts the power domains in an unstable state, leading to ADB400 errors and later crashes in Linux.
CI tests using power domains are slightly updated to make sure the count of on/off calls is even and the results match what we *now* expect.
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/firmware/scmi/sandbox-scmi_devices.c | 1 + drivers/power/domain/power-domain-uclass.c | 37 ++++++++++++++++++++++-- drivers/power/domain/sandbox-power-domain-test.c | 1 + include/power-domain.h | 4 +-- test/dm/power-domain.c | 2 +- 5 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c index 96c2922b067e2886b3fa963bcd7e396f4569a569..9f253b0fd40f703a5ec11d34c197423d27ad8b01 100644 --- a/drivers/firmware/scmi/sandbox-scmi_devices.c +++ b/drivers/firmware/scmi/sandbox-scmi_devices.c @@ -163,4 +163,5 @@ U_BOOT_DRIVER(sandbox_scmi_devices) = { .priv_auto = sizeof(struct sandbox_scmi_device_priv), .remove = sandbox_scmi_devices_remove, .probe = sandbox_scmi_devices_probe, + .flags = DM_FLAG_DEFAULT_PD_CTRL_OFF, }; diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c index 938bd8cbc9ffd1ba2109d702f886b6a99288d063..55a3d95d83c5aaac31acbd877199c87f8f6fb880 100644 --- a/drivers/power/domain/power-domain-uclass.c +++ b/drivers/power/domain/power-domain-uclass.c @@ -12,6 +12,10 @@ #include <power-domain-uclass.h> #include <dm/device-internal.h>
+struct power_domain_priv { + int on_count; +}; + static inline struct power_domain_ops *power_domain_dev_ops(struct udevice *dev) { return (struct power_domain_ops *)dev->driver->ops; @@ -110,19 +114,47 @@ int power_domain_free(struct power_domain *power_domain) int power_domain_on(struct power_domain *power_domain) { struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev); + struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev); + int ret;
debug("%s(power_domain=%p)\n", __func__, power_domain);
- return ops->on ? ops->on(power_domain) : 0; + if (priv->on_count++ > 0) + return 0; + + ret = ops->on ? ops->on(power_domain) : 0; + if (ret) { + priv->on_count--; + return ret; + } + + return 0; }
int power_domain_off(struct power_domain *power_domain) { struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev); + struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev); + int ret;
debug("%s(power_domain=%p)\n", __func__, power_domain);
- return ops->off ? ops->off(power_domain) : 0; + if (priv->on_count <= 0) { + debug("Power domain %s already off.\n", power_domain->dev->name); + priv->on_count--; + return 0; + } + + if (priv->on_count-- > 1) + return 0; + + ret = ops->off ? ops->off(power_domain) : 0; + if (ret) { + priv->on_count++; + return ret; + } + + return 0; }
#if CONFIG_IS_ENABLED(OF_REAL) @@ -180,4 +212,5 @@ int dev_power_domain_off(struct udevice *dev) UCLASS_DRIVER(power_domain) = { .id = UCLASS_POWER_DOMAIN, .name = "power_domain", + .per_device_auto = sizeof(struct power_domain_priv), }; diff --git a/drivers/power/domain/sandbox-power-domain-test.c b/drivers/power/domain/sandbox-power-domain-test.c index 08c15ef342b3dd3ce01807ee59b7e97337f7dde5..5b530974e942ffcba453e53be330baaf3a113a13 100644 --- a/drivers/power/domain/sandbox-power-domain-test.c +++ b/drivers/power/domain/sandbox-power-domain-test.c @@ -51,4 +51,5 @@ U_BOOT_DRIVER(sandbox_power_domain_test) = { .id = UCLASS_MISC, .of_match = sandbox_power_domain_test_ids, .priv_auto = sizeof(struct sandbox_power_domain_test), + .flags = DM_FLAG_DEFAULT_PD_CTRL_OFF, }; diff --git a/include/power-domain.h b/include/power-domain.h index 18525073e5e3534fcbac6fae4e18462f29a4dc49..0266fc2f864ef3645d2a8b3c6fa66fdbd868799c 100644 --- a/include/power-domain.h +++ b/include/power-domain.h @@ -147,7 +147,7 @@ static inline int power_domain_free(struct power_domain *power_domain) #endif
/** - * power_domain_on - Enable power to a power domain. + * power_domain_on - Enable power to a power domain (with refcounting) * * @power_domain: A power domain struct that was previously successfully * requested by power_domain_get(). @@ -163,7 +163,7 @@ static inline int power_domain_on(struct power_domain *power_domain) #endif
/** - * power_domain_off - Disable power to a power domain. + * power_domain_off - Disable power to a power domain (with refcounting) * * @power_domain: A power domain struct that was previously successfully * requested by power_domain_get(). diff --git a/test/dm/power-domain.c b/test/dm/power-domain.c index 896cf5b2ae9d26701150fad70e888f8b135a22b0..8a95f6bdb903be9d1993528d87d5cae0075a83e4 100644 --- a/test/dm/power-domain.c +++ b/test/dm/power-domain.c @@ -27,7 +27,7 @@ static int dm_test_power_domain(struct unit_test_state *uts)
ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "power-domain-test", &dev_test)); - ut_asserteq(1, sandbox_power_domain_query(dev_power_domain, + ut_asserteq(0, sandbox_power_domain_query(dev_power_domain, TEST_POWER_DOMAIN)); ut_assertok(sandbox_power_domain_test_get(dev_test));
Hi Miquel,
On Fri, 10 Jan 2025 at 11:43, Miquel Raynal miquel.raynal@bootlin.com wrote:
It is very surprising that such an uclass, specifically designed to handle resources that may be shared by different devices, is not keeping the count of the number of times a power domain has been enabled/disabled to avoid shutting it down unexpectedly or disabling it several times.
Doing this causes troubles on eg. i.MX8MP because disabling power domains can be done in recursive loops were the same power domain disabled up to 4 times in a row. PGCs seem to have tight FSM internal timings to respect and it is easy to produce a race condition that puts the power domains in an unstable state, leading to ADB400 errors and later crashes in Linux.
CI tests using power domains are slightly updated to make sure the count of on/off calls is even and the results match what we *now* expect.
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com
drivers/firmware/scmi/sandbox-scmi_devices.c | 1 + drivers/power/domain/power-domain-uclass.c | 37 ++++++++++++++++++++++-- drivers/power/domain/sandbox-power-domain-test.c | 1 + include/power-domain.h | 4 +-- test/dm/power-domain.c | 2 +- 5 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c b/drivers/firmware/scmi/sandbox-scmi_devices.c index 96c2922b067e2886b3fa963bcd7e396f4569a569..9f253b0fd40f703a5ec11d34c197423d27ad8b01 100644 --- a/drivers/firmware/scmi/sandbox-scmi_devices.c +++ b/drivers/firmware/scmi/sandbox-scmi_devices.c @@ -163,4 +163,5 @@ U_BOOT_DRIVER(sandbox_scmi_devices) = { .priv_auto = sizeof(struct sandbox_scmi_device_priv), .remove = sandbox_scmi_devices_remove, .probe = sandbox_scmi_devices_probe,
.flags = DM_FLAG_DEFAULT_PD_CTRL_OFF,}; diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c index 938bd8cbc9ffd1ba2109d702f886b6a99288d063..55a3d95d83c5aaac31acbd877199c87f8f6fb880 100644 --- a/drivers/power/domain/power-domain-uclass.c +++ b/drivers/power/domain/power-domain-uclass.c @@ -12,6 +12,10 @@ #include <power-domain-uclass.h> #include <dm/device-internal.h>
+struct power_domain_priv {
int on_count;+};
static inline struct power_domain_ops *power_domain_dev_ops(struct udevice *dev) { return (struct power_domain_ops *)dev->driver->ops; @@ -110,19 +114,47 @@ int power_domain_free(struct power_domain *power_domain) int power_domain_on(struct power_domain *power_domain) { struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);int ret; debug("%s(power_domain=%p)\n", __func__, power_domain);
return ops->on ? ops->on(power_domain) : 0;
if (priv->on_count++ > 0)return 0;
-EALREADY
ret = ops->on ? ops->on(power_domain) : 0;if (ret) {priv->on_count--;return ret;}return 0;}
int power_domain_off(struct power_domain *power_domain) { struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);int ret; debug("%s(power_domain=%p)\n", __func__, power_domain);
return ops->off ? ops->off(power_domain) : 0;
if (priv->on_count <= 0) {debug("Power domain %s already off.\n", power_domain->dev->name);priv->on_count--;return 0;}if (priv->on_count-- > 1)return 0;
-EBUSY
See how the regulator uclass does it.
ret = ops->off ? ops->off(power_domain) : 0;if (ret) {priv->on_count++;return ret;}return 0;}
#if CONFIG_IS_ENABLED(OF_REAL) @@ -180,4 +212,5 @@ int dev_power_domain_off(struct udevice *dev) UCLASS_DRIVER(power_domain) = { .id = UCLASS_POWER_DOMAIN, .name = "power_domain",
.per_device_auto = sizeof(struct power_domain_priv),}; diff --git a/drivers/power/domain/sandbox-power-domain-test.c b/drivers/power/domain/sandbox-power-domain-test.c index 08c15ef342b3dd3ce01807ee59b7e97337f7dde5..5b530974e942ffcba453e53be330baaf3a113a13 100644 --- a/drivers/power/domain/sandbox-power-domain-test.c +++ b/drivers/power/domain/sandbox-power-domain-test.c @@ -51,4 +51,5 @@ U_BOOT_DRIVER(sandbox_power_domain_test) = { .id = UCLASS_MISC, .of_match = sandbox_power_domain_test_ids, .priv_auto = sizeof(struct sandbox_power_domain_test),
.flags = DM_FLAG_DEFAULT_PD_CTRL_OFF,}; diff --git a/include/power-domain.h b/include/power-domain.h index 18525073e5e3534fcbac6fae4e18462f29a4dc49..0266fc2f864ef3645d2a8b3c6fa66fdbd868799c 100644 --- a/include/power-domain.h +++ b/include/power-domain.h @@ -147,7 +147,7 @@ static inline int power_domain_free(struct power_domain *power_domain) #endif
/**
- power_domain_on - Enable power to a power domain.
- power_domain_on - Enable power to a power domain (with refcounting)
- @power_domain: A power domain struct that was previously successfully
requested by power_domain_get().@@ -163,7 +163,7 @@ static inline int power_domain_on(struct power_domain *power_domain) #endif
/**
- power_domain_off - Disable power to a power domain.
- power_domain_off - Disable power to a power domain (with refcounting)
- @power_domain: A power domain struct that was previously successfully
requested by power_domain_get().diff --git a/test/dm/power-domain.c b/test/dm/power-domain.c index 896cf5b2ae9d26701150fad70e888f8b135a22b0..8a95f6bdb903be9d1993528d87d5cae0075a83e4 100644 --- a/test/dm/power-domain.c +++ b/test/dm/power-domain.c @@ -27,7 +27,7 @@ static int dm_test_power_domain(struct unit_test_state *uts)
ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "power-domain-test", &dev_test));
ut_asserteq(1, sandbox_power_domain_query(dev_power_domain,
ut_asserteq(0, sandbox_power_domain_query(dev_power_domain, TEST_POWER_DOMAIN)); ut_assertok(sandbox_power_domain_test_get(dev_test));-- 2.47.0
Regards, SImon
Hello Simon,
int power_domain_on(struct power_domain *power_domain) {
...
if (priv->on_count++ > 0)return 0;-EALREADY
...
int power_domain_off(struct power_domain *power_domain) {
...
if (priv->on_count-- > 1)return 0;-EBUSY
See how the regulator uclass does it.
I really does not understand why we would like to do that.
It is perfectly normal operation to call power_domain_on/off() on the same power domain several times in a row and there is no reason to return an error code. It is quite the opposite, the main reason for power domains is to act like shared regulators. Se while a regulator has one user and doing the same action on it several times does not make much sense and can be reported, that is not how power domains have been thought about in the first place.
Hence, I do not agree with returning error codes in these situations, they are misleading and they would have to be ignored anyway.
Thanks, Miquèl
Hi Miquel,
On Mon, 20 Jan 2025 at 03:34, Miquel Raynal miquel.raynal@bootlin.com wrote:
Hello Simon,
int power_domain_on(struct power_domain *power_domain) {
...
if (priv->on_count++ > 0)return 0;-EALREADY
...
int power_domain_off(struct power_domain *power_domain) {
...
if (priv->on_count-- > 1)return 0;-EBUSY
See how the regulator uclass does it.
I really does not understand why we would like to do that.
It is perfectly normal operation to call power_domain_on/off() on the same power domain several times in a row and there is no reason to return an error code. It is quite the opposite, the main reason for power domains is to act like shared regulators. Se while a regulator has one user and doing the same action on it several times does not make much sense and can be reported, that is not how power domains have been thought about in the first place.
I am not aware of any difference between these two subsystems. If we want a power domain to actually turn off, how many times do we need to call power_domain_off()? The function silently does nothing in many cases, so it is not deterministic. In the case where we *actually* want to turn the power domain off, we are at a loss as to what code to write.
Hence, I do not agree with returning error codes in these situations, they are misleading and they would have to be ignored anyway.
How about creating a power_domain_off_if_allowed() or power_domain_soft_off/on() or power_domain_req_off (for request)?
Regards, SImon
Hi Simon,
On 20/01/2025 at 12:21:04 -07, Simon Glass sjg@chromium.org wrote:
Hi Miquel,
On Mon, 20 Jan 2025 at 03:34, Miquel Raynal miquel.raynal@bootlin.com wrote:
Hello Simon,
int power_domain_on(struct power_domain *power_domain) {
...
if (priv->on_count++ > 0)return 0;-EALREADY
...
int power_domain_off(struct power_domain *power_domain) {
...
if (priv->on_count-- > 1)return 0;-EBUSY
See how the regulator uclass does it.
I really does not understand why we would like to do that.
It is perfectly normal operation to call power_domain_on/off() on the same power domain several times in a row and there is no reason to return an error code. It is quite the opposite, the main reason for power domains is to act like shared regulators. Se while a regulator has one user and doing the same action on it several times does not make much sense and can be reported, that is not how power domains have been thought about in the first place.
I am not aware of any difference between these two subsystems.
Actually you're right on this point, the regulator API works like the pmdomain one. I had a look: it always returns 0, no matter the state of the regulator after the operation (as long as there is no error of course).
If we want a power domain to actually turn off, how many times do we need to call power_domain_off()?
We do not? It is why I add refcounting, if a power domain has 2 consumers, each consumer says whether it needs the power domain to be on or off and the core will handle, but in no case they should force it's state.
The function silently does nothing in many cases, so it is not deterministic.
It is fully deterministic, as long as consumers call power_on/off evenly (and this is already what we request in U-Boot).
In the case where we *actually* want to turn the power domain off, we are at a loss as to what code to write.
Hence, I do not agree with returning error codes in these situations, they are misleading and they would have to be ignored anyway.
How about creating a power_domain_off_if_allowed() or power_domain_soft_off/on() or power_domain_req_off (for request)?
The power domain logic has a hardware reality in many SoCs but is also a software concept that is shared with Linux. Changing the semantics in U-Boot would be very misleading and if my understanding is correct, this approach would be new.
If you really want a way to track the state of the power domain, however, I can maybe add a helper returning its state, ie:
bool power_domain_is_on(domain)
Would that work?
Thanks, Miquèl
Hi Miquel,
On Tue, 21 Jan 2025 at 01:34, Miquel Raynal miquel.raynal@bootlin.com wrote:
Hi Simon,
On 20/01/2025 at 12:21:04 -07, Simon Glass sjg@chromium.org wrote:
Hi Miquel,
On Mon, 20 Jan 2025 at 03:34, Miquel Raynal miquel.raynal@bootlin.com wrote:
Hello Simon,
int power_domain_on(struct power_domain *power_domain) {
...
if (priv->on_count++ > 0)return 0;-EALREADY
...
int power_domain_off(struct power_domain *power_domain) {
...
if (priv->on_count-- > 1)return 0;-EBUSY
See how the regulator uclass does it.
I really does not understand why we would like to do that.
It is perfectly normal operation to call power_domain_on/off() on the same power domain several times in a row and there is no reason to return an error code. It is quite the opposite, the main reason for power domains is to act like shared regulators. Se while a regulator has one user and doing the same action on it several times does not make much sense and can be reported, that is not how power domains have been thought about in the first place.
I am not aware of any difference between these two subsystems.
Actually you're right on this point, the regulator API works like the pmdomain one. I had a look: it always returns 0, no matter the state of the regulator after the operation (as long as there is no error of course).
You can see dm_test_power_regulator_set_enable_if_allowed() for a test that relates to this.
If we want a power domain to actually turn off, how many times do we need to call power_domain_off()?
We do not? It is why I add refcounting, if a power domain has 2 consumers, each consumer says whether it needs the power domain to be on or off and the core will handle, but in no case they should force it's state.
This is a bootloader, so we do sometimes need to force something off, or on.
The function silently does nothing in many cases, so it is not deterministic.
It is fully deterministic, as long as consumers call power_on/off evenly (and this is already what we request in U-Boot).
But I foresee people setting up power domains in board code, not drivers. My concern is that this is hiding the real state.
In the case where we *actually* want to turn the power domain off, we are at a loss as to what code to write.
Hence, I do not agree with returning error codes in these situations, they are misleading and they would have to be ignored anyway.
How about creating a power_domain_off_if_allowed() or power_domain_soft_off/on() or power_domain_req_off (for request)?
The power domain logic has a hardware reality in many SoCs but is also a software concept that is shared with Linux. Changing the semantics in U-Boot would be very misleading and if my understanding is correct, this approach would be new.
Yes this is quite a strong argument. Can you point me to the equivalent API in Linux? I see pm_domain but not the same as U-Boot
If you really want a way to track the state of the power domain, however, I can maybe add a helper returning its state, ie:
bool power_domain_is_on(domain)Would that work?
Not really. We should have an API which returns -EALREADY or -EBUSY based on the ref counts.
But yes, we can change the naming, so that this 'deterministic' ones have a different name, with the current names used for your ref-counting one. Then the ref-counting one is a thin layer on top of the deterministic one.
Regards, Simon
Hi Simon,
If we want a power domain to actually turn off, how many times do we need to call power_domain_off()?
We do not? It is why I add refcounting, if a power domain has 2 consumers, each consumer says whether it needs the power domain to be on or off and the core will handle, but in no case they should force it's state.
This is a bootloader, so we do sometimes need to force something off, or on.
This, I do understand.
The function silently does nothing in many cases, so it is not deterministic.
It is fully deterministic, as long as consumers call power_on/off evenly (and this is already what we request in U-Boot).
But I foresee people setting up power domains in board code, not drivers. My concern is that this is hiding the real state.
Setting up power domains in board code is drawback. It is legacy behaviour, people should switch to the device model (ie. using a proper DT description) and stop messing around with board files. It's been like 5 years since U-Boot forced people to transition, I wouldn't feel bad if these boards were no longer behaving like expected (mind there are very little chances this will break anything, as the kernel is supposed to re-init these domains anyway).
In the case where we *actually* want to turn the power domain off, we are at a loss as to what code to write.
Hence, I do not agree with returning error codes in these situations, they are misleading and they would have to be ignored anyway.
How about creating a power_domain_off_if_allowed() or power_domain_soft_off/on() or power_domain_req_off (for request)?
The power domain logic has a hardware reality in many SoCs but is also a software concept that is shared with Linux. Changing the semantics in U-Boot would be very misleading and if my understanding is correct, this approach would be new.
Yes this is quite a strong argument. Can you point me to the equivalent API in Linux? I see pm_domain but not the same as U-Boot
Yes, pmdomain (former genpd if I get it correctly), how are they different? From my point of view it is the same. The same devices are supported. So why would we want the API to be different?
If you really want a way to track the state of the power domain, however, I can maybe add a helper returning its state, ie:
bool power_domain_is_on(domain)Would that work?
Not really. We should have an API which returns -EALREADY or -EBUSY based on the ref counts. But yes, we can change the naming, so that this 'deterministic' ones have a different name, with the current names used for your ref-counting one. Then the ref-counting one is a thin layer on top of the deterministic one.
Honestly, I am not convinced, but I will anyway assume we need a way to force a domain off. There is no need to force a power domain on however, as after power_domain_on(), the power domain cannot be off (except error condition of course).
So I'll add a helper to force power off. It will be called power_domain_off_force() and be preceded with a comment stating that this is not the standard approach, to guide people towards the refcounted helper instead.
Would this address your request?
Thanks, Miquèl
Hi Miquel,
On Fri, 24 Jan 2025 at 08:20, Miquel Raynal miquel.raynal@bootlin.com wrote:
Hi Simon,
If we want a power domain to actually turn off, how many times do we need to call power_domain_off()?
We do not? It is why I add refcounting, if a power domain has 2 consumers, each consumer says whether it needs the power domain to be on or off and the core will handle, but in no case they should force it's state.
This is a bootloader, so we do sometimes need to force something off, or on.
This, I do understand.
The function silently does nothing in many cases, so it is not deterministic.
It is fully deterministic, as long as consumers call power_on/off evenly (and this is already what we request in U-Boot).
But I foresee people setting up power domains in board code, not drivers. My concern is that this is hiding the real state.
Setting up power domains in board code is drawback. It is legacy behaviour, people should switch to the device model (ie. using a proper DT description) and stop messing around with board files. It's been like 5 years since U-Boot forced people to transition, I wouldn't feel bad if these boards were no longer behaving like expected (mind there are very little chances this will break anything, as the kernel is supposed to re-init these domains anyway).
While I agree with you, the message is not fully landing, certainly not in SPL where there are code-size constraints.
In the case where we *actually* want to turn the power domain off, we are at a loss as to what code to write.
Hence, I do not agree with returning error codes in these situations, they are misleading and they would have to be ignored anyway.
How about creating a power_domain_off_if_allowed() or power_domain_soft_off/on() or power_domain_req_off (for request)?
The power domain logic has a hardware reality in many SoCs but is also a software concept that is shared with Linux. Changing the semantics in U-Boot would be very misleading and if my understanding is correct, this approach would be new.
Yes this is quite a strong argument. Can you point me to the equivalent API in Linux? I see pm_domain but not the same as U-Boot
Yes, pmdomain (former genpd if I get it correctly), how are they different? From my point of view it is the same. The same devices are supported. So why would we want the API to be different?
But can you please point me to the API? I am seeing struct generic_pm_domain, which is uncommented, so it isn't clear what the power_off() and power_on() functions actually do. Perhaps they work differently from what you think?
If you really want a way to track the state of the power domain, however, I can maybe add a helper returning its state, ie:
bool power_domain_is_on(domain)Would that work?
Not really. We should have an API which returns -EALREADY or -EBUSY based on the ref counts. But yes, we can change the naming, so that this 'deterministic' ones have a different name, with the current names used for your ref-counting one. Then the ref-counting one is a thin layer on top of the deterministic one.
Honestly, I am not convinced, but I will anyway assume we need a way to force a domain off. There is no need to force a power domain on however, as after power_domain_on(), the power domain cannot be off (except error condition of course).
So I'll add a helper to force power off. It will be called power_domain_off_force() and be preceded with a comment stating that this is not the standard approach, to guide people towards the refcounted helper instead.
Would this address your request?
I believe it would be better to have a 'low-level' function which handles the refcounting and returns -EALREADY or -EBUSY if it does nothing, with your 'Linux' functions sitting above that. You can put any comment you like on the low-level function.
The idea of 'forcing' something just seems odd to me. I imagine people would call that just to be sure :-)
Regards, Simon
Hi Simon,
On 25/01/2025 at 10:11:22 -07, Simon Glass sjg@chromium.org wrote:
Hi Miquel,
On Fri, 24 Jan 2025 at 08:20, Miquel Raynal miquel.raynal@bootlin.com wrote:
Hi Simon,
If we want a power domain to actually turn off, how many times do we need to call power_domain_off()?
We do not? It is why I add refcounting, if a power domain has 2 consumers, each consumer says whether it needs the power domain to be on or off and the core will handle, but in no case they should force it's state.
This is a bootloader, so we do sometimes need to force something off, or on.
This, I do understand.
The function silently does nothing in many cases, so it is not deterministic.
It is fully deterministic, as long as consumers call power_on/off evenly (and this is already what we request in U-Boot).
But I foresee people setting up power domains in board code, not drivers. My concern is that this is hiding the real state.
Setting up power domains in board code is drawback. It is legacy behaviour, people should switch to the device model (ie. using a proper DT description) and stop messing around with board files. It's been like 5 years since U-Boot forced people to transition, I wouldn't feel bad if these boards were no longer behaving like expected (mind there are very little chances this will break anything, as the kernel is supposed to re-init these domains anyway).
While I agree with you, the message is not fully landing, certainly not in SPL where there are code-size constraints.
I don't think the SPL is a problem here? I consider broken a board file with one main function calling enable() twice on the same domain and disable() only once. Although I am fine implementing a "force power off", I still consider this is not a correct approach.
In the case where we *actually* want to turn the power domain off, we are at a loss as to what code to write.
Hence, I do not agree with returning error codes in these situations, they are misleading and they would have to be ignored anyway.
How about creating a power_domain_off_if_allowed() or power_domain_soft_off/on() or power_domain_req_off (for request)?
The power domain logic has a hardware reality in many SoCs but is also a software concept that is shared with Linux. Changing the semantics in U-Boot would be very misleading and if my understanding is correct, this approach would be new.
Yes this is quite a strong argument. Can you point me to the equivalent API in Linux? I see pm_domain but not the same as U-Boot
Yes, pmdomain (former genpd if I get it correctly), how are they different? From my point of view it is the same. The same devices are supported. So why would we want the API to be different?
But can you please point me to the API? I am seeing struct generic_pm_domain, which is uncommented, so it isn't clear what the power_off() and power_on() functions actually do. Perhaps they work differently from what you think?
->power_on() and ->power_off() perform a state transition, like you would expect. But these callbacks are not supposed to be called directly, they are typically called in the genpd_power_on/off() helpers (through _genpd_power_on/off()), see below.
If you really want a way to track the state of the power domain, however, I can maybe add a helper returning its state, ie:
bool power_domain_is_on(domain)Would that work?
Not really. We should have an API which returns -EALREADY or -EBUSY based on the ref counts.
In Linux, we do not return -EALREADY if the refcount of a power domain indicates it is already on when powering up: https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L922
However, while we do return -EBUSY when turning it off if there are children still enabled, (https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L848) this function is not exposed and there is not a single caller that actually checks the return code. As a matter of fact, disabling the power domains is mostly done asynchronously anyway in a work queue that is supposed to "try" disabling the unused domains: https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L289... So in the end, consumers just do not care about the final status of the domain, and they are unaware of it.
Please note that most of the handling of the domains is abstracted through Runtime PM handling where, again, the return value is never forwarded to the device drivers.
But yes, we can change the naming, so that this 'deterministic' ones have a different name, with the current names used for your ref-counting one. Then the ref-counting one is a thin layer on top of the deterministic one.
Honestly, I am not convinced, but I will anyway assume we need a way to force a domain off. There is no need to force a power domain on however, as after power_domain_on(), the power domain cannot be off (except error condition of course).
So I'll add a helper to force power off. It will be called power_domain_off_force() and be preceded with a comment stating that this is not the standard approach, to guide people towards the refcounted helper instead.
Would this address your request?
I believe it would be better to have a 'low-level' function which handles the refcounting and returns -EALREADY or -EBUSY if it does nothing, with your 'Linux' functions sitting above that. You can put any comment you like on the low-level function.
The idea of 'forcing' something just seems odd to me. I imagine people would call that just to be sure :-)
I am okay having two levels, one returning statuses and the other one hiding them. After some time if nobody complained nor used the low-level one, we can certainly merge them. So I would propose something like:
int power_domain_on_low_level() { //refcount handling, power_on() if needed }
static inline int power_domain_on(pd) { int ret = power_domain_on_low_level(pd); if (ret == -EBUSY || ret == -EALREADY) ret = 0; return ret; }
Would that work for you?
Thanks, Miquèl
Hi Miquel,
On Mon, 27 Jan 2025 at 10:11, Miquel Raynal miquel.raynal@bootlin.com wrote:
Hi Simon,
On 25/01/2025 at 10:11:22 -07, Simon Glass sjg@chromium.org wrote:
Hi Miquel,
On Fri, 24 Jan 2025 at 08:20, Miquel Raynal miquel.raynal@bootlin.com wrote:
Hi Simon,
If we want a power domain to actually turn off, how many times do we need to call power_domain_off()?
We do not? It is why I add refcounting, if a power domain has 2 consumers, each consumer says whether it needs the power domain to be on or off and the core will handle, but in no case they should force it's state.
This is a bootloader, so we do sometimes need to force something off, or on.
This, I do understand.
The function silently does nothing in many cases, so it is not deterministic.
It is fully deterministic, as long as consumers call power_on/off evenly (and this is already what we request in U-Boot).
But I foresee people setting up power domains in board code, not drivers. My concern is that this is hiding the real state.
Setting up power domains in board code is drawback. It is legacy behaviour, people should switch to the device model (ie. using a proper DT description) and stop messing around with board files. It's been like 5 years since U-Boot forced people to transition, I wouldn't feel bad if these boards were no longer behaving like expected (mind there are very little chances this will break anything, as the kernel is supposed to re-init these domains anyway).
While I agree with you, the message is not fully landing, certainly not in SPL where there are code-size constraints.
I don't think the SPL is a problem here? I consider broken a board file with one main function calling enable() twice on the same domain and disable() only once. Although I am fine implementing a "force power off", I still consider this is not a correct approach.
In the case where we *actually* want to turn the power domain off, we are at a loss as to what code to write.
> Hence, I do not agree with returning error codes in these situations, > they are misleading and they would have to be ignored anyway. >
How about creating a power_domain_off_if_allowed() or power_domain_soft_off/on() or power_domain_req_off (for request)?
The power domain logic has a hardware reality in many SoCs but is also a software concept that is shared with Linux. Changing the semantics in U-Boot would be very misleading and if my understanding is correct, this approach would be new.
Yes this is quite a strong argument. Can you point me to the equivalent API in Linux? I see pm_domain but not the same as U-Boot
Yes, pmdomain (former genpd if I get it correctly), how are they different? From my point of view it is the same. The same devices are supported. So why would we want the API to be different?
But can you please point me to the API? I am seeing struct generic_pm_domain, which is uncommented, so it isn't clear what the power_off() and power_on() functions actually do. Perhaps they work differently from what you think?
->power_on() and ->power_off() perform a state transition, like you would expect. But these callbacks are not supposed to be called directly, they are typically called in the genpd_power_on/off() helpers (through _genpd_power_on/off()), see below.
If you really want a way to track the state of the power domain, however, I can maybe add a helper returning its state, ie:
bool power_domain_is_on(domain)Would that work?
Not really. We should have an API which returns -EALREADY or -EBUSY based on the ref counts.
In Linux, we do not return -EALREADY if the refcount of a power domain indicates it is already on when powering up: https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L922
However, while we do return -EBUSY when turning it off if there are children still enabled, (https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L848) this function is not exposed and there is not a single caller that actually checks the return code. As a matter of fact, disabling the power domains is mostly done asynchronously anyway in a work queue that is supposed to "try" disabling the unused domains: https://elixir.bootlin.com/linux/v6.12.6/source/drivers/pmdomain/core.c#L289... So in the end, consumers just do not care about the final status of the domain, and they are unaware of it.
Please note that most of the handling of the domains is abstracted through Runtime PM handling where, again, the return value is never forwarded to the device drivers.
But yes, we can change the naming, so that this 'deterministic' ones have a different name, with the current names used for your ref-counting one. Then the ref-counting one is a thin layer on top of the deterministic one.
Honestly, I am not convinced, but I will anyway assume we need a way to force a domain off. There is no need to force a power domain on however, as after power_domain_on(), the power domain cannot be off (except error condition of course).
So I'll add a helper to force power off. It will be called power_domain_off_force() and be preceded with a comment stating that this is not the standard approach, to guide people towards the refcounted helper instead.
Would this address your request?
I believe it would be better to have a 'low-level' function which handles the refcounting and returns -EALREADY or -EBUSY if it does nothing, with your 'Linux' functions sitting above that. You can put any comment you like on the low-level function.
The idea of 'forcing' something just seems odd to me. I imagine people would call that just to be sure :-)
I am okay having two levels, one returning statuses and the other one hiding them. After some time if nobody complained nor used the low-level one, we can certainly merge them. So I would propose something like:
int power_domain_on_low_level() { //refcount handling, power_on() if needed }
static inline int power_domain_on(pd) { int ret = power_domain_on_low_level(pd); if (ret == -EBUSY || ret == -EALREADY) ret = 0; return ret; }
Would that work for you?
Yes, that's perfect.
Regards, Simon
Hi Simon,
Cheers, Miquèl
Reparenting a clock C with a new parent P means that C will only continue clocking if P is already clocking when the mux is updated. In case the parent is currently disabled, failures (stalls) are likely to happen.
This is exactly what happens on i.MX8 when enabling the video pipeline. We tell LCDIF clocks to use the VIDEO PLL as input, while the VIDEO PLL is currently off. This all happens as part of the assigned-clocks handling procedure, where the reparenting happens before the enable() calls. Enabling the parents as part of the reparenting procedure seems sane and also matches the logic applied in other parts of the CCM.
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/clk/clk-uclass.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index a9937c22dcb6bcc90b14f748a0758810a7435d61..dc89e3071130f1e1c98001a955c4fec210aa5b47 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -593,14 +593,27 @@ int clk_set_parent(struct clk *clk, struct clk *parent) if (!ops->set_parent) return -ENOSYS;
- ret = ops->set_parent(clk, parent); - if (ret) + ret = clk_enable(parent); + if (ret) { + printf("Cannot enable parent %s\n", parent->dev->name); return ret; + }
- if (CONFIG_IS_ENABLED(CLK_CCF)) + ret = ops->set_parent(clk, parent); + if (ret) { + clk_disable(parent); + return ret; + } + + if (CONFIG_IS_ENABLED(CLK_CCF)) { ret = device_reparent(clk->dev, parent->dev); + if (ret) { + clk_disable(parent); + return ret; + } + }
- return ret; + return 0; }
int clk_enable(struct clk *clk)
These are all the clocks needed to get an LCD panel working, going through one of the LCDIF and the LDB. The media AXI and APB clocks are also described.
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/clk/imx/clk-imx8mp.c | 69 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)
diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c index 1d04090ca00785fdf1f4fc751a69506664dbed89..d22676fc55413cf73825d0b4e89014352e303e72 100644 --- a/drivers/clk/imx/clk-imx8mp.c +++ b/drivers/clk/imx/clk-imx8mp.c @@ -14,7 +14,14 @@
#include "clk.h"
+#if CONFIG_IS_ENABLED(VIDEO) +static u32 share_count_media; +#endif + static const char * const pll_ref_sels[] = { "clock-osc-24m", "dummy", "dummy", "dummy", }; +#if CONFIG_IS_ENABLED(VIDEO) +static const char * const video_pll1_bypass_sels[] = {"video_pll1", "video_pll1_ref_sel", }; +#endif static const char * const dram_pll_bypass_sels[] = {"dram_pll", "dram_pll_ref_sel", }; static const char * const arm_pll_bypass_sels[] = {"arm_pll", "arm_pll_ref_sel", }; static const char * const sys_pll1_bypass_sels[] = {"sys_pll1", "sys_pll1_ref_sel", }; @@ -31,6 +38,12 @@ static const char * const imx8mp_hsio_axi_sels[] = {"clock-osc-24m", "sys_pll2_5 "sys_pll2_100m", "sys_pll2_200m", "clk_ext2", "clk_ext4", "audio_pll2_out", };
+#if CONFIG_IS_ENABLED(VIDEO) +static const char * const imx8mp_media_isp_sels[] = {"clock-osc-24m", "sys_pll2_1000m", "sys_pll1_800m", + "sys_pll3_out", "sys_pll1_400m", "audio_pll2_out", + "clk_ext1", "sys_pll2_500m", }; +#endif + static const char * const imx8mp_main_axi_sels[] = {"clock-osc-24m", "sys_pll2_333m", "sys_pll1_800m", "sys_pll2_250m", "sys_pll2_1000m", "audio_pll1_out", "video_pll1_out", "sys_pll1_100m",}; @@ -43,6 +56,16 @@ static const char * const imx8mp_nand_usdhc_sels[] = {"clock-osc-24m", "sys_pll1 "sys_pll2_200m", "sys_pll1_133m", "sys_pll3_out", "sys_pll2_250m", "audio_pll1_out", };
+#if CONFIG_IS_ENABLED(VIDEO) +static const char * const imx8mp_media_axi_sels[] = {"clock-osc-24m", "sys_pll2_1000m", "sys_pll1_800m", + "sys_pll3_out", "sys_pll1_40m", "audio_pll2_out", + "clk_ext1", "sys_pll2_500m", }; + +static const char * const imx8mp_media_apb_sels[] = {"clock-osc-24m", "sys_pll2_125m", "sys_pll1_800m", + "sys_pll3_out", "sys_pll1_40m", "audio_pll2_out", + "clk_ext1", "sys_pll1_133m", }; +#endif + static const char * const imx8mp_noc_sels[] = {"clock-osc-24m", "sys_pll1_800m", "sys_pll3_out", "sys_pll2_1000m", "sys_pll2_500m", "audio_pll1_out", "video_pll1_out", "audio_pll2_out", }; @@ -175,6 +198,17 @@ static const char * const imx8mp_usdhc3_sels[] = {"clock-osc-24m", "sys_pll1_400 "sys_pll2_500m", "sys_pll3_out", "sys_pll1_266m", "audio_pll2_out", "sys_pll1_100m", };
+#if CONFIG_IS_ENABLED(VIDEO) +static const char * const imx8mp_media_disp_pix_sels[] = {"clock-osc-24m", "video_pll1_out", "audio_pll2_out", + "audio_pll1_out", "sys_pll1_800m", + "sys_pll2_1000m", "sys_pll3_out", "clk_ext4", }; + +static const char * const imx8mp_media_ldb_sels[] = {"clock-osc-24m", "sys_pll2_333m", "sys_pll2_100m", + "sys_pll1_800m", "sys_pll2_1000m", + "clk_ext2", "audio_pll2_out", + "video_pll1_out", }; +#endif + static const char * const imx8mp_enet_ref_sels[] = {"clock-osc-24m", "sys_pll2_125m", "sys_pll2_50m", "sys_pll2_100m", "sys_pll1_160m", "audio_pll1_out", "video_pll1_out", "clk_ext4", }; @@ -199,12 +233,19 @@ static int imx8mp_clk_probe(struct udevice *dev)
clk_dm(IMX8MP_CLK_DUMMY, clk_register_fixed_rate(NULL, "dummy", 0));
+#if CONFIG_IS_ENABLED(VIDEO) + clk_dm(IMX8MP_VIDEO_PLL1_REF_SEL, imx_clk_mux("video_pll1_ref_sel", base + 0x28, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels))); +#endif clk_dm(IMX8MP_DRAM_PLL_REF_SEL, imx_clk_mux("dram_pll_ref_sel", base + 0x50, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels))); clk_dm(IMX8MP_ARM_PLL_REF_SEL, imx_clk_mux("arm_pll_ref_sel", base + 0x84, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels))); clk_dm(IMX8MP_SYS_PLL1_REF_SEL, imx_clk_mux("sys_pll1_ref_sel", base + 0x94, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels))); clk_dm(IMX8MP_SYS_PLL2_REF_SEL, imx_clk_mux("sys_pll2_ref_sel", base + 0x104, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels))); clk_dm(IMX8MP_SYS_PLL3_REF_SEL, imx_clk_mux("sys_pll3_ref_sel", base + 0x114, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels)));
+#if CONFIG_IS_ENABLED(VIDEO) + clk_dm(IMX8MP_VIDEO_PLL1, imx_clk_pll14xx("video_pll1", "video_pll1_ref_sel", base + 0x28, + &imx_1443x_pll)); +#endif clk_dm(IMX8MP_DRAM_PLL, imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx_1443x_dram_pll)); clk_dm(IMX8MP_ARM_PLL, imx_clk_pll14xx("arm_pll", "arm_pll_ref_sel", base + 0x84, @@ -216,12 +257,18 @@ static int imx8mp_clk_probe(struct udevice *dev) clk_dm(IMX8MP_SYS_PLL3, imx_clk_pll14xx("sys_pll3", "sys_pll3_ref_sel", base + 0x114, &imx_1416x_pll));
+#if CONFIG_IS_ENABLED(VIDEO) + clk_dm(IMX8MP_VIDEO_PLL1_BYPASS, imx_clk_mux_flags("video_pll1_bypass", base + 0x28, 16, 1, video_pll1_bypass_sels, ARRAY_SIZE(video_pll1_bypass_sels), CLK_SET_RATE_PARENT)); +#endif clk_dm(IMX8MP_DRAM_PLL_BYPASS, imx_clk_mux_flags("dram_pll_bypass", base + 0x50, 4, 1, dram_pll_bypass_sels, ARRAY_SIZE(dram_pll_bypass_sels), CLK_SET_RATE_PARENT)); clk_dm(IMX8MP_ARM_PLL_BYPASS, imx_clk_mux_flags("arm_pll_bypass", base + 0x84, 4, 1, arm_pll_bypass_sels, ARRAY_SIZE(arm_pll_bypass_sels), CLK_SET_RATE_PARENT)); clk_dm(IMX8MP_SYS_PLL1_BYPASS, imx_clk_mux_flags("sys_pll1_bypass", base + 0x94, 4, 1, sys_pll1_bypass_sels, ARRAY_SIZE(sys_pll1_bypass_sels), CLK_SET_RATE_PARENT)); clk_dm(IMX8MP_SYS_PLL2_BYPASS, imx_clk_mux_flags("sys_pll2_bypass", base + 0x104, 4, 1, sys_pll2_bypass_sels, ARRAY_SIZE(sys_pll2_bypass_sels), CLK_SET_RATE_PARENT)); clk_dm(IMX8MP_SYS_PLL3_BYPASS, imx_clk_mux_flags("sys_pll3_bypass", base + 0x114, 4, 1, sys_pll3_bypass_sels, ARRAY_SIZE(sys_pll3_bypass_sels), CLK_SET_RATE_PARENT));
+#if CONFIG_IS_ENABLED(VIDEO) + clk_dm(IMX8MP_VIDEO_PLL1_OUT, imx_clk_gate("video_pll1_out", "video_pll1_bypass", base + 0x28, 13)); +#endif clk_dm(IMX8MP_DRAM_PLL_OUT, imx_clk_gate("dram_pll_out", "dram_pll_bypass", base + 0x50, 13)); clk_dm(IMX8MP_ARM_PLL_OUT, imx_clk_gate("arm_pll_out", "arm_pll_bypass", base + 0x84, 11)); clk_dm(IMX8MP_SYS_PLL1_OUT, imx_clk_gate("sys_pll1_out", "sys_pll1_bypass", base + 0x94, 11)); @@ -267,13 +314,23 @@ static int imx8mp_clk_probe(struct udevice *dev) clk_dm(IMX8MP_CLK_A53_DIV, imx_clk_divider2("arm_a53_div", "arm_a53_cg", base + 0x8000, 0, 3));
clk_dm(IMX8MP_CLK_HSIO_AXI, imx8m_clk_composite("hsio_axi", imx8mp_hsio_axi_sels, base + 0x8380)); +#if CONFIG_IS_ENABLED(VIDEO) + clk_dm(IMX8MP_CLK_MEDIA_ISP, imx8m_clk_composite("media_isp", imx8mp_media_isp_sels, base + 0x8400)); +#endif clk_dm(IMX8MP_CLK_MAIN_AXI, imx8m_clk_composite_critical("main_axi", imx8mp_main_axi_sels, base + 0x8800)); clk_dm(IMX8MP_CLK_ENET_AXI, imx8m_clk_composite_critical("enet_axi", imx8mp_enet_axi_sels, base + 0x8880)); clk_dm(IMX8MP_CLK_NAND_USDHC_BUS, imx8m_clk_composite_critical("nand_usdhc_bus", imx8mp_nand_usdhc_sels, base + 0x8900)); +#if CONFIG_IS_ENABLED(VIDEO) + clk_dm(IMX8MP_CLK_MEDIA_AXI, imx8m_clk_composite("media_axi", imx8mp_media_axi_sels, base + 0x8a00)); + clk_dm(IMX8MP_CLK_MEDIA_APB, imx8m_clk_composite("media_apb", imx8mp_media_apb_sels, base + 0x8a80)); +#endif clk_dm(IMX8MP_CLK_NOC, imx8m_clk_composite_critical("noc", imx8mp_noc_sels, base + 0x8d00)); clk_dm(IMX8MP_CLK_NOC_IO, imx8m_clk_composite_critical("noc_io", imx8mp_noc_io_sels, base + 0x8d80));
clk_dm(IMX8MP_CLK_AHB, imx8m_clk_composite_critical("ahb_root", imx8mp_ahb_sels, base + 0x9000)); +#if CONFIG_IS_ENABLED(VIDEO) + clk_dm(IMX8MP_CLK_MEDIA_DISP2_PIX, imx8m_clk_composite("media_disp2_pix", imx8mp_media_disp_pix_sels, base + 0x9300)); +#endif
clk_dm(IMX8MP_CLK_IPG_ROOT, imx_clk_divider2("ipg_root", "ahb_root", base + 0x9080, 0, 1));
@@ -312,6 +369,10 @@ static int imx8mp_clk_probe(struct udevice *dev)
clk_dm(IMX8MP_CLK_WDOG, imx8m_clk_composite("wdog", imx8mp_wdog_sels, base + 0xb900)); clk_dm(IMX8MP_CLK_USDHC3, imx8m_clk_composite("usdhc3", imx8mp_usdhc3_sels, base + 0xbc80)); +#if CONFIG_IS_ENABLED(VIDEO) + clk_dm(IMX8MP_CLK_MEDIA_DISP1_PIX, imx8m_clk_composite("media_disp1_pix", imx8mp_media_disp_pix_sels, base + 0xbe00)); + clk_dm(IMX8MP_CLK_MEDIA_LDB, imx8m_clk_composite("media_ldb", imx8mp_media_ldb_sels, base + 0xbf00)); +#endif
clk_dm(IMX8MP_CLK_DRAM_ALT_ROOT, imx_clk_fixed_factor("dram_alt_root", "dram_alt", 1, 4)); clk_dm(IMX8MP_CLK_DRAM_CORE, imx_clk_mux2_flags("dram_core_clk", base + 0x9800, 24, 1, imx8mp_dram_core_sels, ARRAY_SIZE(imx8mp_dram_core_sels), CLK_IS_CRITICAL)); @@ -355,6 +416,14 @@ static int imx8mp_clk_probe(struct udevice *dev) clk_dm(IMX8MP_CLK_WDOG2_ROOT, imx_clk_gate4("wdog2_root_clk", "wdog", base + 0x4540, 0)); clk_dm(IMX8MP_CLK_WDOG3_ROOT, imx_clk_gate4("wdog3_root_clk", "wdog", base + 0x4550, 0)); clk_dm(IMX8MP_CLK_HSIO_ROOT, imx_clk_gate4("hsio_root_clk", "ipg_root", base + 0x45c0, 0)); +#if CONFIG_IS_ENABLED(VIDEO) + clk_dm(IMX8MP_CLK_MEDIA_APB_ROOT, imx_clk_gate2_shared2("media_apb_root_clk", "media_apb", base + 0x45d0, 0, &share_count_media)); + clk_dm(IMX8MP_CLK_MEDIA_AXI_ROOT, imx_clk_gate2_shared2("media_axi_root_clk", "media_axi", base + 0x45d0, 0, &share_count_media)); + clk_dm(IMX8MP_CLK_MEDIA_DISP1_PIX_ROOT, imx_clk_gate2_shared2("media_disp1_pix_root_clk", "media_disp1_pix", base + 0x45d0, 0, &share_count_media)); + clk_dm(IMX8MP_CLK_MEDIA_DISP2_PIX_ROOT, imx_clk_gate2_shared2("media_disp2_pix_root_clk", "media_disp2_pix", base + 0x45d0, 0, &share_count_media)); + clk_dm(IMX8MP_CLK_MEDIA_LDB_ROOT, imx_clk_gate2_shared2("media_ldb_root_clk", "media_ldb", base + 0x45d0, 0, &share_count_media)); + clk_dm(IMX8MP_CLK_MEDIA_ISP_ROOT, imx_clk_gate2_shared2("media_isp_root_clk", "media_isp", base + 0x45d0, 0, &share_count_media)); +#endif
clk_dm(IMX8MP_CLK_USDHC3_ROOT, imx_clk_gate4("usdhc3_root_clk", "usdhc3", base + 0x45e0, 0));
Add support for the i.MX8 MEDIAMIX domain which is driving the power over the whole display/rendering pipeline.
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com Reviewed-by: Fabio Estevam festevam@gmail.com --- drivers/power/domain/imx8m-power-domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/drivers/power/domain/imx8m-power-domain.c b/drivers/power/domain/imx8m-power-domain.c index c22fbe60675e96a628f205d10a71b66c7b33ef00..e54ba5d9a5476f678fb48fb16e7217b031acd78a 100644 --- a/drivers/power/domain/imx8m-power-domain.c +++ b/drivers/power/domain/imx8m-power-domain.c @@ -40,6 +40,7 @@ DECLARE_GLOBAL_DATA_PTR; #define IMX8MN_MIPI_A53_DOMAIN BIT(2)
#define IMX8MP_HSIOMIX_A53_DOMAIN BIT(19) +#define IMX8MP_MEDIAMIX_A53_DOMAIN BIT(12) #define IMX8MP_USB2_PHY_A53_DOMAIN BIT(5) #define IMX8MP_USB1_PHY_A53_DOMAIN BIT(4) #define IMX8MP_PCIE_PHY_A53_DOMAIN BIT(3) @@ -63,6 +64,7 @@ DECLARE_GLOBAL_DATA_PTR; #define IMX8MN_MIPI_SW_Pxx_REQ BIT(0)
#define IMX8MP_HSIOMIX_Pxx_REQ BIT(17) +#define IMX8MP_MEDIAMIX_Pxx_REQ BIT(10) #define IMX8MP_USB2_PHY_Pxx_REQ BIT(3) #define IMX8MP_USB1_PHY_Pxx_REQ BIT(2) #define IMX8MP_PCIE_PHY_SW_Pxx_REQ BIT(1) @@ -81,6 +83,9 @@ DECLARE_GLOBAL_DATA_PTR; #define IMX8MP_HSIOMIX_PWRDNACKN BIT(28) #define IMX8MP_HSIOMIX_PWRDNREQN BIT(12)
+#define IMX8MP_MEDIAMIX_PWRDNACKN BIT(30) +#define IMX8MP_MEDIAMIX_PWRDNREQN BIT(14) + /* * The PGC offset values in Reference Manual * (Rev. 1, 01/2018 and the older ones) GPC chapter's @@ -101,6 +106,7 @@ DECLARE_GLOBAL_DATA_PTR; #define IMX8MP_PGC_PCIE 13 #define IMX8MP_PGC_USB1 14 #define IMX8MP_PGC_USB2 15 +#define IMX8MP_PGC_MEDIAMIX 22 #define IMX8MP_PGC_HSIOMIX 29
#define GPC_PGC_CTRL(n) (0x800 + (n) * 0x40) @@ -303,6 +309,17 @@ static const struct imx_pgc_domain imx8mp_pgc_domains[] = { .pgc = BIT(IMX8MP_PGC_HSIOMIX), .keep_clocks = true, }, + + [IMX8MP_POWER_DOMAIN_MEDIAMIX] = { + .bits = { + .pxx = IMX8MP_MEDIAMIX_Pxx_REQ, + .map = IMX8MP_MEDIAMIX_A53_DOMAIN, + .hskreq = IMX8MP_MEDIAMIX_PWRDNREQN, + .hskack = IMX8MP_MEDIAMIX_PWRDNACKN, + }, + .pgc = BIT(IMX8MP_PGC_MEDIAMIX), + .keep_clocks = true, + }, };
static const struct imx_pgc_regs imx8mp_pgc_regs = {
This block delivers power and clocks to the whole display and rendering pipeline.
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/power/domain/Kconfig | 7 ++ drivers/power/domain/Makefile | 1 + drivers/power/domain/imx8mp-mediamix.c | 208 +++++++++++++++++++++++++++++++++ 3 files changed, 216 insertions(+)
diff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig index bd82d2f7044b3c3f8c88dea27c39193efedb4c84..5f5218bd8b5f4bed8283e91da61ac7c8c3eb97ae 100644 --- a/drivers/power/domain/Kconfig +++ b/drivers/power/domain/Kconfig @@ -47,6 +47,13 @@ config IMX8MP_HSIOMIX_BLKCTRL help Enable support for manipulating NXP i.MX8MP on-SoC HSIOMIX block controller.
+config IMX8MP_MEDIAMIX_BLKCTRL + bool "Enable i.MX8MP MEDIAMIX domain driver" + depends on POWER_DOMAIN && IMX8MP + select CLK + help + Enable support for manipulating NXP i.MX8MP on-SoC MEDIAMIX block controller. + config MTK_POWER_DOMAIN bool "Enable the MediaTek power domain driver" depends on POWER_DOMAIN && ARCH_MEDIATEK diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile index 110646c503ab97941a2bb86caf53f94542008219..356ec07163fd299f00e81a629ffc06c663a26c25 100644 --- a/drivers/power/domain/Makefile +++ b/drivers/power/domain/Makefile @@ -9,6 +9,7 @@ obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain-legacy.o imx8-power-domain.o obj-$(CONFIG_IMX8M_POWER_DOMAIN) += imx8m-power-domain.o obj-$(CONFIG_IMX8MP_HSIOMIX_BLKCTRL) += imx8mp-hsiomix.o +obj-$(CONFIG_IMX8MP_MEDIAMIX_BLKCTRL) += imx8mp-mediamix.o obj-$(CONFIG_MTK_POWER_DOMAIN) += mtk-power-domain.o obj-$(CONFIG_MESON_GX_VPU_POWER_DOMAIN) += meson-gx-pwrc-vpu.o obj-$(CONFIG_MESON_EE_POWER_DOMAIN) += meson-ee-pwrc.o diff --git a/drivers/power/domain/imx8mp-mediamix.c b/drivers/power/domain/imx8mp-mediamix.c new file mode 100644 index 0000000000000000000000000000000000000000..78c32ca3d3a87febdefd5d128d39d817674b8d32 --- /dev/null +++ b/drivers/power/domain/imx8mp-mediamix.c @@ -0,0 +1,208 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * i.MX8 MEDIAMIX control block driver + * Copyright (C) 2024 Miquel Raynal miquel.raynal@bootlin.com + * Inspired from Marek Vasut marex@denx.de work on the hsiomix driver. + */ + +#include <asm/io.h> +#include <clk.h> +#include <dm.h> +#include <power-domain-uclass.h> +#include <linux/delay.h> + +#include <dt-bindings/power/imx8mp-power.h> + +#define BLK_SFT_RSTN 0x0 +#define BLK_CLK_EN 0x4 + +struct imx8mp_mediamix_priv { + void __iomem *base; + struct clk clk_apb; + struct clk clk_axi; + struct clk clk_disp2; + struct power_domain pd_bus; + struct power_domain pd_lcdif2; +}; + +static int imx8mp_mediamix_on(struct power_domain *power_domain) +{ + struct udevice *dev = power_domain->dev; + struct imx8mp_mediamix_priv *priv = dev_get_priv(dev); + struct power_domain *domain; + struct clk *clk; + u32 reset; + int ret; + + switch (power_domain->id) { + case IMX8MP_MEDIABLK_PD_LCDIF_2: + domain = &priv->pd_lcdif2; + clk = &priv->clk_disp2; + reset = BIT(11) | BIT(12) | BIT(24); + break; + default: + return -EINVAL; + } + + /* Make sure bus domain is awake */ + ret = power_domain_on(&priv->pd_bus); + if (ret) + return ret; + + /* Put devices into reset */ + clrbits_le32(priv->base + BLK_SFT_RSTN, reset); + + /* Enable upstream clocks */ + ret = clk_enable(&priv->clk_apb); + if (ret) + goto dis_bus_pd; + + ret = clk_enable(&priv->clk_axi); + if (ret) + goto dis_apb_clk; + + /* Enable blk-ctrl clock to allow reset to propagate */ + ret = clk_enable(clk); + if (ret) + goto dis_axi_clk; + setbits_le32(priv->base + BLK_CLK_EN, reset); + + /* Power up upstream GPC domain */ + ret = power_domain_on(domain); + if (ret) + goto dis_lcdif_clk; + + /* Wait for reset to propagate */ + udelay(5); + + /* Release reset */ + setbits_le32(priv->base + BLK_SFT_RSTN, reset); + + return 0; + +dis_lcdif_clk: + clk_disable(clk); +dis_axi_clk: + clk_disable(&priv->clk_axi); +dis_apb_clk: + clk_disable(&priv->clk_apb); +dis_bus_pd: + power_domain_off(&priv->pd_bus); + + return ret; +} + +static int imx8mp_mediamix_off(struct power_domain *power_domain) +{ + struct udevice *dev = power_domain->dev; + struct imx8mp_mediamix_priv *priv = dev_get_priv(dev); + struct power_domain *domain; + struct clk *clk; + u32 reset; + + switch (power_domain->id) { + case IMX8MP_MEDIABLK_PD_LCDIF_2: + domain = &priv->pd_lcdif2; + clk = &priv->clk_disp2; + reset = BIT(11) | BIT(12) | BIT(24); + break; + default: + return -EINVAL; + } + + /* Put devices into reset and disable clocks */ + clrbits_le32(priv->base + BLK_SFT_RSTN, reset); + clrbits_le32(priv->base + BLK_CLK_EN, reset); + + /* Power down upstream GPC domain */ + power_domain_off(domain); + + clk_disable(clk); + clk_disable(&priv->clk_axi); + clk_disable(&priv->clk_apb); + + /* Allow bus domain to suspend */ + power_domain_off(&priv->pd_bus); + + return 0; +} + +static int imx8mp_mediamix_of_xlate(struct power_domain *power_domain, + struct ofnode_phandle_args *args) +{ + power_domain->id = args->args[0]; + + return 0; +} + +static int imx8mp_mediamix_bind(struct udevice *dev) +{ + /* Bind child lcdif */ + return dm_scan_fdt_dev(dev); +} + +static int imx8mp_mediamix_probe(struct udevice *dev) +{ + struct imx8mp_mediamix_priv *priv = dev_get_priv(dev); + int ret; + + priv->base = dev_read_addr_ptr(dev); + + ret = clk_get_by_name(dev, "apb", &priv->clk_apb); + if (ret < 0) + return ret; + + ret = clk_get_by_name(dev, "axi", &priv->clk_axi); + if (ret < 0) + return ret; + + ret = clk_get_by_name(dev, "disp2", &priv->clk_disp2); + if (ret < 0) + return ret; + + ret = power_domain_get_by_name(dev, &priv->pd_bus, "bus"); + if (ret < 0) + return ret; + + ret = power_domain_get_by_name(dev, &priv->pd_lcdif2, "lcdif2"); + if (ret < 0) + goto free_bus_pd; + + return 0; + +free_bus_pd: + power_domain_free(&priv->pd_bus); + return ret; +} + +static int imx8mp_mediamix_remove(struct udevice *dev) +{ + struct imx8mp_mediamix_priv *priv = dev_get_priv(dev); + + power_domain_free(&priv->pd_lcdif2); + power_domain_free(&priv->pd_bus); + + return 0; +} + +static const struct udevice_id imx8mp_mediamix_ids[] = { + { .compatible = "fsl,imx8mp-media-blk-ctrl" }, + { } +}; + +struct power_domain_ops imx8mp_mediamix_ops = { + .on = imx8mp_mediamix_on, + .off = imx8mp_mediamix_off, + .of_xlate = imx8mp_mediamix_of_xlate, +}; + +U_BOOT_DRIVER(imx8mp_mediamix) = { + .name = "imx8mp_mediamix", + .id = UCLASS_POWER_DOMAIN, + .of_match = imx8mp_mediamix_ids, + .bind = imx8mp_mediamix_bind, + .probe = imx8mp_mediamix_probe, + .remove = imx8mp_mediamix_remove, + .priv_auto = sizeof(struct imx8mp_mediamix_priv), + .ops = &imx8mp_mediamix_ops, +};
The IPUv3 is one IP part of the imx world, there are others, and selecting the whole imx/ folder based on such a specific Kconfig symbol is sub-optimal. Let's always enter the imx/ folder, and then selectively compile parts of the folder based on the configuration.
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/video/Makefile | 2 +- drivers/video/imx/Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 5a00438ce0648effdcade42ee255f037c65b00d3..15ba0f1697ee93f2227f60efa67f4ed378f5780a 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -52,7 +52,7 @@ obj-$(CONFIG_VIDEO_COREBOOT) += coreboot.o obj-$(CONFIG_VIDEO_DW_HDMI) += dw_hdmi.o obj-$(CONFIG_VIDEO_DW_MIPI_DSI) += dw_mipi_dsi.o obj-$(CONFIG_VIDEO_EFI) += efi.o -obj-$(CONFIG_VIDEO_IPUV3) += imx/ +obj-y += imx/ obj-$(CONFIG_VIDEO_IVYBRIDGE_IGD) += ivybridge_igd.o obj-$(CONFIG_VIDEO_LCD_ANX9804) += anx9804.o obj-$(CONFIG_VIDEO_LCD_ENDEAVORU) += endeavoru-panel.o diff --git a/drivers/video/imx/Makefile b/drivers/video/imx/Makefile index 179ea651fe8495e24206e2b06354a4f36651056c..46cc44d64b00959fb3880c7befb04a1ebdda1143 100644 --- a/drivers/video/imx/Makefile +++ b/drivers/video/imx/Makefile @@ -3,4 +3,4 @@ # (C) Copyright 2000-2007 # Wolfgang Denk, DENX Software Engineering, wd@denx.de.
-obj-y += mxc_ipuv3_fb.o ipu_common.o ipu_disp.o +obj-$(CONFIG_VIDEO_IPUV3) += mxc_ipuv3_fb.o ipu_common.o ipu_disp.o
Add support for the LVDS Display Bridge (LDB) found on i.MX8MP.
When attached, the bridge driver looks for panels connected to one of its two outputs and adapts its own configuration to use them. There is currently no support for merged/split displays.
Note regarding the clock configuration: The LDB output clock should be absolutely identical to the LCDIF output clock so both blocks can talk to each other synchronously. However, the LDB clock has an internal divisor of 7 (respectively 3.5 in dual configuration) which means the LDB input clock must be explicitly set once we know the configuration.
This driver was tested on i.MX8MP using a single panel connected to the LVDS2 interface.
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/video/imx/Kconfig | 6 ++ drivers/video/imx/Makefile | 1 + drivers/video/imx/ldb.c | 251 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 258 insertions(+)
diff --git a/drivers/video/imx/Kconfig b/drivers/video/imx/Kconfig index 34e8b640595b85678e386a3a251340031780a72e..c5cd2fecf780b62c0264b9627137517388aae1ab 100644 --- a/drivers/video/imx/Kconfig +++ b/drivers/video/imx/Kconfig @@ -13,3 +13,9 @@ config IMX_VIDEO_SKIP config IMX_HDMI bool "Enable HDMI support in IPUv3" depends on VIDEO_IPUV3 + +config IMX_LDB + bool "Freescale i.MX8MP LDB bridge" + depends on VIDEO_BRIDGE + help + Support for i.MX8MP DPI-to-LVDS on-SoC encoder. diff --git a/drivers/video/imx/Makefile b/drivers/video/imx/Makefile index 46cc44d64b00959fb3880c7befb04a1ebdda1143..8d106589b7544ef9694613a2284e2fbfb564cf63 100644 --- a/drivers/video/imx/Makefile +++ b/drivers/video/imx/Makefile @@ -4,3 +4,4 @@ # Wolfgang Denk, DENX Software Engineering, wd@denx.de.
obj-$(CONFIG_VIDEO_IPUV3) += mxc_ipuv3_fb.o ipu_common.o ipu_disp.o +obj-$(CONFIG_IMX_LDB) += ldb.o diff --git a/drivers/video/imx/ldb.c b/drivers/video/imx/ldb.c new file mode 100644 index 0000000000000000000000000000000000000000..10be4421cb57ee8e1ab68eaf7568bc955d31e241 --- /dev/null +++ b/drivers/video/imx/ldb.c @@ -0,0 +1,251 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Derived work from: + * Philippe Cornu philippe.cornu@st.com + * Yannick Fertre yannick.fertre@st.com + * Adapted by Miquel Raynal miquel.raynal@bootlin.com + */ + +#define LOG_CATEGORY UCLASS_VIDEO_BRIDGE + +#include <clk.h> +#include <dm.h> +#include <log.h> +#include <panel.h> +#include <video_bridge.h> +#include <asm/io.h> +#include <linux/delay.h> + +#define LDB_CTRL_CH0_ENABLE BIT(0) +#define LDB_CTRL_CH1_ENABLE BIT(2) +#define LDB_CTRL_CH0_DATA_WIDTH BIT(5) +#define LDB_CTRL_CH0_BIT_MAPPING BIT(6) +#define LDB_CTRL_CH1_DATA_WIDTH BIT(7) +#define LDB_CTRL_CH1_BIT_MAPPING BIT(8) +#define LDB_CTRL_DI0_VSYNC_POLARITY BIT(9) +#define LDB_CTRL_DI1_VSYNC_POLARITY BIT(10) + +#define LVDS_CTRL_CH0_EN BIT(0) +#define LVDS_CTRL_CH1_EN BIT(1) +#define LVDS_CTRL_VBG_EN BIT(2) +#define LVDS_CTRL_PRE_EMPH_EN BIT(4) +#define LVDS_CTRL_PRE_EMPH_ADJ(n) (((n) & 0x7) << 5) +#define LVDS_CTRL_CC_ADJ(n) (((n) & 0x7) << 11) + +struct imx_ldb_priv { + struct clk ldb_clk; + void __iomem *ldb_ctrl; + void __iomem *lvds_ctrl; + struct udevice *lvds1; + struct udevice *lvds2; +}; + +static int imx_ldb_set_backlight(struct udevice *dev, int percent) +{ + struct imx_ldb_priv *priv = dev_get_priv(dev); + int ret; + + if (priv->lvds1) { + ret = panel_enable_backlight(priv->lvds1); + if (ret) { + debug("ldb: Cannot enable lvds1 backlight\n"); + return ret; + } + + ret = panel_set_backlight(priv->lvds1, percent); + if (ret) + return ret; + } + + if (priv->lvds2) { + ret = panel_enable_backlight(priv->lvds2); + if (ret) { + debug("ldb: Cannot enable lvds2 backlight\n"); + return ret; + } + + ret = panel_set_backlight(priv->lvds2, percent); + if (ret) + return ret; + } + + return 0; +} + +static int imx_ldb_of_to_plat(struct udevice *dev) +{ + struct imx_ldb_priv *priv = dev_get_priv(dev); + int ret; + + uclass_get_device_by_endpoint(UCLASS_PANEL, dev, 1, &priv->lvds1); + uclass_get_device_by_endpoint(UCLASS_PANEL, dev, 2, &priv->lvds2); + if (!priv->lvds1 && !priv->lvds2) { + debug("ldb: No remote panel for '%s' (ret=%d)\n", + dev_read_name(dev), ret); + return ret; + } + + return 0; +} + +/* The block has a mysterious x7 internal divisor (x3.5 in dual configuration) */ +#define IMX_LDB_INTERNAL_DIVISOR(x) (((x) * 70) / 10) +#define IMX_LDB_INTERNAL_DIVISOR_DUAL(x) (((x) * 35) / 10) + +static ulong imx_ldb_input_rate(struct imx_ldb_priv *priv, + struct display_timing *timings) +{ + ulong target_rate = timings->pixelclock.typ; + + if (priv->lvds1 && priv->lvds2) + return IMX_LDB_INTERNAL_DIVISOR_DUAL(target_rate); + + return IMX_LDB_INTERNAL_DIVISOR(target_rate); +} + +static int imx_ldb_attach(struct udevice *dev) +{ + struct imx_ldb_priv *priv = dev_get_priv(dev); + struct display_timing timings; + bool format_jeida = false; + bool format_24bpp = true; + u32 ldb_ctrl = 0, lvds_ctrl; + ulong ldb_rate; + int ret; + + /* TODO: update the 24bpp/jeida booleans with proper checks when they + * will be supported. + */ + if (priv->lvds1) { + ret = panel_get_display_timing(priv->lvds1, &timings); + if (ret) { + ret = ofnode_decode_display_timing(dev_ofnode(priv->lvds1), + 0, &timings); + if (ret) { + printf("Cannot decode lvds1 timings (%d)\n", ret); + return ret; + } + } + + ldb_ctrl |= LDB_CTRL_CH0_ENABLE; + if (format_24bpp) + ldb_ctrl |= LDB_CTRL_CH0_DATA_WIDTH; + if (format_jeida) + ldb_ctrl |= LDB_CTRL_CH0_BIT_MAPPING; + if (timings.flags & DISPLAY_FLAGS_VSYNC_HIGH) + ldb_ctrl |= LDB_CTRL_DI0_VSYNC_POLARITY; + } + + if (priv->lvds2) { + ret = panel_get_display_timing(priv->lvds2, &timings); + if (ret) { + ret = ofnode_decode_display_timing(dev_ofnode(priv->lvds2), + 0, &timings); + if (ret) { + printf("Cannot decode lvds2 timings (%d)\n", ret); + return ret; + } + } + + ldb_ctrl |= LDB_CTRL_CH1_ENABLE; + if (format_24bpp) + ldb_ctrl |= LDB_CTRL_CH1_DATA_WIDTH; + if (format_jeida) + ldb_ctrl |= LDB_CTRL_CH1_BIT_MAPPING; + if (timings.flags & DISPLAY_FLAGS_VSYNC_HIGH) + ldb_ctrl |= LDB_CTRL_DI1_VSYNC_POLARITY; + } + + /* + * Not all pixel clocks will work, as the final rate (after internal + * integer division) should be identical to the LCDIF clock, otherwise + * the rendering will appear resized/shimmering. + */ + ldb_rate = imx_ldb_input_rate(priv, &timings); + clk_set_rate(&priv->ldb_clk, ldb_rate); + + writel(ldb_ctrl, priv->ldb_ctrl); + + lvds_ctrl = LVDS_CTRL_CC_ADJ(2) | LVDS_CTRL_PRE_EMPH_EN | + LVDS_CTRL_PRE_EMPH_ADJ(3) | LVDS_CTRL_VBG_EN; + writel(lvds_ctrl, priv->lvds_ctrl); + + /* Wait for VBG to stabilize. */ + udelay(15); + + if (priv->lvds1) + lvds_ctrl |= LVDS_CTRL_CH0_EN; + if (priv->lvds2) + lvds_ctrl |= LVDS_CTRL_CH1_EN; + + writel(lvds_ctrl, priv->lvds_ctrl); + + return 0; +} + +static int imx_ldb_probe(struct udevice *dev) +{ + struct imx_ldb_priv *priv = dev_get_priv(dev); + struct udevice *parent = dev_get_parent(dev); + fdt_addr_t parent_addr, child_addr; + int ret; + + ret = clk_get_by_name(dev, "ldb", &priv->ldb_clk); + if (ret < 0) + return ret; + + parent_addr = dev_read_addr(parent); + if (parent_addr == FDT_ADDR_T_NONE) + return -EINVAL; + + child_addr = dev_read_addr_name(dev, "ldb"); + if (child_addr == FDT_ADDR_T_NONE) + return -EINVAL; + + priv->ldb_ctrl = map_physmem(parent_addr + child_addr, 0, MAP_NOCACHE); + if (!priv->ldb_ctrl) + return -EINVAL; + + child_addr = dev_read_addr_name(dev, "lvds"); + if (child_addr == FDT_ADDR_T_NONE) + return -EINVAL; + + priv->lvds_ctrl = map_physmem(parent_addr + child_addr, 0, MAP_NOCACHE); + if (!priv->lvds_ctrl) + return -EINVAL; + + ret = clk_enable(&priv->ldb_clk); + if (ret) + return ret; + + ret = video_bridge_set_active(dev, true); + if (ret) + goto dis_clk; + + return 0; + +dis_clk: + clk_disable(&priv->ldb_clk); + + return ret; +} + +struct video_bridge_ops imx_ldb_ops = { + .attach = imx_ldb_attach, + .set_backlight = imx_ldb_set_backlight, +}; + +static const struct udevice_id imx_ldb_ids[] = { + { .compatible = "fsl,imx8mp-ldb"}, + { } +}; + +U_BOOT_DRIVER(imx_ldb) = { + .name = "imx-lvds-display-bridge", + .id = UCLASS_VIDEO_BRIDGE, + .of_match = imx_ldb_ids, + .probe = imx_ldb_probe, + .of_to_plat = imx_ldb_of_to_plat, + .ops = &imx_ldb_ops, + .priv_auto = sizeof(struct imx_ldb_priv), +};
Add support for the LCD interfaces (LCDIF1/2). When probed, these interfaces request numerous clocks and power domains, attach the bridge and look for a panel in order to retrieve its capabilities and properties.
There is a similar existing driver in the upper folder for other i.MX targets, I discovered this driver a bit late. It is not targeting the i.MX8MP and I have no idea how different can the LCDIF be on this SoC, but I did not manage to get it work, especially because it is not fully compliant with the device-model, especially on the clocks/power management side which is all ad-hoc. This is normal though, it was contributed more than ten years ago.
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/video/imx/Kconfig | 3 + drivers/video/imx/Makefile | 1 + drivers/video/imx/lcdif.c | 314 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 318 insertions(+)
diff --git a/drivers/video/imx/Kconfig b/drivers/video/imx/Kconfig index c5cd2fecf780b62c0264b9627137517388aae1ab..12f11c2eea8c98cc341448923c80919afbceed46 100644 --- a/drivers/video/imx/Kconfig +++ b/drivers/video/imx/Kconfig @@ -19,3 +19,6 @@ config IMX_LDB depends on VIDEO_BRIDGE help Support for i.MX8MP DPI-to-LVDS on-SoC encoder. + +config IMX_LCDIF + bool "i.MX LCDIFv3 LCD controller" diff --git a/drivers/video/imx/Makefile b/drivers/video/imx/Makefile index 8d106589b7544ef9694613a2284e2fbfb564cf63..1edf5a6bdf0c3afb218fbcd81384c7c277ca6b28 100644 --- a/drivers/video/imx/Makefile +++ b/drivers/video/imx/Makefile @@ -5,3 +5,4 @@
obj-$(CONFIG_VIDEO_IPUV3) += mxc_ipuv3_fb.o ipu_common.o ipu_disp.o obj-$(CONFIG_IMX_LDB) += ldb.o +obj-$(CONFIG_IMX_LCDIF) += lcdif.o diff --git a/drivers/video/imx/lcdif.c b/drivers/video/imx/lcdif.c new file mode 100644 index 0000000000000000000000000000000000000000..eb677e84a5e11c8fa2b66973fb3529dd9d2c797b --- /dev/null +++ b/drivers/video/imx/lcdif.c @@ -0,0 +1,314 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * i.MX8 LCD interface driver inspired from the Linux driver + * Copyright 2019 NXP + * Copyright 2024 Bootlin + * Adapted by Miquel Raynal miquel.raynal@bootlin.com + */ + +#include <asm/io.h> +#include <asm/mach-imx/dma.h> +#include <clk.h> +#include <dm.h> +#include <panel.h> +#include <power-domain.h> +#include <video.h> +#include <video_bridge.h> +#include <linux/delay.h> + +#include "../videomodes.h" + +#define LCDIFV3_CTRL 0x0 +#define LCDIFV3_CTRL_SET 0x4 +#define LCDIFV3_CTRL_CLR 0x8 +#define CTRL_INV_HS BIT(0) +#define CTRL_INV_VS BIT(1) +#define CTRL_INV_DE BIT(2) +#define CTRL_INV_PXCK BIT(3) +#define CTRL_CLK_GATE BIT(30) +#define CTRL_SW_RESET BIT(31) + +#define LCDIFV3_DISP_PARA 0x10 +#define DISP_PARA_DISP_MODE_NORMAL 0 +#define DISP_PARA_LINE_PATTERN_RGB_YUV 0 +#define DISP_PARA_DISP_ON BIT(31) + +#define LCDIFV3_DISP_SIZE 0x14 +#define DISP_SIZE_DELTA_X(x) ((x) & 0xffff) +#define DISP_SIZE_DELTA_Y(x) ((x) << 16) + +#define LCDIFV3_HSYN_PARA 0x18 +#define HSYN_PARA_FP_H(x) ((x) & 0xffff) +#define HSYN_PARA_BP_H(x) ((x) << 16) + +#define LCDIFV3_VSYN_PARA 0x1C +#define VSYN_PARA_FP_V(x) ((x) & 0xffff) +#define VSYN_PARA_BP_V(x) ((x) << 16) + +#define LCDIFV3_VSYN_HSYN_WIDTH 0x20 +#define VSYN_HSYN_PW_H(x) ((x) & 0xffff) +#define VSYN_HSYN_PW_V(x) ((x) << 16) + +#define LCDIFV3_CTRLDESCL0_1 0x200 +#define CTRLDESCL0_1_WIDTH(x) ((x) & 0xffff) +#define CTRLDESCL0_1_HEIGHT(x) ((x) << 16) + +#define LCDIFV3_CTRLDESCL0_3 0x208 +#define CTRLDESCL0_3_PITCH(x) ((x) & 0xFFFF) + +#define LCDIFV3_CTRLDESCL_LOW0_4 0x20C +#define LCDIFV3_CTRLDESCL_HIGH0_4 0x210 + +#define LCDIFV3_CTRLDESCL0_5 0x214 +#define CTRLDESCL0_5_YUV_FORMAT(x) (((x) & 0x3) << 14) +#define CTRLDESCL0_5_BPP(x) (((x) & 0xf) << 24) +#define BPP32_ARGB8888 0x9 +#define CTRLDESCL0_5_SHADOW_LOAD_EN BIT(30) +#define CTRLDESCL0_5_EN BIT(31) + +struct lcdifv3_priv { + void __iomem *base; + struct clk pix_clk; + struct power_domain pd; + struct udevice *panel; + struct udevice *bridge; +}; + +static void lcdifv3_set_mode(struct lcdifv3_priv *priv, + struct display_timing *timings) +{ + u32 reg; + + writel(DISP_SIZE_DELTA_X(timings->hactive.typ) | + DISP_SIZE_DELTA_Y(timings->vactive.typ), + priv->base + LCDIFV3_DISP_SIZE); + + writel(HSYN_PARA_FP_H(timings->hfront_porch.typ) | + HSYN_PARA_BP_H(timings->hback_porch.typ), + priv->base + LCDIFV3_HSYN_PARA); + + writel(VSYN_PARA_BP_V(timings->vback_porch.typ) | + VSYN_PARA_FP_V(timings->vfront_porch.typ), + priv->base + LCDIFV3_VSYN_PARA); + + writel(VSYN_HSYN_PW_H(timings->hsync_len.typ) | + VSYN_HSYN_PW_V(timings->vsync_len.typ), + priv->base + LCDIFV3_VSYN_HSYN_WIDTH); + + writel(CTRLDESCL0_1_WIDTH(timings->hactive.typ) | + CTRLDESCL0_1_HEIGHT(timings->vactive.typ), + priv->base + LCDIFV3_CTRLDESCL0_1); + + if (timings->flags & DISPLAY_FLAGS_HSYNC_LOW) + writel(CTRL_INV_HS, priv->base + LCDIFV3_CTRL_SET); + else + writel(CTRL_INV_HS, priv->base + LCDIFV3_CTRL_CLR); + + if (timings->flags & DISPLAY_FLAGS_VSYNC_LOW) + writel(CTRL_INV_VS, priv->base + LCDIFV3_CTRL_SET); + else + writel(CTRL_INV_VS, priv->base + LCDIFV3_CTRL_CLR); + + if (timings->flags & DISPLAY_FLAGS_DE_LOW) + writel(CTRL_INV_DE, priv->base + LCDIFV3_CTRL_SET); + else + writel(CTRL_INV_DE, priv->base + LCDIFV3_CTRL_CLR); + + if (timings->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE) + writel(CTRL_INV_PXCK, priv->base + LCDIFV3_CTRL_SET); + else + writel(CTRL_INV_PXCK, priv->base + LCDIFV3_CTRL_CLR); + + writel(0, priv->base + LCDIFV3_DISP_PARA); + + reg = readl(priv->base + LCDIFV3_CTRLDESCL0_5); + reg &= ~(CTRLDESCL0_5_BPP(0xf) | CTRLDESCL0_5_YUV_FORMAT(0x3)); + reg |= CTRLDESCL0_5_BPP(BPP32_ARGB8888); + writel(reg, priv->base + LCDIFV3_CTRLDESCL0_5); +} + +static void lcdifv3_enable_controller(struct lcdifv3_priv *priv) +{ + u32 reg; + + reg = readl(priv->base + LCDIFV3_DISP_PARA); + reg |= DISP_PARA_DISP_ON; + writel(reg, priv->base + LCDIFV3_DISP_PARA); + + reg = readl(priv->base + LCDIFV3_CTRLDESCL0_5); + reg |= CTRLDESCL0_5_EN; + writel(reg, priv->base + LCDIFV3_CTRLDESCL0_5); +} + +static int lcdifv3_video_sync(struct udevice *dev) +{ + struct lcdifv3_priv *priv = dev_get_priv(dev); + u32 reg; + + reg = readl(priv->base + LCDIFV3_CTRLDESCL0_5); + reg |= CTRLDESCL0_5_SHADOW_LOAD_EN; + writel(reg, priv->base + LCDIFV3_CTRLDESCL0_5); + + return 0; +} + +static void lcdifv3_init(struct udevice *dev, struct display_timing *timings) +{ + struct video_uc_plat *plat = dev_get_uclass_plat(dev); + struct lcdifv3_priv *priv = dev_get_priv(dev); + + clk_set_rate(&priv->pix_clk, timings->pixelclock.typ); + + writel(CTRL_SW_RESET | CTRL_CLK_GATE, priv->base + LCDIFV3_CTRL_CLR); + udelay(10); + + lcdifv3_set_mode(priv, timings); + + writel(plat->base & 0xFFFFFFFF, priv->base + LCDIFV3_CTRLDESCL_LOW0_4); + writel(plat->base >> 32, priv->base + LCDIFV3_CTRLDESCL_HIGH0_4); + + writel(CTRLDESCL0_3_PITCH(timings->hactive.typ * 4), /* 32bpp */ + priv->base + LCDIFV3_CTRLDESCL0_3); + + lcdifv3_enable_controller(priv); +} + +static int lcdifv3_video_probe(struct udevice *dev) +{ + struct video_uc_plat *plat = dev_get_uclass_plat(dev); + struct video_priv *uc_priv = dev_get_uclass_priv(dev); + struct lcdifv3_priv *priv = dev_get_priv(dev); + struct clk axi_clk, disp_axi_clk; + struct display_timing timings; + u32 fb_start, fb_end; + int ret; + + ret = power_domain_get(dev, &priv->pd); + if (ret < 0) + return ret; + + ret = clk_get_by_name(dev, "pix", &priv->pix_clk); + if (ret < 0) + return ret; + + ret = clk_get_by_name(dev, "axi", &axi_clk); + if (ret < 0) + return ret; + + ret = clk_get_by_name(dev, "disp_axi", &disp_axi_clk); + if (ret < 0) + return ret; + + ret = power_domain_on(&priv->pd); + if (ret) + return ret; + + ret = clk_enable(&priv->pix_clk); + if (ret) + goto dis_pd; + + ret = clk_enable(&axi_clk); + if (ret) + goto dis_pix_clk; + + ret = clk_enable(&disp_axi_clk); + if (ret) + goto dis_axi_clk; + + priv->base = dev_remap_addr(dev); + if (!priv->base) { + ret = -EINVAL; + goto dis_clks; + } + + /* Attach bridge */ + ret = uclass_get_device_by_endpoint(UCLASS_VIDEO_BRIDGE, dev, + 0, &priv->bridge); + if (ret) + goto dis_clks; + + ret = video_bridge_attach(priv->bridge); + if (ret) + goto dis_clks; + + ret = video_bridge_set_backlight(priv->bridge, 80); + if (ret) + goto dis_clks; + + /* Attach panels */ + ret = uclass_get_device_by_endpoint(UCLASS_PANEL, priv->bridge, + 1, &priv->panel); + if (ret) { + ret = uclass_get_device_by_endpoint(UCLASS_PANEL, priv->bridge, + 2, &priv->panel); + if (ret) + goto dis_clks; + } + + ret = panel_get_display_timing(priv->panel, &timings); + if (ret) { + ret = ofnode_decode_display_timing(dev_ofnode(priv->panel), + 0, &timings); + if (ret) { + printf("Cannot decode panel timings (%d)\n", ret); + goto dis_clks; + } + } + + lcdifv3_init(dev, &timings); + + /* Only support 32bpp for now */ + uc_priv->bpix = VIDEO_BPP32; + uc_priv->xsize = timings.hactive.typ; + uc_priv->ysize = timings.vactive.typ; + + /* Enable dcache for the frame buffer */ + fb_start = plat->base & ~(MMU_SECTION_SIZE - 1); + fb_end = ALIGN(plat->base + plat->size, 1 << MMU_SECTION_SHIFT); + mmu_set_region_dcache_behaviour(fb_start, fb_end - fb_start, + DCACHE_WRITEBACK); + video_set_flush_dcache(dev, true); + + return 0; + +dis_clks: + clk_disable(&disp_axi_clk); +dis_axi_clk: + clk_disable(&axi_clk); +dis_pix_clk: + clk_disable(&priv->pix_clk); +dis_pd: + power_domain_off(&priv->pd); + + return ret; +} + +static int lcdifv3_video_bind(struct udevice *dev) +{ + struct video_uc_plat *plat = dev_get_uclass_plat(dev); + + /* Max size supported by LCDIF */ + plat->size = 1920 * 1080 * VNBYTES(VIDEO_BPP32); + + return 0; +} + +static const struct udevice_id lcdifv3_video_ids[] = { + { .compatible = "fsl,imx8mp-lcdif" }, + { } +}; + +static struct video_ops lcdifv3_video_ops = { + .video_sync = lcdifv3_video_sync, +}; + +U_BOOT_DRIVER(lcdifv3_video) = { + .name = "lcdif", + .id = UCLASS_VIDEO, + .of_match = lcdifv3_video_ids, + .bind = lcdifv3_video_bind, + .ops = &lcdifv3_video_ops, + .probe = lcdifv3_video_probe, + .priv_auto = sizeof(struct lcdifv3_priv), + .flags = DM_FLAG_PRE_RELOC | DM_FLAG_OS_PREPARE, +};
There is now support for the display pipeline using one LCD interface, the LDB bridge and of course the related media power domain controls.
This is untested as I do not have the EVK but should be enough to use the display, using bmp, lcdputs and co. For console-like commands, it might be also needed to enable the VIDEO_CONSOLE explicitly using:
setenv stdout serial,vidconsole0
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- configs/imx8mp_evk_defconfig | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/configs/imx8mp_evk_defconfig b/configs/imx8mp_evk_defconfig index 5369f8b84a4172434dcef142c854d06e3437caf1..a3cdf0f3cb237d7b555a2dca741deee21b771e8c 100644 --- a/configs/imx8mp_evk_defconfig +++ b/configs/imx8mp_evk_defconfig @@ -31,6 +31,7 @@ CONFIG_OF_SYSTEM_SETUP=y CONFIG_DEFAULT_FDT_FILE="imx8mp-evk.dtb" CONFIG_SYS_CBSIZE=2048 CONFIG_SYS_PBSIZE=2074 +# CONFIG_CONSOLE_MUX is not set CONFIG_BOARD_LATE_INIT=y CONFIG_SPL_MAX_SIZE=0x26000 CONFIG_SPL_BOARD_INIT=y @@ -60,6 +61,7 @@ CONFIG_CMD_USB=y CONFIG_CMD_USB_SDP=y CONFIG_CMD_USB_MASS_STORAGE=y CONFIG_CMD_CACHE=y +# CONFIG_CMD_CLS is not set CONFIG_CMD_REGULATOR=y CONFIG_CMD_EXT4_WRITE=y CONFIG_OF_CONTROL=y @@ -112,6 +114,7 @@ CONFIG_PINCTRL_IMX8M=y CONFIG_POWER_DOMAIN=y CONFIG_IMX8M_POWER_DOMAIN=y CONFIG_IMX8MP_HSIOMIX_BLKCTRL=y +CONFIG_IMX8MP_MEDIAMIX_BLKCTRL=y CONFIG_DM_PMIC=y CONFIG_DM_PMIC_PCA9450=y CONFIG_SPL_DM_PMIC_PCA9450=y @@ -139,4 +142,8 @@ CONFIG_USB_GADGET=y CONFIG_USB_GADGET_MANUFACTURER="FSL" CONFIG_USB_GADGET_VENDOR_NUM=0x0525 CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5 +CONFIG_VIDEO=y +CONFIG_VIDEO_BRIDGE=y +CONFIG_IMX_LDB=y +CONFIG_IMX_LCDIF=y CONFIG_IMX_WATCHDOG=y
Hi Peng,
On Fri, Jan 10, 2025 at 3:43 PM Miquel Raynal miquel.raynal@bootlin.com wrote:
There is now support for the display pipeline using one LCD interface, the LDB bridge and of course the related media power domain controls.
This is untested as I do not have the EVK but should be enough to use the display, using bmp, lcdputs and co. For console-like commands, it might be also needed to enable the VIDEO_CONSOLE explicitly using:
setenv stdout serial,vidconsole0
I have access to an imx8mp-evk but don't have an LVDS panel to test.
Could you please test this series on an imx8mp-evk + LVDS setup?
Thanks
Hello Fabio,
On 10/01/2025 at 15:53:35 -03, Fabio Estevam festevam@gmail.com wrote:
Hi Peng,
On Fri, Jan 10, 2025 at 3:43 PM Miquel Raynal miquel.raynal@bootlin.com wrote:
There is now support for the display pipeline using one LCD interface, the LDB bridge and of course the related media power domain controls.
This is untested as I do not have the EVK but should be enough to use the display, using bmp, lcdputs and co. For console-like commands, it might be also needed to enable the VIDEO_CONSOLE explicitly using:
setenv stdout serial,vidconsole0I have access to an imx8mp-evk but don't have an LVDS panel to test.
Could you please test this series on an imx8mp-evk + LVDS setup?
I only worked on a custom design based on a i.MX8MP. We do have an EVK, but it has no display. I was asked to add a user, but in practice I cannot test that. I can enable the ldb in the EVK DTS or drop that patch.
Thanks, Miquèl
Hi Miquel,
On Sat, Jan 11, 2025 at 6:51 AM Miquel Raynal miquel.raynal@bootlin.com wrote:
I only worked on a custom design based on a i.MX8MP. We do have an EVK, but it has no display. I was asked to add a user, but in practice I cannot test that. I can enable the ldb in the EVK DTS or drop that patch.
I wanted to have an in-tree user for the i.MX8MP LDB, but if you cannot test, I suggest dropping the imx8mp-evk patch from v4.
Hopefully, someone can enable LDB support for some other imx8mp-based board.
Besides that, there are some comments from Simon on the patch 05/13 that need to be addressed in v4.
I hope v4 can be merged.
On 16/01/2025 at 13:13:53 -03, Fabio Estevam festevam@gmail.com wrote:
Hi Miquel,
On Sat, Jan 11, 2025 at 6:51 AM Miquel Raynal miquel.raynal@bootlin.com wrote:
I only worked on a custom design based on a i.MX8MP. We do have an EVK, but it has no display. I was asked to add a user, but in practice I cannot test that. I can enable the ldb in the EVK DTS or drop that patch.
I wanted to have an in-tree user for the i.MX8MP LDB, but if you cannot test, I suggest dropping the imx8mp-evk patch from v4.
Hopefully, someone can enable LDB support for some other imx8mp-based board.
Ok.
Besides that, there are some comments from Simon on the patch 05/13 that need to be addressed in v4.
I hope v4 can be merged.
Yes, I need to get into this again because he asked something similar in response to one of my previous versions, and I disagreed. I will reconsider his requests. Hopefully we'll settle at some point.
Thanks, Miquèl
On Fri, Jan 10, 2025 at 3:43 PM Miquel Raynal miquel.raynal@bootlin.com wrote:
There is now support for the display pipeline using one LCD interface, the LDB bridge and of course the related media power domain controls.
This is untested as I do not have the EVK but should be enough to use the display, using bmp, lcdputs and co. For console-like commands, it
Hmm, this cannot work. The upstream imx8mp-evk.dts does not have the LDB enabled.
might be also needed to enable the VIDEO_CONSOLE explicitly using:
setenv stdout serial,vidconsole0Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com
configs/imx8mp_evk_defconfig | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/configs/imx8mp_evk_defconfig b/configs/imx8mp_evk_defconfig index 5369f8b84a4172434dcef142c854d06e3437caf1..a3cdf0f3cb237d7b555a2dca741deee21b771e8c 100644 --- a/configs/imx8mp_evk_defconfig +++ b/configs/imx8mp_evk_defconfig @@ -31,6 +31,7 @@ CONFIG_OF_SYSTEM_SETUP=y CONFIG_DEFAULT_FDT_FILE="imx8mp-evk.dtb" CONFIG_SYS_CBSIZE=2048 CONFIG_SYS_PBSIZE=2074 +# CONFIG_CONSOLE_MUX is not set CONFIG_BOARD_LATE_INIT=y CONFIG_SPL_MAX_SIZE=0x26000 CONFIG_SPL_BOARD_INIT=y @@ -60,6 +61,7 @@ CONFIG_CMD_USB=y CONFIG_CMD_USB_SDP=y CONFIG_CMD_USB_MASS_STORAGE=y CONFIG_CMD_CACHE=y +# CONFIG_CMD_CLS is not set CONFIG_CMD_REGULATOR=y CONFIG_CMD_EXT4_WRITE=y CONFIG_OF_CONTROL=y @@ -112,6 +114,7 @@ CONFIG_PINCTRL_IMX8M=y CONFIG_POWER_DOMAIN=y CONFIG_IMX8M_POWER_DOMAIN=y CONFIG_IMX8MP_HSIOMIX_BLKCTRL=y +CONFIG_IMX8MP_MEDIAMIX_BLKCTRL=y CONFIG_DM_PMIC=y CONFIG_DM_PMIC_PCA9450=y CONFIG_SPL_DM_PMIC_PCA9450=y @@ -139,4 +142,8 @@ CONFIG_USB_GADGET=y CONFIG_USB_GADGET_MANUFACTURER="FSL" CONFIG_USB_GADGET_VENDOR_NUM=0x0525 CONFIG_USB_GADGET_PRODUCT_NUM=0xa4a5 +CONFIG_VIDEO=y +CONFIG_VIDEO_BRIDGE=y +CONFIG_IMX_LDB=y +CONFIG_IMX_LCDIF=y CONFIG_IMX_WATCHDOG=y
-- 2.47.0
participants (3)
-
Fabio Estevam -
Miquel Raynal -
Simon Glass