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 D3CEAECAAD4 for ; Fri, 26 Aug 2022 15:47:34 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id D798E848C1; Fri, 26 Aug 2022 17:47:32 +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="r13AGthz"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 4DABA848C1; Fri, 26 Aug 2022 17:47:31 +0200 (CEST) Received: from mail-io1-xd35.google.com (mail-io1-xd35.google.com [IPv6:2607:f8b0:4864:20::d35]) (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 3D23484409 for ; Fri, 26 Aug 2022 17:47:27 +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-io1-xd35.google.com with SMTP id n202so1467936iod.6 for ; Fri, 26 Aug 2022 08:47:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc; bh=cT81+ddbcQFZ1TOLmwi+naJBUlCGuvS7XL212w4ZH/w=; b=r13AGthz9d/mATZ4jlt7Ce6FbR2z60EXTHF6+Pi7+KBCA3xdeXTyO5uNvPC4vz67nm 5RhnhiAR2CImCxFp7ycj3VWk7c2FM5WdSQEezvzsk+UzRy1WTnlWQOwGg93fsEYF1Amv g0wYCAo/7J8P5QIkq2MNLQH+yPxoAi0pb+sKws4FxslSujC8k/iLzUJ4A8YIToIc0tJK WoOz85zhPtzcuaWEKJkpjdVzAH7kmxbqtvD4AbYscC4zONJsJ6YN55Q2p1CqIxBBf+U6 KLhxuR6pyZgv4HKpo3Q4WXVgKfl47K0tRkHVfjdNjsV8Jnp+vUtjdiCA79bu8LQ1Fp1+ 3SuA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc; bh=cT81+ddbcQFZ1TOLmwi+naJBUlCGuvS7XL212w4ZH/w=; b=5iK85DqUKkybOetqGWxpbUpvxy2vpcYqDABs2M/YkuN0wx11Tr+ZRyYtbvSVZAvf08 6I70wIW7GkBs2VJwg6yuFH0E6snAWER7b3F36zJuknz5HcTjMDdOhIhVkYAdC0sjB71C LK5P6y6N9WnGstRz93K4fu0fh2d8cRUPbhpO4sXL/RXsgGJVOr2kFCh5KaA677BOcrxL PpTLOg3BE7qhf0uuuitlPo38fmXd4WH9RqZaNgQCSgMSIU6zTurHWVc33Ehu5JtO6u5s /cSvy0//t7E+r4jsYwaGeWUJ2AC6Lw5LZb5M9PL2fA/MX4bkNMJG7Af3O0ZVqXEs5j+T /8Nw== X-Gm-Message-State: ACgBeo2rPOCkEBBJceIrJlcpoxh8ZBZX7/NQOn+EcWCqj54pZiK+7mJ/ dbHvMBkd30uYqAaBbWC1/VUZqg== X-Google-Smtp-Source: AA6agR4Opb3H6CqaZhF+FLTQwPZoVTmpxT0J9meAUxvROQdalrKTA6OKyYAsSAX3oSJ7bVMAZ2RDTw== X-Received: by 2002:a5e:a70f:0:b0:684:d596:b7e7 with SMTP id b15-20020a5ea70f000000b00684d596b7e7mr3616224iod.84.1661528845856; Fri, 26 Aug 2022 08:47:25 -0700 (PDT) Received: from localhost (rfs.netwinder.org. [206.248.184.2]) by smtp.gmail.com with ESMTPSA id d93-20020a0285e6000000b00346a29a3160sm1049004jai.9.2022.08.26.08.47.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 Aug 2022 08:47:25 -0700 (PDT) Date: Fri, 26 Aug 2022 11:47:24 -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: <20220826154724.GB1235411@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> <20220815024805.GA488169@maple.netwinder.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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 Tue, Aug 23, 2022 at 12:14:31AM -0400, Sean Anderson wrote: >>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. > >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. I did figure out (mostly) what they are for, so I can see some value in keeping at least some of them. But as you said, they are currently unused, so dropping them from the structure make sense. I have prepared patches for this firstly on the kernel side, and then I will make the same change in this u-boot driver. Stay tuned :-) >>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. > >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/ That link [1] is the current driver, which uses the packed encoding (with a register offset and bit number stored as a packed uint16_t). This is based (loosely) on an earlier version of the driver, which you can find in the Schneider kernel repo [2] on the 4.19 and older branch. This version stores clock information is in the device tree [3] and uses _BIT() macro in the clock tables [4] [2] https://github.com/renesas-rz/rzn1_linux/tree/rzn1-stable-v4.19/drivers/clk/rzn1 [3] https://github.com/renesas-rz/rzn1_linux/blob/rzn1-stable-v4.19/arch/arm/boot/dts/rzn1-clocks.dtsi [4] https://github.com/renesas-rz/rzn1_linux/blob/rzn1-stable-v4.19/drivers/clk/rzn1/rzn1-clkctrl-tables.h Evidently this was not deemed suitable, and thus morphed into the version that did get merged [1]. At least that is my guess, I don't know for sure what transpired. I have modified the clock table so that the register offset and bitnum are explicit values, rather than packed together. I will run this up the kernel side and see if they agree. Am still trying to test it though... >>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. > >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. I guess that is plausible explanation.. I will add a comment... >>>>+    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). > >You call clk_get_rate on it. Any time you "create" a new clock, you >must call clk_request. So the situation here is similar to that on the mediatek patches from Weijie Gao [5], where you made a similar comment. These are not real clocks, they have no ops->request, and the only field used is clk->id. This is done primarily to avoid a bunch of malloc of struct clk, particularly in the early u-boot (before relocation). [5] https://lore.kernel.org/all/31b0e1313267c8d342e0e3d1c9f15eaa8e666114.camel@mediatek.com/ >>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) > ... I've fixed this (and several similar instances elsewhere). >>>DIV_ROUND_CLOSEST? >> >>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. While I can't argue with the math, the linux driver upon which this is based uses DIV_ROUND_UP everywhere. Maybe that is something to also re-consider, but for the time being, I don't want to make the change even more complicated... >>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. > >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. Yep, that works fine when developing a driver... but if you are debugging a board, it is often not clear which file(s) should get sprinkled with DEBUG... and turning it on globally is too verbose to be useful in most cases. Anyhow, thanks for your review, and v3 will be posted eventually, once I get the kernel side sorted. -Ralph