| Message ID | 20181109095054.13924-1-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | clk: qcom: gcc: Fix board clock node name | expand |
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.
(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
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
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.
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?
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.
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, },
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(-)