From: Dong Aisheng <dongas86@gmail.com> To: Lucas Stach <l.stach@pengutronix.de> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>, Shawn Guo <shawnguo@kernel.org>, yurovsky@gmail.com, Fabio Estevam <fabio.estevam@nxp.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver Date: Thu, 30 Mar 2017 15:51:11 +0800 [thread overview] Message-ID: <20170330075111.GC29432@b29396-OptiPlex-7040> (raw) In-Reply-To: <1490279749.29056.33.camel@pengutronix.de> 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 = <®_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 >
WARNING: multiple messages have this Message-ID (diff)
From: dongas86@gmail.com (Dong Aisheng) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v7 2/2] soc/imx: Add GPCv2 power gating driver Date: Thu, 30 Mar 2017 15:51:11 +0800 [thread overview] Message-ID: <20170330075111.GC29432@b29396-OptiPlex-7040> (raw) In-Reply-To: <1490279749.29056.33.camel@pengutronix.de> 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 at 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 at IMX7_POWER_DOMAIN_PCIE_PHY { reg = <IMX7_POWER_DOMAIN_PCIE_PHY>; power-supply = <®_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 >
next prev parent reply other threads:[~2017-03-29 15:54 UTC|newest] Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-03-21 14:50 [PATCH v7 0/2] GPCv2 power gating driver Andrey Smirnov 2017-03-21 14:50 ` Andrey Smirnov 2017-03-21 14:50 ` [PATCH v7 1/2] dt-bindings: Add " Andrey Smirnov 2017-03-21 14:50 ` Andrey Smirnov 2017-03-24 6:32 ` Dong Aisheng 2017-03-24 6:32 ` Dong Aisheng 2017-03-27 18:42 ` Andrey Smirnov 2017-03-27 18:42 ` Andrey Smirnov 2017-03-27 18:42 ` Andrey Smirnov 2017-03-30 7:15 ` Dong Aisheng 2017-03-30 7:15 ` Dong Aisheng 2017-03-21 14:50 ` [PATCH v7 2/2] soc/imx: " Andrey Smirnov 2017-03-21 14:50 ` Andrey Smirnov 2017-03-24 6:24 ` Dong Aisheng 2017-03-24 6:24 ` Dong Aisheng 2017-03-23 14:35 ` Lucas Stach 2017-03-23 14:35 ` Lucas Stach 2017-03-30 7:51 ` Dong Aisheng [this message] 2017-03-30 7:51 ` Dong Aisheng 2017-03-29 16:08 ` Lucas Stach 2017-03-29 16:08 ` Lucas Stach 2017-04-01 4:10 ` Dong Aisheng 2017-04-01 4:10 ` Dong Aisheng 2017-03-31 12:28 ` Lucas Stach 2017-03-31 12:28 ` Lucas Stach 2017-04-11 3:22 ` Dong Aisheng 2017-04-11 3:22 ` Dong Aisheng 2017-03-27 18:42 ` Andrey Smirnov 2017-03-27 18:42 ` Andrey Smirnov 2017-03-30 6:58 ` Dong Aisheng 2017-03-30 6:58 ` Dong Aisheng 2017-03-30 7:04 ` Dong Aisheng 2017-03-30 7:04 ` Dong Aisheng
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170330075111.GC29432@b29396-OptiPlex-7040 \ --to=dongas86@gmail.com \ --cc=andrew.smirnov@gmail.com \ --cc=fabio.estevam@nxp.com \ --cc=l.stach@pengutronix.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=shawnguo@kernel.org \ --cc=yurovsky@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.