mbox series

[0/9] i.MX8MP power-domains part 1 and GPU support

Message ID 20220119134027.2931945-1-l.stach@pengutronix.de
Headers show
Series i.MX8MP power-domains part 1 and GPU support | expand

Message

Lucas Stach Jan. 19, 2022, 1:40 p.m. UTC
Hi all,

this series starts adding the power-domain control for the i.MX8MP
SoC. The GPCv2 support is complete (at least from going over the RM,
TF-A and experience with other i.MX8M* SoCs), but not all
power-domains are usable right now. Currently only the HSIO
(USB and PCIe) and GPU power domains are enabled.

Other power domains (MEDIA, VPU, HDMI, AUDIO) can be added when the
blk-ctrl driver support for those domains is ready, which is still
work in progress at the moment. As my priorities are shifting to
other things for a while, I wanted to push out the part that is
usable now and enables more functionality on the i.MX8MP.

Regards,
Lucas

Lucas Stach (9):
  soc: imx: gpcv2: add PGC control register indirection
  dt-bindings: power: add defines for i.MX8MP power domain
  soc: imx: gpcv2: add support for i.MX8MP power domains
  dt-bindings: power: imx8mp: add defines for HSIO blk-ctrl domains
  dt-bindings: soc: add binding for i.MX8MP HSIO blk-ctrl
  soc: imx: add i.MX8MP HSIO blk-ctrl
  arm64: dts: imx8mp: add HSIO power-domains
  arm64: dts: imx8mp: add GPU power domains
  arm64: dts: imx8mp: add GPU nodes

 .../bindings/power/fsl,imx-gpcv2.yaml         |   2 +
 .../soc/imx/fsl,imx8mp-hsio-blk-ctrl.yaml     |  78 +++
 arch/arm64/boot/dts/freescale/imx8mp.dtsi     | 118 ++++-
 drivers/soc/imx/Makefile                      |   1 +
 drivers/soc/imx/gpcv2.c                       | 430 ++++++++++++++++-
 drivers/soc/imx/imx8mp-blk-ctrl.c             | 444 ++++++++++++++++++
 include/dt-bindings/power/imx8mp-power.h      |  35 ++
 7 files changed, 1090 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-hsio-blk-ctrl.yaml
 create mode 100644 drivers/soc/imx/imx8mp-blk-ctrl.c
 create mode 100644 include/dt-bindings/power/imx8mp-power.h

Comments

Abel Vesa Jan. 19, 2022, 2:38 p.m. UTC | #1
On 22-01-19 14:40:18, Lucas Stach wrote:
> Hi all,
> 
> this series starts adding the power-domain control for the i.MX8MP
> SoC. The GPCv2 support is complete (at least from going over the RM,
> TF-A and experience with other i.MX8M* SoCs), but not all
> power-domains are usable right now. Currently only the HSIO
> (USB and PCIe) and GPU power domains are enabled.
> 
> Other power domains (MEDIA, VPU, HDMI, AUDIO) can be added when the
> blk-ctrl driver support for those domains is ready, which is still
> work in progress at the moment. As my priorities are shifting to
> other things for a while, I wanted to push out the part that is
> usable now and enables more functionality on the i.MX8MP.
> 

Great effort! Thanks for working on this!

I started doing it myself a couple of months ago. I did the media and
hdmi blk-ctrls. The audio blk-ctrl is the one that got me stuck since it
has PLLs in it and they need to be part of the clock tree somehow.

Let me know if you want me to send the hdmi and media blk-ctrls.
I'll try to rebase them on top of this patchset.

> Regards,
> Lucas
> 
> Lucas Stach (9):
>   soc: imx: gpcv2: add PGC control register indirection
>   dt-bindings: power: add defines for i.MX8MP power domain
>   soc: imx: gpcv2: add support for i.MX8MP power domains
>   dt-bindings: power: imx8mp: add defines for HSIO blk-ctrl domains
>   dt-bindings: soc: add binding for i.MX8MP HSIO blk-ctrl
>   soc: imx: add i.MX8MP HSIO blk-ctrl
>   arm64: dts: imx8mp: add HSIO power-domains
>   arm64: dts: imx8mp: add GPU power domains
>   arm64: dts: imx8mp: add GPU nodes
> 
>  .../bindings/power/fsl,imx-gpcv2.yaml         |   2 +
>  .../soc/imx/fsl,imx8mp-hsio-blk-ctrl.yaml     |  78 +++
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi     | 118 ++++-
>  drivers/soc/imx/Makefile                      |   1 +
>  drivers/soc/imx/gpcv2.c                       | 430 ++++++++++++++++-
>  drivers/soc/imx/imx8mp-blk-ctrl.c             | 444 ++++++++++++++++++
>  include/dt-bindings/power/imx8mp-power.h      |  35 ++
>  7 files changed, 1090 insertions(+), 18 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,imx8mp-hsio-blk-ctrl.yaml
>  create mode 100644 drivers/soc/imx/imx8mp-blk-ctrl.c
>  create mode 100644 include/dt-bindings/power/imx8mp-power.h
> 
> -- 
> 2.30.2
>
Lucas Stach Jan. 19, 2022, 3:01 p.m. UTC | #2
Hi Abel,

