| Message ID | 1392314571-30107-2-git-send-email-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote: > With the addition of clock-indices, we need to change the renesas > clock implementation to use these instead of the local definition. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > --- > .../bindings/clock/renesas,cpg-mstp-clocks.txt | 2 +- > arch/arm/boot/dts/r8a7790.dtsi | 16 ++++++++-------- > arch/arm/boot/dts/r8a7791.dtsi | 18 +++++++++--------- > drivers/clk/shmobile/clk-mstp.c | 2 +- > 4 files changed, 19 insertions(+), 19 deletions(-) Thanks for your patches! Good to see more common bindings. Regarding how to roll it into the SoC code, I feel that this patch [2/3] and next patch [3/3] potentially causes breakage with the existing binding that was part of the v3.13 upstream kernel. I'd like to maintain backwards compatibility for our users. Also, this patch crosses subsystems which may not be entirely ideal from a merge perspective. If other people in the community agree with [1/3] then may I propose that you rework the series into: [1/3] as-is is fine for me [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c code from [2/3] and merge with [3/3], stay backwards compatible. [3/3] adjust the SoC DTS That would maintain backwards compatibility and keep existing users happy. Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus, On Friday 14 February 2014 11:32:08 Magnus Damm wrote: > On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote: > > With the addition of clock-indices, we need to change the renesas > > clock implementation to use these instead of the local definition. > > > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > > --- > > > > .../bindings/clock/renesas,cpg-mstp-clocks.txt | 2 +- > > arch/arm/boot/dts/r8a7790.dtsi | 16 ++++++++-------- > > arch/arm/boot/dts/r8a7791.dtsi | 18 +++++++++--------- > > drivers/clk/shmobile/clk-mstp.c | 2 +- > > 4 files changed, 19 insertions(+), 19 deletions(-) > > Thanks for your patches! Good to see more common bindings. > > Regarding how to roll it into the SoC code, I feel that this patch [2/3] and > next patch [3/3] potentially causes breakage with the existing binding that > was part of the v3.13 upstream kernel. If I'm not mistaken that's v3.14, not v3.13. > I'd like to maintain backwards compatibility for our users. Also, this patch > crosses subsystems which may not be entirely ideal from a merge perspective. > > If other people in the community agree with [1/3] then may I propose that > you rework the series into: > > [1/3] as-is is fine for me > [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c > code from [2/3] and merge with [3/3], stay backwards compatible. > [3/3] adjust the SoC DTS > > That would maintain backwards compatibility and keep existing users happy. We'll have to address this DT backward compatibility nonsense at some point. We can't consider all bindings as stable as soon as they hit mainline, that just won't fly. In this particular case, given that the driver got merged in v3.14-rc1, and that our mainline multiplatform boards in mainline are virtually useless in real products given their early stage of development, I would rather consider the clocks DT bindings as unstable for at least a couple more kernel versions.
On 14/02/14 13:21, Laurent Pinchart wrote: > Hi Magnus, > > On Friday 14 February 2014 11:32:08 Magnus Damm wrote: >> On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote: >>> With the addition of clock-indices, we need to change the renesas >>> clock implementation to use these instead of the local definition. >>> >>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >>> --- >>> >>> .../bindings/clock/renesas,cpg-mstp-clocks.txt | 2 +- >>> arch/arm/boot/dts/r8a7790.dtsi | 16 ++++++++-------- >>> arch/arm/boot/dts/r8a7791.dtsi | 18 +++++++++--------- >>> drivers/clk/shmobile/clk-mstp.c | 2 +- >>> 4 files changed, 19 insertions(+), 19 deletions(-) >> >> Thanks for your patches! Good to see more common bindings. >> >> Regarding how to roll it into the SoC code, I feel that this patch [2/3] and >> next patch [3/3] potentially causes breakage with the existing binding that >> was part of the v3.13 upstream kernel. > > If I'm not mistaken that's v3.14, not v3.13. > >> I'd like to maintain backwards compatibility for our users. Also, this patch >> crosses subsystems which may not be entirely ideal from a merge perspective. >> >> If other people in the community agree with [1/3] then may I propose that >> you rework the series into: >> >> [1/3] as-is is fine for me >> [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c >> code from [2/3] and merge with [3/3], stay backwards compatible. >> [3/3] adjust the SoC DTS >> >> That would maintain backwards compatibility and keep existing users happy. > > We'll have to address this DT backward compatibility nonsense at some point. > We can't consider all bindings as stable as soon as they hit mainline, that > just won't fly. In this particular case, given that the driver got merged in > v3.14-rc1, and that our mainline multiplatform boards in mainline are > virtually useless in real products given their early stage of development, I > would rather consider the clocks DT bindings as unstable for at least a couple > more kernel versions. :( as someone who is being paid to do this for a real product In this case at-least the backwards compatibility is only a couple of lines to check for the older name.
Hi Laurent, On Fri, Feb 14, 2014 at 10:21 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus, > > On Friday 14 February 2014 11:32:08 Magnus Damm wrote: >> On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote: >> > With the addition of clock-indices, we need to change the renesas >> > clock implementation to use these instead of the local definition. >> > >> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> > --- >> > >> > .../bindings/clock/renesas,cpg-mstp-clocks.txt | 2 +- >> > arch/arm/boot/dts/r8a7790.dtsi | 16 ++++++++-------- >> > arch/arm/boot/dts/r8a7791.dtsi | 18 +++++++++--------- >> > drivers/clk/shmobile/clk-mstp.c | 2 +- >> > 4 files changed, 19 insertions(+), 19 deletions(-) >> >> Thanks for your patches! Good to see more common bindings. >> >> Regarding how to roll it into the SoC code, I feel that this patch [2/3] and >> next patch [3/3] potentially causes breakage with the existing binding that >> was part of the v3.13 upstream kernel. > > If I'm not mistaken that's v3.14, not v3.13. Nope, it's merged in v3.13. Looked it up before sending the email. =) >> I'd like to maintain backwards compatibility for our users. Also, this patch >> crosses subsystems which may not be entirely ideal from a merge perspective. >> >> If other people in the community agree with [1/3] then may I propose that >> you rework the series into: >> >> [1/3] as-is is fine for me >> [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c >> code from [2/3] and merge with [3/3], stay backwards compatible. >> [3/3] adjust the SoC DTS >> >> That would maintain backwards compatibility and keep existing users happy. > > We'll have to address this DT backward compatibility nonsense at some point. > We can't consider all bindings as stable as soon as they hit mainline, that > just won't fly. In this particular case, given that the driver got merged in > v3.14-rc1, and that our mainline multiplatform boards in mainline are > virtually useless in real products given their early stage of development, I > would rather consider the clocks DT bindings as unstable for at least a couple > more kernel versions. We need to chat more frequently my friend. Cheers, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus, On Friday 14 February 2014 23:28:42 Magnus Damm wrote: > On Fri, Feb 14, 2014 at 10:21 PM, Laurent Pinchart wrote: > > On Friday 14 February 2014 11:32:08 Magnus Damm wrote: > >> On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote: > >> > With the addition of clock-indices, we need to change the renesas > >> > clock implementation to use these instead of the local definition. > >> > > >> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > >> > --- > >> > > >> > .../bindings/clock/renesas,cpg-mstp-clocks.txt | 2 +- > >> > arch/arm/boot/dts/r8a7790.dtsi | 16 ++++++++-------- > >> > arch/arm/boot/dts/r8a7791.dtsi | 18 +++++++--------- > >> > drivers/clk/shmobile/clk-mstp.c | 2 +- > >> > 4 files changed, 19 insertions(+), 19 deletions(-) > >> > >> Thanks for your patches! Good to see more common bindings. > >> > >> Regarding how to roll it into the SoC code, I feel that this patch [2/3] > >> and next patch [3/3] potentially causes breakage with the existing > >> binding that was part of the v3.13 upstream kernel. > > > > If I'm not mistaken that's v3.14, not v3.13. > > Nope, it's merged in v3.13. Looked it up before sending the email. =) I might be missing something, but $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc 0 0 0 $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc 6 48 309 > >> I'd like to maintain backwards compatibility for our users. Also, this > >> patch crosses subsystems which may not be entirely ideal from a merge > >> perspective. > >> > >> If other people in the community agree with [1/3] then may I propose that > >> you rework the series into: > >> > >> [1/3] as-is is fine for me > >> [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c > >> code from [2/3] and merge with [3/3], stay backwards compatible. > >> [3/3] adjust the SoC DTS > >> > >> That would maintain backwards compatibility and keep existing users > >> happy. > > > > We'll have to address this DT backward compatibility nonsense at some > > point. We can't consider all bindings as stable as soon as they hit > > mainline, that just won't fly. In this particular case, given that the > > driver got merged in v3.14-rc1, and that our mainline multiplatform > > boards in mainline are virtually useless in real products given their > > early stage of development, I would rather consider the clocks DT > > bindings as unstable for at least a couple more kernel versions. > > We need to chat more frequently my friend. Sure :-)
Hi Ben, On Friday 14 February 2014 14:03:34 Ben Dooks wrote: > On 14/02/14 13:21, Laurent Pinchart wrote: > > On Friday 14 February 2014 11:32:08 Magnus Damm wrote: > >> On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote: > >>> With the addition of clock-indices, we need to change the renesas > >>> clock implementation to use these instead of the local definition. > >>> > >>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > >>> --- > >>> > >>> .../bindings/clock/renesas,cpg-mstp-clocks.txt | 2 +- > >>> arch/arm/boot/dts/r8a7790.dtsi | 16 ++++++++-------- > >>> arch/arm/boot/dts/r8a7791.dtsi | 18 +++++++++--------- > >>> drivers/clk/shmobile/clk-mstp.c | 2 +- > >>> 4 files changed, 19 insertions(+), 19 deletions(-) > >> > >> Thanks for your patches! Good to see more common bindings. > >> > >> Regarding how to roll it into the SoC code, I feel that this patch [2/3] > >> and next patch [3/3] potentially causes breakage with the existing > >> binding that was part of the v3.13 upstream kernel. > > > > If I'm not mistaken that's v3.14, not v3.13. > > > >> I'd like to maintain backwards compatibility for our users. Also, this > >> patch crosses subsystems which may not be entirely ideal from a merge > >> perspective. > >> > >> If other people in the community agree with [1/3] then may I propose that > >> you rework the series into: > >> > >> [1/3] as-is is fine for me > >> [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c > >> code from [2/3] and merge with [3/3], stay backwards compatible. > >> [3/3] adjust the SoC DTS > >> > >> That would maintain backwards compatibility and keep existing users > >> happy. > > > > We'll have to address this DT backward compatibility nonsense at some > > point. We can't consider all bindings as stable as soon as they hit > > mainline, that just won't fly. In this particular case, given that the > > driver got merged in v3.14-rc1, and that our mainline multiplatform > > boards in mainline are virtually useless in real products given their > > early stage of development, I would rather consider the clocks DT > > bindings as unstable for at least a couple more kernel versions. > > :( as someone who is being paid to do this for a real product I understand that, and I won't nack the patch. The problem at hand is broader than this though, I don't think we should consider DT bindings as stable as soon as they hit mainline. A couple of kernel versions are needed to test the bindings. This has been discussed previously, we need a way to mark bindings as staging and later move them to a stable state. > In this case at-least the backwards compatibility is only a couple > of lines to check for the older name.
Hi Laurent, Magnus, On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> >> Regarding how to roll it into the SoC code, I feel that this patch [2/3] >> >> and next patch [3/3] potentially causes breakage with the existing >> >> binding that was part of the v3.13 upstream kernel. >> > >> > If I'm not mistaken that's v3.14, not v3.13. >> >> Nope, it's merged in v3.13. Looked it up before sending the email. =) > > I might be missing something, but > > $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc > 0 0 0 > $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc > 6 48 309 It's the published bindings that count, right? Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt was added in commit f94859c215b6d977794108a1a9a101239e393c09 ("clk: shmobile: Add MSTP clock support"). $ git tag --contains f94859c215b6d977794108a1a9a101239e393c09 v3.14-rc1 v3.14-rc2 $ Now, the bindings must be in flux, as the documentation doesn't match the code, nor the example: Documentation says: + - renesas,indices: Indices of the gate clocks into the group (0 to 31) Example and implementation use "renesas,clock-indices": renesas,clock-indices = < R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3 R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 R8A7790_CLK_MMCIF0 >; Woops... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Saturday 15 February 2014 14:20:16 Geert Uytterhoeven wrote: > Hi Laurent, Magnus, > > On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart wrote: > >> >> Regarding how to roll it into the SoC code, I feel that this patch > >> >> [2/3] and next patch [3/3] potentially causes breakage with the > >> >> existing binding that was part of the v3.13 upstream kernel. > >> > > >> > If I'm not mistaken that's v3.14, not v3.13. > >> > >> Nope, it's merged in v3.13. Looked it up before sending the email. =) > > > > I might be missing something, but > > > > $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc > > 0 0 0 > > > > $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc > > 6 48 309 > > It's the published bindings that count, right? I suppose both count, but they've been mainlined at the same time anyway. > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > was added in commit f94859c215b6d977794108a1a9a101239e393c09 > ("clk: shmobile: Add MSTP clock support"). > > $ git tag --contains f94859c215b6d977794108a1a9a101239e393c09 > v3.14-rc1 > v3.14-rc2 > $ > > Now, the bindings must be in flux, as the documentation doesn't match > the code, nor the example: > > Documentation says: > > + - renesas,indices: Indices of the gate clocks into the group (0 to 31) > > Example and implementation use "renesas,clock-indices": > > renesas,clock-indices = < > R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 > R8A7790_CLK_SDHI3 R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 > R8A7790_CLK_MMCIF0 > > >; > > Woops... You don't need to convince me that the DT bindings need time to stabilize :-) I'll fix this after hearing from Mike on Ben's patches.
Hi Laurent, On Fri, Feb 14, 2014 at 11:34 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus, > > On Friday 14 February 2014 23:28:42 Magnus Damm wrote: >> On Fri, Feb 14, 2014 at 10:21 PM, Laurent Pinchart wrote: >> > On Friday 14 February 2014 11:32:08 Magnus Damm wrote: >> >> On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote: >> >> > With the addition of clock-indices, we need to change the renesas >> >> > clock implementation to use these instead of the local definition. >> >> > >> >> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> >> > --- >> >> > >> >> > .../bindings/clock/renesas,cpg-mstp-clocks.txt | 2 +- >> >> > arch/arm/boot/dts/r8a7790.dtsi | 16 ++++++++-------- >> >> > arch/arm/boot/dts/r8a7791.dtsi | 18 +++++++--------- >> >> > drivers/clk/shmobile/clk-mstp.c | 2 +- >> >> > 4 files changed, 19 insertions(+), 19 deletions(-) >> >> >> >> Thanks for your patches! Good to see more common bindings. >> >> >> >> Regarding how to roll it into the SoC code, I feel that this patch [2/3] >> >> and next patch [3/3] potentially causes breakage with the existing >> >> binding that was part of the v3.13 upstream kernel. >> > >> > If I'm not mistaken that's v3.14, not v3.13. >> >> Nope, it's merged in v3.13. Looked it up before sending the email. =) > > I might be missing something, but > > $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc > 0 0 0 > $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc > 6 48 309 You are right, sorry about that! I'm not sure how I managed to get to that wrong conclusion. >> >> I'd like to maintain backwards compatibility for our users. Also, this >> >> patch crosses subsystems which may not be entirely ideal from a merge >> >> perspective. >> >> >> >> If other people in the community agree with [1/3] then may I propose that >> >> you rework the series into: >> >> >> >> [1/3] as-is is fine for me >> >> [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c >> >> code from [2/3] and merge with [3/3], stay backwards compatible. >> >> [3/3] adjust the SoC DTS >> >> >> >> That would maintain backwards compatibility and keep existing users >> >> happy. >> > >> > We'll have to address this DT backward compatibility nonsense at some >> > point. We can't consider all bindings as stable as soon as they hit >> > mainline, that just won't fly. In this particular case, given that the >> > driver got merged in v3.14-rc1, and that our mainline multiplatform >> > boards in mainline are virtually useless in real products given their >> > early stage of development, I would rather consider the clocks DT >> > bindings as unstable for at least a couple more kernel versions. >> >> We need to chat more frequently my friend. > > Sure :-) Good! Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Sat, Feb 15, 2014 at 10:20 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Laurent, Magnus, > > On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >>> >> Regarding how to roll it into the SoC code, I feel that this patch [2/3] >>> >> and next patch [3/3] potentially causes breakage with the existing >>> >> binding that was part of the v3.13 upstream kernel. >>> > >>> > If I'm not mistaken that's v3.14, not v3.13. >>> >>> Nope, it's merged in v3.13. Looked it up before sending the email. =) >> >> I might be missing something, but >> >> $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc >> 0 0 0 >> $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc >> 6 48 309 > > It's the published bindings that count, right? Good question. I don't know actually. > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > was added in commit f94859c215b6d977794108a1a9a101239e393c09 > ("clk: shmobile: Add MSTP clock support"). > > $ git tag --contains f94859c215b6d977794108a1a9a101239e393c09 > v3.14-rc1 > v3.14-rc2 > $ > > Now, the bindings must be in flux, as the documentation doesn't match > the code, nor the example: > > Documentation says: > > + - renesas,indices: Indices of the gate clocks into the group (0 to 31) > > Example and implementation use "renesas,clock-indices": > > renesas,clock-indices = < > R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3 > R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 > R8A7790_CLK_MMCIF0 > >; > > Woops... Thanks for spotting this. I believe Laurent will fix this up in the not so distant future. Cheers, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus, On Tuesday 18 February 2014 10:50:07 Magnus Damm wrote: > On Sat, Feb 15, 2014 at 10:20 PM, Geert Uytterhoeven wrote: > > On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart wrote: > >>> >> Regarding how to roll it into the SoC code, I feel that this patch > >>> >> [2/3] and next patch [3/3] potentially causes breakage with the > >>> >> existing binding that was part of the v3.13 upstream kernel. > >>> > > >>> > If I'm not mistaken that's v3.14, not v3.13. > >>> > >>> Nope, it's merged in v3.13. Looked it up before sending the email. =) > >> > >> I might be missing something, but > >> > >> $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc > >> 0 0 0 > >> $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc > >> 6 48 309 > > > > It's the published bindings that count, right? > > Good question. I don't know actually. > > > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > was added in commit f94859c215b6d977794108a1a9a101239e393c09 > > ("clk: shmobile: Add MSTP clock support"). > > > > $ git tag --contains f94859c215b6d977794108a1a9a101239e393c09 > > v3.14-rc1 > > v3.14-rc2 > > $ > > > > Now, the bindings must be in flux, as the documentation doesn't match > > the code, nor the example: > > > > Documentation says: > > > > + - renesas,indices: Indices of the gate clocks into the group (0 to 31) > > > > Example and implementation use "renesas,clock-indices": > > renesas,clock-indices = < > > R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 > > R8A7790_CLK_SDHI3 > > R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 > > R8A7790_CLK_SDHI0 > > R8A7790_CLK_MMCIF0 > > >; > > > > Woops... > > Thanks for spotting this. I believe Laurent will fix this up in the not so > distant future. I plan to wait for Mike to comment on Ben's patches before addressing this.
Hi Laurent, On Tue, Feb 18, 2014 at 9:14 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus, > > On Tuesday 18 February 2014 10:50:07 Magnus Damm wrote: >> On Sat, Feb 15, 2014 at 10:20 PM, Geert Uytterhoeven wrote: >> > On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart wrote: >> >>> >> Regarding how to roll it into the SoC code, I feel that this patch >> >>> >> [2/3] and next patch [3/3] potentially causes breakage with the >> >>> >> existing binding that was part of the v3.13 upstream kernel. >> >>> > >> >>> > If I'm not mistaken that's v3.14, not v3.13. >> >>> >> >>> Nope, it's merged in v3.13. Looked it up before sending the email. =) >> >> >> >> I might be missing something, but >> >> >> >> $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc >> >> 0 0 0 >> >> $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc >> >> 6 48 309 >> > >> > It's the published bindings that count, right? >> >> Good question. I don't know actually. >> >> > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt >> > was added in commit f94859c215b6d977794108a1a9a101239e393c09 >> > ("clk: shmobile: Add MSTP clock support"). >> > >> > $ git tag --contains f94859c215b6d977794108a1a9a101239e393c09 >> > v3.14-rc1 >> > v3.14-rc2 >> > $ >> > >> > Now, the bindings must be in flux, as the documentation doesn't match >> > the code, nor the example: >> > >> > Documentation says: >> > >> > + - renesas,indices: Indices of the gate clocks into the group (0 to 31) >> > >> > Example and implementation use "renesas,clock-indices": >> > renesas,clock-indices = < >> > R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 >> > R8A7790_CLK_SDHI3 >> > R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 >> > R8A7790_CLK_SDHI0 >> > R8A7790_CLK_MMCIF0 >> > >; >> > >> > Woops... >> >> Thanks for spotting this. I believe Laurent will fix this up in the not so >> distant future. > > I plan to wait for Mike to comment on Ben's patches before addressing this. Can you please explain about the reason behind this? I may not have enough detailed understanding about this matter, but to me it looks like two separate issues that somehow are combined together: A) patch series from ben - development targeted at future merge windows B) patch to fix inconsistency between documentation and actual code - fix for targeted for -rc? So regardless what the community decision is about A) we probably want to fix B) anyway, no? Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus, On Wednesday 19 February 2014 15:34:39 Magnus Damm wrote: > On Tue, Feb 18, 2014 at 9:14 PM, Laurent Pinchart wrote: > > On Tuesday 18 February 2014 10:50:07 Magnus Damm wrote: > >> On Sat, Feb 15, 2014 at 10:20 PM, Geert Uytterhoeven wrote: > >> > On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart wrote: > >> >>> >> Regarding how to roll it into the SoC code, I feel that this patch > >> >>> >> [2/3] and next patch [3/3] potentially causes breakage with the > >> >>> >> existing binding that was part of the v3.13 upstream kernel. > >> >>> > > >> >>> > If I'm not mistaken that's v3.14, not v3.13. > >> >>> > >> >>> Nope, it's merged in v3.13. Looked it up before sending the email. =) > >> >> > >> >> I might be missing something, but > >> >> > >> >> $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc > >> >> 0 0 0 > >> >> $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc > >> >> 6 48 309 > >> > > >> > It's the published bindings that count, right? > >> > >> Good question. I don't know actually. > >> > >> > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > >> > was added in commit f94859c215b6d977794108a1a9a101239e393c09 > >> > ("clk: shmobile: Add MSTP clock support"). > >> > > >> > $ git tag --contains f94859c215b6d977794108a1a9a101239e393c09 > >> > v3.14-rc1 > >> > v3.14-rc2 > >> > $ > >> > > >> > Now, the bindings must be in flux, as the documentation doesn't match > >> > the code, nor the example: > >> > > >> > Documentation says: > >> > > >> > + - renesas,indices: Indices of the gate clocks into the group (0 to > >> > 31) > >> > > >> > Example and implementation use "renesas,clock-indices": > >> > renesas,clock-indices = < > >> > R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 > >> > R8A7790_CLK_SDHI3 > >> > R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 > >> > R8A7790_CLK_SDHI0 > >> > R8A7790_CLK_MMCIF0 > >> > >; > >> > > >> > Woops... > >> > >> Thanks for spotting this. I believe Laurent will fix this up in the not > >> so distant future. > > > > I plan to wait for Mike to comment on Ben's patches before addressing > > this. > > Can you please explain about the reason behind this? I may not have > enough detailed understanding about this matter, but to me it looks > like two separate issues that somehow are combined together: > > A) patch series from ben - development targeted at future merge windows > B) patch to fix inconsistency between documentation and actual code - > fix for targeted for -rc? > > So regardless what the community decision is about A) we probably want > to fix B) anyway, no? My bad, I had v3.15 in mind, for which I thought there would be no need to fix this if Ben's patches were accepted. I'll submit a patch for v3.14-rc.
diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt index a6a352c..f5b4171 100644 --- a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt @@ -43,7 +43,7 @@ Example clock-output-names = "tpu0", "mmcif1", "sdhi3", "sdhi2", "sdhi1", "sdhi0", "mmcif0"; - renesas,clock-indices = < + clock-indices = < R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3 R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 R8A7790_CLK_MMCIF0 diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi index 54ab318..7959323 100644 --- a/arch/arm/boot/dts/r8a7790.dtsi +++ b/arch/arm/boot/dts/r8a7790.dtsi @@ -671,7 +671,7 @@ reg = <0 0xe6150130 0 4>, <0 0xe6150030 0 4>; clocks = <&mp_clk>; #clock-cells = <1>; - renesas,clock-indices = <R8A7790_CLK_MSIOF0>; + clock-indices = <R8A7790_CLK_MSIOF0>; clock-output-names = "msiof0"; }; mstp1_clks: mstp1_clks@e6150134 { @@ -681,7 +681,7 @@ <&cp_clk>, <&zs_clk>, <&zs_clk>, <&zs_clk>, <&zs_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7790_CLK_TMU1 R8A7790_CLK_TMU3 R8A7790_CLK_TMU2 R8A7790_CLK_CMT0 R8A7790_CLK_TMU0 R8A7790_CLK_VSP1_DU1 R8A7790_CLK_VSP1_DU0 R8A7790_CLK_VSP1_RT R8A7790_CLK_VSP1_SY @@ -696,7 +696,7 @@ clocks = <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7790_CLK_SCIFA2 R8A7790_CLK_SCIFA1 R8A7790_CLK_SCIFA0 R8A7790_CLK_MSIOF2 R8A7790_CLK_SCIFB0 R8A7790_CLK_SCIFB1 R8A7790_CLK_MSIOF1 R8A7790_CLK_MSIOF3 R8A7790_CLK_SCIFB2 @@ -712,7 +712,7 @@ <&cpg_clocks R8A7790_CLK_SD1>, <&cpg_clocks R8A7790_CLK_SD0>, <&mmc0_clk>, <&rclk_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3 R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 R8A7790_CLK_MMCIF0 R8A7790_CLK_CMT1 @@ -726,7 +726,7 @@ reg = <0 0xe6150144 0 4>, <0 0xe615003c 0 4>; clocks = <&extal_clk>, <&p_clk>; #clock-cells = <1>; - renesas,clock-indices = <R8A7790_CLK_THERMAL R8A7790_CLK_PWM>; + clock-indices = <R8A7790_CLK_THERMAL R8A7790_CLK_PWM>; clock-output-names = "thermal", "pwm"; }; mstp7_clks: mstp7_clks@e615014c { @@ -736,7 +736,7 @@ <&p_clk>, <&zx_clk>, <&zx_clk>, <&zx_clk>, <&zx_clk>, <&zx_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7790_CLK_EHCI R8A7790_CLK_HSUSB R8A7790_CLK_HSCIF1 R8A7790_CLK_HSCIF0 R8A7790_CLK_SCIF1 R8A7790_CLK_SCIF0 R8A7790_CLK_DU2 R8A7790_CLK_DU1 R8A7790_CLK_DU0 @@ -752,7 +752,7 @@ clocks = <&zg_clk>, <&zg_clk>, <&zg_clk>, <&zg_clk>, <&p_clk>, <&zs_clk>, <&zs_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7790_CLK_VIN3 R8A7790_CLK_VIN2 R8A7790_CLK_VIN1 R8A7790_CLK_VIN0 R8A7790_CLK_ETHER R8A7790_CLK_SATA1 R8A7790_CLK_SATA0 @@ -766,7 +766,7 @@ clocks = <&p_clk>, <&p_clk>, <&cpg_clocks R8A7790_CLK_QSPI>, <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0 R8A7790_CLK_QSPI_MOD R8A7790_CLK_I2C3 R8A7790_CLK_I2C2 R8A7790_CLK_I2C1 R8A7790_CLK_I2C0 diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi index 240c4ec..166b986 100644 --- a/arch/arm/boot/dts/r8a7791.dtsi +++ b/arch/arm/boot/dts/r8a7791.dtsi @@ -598,7 +598,7 @@ reg = <0 0xe6150130 0 4>, <0 0xe6150030 0 4>; clocks = <&mp_clk>; #clock-cells = <1>; - renesas,clock-indices = <R8A7791_CLK_MSIOF0>; + clock-indices = <R8A7791_CLK_MSIOF0>; clock-output-names = "msiof0"; }; mstp1_clks: mstp1_clks@e6150134 { @@ -607,7 +607,7 @@ clocks = <&p_clk>, <&p_clk>, <&p_clk>, <&rclk_clk>, <&cp_clk>, <&zs_clk>, <&zs_clk>, <&zs_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7791_CLK_TMU1 R8A7791_CLK_TMU3 R8A7791_CLK_TMU2 R8A7791_CLK_CMT0 R8A7791_CLK_TMU0 R8A7791_CLK_VSP1_DU1 R8A7791_CLK_VSP1_DU0 R8A7791_CLK_VSP1_SY @@ -622,7 +622,7 @@ clocks = <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7791_CLK_SCIFA2 R8A7791_CLK_SCIFA1 R8A7791_CLK_SCIFA0 R8A7791_CLK_MSIOF2 R8A7791_CLK_SCIFB0 R8A7791_CLK_SCIFB1 R8A7791_CLK_MSIOF1 R8A7791_CLK_SCIFB2 @@ -637,7 +637,7 @@ clocks = <&cp_clk>, <&sd2_clk>, <&sd1_clk>, <&cpg_clocks R8A7791_CLK_SD0>, <&mmc0_clk>, <&rclk_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7791_CLK_TPU0 R8A7791_CLK_SDHI2 R8A7791_CLK_SDHI1 R8A7791_CLK_SDHI0 R8A7791_CLK_MMCIF0 R8A7791_CLK_CMT1 >; @@ -649,7 +649,7 @@ reg = <0 0xe6150144 0 4>, <0 0xe615003c 0 4>; clocks = <&extal_clk>, <&p_clk>; #clock-cells = <1>; - renesas,clock-indices = <R8A7791_CLK_THERMAL R8A7791_CLK_PWM>; + clock-indices = <R8A7791_CLK_THERMAL R8A7791_CLK_PWM>; clock-output-names = "thermal", "pwm"; }; mstp7_clks: mstp7_clks@e615014c { @@ -659,7 +659,7 @@ <&zs_clk>, <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>, <&zx_clk>, <&zx_clk>, <&zx_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7791_CLK_HSUSB R8A7791_CLK_HSCIF2 R8A7791_CLK_SCIF5 R8A7791_CLK_SCIF4 R8A7791_CLK_HSCIF1 R8A7791_CLK_HSCIF0 R8A7791_CLK_SCIF3 R8A7791_CLK_SCIF2 R8A7791_CLK_SCIF1 @@ -676,7 +676,7 @@ clocks = <&zg_clk>, <&zg_clk>, <&zg_clk>, <&p_clk>, <&zs_clk>, <&zs_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7791_CLK_VIN2 R8A7791_CLK_VIN1 R8A7791_CLK_VIN0 R8A7791_CLK_ETHER R8A7791_CLK_SATA1 R8A7791_CLK_SATA0 >; @@ -690,7 +690,7 @@ <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7791_CLK_RCAN1 R8A7791_CLK_RCAN0 R8A7791_CLK_QSPI_MOD R8A7791_CLK_I2C4 R8A7791_CLK_I2C4 R8A7791_CLK_I2C3 R8A7791_CLK_I2C2 R8A7791_CLK_I2C1 R8A7791_CLK_I2C0 @@ -704,7 +704,7 @@ reg = <0 0xe615099c 0 4>, <0 0xe61509ac 0 4>; clocks = <&mp_clk>, <&mp_clk>, <&mp_clk>; #clock-cells = <1>; - renesas,clock-indices = < + clock-indices = < R8A7791_CLK_SCIFA3 R8A7791_CLK_SCIFA4 R8A7791_CLK_SCIFA5 >; clock-output-names = "scifa3", "scifa4", "scifa5"; diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c index 42d5912..95a2aa7 100644 --- a/drivers/clk/shmobile/clk-mstp.c +++ b/drivers/clk/shmobile/clk-mstp.c @@ -197,7 +197,7 @@ static void __init cpg_mstp_clocks_init(struct device_node *np) continue; parent_name = of_clk_get_parent_name(np, i); - ret = of_property_read_u32_index(np, "renesas,clock-indices", i, + ret = of_property_read_u32_index(np, "clock-indices", i, &clkidx); if (parent_name == NULL || ret < 0) break;
With the addition of clock-indices, we need to change the renesas clock implementation to use these instead of the local definition. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- .../bindings/clock/renesas,cpg-mstp-clocks.txt | 2 +- arch/arm/boot/dts/r8a7790.dtsi | 16 ++++++++-------- arch/arm/boot/dts/r8a7791.dtsi | 18 +++++++++--------- drivers/clk/shmobile/clk-mstp.c | 2 +- 4 files changed, 19 insertions(+), 19 deletions(-)