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

[Anand Moon]

Hi Manivannan,

On Sat, 12 Oct 2024 at 17:35, Manivannan Sadhasivam
<manivannan.sadhasivam@xxxxxxxxxx> wrote:
>
> On Sat, Oct 12, 2024 at 03:34:25PM +0530, Anand Moon wrote:
> > Hi Manivannan,
> >
> > On Sat, 12 Oct 2024 at 13:30, Manivannan Sadhasivam
> > <manivannan.sadhasivam@xxxxxxxxxx> wrote:
> > >
> > > On Sat, Oct 12, 2024 at 12:55:32PM +0530, Anand Moon wrote:
> > > > Hi Manivannan,
> > > >
> > > > Thanks for your review comments.
> > > >
> > > > On Sat, 12 Oct 2024 at 11:48, Manivannan Sadhasivam
> > > > <manivannan.sadhasivam@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Sat, Oct 12, 2024 at 10:36:04AM +0530, Anand Moon wrote:
> > > > > > Refactor the reset control handling in the Rockchip PCIe driver,
> > > > > > introducing 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.
> > > > > >
> > > > >
> > > > > Same comments as previous patch.
> > > > >
> > > > I will explain more about this.
> > > > > > Spilt the reset controller in two groups as pre the RK3399 TRM.
> > > > >
> > > > > *per
> > > > >
> > > > > Also please state the TRM name and section for reference.
> > > > >
> > > > Yes
> > > > > > After power up, the software driver should de-assert the reset of PCIe PHY,
> > > > > > then wait the PLL locked by polling the status, if PLL
> > > > > > has locked, then can de-assert the reset simultaneously
> > > > > > driver need to De-assert the reset pins simultionaly.
> > > > > >
> > > > > >   PIPE_RESET_N/MGMT_STICKY_RESET_N/MGMT_RESET_N/RESET_N.
> > > > > >
> > > > > > - replace devm_reset_control_get_exclusive() with
> > > > > >       devm_reset_control_bulk_get_exclusive().
> > > > > > - replace reset_control_assert with
> > > > > >       reset_control_bulk_assert().
> > > > > > - replace reset_control_deassert with
> > > > > >       reset_control_bulk_deassert().
> > > > > >
> > > > > > Signed-off-by: Anand Moon <linux.amoon@xxxxxxxxx>
> > > > > > ---
> > > > > > v7: replace devm_reset_control_bulk_get_optional_exclusive()
> > > > > >         with devm_reset_control_bulk_get_exclusive()
> > > > > >     update the functional changes.
> > > > > > V6: Add reason for the split of the RESET pins.
> > > > > > v5: Fix the De-assert reset core as per the TRM
> > > > > >     De-assert the PIPE_RESET_N/MGMT_STICKY_RESET_N/MGMT_RESET_N/RESET_N
> > > > > >     simultaneously.
> > > > > > v4: use dev_err_probe in error path.
> > > > > > v3: Fix typo in commit message, dropped reported by.
> > > > > > v2: Fix compilation error reported by Intel test robot
> > > > > >     fixed checkpatch warning.
> > > > > > ---
> > > > > >  drivers/pci/controller/pcie-rockchip.c | 151 +++++--------------------
> > > > > >  drivers/pci/controller/pcie-rockchip.h |  26 +++--
> > > > > >  2 files changed, 49 insertions(+), 128 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> > > > > > index 2777ef0cb599..9a118e2b8cbd 100644
> > > > > > --- a/drivers/pci/controller/pcie-rockchip.c
> > > > > > +++ b/drivers/pci/controller/pcie-rockchip.c
> > >
> > > [...]
> > >
> > > > > > @@ -256,31 +181,15 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
> > > > > >        * Please don't reorder the deassert sequence of the following
> > > > > >        * four reset pins.
> > > > >
> > > > > I don't think my earlier comment on this addressed. Why are you changing the
> > > > > reset order? Why can't you have the resets in below (older) order?
> > > > >
> > > > > static const char * const rockchip_pci_core_rsts[] = {
> > > > >         mgmt-sticky",
> > > > >         "core",
> > > > >         "mgmt",
> > > > >         "pipe",
> > > > > };
> > > >  I will add a comment on this above.
> >
> > I get your point, I missed your point.
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-rockchip.c?h=v6.12-rc2#n275
> >
> > Actually I had these changes, but it got missed out in rebase.
> >
> > >
> > > Sorry, I don't get your response. My suggestion was to keep the resets sorted as
> > > the original order (also indicated by my above snippet).
> >
> > I will go through all the suggestions and modify them accordingly.
> >
> > As per the RK3399 TRM
> > [2] https://rockchip.fr/Rockchip%20RK3399%20TRM%20V1.3%20Part2.pdf
> >
> > 17.5.2.2 Reset Application
> > 17.5.2.2.2 System Reset (describe all the core reset feature)
> > (name as per the dts mapping)
> > RESET_N: - core
> > MGMT_RESET_N:- mgmt
> > MGMT_STICKY_RESET_N:- mgmt-sticky
> > PIPE_RESET_N: - pipe
> > AXI_RESET_N - aclk
> > APB_RESET_N: pclk
> > PM_RESET_N: - pm
> > PCIE_PHY_RESET_N: - phy reset (used in the phy driver).
> >
> > This is the reason for the split of the clk and core reset.
> >
> > Further down in
> > 17.5.8 PCIe Operation
> > 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
> >
> > Should I follow this order ? or the above order.
> > static const char * const rockchip_pci_core_rsts[] = {
> >         "pipe",
> >         "mgmt-sticky",
> >          "mgmt",
> >          "core",
> > };
>
> Ok, thanks for clarifying. I was worried about the comment in the driver that
> warns against changing the order. But TRM rececommends a different order :/
>
> But since no one ever reported any issue, let's go with the existing order. If
> you want to follow TRM, then I'd like to get an Ack from Rockchip Engineers who
> knows the hardware better.
>
I will follow the existing code version,
I was confused with the name and description earlier.
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Thanks
-Anand