Am Mittwoch, dem 19.01.2022 um 16:38 +0200 schrieb Abel Vesa:
> On 22-01-19 14:40:18, Lucas Stach wrote:
> > Hi all,
> > 
> > this series starts adding the power-domain control for the i.MX8MP
> > SoC. The GPCv2 support is complete (at least from going over the RM,
> > TF-A and experience with other i.MX8M* SoCs), but not all
> > power-domains are usable right now. Currently only the HSIO
> > (USB and PCIe) and GPU power domains are enabled.
> > 
> > Other power domains (MEDIA, VPU, HDMI, AUDIO) can be added when the
> > blk-ctrl driver support for those domains is ready, which is still
> > work in progress at the moment. As my priorities are shifting to
> > other things for a while, I wanted to push out the part that is
> > usable now and enables more functionality on the i.MX8MP.
> > 
> 
> Great effort! Thanks for working on this!
> 
> I started doing it myself a couple of months ago. I did the media and
> hdmi blk-ctrls. The audio blk-ctrl is the one that got me stuck since it
> has PLLs in it and they need to be part of the clock tree somehow.
> 
> Let me know if you want me to send the hdmi and media blk-ctrls.
> I'll try to rebase them on top of this patchset.

That would certainly be very helpful!

The HSIO one also has a PLL that can optionally be used as a reference
for the USB and PCIe PHYs. I think it should be doable to integrate
them in the clock tree. We need some additional smarts to save/restore
the clock state when the *MIX domain powers down/up.

Other than that I think we need to add a rule that those blk-ctrl
clocks can only be prepared/enabled when the power domain is already up
to avoid circling back into the clock framework via the GPC, but I
guess that's a reasonable rule for the peripheral drivers to adhere to.
Just always runtime resume the peripheral before enabling any clocks,
or possibly even just enable the clocks in the runtime resume callback
of the driver.

Regards,
Lucas
Abel Vesa Jan. 21, 2022, 9:04 a.m. UTC | #3
On 22-01-19 14:40:24, Lucas Stach wrote:
> The i.MX8MP added some blk-ctrl peripherals that don't follow the regular
> structure of the blk-ctrls in the previous SoCs. Add a new file for those
> with currently only the HSIO blk-ctrl being supported. Others will be added
> later on.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/soc/imx/Makefile          |   1 +
>  drivers/soc/imx/imx8mp-blk-ctrl.c | 444 ++++++++++++++++++++++++++++++
>  2 files changed, 445 insertions(+)
>  create mode 100644 drivers/soc/imx/imx8mp-blk-ctrl.c
> 
> diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
> index 8a707077914c..63cd29f6d4d2 100644
> --- a/drivers/soc/imx/Makefile
> +++ b/drivers/soc/imx/Makefile
> @@ -6,3 +6,4 @@ 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
> +obj-$(CONFIG_SOC_IMX8M) += imx8mp-blk-ctrl.o
> diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c

So far the imx8m-blk-ctrl is used only by i.MX8MM, while this new one is
used by i.MX8MP. Do you think we can have the generic stuff we should
probably put the generic stuff in something new called imx-blk-ctrl
(which could be used for future non-i.MX8) ? Then maybe we can have one
file per SoC that only adds the SoC specific stuff. For example, you
define those structs that look quite the same in each file. Same goes
for the probe function.

I can prepare a patch and send it, if you want.

