Re: [PATCH v10 05/17] clk: tegra: Introduce ability for SoC-specific reset control callbacks

[Thierry Reding]

On Tue, May 19, 2015 at 06:06:39PM +0300, Mikko Perttunen wrote:
> On 05/19/2015 05:59 PM, Thierry Reding wrote:
> >On Tue, May 19, 2015 at 02:39:27PM +0300, Mikko Perttunen wrote:
> >>This patch allows SoC-specific CAR initialization routines to register
> >>their own reset_assert and reset_deassert callbacks with the common Tegra
> >>CAR code. If defined, the common code will call these callbacks when a
> >>reset control with number >= num_periph_banks * 32 is attempted to be asserted
> >>or deasserted respectively. Numbers greater than or equal to num_periph_banks * 32
> >>are used to avoid clashes with low numbers that are automatically mapped to
> >>standard CAR reset lines.
> >>
> >>Each SoC with these special resets should specify the defined reset control
> >>numbers in a device tree header file.
> >
> >This is looking pretty good, but I think we can simplify a wee bit
> >more...
> >
> >>Signed-off-by: Mikko Perttunen <mikko.perttunen@xxxxxxxx>
> >>Acked-by: Michael Turquette <mturquette@xxxxxxxxxx>
> >>---
> >>  drivers/clk/tegra/clk.c | 39 +++++++++++++++++++++++++++++++--------
> >>  drivers/clk/tegra/clk.h |  3 +++
> >>  2 files changed, 34 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
> >>index 41cd87c..c093ed9 100644
> >>--- a/drivers/clk/tegra/clk.c
> >>+++ b/drivers/clk/tegra/clk.c
> >>@@ -49,7 +49,6 @@
> >>  #define RST_DEVICES_L			0x004
> >>  #define RST_DEVICES_H			0x008
> >>  #define RST_DEVICES_U			0x00C
> >>-#define RST_DFLL_DVCO			0x2F4
> >>  #define RST_DEVICES_V			0x358
> >>  #define RST_DEVICES_W			0x35C
> >>  #define RST_DEVICES_X			0x28C
> >>@@ -79,6 +78,11 @@ static struct clk **clks;
> >>  static int clk_num;
> >>  static struct clk_onecell_data clk_data;
> >>
> >>+/* Handlers for SoC-specific reset lines */
> >>+static int (*special_reset_assert)(unsigned long);
> >>+static int (*special_reset_deassert)(unsigned long);
> >>+static int special_reset_num;
> >
> >I think we can get rid of this if we...
> >
> >>+
> >>  static struct tegra_clk_periph_regs periph_regs[] = {
> >>  	[0] = {
> >>  		.enb_reg = CLK_OUT_ENB_L,
> >>@@ -152,19 +156,29 @@ static int tegra_clk_rst_assert(struct reset_controller_dev *rcdev,
> >>  	 */
> >>  	tegra_read_chipid();
> >>
> >>-	writel_relaxed(BIT(id % 32),
> >>-			clk_base + periph_regs[id / 32].rst_set_reg);
> >>+	if (id < periph_banks * 32) {
> >>+		writel_relaxed(BIT(id % 32),
> >>+			       clk_base + periph_regs[id / 32].rst_set_reg);
> >>+		return 0;
> >>+	} else if (id < periph_banks * 32 + special_reset_num) {
> >>+		return special_reset_assert(id);
> >>+	}
> >
> >... pass id - periph_banks * 32 into special_reset_assert(). Oh, but
> >then...
> 
> The reason I don't subtract periph_banks * 32 is because this way the code
> in the SoC-specific callback can just include the dt-bindings header and use
> the same defines used in the device tree.

Indeed, you're right. Now if

	static int special_reset_num;

could be turned into

	static unsigned int num_special_reset;

I'd be happy to take this into the Tegra clock tree.

Thierry

Attachment: pgpLmFAnfCinH.pgp
Description: PGP signature