Re: [PATCH v9 06/10] PCI: dwc: ep: Call dw_pcie_ep_init_registers() API directly from all glue drivers
On Fri, Mar 08, 2024 at 11:22:52AM +0100, Niklas Cassel wrote: > On Fri, Mar 08, 2024 at 03:19:47PM +0530, Manivannan Sadhasivam wrote: > > > > > > @@ -467,6 +467,13 @@ static int dra7xx_add_pcie_ep(struct > > > > > > dra7xx_pcie *dra7xx, > > > > > > return ret; > > > > > > } > > > > > > > > > > > > + ret = dw_pcie_ep_init_registers(ep); > > > > > > + if (ret) { > > > > > > > > > > Here you are using if (ret) to error check the return from > > > > > dw_pcie_ep_init_registers(). > > > > > > > > > > > > > > > > index c0c62533a3f1..8392894ed286 100644 > > > > > > --- a/drivers/pci/controller/dwc/pci-keystone.c > > > > > > +++ b/drivers/pci/controller/dwc/pci-keystone.c > > > > > > @@ -1286,6 +1286,13 @@ static int ks_pcie_probe(struct > > > > > > platform_device *pdev) > > > > > > ret = dw_pcie_ep_init(>ep); > > > > > > if (ret < 0) > > > > > > goto err_get_sync; > > > > > > + > > > > > > + ret = dw_pcie_ep_init_registers(>ep); > > > > > > + if (ret < 0) { > > > > > > > > > > Here you are using if (ret < 0) to error check the return from > > > > > dw_pcie_ep_init_registers(). Please be consistent. > > > > > > > > > > > > > I maintained the consistency w.r.t individual drivers. Please check them > > > > individually. > > > > > > > > If I maintain consistency w.r.t this patch, then the style will change > > > > within > > > > the drivers. > > > > > > Personally, I disagree with that. > > > > > > All glue drivers should use the same way of checking dw_pcie_ep_init(), > > > depending on the kdoc of dw_pcie_ep_init(). > > > > > > If the kdoc for dw_pcie_ep_init() says returns 0 on success, > > > then I think that it is strictly more correct to do: > > > > > > ret = dw_pcie_ep_init() > > > if (ret) { > > > > > > } > > > > > > And if a glue driver doesn't look like that, then I think we should change > > > them. (Same reasoning for dw_pcie_ep_init_registers().) > > > > > > > > > If you read code that looks like: > > > ret = dw_pcie_ep_init() > > > if (ret < 0) { > > > > > > } > > > > > > then you assume that is is a function with a kdoc that says it can return > > > 0 > > > or a positive value on success, e.g. a function that returns an index in > > > an > > > array. > > > > > > > But if you read the same function from the individual drivers, it could > > present > > a different opinion because the samantics is different than others. > > Is there any glue driver where a positive result from dw_pcie_ep_init() is > considered valid? > > > > > > I'm not opposed to keeping the API semantics consistent, but we have to take > > account of the drivers style as well. > > kdoc > "driver style" > IMO, but you are the maintainer, I just offered my 50 cents :) > Those valuable 50 cents :) Looking at it again, I think you are right. We should honor the API over driver's own style. I've changed the semantics in next version, thanks! - Mani -- மணிவண்ணன் சதாசிவம்
RE: [PATCH v9 06/10] PCI: dwc: ep: Call dw_pcie_ep_init_registers() API directly from all glue drivers
> From: Manivannan Sadhasivam, Sent: Monday, March 4, 2024 6:22 PM > > Currently, dw_pcie_ep_init_registers() API is directly called by the glue > drivers requiring active refclk from host. But for the other drivers, it is > getting called implicitly by dw_pcie_ep_init(). This is due to the fact > that this API initializes DWC EP specific registers and that requires an > active refclk (either from host or generated locally by endpoint itsef). > > But, this causes a discrepancy among the glue drivers. So to avoid this > confusion, let's call this API directly from all glue drivers irrespective > of refclk dependency. Only difference here is that the drivers requiring > refclk from host will call this API only after the refclk is received and > other drivers without refclk dependency will call this API right after > dw_pcie_ep_init(). > > With this change, the check for 'core_init_notifier' flag can now be > dropped from dw_pcie_ep_init() API. This will also allow us to remove the > 'core_init_notifier' flag completely in the later commits. > > Signed-off-by: Manivannan Sadhasivam > --- > drivers/pci/controller/dwc/pci-dra7xx.c | 7 +++ > drivers/pci/controller/dwc/pci-imx6.c | 8 > drivers/pci/controller/dwc/pci-keystone.c | 9 + > drivers/pci/controller/dwc/pci-layerscape-ep.c| 7 +++ > drivers/pci/controller/dwc/pcie-artpec6.c | 13 - > drivers/pci/controller/dwc/pcie-designware-ep.c | 22 -- > drivers/pci/controller/dwc/pcie-designware-plat.c | 9 + > drivers/pci/controller/dwc/pcie-keembay.c | 16 +++- > drivers/pci/controller/dwc/pcie-rcar-gen4.c | 12 +++- Thank you for the patch! About pcie-rcar-gen4.c, Reviewed-by: Yoshihiro Shimoda Best regards, Yoshihiro Shimoda > drivers/pci/controller/dwc/pcie-uniphier-ep.c | 13 - > 10 files changed, 90 insertions(+), 26 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c > b/drivers/pci/controller/dwc/pci-dra7xx.c > index 0e406677060d..395042b29ffc 100644 > --- a/drivers/pci/controller/dwc/pci-dra7xx.c > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > @@ -467,6 +467,13 @@ static int dra7xx_add_pcie_ep(struct dra7xx_pcie *dra7xx, > return ret; > } > > + ret = dw_pcie_ep_init_registers(ep); > + if (ret) { > + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); > + dw_pcie_ep_deinit(ep); > + return ret; > + } > + > return 0; > } > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > b/drivers/pci/controller/dwc/pci-imx6.c > index dc2c036ab28c..bfcafa440ddb 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -1136,6 +1136,14 @@ static int imx6_add_pcie_ep(struct imx6_pcie > *imx6_pcie, > dev_err(dev, "failed to initialize endpoint\n"); > return ret; > } > + > + ret = dw_pcie_ep_init_registers(ep); > + if (ret) { > + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); > + dw_pcie_ep_deinit(ep); > + return ret; > + } > + > /* Start LTSSM. */ > imx6_pcie_ltssm_enable(dev); > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c > b/drivers/pci/controller/dwc/pci-keystone.c > index c0c62533a3f1..8392894ed286 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -1286,6 +1286,13 @@ static int ks_pcie_probe(struct platform_device *pdev) > ret = dw_pcie_ep_init(>ep); > if (ret < 0) > goto err_get_sync; > + > + ret = dw_pcie_ep_init_registers(>ep); > + if (ret < 0) { > + dev_err(dev, "Failed to initialize DWC endpoint > registers\n"); > + goto err_ep_init; > + } > + > break; > default: > dev_err(dev, "INVALID device type %d\n", mode); > @@ -1295,6 +1302,8 @@ static int ks_pcie_probe(struct platform_device *pdev) > > return 0; > > +err_ep_init: > + dw_pcie_ep_deinit(>ep); > err_get_sync: > pm_runtime_put(dev); > pm_runtime_disable(dev); > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c > b/drivers/pci/controller/dwc/pci-layerscape-ep.c > index 2e398494e7c0..b712fdd06549 100644 > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c > @@ -276,6 +276,13 @@ static int __init ls_pcie_ep_probe(struct > platform_device *pdev) > if (ret) > return ret; > > + ret = dw_pcie_ep_init_registers(>ep); > + if (ret) { > + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); > + dw_pcie_ep_deinit(>ep); > + return ret; > + } > + > return
Re: [PATCH v9 06/10] PCI: dwc: ep: Call dw_pcie_ep_init_registers() API directly from all glue drivers
On Fri, Mar 08, 2024 at 03:19:47PM +0530, Manivannan Sadhasivam wrote: > > > > > @@ -467,6 +467,13 @@ static int dra7xx_add_pcie_ep(struct dra7xx_pcie > > > > > *dra7xx, > > > > > return ret; > > > > > } > > > > > > > > > > + ret = dw_pcie_ep_init_registers(ep); > > > > > + if (ret) { > > > > > > > > Here you are using if (ret) to error check the return from > > > > dw_pcie_ep_init_registers(). > > > > > > > > > > > > > index c0c62533a3f1..8392894ed286 100644 > > > > > --- a/drivers/pci/controller/dwc/pci-keystone.c > > > > > +++ b/drivers/pci/controller/dwc/pci-keystone.c > > > > > @@ -1286,6 +1286,13 @@ static int ks_pcie_probe(struct > > > > > platform_device *pdev) > > > > > ret = dw_pcie_ep_init(>ep); > > > > > if (ret < 0) > > > > > goto err_get_sync; > > > > > + > > > > > + ret = dw_pcie_ep_init_registers(>ep); > > > > > + if (ret < 0) { > > > > > > > > Here you are using if (ret < 0) to error check the return from > > > > dw_pcie_ep_init_registers(). Please be consistent. > > > > > > > > > > I maintained the consistency w.r.t individual drivers. Please check them > > > individually. > > > > > > If I maintain consistency w.r.t this patch, then the style will change > > > within > > > the drivers. > > > > Personally, I disagree with that. > > > > All glue drivers should use the same way of checking dw_pcie_ep_init(), > > depending on the kdoc of dw_pcie_ep_init(). > > > > If the kdoc for dw_pcie_ep_init() says returns 0 on success, > > then I think that it is strictly more correct to do: > > > > ret = dw_pcie_ep_init() > > if (ret) { > > > > } > > > > And if a glue driver doesn't look like that, then I think we should change > > them. (Same reasoning for dw_pcie_ep_init_registers().) > > > > > > If you read code that looks like: > > ret = dw_pcie_ep_init() > > if (ret < 0) { > > > > } > > > > then you assume that is is a function with a kdoc that says it can return 0 > > or a positive value on success, e.g. a function that returns an index in an > > array. > > > > But if you read the same function from the individual drivers, it could > present > a different opinion because the samantics is different than others. Is there any glue driver where a positive result from dw_pcie_ep_init() is considered valid? > > I'm not opposed to keeping the API semantics consistent, but we have to take > account of the drivers style as well. kdoc > "driver style" IMO, but you are the maintainer, I just offered my 50 cents :) Kind regards, Niklas
Re: [PATCH v9 06/10] PCI: dwc: ep: Call dw_pcie_ep_init_registers() API directly from all glue drivers
On Fri, Mar 08, 2024 at 10:05:11AM +0100, Niklas Cassel wrote: > On Fri, Mar 08, 2024 at 11:06:24AM +0530, Manivannan Sadhasivam wrote: > > On Thu, Mar 07, 2024 at 09:36:56PM +0100, Niklas Cassel wrote: > > > On Mon, Mar 04, 2024 at 02:52:18PM +0530, Manivannan Sadhasivam wrote: > > > > Currently, dw_pcie_ep_init_registers() API is directly called by the > > > > glue > > > > drivers requiring active refclk from host. But for the other drivers, > > > > it is > > > > getting called implicitly by dw_pcie_ep_init(). This is due to the fact > > > > that this API initializes DWC EP specific registers and that requires an > > > > active refclk (either from host or generated locally by endpoint itsef). > > > > > > > > But, this causes a discrepancy among the glue drivers. So to avoid this > > > > confusion, let's call this API directly from all glue drivers > > > > irrespective > > > > of refclk dependency. Only difference here is that the drivers requiring > > > > refclk from host will call this API only after the refclk is received > > > > and > > > > other drivers without refclk dependency will call this API right after > > > > dw_pcie_ep_init(). > > > > > > > > With this change, the check for 'core_init_notifier' flag can now be > > > > dropped from dw_pcie_ep_init() API. This will also allow us to remove > > > > the > > > > 'core_init_notifier' flag completely in the later commits. > > > > > > > > Signed-off-by: Manivannan Sadhasivam > > > > --- > > > > drivers/pci/controller/dwc/pci-dra7xx.c | 7 +++ > > > > drivers/pci/controller/dwc/pci-imx6.c | 8 > > > > drivers/pci/controller/dwc/pci-keystone.c | 9 + > > > > drivers/pci/controller/dwc/pci-layerscape-ep.c| 7 +++ > > > > drivers/pci/controller/dwc/pcie-artpec6.c | 13 - > > > > drivers/pci/controller/dwc/pcie-designware-ep.c | 22 > > > > -- > > > > drivers/pci/controller/dwc/pcie-designware-plat.c | 9 + > > > > drivers/pci/controller/dwc/pcie-keembay.c | 16 +++- > > > > drivers/pci/controller/dwc/pcie-rcar-gen4.c | 12 +++- > > > > drivers/pci/controller/dwc/pcie-uniphier-ep.c | 13 - > > > > 10 files changed, 90 insertions(+), 26 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c > > > > b/drivers/pci/controller/dwc/pci-dra7xx.c > > > > index 0e406677060d..395042b29ffc 100644 > > > > --- a/drivers/pci/controller/dwc/pci-dra7xx.c > > > > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > > > > @@ -467,6 +467,13 @@ static int dra7xx_add_pcie_ep(struct dra7xx_pcie > > > > *dra7xx, > > > > return ret; > > > > } > > > > > > > > + ret = dw_pcie_ep_init_registers(ep); > > > > + if (ret) { > > > > > > Here you are using if (ret) to error check the return from > > > dw_pcie_ep_init_registers(). > > > > > > > > > > index c0c62533a3f1..8392894ed286 100644 > > > > --- a/drivers/pci/controller/dwc/pci-keystone.c > > > > +++ b/drivers/pci/controller/dwc/pci-keystone.c > > > > @@ -1286,6 +1286,13 @@ static int ks_pcie_probe(struct platform_device > > > > *pdev) > > > > ret = dw_pcie_ep_init(>ep); > > > > if (ret < 0) > > > > goto err_get_sync; > > > > + > > > > + ret = dw_pcie_ep_init_registers(>ep); > > > > + if (ret < 0) { > > > > > > Here you are using if (ret < 0) to error check the return from > > > dw_pcie_ep_init_registers(). Please be consistent. > > > > > > > I maintained the consistency w.r.t individual drivers. Please check them > > individually. > > > > If I maintain consistency w.r.t this patch, then the style will change > > within > > the drivers. > > Personally, I disagree with that. > > All glue drivers should use the same way of checking dw_pcie_ep_init(), > depending on the kdoc of dw_pcie_ep_init(). > > If the kdoc for dw_pcie_ep_init() says returns 0 on success, > then I think that it is strictly more correct to do: > > ret = dw_pcie_ep_init() > if (ret) { > > } > > And if a glue driver doesn't look like that, then I think we should change > them. (Same reasoning for dw_pcie_ep_init_registers().) > > > If you read code that looks like: > ret = dw_pcie_ep_init() > if (ret < 0) { > > } > > then you assume that is is a function with a kdoc that says it can return 0 > or a positive value on success, e.g. a function that returns an index in an > array. > But if you read the same function from the individual drivers, it could present a different opinion because the samantics is different than others. I'm not opposed to keeping the API semantics consistent, but we have to take account of the drivers style as well. - Mani -- மணிவண்ணன் சதாசிவம்
Re: [PATCH v9 06/10] PCI: dwc: ep: Call dw_pcie_ep_init_registers() API directly from all glue drivers
On Fri, Mar 08, 2024 at 11:06:24AM +0530, Manivannan Sadhasivam wrote: > On Thu, Mar 07, 2024 at 09:36:56PM +0100, Niklas Cassel wrote: > > On Mon, Mar 04, 2024 at 02:52:18PM +0530, Manivannan Sadhasivam wrote: > > > Currently, dw_pcie_ep_init_registers() API is directly called by the glue > > > drivers requiring active refclk from host. But for the other drivers, it > > > is > > > getting called implicitly by dw_pcie_ep_init(). This is due to the fact > > > that this API initializes DWC EP specific registers and that requires an > > > active refclk (either from host or generated locally by endpoint itsef). > > > > > > But, this causes a discrepancy among the glue drivers. So to avoid this > > > confusion, let's call this API directly from all glue drivers irrespective > > > of refclk dependency. Only difference here is that the drivers requiring > > > refclk from host will call this API only after the refclk is received and > > > other drivers without refclk dependency will call this API right after > > > dw_pcie_ep_init(). > > > > > > With this change, the check for 'core_init_notifier' flag can now be > > > dropped from dw_pcie_ep_init() API. This will also allow us to remove the > > > 'core_init_notifier' flag completely in the later commits. > > > > > > Signed-off-by: Manivannan Sadhasivam > > > --- > > > drivers/pci/controller/dwc/pci-dra7xx.c | 7 +++ > > > drivers/pci/controller/dwc/pci-imx6.c | 8 > > > drivers/pci/controller/dwc/pci-keystone.c | 9 + > > > drivers/pci/controller/dwc/pci-layerscape-ep.c| 7 +++ > > > drivers/pci/controller/dwc/pcie-artpec6.c | 13 - > > > drivers/pci/controller/dwc/pcie-designware-ep.c | 22 > > > -- > > > drivers/pci/controller/dwc/pcie-designware-plat.c | 9 + > > > drivers/pci/controller/dwc/pcie-keembay.c | 16 +++- > > > drivers/pci/controller/dwc/pcie-rcar-gen4.c | 12 +++- > > > drivers/pci/controller/dwc/pcie-uniphier-ep.c | 13 - > > > 10 files changed, 90 insertions(+), 26 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c > > > b/drivers/pci/controller/dwc/pci-dra7xx.c > > > index 0e406677060d..395042b29ffc 100644 > > > --- a/drivers/pci/controller/dwc/pci-dra7xx.c > > > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > > > @@ -467,6 +467,13 @@ static int dra7xx_add_pcie_ep(struct dra7xx_pcie > > > *dra7xx, > > > return ret; > > > } > > > > > > + ret = dw_pcie_ep_init_registers(ep); > > > + if (ret) { > > > > Here you are using if (ret) to error check the return from > > dw_pcie_ep_init_registers(). > > > > > > > index c0c62533a3f1..8392894ed286 100644 > > > --- a/drivers/pci/controller/dwc/pci-keystone.c > > > +++ b/drivers/pci/controller/dwc/pci-keystone.c > > > @@ -1286,6 +1286,13 @@ static int ks_pcie_probe(struct platform_device > > > *pdev) > > > ret = dw_pcie_ep_init(>ep); > > > if (ret < 0) > > > goto err_get_sync; > > > + > > > + ret = dw_pcie_ep_init_registers(>ep); > > > + if (ret < 0) { > > > > Here you are using if (ret < 0) to error check the return from > > dw_pcie_ep_init_registers(). Please be consistent. > > > > I maintained the consistency w.r.t individual drivers. Please check them > individually. > > If I maintain consistency w.r.t this patch, then the style will change within > the drivers. Personally, I disagree with that. All glue drivers should use the same way of checking dw_pcie_ep_init(), depending on the kdoc of dw_pcie_ep_init(). If the kdoc for dw_pcie_ep_init() says returns 0 on success, then I think that it is strictly more correct to do: ret = dw_pcie_ep_init() if (ret) { } And if a glue driver doesn't look like that, then I think we should change them. (Same reasoning for dw_pcie_ep_init_registers().) If you read code that looks like: ret = dw_pcie_ep_init() if (ret < 0) { } then you assume that is is a function with a kdoc that says it can return 0 or a positive value on success, e.g. a function that returns an index in an array. Kind regards, Niklas
Re: [PATCH v9 06/10] PCI: dwc: ep: Call dw_pcie_ep_init_registers() API directly from all glue drivers
On Thu, Mar 07, 2024 at 09:36:56PM +0100, Niklas Cassel wrote: > On Mon, Mar 04, 2024 at 02:52:18PM +0530, Manivannan Sadhasivam wrote: > > Currently, dw_pcie_ep_init_registers() API is directly called by the glue > > drivers requiring active refclk from host. But for the other drivers, it is > > getting called implicitly by dw_pcie_ep_init(). This is due to the fact > > that this API initializes DWC EP specific registers and that requires an > > active refclk (either from host or generated locally by endpoint itsef). > > > > But, this causes a discrepancy among the glue drivers. So to avoid this > > confusion, let's call this API directly from all glue drivers irrespective > > of refclk dependency. Only difference here is that the drivers requiring > > refclk from host will call this API only after the refclk is received and > > other drivers without refclk dependency will call this API right after > > dw_pcie_ep_init(). > > > > With this change, the check for 'core_init_notifier' flag can now be > > dropped from dw_pcie_ep_init() API. This will also allow us to remove the > > 'core_init_notifier' flag completely in the later commits. > > > > Signed-off-by: Manivannan Sadhasivam > > --- > > drivers/pci/controller/dwc/pci-dra7xx.c | 7 +++ > > drivers/pci/controller/dwc/pci-imx6.c | 8 > > drivers/pci/controller/dwc/pci-keystone.c | 9 + > > drivers/pci/controller/dwc/pci-layerscape-ep.c| 7 +++ > > drivers/pci/controller/dwc/pcie-artpec6.c | 13 - > > drivers/pci/controller/dwc/pcie-designware-ep.c | 22 > > -- > > drivers/pci/controller/dwc/pcie-designware-plat.c | 9 + > > drivers/pci/controller/dwc/pcie-keembay.c | 16 +++- > > drivers/pci/controller/dwc/pcie-rcar-gen4.c | 12 +++- > > drivers/pci/controller/dwc/pcie-uniphier-ep.c | 13 - > > 10 files changed, 90 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c > > b/drivers/pci/controller/dwc/pci-dra7xx.c > > index 0e406677060d..395042b29ffc 100644 > > --- a/drivers/pci/controller/dwc/pci-dra7xx.c > > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > > @@ -467,6 +467,13 @@ static int dra7xx_add_pcie_ep(struct dra7xx_pcie > > *dra7xx, > > return ret; > > } > > > > + ret = dw_pcie_ep_init_registers(ep); > > + if (ret) { > > Here you are using if (ret) to error check the return from > dw_pcie_ep_init_registers(). > > > > index c0c62533a3f1..8392894ed286 100644 > > --- a/drivers/pci/controller/dwc/pci-keystone.c > > +++ b/drivers/pci/controller/dwc/pci-keystone.c > > @@ -1286,6 +1286,13 @@ static int ks_pcie_probe(struct platform_device > > *pdev) > > ret = dw_pcie_ep_init(>ep); > > if (ret < 0) > > goto err_get_sync; > > + > > + ret = dw_pcie_ep_init_registers(>ep); > > + if (ret < 0) { > > Here you are using if (ret < 0) to error check the return from > dw_pcie_ep_init_registers(). Please be consistent. > I maintained the consistency w.r.t individual drivers. Please check them individually. If I maintain consistency w.r.t this patch, then the style will change within the drivers. - Mani > > > diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c > > b/drivers/pci/controller/dwc/pcie-artpec6.c > > index 9ed0a9ba7619..0edd9ab3f139 100644 > > --- a/drivers/pci/controller/dwc/pcie-artpec6.c > > +++ b/drivers/pci/controller/dwc/pcie-artpec6.c > > @@ -441,7 +441,18 @@ static int artpec6_pcie_probe(struct platform_device > > *pdev) > > > > pci->ep.ops = _ep_ops; > > > > - return dw_pcie_ep_init(>ep); > > + ret = dw_pcie_ep_init(>ep); > > + if (ret < 0) > > Here you are using if (ret < 0) to error check the return from > dw_pcie_ep_init(). > > > > index 778588b4be70..ca9b22e654cd 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-plat.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-plat.c > > @@ -145,6 +145,15 @@ static int dw_plat_pcie_probe(struct platform_device > > *pdev) > > > > pci->ep.ops = _ep_ops; > > ret = dw_pcie_ep_init(>ep); > > + if (ret) > > Here you are using if (ret) to error check the return from > dw_pcie_ep_init(). Please be consistent. -- மணிவண்ணன் சதாசிவம்
Re: [PATCH v9 06/10] PCI: dwc: ep: Call dw_pcie_ep_init_registers() API directly from all glue drivers
On Mon, Mar 04, 2024 at 02:52:18PM +0530, Manivannan Sadhasivam wrote: > Currently, dw_pcie_ep_init_registers() API is directly called by the glue > drivers requiring active refclk from host. But for the other drivers, it is > getting called implicitly by dw_pcie_ep_init(). This is due to the fact > that this API initializes DWC EP specific registers and that requires an > active refclk (either from host or generated locally by endpoint itsef). > > But, this causes a discrepancy among the glue drivers. So to avoid this > confusion, let's call this API directly from all glue drivers irrespective > of refclk dependency. Only difference here is that the drivers requiring > refclk from host will call this API only after the refclk is received and > other drivers without refclk dependency will call this API right after > dw_pcie_ep_init(). > > With this change, the check for 'core_init_notifier' flag can now be > dropped from dw_pcie_ep_init() API. This will also allow us to remove the > 'core_init_notifier' flag completely in the later commits. > > Signed-off-by: Manivannan Sadhasivam > --- > drivers/pci/controller/dwc/pci-dra7xx.c | 7 +++ > drivers/pci/controller/dwc/pci-imx6.c | 8 > drivers/pci/controller/dwc/pci-keystone.c | 9 + > drivers/pci/controller/dwc/pci-layerscape-ep.c| 7 +++ > drivers/pci/controller/dwc/pcie-artpec6.c | 13 - > drivers/pci/controller/dwc/pcie-designware-ep.c | 22 -- > drivers/pci/controller/dwc/pcie-designware-plat.c | 9 + > drivers/pci/controller/dwc/pcie-keembay.c | 16 +++- > drivers/pci/controller/dwc/pcie-rcar-gen4.c | 12 +++- > drivers/pci/controller/dwc/pcie-uniphier-ep.c | 13 - > 10 files changed, 90 insertions(+), 26 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c > b/drivers/pci/controller/dwc/pci-dra7xx.c > index 0e406677060d..395042b29ffc 100644 > --- a/drivers/pci/controller/dwc/pci-dra7xx.c > +++ b/drivers/pci/controller/dwc/pci-dra7xx.c > @@ -467,6 +467,13 @@ static int dra7xx_add_pcie_ep(struct dra7xx_pcie *dra7xx, > return ret; > } > > + ret = dw_pcie_ep_init_registers(ep); > + if (ret) { Here you are using if (ret) to error check the return from dw_pcie_ep_init_registers(). > index c0c62533a3f1..8392894ed286 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -1286,6 +1286,13 @@ static int ks_pcie_probe(struct platform_device *pdev) > ret = dw_pcie_ep_init(>ep); > if (ret < 0) > goto err_get_sync; > + > + ret = dw_pcie_ep_init_registers(>ep); > + if (ret < 0) { Here you are using if (ret < 0) to error check the return from dw_pcie_ep_init_registers(). Please be consistent. > diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c > b/drivers/pci/controller/dwc/pcie-artpec6.c > index 9ed0a9ba7619..0edd9ab3f139 100644 > --- a/drivers/pci/controller/dwc/pcie-artpec6.c > +++ b/drivers/pci/controller/dwc/pcie-artpec6.c > @@ -441,7 +441,18 @@ static int artpec6_pcie_probe(struct platform_device > *pdev) > > pci->ep.ops = _ep_ops; > > - return dw_pcie_ep_init(>ep); > + ret = dw_pcie_ep_init(>ep); > + if (ret < 0) Here you are using if (ret < 0) to error check the return from dw_pcie_ep_init(). > index 778588b4be70..ca9b22e654cd 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-plat.c > +++ b/drivers/pci/controller/dwc/pcie-designware-plat.c > @@ -145,6 +145,15 @@ static int dw_plat_pcie_probe(struct platform_device > *pdev) > > pci->ep.ops = _ep_ops; > ret = dw_pcie_ep_init(>ep); > + if (ret) Here you are using if (ret) to error check the return from dw_pcie_ep_init(). Please be consistent.