> new file mode 100644
> index 000000000000..7f4e1a151d2b
> --- /dev/null
> +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c
> @@ -0,0 +1,444 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * Copyright 2022 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/imx8mp-power.h>
> +
> +#define GPR_REG0		0x0
> +#define  PCIE_CLOCK_MODULE_EN	BIT(0)
> +#define  USB_CLOCK_MODULE_EN	BIT(1)
> +
> +struct imx8mp_hsio_blk_ctrl_domain;
> +
> +struct imx8mp_hsio_blk_ctrl {
> +	struct device *dev;
> +	struct notifier_block power_nb;
> +	struct device *bus_power_dev;
> +	struct regmap *regmap;
> +	struct imx8mp_hsio_blk_ctrl_domain *domains;
> +	struct genpd_onecell_data onecell_data;
> +};
> +
> +struct imx8mp_hsio_blk_ctrl_domain_data {
> +	const char *name;
> +	const char *clk_name;
> +	const char *gpc_name;
> +};
> +
> +struct imx8mp_hsio_blk_ctrl_domain {
> +	struct generic_pm_domain genpd;
> +	struct clk *clk;
> +	struct device *power_dev;
> +	struct imx8mp_hsio_blk_ctrl *bc;
> +	int id;
> +};
> +
> +static inline struct imx8mp_hsio_blk_ctrl_domain *
> +to_imx8mp_hsio_blk_ctrl_domain(struct generic_pm_domain *genpd)
> +{
> +	return container_of(genpd, struct imx8mp_hsio_blk_ctrl_domain, genpd);
> +}
> +
> +static int imx8mp_hsio_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> +{
> +	struct imx8mp_hsio_blk_ctrl_domain *domain =
> +			to_imx8mp_hsio_blk_ctrl_domain(genpd);
> +	struct imx8mp_hsio_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;
> +	}
> +
> +	/* enable upstream and blk-ctrl clocks */
> +	ret = clk_prepare_enable(domain->clk);
> +	if (ret) {
> +		dev_err(bc->dev, "failed to enable clocks\n");
> +		goto bus_put;
> +	}
> +
> +	switch (domain->id) {
> +	case IMX8MP_HSIOBLK_PD_USB:
> +		regmap_set_bits(bc->regmap, GPR_REG0, USB_CLOCK_MODULE_EN);
> +		break;
> +	case IMX8MP_HSIOBLK_PD_PCIE:
> +		regmap_set_bits(bc->regmap, GPR_REG0, PCIE_CLOCK_MODULE_EN);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	/* 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;
> +	}
> +
> +	return 0;
> +
> +clk_disable:
> +	clk_disable_unprepare(domain->clk);
> +bus_put:
> +	pm_runtime_put(bc->bus_power_dev);
> +
> +	return ret;
> +}
> +
> +static int imx8mp_hsio_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> +{
> +	struct imx8mp_hsio_blk_ctrl_domain *domain =
> +			to_imx8mp_hsio_blk_ctrl_domain(genpd);
> +	struct imx8mp_hsio_blk_ctrl *bc = domain->bc;
> +
> +	/* disable clocks */
> +	switch (domain->id) {
> +	case IMX8MP_HSIOBLK_PD_USB:
> +		regmap_clear_bits(bc->regmap, GPR_REG0, USB_CLOCK_MODULE_EN);
> +		break;
> +	case IMX8MP_HSIOBLK_PD_PCIE:
> +		regmap_clear_bits(bc->regmap, GPR_REG0, PCIE_CLOCK_MODULE_EN);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	clk_disable_unprepare(domain->clk);
> +
> +	/* 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 const struct imx8mp_hsio_blk_ctrl_domain_data imx8mp_hsio_domain_data[] = {
> +	[IMX8MP_HSIOBLK_PD_USB] = {
> +		.name = "hsioblk-usb",
> +		.clk_name = "usb",
> +		.gpc_name = "usb",
> +	},
> +	[IMX8MP_HSIOBLK_PD_USB_PHY1] = {
> +		.name = "hsioblk-ubs-phy1",
> +		.gpc_name = "usb-phy1",
> +	},
> +	[IMX8MP_HSIOBLK_PD_USB_PHY2] = {
> +		.name = "hsioblk-ubs-phy2",
> +		.gpc_name = "usb-phy2",
> +	},
> +	[IMX8MP_HSIOBLK_PD_PCIE] = {
> +		.name = "hsioblk-pcie",
> +		.clk_name = "pcie",
> +		.gpc_name = "pcie",
> +	},
> +	[IMX8MP_HSIOBLK_PD_PCIE_PHY] = {
> +		.name = "hsioblk-pcie-phy",
> +		.gpc_name = "pcie-phy",
> +	},
> +};
> +
> +static int imx8mp_hsio_power_notifier(struct notifier_block *nb,
> +				      unsigned long action, void *data)
> +{
> +	struct imx8mp_hsio_blk_ctrl *bc = container_of(nb, struct imx8mp_hsio_blk_ctrl,
> +						 power_nb);
> +	struct clk *usb_clk = bc->domains[IMX8MP_HSIOBLK_PD_USB].clk;
> +	int ret;
> +
> +	switch (action) {
> +	case GENPD_NOTIFY_ON:
> +		/*
> +		 * enable USB clock for a moment for the power-on ADB handshake
> +		 * to proceed
> +		 */
> +		ret = clk_prepare_enable(usb_clk);
> +		if (ret)
> +			return NOTIFY_BAD;
> +		regmap_set_bits(bc->regmap, GPR_REG0, USB_CLOCK_MODULE_EN);
> +
> +		udelay(5);
> +
> +		regmap_clear_bits(bc->regmap, GPR_REG0, USB_CLOCK_MODULE_EN);
> +		clk_disable_unprepare(usb_clk);
> +		break;
> +	case GENPD_NOTIFY_PRE_OFF:
> +		/* enable USB clock for the power-down ADB handshake to work */
> +		ret = clk_prepare_enable(usb_clk);
> +		if (ret)
> +			return NOTIFY_BAD;
> +
> +		regmap_set_bits(bc->regmap, GPR_REG0, USB_CLOCK_MODULE_EN);
> +		break;
> +	case GENPD_NOTIFY_OFF:
> +		clk_disable_unprepare(usb_clk);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int imx8mp_hsio_blk_ctrl_probe(struct platform_device *pdev)
> +{
> +	int num_domains = ARRAY_SIZE(imx8mp_hsio_domain_data);
> +	struct device *dev = &pdev->dev;
> +	struct imx8mp_hsio_blk_ctrl *bc;
> +	void __iomem *base;
> +	int i, ret;
> +
> +	struct regmap_config regmap_config = {
> +		.reg_bits	= 32,
> +		.val_bits	= 32,
> +		.reg_stride	= 4,
> +		.max_register	= 0x24,
> +	};
> +
> +	bc = devm_kzalloc(dev, sizeof(*bc), GFP_KERNEL);
> +	if (!bc)
> +		return -ENOMEM;
> +
> +	bc->dev = dev;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	bc->regmap = devm_regmap_init_mmio(dev, base, &regmap_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, num_domains,
> +				   sizeof(struct imx8mp_hsio_blk_ctrl_domain),
> +				   GFP_KERNEL);
> +	if (!bc->domains)
> +		return -ENOMEM;
> +
> +	bc->onecell_data.num_domains = num_domains;
> +	bc->onecell_data.xlate = imx8m_blk_ctrl_xlate;
> +	bc->onecell_data.domains =
> +		devm_kcalloc(dev, 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 < num_domains; i++) {
> +		const struct imx8mp_hsio_blk_ctrl_domain_data *data =
> +				&imx8mp_hsio_domain_data[i];
> +		struct imx8mp_hsio_blk_ctrl_domain *domain = &bc->domains[i];
> +
> +		if (data->clk_name) {
> +			domain->clk = devm_clk_get(dev, data->clk_name);
> +			if (IS_ERR(domain->clk)) {
> +				ret = PTR_ERR(domain->clk);
> +				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 = imx8mp_hsio_blk_ctrl_power_on;
> +		domain->genpd.power_off = imx8mp_hsio_blk_ctrl_power_off;
> +		domain->bc = bc;
> +		domain->id = i;
> +
> +		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 = imx8mp_hsio_power_notifier;
> +	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 imx8mp_hsio_blk_ctrl_remove(struct platform_device *pdev)
> +{
> +	struct imx8mp_hsio_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 imx8mp_hsio_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 imx8mp_hsio_blk_ctrl_suspend(struct device *dev)
> +{
> +	struct imx8mp_hsio_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 imx8mp_hsio_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 imx8mp_hsio_blk_ctrl_resume(struct device *dev)
> +{
> +	struct imx8mp_hsio_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 imx8mp_hsio_blk_ctrl_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(imx8mp_hsio_blk_ctrl_suspend,
> +				imx8mp_hsio_blk_ctrl_resume)
> +};
> +
> +static const struct of_device_id imx8mp_hsio_blk_ctrl_of_match[] = {
> +	{
> +		.compatible = "fsl,imx8mp-hsio-blk-ctrl",
> +	}, {
> +		/* Sentinel */
> +	}
> +};
> +MODULE_DEVICE_TABLE(of, imx8m_blk_ctrl_of_match);
> +
> +static struct platform_driver imx8mp_hsio_blk_ctrl_driver = {
> +	.probe = imx8mp_hsio_blk_ctrl_probe,
> +	.remove = imx8mp_hsio_blk_ctrl_remove,
> +	.driver = {
> +		.name = "imx8mp-hsio-blk-ctrl",
> +		.pm = &imx8mp_hsio_blk_ctrl_pm_ops,
> +		.of_match_table = imx8mp_hsio_blk_ctrl_of_match,
> +	},
> +};
> +module_platform_driver(imx8mp_hsio_blk_ctrl_driver);
> -- 
> 2.30.2
>
Lucas Stach Jan. 21, 2022, 9:19 a.m. UTC | #4
Hi Abel,

Am Freitag, dem 21.01.2022 um 11:04 +0200 schrieb Abel Vesa:
> On 22-01-19 14:40:24, Lucas Stach wrote:
> > The i.MX8MP added some blk-ctrl peripherals that don't follow the regular
> > structure of the blk-ctrls in the previous SoCs. Add a new file for those
> > with currently only the HSIO blk-ctrl being supported. Others will be added
> > later on.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/soc/imx/Makefile          |   1 +
> >  drivers/soc/imx/imx8mp-blk-ctrl.c | 444 ++++++++++++++++++++++++++++++
> >  2 files changed, 445 insertions(+)
> >  create mode 100644 drivers/soc/imx/imx8mp-blk-ctrl.c
> > 
> > diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
> > index 8a707077914c..63cd29f6d4d2 100644
> > --- a/drivers/soc/imx/Makefile
> > +++ b/drivers/soc/imx/Makefile
> > @@ -6,3 +6,4 @@ 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
> > +obj-$(CONFIG_SOC_IMX8M) += imx8mp-blk-ctrl.o
> > diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c
> 
> So far the imx8m-blk-ctrl is used only by i.MX8MM, while this new one is
> used by i.MX8MP. Do you think we can have the generic stuff we should
> probably put the generic stuff in something new called imx-blk-ctrl
> (which could be used for future non-i.MX8) ? Then maybe we can have one
> file per SoC that only adds the SoC specific stuff. For example, you
> define those structs that look quite the same in each file. Same goes
> for the probe function.
> 
The i.MX8MP VPU and MEDIA blk-ctrls also match the structure laid out
in imx8m-blk-ctrl and should use this driver. HSIO, AUDIO and HDMI
however don't have that regular clk/reset register structure and need
special handling, that's why I added the new file for those.
 
> I can prepare a patch and send it, if you want.

For now I didn't consider the overlap big enough to warrant that. While
many things look similar they are different in little details, so I
didn't want to start with adding abstractions there. Usually it's
easier to first (mostly) duplicate some code and then refactor to
abstract some things and move them into common code when you have the
full picture where the real overlap is. Trying to move too much stuff
into common code before having the full picture is usually a recipe for
unreadable code.

However, I won't stop you from giving it a shot. I just think it would
be better to first get all the blk-ctrls on the 8MP working, even with
some duplication, and then clean things up when we have a more complete
picture.

Regards,
Lucas

> 
> > new file mode 100644
> > index 000000000000..7f4e1a151d2b
> > --- /dev/null
> > +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c
> > @@ -0,0 +1,444 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +/*
> > + * Copyright 2022 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/imx8mp-power.h>
> > +
> > +#define GPR_REG0		0x0
> > +#define  PCIE_CLOCK_MODULE_EN	BIT(0)
> > +#define  USB_CLOCK_MODULE_EN	BIT(1)
> > +
> > +struct imx8mp_hsio_blk_ctrl_domain;
> > +
> > +struct imx8mp_hsio_blk_ctrl {
> > +	struct device *dev;
> > +	struct notifier_block power_nb;
> > +	struct device *bus_power_dev;
> > +	struct regmap *regmap;
> > +	struct imx8mp_hsio_blk_ctrl_domain *domains;
> > +	struct genpd_onecell_data onecell_data;
> > +};
> > +
> > +struct imx8mp_hsio_blk_ctrl_domain_data {
> > +	const char *name;
> > +	const char *clk_name;
> > +	const char *gpc_name;
> > +};
> > +
> > +struct imx8mp_hsio_blk_ctrl_domain {
> > +	struct generic_pm_domain genpd;
> > +	struct clk *clk;
> > +	struct device *power_dev;
> > +	struct imx8mp_hsio_blk_ctrl *bc;
> > +	int id;
> > +};
> > +
> > +static inline struct imx8mp_hsio_blk_ctrl_domain *
> > +to_imx8mp_hsio_blk_ctrl_domain(struct generic_pm_domain *genpd)
> > +{
> > +	return container_of(genpd, struct imx8mp_hsio_blk_ctrl_domain, genpd);
> > +}
> > +
> > +static int imx8mp_hsio_blk_ctrl_power_on(struct generic_pm_domain *genpd)
> > +{
> > +	struct imx8mp_hsio_blk_ctrl_domain *domain =
> > +			to_imx8mp_hsio_blk_ctrl_domain(genpd);
> > +	struct imx8mp_hsio_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;
> > +	}
> > +
> > +	/* enable upstream and blk-ctrl clocks */
> > +	ret = clk_prepare_enable(domain->clk);
> > +	if (ret) {
> > +		dev_err(bc->dev, "failed to enable clocks\n");
> > +		goto bus_put;
> > +	}
> > +
> > +	switch (domain->id) {
> > +	case IMX8MP_HSIOBLK_PD_USB:
> > +		regmap_set_bits(bc->regmap, GPR_REG0, USB_CLOCK_MODULE_EN);
> > +		break;
> > +	case IMX8MP_HSIOBLK_PD_PCIE:
> > +		regmap_set_bits(bc->regmap, GPR_REG0, PCIE_CLOCK_MODULE_EN);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	/* 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;
> > +	}
> > +
> > +	return 0;
> > +
> > +clk_disable:
> > +	clk_disable_unprepare(domain->clk);
> > +bus_put:
> > +	pm_runtime_put(bc->bus_power_dev);
> > +
> > +	return ret;
> > +}
> > +
> > +static int imx8mp_hsio_blk_ctrl_power_off(struct generic_pm_domain *genpd)
> > +{
> > +	struct imx8mp_hsio_blk_ctrl_domain *domain =
> > +			to_imx8mp_hsio_blk_ctrl_domain(genpd);
> > +	struct imx8mp_hsio_blk_ctrl *bc = domain->bc;
> > +
> > +	/* disable clocks */
> > +	switch (domain->id) {
> > +	case IMX8MP_HSIOBLK_PD_USB:
> > +		regmap_clear_bits(bc->regmap, GPR_REG0, USB_CLOCK_MODULE_EN);
> > +		break;
> > +	case IMX8MP_HSIOBLK_PD_PCIE:
> > +		regmap_clear_bits(bc->regmap, GPR_REG0, PCIE_CLOCK_MODULE_EN);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	clk_disable_unprepare(domain->clk);
> > +
> > +	/* 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 const struct imx8mp_hsio_blk_ctrl_domain_data imx8mp_hsio_domain_data[] = {
> > +	[IMX8MP_HSIOBLK_PD_USB] = {
> > +		.name = "hsioblk-usb",
> > +		.clk_name = "usb",
> > +		.gpc_name = "usb",
> > +	},
> > +	[IMX8MP_HSIOBLK_PD_USB_PHY1] = {
> > +		.name = "hsioblk-ubs-phy1",
> > +		.gpc_name = "usb-phy1",
> > +	},
> > +	[IMX8MP_HSIOBLK_PD_USB_PHY2] = {
> > +		.name = "hsioblk-ubs-phy2",
> > +		.gpc_name = "usb-phy2",
> > +	},
> > +	[IMX8MP_HSIOBLK_PD_PCIE] = {
> > +		.name = "hsioblk-pcie",
> > +		.clk_name = "pcie",
> > +		.gpc_name = "pcie",
> > +	},
> > +	[IMX8MP_HSIOBLK_PD_PCIE_PHY] = {
> > +		.name = "hsioblk-pcie-phy",
> > +		.gpc_name = "pcie-phy",
> > +	},
> > +};
> > +
> > +static int imx8mp_hsio_power_notifier(struct notifier_block *nb,
> > +				      unsigned long action, void *data)
> > +{
> > +	struct imx8mp_hsio_blk_ctrl *bc = container_of(nb, struct imx8mp_hsio_blk_ctrl,
> > +						 power_nb);
> > +	struct clk *usb_clk = bc->domains[IMX8MP_HSIOBLK_PD_USB].clk;
> > +	int ret;
> > +
> > +	switch (action) {
> > +	case GENPD_NOTIFY_ON:
> > +		/*
> > +		 * enable USB clock for a moment for the power-on ADB handshake
> > +		 * to proceed
> > +		 */
> > +		ret = clk_prepare_enable(usb_clk);
> > +		if (ret)
> > +			return NOTIFY_BAD;
> > +		regmap_set_bits(bc->regmap, GPR_REG0, USB_CLOCK_MODULE_EN);
> > +
> > +		udelay(5);
> > +
> > +		regmap_clear_bits(bc->regmap, GPR_REG0, USB_CLOCK_MODULE_EN);
> > +		clk_disable_unprepare(usb_clk);
> > +		break;
> > +	case GENPD_NOTIFY_PRE_OFF:
> > +		/* enable USB clock for the power-down ADB handshake to work */
> > +		ret = clk_prepare_enable(usb_clk);
> > +		if (ret)
> > +			return NOTIFY_BAD;
> > +
> > +		regmap_set_bits(bc->regmap, GPR_REG0, USB_CLOCK_MODULE_EN);
> > +		break;
> > +	case GENPD_NOTIFY_OFF:
> > +		clk_disable_unprepare(usb_clk);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> > +static int imx8mp_hsio_blk_ctrl_probe(struct platform_device *pdev)
> > +{
> > +	int num_domains = ARRAY_SIZE(imx8mp_hsio_domain_data);
> > +	struct device *dev = &pdev->dev;
> > +	struct imx8mp_hsio_blk_ctrl *bc;
> > +	void __iomem *base;
> > +	int i, ret;
> > +
> > +	struct regmap_config regmap_config = {
> > +		.reg_bits	= 32,
> > +		.val_bits	= 32,
> > +		.reg_stride	= 4,
> > +		.max_register	= 0x24,
> > +	};
> > +
> > +	bc = devm_kzalloc(dev, sizeof(*bc), GFP_KERNEL);
> > +	if (!bc)
> > +		return -ENOMEM;
> > +
> > +	bc->dev = dev;
> > +
> > +	base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	bc->regmap = devm_regmap_init_mmio(dev, base, &regmap_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, num_domains,
> > +				   sizeof(struct imx8mp_hsio_blk_ctrl_domain),
> > +				   GFP_KERNEL);
> > +	if (!bc->domains)
> > +		return -ENOMEM;
> > +
> > +	bc->onecell_data.num_domains = num_domains;
> > +	bc->onecell_data.xlate = imx8m_blk_ctrl_xlate;
> > +	bc->onecell_data.domains =
> > +		devm_kcalloc(dev, 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 < num_domains; i++) {
> > +		const struct imx8mp_hsio_blk_ctrl_domain_data *data =
> > +				&imx8mp_hsio_domain_data[i];
> > +		struct imx8mp_hsio_blk_ctrl_domain *domain = &bc->domains[i];
> > +
> > +		if (data->clk_name) {
> > +			domain->clk = devm_clk_get(dev, data->clk_name);
> > +			if (IS_ERR(domain->clk)) {
> > +				ret = PTR_ERR(domain->clk);
> > +				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 = imx8mp_hsio_blk_ctrl_power_on;
> > +		domain->genpd.power_off = imx8mp_hsio_blk_ctrl_power_off;
> > +		domain->bc = bc;
> > +		domain->id = i;
> > +
> > +		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 = imx8mp_hsio_power_notifier;
> > +	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 imx8mp_hsio_blk_ctrl_remove(struct platform_device *pdev)
> > +{
> > +	struct imx8mp_hsio_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 imx8mp_hsio_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 imx8mp_hsio_blk_ctrl_suspend(struct device *dev)
> > +{
> > +	struct imx8mp_hsio_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 imx8mp_hsio_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 imx8mp_hsio_blk_ctrl_resume(struct device *dev)
> > +{
> > +	struct imx8mp_hsio_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 imx8mp_hsio_blk_ctrl_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(imx8mp_hsio_blk_ctrl_suspend,
> > +				imx8mp_hsio_blk_ctrl_resume)
> > +};
> > +
> > +static const struct of_device_id imx8mp_hsio_blk_ctrl_of_match[] = {
> > +	{
> > +		.compatible = "fsl,imx8mp-hsio-blk-ctrl",
> > +	}, {
> > +		/* Sentinel */
> > +	}
> > +};
> > +MODULE_DEVICE_TABLE(of, imx8m_blk_ctrl_of_match);
> > +
> > +static struct platform_driver imx8mp_hsio_blk_ctrl_driver = {
> > +	.probe = imx8mp_hsio_blk_ctrl_probe,
> > +	.remove = imx8mp_hsio_blk_ctrl_remove,
> > +	.driver = {
> > +		.name = "imx8mp-hsio-blk-ctrl",
> > +		.pm = &imx8mp_hsio_blk_ctrl_pm_ops,
> > +		.of_match_table = imx8mp_hsio_blk_ctrl_of_match,
> > +	},
> > +};
> > +module_platform_driver(imx8mp_hsio_blk_ctrl_driver);
> > -- 
> > 2.30.2
> >
Alexander Stein Jan. 26, 2022, 1:51 p.m. UTC | #5
Am Mittwoch, 19. Januar 2022, 14:40:27 CET schrieb Lucas Stach:
> Add the DT nodes for both the 3D and 2D GPU cores found on the i.MX8MP.
> 
> etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6204
> etnaviv-gpu 38008000.gpu: model: GC520, revision: 5341
> [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0

Unfortunately it does not work when CONFIG_DRM_ETNAVIV=m
etnaviv-gpu 38000000.gpu: model: GC0, revision: 0
etnaviv-gpu 38000000.gpu: Unknown GPU model
etnaviv-gpu 38008000.gpu: model: GC0, revision: 0
etnaviv-gpu 38008000.gpu: Unknown GPU model

When I use CONFIG_DRM_ETNAVIV=y, I get the same log message as you. It's not 
related to this patch, but I have no clue if the cause is in blk-ctrl or pgc.

I think (don't know for sure yet) my random errors on USB side are gone when 
USB drivers (PHY as well) are built-in. But I might be wrong here.

Best regards,
Alexander
Alexander Stein Jan. 26, 2022, 2:02 p.m. UTC | #6
Hi Lucas,

Am Mittwoch, 19. Januar 2022, 14:40:25 CET schrieb Lucas Stach:
> This adds the GPC and HSIO blk-ctrl nodes providing power control for
> the high-speed (USB and PCIe) IOs.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

$ make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/usb/
fsl,imx8mp-dwc3.yaml gives the warnings:
> usb@32f10100: 'power-domains' does not match any of the regexes: 
'^usb@[0-9a-f]+$', 'pinctrl-[0-9]+'
>         From schema: linux/Documentation/devicetree/bindings/usb/fsl,imx8mp-
dwc3.yaml
> usb@32f10108: 'power-domains' does not match any of the regexes: 
'^usb@[0-9a-f]+$', 'pinctrl-[0-9]+'
>         From schema: linux/Documentation/devicetree/bindings/usb/fsl,imx8mp-
dwc3.yaml

Alexander

> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 63 ++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index
> 04d259de5667..b76af96b9b5c 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -4,6 +4,7 @@
>   */
> 
>  #include <dt-bindings/clock/imx8mp-clock.h>
> +#include <dt-bindings/power/imx8mp-power.h>
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> @@ -443,6 +444,44 @@ src: reset-controller@30390000 {
>  				interrupts = <GIC_SPI 89 
IRQ_TYPE_LEVEL_HIGH>;
>  				#reset-cells = <1>;
>  			};
> +
> +			gpc: gpc@303a0000 {
> +				compatible = "fsl,imx8mp-gpc";
> +				reg = <0x303a0000 0x10000>;
> +				interrupt-parent = <&gic>;
> +				interrupt-controller;
> +				#interrupt-cells = <3>;
> +
> +				pgc {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					pgc_pcie_phy: power-
domain@1 {
> +						#power-
domain-cells = <0>;
> +						reg = 
<IMX8MP_POWER_DOMAIN_PCIE_PHY>;
> +					};
> +
> +					pgc_usb1_phy: power-
domain@2 {
> +						#power-
domain-cells = <0>;
> +						reg = 
<IMX8MP_POWER_DOMAIN_USB1_PHY>;
> +					};
> +
> +					pgc_usb2_phy: power-
domain@3 {
> +						#power-
domain-cells = <0>;
> +						reg = 
<IMX8MP_POWER_DOMAIN_USB2_PHY>;
> +					};
> +
> +					pgc_hsiomix: power-
domains@17 {
> +						#power-
domain-cells = <0>;
> +						reg = 
<IMX8MP_POWER_DOMAIN_HSIOMIX>;
> +						clocks = 
<&clk IMX8MP_CLK_HSIO_AXI>,
> +							 
<&clk IMX8MP_CLK_HSIO_ROOT>;
> +						assigned-
clocks = <&clk IMX8MP_CLK_HSIO_AXI>;
> +						assigned-
clock-parents = <&clk IMX8MP_SYS_PLL2_500M>;
> +						assigned-
clock-rates = <500000000>;
> +					};
> +				};
> +			};
>  		};
> 
>  		aips2: bus@30400000 {
> @@ -875,6 +914,20 @@ ddr-pmu@3d800000 {
>  			interrupts = <GIC_SPI 98 
IRQ_TYPE_LEVEL_HIGH>;
>  		};
> 
> +		hsio_blk_ctrl: blk-ctrl@32f10000 {
> +			compatible = "fsl,imx8mp-hsio-blk-ctrl", 
"syscon";
> +			reg = <0x32f10000 0x24>;
> +			clocks = <&clk IMX8MP_CLK_USB_ROOT>,
> +				 <&clk IMX8MP_CLK_PCIE_ROOT>;
> +			clock-names = "usb", "pcie";
> +			power-domains = <&pgc_hsiomix>, 
<&pgc_hsiomix>,
> +					<&pgc_usb1_phy>, 
<&pgc_usb2_phy>,
> +					<&pgc_hsiomix>, 
<&pgc_pcie_phy>;
> +			power-domain-names = "bus", "usb", "usb-
phy1",
> +					     "usb-phy2", 
"pcie", "pcie-phy";
> +			#power-domain-cells = <1>;
> +		};
> +
>  		usb3_phy0: usb-phy@381f0040 {
>  			compatible = "fsl,imx8mp-usb-phy";
>  			reg = <0x381f0040 0x40>;
> @@ -882,6 +935,7 @@ usb3_phy0: usb-phy@381f0040 {
>  			clock-names = "phy";
>  			assigned-clocks = <&clk 
IMX8MP_CLK_USB_PHY_REF>;
>  			assigned-clock-parents = <&clk 
IMX8MP_CLK_24M>;
> +			power-domains = <&hsio_blk_ctrl 
IMX8MP_HSIOBLK_PD_USB_PHY1>;
>  			#phy-cells = <0>;
>  			status = "disabled";
>  		};
> @@ -893,6 +947,7 @@ usb3_0: usb@32f10100 {
>  				 <&clk IMX8MP_CLK_USB_ROOT>;
>  			clock-names = "hsio", "suspend";
>  			interrupts = <GIC_SPI 148 
IRQ_TYPE_LEVEL_HIGH>;
> +			power-domains = <&hsio_blk_ctrl 
IMX8MP_HSIOBLK_PD_USB>;
>  			#address-cells = <1>;
>  			#size-cells = <1>;
>  			dma-ranges = <0x40000000 0x40000000 
0xc0000000>;
> @@ -906,9 +961,6 @@ usb_dwc3_0: usb@38100000 {
>  					 <&clk 
IMX8MP_CLK_USB_CORE_REF>,
>  					 <&clk 
IMX8MP_CLK_USB_ROOT>;
>  				clock-names = "bus_early", "ref", 
"suspend";
> -				assigned-clocks = <&clk 
IMX8MP_CLK_HSIO_AXI>;
> -				assigned-clock-parents = <&clk 
IMX8MP_SYS_PLL2_500M>;
> -				assigned-clock-rates = 
<500000000>;
>  				interrupts = <GIC_SPI 40 
IRQ_TYPE_LEVEL_HIGH>;
>  				phys = <&usb3_phy0>, <&usb3_phy0>;
>  				phy-names = "usb2-phy", "usb3-
phy";
> @@ -924,6 +976,7 @@ usb3_phy1: usb-phy@382f0040 {
>  			clock-names = "phy";
>  			assigned-clocks = <&clk 
IMX8MP_CLK_USB_PHY_REF>;
>  			assigned-clock-parents = <&clk 
IMX8MP_CLK_24M>;
> +			power-domains = <&hsio_blk_ctrl 
IMX8MP_HSIOBLK_PD_USB_PHY2>;
>  			#phy-cells = <0>;
>  		};
> 
> @@ -934,6 +987,7 @@ usb3_1: usb@32f10108 {
>  				 <&clk IMX8MP_CLK_USB_ROOT>;
>  			clock-names = "hsio", "suspend";
>  			interrupts = <GIC_SPI 149 
IRQ_TYPE_LEVEL_HIGH>;
> +			power-domains = <&hsio_blk_ctrl 
IMX8MP_HSIOBLK_PD_USB>;
>  			#address-cells = <1>;
>  			#size-cells = <1>;
>  			dma-ranges = <0x40000000 0x40000000 
0xc0000000>;
> @@ -947,9 +1001,6 @@ usb_dwc3_1: usb@38200000 {
>  					 <&clk 
IMX8MP_CLK_USB_CORE_REF>,
>  					 <&clk 
IMX8MP_CLK_USB_ROOT>;
>  				clock-names = "bus_early", "ref", 
"suspend";
> -				assigned-clocks = <&clk 
IMX8MP_CLK_HSIO_AXI>;
> -				assigned-clock-parents = <&clk 
IMX8MP_SYS_PLL2_500M>;
> -				assigned-clock-rates = 
<500000000>;
>  				interrupts = <GIC_SPI 41 
IRQ_TYPE_LEVEL_HIGH>;
>  				phys = <&usb3_phy1>, <&usb3_phy1>;
>  				phy-names = "usb2-phy", "usb3-
phy";
Lucas Stach Feb. 7, 2022, 7:27 p.m. UTC | #7
Hi Alexander,

Am Mittwoch, dem 26.01.2022 um 14:51 +0100 schrieb Alexander Stein:
> Am Mittwoch, 19. Januar 2022, 14:40:27 CET schrieb Lucas Stach:
> > Add the DT nodes for both the 3D and 2D GPU cores found on the i.MX8MP.
> > 
> > etnaviv-gpu 38000000.gpu: model: GC7000, revision: 6204
> > etnaviv-gpu 38008000.gpu: model: GC520, revision: 5341
> > [drm] Initialized etnaviv 1.3.0 20151214 for etnaviv on minor 0
> 
> Unfortunately it does not work when CONFIG_DRM_ETNAVIV=m
> etnaviv-gpu 38000000.gpu: model: GC0, revision: 0
> etnaviv-gpu 38000000.gpu: Unknown GPU model
> etnaviv-gpu 38008000.gpu: model: GC0, revision: 0
> etnaviv-gpu 38008000.gpu: Unknown GPU model
> 
> When I use CONFIG_DRM_ETNAVIV=y, I get the same log message as you. It's not 
> related to this patch, but I have no clue if the cause is in blk-ctrl or pgc.

Thanks for the report. This was caused by some wrong clock handles in
the GPU and GPC nodes. Fixed in v2.

Regards,
Lucas