Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670198 > unrolled thread

Re: [PATCH 9/9] clk: imx: add imx7ulp clk driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 9/9] clk: imx: add imx7ulp clk driver Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 04:10 +0200
    Re: [PATCH 9/9] clk: imx: add imx7ulp clk driver Dong Aisheng <dongas86@gmail.com> - 2017-06-20 11:50 +0200
      Re: [PATCH 9/9] clk: imx: add imx7ulp clk driver Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 22:50 +0200
        RE: [PATCH 9/9] clk: imx: add imx7ulp clk driver "A.s. Dong" <aisheng.dong@nxp.com> - 2017-06-21 09:20 +0200
          Re: [PATCH 9/9] clk: imx: add imx7ulp clk driver Stephen Boyd <sboyd@codeaurora.org> - 2017-07-01 02:40 +0200
            RE: [PATCH 9/9] clk: imx: add imx7ulp clk driver "A.s. Dong" <aisheng.dong@nxp.com> - 2017-07-03 05:20 +0200

#1670198 — Re: [PATCH 9/9] clk: imx: add imx7ulp clk driver

On 05/15, Dong Aisheng wrote:
> +
> +	clks[IMX7ULP_CLK_VIU]		= imx_clk_gate("viu",	"nic1_clk",	base + 0xA0, 30);
> +	clks[IMX7ULP_CLK_PCTLC]		= imx_clk_gate("pctlc", "nic1_bus_clk", base + 0xB8, 30);
> +	clks[IMX7ULP_CLK_PCTLD]		= imx_clk_gate("pctld", "nic1_bus_clk", base + 0xBC, 30);
> +	clks[IMX7ULP_CLK_PCTLE]		= imx_clk_gate("pctle", "nic1_bus_clk", base + 0xc0, 30);
> +	clks[IMX7ULP_CLK_PCTLF]		= imx_clk_gate("pctlf", "nic1_bus_clk", base + 0xc4, 30);
> +
> +	clks[IMX7ULP_CLK_GPU3D]		= imx_clk_composite("gpu3d",   periph_plat_sels, ARRAY_SIZE(periph_plat_sels), true, false, true, base + 0x140);
> +	clks[IMX7ULP_CLK_GPU2D]		= imx_clk_composite("gpu2d",   periph_plat_sels, ARRAY_SIZE(periph_plat_sels), true, false, true, base + 0x144);
> +
> +	imx_check_clocks(clks, ARRAY_SIZE(clks));
> +
> +	clk_data.clks = clks;
> +	clk_data.clk_num = ARRAY_SIZE(clks);
> +	of_clk_add_provider(scg_node, of_clk_src_onecell_get, &clk_data);

Please use of_clk_add_hw_provider() instead, and the associated
clk_hw registration APIs.

> +
> +	pr_info("i.MX7ULP clock tree init done.\n");

pr_debug?

> +}
> +
> +CLK_OF_DECLARE(imx7ulp, "fsl,imx7ulp-clock", imx7ulp_clocks_init);
> 

Any reason why it can't be a platform driver? If not, please add
some comment explaining why.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1670588

On Mon, Jun 19, 2017 at 07:01:19PM -0700, Stephen Boyd wrote:
> On 05/15, Dong Aisheng wrote:
> > +
> > +	clks[IMX7ULP_CLK_VIU]		= imx_clk_gate("viu",	"nic1_clk",	base + 0xA0, 30);
> > +	clks[IMX7ULP_CLK_PCTLC]		= imx_clk_gate("pctlc", "nic1_bus_clk", base + 0xB8, 30);
> > +	clks[IMX7ULP_CLK_PCTLD]		= imx_clk_gate("pctld", "nic1_bus_clk", base + 0xBC, 30);
> > +	clks[IMX7ULP_CLK_PCTLE]		= imx_clk_gate("pctle", "nic1_bus_clk", base + 0xc0, 30);
> > +	clks[IMX7ULP_CLK_PCTLF]		= imx_clk_gate("pctlf", "nic1_bus_clk", base + 0xc4, 30);
> > +
> > +	clks[IMX7ULP_CLK_GPU3D]		= imx_clk_composite("gpu3d",   periph_plat_sels, ARRAY_SIZE(periph_plat_sels), true, false, true, base + 0x140);
> > +	clks[IMX7ULP_CLK_GPU2D]		= imx_clk_composite("gpu2d",   periph_plat_sels, ARRAY_SIZE(periph_plat_sels), true, false, true, base + 0x144);
> > +
> > +	imx_check_clocks(clks, ARRAY_SIZE(clks));
> > +
> > +	clk_data.clks = clks;
> > +	clk_data.clk_num = ARRAY_SIZE(clks);
> > +	of_clk_add_provider(scg_node, of_clk_src_onecell_get, &clk_data);
> 
> Please use of_clk_add_hw_provider() instead, and the associated
> clk_hw registration APIs.
> 

