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