Re: [PATCH v8 2/3] PCI: rockchip: Simplify reset control handling by using reset_control_bulk*() function

[Manivannan Sadhasivam]

On Tue, Oct 15, 2024 at 02:30:23PM +0530, Anand Moon wrote:
> Hi Manivannan,
> 
> Thanks for your review comments.
> 
> On Tue, 15 Oct 2024 at 10:41, Manivannan Sadhasivam
> <manivannan.sadhasivam@xxxxxxxxxx> wrote:
> >
> > On Mon, Oct 14, 2024 at 07:22:03PM +0530, Anand Moon wrote:
> > > Refactor the reset control handling in the Rockchip PCIe driver,
> > > introduce a more robust and efficient method for assert and
> > > deassert reset controller using reset_control_bulk*() API. Using the
> > > reset_control_bulk APIs, the reset handling for the core clocks reset
> > > unit becomes much simpler.
> > >
> > > Spilt the reset controller in two groups as per the
> > >  RK3399 TM  17.5.8.1 PCIe Initialization Sequence
> > >     17.5.8.1.1 PCIe as Root Complex.
> > >
> > > 6. De-assert the PIPE_RESET_N/MGMT_STICKY_RESET_N/MGMT_RESET_N/RESET_N
> > >    simultaneously.
> > >
> >
> > I'd reword it slightly:
> >
> > Following the recommendations in 'Rockchip RK3399 TRM v1.3 Part2':
> >
> > 1. Split the reset controls into two groups as per section '17.5.8.1.1 PCIe
> > as Root Complex'.
> >
> > 2. Deassert the 'Pipe, MGMT Sticky, MGMT, Core' resets in groups as per section
> > '17.5.8.1.1 PCIe as Root Complex'. This is accomplished using the
> > reset_control_bulk APIs.
> >
> > > - devm_reset_control_bulk_get_exclusive(): Allows the driver to get all
> > >   resets defined in the DT thereby removing the hardcoded reset names
> > >   in the driver.
> > > - reset_control_bulk_assert(): Allows the driver to assert the resets
> > >   defined in the driver.
> > > - reset_control_bulk_deassert(): Allows the driver to deassert the resets
> > >   defined in the driver.
> > >
> >
> > No need to list out the APIs. Just add them to the first paragraph itself to
> > explain how they are used.
> >
> 
> Here is a short version of the commit message.
> 
> Introduce a more robust and efficient method for assert and deassert
> the reset controller using the reset_control_bulk*() API.
> Simplify reset handling for the core clocks reset unit with the
> reset_control_bulk APIs.
> 
> devm_reset_control_bulk_get_exclusive(): Obtain all resets from the
>             device tree, removing hardcoded names.
> reset_control_bulk_assert(): assert the resets defined in the driver.
> reset_control_bulk_deassert(): deassert the resets defined in the driver..
> 

How about,

"Currently, the driver acquires and asserts/deasserts the resets individually
thereby making the driver complex to read. But this can be simplified by using
the reset_control_bulk APIs. Use devm_reset_control_bulk_get_exclusive() API to
acquire all the resets and use reset_control_bulk_{assert/deassert}() APIs to
assert/deassert them in bulk.

Also follow the recommendations provided in 'Rockchip RK3399 TRM v1.3 Part2':
..."

- Mani

> Following the recommendations in 'Rockchip RK3399 TRM v1.3 Part2':
> 
> 1. Split the reset controls into two groups as per section '17.5.8.1.1 PCIe
> as Root Complex'.
> 
> 2. Deassert the 'Pipe, MGMT Sticky, MGMT, Core' resets in groups as per section
> '17.5.8.1.1 PCIe as Root Complex'. This is accomplished using the
> reset_control_bulk APIs.
> 
> Does this look good to you? Let me know if you need any further adjustments!
> 
> I will fix this for CLK bulk as well.
> 
> > > Signed-off-by: Anand Moon <linux.amoon@xxxxxxxxx>
> >
> > Some nitpicks below. Rest looks good.
> 
> I will fix these in the next version.
> 
> > - Mani
> >
> > --
> > மணிவண்ணன் சதாசிவம்
> 
> Thanks
> -Anand

-- 
மணிவண்ணன் சதாசிவம்