From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Tue, 01 Oct 2013 14:27:51 +0200 Subject: [PATCH 5/6] clk: emev2: Add support for emev2 SMU clocks with DT In-Reply-To: References: <52410F86.4040301@renesas.com> <20130924131542.f865fad4dec98e024c0d4676@ops.dti.ne.jp> Message-ID: <690317658.gaTDz3TliP@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Yoshii-san, Thank you for the patch. (CC'ing LAMK as a generic CCF question follows) On Tuesday 01 October 2013 18:15:26 Magnus Damm wrote: > On Tue, Sep 24, 2013 at 1:15 PM, wrote: > > Common clock framework version of emev2 clock support. > > smu_clkdiv and smu_gclk are handled. > > So far, reparent is not implemented, and is fixed to index #0. > > SMU and small numbers of clocks are described in emev2.dtsi. > > > > That function and numbers of clocks are equivalent to current > > sh-clkfwk version. It is just enough to run kzm9d-reference. > > > > Signed-off-by: Takashi Yoshii > > --- > > > > arch/arm/boot/dts/emev2.dtsi | 84 +++++++++++++++++++++++++++++++ > > drivers/clk/Makefile | 2 + > > drivers/clk/shmobile/Makefile | 5 ++ > > drivers/clk/shmobile/clk-emev2.c | 104 ++++++++++++++++++++++++++++++++++ > > 4 files changed, 195 insertions(+) > > create mode 100644 drivers/clk/shmobile/Makefile > > create mode 100644 drivers/clk/shmobile/clk-emev2.c > > Hi Yoshii-san, > > Thanks for your efforts on this. I'm very pleased to see that you describe > the clock topology using DT. What is the generally accepted practice when an IP core provides a large number of clocks, with either one register or one register bit dedicated to each clock ? Should each clock be described as one DT node, or should the IP core be described by a single DT node ? I also see both platforms using CLK_OF_DECLARE and platforms calling a clock init function provided by drivers/clk/.c in the SoC setup code in arch/arm/. What is the preferred practice there ? > In general I think your patch looks fine, but I have some comment related to > the multiplatform integration, please see below. > > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > > index 7b11106..3e64ac4 100644 > > --- a/drivers/clk/Makefile > > +++ b/drivers/clk/Makefile > > @@ -32,6 +32,8 @@ obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o > > > > obj-$(CONFIG_ARCH_ZYNQ) += zynq/ > > obj-$(CONFIG_ARCH_TEGRA) += tegra/ > > obj-$(CONFIG_PLAT_SAMSUNG) += samsung/ > > > > +obj-$(CONFIG_ARCH_SHMOBILE) += shmobile/ > > +obj-$(CONFIG_ARCH_SHMOBILE_MULTI) += shmobile/ > > Here I believe it is enough that you only use > CONFIG_ARCH_SHMOBILE_MULTI. Building common clocks to coexist with the > old legacy board code does not make any sense IMO. If you think it > makes sense for some reason, please explain why. =) > > > diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile > > new file mode 100644 > > index 0000000..6a26eb6 > > --- /dev/null > > +++ b/drivers/clk/shmobile/Makefile > > @@ -0,0 +1,5 @@ > > +ifeq ($(CONFIG_COMMON_CLK), y) > > +obj-$(CONFIG_ARCH_EMEV2) += clk-emev2.o > > +endif > > I don't think you would need the above ifeq/endif wrapper if you only > used CONFIG_ARCH_SHMOBILE_MULTI above. > > Apart from that it looks good to me. And, yes, I have tested this on > my KZM9D board together with multiplatform and it works very well! -- Regards, Laurent Pinchart