diff mbox series

[v2,1/8] PCI: rcar-gen2: Add support for clocks

Message ID 20220414074011.500533-2-herve.codina@bootlin.com (mailing list archive)
State Superseded
Headers show
Series RZN1 USB Host support | expand

Commit Message

Herve Codina April 14, 2022, 7:40 a.m. UTC
The PCI rcar-gen2 does not call any clk_prepare_enable().
This lead to an access failure when the driver tries to access
the IP (at least on a RZ/N1D platform).

Prepare and enable clocks using the bulk version of
clk_prepare_enable() in order to prepare and enable all clocks
attached to this device.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/pci/controller/pci-rcar-gen2.c | 28 ++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Comments

Geert Uytterhoeven April 14, 2022, 8:45 a.m. UTC | #1
Hi Hervé,

Thanks for your patch!

On Thu, Apr 14, 2022 at 9:40 AM Herve Codina <herve.codina@bootlin.com> wrote:
> The PCI rcar-gen2 does not call any clk_prepare_enable().

Correct, this driver manages the clocks indirectly through Runtime PM.

> This lead to an access failure when the driver tries to access
> the IP (at least on a RZ/N1D platform).

I expect adding

    power-domans = <&sysctrl>;

to the pci_usb node makes this patch redundant.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Herve Codina April 14, 2022, 11:29 a.m. UTC | #2
Hi Geert,

On Thu, 14 Apr 2022 10:45:54 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Hervé,
> 
> Thanks for your patch!
> 
> On Thu, Apr 14, 2022 at 9:40 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > The PCI rcar-gen2 does not call any clk_prepare_enable().  
> 
> Correct, this driver manages the clocks indirectly through Runtime PM.
> 
> > This lead to an access failure when the driver tries to access
> > the IP (at least on a RZ/N1D platform).  
> 
> I expect adding
> 
>     power-domans = <&sysctrl>;
> 
> to the pci_usb node makes this patch redundant.

Seems not enough.
I tried what you suggest :
 - Added 'power-domains = <&systrl>;' to the pci_usb node
 - Added missing '#power-domain-cells = <0>;' to sysctrl node
 - Reverted my patch.

The system crashed at boot:
--- 8< ---
...
[    0.705309] loop: module loaded
[    0.709597] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
[    0.716804] ehci-pci: EHCI PCI platform driver
[    0.721716] ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver
[    0.728522] ohci-pci: OHCI PCI platform driver
[    0.733581] usbcore: registered new interface driver usb-storage
[    0.740458] ThumbEE CPU extension supported.
[    0.745093] Registering SWP/SWPB emulation handler
[    0.797518] rzn1-pinctrl 40067000.pinctrl: probed
[    0.803311] pci-rcar-gen2 40030000.pci: host bridge /soc/pci@40030000 ranges:
[    0.811255] pci-rcar-gen2 40030000.pci:      MEM 0x0040020000..0x004002ffff -> 0x0040020000
[    0.820373] pci-rcar-gen2 40030000.pci:   IB MEM 0x0080000000..0x00bfffffff -> 0x0080000000
[    0.829609] 8<--- cut here ---
[    0.832958] Unhandled fault: external abort on non-linefetch (0x1008) at 0x90b5f848
[    0.841259] [90b5f848] *pgd=82149811, *pte=40030653, *ppte=40030453
[    0.848093] Internal error: : 1008 [#1] SMP ARM
[    0.853024] Modules linked in:
[    0.856398] CPU: 0 PID: 31 Comm: kworker/u4:1 Not tainted 5.18.0-rc2-00009-g803ee9fd9fa5-dirty #5
[    0.865998] Hardware name: Generic DT based system
[    0.871176] Workqueue: events_unbound deferred_probe_work_func
[    0.877539] PC is at rcar_pci_probe+0x15c/0x2f8
[    0.882454] LR is at _raw_spin_unlock_irqrestore+0x24/0x2c
[    0.888434] pc : [<803ea428>]    lr : [<804dc9b0>]    psr: 60000013
[    0.895193] sp : 90aa5e40  ip : 8217c4e0  fp : 00000000
[    0.900857] r10: 80e7bd30  r9 : 80000000  r8 : 40000000
[    0.906532] r7 : 80000000  r6 : 8217c410  r5 : 821d3400  r4 : 90b5f000
[    0.913580] r3 : 00000009  r2 : 5c120fb6  r1 : 60000013  r0 : 00000000
[    0.920646] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    0.928365] Control: 10c5387d  Table: 8000406a  DAC: 00000051
...
--- 8< ---

I also added a trace printk in r9a06g032-clocks.c and
r9a06g032_attach_dev() was never called.

Did I miss to set something ?

Regards,
Hervé
Geert Uytterhoeven April 14, 2022, 11:48 a.m. UTC | #3
Hi Hervé,

On Thu, Apr 14, 2022 at 1:29 PM Herve Codina <herve.codina@bootlin.com> wrote:
> On Thu, 14 Apr 2022 10:45:54 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Apr 14, 2022 at 9:40 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > > The PCI rcar-gen2 does not call any clk_prepare_enable().
> >
> > Correct, this driver manages the clocks indirectly through Runtime PM.
> >
> > > This lead to an access failure when the driver tries to access
> > > the IP (at least on a RZ/N1D platform).
> >
> > I expect adding
> >
> >     power-domans = <&sysctrl>;
> >
> > to the pci_usb node makes this patch redundant.
>
> Seems not enough.
> I tried what you suggest :
>  - Added 'power-domains = <&systrl>;' to the pci_usb node
>  - Added missing '#power-domain-cells = <0>;' to sysctrl node
>  - Reverted my patch.
>
> The system crashed at boot:

