Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver

From: Dong Aisheng
Date: Wed Mar 29 2017 - 11:55:26 EST


Hi Lucas,

On Thu, Mar 23, 2017 at 03:35:49PM +0100, Lucas Stach wrote:
> Hi Dong,
>
> Am Freitag, den 24.03.2017, 14:24 +0800 schrieb Dong Aisheng:
> [...]
> > > +static struct platform_driver imx7_pgc_domain_driver = {
> > > + .driver = {
> > > + .name = "imx7-pgc",
> > > + },
> > > + .probe = imx7_pgc_domain_probe,
> > > + .remove = imx7_pgc_domain_remove,
> > > + .id_table = imx7_pgc_domain_id,
> > > +};
> > > +builtin_platform_driver(imx7_pgc_domain_driver)
> >
> > Again, i have a fundamental question about this patch implementation
> > that why we choose above way to register the power domain?
> >
> > I'm sorry that i did not know too much history.
> > Would you guys please help share some information?
> >
> > Because AFAIK this way will register each domain as a power domain
> > provider which is a bit violate the real HW and current power domain
> > framework design. And it is a bit more complicated to use than before.
> >
> > IMHO i would rather prefer the old traditional and simpler way that one
> > provider (GPC) supplies multiple domains (PCIE/MIPI/HSIC PHY domain)
> > than this patch does.
> >
> > However, i might be wrong. Please help to clear.
>
> This way we can properly describe each power domain with the regulator
> supplying the domain and the clocks of the devices inside the domain in
> the device tree.
>

Thanks for the explaination. I understand that purpose.

Now my concern is why we doing things like this:
Builtin two platforms driver and use one to dynamically create
device to trigger another driver bind to register the domain.

static int imx7_pgc_domain_probe(struct platform_device *pdev)
{
of_genpd_add_provider_simple(domain->dev->of_node,
&domain->genpd);
}

static struct platform_driver imx7_pgc_domain_driver = {
.driver = {
.name = "imx7-pgc",
},
.probe = imx7_pgc_domain_probe,
};
builtin_platform_driver(imx7_pgc_domain_driver)


static int imx_gpcv2_probe(struct platform_device *pdev)
{

for_each_child_of_node(pgc_np, np) {
pd_pdev = platform_device_alloc("imx7-pgc-domain",
domain_index);
ret = platform_device_add(pd_pdev);
}
}

static struct platform_driver imx_gpc_driver = {
.driver = {
.name = "imx-gpcv2",
.of_match_table = imx_gpcv2_dt_ids,
},
.probe = imx_gpcv2_probe,
};
builtin_platform_driver(imx_gpc_driver)

Is there any special purpose or i missed something?

Can we just use one or a simple core_initcall(imx_gpcv2_probe) cause
this probably should be registered early for other consumers?

Personally i'd be more like Rockchip's power domain implementation.
See:
arch/arm/boot/dts/rk3288.dtsi
drivers/soc/rockchip/pm_domains.c
Dcumentation/devicetree/bindings/soc/rockchip/power_domain.txt

How about refer to the Rockchip's way?

Then it could also address our issues and the binding would be
still like:
gpc: gpc@303a0000 {
compatible = "fsl,imx7d-gpc";
reg = <0x303a0000 0x1000>;
interrupt-controller;
interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
#interrupt-cells = <3>;
interrupt-parent = <&intc>;

pgc {
#address-cells = <1>;
#size-cells = <0>;

pgc_pcie_phy: power-domain@IMX7_POWER_DOMAIN_PCIE_PHY {
reg = <IMX7_POWER_DOMAIN_PCIE_PHY>;
power-supply = <&reg_1p0d>;
clocks = <xxx>;
};

....
};
};

It also drops #power-domain-cells and register domain by
one provider with multi domains which is more align with HW.

How do you think of it?

Regards
Dong Aisheng

> This is needed as for the upstream version we are controlling the
> regulator from the GPC driver, as opposed to the downstream version,
> where each device has to implement the regulator handling and power
> up/down sequencing.
>
> See the rationale in the commits adding the multidomain support to the
> i.MX6 GPC.
>
> Regards,
> Lucas
>