| Message ID | 20210910202640.980366-11-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| 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);
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
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 >>> >
Le 05/10/2021 à 14:36, Benjamin Gaignard a écrit : > > 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. Hi Lucas, I have done more tests with the following condition: - no implementation of pm_runtime_* in the IMX part of the driver (i.e. resume and reset functions are empty) => so power domain on/off are called only when the delay set for autosuspend expires not for all frames. I have added traces and the board in hanging after calling imx8mq_vpu_power_notifier() when going off. Before that I have the power on sequence, few interrupts from G2 and power off occur because of the delay. If I reset and enable G1 and G2 clocks in imx8mq_vpu_power_notifier() I can see a call to codec run() but that hang too. I have also try to enable all the clocks (bus, G1, G2) when using only G2 but the results are the same. I don't if that could help you. If you want me to do more tests please tell me. Regards, Benjamin > 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 >>>> >>
On Fri, Sep 10, 2021 at 3:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > 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. > Sorry to resurrect an old thread, but I'm running into issues enabling new peripherals, and I think it's related to this, but I am not certain. I have attempted to enable the various VPU's, and in doing so, I referenced the vpu_blk_ctrl for the respective power-domains reference [1]. The various VPU devices enumerate, and I can see the various VPU power domains turn on and off a boot. [ 1.968565] genpd genpd:0:38330000.blk-ctrl: adding to PM domain vpumix [ 1.981208] genpd genpd:1:38330000.blk-ctrl: adding to PM domain vpu-g1 [ 1.993838] genpd genpd:2:38330000.blk-ctrl: adding to PM domain vpu-g2 [ 2.006455] genpd genpd:3:38330000.blk-ctrl: adding to PM domain vpu-h1 [ 8.922661] hantro_vpu: module is from the staging directory, the quality is unknown, you have been warned. [ 9.008147] hantro_vpu: module is from the staging directory, the quality is unknown, you have been warned. [ 9.044041] hantro-vpu 38300000.video-codec: adding to PM domain vpublk-g1 [ 9.050959] hantro-vpu 38300000.video-codec: genpd_add_device() [ 9.063349] PM: vpumix: Power-on latency exceeded, new value 50875 ns [ 9.083437] PM: vpu-g1: Power-on latency exceeded, new value 26125 ns [ 9.104778] PM: vpublk-g1: Power-on latency exceeded, new value 47819250 ns [ 9.211988] hantro-vpu 38300000.video-codec: registered nxp,imx8mm-vpu-dec as /dev/video0 [ 9.259680] hantro-vpu 38300000.video-codec: genpd_runtime_resume() [ 9.297395] hantro-vpu 38300000.video-codec: resume latency exceeded, 2750 ns [ 9.307767] hantro-vpu 38310000.video-codec: adding to PM domain vpublk-g2 [ 9.316462] hantro-vpu 38310000.video-codec: genpd_add_device() [ 9.328807] PM: vpu-g2: Power-on latency exceeded, new value 26625 ns [ 9.342401] PM: vpublk-g2: Power-on latency exceeded, new value 19965125 ns [ 9.430254] hantro-vpu 38300000.video-codec: genpd_runtime_suspend() [ 9.436683] hantro-vpu 38300000.video-codec: suspend latency exceeded, 1625 ns [ 9.443984] PM: vpublk-g1: Power-off latency exceeded, new value 16625 ns [ 9.462361] hantro-vpu 38310000.video-codec: registered nxp,imx8mm-vpu-g2-dec as /dev/video2 [ 9.491848] PM: vpu-g1: Power-off latency exceeded, new value 17125 ns [ 9.506353] hantro-vpu 38310000.video-codec: genpd_runtime_resume() [ 9.512735] hantro-vpu 38310000.video-codec: resume latency exceeded, 2750 ns [ 9.520306] hantro-vpu 38320000.video-codec: adding to PM domain vpublk-h1 [ 9.527268] hantro-vpu 38320000.video-codec: genpd_add_device() [ 9.539632] PM: vpu-h1: Power-on latency exceeded, new value 27750 ns [ 9.553358] PM: vpublk-h1: Power-on latency exceeded, new value 20112125 ns [ 9.605800] hantro_vpu: module is from the staging directory, the quality is unknown, you have been warned. [ 9.642194] hantro-vpu 38310000.video-codec: genpd_runtime_suspend() [ 9.648634] hantro-vpu 38310000.video-codec: suspend latency exceeded, 1500 ns [ 9.655974] PM: vpublk-g2: Power-off latency exceeded, new value 12625 ns [ 9.703863] PM: vpu-g2: Power-off latency exceeded, new value 16750 ns [ 9.754527] hantro-vpu 38320000.video-codec: registered nxp,imx8mm-vpu-h1-enc as /dev/video3 [ 9.785424] hantro-vpu 38320000.video-codec: genpd_runtime_resume() [ 9.796034] hantro-vpu 38320000.video-codec: resume latency exceeded, 2000 ns [ 9.934247] hantro-vpu 38320000.video-codec: genpd_runtime_suspend() [ 9.940707] hantro-vpu 38320000.video-codec: suspend latency exceeded, 1500 ns [ 9.948042] PM: vpublk-h1: Power-off latency exceeded, new value 18875 ns [ 9.975951] PM: vpu-h1: Power-off latency exceeded, new value 17625 ns [ 9.999970] PM: vpumix: Power-off latency exceeded, new value 25750 ns However, because the vpumix is disabled here, I think it might be what's causing a hang when I attempt to read the regmap registers with cat /sys/kernel/debug/regmap/38330000.blk-ctrl/registers I was looking at the ATF code that NXP has, and it doesn't appear that the VPUMIX ever shuts down, and for that matter, I don't think the g1, g2, and h1 domains shutdown either. I think it makes sense to have the domains turned off when not in use, but I have a few comments / questions below.... > 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); > + 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)); The individual VPU's (IMX8MM_VPUBLK_PD_G1, G2, and H1) have clk_mask values that set these bits when used and clear them when disabled. If the VPUMix needs them set, shouldn't the IMX8MM_VPUBLK_PD_xx leave them on until they are cleared by the VPUMIX? > + > + 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); Should these registers ever get turned off when we disable the VPUMIX? These registers don't have good descriptions in the TRM describing how they interact with the VPU's, and it's unclear to me what happens if they are set and the VPUMIX and various VPU's are disabled. I am curious to know if they should get enabled/disabled with their respective IMX8MM_VPUBLK_PD_xx domain. The Example Code 5 in the TRM doesn't set these bits in their VPUMIX example, so it makes me think they're OK to leave with the individual IMX8MM_VPUBLK_PD_xx domains. I ask, because if I run v4l2-compliance on the H1 encoder, it also appears to hang, but the power-domains appear to be attempting to start, and the vpumix is the last thing to start, and I would expect the pgc_vpu_h1 domain to follow, then end with the vpublk-h1 starting. # v4l2-compliance -d3 [ 271.001098] hantro-vpu 38320000.video-codec: genpd_runtime_resume() [ 271.007447] genpd genpd:0:38330000.blk-ctrl: genpd_runtime_resume() [ 271.013796] PM: vpumix: Power-on latency exceeded, new value 40623 ns [ 271.020314] genpd genpd:3:38330000.blk-ctrl: genpd_runtime_resume() adam [1] - https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=576435 > + } > + > + 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); > -- > 2.30.2 >
Hi Adam, Am Sonntag, dem 07.11.2021 um 15:08 -0600 schrieb Adam Ford: > On Fri, Sep 10, 2021 at 3:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > 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. > > > Sorry to resurrect an old thread, but I'm running into issues enabling > new peripherals, and I think it's related to this, but I am not > certain. > > I have attempted to enable the various VPU's, and in doing so, I > referenced the vpu_blk_ctrl for the respective power-domains reference > [1]. > > The various VPU devices enumerate, and I can see the various VPU power > domains turn on and off a boot. > > [ 1.968565] genpd genpd:0:38330000.blk-ctrl: adding to PM domain vpumix > [ 1.981208] genpd genpd:1:38330000.blk-ctrl: adding to PM domain vpu-g1 > [ 1.993838] genpd genpd:2:38330000.blk-ctrl: adding to PM domain vpu-g2 > [ 2.006455] genpd genpd:3:38330000.blk-ctrl: adding to PM domain vpu-h1 > [ 8.922661] hantro_vpu: module is from the staging directory, the > quality is unknown, you have been warned. > [ 9.008147] hantro_vpu: module is from the staging directory, the > quality is unknown, you have been warned. > [ 9.044041] hantro-vpu 38300000.video-codec: adding to PM domain vpublk-g1 > [ 9.050959] hantro-vpu 38300000.video-codec: genpd_add_device() > [ 9.063349] PM: vpumix: Power-on latency exceeded, new value 50875 ns > [ 9.083437] PM: vpu-g1: Power-on latency exceeded, new value 26125 ns > [ 9.104778] PM: vpublk-g1: Power-on latency exceeded, new value 47819250 ns > [ 9.211988] hantro-vpu 38300000.video-codec: registered > nxp,imx8mm-vpu-dec as /dev/video0 > [ 9.259680] hantro-vpu 38300000.video-codec: genpd_runtime_resume() > [ 9.297395] hantro-vpu 38300000.video-codec: resume latency exceeded, 2750 ns > [ 9.307767] hantro-vpu 38310000.video-codec: adding to PM domain vpublk-g2 > [ 9.316462] hantro-vpu 38310000.video-codec: genpd_add_device() > [ 9.328807] PM: vpu-g2: Power-on latency exceeded, new value 26625 ns > [ 9.342401] PM: vpublk-g2: Power-on latency exceeded, new value 19965125 ns > [ 9.430254] hantro-vpu 38300000.video-codec: genpd_runtime_suspend() > [ 9.436683] hantro-vpu 38300000.video-codec: suspend latency > exceeded, 1625 ns > [ 9.443984] PM: vpublk-g1: Power-off latency exceeded, new value 16625 ns > [ 9.462361] hantro-vpu 38310000.video-codec: registered > nxp,imx8mm-vpu-g2-dec as /dev/video2 > [ 9.491848] PM: vpu-g1: Power-off latency exceeded, new value 17125 ns > [ 9.506353] hantro-vpu 38310000.video-codec: genpd_runtime_resume() > [ 9.512735] hantro-vpu 38310000.video-codec: resume latency exceeded, 2750 ns > [ 9.520306] hantro-vpu 38320000.video-codec: adding to PM domain vpublk-h1 > [ 9.527268] hantro-vpu 38320000.video-codec: genpd_add_device() > [ 9.539632] PM: vpu-h1: Power-on latency exceeded, new value 27750 ns > [ 9.553358] PM: vpublk-h1: Power-on latency exceeded, new value 20112125 ns > [ 9.605800] hantro_vpu: module is from the staging directory, the > quality is unknown, you have been warned. > [ 9.642194] hantro-vpu 38310000.video-codec: genpd_runtime_suspend() > [ 9.648634] hantro-vpu 38310000.video-codec: suspend latency > exceeded, 1500 ns > [ 9.655974] PM: vpublk-g2: Power-off latency exceeded, new value 12625 ns > [ 9.703863] PM: vpu-g2: Power-off latency exceeded, new value 16750 ns > [ 9.754527] hantro-vpu 38320000.video-codec: registered > nxp,imx8mm-vpu-h1-enc as /dev/video3 > [ 9.785424] hantro-vpu 38320000.video-codec: genpd_runtime_resume() > [ 9.796034] hantro-vpu 38320000.video-codec: resume latency exceeded, 2000 ns > [ 9.934247] hantro-vpu 38320000.video-codec: genpd_runtime_suspend() > [ 9.940707] hantro-vpu 38320000.video-codec: suspend latency > exceeded, 1500 ns > [ 9.948042] PM: vpublk-h1: Power-off latency exceeded, new value 18875 ns > [ 9.975951] PM: vpu-h1: Power-off latency exceeded, new value 17625 ns > [ 9.999970] PM: vpumix: Power-off latency exceeded, new value 25750 ns > > However, because the vpumix is disabled here, I think it might be > what's causing a hang when I attempt to read the regmap registers with > > cat /sys/kernel/debug/regmap/38330000.blk-ctrl/registers That is expected, as regmap doesn't tie into the runtime PM and is the same behavior as on other devices when you try to read the registers via regmap while the peripheral clock is gated. > > I was looking at the ATF code that NXP has, and it doesn't appear that > the VPUMIX ever shuts down, and for that matter, I don't think the g1, > g2, and h1 domains shutdown either. > > I think it makes sense to have the domains turned off when not in use, > but I have a few comments / questions below.... > > > 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); > > + 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)); > > The individual VPU's (IMX8MM_VPUBLK_PD_G1, G2, and H1) have clk_mask > values that set these bits when used and clear them when disabled. If > the VPUMix needs them set, shouldn't the IMX8MM_VPUBLK_PD_xx leave > them on until they are cleared by the VPUMIX? No, VPUMIX only need those clocks to be running for the ADB handshake to work. The handshake is only executed when powering up/down a GPC domain. So we can clockgate the individual Hantro cores when not in use while the power domain is up. > > > + > > + 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); > > Should these registers ever get turned off when we disable the VPUMIX? > These registers don't have good descriptions in the TRM describing how > they interact with the VPU's, and it's unclear to me what happens if > they are set and the VPUMIX and various VPU's are disabled. > Those registers lose their state when the VPUMIX power collapses, thus we need to re-initialize them when powering up the domain. As far as I understand it those registers just carry some strap bits for the decoders/encoder to let them know which features should be enabled. > I am > curious to know if they should get enabled/disabled with their > respective IMX8MM_VPUBLK_PD_xx domain. The Example Code 5 in the TRM > doesn't set these bits in their VPUMIX example, so it makes me think > they're OK to leave with the individual IMX8MM_VPUBLK_PD_xx domains. > > I ask, because if I run v4l2-compliance on the H1 encoder, it also > appears to hang, but the power-domains appear to be attempting to > start, and the vpumix is the last thing to start, and I would expect > the pgc_vpu_h1 domain to follow, then end with the > vpublk-h1 starting. > > # v4l2-compliance -d3 > [ 271.001098] hantro-vpu 38320000.video-codec: genpd_runtime_resume() > [ 271.007447] genpd genpd:0:38330000.blk-ctrl: genpd_runtime_resume() > [ 271.013796] PM: vpumix: Power-on latency exceeded, new value 40623 ns > [ 271.020314] genpd genpd:3:38330000.blk-ctrl: genpd_runtime_resume() > This looks correct to me. genpd:0:38330000.blk-ctrl is the VPU blk-ctrl bus domain, which should start the GPC vpumix domain, genpd:3:38330000.blk-ctrl is the blk-ctrl H1 domain, which should in turn power up the GPC h1 domain. Do you know where exactly things are hanging? Regards, Lucas > adam > > [1] - https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=576435 > > + } > > + > > + 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); > > -- > > 2.30.2 > >
On Mon, Nov 8, 2021 at 2:35 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > Hi Adam, > > Am Sonntag, dem 07.11.2021 um 15:08 -0600 schrieb Adam Ford: > > On Fri, Sep 10, 2021 at 3:26 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > 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. > > > > > Sorry to resurrect an old thread, but I'm running into issues enabling > > new peripherals, and I think it's related to this, but I am not > > certain. > > > > I have attempted to enable the various VPU's, and in doing so, I > > referenced the vpu_blk_ctrl for the respective power-domains reference > > [1]. > > > > The various VPU devices enumerate, and I can see the various VPU power > > domains turn on and off a boot. > > > > [ 1.968565] genpd genpd:0:38330000.blk-ctrl: adding to PM domain vpumix > > [ 1.981208] genpd genpd:1:38330000.blk-ctrl: adding to PM domain vpu-g1 > > [ 1.993838] genpd genpd:2:38330000.blk-ctrl: adding to PM domain vpu-g2 > > [ 2.006455] genpd genpd:3:38330000.blk-ctrl: adding to PM domain vpu-h1 > > [ 8.922661] hantro_vpu: module is from the staging directory, the > > quality is unknown, you have been warned. > > [ 9.008147] hantro_vpu: module is from the staging directory, the > > quality is unknown, you have been warned. > > [ 9.044041] hantro-vpu 38300000.video-codec: adding to PM domain vpublk-g1 > > [ 9.050959] hantro-vpu 38300000.video-codec: genpd_add_device() > > [ 9.063349] PM: vpumix: Power-on latency exceeded, new value 50875 ns > > [ 9.083437] PM: vpu-g1: Power-on latency exceeded, new value 26125 ns > > [ 9.104778] PM: vpublk-g1: Power-on latency exceeded, new value 47819250 ns > > [ 9.211988] hantro-vpu 38300000.video-codec: registered > > nxp,imx8mm-vpu-dec as /dev/video0 > > [ 9.259680] hantro-vpu 38300000.video-codec: genpd_runtime_resume() > > [ 9.297395] hantro-vpu 38300000.video-codec: resume latency exceeded, 2750 ns > > [ 9.307767] hantro-vpu 38310000.video-codec: adding to PM domain vpublk-g2 > > [ 9.316462] hantro-vpu 38310000.video-codec: genpd_add_device() > > [ 9.328807] PM: vpu-g2: Power-on latency exceeded, new value 26625 ns > > [ 9.342401] PM: vpublk-g2: Power-on latency exceeded, new value 19965125 ns > > [ 9.430254] hantro-vpu 38300000.video-codec: genpd_runtime_suspend() > > [ 9.436683] hantro-vpu 38300000.video-codec: suspend latency > > exceeded, 1625 ns > > [ 9.443984] PM: vpublk-g1: Power-off latency exceeded, new value 16625 ns > > [ 9.462361] hantro-vpu 38310000.video-codec: registered > > nxp,imx8mm-vpu-g2-dec as /dev/video2 > > [ 9.491848] PM: vpu-g1: Power-off latency exceeded, new value 17125 ns > > [ 9.506353] hantro-vpu 38310000.video-codec: genpd_runtime_resume() > > [ 9.512735] hantro-vpu 38310000.video-codec: resume latency exceeded, 2750 ns > > [ 9.520306] hantro-vpu 38320000.video-codec: adding to PM domain vpublk-h1 > > [ 9.527268] hantro-vpu 38320000.video-codec: genpd_add_device() > > [ 9.539632] PM: vpu-h1: Power-on latency exceeded, new value 27750 ns > > [ 9.553358] PM: vpublk-h1: Power-on latency exceeded, new value 20112125 ns > > [ 9.605800] hantro_vpu: module is from the staging directory, the > > quality is unknown, you have been warned. > > [ 9.642194] hantro-vpu 38310000.video-codec: genpd_runtime_suspend() > > [ 9.648634] hantro-vpu 38310000.video-codec: suspend latency > > exceeded, 1500 ns > > [ 9.655974] PM: vpublk-g2: Power-off latency exceeded, new value 12625 ns > > [ 9.703863] PM: vpu-g2: Power-off latency exceeded, new value 16750 ns > > [ 9.754527] hantro-vpu 38320000.video-codec: registered > > nxp,imx8mm-vpu-h1-enc as /dev/video3 > > [ 9.785424] hantro-vpu 38320000.video-codec: genpd_runtime_resume() > > [ 9.796034] hantro-vpu 38320000.video-codec: resume latency exceeded, 2000 ns > > [ 9.934247] hantro-vpu 38320000.video-codec: genpd_runtime_suspend() > > [ 9.940707] hantro-vpu 38320000.video-codec: suspend latency > > exceeded, 1500 ns > > [ 9.948042] PM: vpublk-h1: Power-off latency exceeded, new value 18875 ns > > [ 9.975951] PM: vpu-h1: Power-off latency exceeded, new value 17625 ns > > [ 9.999970] PM: vpumix: Power-off latency exceeded, new value 25750 ns > > > > However, because the vpumix is disabled here, I think it might be > > what's causing a hang when I attempt to read the regmap registers with > > > > cat /sys/kernel/debug/regmap/38330000.blk-ctrl/registers > > That is expected, as regmap doesn't tie into the runtime PM and is the > same behavior as on other devices when you try to read the registers > via regmap while the peripheral clock is gated. Ok. That makes sense. I think I was so focussed on getting the regmap working, I assumed something was wrong with the blk-ctrl. > > > > > I was looking at the ATF code that NXP has, and it doesn't appear that > > the VPUMIX ever shuts down, and for that matter, I don't think the g1, > > g2, and h1 domains shutdown either. > > > > I think it makes sense to have the domains turned off when not in use, > > but I have a few comments / questions below.... > > > > > 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); > > > + 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)); > > > > The individual VPU's (IMX8MM_VPUBLK_PD_G1, G2, and H1) have clk_mask > > values that set these bits when used and clear them when disabled. If > > the VPUMix needs them set, shouldn't the IMX8MM_VPUBLK_PD_xx leave > > them on until they are cleared by the VPUMIX? > > No, VPUMIX only need those clocks to be running for the ADB handshake > to work. The handshake is only executed when powering up/down a GPC > domain. So we can clockgate the individual Hantro cores when not in use > while the power domain is up. Ok, that makes sense. > > > > > > + > > > + 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); > > > > Should these registers ever get turned off when we disable the VPUMIX? > > These registers don't have good descriptions in the TRM describing how > > they interact with the VPU's, and it's unclear to me what happens if > > they are set and the VPUMIX and various VPU's are disabled. > > > Those registers lose their state when the VPUMIX power collapses, thus > we need to re-initialize them when powering up the domain. As far as I > understand it those registers just carry some strap bits for the > decoders/encoder to let them know which features should be enabled. > > > I am > > curious to know if they should get enabled/disabled with their > > respective IMX8MM_VPUBLK_PD_xx domain. The Example Code 5 in the TRM > > doesn't set these bits in their VPUMIX example, so it makes me think > > they're OK to leave with the individual IMX8MM_VPUBLK_PD_xx domains. > > > > I ask, because if I run v4l2-compliance on the H1 encoder, it also > > appears to hang, but the power-domains appear to be attempting to > > start, and the vpumix is the last thing to start, and I would expect > > the pgc_vpu_h1 domain to follow, then end with the > > vpublk-h1 starting. > > > > # v4l2-compliance -d3 > > [ 271.001098] hantro-vpu 38320000.video-codec: genpd_runtime_resume() > > [ 271.007447] genpd genpd:0:38330000.blk-ctrl: genpd_runtime_resume() > > [ 271.013796] PM: vpumix: Power-on latency exceeded, new value 40623 ns > > [ 271.020314] genpd genpd:3:38330000.blk-ctrl: genpd_runtime_resume() > > > This looks correct to me. genpd:0:38330000.blk-ctrl is the VPU blk-ctrl > bus domain, which should start the GPC vpumix domain, > genpd:3:38330000.blk-ctrl is the blk-ctrl H1 domain, which should in > turn power up the GPC h1 domain. > > Do you know where exactly things are hanging? I added more debugging and it appears to be hanging in the VPU code itself and not the power domain like I originally thought. I was focussed on the power domain because the regmap was hanging that I didn't even consider the possibility it was the VPU code itself. [ 377.840195] hantro-vpu 38320000.video-codec: genpd_runtime_resume(): [ 377.846675] hantro-vpu 38320000.video-codec: vpublk-h1() [ 377.852115] genpd genpd:0:38330000.blk-ctrl: genpd_runtime_resume(): [ 377.858581] genpd genpd:0:38330000.blk-ctrl: vpumix() [ 377.863658] imx_pgc_power_up(): vpumix [ 377.867468] imx_pgc_power_up(): Success [ 377.871351] genpd genpd:3:38330000.blk-ctrl: genpd_runtime_resume(): [ 377.877810] genpd genpd:3:38330000.blk-ctrl: vpu-h1() [ 377.882891] imx_pgc_power_up(): vpu-h1 [ 377.886679] imx_pgc_power_up(): Success [ 377.890540] genpd genpd:3:38330000.blk-ctrl: resume latency exceeded, 750 ns [ 377.897636] PM: vpublk-h1: Power-on latency exceeded, new value 45528719 ns [ 377.904635] hantro_h1_jpeg_enc_run() Thanks for your help. I think we're good. I'll focus my work on the VPU encoder. Sorry for the noise. I think I better understand the blk-ctrl now too. adam > > Regards, > Lucas > > > adam > > > > [1] - https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=576435 > > > + } > > > + > > > + 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); > > > -- > > > 2.30.2 > > > > >
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); + 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);