| Message ID | 1475743800-1036-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 06, 2016 at 04:50:00PM +0800, Shawn Lin wrote: > From: Brian Norris <briannorris@chromium.org> > > rk3399 supports PCIe 2.x link speeds marginally at best, and on some > boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500 > ms waiting for training that will never happen, let's add a property > from devicetree to specify link capability. > > Signed-off-by: Brian Norris <briannorris@chromium.org> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > --- > > Changes in v4: > - define link_gen as u32 > - elaborate more for rockchip,max-link-speed on doc > > Changes in v3: > - Cast a warning for invalid max link speed and use gen1 for it. > That looks better than v2. (Suggested by Brian) > > Changes in v2: > - rename the property to rockchip,max-link-speed according to > Bjorn's recommendation and take some bits from imx6q-pcie to > make this requirement more consisent. > > .../devicetree/bindings/pci/rockchip-pcie.txt | 4 ++ > drivers/pci/host/pcie-rockchip.c | 63 ++++++++++++++-------- > 2 files changed, 44 insertions(+), 23 deletions(-) Not sure if it means anything much, since I wrote half of this, but: Reviewed-by: Brian Norris <briannorris@chromium.org>
On Thu, Oct 06, 2016 at 04:50:00PM +0800, Shawn Lin wrote: > From: Brian Norris <briannorris@chromium.org> > > rk3399 supports PCIe 2.x link speeds marginally at best, and on some > boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500 > ms waiting for training that will never happen, let's add a property > from devicetree to specify link capability. > > Signed-off-by: Brian Norris <briannorris@chromium.org> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > --- > > Changes in v4: > - define link_gen as u32 > - elaborate more for rockchip,max-link-speed on doc > > Changes in v3: > - Cast a warning for invalid max link speed and use gen1 for it. > That looks better than v2. (Suggested by Brian) > > Changes in v2: > - rename the property to rockchip,max-link-speed according to > Bjorn's recommendation and take some bits from imx6q-pcie to > make this requirement more consisent. > > .../devicetree/bindings/pci/rockchip-pcie.txt | 4 ++ > drivers/pci/host/pcie-rockchip.c | 63 ++++++++++++++-------- > 2 files changed, 44 insertions(+), 23 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt > index ba67b39..9bb29de 100644 > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt > @@ -42,6 +42,10 @@ Required properties: > Optional Property: > - ep-gpios: contain the entry for pre-reset gpio > - num-lanes: number of lanes to use > +- rockchip,max-link-speed: Specify PCI gen for link capability. Must > + be '2' for gen2, and '1' for gen1, otherwise will default to gen1. > + For backward compatibility, if this property isn't assigned, we > + use gen2 by default. Defaults to gen1 or gen2? Let's drop rockchip and make this a common property. > - vpcie3v3-supply: The phandle to the 3.3v regulator to use for PCIe. > - vpcie1v8-supply: The phandle to the 1.8v regulator to use for PCIe. > - vpcie0v9-supply: The phandle to the 0.9v regulator to use for PCIe.
Hi Rob, On Mon, Oct 10, 2016 at 09:16:39AM -0500, Rob Herring wrote: > On Thu, Oct 06, 2016 at 04:50:00PM +0800, Shawn Lin wrote: > > From: Brian Norris <briannorris@chromium.org> > > > > rk3399 supports PCIe 2.x link speeds marginally at best, and on some > > boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500 > > ms waiting for training that will never happen, let's add a property > > from devicetree to specify link capability. > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > > > --- > > > > Changes in v4: > > - define link_gen as u32 > > - elaborate more for rockchip,max-link-speed on doc > > > > Changes in v3: > > - Cast a warning for invalid max link speed and use gen1 for it. > > That looks better than v2. (Suggested by Brian) > > > > Changes in v2: > > - rename the property to rockchip,max-link-speed according to > > Bjorn's recommendation and take some bits from imx6q-pcie to > > make this requirement more consisent. > > > > .../devicetree/bindings/pci/rockchip-pcie.txt | 4 ++ > > drivers/pci/host/pcie-rockchip.c | 63 ++++++++++++++-------- > > 2 files changed, 44 insertions(+), 23 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt > > index ba67b39..9bb29de 100644 > > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt > > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt > > @@ -42,6 +42,10 @@ Required properties: > > Optional Property: > > - ep-gpios: contain the entry for pre-reset gpio > > - num-lanes: number of lanes to use > > +- rockchip,max-link-speed: Specify PCI gen for link capability. Must > > + be '2' for gen2, and '1' for gen1, otherwise will default to gen1. > > + For backward compatibility, if this property isn't assigned, we > > + use gen2 by default. > > Defaults to gen1 or gen2? The wording is a bit confusing, but I think Shawn intended for: (a) if no property is provided, default to the maximum supported (i.e., gen2), to keep backward compatibility (b) if a property is provided, but it doesn't contain 1 or 2, just default to 1 I already disagreed with (b) (and suggested we make this an error instead; we have no business accepting malformed device trees here IMO). Alternatives to clear up the confusion Rob pointed out might include: (1) explaining this better (i.e., what does "otherwise" mean in the original sentence?) (2) changing this to uniformly default to 2 (if someone used an unsupported value, treat it as if the property wasn't there at all) (3) reject incorrect values outright (so we don't have this fuzzy middle ground at all) > Let's drop rockchip and make this a common property. As in, handle this in some generic/common way? AFAICT, there's really no way to do that given the current structure of PCIe handling (and the nature of these kinds of limitations). Or, do you just mean make the name common? (i.e,. "max-link-speed" instead of "rockchip,max-link-speed") Also for the record, Shawn already sent v5 before your comments. Presumably he'll need a v6 then... Brian > > - vpcie3v3-supply: The phandle to the 3.3v regulator to use for PCIe. > > - vpcie1v8-supply: The phandle to the 1.8v regulator to use for PCIe. > > - vpcie0v9-supply: The phandle to the 0.9v regulator to use for PCIe.
On Mon, Oct 10, 2016 at 12:20 PM, Brian Norris <briannorris@chromium.org> wrote: > Hi Rob, > > On Mon, Oct 10, 2016 at 09:16:39AM -0500, Rob Herring wrote: >> On Thu, Oct 06, 2016 at 04:50:00PM +0800, Shawn Lin wrote: >> > From: Brian Norris <briannorris@chromium.org> >> > >> > rk3399 supports PCIe 2.x link speeds marginally at best, and on some >> > boards, the link won't train at 5 GT/s at all. Rather than sacrifice 500 >> > ms waiting for training that will never happen, let's add a property >> > from devicetree to specify link capability. >> > >> > Signed-off-by: Brian Norris <briannorris@chromium.org> >> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> > >> > --- >> > >> > Changes in v4: >> > - define link_gen as u32 >> > - elaborate more for rockchip,max-link-speed on doc >> > >> > Changes in v3: >> > - Cast a warning for invalid max link speed and use gen1 for it. >> > That looks better than v2. (Suggested by Brian) >> > >> > Changes in v2: >> > - rename the property to rockchip,max-link-speed according to >> > Bjorn's recommendation and take some bits from imx6q-pcie to >> > make this requirement more consisent. >> > >> > .../devicetree/bindings/pci/rockchip-pcie.txt | 4 ++ >> > drivers/pci/host/pcie-rockchip.c | 63 ++++++++++++++-------- >> > 2 files changed, 44 insertions(+), 23 deletions(-) >> > >> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt >> > index ba67b39..9bb29de 100644 >> > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt >> > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt >> > @@ -42,6 +42,10 @@ Required properties: >> > Optional Property: >> > - ep-gpios: contain the entry for pre-reset gpio >> > - num-lanes: number of lanes to use >> > +- rockchip,max-link-speed: Specify PCI gen for link capability. Must >> > + be '2' for gen2, and '1' for gen1, otherwise will default to gen1. >> > + For backward compatibility, if this property isn't assigned, we >> > + use gen2 by default. >> >> Defaults to gen1 or gen2? > > The wording is a bit confusing, but I think Shawn intended for: > (a) if no property is provided, default to the maximum supported (i.e., > gen2), to keep backward compatibility > (b) if a property is provided, but it doesn't contain 1 or 2, just > default to 1 > > I already disagreed with (b) (and suggested we make this an error > instead; we have no business accepting malformed device trees here IMO). > Alternatives to clear up the confusion Rob pointed out might include: > (1) explaining this better (i.e., what does "otherwise" mean in the > original sentence?) > (2) changing this to uniformly default to 2 (if someone used an > unsupported value, treat it as if the property wasn't there at all) > (3) reject incorrect values outright (so we don't have this fuzzy middle > ground at all) Sounds good. >> Let's drop rockchip and make this a common property. > > As in, handle this in some generic/common way? AFAICT, there's really > no way to do that given the current structure of PCIe handling (and the > nature of these kinds of limitations). > > Or, do you just mean make the name common? (i.e,. "max-link-speed" > instead of "rockchip,max-link-speed") Right, just the name and document in a common spot. Could be a simple helper function that drivers can call as well. Rob
diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt index ba67b39..9bb29de 100644 --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt @@ -42,6 +42,10 @@ Required properties: Optional Property: - ep-gpios: contain the entry for pre-reset gpio - num-lanes: number of lanes to use +- rockchip,max-link-speed: Specify PCI gen for link capability. Must + be '2' for gen2, and '1' for gen1, otherwise will default to gen1. + For backward compatibility, if this property isn't assigned, we + use gen2 by default. - vpcie3v3-supply: The phandle to the 3.3v regulator to use for PCIe. - vpcie1v8-supply: The phandle to the 1.8v regulator to use for PCIe. - vpcie0v9-supply: The phandle to the 0.9v regulator to use for PCIe. diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c index 35591b1..6dde999 100644 --- a/drivers/pci/host/pcie-rockchip.c +++ b/drivers/pci/host/pcie-rockchip.c @@ -53,6 +53,7 @@ #define PCIE_CLIENT_ARI_ENABLE HIWORD_UPDATE_BIT(0x0008) #define PCIE_CLIENT_CONF_LANE_NUM(x) HIWORD_UPDATE(0x0030, ENCODE_LANES(x)) #define PCIE_CLIENT_MODE_RC HIWORD_UPDATE_BIT(0x0040) +#define PCIE_CLIENT_GEN_SEL_1 HIWORD_UPDATE(0x0080, 0) #define PCIE_CLIENT_GEN_SEL_2 HIWORD_UPDATE_BIT(0x0080) #define PCIE_CLIENT_BASIC_STATUS1 (PCIE_CLIENT_BASE + 0x48) #define PCIE_CLIENT_LINK_STATUS_UP 0x00300000 @@ -205,6 +206,7 @@ struct rockchip_pcie { struct gpio_desc *ep_gpio; u32 lanes; u8 root_bus_nr; + u32 link_gen; struct device *dev; struct irq_domain *irq_domain; }; @@ -443,14 +445,19 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) return err; } + if (rockchip->link_gen == 2) + rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_2, + PCIE_CLIENT_CONFIG); + else + rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1, + PCIE_CLIENT_CONFIG); + rockchip_pcie_write(rockchip, PCIE_CLIENT_CONF_ENABLE | PCIE_CLIENT_LINK_TRAIN_ENABLE | PCIE_CLIENT_ARI_ENABLE | PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes) | - PCIE_CLIENT_MODE_RC | - PCIE_CLIENT_GEN_SEL_2, - PCIE_CLIENT_CONFIG); + PCIE_CLIENT_MODE_RC, PCIE_CLIENT_CONFIG); err = phy_power_on(rockchip->phy); if (err) { @@ -550,29 +557,31 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) msleep(20); } - /* - * Enable retrain for gen2. This should be configured only after - * gen1 finished. - */ - status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS); - status |= PCIE_RC_CONFIG_LCS_RETRAIN_LINK; - rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS); + if (rockchip->link_gen == 2) { + /* + * Enable retrain for gen2. This should be configured only after + * gen1 finished. + */ + status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS); + status |= PCIE_RC_CONFIG_LCS_RETRAIN_LINK; + rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS); + + timeout = jiffies + msecs_to_jiffies(500); + for (;;) { + status = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL); + if ((status & PCIE_CORE_PL_CONF_SPEED_MASK) == + PCIE_CORE_PL_CONF_SPEED_5G) { + dev_dbg(dev, "PCIe link training gen2 pass!\n"); + break; + } - timeout = jiffies + msecs_to_jiffies(500); - for (;;) { - status = rockchip_pcie_read(rockchip, PCIE_CORE_CTRL); - if ((status & PCIE_CORE_PL_CONF_SPEED_MASK) == - PCIE_CORE_PL_CONF_SPEED_5G) { - dev_dbg(dev, "PCIe link training gen2 pass!\n"); - break; - } + if (time_after(jiffies, timeout)) { + dev_dbg(dev, "PCIe link training gen2 timeout, fall back to gen1!\n"); + break; + } - if (time_after(jiffies, timeout)) { - dev_dbg(dev, "PCIe link training gen2 timeout, fall back to gen1!\n"); - break; + msleep(20); } - - msleep(20); } /* Check the final link width from negotiated lane counter from MGMT */ @@ -781,6 +790,14 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) rockchip->lanes = 1; } + rockchip->link_gen = 2; + err = of_property_read_u32(node, "rockchip,max-link-speed", + &rockchip->link_gen); + if (!err && rockchip->link_gen != 1 && rockchip->link_gen != 2) { + dev_warn(dev, "invalid max-link-speed, default to use gen1\n"); + rockchip->link_gen = 1; + } + rockchip->core_rst = devm_reset_control_get(dev, "core"); if (IS_ERR(rockchip->core_rst)) { if (PTR_ERR(rockchip->core_rst) != -EPROBE_DEFER)