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.
-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