Sure, will do it.

> > +
> > +	pr_info("i.MX7ULP clock tree init done.\n");
> 
> pr_debug?
> 

Yes

> > +}
> > +
> > +CLK_OF_DECLARE(imx7ulp, "fsl,imx7ulp-clock", imx7ulp_clocks_init);
> > 
> 
> Any reason why it can't be a platform driver? If not, please add
> some comment explaining why.
> 

Timer is using it at early stage. GIC seems not although standard
binding claim possible clock requirement.
Others still not sure.

What your suggestion?
Convert timer to platform driver and make clock as platform driver as well?

Regards
Dong Aisheng

> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1671102

On 06/20, Dong Aisheng wrote:
> On Mon, Jun 19, 2017 at 07:01:19PM -0700, Stephen Boyd wrote:
> > 
> > Any reason why it can't be a platform driver? If not, please add
> > some comment explaining why.
> > 
> 
> Timer is using it at early stage. GIC seems not although standard
> binding claim possible clock requirement.
> Others still not sure.
> 
> What your suggestion?
> Convert timer to platform driver and make clock as platform driver as well?
> 

The timer can't be a platform driver because it would be too
late. The clock driver could register whatever clks are required
for the timer/GIC in a CLK_OF_DECLARE_DRIVER hook, and then leave
the rest to a platform driver. This way we get some of the device
driver framework in this code.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1671418

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@codeaurora.org]
> Sent: Wednesday, June 21, 2017 4:42 AM
> To: Dong Aisheng
> Cc: A.s. Dong; linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com;
> shawnguo@kernel.org; Anson Huang; Jacky Bai
> Subject: Re: [PATCH 9/9] clk: imx: add imx7ulp clk driver
> 
> On 06/20, Dong Aisheng wrote:
> > On Mon, Jun 19, 2017 at 07:01:19PM -0700, Stephen Boyd wrote:
> > >
> > > Any reason why it can't be a platform driver? If not, please add
> > > some comment explaining why.
> > >
> >
> > Timer is using it at early stage. GIC seems not although standard
> > binding claim possible clock requirement.
> > Others still not sure.
> >
> > What your suggestion?
> > Convert timer to platform driver and make clock as platform driver as
> well?
> >
> 
> The timer can't be a platform driver because it would be too late. The
> clock driver could register whatever clks are required for the timer/GIC
> in a CLK_OF_DECLARE_DRIVER hook, and then leave the rest to a platform
> driver. This way we get some of the device driver framework in this code.
> 

Okay, I could try it. Thanks.

One thing is that TPM clock has a lot parents and parents having parents,
as well as PIT timer. So I may need enable more than half clocks in
CLK_OF_DECLARE_DRIVER hook.

BTW, What's benefit to convert into two parts of probe?
I'm not quite if I already get it all, can you help clarify it?

Regards
Dong Aisheng

> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1679221

On 06/21, A.s. Dong wrote:
> > -----Original Message-----
> > From: Stephen Boyd [mailto:sboyd@codeaurora.org]
> > Sent: Wednesday, June 21, 2017 4:42 AM
> > To: Dong Aisheng
> > Cc: A.s. Dong; linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com;
> > shawnguo@kernel.org; Anson Huang; Jacky Bai
> > Subject: Re: [PATCH 9/9] clk: imx: add imx7ulp clk driver
> > 
> > On 06/20, Dong Aisheng wrote:
> > > On Mon, Jun 19, 2017 at 07:01:19PM -0700, Stephen Boyd wrote:
> > > >
> > > > Any reason why it can't be a platform driver? If not, please add
> > > > some comment explaining why.
> > > >
> > >
> > > Timer is using it at early stage. GIC seems not although standard
> > > binding claim possible clock requirement.
> > > Others still not sure.
> > >
> > > What your suggestion?
> > > Convert timer to platform driver and make clock as platform driver as
> > well?
> > >
> > 
> > The timer can't be a platform driver because it would be too late. The
> > clock driver could register whatever clks are required for the timer/GIC
> > in a CLK_OF_DECLARE_DRIVER hook, and then leave the rest to a platform
> > driver. This way we get some of the device driver framework in this code.
> > 
> 
> Okay, I could try it. Thanks.
> 
> One thing is that TPM clock has a lot parents and parents having parents,
> as well as PIT timer. So I may need enable more than half clocks in
> CLK_OF_DECLARE_DRIVER hook.

