[PATCH] clk: qcom: gcc: Fix board clock node name

[PATCH] clk: qcom: gcc: Fix board clock node name

[Vinod Koul]

Device tree node name are not supposed to have "_" in them so fix the
node name use of xo_board to xo-board

Fixes: 652f1813c113 ("clk: qcom: gcc: Add global clock controller driver for QCS404")
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---

Steve: RobH pointed this on DTS patches, would be great if you can pick this
as a fix

 drivers/clk/qcom/gcc-qcs404.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
index e4ca6a45f313..ef1b267cb058 100644
--- a/drivers/clk/qcom/gcc-qcs404.c
+++ b/drivers/clk/qcom/gcc-qcs404.c
@@ -265,7 +265,7 @@ static struct clk_fixed_factor cxo = {
 	.div = 1,
 	.hw.init = &(struct clk_init_data){
 		.name = "cxo",
-		.parent_names = (const char *[]){ "xo_board" },
+		.parent_names = (const char *[]){ "xo-board" },
 		.num_parents = 1,
 		.ops = &clk_fixed_factor_ops,
 	},
-- 
2.14.4

Re: [PATCH] clk: qcom: gcc: Fix board clock node name

[Stephen Boyd]

Quoting Vinod Koul (2018-11-09 01:50:54)
> Device tree node name are not supposed to have "_" in them so fix the
> node name use of xo_board to xo-board
> 
> Fixes: 652f1813c113 ("clk: qcom: gcc: Add global clock controller driver for QCS404")
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> 
> Steve: RobH pointed this on DTS patches, would be great if you can pick this
> as a fix

Ok.

> 
>  drivers/clk/qcom/gcc-qcs404.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
> index e4ca6a45f313..ef1b267cb058 100644
> --- a/drivers/clk/qcom/gcc-qcs404.c
> +++ b/drivers/clk/qcom/gcc-qcs404.c
> @@ -265,7 +265,7 @@ static struct clk_fixed_factor cxo = {
>         .div = 1,
>         .hw.init = &(struct clk_init_data){
>                 .name = "cxo",
> -               .parent_names = (const char *[]){ "xo_board" },
> +               .parent_names = (const char *[]){ "xo-board" },

We have xo_board used everywhere else in drivers/clk/qcom/ so this makes
me concerned. Wouldn't a better answer be to add clock-output-names to
the xo-board DT node and have arm-soc merge that through. I mostly want
to keep things consistent here so that if we need to inject an xo_board
clk into the system then we can do so generically instead of making it
per-platform. Of course, if we're never going to have this problem on
qcs404 then it will be fine to start differing here. So I'm leaning
towards merge this patch, just please ack my concern here and tell me it
won't be a problem and I'll be happy to merge to clk-fixes.

BTW, can you also specify a 'clocks' property in the GCC node and send
the xo_board node there? In fact, we should do that for every GCC node
in the tree. Care to do that and also add sleep_clk to each clock
controller node that uses it? This is useful to do so that we can more
easily see where clocks are going between clock controller nodes.

Re: [PATCH] clk: qcom: gcc: Fix board clock node name

[Vinod Koul]

(Add Rob & Bjorn)

Hi Steve,

On 09-11-18, 09:12, Stephen Boyd wrote:
> Quoting Vinod Koul (2018-11-09 01:50:54)
> > Device tree node name are not supposed to have "_" in them so fix the
> > node name use of xo_board to xo-board
> > 
> > Fixes: 652f1813c113 ("clk: qcom: gcc: Add global clock controller driver for QCS404")
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> > 
> > Steve: RobH pointed this on DTS patches, would be great if you can pick this
> > as a fix
> 
> Ok.
> 
> > 
> >  drivers/clk/qcom/gcc-qcs404.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
> > index e4ca6a45f313..ef1b267cb058 100644
> > --- a/drivers/clk/qcom/gcc-qcs404.c
> > +++ b/drivers/clk/qcom/gcc-qcs404.c
> > @@ -265,7 +265,7 @@ static struct clk_fixed_factor cxo = {
> >         .div = 1,
> >         .hw.init = &(struct clk_init_data){
> >                 .name = "cxo",
> > -               .parent_names = (const char *[]){ "xo_board" },
> > +               .parent_names = (const char *[]){ "xo-board" },
> 
> We have xo_board used everywhere else in drivers/clk/qcom/ so this makes
> me concerned. Wouldn't a better answer be to add clock-output-names to
> the xo-board DT node and have arm-soc merge that through. I mostly want
> to keep things consistent here so that if we need to inject an xo_board
> clk into the system then we can do so generically instead of making it
> per-platform. Of course, if we're never going to have this problem on
> qcs404 then it will be fine to start differing here. So I'm leaning
> towards merge this patch, just please ack my concern here and tell me it
> won't be a problem and I'll be happy to merge to clk-fixes.

So this is a warning from DT compiler and triggered with W=12, I
see tons of examples using "_" in node names. Clearly someone realized
it (Rob ?) added a warning for it.

As you rightly thought, qcs404 will be okay as we are starting out and following
few conventions so keeping this saner :)

> BTW, can you also specify a 'clocks' property in the GCC node and send
> the xo_board node there? In fact, we should do that for every GCC node
> in the tree. Care to do that and also add sleep_clk to each clock
> controller node that uses it? This is useful to do so that we can more
> easily see where clocks are going between clock controller nodes.

I agree that it makes sense to add the property in gcc node. I will add
this in my list and chase if after my current task completes, if that is
fine by you

Thanks
-- 
~Vinod

Re: [PATCH] clk: qcom: gcc: Fix board clock node name

[Vinod Koul]

Fix Rob's email id... and looping him correctly :)

On 09-11-18, 23:18, Vinod Koul wrote:
> (Add Rob & Bjorn)
> 
> Hi Steve,
> 
> On 09-11-18, 09:12, Stephen Boyd wrote:
> > Quoting Vinod Koul (2018-11-09 01:50:54)
> > > Device tree node name are not supposed to have "_" in them so fix the
> > > node name use of xo_board to xo-board
> > > 
> > > Fixes: 652f1813c113 ("clk: qcom: gcc: Add global clock controller driver for QCS404")
> > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > ---
> > > 
> > > Steve: RobH pointed this on DTS patches, would be great if you can pick this
> > > as a fix
> > 
> > Ok.
> > 
> > > 
> > >  drivers/clk/qcom/gcc-qcs404.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
> > > index e4ca6a45f313..ef1b267cb058 100644
> > > --- a/drivers/clk/qcom/gcc-qcs404.c
> > > +++ b/drivers/clk/qcom/gcc-qcs404.c
> > > @@ -265,7 +265,7 @@ static struct clk_fixed_factor cxo = {
> > >         .div = 1,
> > >         .hw.init = &(struct clk_init_data){
> > >                 .name = "cxo",
> > > -               .parent_names = (const char *[]){ "xo_board" },
> > > +               .parent_names = (const char *[]){ "xo-board" },
> > 
> > We have xo_board used everywhere else in drivers/clk/qcom/ so this makes
> > me concerned. Wouldn't a better answer be to add clock-output-names to
> > the xo-board DT node and have arm-soc merge that through. I mostly want
> > to keep things consistent here so that if we need to inject an xo_board
> > clk into the system then we can do so generically instead of making it
> > per-platform. Of course, if we're never going to have this problem on
> > qcs404 then it will be fine to start differing here. So I'm leaning
> > towards merge this patch, just please ack my concern here and tell me it
> > won't be a problem and I'll be happy to merge to clk-fixes.
> 
> So this is a warning from DT compiler and triggered with W=12, I
> see tons of examples using "_" in node names. Clearly someone realized
> it (Rob ?) added a warning for it.
> 
> As you rightly thought, qcs404 will be okay as we are starting out and following
> few conventions so keeping this saner :)
> 
> > BTW, can you also specify a 'clocks' property in the GCC node and send
> > the xo_board node there? In fact, we should do that for every GCC node
> > in the tree. Care to do that and also add sleep_clk to each clock
> > controller node that uses it? This is useful to do so that we can more
> > easily see where clocks are going between clock controller nodes.
> 
> I agree that it makes sense to add the property in gcc node. I will add
> this in my list and chase if after my current task completes, if that is
> fine by you
> 
> Thanks
> -- 
> ~Vinod

-- 
~Vinod

Re: [PATCH] clk: qcom: gcc: Fix board clock node name

[Stephen Boyd]

Quoting Vinod Koul (2018-11-09 09:51:05)
> On 09-11-18, 23:18, Vinod Koul wrote:
> > On 09-11-18, 09:12, Stephen Boyd wrote:
> > > Quoting Vinod Koul (2018-11-09 01:50:54)
> > > >  drivers/clk/qcom/gcc-qcs404.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
> > > > index e4ca6a45f313..ef1b267cb058 100644
> > > > --- a/drivers/clk/qcom/gcc-qcs404.c
> > > > +++ b/drivers/clk/qcom/gcc-qcs404.c
> > > > @@ -265,7 +265,7 @@ static struct clk_fixed_factor cxo = {
> > > >         .div = 1,
> > > >         .hw.init = &(struct clk_init_data){
> > > >                 .name = "cxo",
> > > > -               .parent_names = (const char *[]){ "xo_board" },
> > > > +               .parent_names = (const char *[]){ "xo-board" },
> > > 
> > > We have xo_board used everywhere else in drivers/clk/qcom/ so this makes
> > > me concerned. Wouldn't a better answer be to add clock-output-names to
> > > the xo-board DT node and have arm-soc merge that through. I mostly want
> > > to keep things consistent here so that if we need to inject an xo_board
> > > clk into the system then we can do so generically instead of making it
> > > per-platform. Of course, if we're never going to have this problem on
> > > qcs404 then it will be fine to start differing here. So I'm leaning
> > > towards merge this patch, just please ack my concern here and tell me it
> > > won't be a problem and I'll be happy to merge to clk-fixes.
> > 
> > So this is a warning from DT compiler and triggered with W=12, I
> > see tons of examples using "_" in node names. Clearly someone realized
> > it (Rob ?) added a warning for it.
> > 
> > As you rightly thought, qcs404 will be okay as we are starting out and following
> > few conventions so keeping this saner :)
> > 
> > > BTW, can you also specify a 'clocks' property in the GCC node and send
> > > the xo_board node there? In fact, we should do that for every GCC node
> > > in the tree. Care to do that and also add sleep_clk to each clock
> > > controller node that uses it? This is useful to do so that we can more
> > > easily see where clocks are going between clock controller nodes.
> > 
> > I agree that it makes sense to add the property in gcc node. I will add
> > this in my list and chase if after my current task completes, if that is
> > fine by you
> > 

Ok. Thanks for checking. Applied to clk-fixes. 

Re: [PATCH] clk: qcom: gcc: Fix board clock node name

[Taniya Das]

Hello Vinod,

On 11/9/2018 3:20 PM, Vinod Koul wrote:
> Device tree node name are not supposed to have "_" in them so fix the
> node name use of xo_board to xo-board
> 
> Fixes: 652f1813c113 ("clk: qcom: gcc: Add global clock controller driver for QCS404")
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> 
> Steve: RobH pointed this on DTS patches, would be great if you can pick this
> as a fix
> 
>   drivers/clk/qcom/gcc-qcs404.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
> index e4ca6a45f313..ef1b267cb058 100644
> --- a/drivers/clk/qcom/gcc-qcs404.c
> +++ b/drivers/clk/qcom/gcc-qcs404.c
> @@ -265,7 +265,7 @@ static struct clk_fixed_factor cxo = {
>   	.div = 1,
>   	.hw.init = &(struct clk_init_data){
>   		.name = "cxo",
> -		.parent_names = (const char *[]){ "xo_board" },
> +		.parent_names = (const char *[]){ "xo-board" },
>   		.num_parents = 1,
>   		.ops = &clk_fixed_factor_ops,
>   	},
> 

This fixed clock needs to be removed, once the RPM<->SMD clocks are 
added. Why not have this clock part of the device Tree?

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

Re: [PATCH] clk: qcom: gcc: Fix board clock node name

[Stephen Boyd]

Quoting Taniya Das (2018-11-10 18:12:28)
> Hello Vinod,
> 
> On 11/9/2018 3:20 PM, Vinod Koul wrote:
> > Device tree node name are not supposed to have "_" in them so fix the
> > node name use of xo_board to xo-board
> > 
> > Fixes: 652f1813c113 ("clk: qcom: gcc: Add global clock controller driver for QCS404")
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> > 
> > Steve: RobH pointed this on DTS patches, would be great if you can pick this
> > as a fix
> > 
> >   drivers/clk/qcom/gcc-qcs404.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
> > index e4ca6a45f313..ef1b267cb058 100644
> > --- a/drivers/clk/qcom/gcc-qcs404.c
> > +++ b/drivers/clk/qcom/gcc-qcs404.c
> > @@ -265,7 +265,7 @@ static struct clk_fixed_factor cxo = {
> >       .div = 1,
> >       .hw.init = &(struct clk_init_data){
> >               .name = "cxo",
> > -             .parent_names = (const char *[]){ "xo_board" },
> > +             .parent_names = (const char *[]){ "xo-board" },
> >               .num_parents = 1,
> >               .ops = &clk_fixed_factor_ops,
> >       },
> > 
> 
> This fixed clock needs to be removed, once the RPM<->SMD clocks are 
> added. Why not have this clock part of the device Tree?
> 

If the clk needs to be removed at some point, then putting it into DT
instead of leaving it in the driver is the wrong direction to take. We
don't want to have to change DT as often as we change driver code.