linux-kernel - Re: [RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c910527d-e2d7-31ca-efd9-1915db62d85d@sholland.org>
Date:   Tue, 28 Sep 2021 02:46:39 -0500
From:   Samuel Holland <samuel@...lland.org>
To:     Maxime Ripard <maxime@...no.tech>
Cc:     Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
        linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

On 9/9/21 3:45 AM, Maxime Ripard wrote:
> On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote:
>> On 9/3/21 9:50 AM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote:
>>>> This patch series adds a CCU driver for the RTC in the H616 and R329.
>>>> The extra patches at the end of this series show how it would be
>>>> explanded to additional hardware variants.
>>>>
>>>> The driver is intended to support the existing binding used for the H6,
>>>> but also an updated binding which includes all RTC input clocks. I do
>>>> not know how to best represent that binding -- that is a major reason
>>>> why this series is an RFC.
>>>>
>>>> A future patch series could add functionality to the driver to manage
>>>> IOSC calibration at boot and during suspend/resume.
>>>>
>>>> It may be possible to support all of these hardware variants in the
>>>> existing RTC clock driver and avoid some duplicate code, but I'm
>>>> concerned about the complexity there, without any of the CCU
>>>> abstraction.
>>>>
>>>> This series is currently based on top of the other series I just sent
>>>> (clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
>>>> elsewhere.
>>>
>>> I'm generally ok with this, it makes sense to move it to sunxi-ng,
>>> especially with that other series of yours.
>>>
>>> My main concern about this is the split driver approach. We used to have
>>> that before in the RTC too, but it was mostly due to the early clock
>>> requirements. With your previous work, that requirement is not there
>>> anymore and we can just register it as a device just like the other
>>> clock providers.
>>
>> That's a good point. Originally, I had this RTC CCU providing osc24M, so
>> it did need to be an early provider. But with the current version, we
>> could have the RTC platform driver call devm_sunxi_ccu_probe. That does
>> seem cleaner.
>>
>> (Since it wasn't immediately obvious to me why this works: the only
>> early provider remaining is the sun5i CCU, and it doesn't use the sun6i
>> RTC driver.)
>>
>>> And since we can register all those clocks at device probe time, we
>>> don't really need to split the driver in two (and especially in two
>>> different places). The only obstacle to this after your previous series
>>> is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
>>> functions public, but that can easily be fixed by moving their
>>> definition to include/linux/clk/sunxi-ng.h
>>
>> Where are you thinking the clock definitions would go? We don't export
>> any of those structures (ccu_mux, ccu_common) or macros
>> (SUNXI_CCU_GATE_DATA) in a public header either.
> 
> Ah, right...
> 
>> Would you want to export those? That seems like a lot of churn. Or would
>> we put the CCU descriptions in drivers/clk/sunxi-ng and export a
>> function that the RTC driver can call? (Or some other idea?)
> 
> I guess we could export it. There's some fairly big headers in
> include/linux/clk already (tegra and ti), it's not uAPI and we do have
> reasons to do so, so I guess it's fine.
> 
> I'd like to avoid having two drivers for the same device if possible,
> especially in two separate places. This creates some confusion since the
> general expectation is that there's only one driver per device. There's
> also the fact that this could lead to subtle bugs since the probe order
> is the link order (or module loading).

I don't think there can be two "struct device"s for a single OF node. So
if the CCU part is in drivers/clk/sunxi-ng, the CCU "probe" function
would have to be called from the RTC driver. Since there has to be
cooperation anyway, I don't think there would be any ordering problems.

> And synchronizing access to registers between those two drivers will be
> hard, while we could just share the same spin lock between the RTC and
> clock drivers if they are instanciated in the same place.

While the RTC driver currently shares a spinlock between the clock part
and the RTC part, there isn't actually any overlap in register usage
between the two. So there doesn't need to be any synchronization.

Regards,
Samuel