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:

quoted

On Thu, Mar 07, 2024 at 09:36:56PM +0100, Niklas Cassel wrote:

quoted

On Mon, Mar 04, 2024 at 02:52:18PM +0530, Manivannan Sadhasivam wrote:

quoted

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 <redacted>
---
 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().

quoted

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(&pci->ep);
 		if (ret < 0)
 			goto err_get_sync;
+
+		ret = dw_pcie_ep_init_registers(&pci->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) {
	<error handling>
}

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) {
	<error handling>
}

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

-- 
மணிவண்ணன் சதாசிவம்