From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5AA2DC25B06 for ; Mon, 15 Aug 2022 02:48:15 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 9E154840A9; Mon, 15 Aug 2022 04:48:13 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="ePNFiDaE"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 5322D847F9; Mon, 15 Aug 2022 04:48:12 +0200 (CEST) Received: from mail-qk1-x732.google.com (mail-qk1-x732.google.com [IPv6:2607:f8b0:4864:20::732]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 0361D840A9 for ; Mon, 15 Aug 2022 04:48:09 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ralph.siemsen@linaro.org Received: by mail-qk1-x732.google.com with SMTP id g21so1077656qka.5 for ; Sun, 14 Aug 2022 19:48:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc; bh=HyWnKhRBSvCi0wFWfvwH4Q7eHbiI0Nxi9C6BPrVVzuk=; b=ePNFiDaEmEcLc4FVvQLy4BZ9ftG+7NxhOLA53SX90OhhQqs8H1II1u5yslpKYXDYPz tBsivRUTL5gF7vhyNstXEgN0MED9rQBDbkBRDU6LnUaXVNvtzEqv18IMVF8l8ObTn1GR G43UBLX3ZFXi+Dt+GbdakTM2BtGRSJ/72aJ5iK/CE1BOFrCxvSJEln8kdldQThBEY0FB l6SmquaPYYQ8aFUr212z7ptcyqhJOw8a8Us7MtcWDnGzReJZgF3efsxubQXxqXb+R1Sq pI8cfyj2h31qdsXz+1KIo+32dt+nKP9EAkRwqPpdTcxbUYjwJdYCtw/T/DBju2Zz5ct/ HJFQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc; bh=HyWnKhRBSvCi0wFWfvwH4Q7eHbiI0Nxi9C6BPrVVzuk=; b=1nmcByTbmGJ3OnKTnU9Q8AorlG4Y5HBzC3FBb8j7Ae/JGqpiIJnJyl1AL/ETygGISX PgtpVZCpbadt+Rjhc0j35nIKmrs952nAO1SQEM+V0kVZArSUjoEzQIbQNYL3WtKCqxY0 dlyv0a6mIs8RgXtQGGjKRUyfogMLt/kX2H47WlmEb/loG6k4xmzTHCTwCDkfsD7WgmrY jTbF16Ojy8N3O76e5Nilyv6zPZM/q528eNkO1ja0YHD6o5du02z+v41LmihGweULVUHr XWV3s4m3dgF1y7canlUXuendkWUiaFxdXlJ/cN89wgw2UXD1AbxqEaahcnClzTOHuN7H /r4A== X-Gm-Message-State: ACgBeo3ivVj+BMjm5LYsGE1GaoBUP54Pe14NjCl4KyrSpuOpJkDhelJk Budjhrx8jbUhFissjRUliAIqqQ== X-Google-Smtp-Source: AA6agR5iw2mbFjCO+ZcGsYKUME3xKBx9jjTH6OHF/C1i5WQ/UitpFXgHsNj9URCRuygyZUpzUEoqwg== X-Received: by 2002:ae9:e907:0:b0:6b9:1c0b:63e3 with SMTP id x7-20020ae9e907000000b006b91c0b63e3mr10026970qkf.307.1660531687552; Sun, 14 Aug 2022 19:48:07 -0700 (PDT) Received: from localhost (rfs.netwinder.org. [206.248.184.2]) by smtp.gmail.com with ESMTPSA id h13-20020a05620a284d00b006b872b606b1sm7678046qkp.128.2022.08.14.19.48.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 14 Aug 2022 19:48:06 -0700 (PDT) Date: Sun, 14 Aug 2022 22:48:05 -0400 From: Ralph Siemsen To: Sean Anderson Cc: u-boot@lists.denx.de, Lukasz Majewski Subject: Re: [RFC PATCH v1 3/9] clk: renesas: add R906G032 driver Message-ID: <20220815024805.GA488169@maple.netwinder.org> References: <20220809125959.217333-1-ralph.siemsen@linaro.org> <20220809125959.217333-4-ralph.siemsen@linaro.org> <152abc91-7460-1a4d-063a-136b4b5f0d4b@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <152abc91-7460-1a4d-063a-136b4b5f0d4b@gmail.com> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On Sat, Aug 13, 2022 at 01:30:19AM -0400, Sean Anderson wrote: >+ >>+ u16 gate, reset, ready, midle, >>+ scon, mirack, mistat; > >What are the scon/mirack/mistat fields for? You define them for a lot >of clocks, but I don't see them used in the driver. These came from the Linux driver of the same name. I can only speculate that in turn, the Linux driver definitions were auto-generated from vendor provided XML or similar documentation. I figured that it would be best to match the Linux kernel clock driver. That way fixes can easily be shared. In fact while doing this work, I found an error in the clock table [1] and I also made some simplifications [2]. Regarding the unused fields (scon, mirack, mistat): I am not really sure what their purpose is. Maybe there is some value in having them. I'll try to find out more information about them. If we do decide to drop them, I would like to keep it synchronised with the Linux driver. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2dee50ab9e72a3cae75b65e5934c8dd3e9bf01bc [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f46efcc4746f5c1a539df9db625c04321f75e494 >>+}; >>+ >>+/* This is used to describe a clock for instantiation */ >>+struct r9a06g032_clkdesc { >>+ const char *name; >>+ uint32_t managed: 1; >>+ uint32_t type: 3; > >I wonder if we could define the enum here? This is still part of the code which I've intentionally kept identical to the Linux driver. I do agree that moving the enum seems reasonable, I'll put together a patch for this (and test it) and see if I can get it accepted on the kernel side. >>+ uint32_t index: 8; >>+ uint32_t source : 8; /* source index + 1 (0 == none) */ >>+ /* these are used to populate the bitsel struct */ >>+ union { >>+ struct r9a06g032_gate gate; >>+ /* for dividers */ >>+ struct { >>+ unsigned int div_min : 10, div_max : 10, reg: 10; >>+ u16 div_table[4]; >>+ }; >>+ /* For fixed-factor ones */ >>+ struct { >>+ u16 div, mul; >>+ }; >>+ /* for dual gate */ >>+ struct { >>+ uint16_t group : 1; >>+ u16 sel, g1, r1, g2, r2; >>+ } dual; >>+ }; >>+}; >>+ >>+#define I_GATE(_clk, _rst, _rdy, _midle, _scon, _mirack, _mistat) \ >>+ { .gate = _clk, .reset = _rst, \ > >If these fields have bitfield inside them, then those bitfields should >be assigned/constructed separately. That is, if .reset is actually a combined >offset/bit, then you need to expose those in the macro. Since you have a lot of these, you might want to do something like > >#define BIT_OFFSET GENMASK(15, 5) >#define BIT_SHIFT GENMASK(4, 0) > >#define PACK_BIT(offset, shift) (FIELD_PREP(BIT_OFFSET, offset) | FIELD_PREP(BIT_SHIFT, shift)) I think it happened before I started working on RZ/N1, but there seemed to be quite a few iterations on how to represent the clock tree. At one point there were macros to assign/construct the bitfield values. And then a different way, and eventually the direct hex values you now see in the clock tables. At the risk of re-opening old wounds (luckily not mine) I decided to just leave this part exactly as-is in the Linux driver. >>+ >>+/* Internal clock IDs */ >>+#define R9A06G032_CLKOUT 0 >>+#define R9A06G032_CLKOUT_D10 2 >>+#define R9A06G032_CLKOUT_D16 3 >>+#define R9A06G032_CLKOUT_D160 4 >>[...] >>+#define R9A06G032_UART_GROUP_012 154 >>+#define R9A06G032_UART_GROUP_34567 155 > >Can you put these in your dt-bindings header? I think that would make it >much clearer why there are gaps, and would avoid someone accidentally >duplicating a clock id (although I suppose your array initializer below >might complain?) In fact quite a few of them are in the dt-bindings already, see include/dt-bindings/clock/r9a06g032-sysctrl.h I'm not really sure why some of these are defined in the .C file while others are in the dt-bindings header. Like much of the other bits, this was something I just carried over as-is from the Linux driver. >>+ parent->id = ~0; /* Top-level clock */ > >Can you use a define for this (instead of referring to ~0 everywhere) Yes, that sounds reasonable. My list of fixes (for both Linux driver and the u-boot one) is going to get rather long ;-) >>+ else >>+ parent->id = desc->source - 1; >>+ >>+ parent->dev = clk->dev; > >I think you need to clk_request here. Normally clk_request is called by a driver wishing to use a particular clock. That is not the case here. This is in a helper function used to compute the current rate of a given clock. It only looks at the local table (struct r9a06g032_clkdsc). >>+/* register/bit pairs are encoded as an uint16_t */ >>+static void >>+clk_rdesc_set(struct r9a06g032_priv *clocks, >>+ u16 one, unsigned int on) >>+{ >>+ uint offset = 4 * (one >> 5); >>+ uint mask = 1U << (one & 0x1f); >>+ uint val = ((!!on) << (one & 0x1f)); > >Please either use bitfields for this, or use FIELD_GET() and friends. Yes, this would be clearer - however as mentioned above, there was already quite a bit of teeth-gnashing about this encoding. I will prepare a patch for the Linux side and see what kind of reply I get. I would very much prefer to keep both in sync as much as possible. >>+/* >>+ * Fixed factor clock >>+ */ >>+static ulong r9a06g032_ffc_get_rate(struct clk *clk) >>+{ >>+ const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk); >>+ unsigned long parent_rate = r9a06g032_clk_get_parent_rate(clk); >>+ unsigned long long rate; >>+ >>+ if (parent_rate == 0) { >>+ debug("%s: parent_rate is zero\n", __func__); >>+ return 0; >>+ } >>+ >>+ rate = (unsigned long long)parent_rate * desc->mul; >>+ rate = DIV_ROUND_UP(rate, desc->div); >>+ return (ulong)rate; >>+} >>+ >>+/* >>+ * This implements R9A06G032 clock divider 'driver'. This differs from the >>+ * standard clk_divider because the set_rate method must also set b[31] to >>+ * trigger the hardware rate change. In theory it should also wait for this >>+ * bit to clear. >>+ */ >>+static ulong r9a06g032_div_get_rate(struct clk *clk) >>+{ >>+ struct r9a06g032_priv *clocks = dev_get_priv(clk->dev); >>+ const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk); >>+ unsigned long parent_rate = r9a06g032_clk_get_parent_rate(clk); >>+ u32 div = 0; >>+ >>+ if (parent_rate == 0) { >>+ debug("%s: parent_rate is zero\n", __func__); > >Didn't you already log this? It is for a different clock type. However I will see if I do something to avoid the duplication. > >>+ return 0; >>+ } >>+ >>+ regmap_read(clocks->regmap, 4 * desc->reg, &div); >>+ >>+ if (div < desc->div_min) >>+ div = desc->div_min; >>+ else if (div > desc->div_max) >>+ div = desc->div_max; >>+ return DIV_ROUND_UP(parent_rate, div); > >DIV_ROUND_CLOSEST? I'm hesitant to change the logic on this, as it could subtly alter the values. >>+ /* TODO: use the .div_table if provided */ >>+ if (desc->div_table[0]) >>+ pr_err("ERROR: %s: div_table not implemented\n", __func__); > >dev_err > >But can't you just leave out the div_table member? Only a few of the clocks have a div_table, but for those, the table specifies the allowable divider values. So right now, the code cannot correctly set such clocks, as it may try programming illegal values (most likely with the result that you don't get the expected rate). The Linux driver does implement the div_table logic, I simply did not carry it over yet to u-boot. Mostly because I have not yet had need for one of the few clocks which does use the table. I do plan to add it. >>+/* >>+ * Main clock driver >>+ */ >>+static int r9a06g032_clk_enable(struct clk *clk) >>+{ >>+ const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk); >>+ >>+ switch (desc->type) { >>+ case K_GATE: >>+ return r9a06g032_clk_gate_enable(clk); >>+ case K_DUALGATE: >>+ return r9a06g032_clk_dualgate_enable(clk); >>+ default: >>+ printf("ERROR: %s:%d unhandled type=%d\n", __func__, __LINE__, desc->type); > >Assert or dev_dbg is better here. This is "impossible" so we try and >avoid increasing image size in these cases. Fair enough. Though I have to say I have been bitten by this kind of thing a few times. After having spent time debugging-via-printf, I would then discover an assert or dev_dbg that points out the exact problem. If only I had enabled DEBUG for that file! And yet if I enable DEBUG globally, there is so much noise, that I don't notice the one message that might have been helpful. >The overall structure looks good; most of these things you >should be able to iron out fairly easily. I have skipped over a few of the smaller points regarding return values, etc, but I will address them in next version of the patch. Thanks again for your time reviewing this. Regards, -Ralph