Re: [PATCH 3/9] PCI: imx6: Simplify reset handling by using by using *_FLAG_HAS_*_RESET

[Frank Li <Frank.li@nxp.com>]

On Wed, Dec 06, 2023 at 05:52:23PM +0100, Philipp Zabel wrote:
> Hi Frank,
> 
> On Mi, 2023-12-06 at 10:58 -0500, Frank Li wrote:
> > Refactors the reset handling logic in the imx6 PCI driver by adding
> > IMX6_PCIE_FLAG_HAS_*_RESET bitmask define for drvdata::flags.
> > 
> > The drvdata::flags and a bitmask ensures a cleaner and more scalable
> > switch-case structure for handling reset.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 115 +++++++++++---------------
> >  1 file changed, 47 insertions(+), 68 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index bcf52aa86462..62d77fabd82a 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> [...]
> > @@ -696,18 +698,13 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> >  
> >  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> >  {
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX7D:
> > -	case IMX8MQ:
> > -	case IMX8MQ_EP:
> > +	if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHY_RESET))
> 
> This check is not strictly necessary. If the flag is not set,
> imx6_pcie->pciephy_reset is guaranteed to be NULL, and then
> reset_control_assert() is a no-op. Same for the other (de)assert
> calls below.
> 
> [...]
> 
> > @@ -1335,36 +1319,24 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> >  					     "failed to get pcie phy\n");
> >  	}
> >  
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX7D:
> > -		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > -			imx6_pcie->controller_id = 1;
> > -
> > -		imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev,
> > -									    "pciephy");
> > -		if (IS_ERR(imx6_pcie->pciephy_reset)) {
> > -			dev_err(dev, "Failed to get PCIEPHY reset control\n");
> > -			return PTR_ERR(imx6_pcie->pciephy_reset);
> > -		}
> > -
> > -		imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> > -									 "apps");
> > -		if (IS_ERR(imx6_pcie->apps_reset)) {
> > -			dev_err(dev, "Failed to get PCIE APPS reset control\n");
> > -			return PTR_ERR(imx6_pcie->apps_reset);
> > -		}
> > -		break;
> > -	case IMX8MM:
> > -	case IMX8MM_EP:
> > -	case IMX8MP:
> > -	case IMX8MP_EP:
> > -		imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> > -									 "apps");
> > +	if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_APP_RESET)) {
> > +		imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev, "apps");
> [...]
> 
> I wonder whether we should just defer the check whether apps/pciephy
> resets should be used to the device tree validation, and make this an
> unconditional call to get an optional reset control:
> 
> 	imx6_pcie->apps_reset = devm_reset_control_get_optional_exclusive(dev, "apps");

I think double check here is neccesary. No sure if dts file version
binding yaml was exactly matched driver. Sometime user use old version dts. 
Double check here will help identify the dts problem. 

Frank 
> 
> regards
> Philipp