Re: [PATCH RESEND v5 6/8] clk: baikal-t1: Move reset-controls code into a dedicated module

[Serge Semin]

Hi Philipp

On Thu, Jul 07, 2022 at 01:10:18AM +0300, Serge Semin wrote:
> On Wed, Jul 06, 2022 at 11:16:34AM +0200, Philipp Zabel wrote:
> > 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.
> 
> Agreed then. I'll update the patches and resend the series shortly.
> Thank you very much for review.

The series has been updated the way you asked. Please review:

Link: https://lore.kernel.org/linux-clk/20220708192725.9501-1-Sergey.Semin@xxxxxxxxxxxxxxxxxxxx/

-Sergey

> 
> -Sergey
> 
> > 
> > [...]
> > > > 
> > > > 
> > > > 
> > > > 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