That's fine.

> 
> BTW, What's benefit to convert into two parts of probe?
> I'm not quite if I already get it all, can you help clarify it?
> 

The benefit is that we still get a platform driver and we can
associate a device pointer with the clock controller eventually.
Here's a reply I sent yesterday on the same topic:

Reasons (in no particular order):

  1. We get a dev pointer to use with clk_hw_register()

  2. We can handle probe defer if some resource is not available

  3. Using device model gets us a hook into power management frameworks
     like runtime PM and system PM for things like suspend and hibernate

  4. It encourages a single DT node clk controller style binding
     instead of a single node per clk style binding

  5. We can use non-DT specific functions like devm_ioremap_resource() to map
     registers and acquire other resources, leading to more portable and
     generic code

  6. We may be able to make the device driver a module, which will
     make distros happy if we don't have to compile in all
     these clk drivers to the resulting vmlinux (this one doesn't
     apply here)

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1679618

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@codeaurora.org]
> Sent: Saturday, July 01, 2017 8:35 AM
> To: A.s. Dong
> Cc: Dong Aisheng; linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com;
> shawnguo@kernel.org; Anson Huang; Jacky Bai
> Subject: Re: [PATCH 9/9] clk: imx: add imx7ulp clk driver
> 
> On 06/21, A.s. Dong wrote:
> > > -----Original Message-----
> > > From: Stephen Boyd [mailto:sboyd@codeaurora.org]
> > > Sent: Wednesday, June 21, 2017 4:42 AM
> > > To: Dong Aisheng
> > > Cc: A.s. Dong; linux-clk@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > mturquette@baylibre.com; shawnguo@kernel.org; Anson Huang; Jacky Bai
> > > Subject: Re: [PATCH 9/9] clk: imx: add imx7ulp clk driver
> > >
> > > On 06/20, Dong Aisheng wrote:
> > > > On Mon, Jun 19, 2017 at 07:01:19PM -0700, Stephen Boyd wrote:
> > > > >
> > > > > Any reason why it can't be a platform driver? If not, please add
> > > > > some comment explaining why.
> > > > >
> > > >
> > > > Timer is using it at early stage. GIC seems not although standard
> > > > binding claim possible clock requirement.
> > > > Others still not sure.
> > > >
> > > > What your suggestion?
> > > > Convert timer to platform driver and make clock as platform driver
> > > > as
> > > well?
> > > >
> > >
> > > The timer can't be a platform driver because it would be too late.
> > > The clock driver could register whatever clks are required for the
> > > timer/GIC in a CLK_OF_DECLARE_DRIVER hook, and then leave the rest
> > > to a platform driver. This way we get some of the device driver
> framework in this code.
> > >
> >
> > Okay, I could try it. Thanks.
> >
> > One thing is that TPM clock has a lot parents and parents having
> > parents, as well as PIT timer. So I may need enable more than half
> > clocks in CLK_OF_DECLARE_DRIVER hook.
> 
> That's fine.
> 
> >
> > BTW, What's benefit to convert into two parts of probe?
> > I'm not quite if I already get it all, can you help clarify it?
> >
> 
> The benefit is that we still get a platform driver and we can associate a
> device pointer with the clock controller eventually.
> Here's a reply I sent yesterday on the same topic:
> 
> Reasons (in no particular order):
> 
>   1. We get a dev pointer to use with clk_hw_register()
> 
>   2. We can handle probe defer if some resource is not available
> 
>   3. Using device model gets us a hook into power management frameworks
>      like runtime PM and system PM for things like suspend and hibernate
> 
>   4. It encourages a single DT node clk controller style binding
>      instead of a single node per clk style binding
> 
>   5. We can use non-DT specific functions like devm_ioremap_resource() to
> map
>      registers and acquire other resources, leading to more portable and
>      generic code
> 
>   6. We may be able to make the device driver a module, which will
>      make distros happy if we don't have to compile in all
>      these clk drivers to the resulting vmlinux (this one doesn't
>      apply here)
> 

Very clear.
Thanks for the great explanation.

Regards
Dong Aisheng

> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web