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 90F2BC25B08 for ; Tue, 23 Aug 2022 04:14:41 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 5611684119; Tue, 23 Aug 2022 06:14:39 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com 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=gmail.com header.i=@gmail.com header.b="OXpjIqgJ"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id CA2A6844A8; Tue, 23 Aug 2022 06:14:37 +0200 (CEST) Received: from mail-qv1-xf2d.google.com (mail-qv1-xf2d.google.com [IPv6:2607:f8b0:4864:20::f2d]) (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 D4CE4840D2 for ; Tue, 23 Aug 2022 06:14:33 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=seanga2@gmail.com Received: by mail-qv1-xf2d.google.com with SMTP id m17so9721766qvv.7 for ; Mon, 22 Aug 2022 21:14:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject:from:to:cc; bh=q4YmcFXwSnokM8AWG6us6Nf3K+mZDmcGsjCNIPe9nS8=; b=OXpjIqgJ5mZr3IxpkvixNn8MTMMbbWHdCgQdRlfFl3US77017mAC7rZsFsmdxmSCQ5 IUdCciz9zTZ6HlT92jdEpLs0ceHd9MlTVgfR5nB715mksYCPM4L77RTa0w3fjXAqX8na 9bP086QR5XGDadRRUDHHO+AdqDJZHOdVSujwQV/fsaIXyMf86eStVdQtjuj5FrgmFVdb K6Z1tPea0h7jvoiJ7DAiaOdITL4bfOhEsvMh3MdSZK5juLU0m9bRrgQm2J8kzZ1Y3rGG sBjJGQxyaWxAFb0vTWH9+53KJObwOV94zcArcm4ErncAeieHhqK28ZBMWjOBrwou0bf7 5Jmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject :x-gm-message-state:from:to:cc; bh=q4YmcFXwSnokM8AWG6us6Nf3K+mZDmcGsjCNIPe9nS8=; b=C8HrCPgLvvtycabZxA9+fNzEEbq+q45slLf6o2Li5xOVAVL5sm5xlpqJuYCu6Z+md2 Lw+ljJaxOcO/JB5+4G0+N4X78nnC8ZZgC5hheVe22OXbB6Ntc+ZdkRCnjW1WPK8VUABW fJdUhDbgunAMoabRU8sdr1+G0myO4ITJFvstGpOiO3drJoUUpc0vI8tt728Dm5ZpxeoG L8GFJAdahuKEdNBXroS1allh5/lNEO4IAH/6t0wRW9W3TnsH8PUHjm8CsG5LC76CL9GK 1z4cAuX43cPSb1MJEI4fTzC/N/FJLg6shZhFodHWGksKQE5Gt0ldGMmBTZqCtGr7u8CE YWPA== X-Gm-Message-State: ACgBeo2J+aoP1SoFF8AOIe6t50gN1IkwgXG7hRRnLzbrprZ+iDinx/6a qygGEEAx3sS8B33hctXXqAc= X-Google-Smtp-Source: AA6agR4t3G+pMyaaiurwlSzR9Ihc9vKIEpYxHiVikdBI8dTDB4khw94jZKIFrN0l5Lqg3qj7Xp2jZg== X-Received: by 2002:a05:6214:1d24:b0:496:d058:182b with SMTP id f4-20020a0562141d2400b00496d058182bmr10775962qvd.121.1661228072345; Mon, 22 Aug 2022 21:14:32 -0700 (PDT) Received: from [192.168.1.201] (pool-173-73-95-180.washdc.fios.verizon.net. [173.73.95.180]) by smtp.gmail.com with ESMTPSA id c26-20020ac8111a000000b003431446588fsm10213254qtj.5.2022.08.22.21.14.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 22 Aug 2022 21:14:31 -0700 (PDT) Subject: Re: [RFC PATCH v1 3/9] clk: renesas: add R906G032 driver To: Ralph Siemsen Cc: u-boot@lists.denx.de, Lukasz Majewski References: <20220809125959.217333-1-ralph.siemsen@linaro.org> <20220809125959.217333-4-ralph.siemsen@linaro.org> <152abc91-7460-1a4d-063a-136b4b5f0d4b@gmail.com> <20220815024805.GA488169@maple.netwinder.org> From: Sean Anderson Message-ID: Date: Tue, 23 Aug 2022 00:14:31 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20220815024805.GA488169@maple.netwinder.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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 8/14/22 10:48 PM, Ralph Siemsen wrote: > On Sat, Aug 13, 2022 at 01:30:19AM -0400, Sean Anderson wrote: >> + >>> +=C2=A0=C2=A0=C2=A0 u16 gate, reset, ready, midle, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 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. >=20 > 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 vend= or provided XML or similar documentation. >=20 > 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 fo= und an error in the clock table [1] and I also made some simplifications = [2]. >=20 > Regarding the unused fields (scon, mirack, mistat): I am not really sur= e what their purpose is. Maybe there is some value in having them. I'll t= ry 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. OK, well if you don't use them then perhaps you can just leave them in the macro but remove them from the struct. That way you can add support for them later if you need to, but they don't take up space in the mean time. A comment summarizing your explanation above would be helpful. > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/= commit/?id=3D2dee50ab9e72a3cae75b65e5934c8dd3e9bf01bc >=20 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/= commit/?id=3Df46efcc4746f5c1a539df9db625c04321f75e494 >=20 >>> +}; >>> + >>> +/* This is used to describe a clock for instantiation */ >>> +struct r9a06g032_clkdesc { >>> +=C2=A0=C2=A0=C2=A0 const char *name; >>> +=C2=A0=C2=A0=C2=A0 uint32_t managed: 1; >>> +=C2=A0=C2=A0=C2=A0 uint32_t type: 3; >> >> I wonder if we could define the enum here? >=20 > 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 ac= cepted on the kernel side. >=20 >>> +=C2=A0=C2=A0=C2=A0 uint32_t index: 8; >>> +=C2=A0=C2=A0=C2=A0 uint32_t source : 8; /* source index + 1 (0 =3D=3D= none) */ >>> +=C2=A0=C2=A0=C2=A0 /* these are used to populate the bitsel struct *= / >>> +=C2=A0=C2=A0=C2=A0 union { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct r9a06g032_gate gat= e; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* for dividers */ >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u= nsigned int div_min : 10, div_max : 10, reg: 10; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u= 16 div_table[4]; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* For fixed-factor ones = */ >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u= 16 div, mul; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* for dual gate */ >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u= int16_t group : 1; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u= 16 sel, g1, r1, g2, r2; >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } dual; >>> +=C2=A0=C2=A0=C2=A0 }; >>> +}; >>> + >>> +#define I_GATE(_clk, _rst, _rdy, _midle, _scon, _mirack, _mistat) \ >>> +=C2=A0=C2=A0=C2=A0 { .gate =3D _clk, .reset =3D _rst, \ >> >> If these fields have bitfield inside them, then those bitfields should= >> be assigned/constructed separately. That is, if .reset is actually a c= ombined >> 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=C2=A0=C2=A0=C2=A0 GENMASK(15, 5) >> #define BIT_SHIFT=C2=A0=C2=A0=C2=A0 GENMASK(4, 0) >> >> #define PACK_BIT(offset, shift)=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 (FIELD_PREP(BIT_OFFSET, offset) | FIELD_PREP(BIT_SHIFT, shift)) >=20 > 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. >=20 > At the risk of re-opening old wounds (luckily not mine) I decided to ju= st leave this part exactly as-is in the Linux driver. Can you link to that discussion? The earliest discussion of that series I could find was [1], and there's no mention of the encoding. This encoding scheme seems to be used only for this SoC, and not for any of the other renesas drivers. I suspect that this just wasn't reviewed in detail the first time around... [1] https://lore.kernel.org/all/1527154169-32380-6-git-send-email-michel.= pollet@bp.renesas.com/ >>> + >>> +/* Internal clock IDs */ >>> +#define R9A06G032_CLKOUT=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 0= >>> +#define R9A06G032_CLKOUT_D10=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= 2 >>> +#define R9A06G032_CLKOUT_D16=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= 3 >>> +#define R9A06G032_CLKOUT_D160=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 4 >>> [...] >>> +#define R9A06G032_UART_GROUP_012=C2=A0=C2=A0=C2=A0 154 >>> +#define R9A06G032_UART_GROUP_34567=C2=A0=C2=A0=C2=A0 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 belo= w >> might complain?) >=20 > In fact quite a few of them are in the dt-bindings already, see include= /dt-bindings/clock/r9a06g032-sysctrl.h >=20 > 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 w= as something I just carried over as-is from the Linux driver. I think these are "internal" clocks (that is, clocks which don't really exist like intermediate dividers) whereas the others are public-facing clocks. It's up to you, but maybe have a comment noting where the other ids come from. >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 parent->id =3D ~0;=C2=A0=C2= =A0=C2=A0 /* Top-level clock */ >> >> Can you use a define for this (instead of referring to ~0 everywhere) >=20 > Yes, that sounds reasonable. My list of fixes (for both Linux driver an= d the u-boot one) is going to get rather long ;-) >=20 >>> +=C2=A0=C2=A0=C2=A0 else >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 parent->id =3D desc->sour= ce - 1; >>> + >>> +=C2=A0=C2=A0=C2=A0 parent->dev =3D clk->dev; >> >> I think you need to clk_request here. >=20 > 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 co= mpute the current rate of a given clock. It only looks at the local table= (struct r9a06g032_clkdsc). You call clk_get_rate on it. Any time you "create" a new clock, you must call clk_request. >>> +/* register/bit pairs are encoded as an uint16_t */ >>> +static void >>> +clk_rdesc_set(struct r9a06g032_priv *clocks, >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u16 one, unsi= gned int on) >>> +{ >>> +=C2=A0=C2=A0=C2=A0 uint offset =3D 4 * (one >> 5); >>> +=C2=A0=C2=A0=C2=A0 uint mask =3D 1U << (one & 0x1f); >>> +=C2=A0=C2=A0=C2=A0 uint val =3D ((!!on) << (one & 0x1f)); >> >> Please either use bitfields for this, or use FIELD_GET() and friends. >=20 > Yes, this would be clearer - however as mentioned above, there was alre= ady quite a bit of teeth-gnashing about this encoding. I will prepare a p= atch for the Linux side and see what kind of reply I get. >=20 > I would very much prefer to keep both in sync as much as possible. >=20 >>> +/* >>> + * Fixed factor clock >>> + */ >>> +static ulong r9a06g032_ffc_get_rate(struct clk *clk) >>> +{ >>> +=C2=A0=C2=A0=C2=A0 const struct r9a06g032_clkdesc *desc =3D r9a06g03= 2_clk_get(clk); >>> +=C2=A0=C2=A0=C2=A0 unsigned long parent_rate =3D r9a06g032_clk_get_p= arent_rate(clk); >>> +=C2=A0=C2=A0=C2=A0 unsigned long long rate; >>> + >>> +=C2=A0=C2=A0=C2=A0 if (parent_rate =3D=3D 0) { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 debug("%s: parent_rate is= zero\n", __func__); >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0; >>> +=C2=A0=C2=A0=C2=A0 } >>> + >>> +=C2=A0=C2=A0=C2=A0 rate =3D (unsigned long long)parent_rate * desc->= mul; >>> +=C2=A0=C2=A0=C2=A0 rate =3D DIV_ROUND_UP(rate, desc->div); >>> +=C2=A0=C2=A0=C2=A0 return (ulong)rate; >>> +} >>> + >>> +/* >>> + * This implements R9A06G032 clock divider 'driver'. This differs fr= om 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 f= or this >>> + * bit to clear. >>> + */ >>> +static ulong r9a06g032_div_get_rate(struct clk *clk) >>> +{ >>> +=C2=A0=C2=A0=C2=A0 struct r9a06g032_priv *clocks =3D dev_get_priv(cl= k->dev); >>> +=C2=A0=C2=A0=C2=A0 const struct r9a06g032_clkdesc *desc =3D r9a06g03= 2_clk_get(clk); >>> +=C2=A0=C2=A0=C2=A0 unsigned long parent_rate =3D r9a06g032_clk_get_p= arent_rate(clk); >>> +=C2=A0=C2=A0=C2=A0 u32 div =3D 0; >>> + >>> +=C2=A0=C2=A0=C2=A0 if (parent_rate =3D=3D 0) { >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 debug("%s: parent_rate is= zero\n", __func__); >> >> Didn't you already log this? >=20 > It is for a different clock type. However I will see if I do something = to avoid the duplication. I mean in r9a06g032_clk_get_parent_rate. You can also just do if (!parent_rate) ... >> >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0; >>> +=C2=A0=C2=A0=C2=A0 } >>> + >>> +=C2=A0=C2=A0=C2=A0 regmap_read(clocks->regmap, 4 * desc->reg, &div);= >>> + >>> +=C2=A0=C2=A0=C2=A0 if (div < desc->div_min) >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 div =3D desc->div_min; >>> +=C2=A0=C2=A0=C2=A0 else if (div > desc->div_max) >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 div =3D desc->div_max; >>> +=C2=A0=C2=A0=C2=A0 return DIV_ROUND_UP(parent_rate, div); >> >> DIV_ROUND_CLOSEST? >=20 > I'm hesitant to change the logic on this, as it could subtly alter the = values. Well if you have 2MHz divided by 3, the resulting rate is closer to 666667 kHz than 666666 Hz. >>> +=C2=A0=C2=A0=C2=A0 /* TODO: use the .div_table if provided */ >>> +=C2=A0=C2=A0=C2=A0 if (desc->div_table[0]) >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_err("ERROR: %s: div_ta= ble not implemented\n", __func__); >> >> dev_err >> >> But can't you just leave out the div_table member? >=20 > Only a few of the clocks have a div_table, but for those, the table spe= cifies the allowable divider values. So right now, the code cannot correc= tly set such clocks, as it may try programming illegal values (most likel= y with the result that you don't get the expected rate). >=20 > The Linux driver does implement the div_table logic, I simply did not c= arry it over yet to u-boot. Mostly because I have not yet had need for on= e of the few clocks which does use the table. I do plan to add it. OK >>> +/* >>> + * Main clock driver >>> + */ >>> +static int r9a06g032_clk_enable(struct clk *clk) >>> +{ >>> +=C2=A0=C2=A0=C2=A0 const struct r9a06g032_clkdesc *desc =3D r9a06g03= 2_clk_get(clk); >>> + >>> +=C2=A0=C2=A0=C2=A0 switch (desc->type) { >>> +=C2=A0=C2=A0=C2=A0 case K_GATE: >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return r9a06g032_clk_gate= _enable(clk); >>> +=C2=A0=C2=A0=C2=A0 case K_DUALGATE: >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return r9a06g032_clk_dual= gate_enable(clk); >>> +=C2=A0=C2=A0=C2=A0 default: >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 printf("ERROR: %s:%d unha= ndled type=3D%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. >=20 > Fair enough. Though I have to say I have been bitten by this kind of th= ing a few times. After having spent time debugging-via-printf, I would th= en discover an assert or dev_dbg that points out the exact problem. If on= ly 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 h= ave been helpful. I generally stick a `#define DEBUG` at the top of a file any time I get an error with no message. You still return 0, so I suggest returning -EINVAL (or something else uncommon) so you have somewhere to start. >> The overall structure looks good; most of these things you >> should be able to iron out fairly easily. >=20 > 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. >=20 > Regards, > -Ralph