> [    0.832958] Unhandled fault: external abort on non-linefetch (0x1008) at 0x90b5f848

That's indeed a typical symptom of accessing a module's registers
while the module's clock is disabled.

> I also added a trace printk in r9a06g032-clocks.c and
> r9a06g032_attach_dev() was never called.
>
> Did I miss to set something ?

Do you have CONFIG_PM and CONFIG_PM_GENERIC_DOMAINS
enabled?
Apparently ARCH_RZN1 does not select these options yet.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Herve Codina April 14, 2022, 1:42 p.m. UTC | #4
Hi Geert,

On Thu, 14 Apr 2022 13:48:22 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Hervé,
> 
> On Thu, Apr 14, 2022 at 1:29 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > On Thu, 14 Apr 2022 10:45:54 +0200
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> > > On Thu, Apr 14, 2022 at 9:40 AM Herve Codina <herve.codina@bootlin.com> wrote:  
> > > > The PCI rcar-gen2 does not call any clk_prepare_enable().  
> > >
> > > Correct, this driver manages the clocks indirectly through Runtime PM.
> > >  
> > > > This lead to an access failure when the driver tries to access
> > > > the IP (at least on a RZ/N1D platform).  
> > >
> > > I expect adding
> > >
> > >     power-domans = <&sysctrl>;
> > >
> > > to the pci_usb node makes this patch redundant.  
> >
> > Seems not enough.
> > I tried what you suggest :
> >  - Added 'power-domains = <&systrl>;' to the pci_usb node
> >  - Added missing '#power-domain-cells = <0>;' to sysctrl node
> >  - Reverted my patch.
> >
> > The system crashed at boot:  
> 
> > [    0.832958] Unhandled fault: external abort on non-linefetch (0x1008) at 0x90b5f848  
> 
> That's indeed a typical symptom of accessing a module's registers
> while the module's clock is disabled.
> 
> > I also added a trace printk in r9a06g032-clocks.c and
> > r9a06g032_attach_dev() was never called.
> >
> > Did I miss to set something ?  
> 
> Do you have CONFIG_PM and CONFIG_PM_GENERIC_DOMAINS
> enabled?
> Apparently ARCH_RZN1 does not select these options yet.
> 

Thanks a lot for pointing this.

I added select CONFIG_PM and CONFIG_PM_GENERIC_DOMAINS
in ARCH_RZN1 and it works.

I will remove my patch calling clk_bulk_prepare_enable() and
add some new patches to enable power domains in the v3 series.

Regards,
Hervé
diff mbox series

Patch

diff --git a/drivers/pci/controller/pci-rcar-gen2.c b/drivers/pci/controller/pci-rcar-gen2.c
index 35804ea394fd..528bc3780e01 100644
--- a/drivers/pci/controller/pci-rcar-gen2.c
+++ b/drivers/pci/controller/pci-rcar-gen2.c
@@ -8,6 +8,7 @@ 
  * Author: Valentine Barshak <valentine.barshak@cogentembedded.com>
  */
 
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -99,6 +100,8 @@  struct rcar_pci {
 	struct resource mem_res;
 	struct resource *cfg_res;
 	int irq;
+	struct clk_bulk_data *clocks;
+	int nclocks;
 };
 
 /* PCI configuration space operations */
@@ -282,6 +285,7 @@  static int rcar_pci_probe(struct platform_device *pdev)
 	struct rcar_pci *priv;
 	struct pci_host_bridge *bridge;
 	void __iomem *reg;
+	int ret;
 
 	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*priv));
 	if (!bridge)
@@ -305,13 +309,25 @@  static int rcar_pci_probe(struct platform_device *pdev)
 	priv->mem_res = *mem_res;
 	priv->cfg_res = cfg_res;
 
+	ret = devm_clk_bulk_get_all(dev, &priv->clocks);
+	if (ret < 0) {
+		dev_err(dev, "failed to get clocks %d\n", ret);
+		return ret;
+	}
+	priv->nclocks = ret;
+
+	ret = clk_bulk_prepare_enable(priv->nclocks, priv->clocks);
+	if (ret)
+		return ret;
+
 	priv->irq = platform_get_irq(pdev, 0);
 	priv->reg = reg;
 	priv->dev = dev;
 
 	if (priv->irq < 0) {
 		dev_err(dev, "no valid irq found\n");
-		return priv->irq;
+		ret = priv->irq;
+		goto disable_clocks;
 	}
 
 	bridge->ops = &rcar_pci_ops;
@@ -320,7 +336,15 @@  static int rcar_pci_probe(struct platform_device *pdev)
 
 	rcar_pci_setup(priv);
 
-	return pci_host_probe(bridge);
+	ret = pci_host_probe(bridge);
+	if (ret < 0)
+		goto disable_clocks;
+
+	return 0;
+
+disable_clocks:
+	clk_bulk_disable_unprepare(priv->nclocks, priv->clocks);
+	return ret;
 }
 
 static const struct of_device_id rcar_pci_of_match[] = {