Message-ID: <f28de0c61c06396e36756f2d4f3379fab26abdbf.camel@pengutronix.de>
Date:   Wed, 06 Jul 2022 11:16:34 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Serge Semin <fancer.lancer@...il.com>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        linux-clk@...r.kernel.org, linux-mips@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND v5 6/8] clk: baikal-t1: Move reset-controls code
 into a dedicated module

Hi Serge,

On Mi, 2022-07-06 at 01:07 +0300, Serge Semin wrote:
[...]
> > What is the reason for separating ccu-rst.c and clk-ccu-rst.c?
> > 
> > I expect implementing the reset ops and registering the reset
> > controller in the same compilation unit would be easier.
> 
> From the very beginning of the Baikal-T1 driver live the Clock/Reset functionality
> has been split up into two parts:
> 1. ccu-{div,pll}.c - Clock/Reset operations implementation.
> 2. clk-ccu-{div,pll}.c - Clock/Reset kernel interface implementation.
> At least for the clk-part it has made the driver much easier to read.
> Code in 1. provides the interface methods like
> ccu_{div,pll}_hw_register() to register a clock provider corresponding
> to the CCU divider/PLL of the particular type. Code in 2. uses these
> methods to create the CCU Dividers/PLL clock descriptors and register
> the of-based clocks in the system. The reset functionality was
> redistributed in the same manner in the framework of the ccu-div.c and
> clk-ccu-div.c modules.
> 
> A similar approach I was trying to utilize in the framework of the
> separate CCU Resets implementation. Although it turned out to be not as
> handy as it was for the clock-part due to the different clock and
> reset subsystems API (clock subsystem provides a single clock
> source based API, while the reset subsystem expects to have the whole
> resets controller described). Anyway I've decided to preserve as much
> similarities as possible for the sake of the code unification and
> better readability/maintainability. Thus the reset lines control
> methods have been placed in the ccu-rst.c object file, while the reset
> control registration has been implemented in the clk-ccu-rst.c module.

Thank you for the detailed explanation. I think that splitting doesn't
help readability much in this case, but I realize that may just be a
matter of preference.

[...]
> > I don't think this is necessary, see my comments below. Since the reset
> > ids are contiguous, just setting nr_resets and using the default
> > .of_xlate should be enough to make sure this is never called with an
> > invalid id.
> 
> Using non-contiguous !Clock! IDs turned to be unexpectedly handy. Due to
> that design I was able to add the internal clock providers hidden from
> the DTS users but still visible in the clocks hierarchy. It has made the
> clocks implementation as detailed as possible and protected from the
> improper clocks usage. It also simplified a new clock providers adding
> in future (though there won't be clock sources left undefined in the
> SoC after this patchset is applied).
> 
> All of that made me thinking that the same approach can be useful in
> the framework of the CCU reset controls implementation too at the very
> least for the code unification. Although after the next patch in the
> series is applied there won't be resets left undefined in the
> Baikal-T1 SoC. So from another side you might be partly right on
> suggesting to drop the independent reset IDs/descriptors design and
> just assume the IDs contiguousness.
> 
> So could you please confirm that you still insists on dropping it?

Please drop it, then. I don't think there is value in carrying this
complexity just because it makes the code more similar to the
neighboring clk code.

I'd prefer to keep the reset ids contiguous, so future hardware should
just get a different set of contiguous IDs, or new IDs appended
contiguously as you do in patch 7.

[...]
> > 
> > 
> > 
> > I would fold this into ccu_rst_hw_unregister().
> 
> I disagree in this part. Splitting up the interface methods in a set
> of the small coherent methods like protagonists and respective
> antagonists makes the code much easier to read and maintain. So I
> will insist on having the ccu_rst_free_data() method even if it is
> left with only a single kfree() function invocation.
[...]
> I have to disagree for the same reason as I would preserve the
> ccu_rst_free_data() method here. Please see my comment above.

I'm fine with that.

> 
regards
Philipp

Powered by blists - more mailing lists