| Message ID | 20210910202640.980366-1-l.stach@pengutronix.de |
|---|---|
| Headers | show |
| Series | i.MX8MM GPC improvements and BLK_CTRL driver | expand |
Le 10/09/2021 à 22:26, Lucas Stach a écrit : > This adds a driver for the blk-ctrl blocks found in the i.MX8M* line of > SoCs. The blk-ctrl is a top-level peripheral located in the various *MIX > power domains and interacts with the GPC power controller to provide the > peripherals in the power domain access to the NoC and ensures that those > peripherals are properly reset when their respective power domain is > brought back to life. > > Software needs to do different things to make the bus handshake happen > after the GPC *MIX domain is powered up and before it is powered down. > As the requirements are quite different between the various blk-ctrls > there is a callback function provided to hook in the proper sequence. > > The peripheral domains are quite uniform, they handle the soft clock > enables and resets in the blk-ctrl address space and sequencing with the > upstream GPC power domains. Hi Lucas, I have tried to use your patches for IMX8MQ but it seems that the hardware have different architecture. On IMX8MQ there is only one VPU domain for G1 and G2 and that doesn't match with your implementation where it is needed to have "bus" and devices power domain. From what I experiment in current IMX8MQ implementation of blk-ctrl (inside VPU driver) enabling the 3 clocks (bus, G1, G2) is needed to reset the VPUs. Do you think you can update your design to take care of these hardware variations ? Regards, Benjamin > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > Acked-by: Frieder Schrempf <frieder.schrempf@kontron.de> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > This commit includes the full code to drive the VPUMIX domain on the > i.MX8MM, as the skeleton driver would probably be harder to review > without the context provided by one blk-ctrl implementation. Other > blk-ctrl implementations will follow, based on this overall structure. > > v4: > - fix commit message typos > - fix superfluous whitespace > - constify clk_names more > --- > drivers/soc/imx/Makefile | 1 + > drivers/soc/imx/imx8m-blk-ctrl.c | 453 +++++++++++++++++++++++++++++++ > 2 files changed, 454 insertions(+) > create mode 100644 drivers/soc/imx/imx8m-blk-ctrl.c > > diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile > index 078dc918f4f3..8a707077914c 100644 > --- a/drivers/soc/imx/Makefile > +++ b/drivers/soc/imx/Makefile > @@ -5,3 +5,4 @@ endif > obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o > obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o > obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o > +obj-$(CONFIG_SOC_IMX8M) += imx8m-blk-ctrl.o > diff --git a/drivers/soc/imx/imx8m-blk-ctrl.c b/drivers/soc/imx/imx8m-blk-ctrl.c > new file mode 100644 > index 000000000000..f2d74669d683 > --- /dev/null > +++ b/drivers/soc/imx/imx8m-blk-ctrl.c > @@ -0,0 +1,453 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +/* > + * Copyright 2021 Pengutronix, Lucas Stach <kernel@pengutronix.de> > + */ > + > +#include <linux/device.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pm_domain.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <linux/clk.h> > + > +#include <dt-bindings/power/imx8mm-power.h> > + > +#define BLK_SFT_RSTN 0x0 > +#define BLK_CLK_EN 0x4 > + > +struct imx8m_blk_ctrl_domain; > + > +struct imx8m_blk_ctrl { > + struct device *dev; > + struct notifier_block power_nb; > + struct device *bus_power_dev; > + struct regmap *regmap; > + struct imx8m_blk_ctrl_domain *domains; > + struct genpd_onecell_data onecell_data; > +}; > + > +struct imx8m_blk_ctrl_domain_data { > + const char *name; > + const char * const *clk_names; > + int num_clks; > + const char *gpc_name; > + u32 rst_mask; > + u32 clk_mask; > +}; > + > +#define DOMAIN_MAX_CLKS 3 > + > +struct imx8m_blk_ctrl_domain { > + struct generic_pm_domain genpd; > + const struct imx8m_blk_ctrl_domain_data *data; > + struct clk_bulk_data clks[DOMAIN_MAX_CLKS]; > + struct device *power_dev; > + struct imx8m_blk_ctrl *bc; > +}; > + > +struct imx8m_blk_ctrl_data { > + int max_reg; > + notifier_fn_t power_notifier_fn; > + const struct imx8m_blk_ctrl_domain_data *domains; > + int num_domains; > +}; > + > +static inline struct imx8m_blk_ctrl_domain * > +to_imx8m_blk_ctrl_domain(struct generic_pm_domain *genpd) > +{ > + return container_of(genpd, struct imx8m_blk_ctrl_domain, genpd); > +} > + > +static int imx8m_blk_ctrl_power_on(struct generic_pm_domain *genpd) > +{ > + struct imx8m_blk_ctrl_domain *domain = to_imx8m_blk_ctrl_domain(genpd); > + const struct imx8m_blk_ctrl_domain_data *data = domain->data; > + struct imx8m_blk_ctrl *bc = domain->bc; > + int ret; > + > + /* make sure bus domain is awake */ > + ret = pm_runtime_get_sync(bc->bus_power_dev); > + if (ret < 0) { > + pm_runtime_put_noidle(bc->bus_power_dev); > + dev_err(bc->dev, "failed to power up bus domain\n"); > + return ret; > + } > + > + /* put devices into reset */ > + regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask); > + > + /* enable upstream and blk-ctrl clocks to allow reset to propagate */ > + ret = clk_bulk_prepare_enable(data->num_clks, domain->clks); Maybe the clocks should be enabled before clear BLK_SFT_RSTN ? > + if (ret) { > + dev_err(bc->dev, "failed to enable clocks\n"); > + goto bus_put; > + } > + regmap_set_bits(bc->regmap, BLK_CLK_EN, data->clk_mask); > + > + /* power up upstream GPC domain */ > + ret = pm_runtime_get_sync(domain->power_dev); > + if (ret < 0) { > + dev_err(bc->dev, "failed to power up peripheral domain\n"); > + goto clk_disable; > + } > + > + /* wait for reset to propagate */ > + udelay(5); > + > + /* release reset */ > + regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask); > + > + /* disable upstream clocks */ > + clk_bulk_disable_unprepare(data->num_clks, domain->clks); > + > + return 0; > + > +clk_disable: > + clk_bulk_disable_unprepare(data->num_clks, domain->clks); > +bus_put: > + pm_runtime_put(bc->bus_power_dev); > + > + return ret; > +} > + > +static int imx8m_blk_ctrl_power_off(struct generic_pm_domain *genpd) > +{ > + struct imx8m_blk_ctrl_domain *domain = to_imx8m_blk_ctrl_domain(genpd); > + const struct imx8m_blk_ctrl_domain_data *data = domain->data; > + struct imx8m_blk_ctrl *bc = domain->bc; > + > + /* put devices into reset and disable clocks */ > + regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask); > + regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask); > + > + /* power down upstream GPC domain */ > + pm_runtime_put(domain->power_dev); > + > + /* allow bus domain to suspend */ > + pm_runtime_put(bc->bus_power_dev); > + > + return 0; > +} > + > +static struct generic_pm_domain * > +imx8m_blk_ctrl_xlate(struct of_phandle_args *args, void *data) > +{ > + struct genpd_onecell_data *onecell_data = data; > + unsigned int index = args->args[0]; > + > + if (args->args_count != 1 || > + index > onecell_data->num_domains) > + return ERR_PTR(-EINVAL); > + > + return onecell_data->domains[index]; > +} > + > +static struct lock_class_key blk_ctrl_genpd_lock_class; > + > +static int imx8m_blk_ctrl_probe(struct platform_device *pdev) > +{ > + const struct imx8m_blk_ctrl_data *bc_data; > + struct device *dev = &pdev->dev; > + struct imx8m_blk_ctrl *bc; > + void __iomem *base; > + int i, ret; > + > + struct regmap_config regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + }; > + > + bc = devm_kzalloc(dev, sizeof(*bc), GFP_KERNEL); > + if (!bc) > + return -ENOMEM; > + > + bc->dev = dev; > + > + bc_data = of_device_get_match_data(dev); > + > + base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + regmap_config.max_register = bc_data->max_reg; > + bc->regmap = devm_regmap_init_mmio(dev, base, ®map_config); > + if (IS_ERR(bc->regmap)) > + return dev_err_probe(dev, PTR_ERR(bc->regmap), > + "failed to init regmap\n"); > + > + bc->domains = devm_kcalloc(dev, bc_data->num_domains, > + sizeof(struct imx8m_blk_ctrl_domain), > + GFP_KERNEL); > + if (!bc->domains) > + return -ENOMEM; > + > + bc->onecell_data.num_domains = bc_data->num_domains; > + bc->onecell_data.xlate = imx8m_blk_ctrl_xlate; > + bc->onecell_data.domains = > + devm_kcalloc(dev, bc_data->num_domains, > + sizeof(struct generic_pm_domain *), GFP_KERNEL); > + if (!bc->onecell_data.domains) > + return -ENOMEM; > + > + bc->bus_power_dev = genpd_dev_pm_attach_by_name(dev, "bus"); > + if (IS_ERR(bc->bus_power_dev)) > + return dev_err_probe(dev, PTR_ERR(bc->bus_power_dev), > + "failed to attach power domain\n"); > + > + for (i = 0; i < bc_data->num_domains; i++) { > + const struct imx8m_blk_ctrl_domain_data *data = &bc_data->domains[i]; > + struct imx8m_blk_ctrl_domain *domain = &bc->domains[i]; > + int j; > + > + domain->data = data; > + > + for (j = 0; j < data->num_clks; j++) > + domain->clks[j].id = data->clk_names[j]; > + > + ret = devm_clk_bulk_get(dev, data->num_clks, domain->clks); > + if (ret) { > + dev_err_probe(dev, ret, "failed to get clock\n"); > + goto cleanup_pds; > + } > + > + domain->power_dev = > + dev_pm_domain_attach_by_name(dev, data->gpc_name); > + if (IS_ERR(domain->power_dev)) { > + dev_err_probe(dev, PTR_ERR(domain->power_dev), > + "failed to attach power domain\n"); > + ret = PTR_ERR(domain->power_dev); > + goto cleanup_pds; > + } > + > + domain->genpd.name = data->name; > + domain->genpd.power_on = imx8m_blk_ctrl_power_on; > + domain->genpd.power_off = imx8m_blk_ctrl_power_off; > + domain->bc = bc; > + > + ret = pm_genpd_init(&domain->genpd, NULL, true); > + if (ret) { > + dev_err_probe(dev, ret, "failed to init power domain\n"); > + dev_pm_domain_detach(domain->power_dev, true); > + goto cleanup_pds; > + } > + > + /* > + * We use runtime PM to trigger power on/off of the upstream GPC > + * domain, as a strict hierarchical parent/child power domain > + * setup doesn't allow us to meet the sequencing requirements. > + * This means we have nested locking of genpd locks, without the > + * nesting being visible at the genpd level, so we need a > + * separate lock class to make lockdep aware of the fact that > + * this are separate domain locks that can be nested without a > + * self-deadlock. > + */ > + lockdep_set_class(&domain->genpd.mlock, > + &blk_ctrl_genpd_lock_class); > + > + bc->onecell_data.domains[i] = &domain->genpd; > + } > + > + ret = of_genpd_add_provider_onecell(dev->of_node, &bc->onecell_data); > + if (ret) { > + dev_err_probe(dev, ret, "failed to add power domain provider\n"); > + goto cleanup_pds; > + } > + > + bc->power_nb.notifier_call = bc_data->power_notifier_fn; > + ret = dev_pm_genpd_add_notifier(bc->bus_power_dev, &bc->power_nb); > + if (ret) { > + dev_err_probe(dev, ret, "failed to add power notifier\n"); > + goto cleanup_provider; > + } > + > + dev_set_drvdata(dev, bc); > + > + return 0; > + > +cleanup_provider: > + of_genpd_del_provider(dev->of_node); > +cleanup_pds: > + for (i--; i >= 0; i--) { > + pm_genpd_remove(&bc->domains[i].genpd); > + dev_pm_domain_detach(bc->domains[i].power_dev, true); > + } > + > + dev_pm_domain_detach(bc->bus_power_dev, true); > + > + return ret; > +} > + > +static int imx8m_blk_ctrl_remove(struct platform_device *pdev) > +{ > + struct imx8m_blk_ctrl *bc = dev_get_drvdata(&pdev->dev); > + int i; > + > + of_genpd_del_provider(pdev->dev.of_node); > + > + for (i = 0; bc->onecell_data.num_domains; i++) { > + struct imx8m_blk_ctrl_domain *domain = &bc->domains[i]; > + > + pm_genpd_remove(&domain->genpd); > + dev_pm_domain_detach(domain->power_dev, true); > + } > + > + dev_pm_genpd_remove_notifier(bc->bus_power_dev); > + > + dev_pm_domain_detach(bc->bus_power_dev, true); > + > + return 0; > +} > + > +#ifdef CONFIG_PM_SLEEP > +static int imx8m_blk_ctrl_suspend(struct device *dev) > +{ > + struct imx8m_blk_ctrl *bc = dev_get_drvdata(dev); > + int ret, i; > + > + /* > + * This may look strange, but is done so the generic PM_SLEEP code > + * can power down our domains and more importantly power them up again > + * after resume, without tripping over our usage of runtime PM to > + * control the upstream GPC domains. Things happen in the right order > + * in the system suspend/resume paths due to the device parent/child > + * hierarchy. > + */ > + ret = pm_runtime_get_sync(bc->bus_power_dev); > + if (ret < 0) { > + pm_runtime_put_noidle(bc->bus_power_dev); > + return ret; > + } > + > + for (i = 0; i < bc->onecell_data.num_domains; i++) { > + struct imx8m_blk_ctrl_domain *domain = &bc->domains[i]; > + > + ret = pm_runtime_get_sync(domain->power_dev); > + if (ret < 0) { > + pm_runtime_put_noidle(domain->power_dev); > + goto out_fail; > + } > + } > + > + return 0; > + > +out_fail: > + for (i--; i >= 0; i--) > + pm_runtime_put(bc->domains[i].power_dev); > + > + pm_runtime_put(bc->bus_power_dev); > + > + return ret; > +} > + > +static int imx8m_blk_ctrl_resume(struct device *dev) > +{ > + struct imx8m_blk_ctrl *bc = dev_get_drvdata(dev); > + int i; > + > + for (i = 0; i < bc->onecell_data.num_domains; i++) > + pm_runtime_put(bc->domains[i].power_dev); > + > + pm_runtime_put(bc->bus_power_dev); > + > + return 0; > +} > +#endif > + > +static const struct dev_pm_ops imx8m_blk_ctrl_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(imx8m_blk_ctrl_suspend, imx8m_blk_ctrl_resume) > +}; > + > +static int imx8mm_vpu_power_notifier(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct imx8m_blk_ctrl *bc = container_of(nb, struct imx8m_blk_ctrl, > + power_nb); > + > + if (action != GENPD_NOTIFY_ON && action != GENPD_NOTIFY_PRE_OFF) > + return NOTIFY_OK; > + > + /* > + * The ADB in the VPUMIX domain has no separate reset and clock > + * enable bits, but is ungated together with the VPU clocks. To > + * allow the handshake with the GPC to progress we put the VPUs > + * in reset and ungate the clocks. > + */ > + regmap_clear_bits(bc->regmap, BLK_SFT_RSTN, BIT(0) | BIT(1) | BIT(2)); > + regmap_set_bits(bc->regmap, BLK_CLK_EN, BIT(0) | BIT(1) | BIT(2)); > + > + if (action == GENPD_NOTIFY_ON) { > + /* > + * On power up we have no software backchannel to the GPC to > + * wait for the ADB handshake to happen, so we just delay for a > + * bit. On power down the GPC driver waits for the handshake. > + */ > + udelay(5); > + > + /* set "fuse" bits to enable the VPUs */ > + regmap_set_bits(bc->regmap, 0x8, 0xffffffff); > + regmap_set_bits(bc->regmap, 0xc, 0xffffffff); > + regmap_set_bits(bc->regmap, 0x10, 0xffffffff); > + regmap_set_bits(bc->regmap, 0x14, 0xffffffff); > + } > + > + return NOTIFY_OK; > +} > + > +static const struct imx8m_blk_ctrl_domain_data imx8m_vpu_blk_ctl_domain_data[] = { > + [IMX8MM_VPUBLK_PD_G1] = { > + .name = "vpublk-g1", > + .clk_names = (const char *[]){ "g1", }, > + .num_clks = 1, > + .gpc_name = "g1", > + .rst_mask = BIT(1), > + .clk_mask = BIT(1), > + }, > + [IMX8MM_VPUBLK_PD_G2] = { > + .name = "vpublk-g2", > + .clk_names = (const char *[]){ "g2", }, > + .num_clks = 1, > + .gpc_name = "g2", > + .rst_mask = BIT(0), > + .clk_mask = BIT(0), > + }, > + [IMX8MM_VPUBLK_PD_H1] = { > + .name = "vpublk-h1", > + .clk_names = (const char *[]){ "h1", }, > + .num_clks = 1, > + .gpc_name = "h1", > + .rst_mask = BIT(2), > + .clk_mask = BIT(2), > + }, > +}; > + > +static const struct imx8m_blk_ctrl_data imx8m_vpu_blk_ctl_dev_data = { > + .max_reg = 0x18, > + .power_notifier_fn = imx8mm_vpu_power_notifier, > + .domains = imx8m_vpu_blk_ctl_domain_data, > + .num_domains = ARRAY_SIZE(imx8m_vpu_blk_ctl_domain_data), > +}; > + > +static const struct of_device_id imx8m_blk_ctrl_of_match[] = { > + { > + .compatible = "fsl,imx8mm-vpu-blk-ctrl", > + .data = &imx8m_vpu_blk_ctl_dev_data > + }, { > + /* Sentinel */ > + } > +}; > +MODULE_DEVICE_TABLE(of, imx8m_blk_ctrl_of_match); > + > +static struct platform_driver imx8m_blk_ctrl_driver = { > + .probe = imx8m_blk_ctrl_probe, > + .remove = imx8m_blk_ctrl_remove, > + .driver = { > + .name = "imx8m-blk-ctrl", > + .pm = &imx8m_blk_ctrl_pm_ops, > + .of_match_table = imx8m_blk_ctrl_of_match, > + }, > +}; > +module_platform_driver(imx8m_blk_ctrl_driver);
On Fri, 10 Sep 2021 22:26:31 +0200, Lucas Stach wrote: > This adds the defines for the power domains provided by the VPU > blk-ctrl. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > include/dt-bindings/power/imx8mm-power.h | 4 ++++ > 1 file changed, 4 insertions(+) > Acked-by: Rob Herring <robh@kernel.org>
On Fri, 10 Sep 2021 22:26:33 +0200, Lucas Stach wrote: > This adds the DT binding for the i.MX8MM VPU blk-ctrl. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > .../soc/imx/fsl,imx8mm-disp-blk-ctrl.yaml | 94 +++++++++++++++++++ > 1 file changed, 94 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mm-disp-blk-ctrl.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Add the DT node for the GPC, including all the PGC power domains, > some of them are not fully functional yet, as they require interaction > with the blk-ctrls to properly power up/down the peripherals. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > 1 file changed, 107 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > index e7648c3b8390..3922f26f8fd4 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > @@ -7,6 +7,8 @@ > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/input/input.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/power/imx8mm-power.h> > +#include <dt-bindings/reset/imx8mq-reset.h> > #include <dt-bindings/thermal/thermal.h> > > #include "imx8mm-pinfunc.h" > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > #reset-cells = <1>; > }; > + > + gpc: gpc@303a0000 { > + compatible = "fsl,imx8mm-gpc"; > + reg = <0x303a0000 0x10000>; > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-parent = <&gic>; > + interrupt-controller; > + #interrupt-cells = <3>; > + > + pgc { > + #address-cells = <1>; > + #size-cells = <0>; > + > + pgc_hsiomix: power-domain@0 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > + }; > + > + pgc_pcie: power-domain@1 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > + power-domains = <&pgc_hsiomix>; > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > + }; > + > + pgc_otg1: power-domain@2 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > + power-domains = <&pgc_hsiomix>; > + }; > + > + pgc_otg2: power-domain@3 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > + power-domains = <&pgc_hsiomix>; > + }; > + > + pgc_gpumix: power-domain@4 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > + <&clk IMX8MM_CLK_GPU_AHB>; > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > + <&clk IMX8MM_CLK_GPU_AHB>; > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > + <&clk IMX8MM_SYS_PLL1_800M>; > + assigned-clock-rates = <800000000>, <400000000>; > + }; > + > + pgc_gpu: power-domain@5 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > + power-domains = <&pgc_gpumix>; > + }; > + > + pgc_vpumix: power-domain@6 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > + clocks = <&clk IMX8MM_CLK_VPU_DEC_ROOT>; > + assigned-clocks = <&clk IMX8MM_CLK_VPU_BUS>; > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>; > + resets = <&src IMX8MQ_RESET_VPU_RESET>; > + }; > + > + pgc_vpu_g1: power-domain@7 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > + }; > + > + pgc_vpu_g2: power-domain@8 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > + }; > + > + pgc_vpu_h1: power-domain@9 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > + }; > + > + pgc_dispmix: power-domain@10 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > + <&clk IMX8MM_CLK_DISP_APB>; > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > + <&clk IMX8MM_SYS_PLL1_800M>; > + assigned-clock-rates = <500000000>, <200000000>; > + }; > + > + pgc_mipi: power-domain@11 { > + #power-domain-cells = <0>; > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > + }; > + }; > + }; > }; > > aips2: bus@30400000 { > -- > 2.30.2 > Lucas, I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' series for imx8mm-venice* and imx8mn-venice* boards. Thank you for this work and I hope to see it merged soon! I have found that on the imx8mm-venice-gw7901 board which does not use MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this particular patch. If I comment out the pgc_mipi domain and subsequent disp_blk_ctrl node from a later patch it resolves the hang. Is this behavior expected and what would your recommendation be to work around it? Best regards, Tim
Hi Tim, Am Freitag, dem 01.10.2021 um 16:07 -0700 schrieb Tim Harvey: > On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Add the DT node for the GPC, including all the PGC power domains, > > some of them are not fully functional yet, as they require interaction > > with the blk-ctrls to properly power up/down the peripherals. > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > > 1 file changed, 107 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > index e7648c3b8390..3922f26f8fd4 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > @@ -7,6 +7,8 @@ > > #include <dt-bindings/gpio/gpio.h> > > #include <dt-bindings/input/input.h> > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > +#include <dt-bindings/power/imx8mm-power.h> > > +#include <dt-bindings/reset/imx8mq-reset.h> > > #include <dt-bindings/thermal/thermal.h> > > > > #include "imx8mm-pinfunc.h" > > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > #reset-cells = <1>; > > }; > > + > > + gpc: gpc@303a0000 { > > + compatible = "fsl,imx8mm-gpc"; > > + reg = <0x303a0000 0x10000>; > > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-parent = <&gic>; > > + interrupt-controller; > > + #interrupt-cells = <3>; > > + > > + pgc { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + pgc_hsiomix: power-domain@0 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > > + }; > > + > > + pgc_pcie: power-domain@1 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > > + power-domains = <&pgc_hsiomix>; > > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > > + }; > > + > > + pgc_otg1: power-domain@2 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > > + power-domains = <&pgc_hsiomix>; > > + }; > > + > > + pgc_otg2: power-domain@3 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > > + power-domains = <&pgc_hsiomix>; > > + }; > > + > > + pgc_gpumix: power-domain@4 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > + <&clk IMX8MM_CLK_GPU_AHB>; > > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > > + <&clk IMX8MM_CLK_GPU_AHB>; > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > > + <&clk IMX8MM_SYS_PLL1_800M>; > > + assigned-clock-rates = <800000000>, <400000000>; > > + }; > > + > > + pgc_gpu: power-domain@5 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > > + power-domains = <&pgc_gpumix>; > > + }; > > + > > + pgc_vpumix: power-domain@6 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > > + clocks = <&clk IMX8MM_CLK_VPU_DEC_ROOT>; > > + assigned-clocks = <&clk IMX8MM_CLK_VPU_BUS>; > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>; > > + resets = <&src IMX8MQ_RESET_VPU_RESET>; > > + }; > > + > > + pgc_vpu_g1: power-domain@7 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > > + }; > > + > > + pgc_vpu_g2: power-domain@8 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > > + }; > > + > > + pgc_vpu_h1: power-domain@9 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > > + }; > > + > > + pgc_dispmix: power-domain@10 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > > + <&clk IMX8MM_CLK_DISP_APB>; > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > > + <&clk IMX8MM_SYS_PLL1_800M>; > > + assigned-clock-rates = <500000000>, <200000000>; > > + }; > > + > > + pgc_mipi: power-domain@11 { > > + #power-domain-cells = <0>; > > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > > + }; > > + }; > > + }; > > }; > > > > aips2: bus@30400000 { > > -- > > 2.30.2 > > > > Lucas, > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > this work and I hope to see it merged soon! > > I have found that on the imx8mm-venice-gw7901 board which does not use > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > particular patch. If I comment out the pgc_mipi domain and subsequent > disp_blk_ctrl node from a later patch it resolves the hang. Is this > behavior expected and what would your recommendation be to work around > it? No, this isn't expected. If there are no active devices in the MIPI domain, the power domain should not be touched, as we treat all of them as disabled initially. If we don't touch the domain I would expect that the power supply not being present shouldn't be an issue. Can you check if something in your system causes this power domain to be powered up? Easiest way is probably to sprinkle a printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and imx_gpc_power_on(). Regards, Lucas
On Fri, Oct 1, 2021 at 4:20 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Hi Tim, > > Am Freitag, dem 01.10.2021 um 16:07 -0700 schrieb Tim Harvey: > > On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Add the DT node for the GPC, including all the PGC power domains, > > > some of them are not fully functional yet, as they require interaction > > > with the blk-ctrls to properly power up/down the peripherals. > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > --- > > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > > > 1 file changed, 107 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > index e7648c3b8390..3922f26f8fd4 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > @@ -7,6 +7,8 @@ > > > #include <dt-bindings/gpio/gpio.h> > > > #include <dt-bindings/input/input.h> > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > +#include <dt-bindings/power/imx8mm-power.h> > > > +#include <dt-bindings/reset/imx8mq-reset.h> > > > #include <dt-bindings/thermal/thermal.h> > > > > > > #include "imx8mm-pinfunc.h" > > > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > > #reset-cells = <1>; > > > }; > > > + > > > + gpc: gpc@303a0000 { > > > + compatible = "fsl,imx8mm-gpc"; > > > + reg = <0x303a0000 0x10000>; > > > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > > + interrupt-parent = <&gic>; > > > + interrupt-controller; > > > + #interrupt-cells = <3>; > > > + > > > + pgc { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + pgc_hsiomix: power-domain@0 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > > > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > > > + }; > > > + > > > + pgc_pcie: power-domain@1 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > > > + power-domains = <&pgc_hsiomix>; > > > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > > > + }; > > > + > > > + pgc_otg1: power-domain@2 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > > > + power-domains = <&pgc_hsiomix>; > > > + }; > > > + > > > + pgc_otg2: power-domain@3 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > > > + power-domains = <&pgc_hsiomix>; > > > + }; > > > + > > > + pgc_gpumix: power-domain@4 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > > > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > + assigned-clock-rates = <800000000>, <400000000>; > > > + }; > > > + > > > + pgc_gpu: power-domain@5 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > > > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > > > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > > > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > > > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > > > + power-domains = <&pgc_gpumix>; > > > + }; > > > + > > > + pgc_vpumix: power-domain@6 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > > > + clocks = <&clk IMX8MM_CLK_VPU_DEC_ROOT>; > > > + assigned-clocks = <&clk IMX8MM_CLK_VPU_BUS>; > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>; > > > + resets = <&src IMX8MQ_RESET_VPU_RESET>; > > > + }; > > > + > > > + pgc_vpu_g1: power-domain@7 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > > > + }; > > > + > > > + pgc_vpu_g2: power-domain@8 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > > > + }; > > > + > > > + pgc_vpu_h1: power-domain@9 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > > > + }; > > > + > > > + pgc_dispmix: power-domain@10 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > > > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > > > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > > > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > > > + <&clk IMX8MM_CLK_DISP_APB>; > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > + assigned-clock-rates = <500000000>, <200000000>; > > > + }; > > > + > > > + pgc_mipi: power-domain@11 { > > > + #power-domain-cells = <0>; > > > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > > > + }; > > > + }; > > > + }; > > > }; > > > > > > aips2: bus@30400000 { > > > -- > > > 2.30.2 > > > > > > > Lucas, > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > this work and I hope to see it merged soon! > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > particular patch. If I comment out the pgc_mipi domain and subsequent > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > behavior expected and what would your recommendation be to work around > > it? > > No, this isn't expected. If there are no active devices in the MIPI > domain, the power domain should not be touched, as we treat all of them > as disabled initially. If we don't touch the domain I would expect that > the power supply not being present shouldn't be an issue. > > Can you check if something in your system causes this power domain to > be powered up? Easiest way is probably to sprinkle a > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > imx_gpc_power_on(). > Lucas, Here's what I see before I hang (debug print on both power on/off followed by a msleep(1000) to make sure I see it before I hang): [ 0.518319] imx_pgc_power_up hsiomix [ 0.624031] imx_pgc_power_down hsiomix [ 0.731879] imx_pgc_power_up hsiomix [ 0.839906] imx_pgc_power_down hsiomix [ 0.947875] imx_pgc_power_up hsiomix [ 1.055859] imx_pgc_power_down hsiomix [ 1.057296] imx_pgc_power_up gpumix [ 1.167884] imx_pgc_power_down gpumix [ 0.518513] imx_pgc_power_up hsiomix [ 0.519933] imx_pgc_power_up gpumix Tim
On Fri, Oct 1, 2021 at 5:06 PM Tim Harvey <tharvey@gateworks.com> wrote: > > On Fri, Oct 1, 2021 at 4:20 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Hi Tim, > > > > Am Freitag, dem 01.10.2021 um 16:07 -0700 schrieb Tim Harvey: > > > On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > Add the DT node for the GPC, including all the PGC power domains, > > > > some of them are not fully functional yet, as they require interaction > > > > with the blk-ctrls to properly power up/down the peripherals. > > > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > > --- > > > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > > > > 1 file changed, 107 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > index e7648c3b8390..3922f26f8fd4 100644 > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > @@ -7,6 +7,8 @@ > > > > #include <dt-bindings/gpio/gpio.h> > > > > #include <dt-bindings/input/input.h> > > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > +#include <dt-bindings/power/imx8mm-power.h> > > > > +#include <dt-bindings/reset/imx8mq-reset.h> > > > > #include <dt-bindings/thermal/thermal.h> > > > > > > > > #include "imx8mm-pinfunc.h" > > > > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > > > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > > > #reset-cells = <1>; > > > > }; > > > > + > > > > + gpc: gpc@303a0000 { > > > > + compatible = "fsl,imx8mm-gpc"; > > > > + reg = <0x303a0000 0x10000>; > > > > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > > > + interrupt-parent = <&gic>; > > > > + interrupt-controller; > > > > + #interrupt-cells = <3>; > > > > + > > > > + pgc { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + pgc_hsiomix: power-domain@0 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > > > > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > > > > + }; > > > > + > > > > + pgc_pcie: power-domain@1 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > > > > + power-domains = <&pgc_hsiomix>; > > > > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > > > > + }; > > > > + > > > > + pgc_otg1: power-domain@2 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > > > > + power-domains = <&pgc_hsiomix>; > > > > + }; > > > > + > > > > + pgc_otg2: power-domain@3 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > > > > + power-domains = <&pgc_hsiomix>; > > > > + }; > > > > + > > > > + pgc_gpumix: power-domain@4 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > > > > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > + assigned-clock-rates = <800000000>, <400000000>; > > > > + }; > > > > + > > > > + pgc_gpu: power-domain@5 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > > > > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > > > > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > > > > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > > > > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > > > > + power-domains = <&pgc_gpumix>; > > > > + }; > > > > + > > > > + pgc_vpumix: power-domain@6 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > > > > + clocks = <&clk IMX8MM_CLK_VPU_DEC_ROOT>; > > > > + assigned-clocks = <&clk IMX8MM_CLK_VPU_BUS>; > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>; > > > > + resets = <&src IMX8MQ_RESET_VPU_RESET>; > > > > + }; > > > > + > > > > + pgc_vpu_g1: power-domain@7 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > > > > + }; > > > > + > > > > + pgc_vpu_g2: power-domain@8 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > > > > + }; > > > > + > > > > + pgc_vpu_h1: power-domain@9 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > > > > + }; > > > > + > > > > + pgc_dispmix: power-domain@10 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > > > > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > > > > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > > > > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > > > > + <&clk IMX8MM_CLK_DISP_APB>; > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > + assigned-clock-rates = <500000000>, <200000000>; > > > > + }; > > > > + > > > > + pgc_mipi: power-domain@11 { > > > > + #power-domain-cells = <0>; > > > > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > > > > + }; > > > > + }; > > > > + }; > > > > }; > > > > > > > > aips2: bus@30400000 { > > > > -- > > > > 2.30.2 > > > > > > > > > > Lucas, > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > this work and I hope to see it merged soon! > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > behavior expected and what would your recommendation be to work around > > > it? > > > > No, this isn't expected. If there are no active devices in the MIPI > > domain, the power domain should not be touched, as we treat all of them > > as disabled initially. If we don't touch the domain I would expect that > > the power supply not being present shouldn't be an issue. > > > > Can you check if something in your system causes this power domain to > > be powered up? Easiest way is probably to sprinkle a > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > imx_gpc_power_on(). > > > > Lucas, > > Here's what I see before I hang (debug print on both power on/off > followed by a msleep(1000) to make sure I see it before I hang): > [ 0.518319] imx_pgc_power_up hsiomix > [ 0.624031] imx_pgc_power_down hsiomix > [ 0.731879] imx_pgc_power_up hsiomix > [ 0.839906] imx_pgc_power_down hsiomix > [ 0.947875] imx_pgc_power_up hsiomix > [ 1.055859] imx_pgc_power_down hsiomix > [ 1.057296] imx_pgc_power_up gpumix > [ 1.167884] imx_pgc_power_down gpumix > [ 0.518513] imx_pgc_power_up hsiomix > [ 0.519933] imx_pgc_power_up gpumix > The board also has IMX8MM VDD_GPU pins not connected so it makes sense that we hang here I suppose. Yet if I add the folloiwng to imx8mm-venice-gw7901.dts it still tries to enable them and hangs: &gpu_2d { status = "disabled"; }; &gpu_3d { status = "disabled"; }; &vpu_blk_ctrl { status = "disabled"; }; Tim
Am Freitag, dem 01.10.2021 um 17:15 -0700 schrieb Tim Harvey: > On Fri, Oct 1, 2021 at 5:06 PM Tim Harvey <tharvey@gateworks.com> wrote: > > > > On Fri, Oct 1, 2021 at 4:20 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Hi Tim, > > > > > > Am Freitag, dem 01.10.2021 um 16:07 -0700 schrieb Tim Harvey: > > > > On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > > > Add the DT node for the GPC, including all the PGC power domains, > > > > > some of them are not fully functional yet, as they require interaction > > > > > with the blk-ctrls to properly power up/down the peripherals. > > > > > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > > > --- > > > > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > > > > > 1 file changed, 107 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > index e7648c3b8390..3922f26f8fd4 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > @@ -7,6 +7,8 @@ > > > > > #include <dt-bindings/gpio/gpio.h> > > > > > #include <dt-bindings/input/input.h> > > > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > > +#include <dt-bindings/power/imx8mm-power.h> > > > > > +#include <dt-bindings/reset/imx8mq-reset.h> > > > > > #include <dt-bindings/thermal/thermal.h> > > > > > > > > > > #include "imx8mm-pinfunc.h" > > > > > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > > > > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > > > > #reset-cells = <1>; > > > > > }; > > > > > + > > > > > + gpc: gpc@303a0000 { > > > > > + compatible = "fsl,imx8mm-gpc"; > > > > > + reg = <0x303a0000 0x10000>; > > > > > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > > > > + interrupt-parent = <&gic>; > > > > > + interrupt-controller; > > > > > + #interrupt-cells = <3>; > > > > > + > > > > > + pgc { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + pgc_hsiomix: power-domain@0 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > > > > > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > > > > > + }; > > > > > + > > > > > + pgc_pcie: power-domain@1 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > > > > > + power-domains = <&pgc_hsiomix>; > > > > > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > > > > > + }; > > > > > + > > > > > + pgc_otg1: power-domain@2 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > > > > > + power-domains = <&pgc_hsiomix>; > > > > > + }; > > > > > + > > > > > + pgc_otg2: power-domain@3 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > > > > > + power-domains = <&pgc_hsiomix>; > > > > > + }; > > > > > + > > > > > + pgc_gpumix: power-domain@4 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > > > > > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > > + assigned-clock-rates = <800000000>, <400000000>; > > > > > + }; > > > > > + > > > > > + pgc_gpu: power-domain@5 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > > > > > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > > > > > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > > > > > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > > > > > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > > > > > + power-domains = <&pgc_gpumix>; > > > > > + }; > > > > > + > > > > > + pgc_vpumix: power-domain@6 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > > > > > + clocks = <&clk IMX8MM_CLK_VPU_DEC_ROOT>; > > > > > + assigned-clocks = <&clk IMX8MM_CLK_VPU_BUS>; > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>; > > > > > + resets = <&src IMX8MQ_RESET_VPU_RESET>; > > > > > + }; > > > > > + > > > > > + pgc_vpu_g1: power-domain@7 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > > > > > + }; > > > > > + > > > > > + pgc_vpu_g2: power-domain@8 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > > > > > + }; > > > > > + > > > > > + pgc_vpu_h1: power-domain@9 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > > > > > + }; > > > > > + > > > > > + pgc_dispmix: power-domain@10 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > > > > > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > > > > > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > > > > > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > > > > > + <&clk IMX8MM_CLK_DISP_APB>; > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > > + assigned-clock-rates = <500000000>, <200000000>; > > > > > + }; > > > > > + > > > > > + pgc_mipi: power-domain@11 { > > > > > + #power-domain-cells = <0>; > > > > > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > }; > > > > > > > > > > aips2: bus@30400000 { > > > > > -- > > > > > 2.30.2 > > > > > > > > > > > > > Lucas, > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > this work and I hope to see it merged soon! > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > behavior expected and what would your recommendation be to work around > > > > it? > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > domain, the power domain should not be touched, as we treat all of them > > > as disabled initially. If we don't touch the domain I would expect that > > > the power supply not being present shouldn't be an issue. > > > > > > Can you check if something in your system causes this power domain to > > > be powered up? Easiest way is probably to sprinkle a > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > imx_gpc_power_on(). > > > > > > > Lucas, > > > > Here's what I see before I hang (debug print on both power on/off > > followed by a msleep(1000) to make sure I see it before I hang): > > [ 0.518319] imx_pgc_power_up hsiomix > > [ 0.624031] imx_pgc_power_down hsiomix > > [ 0.731879] imx_pgc_power_up hsiomix > > [ 0.839906] imx_pgc_power_down hsiomix > > [ 0.947875] imx_pgc_power_up hsiomix > > [ 1.055859] imx_pgc_power_down hsiomix > > [ 1.057296] imx_pgc_power_up gpumix > > [ 1.167884] imx_pgc_power_down gpumix > > [ 0.518513] imx_pgc_power_up hsiomix > > [ 0.519933] imx_pgc_power_up gpumix > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > that we hang here I suppose. Yet if I add the folloiwng to > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > &gpu_2d { > status = "disabled"; > }; > > &gpu_3d { > status = "disabled"; > }; > > &vpu_blk_ctrl { > status = "disabled"; > }; The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the driver gets probed, so the driver core will power up the gpumix domain for a moment during kernel init. To avoid this you must at least set the status of the pgc_gpu node to disabled. Regards, Lucas
Hi Benjamin, Am Dienstag, dem 14.09.2021 um 17:46 +0200 schrieb Benjamin Gaignard: > Le 10/09/2021 à 22:26, Lucas Stach a écrit : > > This adds a driver for the blk-ctrl blocks found in the i.MX8M* line of > > SoCs. The blk-ctrl is a top-level peripheral located in the various *MIX > > power domains and interacts with the GPC power controller to provide the > > peripherals in the power domain access to the NoC and ensures that those > > peripherals are properly reset when their respective power domain is > > brought back to life. > > > > Software needs to do different things to make the bus handshake happen > > after the GPC *MIX domain is powered up and before it is powered down. > > As the requirements are quite different between the various blk-ctrls > > there is a callback function provided to hook in the proper sequence. > > > > The peripheral domains are quite uniform, they handle the soft clock > > enables and resets in the blk-ctrl address space and sequencing with the > > upstream GPC power domains. > > Hi Lucas, > > I have tried to use your patches for IMX8MQ but it seems that the hardware > have different architecture. > On IMX8MQ there is only one VPU domain for G1 and G2 and that doesn't match > with your implementation where it is needed to have "bus" and devices power domain. > From what I experiment in current IMX8MQ implementation of blk-ctrl (inside VPU driver) > enabling the 3 clocks (bus, G1, G2) is needed to reset the VPUs. > > Do you think you can update your design to take care of these hardware variations ? The clocking/reset of the blk-ctrl and ADB in the i.MX8MQ VPU power domain is really a bit strange, as the ADB reset is tied to the VPU resets and the clk-ctrl seem to require all 3 VPU clocks, instead of only the bus clock as in newer designs. However I was able to make it work with the existing blk-ctrl driver design. My current WIP patches (only tested with the G1 core so far) on top of the v5 of the series I just sent out can be found here: https://git.pengutronix.de/cgit/lst/linux/log/?h=imx8mq-vpu-blk-ctrl Hope this helps. Regards, Lucas
'On Fri, Oct 1, 2021 at 5:25 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Freitag, dem 01.10.2021 um 17:15 -0700 schrieb Tim Harvey: > > On Fri, Oct 1, 2021 at 5:06 PM Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > On Fri, Oct 1, 2021 at 4:20 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > Hi Tim, > > > > > > > > Am Freitag, dem 01.10.2021 um 16:07 -0700 schrieb Tim Harvey: > > > > > On Fri, Sep 10, 2021 at 1:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > > > > > Add the DT node for the GPC, including all the PGC power domains, > > > > > > some of them are not fully functional yet, as they require interaction > > > > > > with the blk-ctrls to properly power up/down the peripherals. > > > > > > > > > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > > > > > --- > > > > > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 107 ++++++++++++++++++++++ > > > > > > 1 file changed, 107 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > > index e7648c3b8390..3922f26f8fd4 100644 > > > > > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > > > > @@ -7,6 +7,8 @@ > > > > > > #include <dt-bindings/gpio/gpio.h> > > > > > > #include <dt-bindings/input/input.h> > > > > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > > > > > +#include <dt-bindings/power/imx8mm-power.h> > > > > > > +#include <dt-bindings/reset/imx8mq-reset.h> > > > > > > #include <dt-bindings/thermal/thermal.h> > > > > > > > > > > > > #include "imx8mm-pinfunc.h" > > > > > > @@ -609,6 +611,111 @@ src: reset-controller@30390000 { > > > > > > interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>; > > > > > > #reset-cells = <1>; > > > > > > }; > > > > > > + > > > > > > + gpc: gpc@303a0000 { > > > > > > + compatible = "fsl,imx8mm-gpc"; > > > > > > + reg = <0x303a0000 0x10000>; > > > > > > + interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>; > > > > > > + interrupt-parent = <&gic>; > > > > > > + interrupt-controller; > > > > > > + #interrupt-cells = <3>; > > > > > > + > > > > > > + pgc { > > > > > > + #address-cells = <1>; > > > > > > + #size-cells = <0>; > > > > > > + > > > > > > + pgc_hsiomix: power-domain@0 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_HSIOMIX>; > > > > > > + clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > > > + assigned-clocks = <&clk IMX8MM_CLK_USB_BUS>; > > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_500M>; > > > > > > + }; > > > > > > + > > > > > > + pgc_pcie: power-domain@1 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_PCIE>; > > > > > > + power-domains = <&pgc_hsiomix>; > > > > > > + clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>; > > > > > > + }; > > > > > > + > > > > > > + pgc_otg1: power-domain@2 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG1>; > > > > > > + power-domains = <&pgc_hsiomix>; > > > > > > + }; > > > > > > + > > > > > > + pgc_otg2: power-domain@3 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_OTG2>; > > > > > > + power-domains = <&pgc_hsiomix>; > > > > > > + }; > > > > > > + > > > > > > + pgc_gpumix: power-domain@4 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_GPUMIX>; > > > > > > + clocks = <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > > > + assigned-clocks = <&clk IMX8MM_CLK_GPU_AXI>, > > > > > > + <&clk IMX8MM_CLK_GPU_AHB>; > > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>, > > > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > > > + assigned-clock-rates = <800000000>, <400000000>; > > > > > > + }; > > > > > > + > > > > > > + pgc_gpu: power-domain@5 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_GPU>; > > > > > > + clocks = <&clk IMX8MM_CLK_GPU_AHB>, > > > > > > + <&clk IMX8MM_CLK_GPU_BUS_ROOT>, > > > > > > + <&clk IMX8MM_CLK_GPU2D_ROOT>, > > > > > > + <&clk IMX8MM_CLK_GPU3D_ROOT>; > > > > > > + resets = <&src IMX8MQ_RESET_GPU_RESET>; > > > > > > + power-domains = <&pgc_gpumix>; > > > > > > + }; > > > > > > + > > > > > > + pgc_vpumix: power-domain@6 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUMIX>; > > > > > > + clocks = <&clk IMX8MM_CLK_VPU_DEC_ROOT>; > > > > > > + assigned-clocks = <&clk IMX8MM_CLK_VPU_BUS>; > > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_800M>; > > > > > > + resets = <&src IMX8MQ_RESET_VPU_RESET>; > > > > > > + }; > > > > > > + > > > > > > + pgc_vpu_g1: power-domain@7 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG1>; > > > > > > + }; > > > > > > + > > > > > > + pgc_vpu_g2: power-domain@8 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUG2>; > > > > > > + }; > > > > > > + > > > > > > + pgc_vpu_h1: power-domain@9 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_VPUH1>; > > > > > > + }; > > > > > > + > > > > > > + pgc_dispmix: power-domain@10 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_DISPMIX>; > > > > > > + clocks = <&clk IMX8MM_CLK_DISP_APB_ROOT>, > > > > > > + <&clk IMX8MM_CLK_DISP_AXI_ROOT>; > > > > > > + assigned-clocks = <&clk IMX8MM_CLK_DISP_AXI>, > > > > > > + <&clk IMX8MM_CLK_DISP_APB>; > > > > > > + assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_1000M>, > > > > > > + <&clk IMX8MM_SYS_PLL1_800M>; > > > > > > + assigned-clock-rates = <500000000>, <200000000>; > > > > > > + }; > > > > > > + > > > > > > + pgc_mipi: power-domain@11 { > > > > > > + #power-domain-cells = <0>; > > > > > > + reg = <IMX8MM_POWER_DOMAIN_MIPI>; > > > > > > + }; > > > > > > + }; > > > > > > + }; > > > > > > }; > > > > > > > > > > > > aips2: bus@30400000 { > > > > > > -- > > > > > > 2.30.2 > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > this work and I hope to see it merged soon! > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > behavior expected and what would your recommendation be to work around > > > > > it? > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > domain, the power domain should not be touched, as we treat all of them > > > > as disabled initially. If we don't touch the domain I would expect that > > > > the power supply not being present shouldn't be an issue. > > > > > > > > Can you check if something in your system causes this power domain to > > > > be powered up? Easiest way is probably to sprinkle a > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > imx_gpc_power_on(). > > > > > > > > > > Lucas, > > > > > > Here's what I see before I hang (debug print on both power on/off > > > followed by a msleep(1000) to make sure I see it before I hang): > > > [ 0.518319] imx_pgc_power_up hsiomix > > > [ 0.624031] imx_pgc_power_down hsiomix > > > [ 0.731879] imx_pgc_power_up hsiomix > > > [ 0.839906] imx_pgc_power_down hsiomix > > > [ 0.947875] imx_pgc_power_up hsiomix > > > [ 1.055859] imx_pgc_power_down hsiomix > > > [ 1.057296] imx_pgc_power_up gpumix > > > [ 1.167884] imx_pgc_power_down gpumix > > > [ 0.518513] imx_pgc_power_up hsiomix > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > that we hang here I suppose. Yet if I add the folloiwng to > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > &gpu_2d { > > status = "disabled"; > > }; > > > > &gpu_3d { > > status = "disabled"; > > }; > > > > &vpu_blk_ctrl { > > status = "disabled"; > > }; > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > driver gets probed, so the driver core will power up the gpumix domain > for a moment during kernel init. To avoid this you must at least set > the status of the pgc_gpu node to disabled. > I've tried that and it doesn't work either. &gpu_2d { status = "disabled"; }; &gpu_3d { status = "disabled"; }; &vpu_blk_ctrl { status = "disabled"; }; &pgc_gpumix { status = "disabled"; }; &pgc_gpu { status = "disabled"; }; The interesting thing is that the first patch that causes this is 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is before the addition of the other nodes. Therefore these are being enabled by default regardless of the above nodes being disabled (or not even added yet to imx8mm.dtsi). Tim
Hi Tim, Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > [...] > > > > > > Lucas, > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > behavior expected and what would your recommendation be to work around > > > > > > it? > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > Lucas, > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > that we hang here I suppose. Yet if I add the folloiwng to > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > &gpu_2d { > > > status = "disabled"; > > > }; > > > > > > &gpu_3d { > > > status = "disabled"; > > > }; > > > > > > &vpu_blk_ctrl { > > > status = "disabled"; > > > }; > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > driver gets probed, so the driver core will power up the gpumix domain > > for a moment during kernel init. To avoid this you must at least set > > the status of the pgc_gpu node to disabled. > > > > I've tried that and it doesn't work either. > > &gpu_2d { > status = "disabled"; > }; > > &gpu_3d { > status = "disabled"; > }; > > &vpu_blk_ctrl { > status = "disabled"; > }; > > &pgc_gpumix { > status = "disabled"; > }; > > &pgc_gpu { > status = "disabled"; > }; > > The interesting thing is that the first patch that causes this is > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > before the addition of the other nodes. Therefore these are being > enabled by default regardless of the above nodes being disabled (or > not even added yet to imx8mm.dtsi). My bad, I didn't think about the fact that the power domain devices are not instantiated via the common OF populate code. For the status property to work as expected the GPCv2 code needs to check this property. I've just sent a patch to make this happen. Can you give it another try with your DT changes and this patch applied? Regards, Lucas
On Sat, Oct 2, 2021 at 5:51 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > Hi Tim, > > Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > > > [...] > > > > > > > Lucas, > > > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > > behavior expected and what would your recommendation be to work around > > > > > > > it? > > > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > > that we hang here I suppose. Yet if I add the folloiwng to > > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > > &gpu_2d { > > > > status = "disabled"; > > > > }; > > > > > > > > &gpu_3d { > > > > status = "disabled"; > > > > }; > > > > > > > > &vpu_blk_ctrl { > > > > status = "disabled"; > > > > }; > > > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > > driver gets probed, so the driver core will power up the gpumix domain > > > for a moment during kernel init. To avoid this you must at least set > > > the status of the pgc_gpu node to disabled. > > > > > > > I've tried that and it doesn't work either. > > > > &gpu_2d { > > status = "disabled"; > > }; > > > > &gpu_3d { > > status = "disabled"; > > }; > > > > &vpu_blk_ctrl { > > status = "disabled"; > > }; > > > > &pgc_gpumix { > > status = "disabled"; > > }; > > > > &pgc_gpu { > > status = "disabled"; > > }; > > > > The interesting thing is that the first patch that causes this is > > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > > before the addition of the other nodes. Therefore these are being > > enabled by default regardless of the above nodes being disabled (or > > not even added yet to imx8mm.dtsi). > > My bad, I didn't think about the fact that the power domain devices are > not instantiated via the common OF populate code. For the status > property to work as expected the GPCv2 code needs to check this > property. I've just sent a patch to make this happen. Can you give it > another try with your DT changes and this patch applied? > Yes, this makes sense and resolves it with only disabling pgc_gpumix in my dts. I believe it is fine for me to not specifically disable gpu_2d and gpu_3d as they are not enabled by default in imx8mm.dtsi and I don't believe that will change and I don't 'currently' need to disable disp_blk_ctrl or pgc_mipi but wondering if I should as the MIPI rails are not hooked up either? (ie will something added in the future try to use them?) What needs to be done to get this series merged? I suppose it's too late to get it into 5.13? Best regards, Tim
Am Sonntag, dem 03.10.2021 um 10:17 -0700 schrieb Tim Harvey: > On Sat, Oct 2, 2021 at 5:51 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Hi Tim, > > > > Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > > > > > [...] > > > > > > > > Lucas, > > > > > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > > > behavior expected and what would your recommendation be to work around > > > > > > > > it? > > > > > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > > > that we hang here I suppose. Yet if I add the folloiwng to > > > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > > > &gpu_2d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &gpu_3d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &vpu_blk_ctrl { > > > > > status = "disabled"; > > > > > }; > > > > > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > > > driver gets probed, so the driver core will power up the gpumix domain > > > > for a moment during kernel init. To avoid this you must at least set > > > > the status of the pgc_gpu node to disabled. > > > > > > > > > > I've tried that and it doesn't work either. > > > > > > &gpu_2d { > > > status = "disabled"; > > > }; > > > > > > &gpu_3d { > > > status = "disabled"; > > > }; > > > > > > &vpu_blk_ctrl { > > > status = "disabled"; > > > }; > > > > > > &pgc_gpumix { > > > status = "disabled"; > > > }; > > > > > > &pgc_gpu { > > > status = "disabled"; > > > }; > > > > > > The interesting thing is that the first patch that causes this is > > > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > > > before the addition of the other nodes. Therefore these are being > > > enabled by default regardless of the above nodes being disabled (or > > > not even added yet to imx8mm.dtsi). > > > > My bad, I didn't think about the fact that the power domain devices are > > not instantiated via the common OF populate code. For the status > > property to work as expected the GPCv2 code needs to check this > > property. I've just sent a patch to make this happen. Can you give it > > another try with your DT changes and this patch applied? > > > > Yes, this makes sense and resolves it with only disabling pgc_gpumix in my dts. > > I believe it is fine for me to not specifically disable gpu_2d and > gpu_3d as they are not enabled by default in imx8mm.dtsi and I don't > believe that will change and I don't 'currently' need to disable > disp_blk_ctrl or pgc_mipi but wondering if I should as the MIPI rails > are not hooked up either? (ie will something added in the future try > to use them?) > I think it would be better to disable the GPU devices in your DT. They don't have a status property and thus are enabled by default. They won't probe without the power domain being available, but it will cause those devices to stay in probe deferred state forever, which may confuse some users. So I guess it would be good style to disable the GPU nodes. One consequence of disabling the MIPI power domain is that the disp- blk-ctrl driver won't probe anymore, as it needs all the referenced power domains to be available. Do you think this might be an issue? Is there a valid system configuration where you would use devices from from the display power domain, but not the MIPI domain? If that's a valid case we might need to make this configuration possible in the disp-blk-ctrl driver. > What needs to be done to get this series merged? I suppose it's too > late to get it into 5.13? 5.13? Way too late, even the 5.15 merge window is long closed. The series is almost completely reviewed and the DT bindings are also good to go. So if no one happens to run into any real showstopper, Shawn may be willing to pick up the series and stage it for the 5.16 merge window. Regards, Lucas
On Sun, Oct 3, 2021 at 12:44 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Sonntag, dem 03.10.2021 um 10:17 -0700 schrieb Tim Harvey: > > On Sat, Oct 2, 2021 at 5:51 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Hi Tim, > > > > > > Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > > > > > > > [...] > > > > > > > > > Lucas, > > > > > > > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > > > > behavior expected and what would your recommendation be to work around > > > > > > > > > it? > > > > > > > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > > > > that we hang here I suppose. Yet if I add the folloiwng to > > > > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > > > > &gpu_2d { > > > > > > status = "disabled"; > > > > > > }; > > > > > > > > > > > > &gpu_3d { > > > > > > status = "disabled"; > > > > > > }; > > > > > > > > > > > > &vpu_blk_ctrl { > > > > > > status = "disabled"; > > > > > > }; > > > > > > > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > > > > driver gets probed, so the driver core will power up the gpumix domain > > > > > for a moment during kernel init. To avoid this you must at least set > > > > > the status of the pgc_gpu node to disabled. > > > > > > > > > > > > > I've tried that and it doesn't work either. > > > > > > > > &gpu_2d { > > > > status = "disabled"; > > > > }; > > > > > > > > &gpu_3d { > > > > status = "disabled"; > > > > }; > > > > > > > > &vpu_blk_ctrl { > > > > status = "disabled"; > > > > }; > > > > > > > > &pgc_gpumix { > > > > status = "disabled"; > > > > }; > > > > > > > > &pgc_gpu { > > > > status = "disabled"; > > > > }; > > > > > > > > The interesting thing is that the first patch that causes this is > > > > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > > > > before the addition of the other nodes. Therefore these are being > > > > enabled by default regardless of the above nodes being disabled (or > > > > not even added yet to imx8mm.dtsi). > > > > > > My bad, I didn't think about the fact that the power domain devices are > > > not instantiated via the common OF populate code. For the status > > > property to work as expected the GPCv2 code needs to check this > > > property. I've just sent a patch to make this happen. Can you give it > > > another try with your DT changes and this patch applied? > > > > > > > Yes, this makes sense and resolves it with only disabling pgc_gpumix in my dts. > > > > I believe it is fine for me to not specifically disable gpu_2d and > > gpu_3d as they are not enabled by default in imx8mm.dtsi and I don't > > believe that will change and I don't 'currently' need to disable > > disp_blk_ctrl or pgc_mipi but wondering if I should as the MIPI rails > > are not hooked up either? (ie will something added in the future try > > to use them?) > > > I think it would be better to disable the GPU devices in your DT. They > don't have a status property and thus are enabled by default. They > won't probe without the power domain being available, but it will cause > those devices to stay in probe deferred state forever, which may > confuse some users. So I guess it would be good style to disable the > GPU nodes. > it still tries to power the pgc_gpumix domain if I 'only' disable gpu_2d and gpu_3d. I have to disable 'pgc_gpumix' specifically - is that what you expect here? > One consequence of disabling the MIPI power domain is that the disp- > blk-ctrl driver won't probe anymore, as it needs all the referenced > power domains to be available. Do you think this might be an issue? Is > there a valid system configuration where you would use devices from > from the display power domain, but not the MIPI domain? If that's a > valid case we might need to make this configuration possible in the > disp-blk-ctrl driver. In the case of imx8mm I don't see that you can have any display without MIPI however the imx8mp can have HDMI display without needing MIPI so in general that should be allowed. > > > What needs to be done to get this series merged? I suppose it's too > > late to get it into 5.13? > > 5.13? Way too late, even the 5.15 merge window is long closed. The > series is almost completely reviewed and the DT bindings are also good > to go. So if no one happens to run into any real showstopper, Shawn may > be willing to pick up the series and stage it for the 5.16 merge > window. oops... I meant 5.15 but yes, seems too late for that. We seem to have time to get this into 5.16 however. I'm not sure what Shawn's cut-off is there. Tim
Am Sonntag, dem 03.10.2021 um 14:21 -0700 schrieb Tim Harvey: > On Sun, Oct 3, 2021 at 12:44 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Am Sonntag, dem 03.10.2021 um 10:17 -0700 schrieb Tim Harvey: > > > On Sat, Oct 2, 2021 at 5:51 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > Hi Tim, > > > > > > > > Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > > > > > > > > > [...] > > > > > > > > > > Lucas, > > > > > > > > > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > > > > > behavior expected and what would your recommendation be to work around > > > > > > > > > > it? > > > > > > > > > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > > > > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > > > > > that we hang here I suppose. Yet if I add the folloiwng to > > > > > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > > > > > &gpu_2d { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > > > &gpu_3d { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > > > &vpu_blk_ctrl { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > > > > > driver gets probed, so the driver core will power up the gpumix domain > > > > > > for a moment during kernel init. To avoid this you must at least set > > > > > > the status of the pgc_gpu node to disabled. > > > > > > > > > > > > > > > > I've tried that and it doesn't work either. > > > > > > > > > > &gpu_2d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &gpu_3d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &vpu_blk_ctrl { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &pgc_gpumix { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &pgc_gpu { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > The interesting thing is that the first patch that causes this is > > > > > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > > > > > before the addition of the other nodes. Therefore these are being > > > > > enabled by default regardless of the above nodes being disabled (or > > > > > not even added yet to imx8mm.dtsi). > > > > > > > > My bad, I didn't think about the fact that the power domain devices are > > > > not instantiated via the common OF populate code. For the status > > > > property to work as expected the GPCv2 code needs to check this > > > > property. I've just sent a patch to make this happen. Can you give it > > > > another try with your DT changes and this patch applied? > > > > > > > > > > Yes, this makes sense and resolves it with only disabling pgc_gpumix in my dts. > > > > > > I believe it is fine for me to not specifically disable gpu_2d and > > > gpu_3d as they are not enabled by default in imx8mm.dtsi and I don't > > > believe that will change and I don't 'currently' need to disable > > > disp_blk_ctrl or pgc_mipi but wondering if I should as the MIPI rails > > > are not hooked up either? (ie will something added in the future try > > > to use them?) > > > > > I think it would be better to disable the GPU devices in your DT. They > > don't have a status property and thus are enabled by default. They > > won't probe without the power domain being available, but it will cause > > those devices to stay in probe deferred state forever, which may > > confuse some users. So I guess it would be good style to disable the > > GPU nodes. > > > > it still tries to power the pgc_gpumix domain if I 'only' disable > gpu_2d and gpu_3d. I have to disable 'pgc_gpumix' specifically - is > that what you expect here? > > > One consequence of disabling the MIPI power domain is that the disp- > > blk-ctrl driver won't probe anymore, as it needs all the referenced > > power domains to be available. Do you think this might be an issue? Is > > there a valid system configuration where you would use devices from > > from the display power domain, but not the MIPI domain? If that's a > > valid case we might need to make this configuration possible in the > > disp-blk-ctrl driver. > > In the case of imx8mm I don't see that you can have any display > without MIPI however the imx8mp can have HDMI display without needing > MIPI so in general that should be allowed. I was thinking about the camera input path, but this one also needs the MIPI domain. The camera interface is useless without the MIPI PHY to get data from. The i.MX8MP is a different topic, as the HDMI part is a separate blk-ctrl domain there. With that in mind, I think things are okay as they are implemented now, as I don't see any use-case where one would want to use devices from the DISP domain (CSI, LCDIF) without having the MIPI PHY parts available. Regards, Lucas > > > > > > What needs to be done to get this series merged? I suppose it's too > > > late to get it into 5.13? > > > > 5.13? Way too late, even the 5.15 merge window is long closed. The > > series is almost completely reviewed and the DT bindings are also good > > to go. So if no one happens to run into any real showstopper, Shawn may > > be willing to pick up the series and stage it for the 5.16 merge > > window. > > oops... I meant 5.15 but yes, seems too late for that. We seem to have > time to get this into 5.16 however. I'm not sure what Shawn's cut-off > is there. > > Tim
Am Sonntag, dem 03.10.2021 um 14:21 -0700 schrieb Tim Harvey: > On Sun, Oct 3, 2021 at 12:44 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Am Sonntag, dem 03.10.2021 um 10:17 -0700 schrieb Tim Harvey: > > > On Sat, Oct 2, 2021 at 5:51 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > Hi Tim, > > > > > > > > Am Freitag, dem 01.10.2021 um 19:43 -0700 schrieb Tim Harvey: > > > > > > > > > > > > > [...] > > > > > > > > > > Lucas, > > > > > > > > > > > > > > > > > > > > I've been using your 'i.MX8MM GPC improvements and BLK_CTRL driver' > > > > > > > > > > series for imx8mm-venice* and imx8mn-venice* boards. Thank you for > > > > > > > > > > this work and I hope to see it merged soon! > > > > > > > > > > > > > > > > > > > > I have found that on the imx8mm-venice-gw7901 board which does not use > > > > > > > > > > MIPI and thus does not connect VDD_MIPI_1P8, VDD_MIPI_1P2, > > > > > > > > > > VDD_MIPI_0P9, MIPI_VREG_CAP pins on the IMX8MM hangs with this > > > > > > > > > > particular patch. If I comment out the pgc_mipi domain and subsequent > > > > > > > > > > disp_blk_ctrl node from a later patch it resolves the hang. Is this > > > > > > > > > > behavior expected and what would your recommendation be to work around > > > > > > > > > > it? > > > > > > > > > > > > > > > > > > No, this isn't expected. If there are no active devices in the MIPI > > > > > > > > > domain, the power domain should not be touched, as we treat all of them > > > > > > > > > as disabled initially. If we don't touch the domain I would expect that > > > > > > > > > the power supply not being present shouldn't be an issue. > > > > > > > > > > > > > > > > > > Can you check if something in your system causes this power domain to > > > > > > > > > be powered up? Easiest way is probably to sprinkle a > > > > > > > > > printk("%s\n, genpd->name) in both imx8m_blk_ctrl_power_on() and > > > > > > > > > imx_gpc_power_on(). > > > > > > > > > > > > > > > > > > > > > > > > > Lucas, > > > > > > > > > > > > > > > > Here's what I see before I hang (debug print on both power on/off > > > > > > > > followed by a msleep(1000) to make sure I see it before I hang): > > > > > > > > [ 0.518319] imx_pgc_power_up hsiomix > > > > > > > > [ 0.624031] imx_pgc_power_down hsiomix > > > > > > > > [ 0.731879] imx_pgc_power_up hsiomix > > > > > > > > [ 0.839906] imx_pgc_power_down hsiomix > > > > > > > > [ 0.947875] imx_pgc_power_up hsiomix > > > > > > > > [ 1.055859] imx_pgc_power_down hsiomix > > > > > > > > [ 1.057296] imx_pgc_power_up gpumix > > > > > > > > [ 1.167884] imx_pgc_power_down gpumix > > > > > > > > [ 0.518513] imx_pgc_power_up hsiomix > > > > > > > > [ 0.519933] imx_pgc_power_up gpumix > > > > > > > > > > > > > > > > > > > > > > The board also has IMX8MM VDD_GPU pins not connected so it makes sense > > > > > > > that we hang here I suppose. Yet if I add the folloiwng to > > > > > > > imx8mm-venice-gw7901.dts it still tries to enable them and hangs: > > > > > > > &gpu_2d { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > > > &gpu_3d { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > > > &vpu_blk_ctrl { > > > > > > > status = "disabled"; > > > > > > > }; > > > > > > > > > > > > The pgc_gpu is a "active" consumer of the pgc_gpumix domain while the > > > > > > driver gets probed, so the driver core will power up the gpumix domain > > > > > > for a moment during kernel init. To avoid this you must at least set > > > > > > the status of the pgc_gpu node to disabled. > > > > > > > > > > > > > > > > I've tried that and it doesn't work either. > > > > > > > > > > &gpu_2d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &gpu_3d { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &vpu_blk_ctrl { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &pgc_gpumix { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > &pgc_gpu { > > > > > status = "disabled"; > > > > > }; > > > > > > > > > > The interesting thing is that the first patch that causes this is > > > > > 'arm64: dts: imx8mm: add GPC node' which just adds the gpc node and is > > > > > before the addition of the other nodes. Therefore these are being > > > > > enabled by default regardless of the above nodes being disabled (or > > > > > not even added yet to imx8mm.dtsi). > > > > > > > > My bad, I didn't think about the fact that the power domain devices are > > > > not instantiated via the common OF populate code. For the status > > > > property to work as expected the GPCv2 code needs to check this > > > > property. I've just sent a patch to make this happen. Can you give it > > > > another try with your DT changes and this patch applied? > > > > > > > > > > Yes, this makes sense and resolves it with only disabling pgc_gpumix in my dts. > > > > > > I believe it is fine for me to not specifically disable gpu_2d and > > > gpu_3d as they are not enabled by default in imx8mm.dtsi and I don't > > > believe that will change and I don't 'currently' need to disable > > > disp_blk_ctrl or pgc_mipi but wondering if I should as the MIPI rails > > > are not hooked up either? (ie will something added in the future try > > > to use them?) > > > > > I think it would be better to disable the GPU devices in your DT. They > > don't have a status property and thus are enabled by default. They > > won't probe without the power domain being available, but it will cause > > those devices to stay in probe deferred state forever, which may > > confuse some users. So I guess it would be good style to disable the > > GPU nodes. > > > > it still tries to power the pgc_gpumix domain if I 'only' disable > gpu_2d and gpu_3d. I have to disable 'pgc_gpumix' specifically - is > that what you expect here? Yes, that's expected. As I said earlier, the gpu domain is considered a active child device of the gpumix domain if it isn't disabled in DT, so the driver core will try to power up the gpumix domain before the gpu domain is probed. You must disable the power domains in DT to avoid them ever being touched on your board. You should disable the GPU devices depending on this power domain to avoid the GPU devices staying in probe deferred limbo forever. Regards, Lucas
Le 02/10/2021 à 03:07, Lucas Stach a écrit : > Hi Benjamin, > > Am Dienstag, dem 14.09.2021 um 17:46 +0200 schrieb Benjamin Gaignard: >> Le 10/09/2021 à 22:26, Lucas Stach a écrit : >>> This adds a driver for the blk-ctrl blocks found in the i.MX8M* line of >>> SoCs. The blk-ctrl is a top-level peripheral located in the various *MIX >>> power domains and interacts with the GPC power controller to provide the >>> peripherals in the power domain access to the NoC and ensures that those >>> peripherals are properly reset when their respective power domain is >>> brought back to life. >>> >>> Software needs to do different things to make the bus handshake happen >>> after the GPC *MIX domain is powered up and before it is powered down. >>> As the requirements are quite different between the various blk-ctrls >>> there is a callback function provided to hook in the proper sequence. >>> >>> The peripheral domains are quite uniform, they handle the soft clock >>> enables and resets in the blk-ctrl address space and sequencing with the >>> upstream GPC power domains. >> Hi Lucas, >> >> I have tried to use your patches for IMX8MQ but it seems that the hardware >> have different architecture. >> On IMX8MQ there is only one VPU domain for G1 and G2 and that doesn't match >> with your implementation where it is needed to have "bus" and devices power domain. >> From what I experiment in current IMX8MQ implementation of blk-ctrl (inside VPU driver) >> enabling the 3 clocks (bus, G1, G2) is needed to reset the VPUs. >> >> Do you think you can update your design to take care of these hardware variations ? > The clocking/reset of the blk-ctrl and ADB in the i.MX8MQ VPU power > domain is really a bit strange, as the ADB reset is tied to the VPU > resets and the clk-ctrl seem to require all 3 VPU clocks, instead of > only the bus clock as in newer designs. However I was able to make it > work with the existing blk-ctrl driver design. > > My current WIP patches (only tested with the G1 core so far) on top of > the v5 of the series I just sent out can be found here: > https://git.pengutronix.de/cgit/lst/linux/log/?h=imx8mq-vpu-blk-ctrl > > Hope this helps. Hi Lucas, I have been able to test your branch on my iMX8MQ. I confirm that G1 is working fine, I able to decode H264 files. I wasn't able to make G2 works, I think it is coming from the reset sequence done before each frame decoding in G2 driver. I have change imx8mq_runtime_resume() and imx8m_vpu_reset() to call pm_runtime_put() and pm_runtime_get() to perform a reset like. Without that G2 hangs when decoding the first frame. One G1 it seems that doing a reset before each frame decoding is not needed. On DT I had to assignee G1 and G2 on the both nodes to avoid a warning at probe time. assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, <&clk IMX8MQ_CLK_VPU_G2>, <&clk IMX8MQ_VPU_PLL_BYPASS>; assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>, <&clk IMX8MQ_VPU_PLL_OUT>, <&clk IMX8MQ_VPU_PLL>; assigned-clock-rates = <600000000>, <300000000>, <0>; I also set G2 clock at 300Mhz as specify in the TRM. Even with all this G2 doesn't fire interrupts. Benjamin > > Regards, > Lucas >
Hi Benjamin, Am Montag, dem 04.10.2021 um 16:27 +0200 schrieb Benjamin Gaignard: > Le 02/10/2021 à 03:07, Lucas Stach a écrit : > > Hi Benjamin, > > > > Am Dienstag, dem 14.09.2021 um 17:46 +0200 schrieb Benjamin Gaignard: > > > Le 10/09/2021 à 22:26, Lucas Stach a écrit : > > > > This adds a driver for the blk-ctrl blocks found in the i.MX8M* line of > > > > SoCs. The blk-ctrl is a top-level peripheral located in the various *MIX > > > > power domains and interacts with the GPC power controller to provide the > > > > peripherals in the power domain access to the NoC and ensures that those > > > > peripherals are properly reset when their respective power domain is > > > > brought back to life. > > > > > > > > Software needs to do different things to make the bus handshake happen > > > > after the GPC *MIX domain is powered up and before it is powered down. > > > > As the requirements are quite different between the various blk-ctrls > > > > there is a callback function provided to hook in the proper sequence. > > > > > > > > The peripheral domains are quite uniform, they handle the soft clock > > > > enables and resets in the blk-ctrl address space and sequencing with the > > > > upstream GPC power domains. > > > Hi Lucas, > > > > > > I have tried to use your patches for IMX8MQ but it seems that the hardware > > > have different architecture. > > > On IMX8MQ there is only one VPU domain for G1 and G2 and that doesn't match > > > with your implementation where it is needed to have "bus" and devices power domain. > > > From what I experiment in current IMX8MQ implementation of blk-ctrl (inside VPU driver) > > > enabling the 3 clocks (bus, G1, G2) is needed to reset the VPUs. > > > > > > Do you think you can update your design to take care of these hardware variations ? > > The clocking/reset of the blk-ctrl and ADB in the i.MX8MQ VPU power > > domain is really a bit strange, as the ADB reset is tied to the VPU > > resets and the clk-ctrl seem to require all 3 VPU clocks, instead of > > only the bus clock as in newer designs. However I was able to make it > > work with the existing blk-ctrl driver design. > > > > My current WIP patches (only tested with the G1 core so far) on top of > > the v5 of the series I just sent out can be found here: > > https://git.pengutronix.de/cgit/lst/linux/log/?h=imx8mq-vpu-blk-ctrl > > > > Hope this helps. > > Hi Lucas, > > I have been able to test your branch on my iMX8MQ. > I confirm that G1 is working fine, I able to decode H264 files. > > I wasn't able to make G2 works, I think it is coming from the reset sequence > done before each frame decoding in G2 driver. > I have change imx8mq_runtime_resume() and imx8m_vpu_reset() > to call pm_runtime_put() and pm_runtime_get() to perform a reset like. I think you need to use the _sync variants of those functions to make sure the domain is going through the reset state. However that seems like a pretty heavy-weight thing to do if the decoder really requires a reset before each frame. Excuse my ignorance about the G2 block, but this sounds like a quite odd requirement. Is this to work around some hardware erratum? If we really need to reset the G2 before each frame, I think it would be best to also expose a reset controller from the blk-ctrl driver, to allow the G2 driver to do a light-weight reset, instead of doing this runtime PM transition. Regards, Lucas > Without that G2 hangs when decoding the first frame. > > One G1 it seems that doing a reset before each frame decoding is not needed. > > On DT I had to assignee G1 and G2 on the both nodes to avoid a warning at probe time. > assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, > <&clk IMX8MQ_CLK_VPU_G2>, > <&clk IMX8MQ_VPU_PLL_BYPASS>; > assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>, > <&clk IMX8MQ_VPU_PLL_OUT>, > <&clk IMX8MQ_VPU_PLL>; > assigned-clock-rates = <600000000>, <300000000>, <0>; > > I also set G2 clock at 300Mhz as specify in the TRM. > Even with all this G2 doesn't fire interrupts. > > Benjamin > > > > > Regards, > > Lucas > >
Le 05/10/2021 à 12:03, Lucas Stach a écrit : > Hi Benjamin, > > Am Montag, dem 04.10.2021 um 16:27 +0200 schrieb Benjamin Gaignard: >> Le 02/10/2021 à 03:07, Lucas Stach a écrit : >>> Hi Benjamin, >>> >>> Am Dienstag, dem 14.09.2021 um 17:46 +0200 schrieb Benjamin Gaignard: >>>> Le 10/09/2021 à 22:26, Lucas Stach a écrit : >>>>> This adds a driver for the blk-ctrl blocks found in the i.MX8M* line of >>>>> SoCs. The blk-ctrl is a top-level peripheral located in the various *MIX >>>>> power domains and interacts with the GPC power controller to provide the >>>>> peripherals in the power domain access to the NoC and ensures that those >>>>> peripherals are properly reset when their respective power domain is >>>>> brought back to life. >>>>> >>>>> Software needs to do different things to make the bus handshake happen >>>>> after the GPC *MIX domain is powered up and before it is powered down. >>>>> As the requirements are quite different between the various blk-ctrls >>>>> there is a callback function provided to hook in the proper sequence. >>>>> >>>>> The peripheral domains are quite uniform, they handle the soft clock >>>>> enables and resets in the blk-ctrl address space and sequencing with the >>>>> upstream GPC power domains. >>>> Hi Lucas, >>>> >>>> I have tried to use your patches for IMX8MQ but it seems that the hardware >>>> have different architecture. >>>> On IMX8MQ there is only one VPU domain for G1 and G2 and that doesn't match >>>> with your implementation where it is needed to have "bus" and devices power domain. >>>> From what I experiment in current IMX8MQ implementation of blk-ctrl (inside VPU driver) >>>> enabling the 3 clocks (bus, G1, G2) is needed to reset the VPUs. >>>> >>>> Do you think you can update your design to take care of these hardware variations ? >>> The clocking/reset of the blk-ctrl and ADB in the i.MX8MQ VPU power >>> domain is really a bit strange, as the ADB reset is tied to the VPU >>> resets and the clk-ctrl seem to require all 3 VPU clocks, instead of >>> only the bus clock as in newer designs. However I was able to make it >>> work with the existing blk-ctrl driver design. >>> >>> My current WIP patches (only tested with the G1 core so far) on top of >>> the v5 of the series I just sent out can be found here: >>> https://git.pengutronix.de/cgit/lst/linux/log/?h=imx8mq-vpu-blk-ctrl >>> >>> Hope this helps. >> Hi Lucas, >> >> I have been able to test your branch on my iMX8MQ. >> I confirm that G1 is working fine, I able to decode H264 files. >> >> I wasn't able to make G2 works, I think it is coming from the reset sequence >> done before each frame decoding in G2 driver. >> I have change imx8mq_runtime_resume() and imx8m_vpu_reset() >> to call pm_runtime_put() and pm_runtime_get() to perform a reset like. > I think you need to use the _sync variants of those functions to make > sure the domain is going through the reset state. However that seems > like a pretty heavy-weight thing to do if the decoder really requires a > reset before each frame. Excuse my ignorance about the G2 block, but > this sounds like a quite odd requirement. Is this to work around some > hardware erratum? When using _sync variants kernel stop when probing G2. I don't have any documentation about the link between the control block and G2. I have done lot of tests and resetting the hardware block between each frame seems to be mandatory. That also the case in Hantro proprietary stack. > > If we really need to reset the G2 before each frame, I think it would > be best to also expose a reset controller from the blk-ctrl driver, to > allow the G2 driver to do a light-weight reset, instead of doing this > runtime PM transition. I do agree and I have already done a attempt in this way: https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=440031&state=%2A&archive=both but Philipp have fairly argue that clocks enabling is not the job of a reset driver. Thanks for the time you spend on this topic. Regards, Benjamin > > Regards, > Lucas > >> Without that G2 hangs when decoding the first frame. >> >> One G1 it seems that doing a reset before each frame decoding is not needed. >> >> On DT I had to assignee G1 and G2 on the both nodes to avoid a warning at probe time. >> assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>, >> <&clk IMX8MQ_CLK_VPU_G2>, >> <&clk IMX8MQ_VPU_PLL_BYPASS>; >> assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>, >> <&clk IMX8MQ_VPU_PLL_OUT>, >> <&clk IMX8MQ_VPU_PLL>; >> assigned-clock-rates = <600000000>, <300000000>, <0>; >> >> I also set G2 clock at 300Mhz as specify in the TRM. >> Even with all this G2 doesn't fire interrupts. >> >> Benjamin >> >>> Regards, >>> Lucas >>> >
Hi all, as it seems there is considerable interest in landing this despite some spurious issues still being present, here's a 4th revision of the series to not drop the ball. Even though I still did not have time to try to dig into the open issues reported by Frieder, I think there is a high chance that those are caused by something like a missed timing constraint and not by something that would invalidate the overall design of the GPC and BLK_CTRL interaction as laid out by this series. So I still hope that we can land this series and apply fixes as we find them. Regards, Lucas Frieder Schrempf (1): arm64: dts: imx8mm: Add GPU nodes for 2D and 3D core Lucas Stach (15): Revert "soc: imx: gpcv2: move reset assert after requesting domain power up" soc: imx: gpcv2: add lockdep annotation soc: imx: gpcv2: add domain option to keep domain clocks enabled soc: imx: gpcv2: keep i.MX8M* bus clocks enabled soc: imx: gpcv2: support system suspend/resume dt-bindings: soc: add binding for i.MX8MM VPU blk-ctrl dt-bindings: power: imx8mm: add defines for VPU blk-ctrl domains soc: imx: add i.MX8M blk-ctrl driver dt-bindings: soc: add binding for i.MX8MM DISP blk-ctrl dt-bindings: power: imx8mm: add defines for DISP blk-ctrl domains soc: imx: imx8m-blk-ctrl: add DISP blk-ctrl arm64: dts: imx8mm: add GPC node arm64: dts: imx8mm: put USB controllers into power-domains arm64: dts: imx8mm: add VPU blk-ctrl arm64: dts: imx8mm: add DISP blk-ctrl Marek Vasut (2): soc: imx: gpcv2: Turn domain->pgc into bitfield soc: imx: gpcv2: Set both GPC_PGC_nCTRL(GPU_2D|GPU_3D) for MX8MM GPU domain .../soc/imx/fsl,imx8mm-disp-blk-ctrl.yaml | 94 ++++ .../soc/imx/fsl,imx8mm-vpu-blk-ctrl.yaml | 76 +++ arch/arm64/boot/dts/freescale/imx8mm.dtsi | 180 ++++++ drivers/soc/imx/Makefile | 1 + drivers/soc/imx/gpcv2.c | 130 +++-- drivers/soc/imx/imx8m-blk-ctrl.c | 523 ++++++++++++++++++ include/dt-bindings/power/imx8mm-power.h | 9 + 7 files changed, 972 insertions(+), 41 deletions(-) create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mm-disp-blk-ctrl.yaml create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mm-vpu-blk-ctrl.yaml create mode 100644 drivers/soc/imx/imx8m-blk-ctrl.c