RE: [PATCH v16 19/22] PCI: rcar-gen4: Add R-Car Gen4 PCIe Host support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Serge,

> From: Serge Semin, Sent: Friday, June 9, 2023 7:54 PM
<snip>
> > > > static int rcar_gen4_pcie_start_link(struct dw_pcie *dw)
> > > > {
> > > >         struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > > >         int i;
> > > >
> > > >         rcar_gen4_pcie_ltssm_enable(rcar, true);
> > > >
> > > >         /*
> > > >          * Require direct speed change here. Otherwise RDLH_LINK_UP of
> > > >          * PCIEINTSTS0 which is this controller specific register may not
> > > >          * be set.
> > > >          */
> > >
> > > >         if (rcar->needs_speed_change) {
> > >
> > > Seeing this is specified for the root port only what about
> > > replacing the statement with just test whether the rcar_gen4_pcie.mode ==
> > > DW_PCIE_RC_TYPE? Thus you'll be ablt to drop the needs_speed_change field.
> >
> > Thank you for the comment. I'll fix it.
> >
> > > BTW Just curious. Why is the loop below enabled for the Root Port
> > > only? What about the end-point controller? It's the same hardware
> > > after all..
> >
> > This is reused from v16 and then it used "link retraining" which is only for
> > the Root Port. As you mentioned, it seems endpoint controller is also needed
> > if we use direct speed change.
> >
> > > >                 for (i = 0; i < SPEED_CHANGE_MAX_RETRIES; i++) {
> > > >                         rcar_gen4_pcie_speed_change(dw);
> > > >                         msleep(100);
> > > >                         if (rcar_gen4_pcie_check_current_link(dw))
> > > >                                 return 0;
> > > >                 }
> > >
> > > Did you trace how many iterations this loop normally takes?
> >
> > i = 0 or 1 (if the max-link-speed is suitable for a connected device.)
> >
> > > Is it
> > > constant or varies for the same platform setup and a connected link
> > > partner? Does the number of iterations depend on the target link speed
> > > specified via the "max-link-speed" property?
> >
> 
> > This is not related to the "max-link-speed". It seems to related to
> > a link partner.
> > 		LinkCap	max-link-speed	loop
> > Device A		4	4		1
> > Device A		4	3		1
> > Device B		3	3		0
> 
> Great! If so I would have just left a single unconditional
> rcar_gen4_pcie_speed_change() call placed right after the
> rcar_gen4_pcie_ltssm_enable() method with no delays afterwards. These
> methods would have been invoked in the framework of
> dw_pcie_start_link() after which the dw_pcie_wait_for_link() method is
> called with several checks parted with the ~100ms delay. It will make
> sure that at least some link is up with the link state printed to the
> system log. If for some reason the performance degradation happens
> then it will be up to the system administrator to investigate what was
> wrong. Your driver did as much is it could to reach the best link gen.

IIUC, is your suggestion like the following code?
---
	rcar_gen4_pcie_ltssm_enable(rcar, true);
	if (!dw_pcie_wait_for_link(dw)) {
		rcar_gen4_pcie_speed_change(dw);
		return 0;
	}
---

Unfortunately, it doesn't work correctly...
The following code can work correctly. The value of i is still 1 on the device A.
What do you think that the following code is acceptable?
---
	rcar_gen4_pcie_ltssm_enable(rcar, true);
	for (i = 0; i < SPEED_CHANGE_MAX_RETRIES; i++) {
		msleep(100);
		rcar_gen4_pcie_speed_change(dw);
		if (dw_pcie_link_up(dw)) {
			printk("%s:%d\n", __func__, i);
			return 0;
		}
	}
---

Best regards,
Yoshihiro Shimoda

> -Serge(y)





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux