| Message ID | 20240304-pci-dbi-rework-v9-6-29d433d99cda@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: dwc: ep: Fix DBI access failure for drivers requiring refclk from host | expand |
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 <manivannan.sadhasivam@linaro.org> > --- > 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(&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. > 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 = &pcie_ep_ops; > > - return dw_pcie_ep_init(&pci->ep); > + ret = dw_pcie_ep_init(&pci->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 = &pcie_ep_ops; > ret = dw_pcie_ep_init(&pci->ep); > + if (ret) Here you are using if (ret) to error check the return from dw_pcie_ep_init(). Please be consistent.
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 <manivannan.sadhasivam@linaro.org> > > --- > > 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(&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. - 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 = &pcie_ep_ops; > > > > - return dw_pcie_ep_init(&pci->ep); > > + ret = dw_pcie_ep_init(&pci->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 = &pcie_ep_ops; > > ret = dw_pcie_ep_init(&pci->ep); > > + if (ret) > > Here you are using if (ret) to error check the return from > dw_pcie_ep_init(). Please be consistent.
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 <manivannan.sadhasivam@linaro.org> > > > --- > > > 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(&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. Kind regards, Niklas
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 <manivannan.sadhasivam@linaro.org> > > > > --- > > > > 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(&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
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(&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. 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
> 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 <manivannan.sadhasivam@linaro.org> > --- > 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 <yoshihiro.shimoda.uh@renesas.com> 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(&pci->ep); > if (ret < 0) > goto err_get_sync; > + > + ret = dw_pcie_ep_init_registers(&pci->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(&pci->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(&pci->ep); > + if (ret) { > + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); > + dw_pcie_ep_deinit(&pci->ep); > + return ret; > + } > + > return ls_pcie_ep_interrupt_init(pcie, pdev); > } > > 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 = &pcie_ep_ops; > > - return dw_pcie_ep_init(&pci->ep); > + ret = dw_pcie_ep_init(&pci->ep); > + if (ret < 0) > + return ret; > + > + ret = dw_pcie_ep_init_registers(&pci->ep); > + if (ret) { > + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); > + dw_pcie_ep_deinit(&pci->ep); > + return ret; > + } > + > + break; > default: > dev_err(dev, "INVALID device type %d\n", artpec6_pcie->mode); > } > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index ed1f2afd830a..278bdc9b2269 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -729,7 +729,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > struct device *dev = pci->dev; > struct platform_device *pdev = to_platform_device(dev); > struct device_node *np = dev->of_node; > - const struct pci_epc_features *epc_features; > > INIT_LIST_HEAD(&ep->func_list); > > @@ -775,29 +774,8 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > goto err_exit_epc_mem; > } > > - if (ep->ops->get_features) { > - epc_features = ep->ops->get_features(ep); > - if (epc_features->core_init_notifier) > - return 0; > - } > - > - /* > - * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this > - * step as platforms that implement 'core_init_notifier' feature may > - * not have the hardware ready (i.e. core initialized) for access > - * (Ex: tegra194). Any hardware access on such platforms result > - * in system hang. > - */ > - ret = dw_pcie_ep_init_registers(ep); > - if (ret) > - goto err_free_epc_mem; > - > return 0; > > -err_free_epc_mem: > - pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem, > - epc->mem->window.page_size); > - > err_exit_epc_mem: > pci_epc_mem_exit(epc); > > diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c > 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 = &pcie_ep_ops; > ret = dw_pcie_ep_init(&pci->ep); > + if (ret) > + return ret; > + > + ret = dw_pcie_ep_init_registers(&pci->ep); > + if (ret) { > + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); > + dw_pcie_ep_deinit(&pci->ep); > + } > + > break; > default: > dev_err(dev, "INVALID device type %d\n", dw_plat_pcie->mode); > diff --git a/drivers/pci/controller/dwc/pcie-keembay.c b/drivers/pci/controller/dwc/pcie-keembay.c > index 208d3b0ba196..250d6acf16dc 100644 > --- a/drivers/pci/controller/dwc/pcie-keembay.c > +++ b/drivers/pci/controller/dwc/pcie-keembay.c > @@ -392,6 +392,7 @@ static int keembay_pcie_probe(struct platform_device *pdev) > struct keembay_pcie *pcie; > struct dw_pcie *pci; > enum dw_pcie_device_mode mode; > + int ret; > > data = device_get_match_data(dev); > if (!data) > @@ -426,11 +427,24 @@ static int keembay_pcie_probe(struct platform_device *pdev) > return -ENODEV; > > pci->ep.ops = &keembay_pcie_ep_ops; > - return dw_pcie_ep_init(&pci->ep); > + ret = dw_pcie_ep_init(&pci->ep); > + if (ret) > + return ret; > + > + ret = dw_pcie_ep_init_registers(&pci->ep); > + if (ret) { > + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); > + dw_pcie_ep_deinit(&pci->ep); > + return ret; > + } > + > + break; > default: > dev_err(dev, "Invalid device type %d\n", pcie->mode); > return -ENODEV; > } > + > + return 0; > } > > static const struct keembay_pcie_of_data keembay_pcie_rc_of_data = { > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c > index 9d9d22e367bb..fb7c03639a53 100644 > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c > @@ -414,6 +414,7 @@ static const struct dw_pcie_ep_ops pcie_ep_ops = { > static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar) > { > struct dw_pcie_ep *ep = &rcar->dw.ep; > + struct device *dev = rcar->dw.dev; > int ret; > > if (!IS_ENABLED(CONFIG_PCIE_RCAR_GEN4_EP)) > @@ -422,8 +423,17 @@ static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar) > ep->ops = &pcie_ep_ops; > > ret = dw_pcie_ep_init(ep); > - if (ret) > + if (ret) { > rcar_gen4_pcie_ep_deinit(rcar); > + 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); > + rcar_gen4_pcie_ep_deinit(rcar); > + } > > return ret; > } > diff --git a/drivers/pci/controller/dwc/pcie-uniphier-ep.c b/drivers/pci/controller/dwc/pcie-uniphier-ep.c > index 3fced0d3e851..82ccaea089be 100644 > --- a/drivers/pci/controller/dwc/pcie-uniphier-ep.c > +++ b/drivers/pci/controller/dwc/pcie-uniphier-ep.c > @@ -399,7 +399,18 @@ static int uniphier_pcie_ep_probe(struct platform_device *pdev) > return ret; > > priv->pci.ep.ops = &uniphier_pcie_ep_ops; > - return dw_pcie_ep_init(&priv->pci.ep); > + ret = dw_pcie_ep_init(&priv->pci.ep); > + if (ret) > + return ret; > + > + ret = dw_pcie_ep_init_registers(&priv->pci.ep); > + if (ret) { > + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); > + dw_pcie_ep_deinit(&priv->pci.ep); > + return ret; > + } > + > + return 0; > } > > static const struct uniphier_pcie_ep_soc_data uniphier_pro5_data = { > > -- > 2.25.1
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(&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. > > 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
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(&pci->ep); if (ret < 0) goto err_get_sync; + + ret = dw_pcie_ep_init_registers(&pci->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(&pci->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(&pci->ep); + if (ret) { + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); + dw_pcie_ep_deinit(&pci->ep); + return ret; + } + return ls_pcie_ep_interrupt_init(pcie, pdev); } 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 = &pcie_ep_ops; - return dw_pcie_ep_init(&pci->ep); + ret = dw_pcie_ep_init(&pci->ep); + if (ret < 0) + return ret; + + ret = dw_pcie_ep_init_registers(&pci->ep); + if (ret) { + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); + dw_pcie_ep_deinit(&pci->ep); + return ret; + } + + break; default: dev_err(dev, "INVALID device type %d\n", artpec6_pcie->mode); } diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index ed1f2afd830a..278bdc9b2269 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -729,7 +729,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) struct device *dev = pci->dev; struct platform_device *pdev = to_platform_device(dev); struct device_node *np = dev->of_node; - const struct pci_epc_features *epc_features; INIT_LIST_HEAD(&ep->func_list); @@ -775,29 +774,8 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) goto err_exit_epc_mem; } - if (ep->ops->get_features) { - epc_features = ep->ops->get_features(ep); - if (epc_features->core_init_notifier) - return 0; - } - - /* - * NOTE:- Avoid accessing the hardware (Ex:- DBI space) before this - * step as platforms that implement 'core_init_notifier' feature may - * not have the hardware ready (i.e. core initialized) for access - * (Ex: tegra194). Any hardware access on such platforms result - * in system hang. - */ - ret = dw_pcie_ep_init_registers(ep); - if (ret) - goto err_free_epc_mem; - return 0; -err_free_epc_mem: - pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem, - epc->mem->window.page_size); - err_exit_epc_mem: pci_epc_mem_exit(epc); diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c 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 = &pcie_ep_ops; ret = dw_pcie_ep_init(&pci->ep); + if (ret) + return ret; + + ret = dw_pcie_ep_init_registers(&pci->ep); + if (ret) { + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); + dw_pcie_ep_deinit(&pci->ep); + } + break; default: dev_err(dev, "INVALID device type %d\n", dw_plat_pcie->mode); diff --git a/drivers/pci/controller/dwc/pcie-keembay.c b/drivers/pci/controller/dwc/pcie-keembay.c index 208d3b0ba196..250d6acf16dc 100644 --- a/drivers/pci/controller/dwc/pcie-keembay.c +++ b/drivers/pci/controller/dwc/pcie-keembay.c @@ -392,6 +392,7 @@ static int keembay_pcie_probe(struct platform_device *pdev) struct keembay_pcie *pcie; struct dw_pcie *pci; enum dw_pcie_device_mode mode; + int ret; data = device_get_match_data(dev); if (!data) @@ -426,11 +427,24 @@ static int keembay_pcie_probe(struct platform_device *pdev) return -ENODEV; pci->ep.ops = &keembay_pcie_ep_ops; - return dw_pcie_ep_init(&pci->ep); + ret = dw_pcie_ep_init(&pci->ep); + if (ret) + return ret; + + ret = dw_pcie_ep_init_registers(&pci->ep); + if (ret) { + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); + dw_pcie_ep_deinit(&pci->ep); + return ret; + } + + break; default: dev_err(dev, "Invalid device type %d\n", pcie->mode); return -ENODEV; } + + return 0; } static const struct keembay_pcie_of_data keembay_pcie_rc_of_data = { diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c index 9d9d22e367bb..fb7c03639a53 100644 --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c @@ -414,6 +414,7 @@ static const struct dw_pcie_ep_ops pcie_ep_ops = { static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar) { struct dw_pcie_ep *ep = &rcar->dw.ep; + struct device *dev = rcar->dw.dev; int ret; if (!IS_ENABLED(CONFIG_PCIE_RCAR_GEN4_EP)) @@ -422,8 +423,17 @@ static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar) ep->ops = &pcie_ep_ops; ret = dw_pcie_ep_init(ep); - if (ret) + if (ret) { rcar_gen4_pcie_ep_deinit(rcar); + 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); + rcar_gen4_pcie_ep_deinit(rcar); + } return ret; } diff --git a/drivers/pci/controller/dwc/pcie-uniphier-ep.c b/drivers/pci/controller/dwc/pcie-uniphier-ep.c index 3fced0d3e851..82ccaea089be 100644 --- a/drivers/pci/controller/dwc/pcie-uniphier-ep.c +++ b/drivers/pci/controller/dwc/pcie-uniphier-ep.c @@ -399,7 +399,18 @@ static int uniphier_pcie_ep_probe(struct platform_device *pdev) return ret; priv->pci.ep.ops = &uniphier_pcie_ep_ops; - return dw_pcie_ep_init(&priv->pci.ep); + ret = dw_pcie_ep_init(&priv->pci.ep); + if (ret) + return ret; + + ret = dw_pcie_ep_init_registers(&priv->pci.ep); + if (ret) { + dev_err(dev, "Failed to initialize DWC endpoint registers\n"); + dw_pcie_ep_deinit(&priv->pci.ep); + return ret; + } + + return 0; } static const struct uniphier_pcie_ep_soc_data uniphier_pro5_data = {
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 <manivannan.sadhasivam@linaro.org> --- 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(-)