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

[Anand Moon]

Hi Manivannan,

On Tue, 15 Oct 2024 at 18:29, Manivannan Sadhasivam
<manivannan.sadhasivam@xxxxxxxxxx> wrote:
>
> 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

Sorry for the trouble,.
Yes, I will try to incorporate your suggestions

Thanks
-Anand
>
> > 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
>
> --
> மணிவண்ணன் சதாசிவம்