drivers/pci/controller/pcie-mediatek.c | 7 +++++++ 1 file changed, 7 insertions(+)
Described in PCIe CEM specification sections 2.2 (PERST# Signal) and
2.2.1 (Initial Power-Up (G3 to S0)). The deassertion of PERST# should
be delayed 100ms (TPVPERL) for the power and clock to become stable.
Signed-off-by: qizhong cheng <qizhong.cheng@mediatek.com>
Acked-by: Pali Rohár <pali@kernel.org>
---
v2:
- Typo fix.
- Rewrap into one paragraph.
drivers/pci/controller/pcie-mediatek.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 2f3f974977a3..a61ea3940471 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -702,6 +702,13 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
*/
writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
+ /*
+ * Described in PCIe CEM specification sections 2.2 (PERST# Signal) and
+ * 2.2.1 (Initial Power-Up (G3 to S0)). The deassertion of PERST# should
+ * be delayed 100ms (TPVPERL) for the power and clock to become stable.
+ */
+ msleep(100);
+
/* De-assert PHY, PE, PIPE, MAC and configuration reset */
val = readl(port->base + PCIE_RST_CTRL);
val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
--
2.25.1
[+cc Marc, Alyssa, Mark, Luca for reset timing questions]
On Tue, Dec 07, 2021 at 04:41:53PM +0800, qizhong cheng wrote:
> Described in PCIe CEM specification sections 2.2 (PERST# Signal) and
> 2.2.1 (Initial Power-Up (G3 to S0)). The deassertion of PERST# should
> be delayed 100ms (TPVPERL) for the power and clock to become stable.
>
> Signed-off-by: qizhong cheng <qizhong.cheng@mediatek.com>
> Acked-by: Pali Rohár <pali@kernel.org>
> ---
>
> v2:
> - Typo fix.
> - Rewrap into one paragraph.
1) If you change something, even in the commit log or comments, it is
a new version, not a "RESEND". A "RESEND" means "I sent this quite a
while ago and didn't hear anything, so I'm sending the exact same
thing again in case the first one got lost."
2) I suggested a subject line update, which apparently got missed.
Here's a better one:
PCI: mediatek: Assert PERST# for 100ms for power and clock to stabilize
3) Most importantly, this needs to be reconciled with the similar
change to the apple driver:
https://lore.kernel.org/r/20211123180636.80558-2-maz@kernel.org
In the apple driver, we're doing:
- Assert PERST#
- Set up REFCLK
- Sleep 100us (T_perst-clk, CEM r5 2.2, 2.9.2)
- Deassert PERST#
- Sleep 100ms (not sure there's a name? PCIe r5 6.6.1)
But here in mediatek, we're doing:
- Assert PERST#
- Sleep 100ms (T_pvperl, CEM r5 2.2, 2.2.1, 2.9.2)
- Deassert PERST#
My questions:
- Where does apple enforce T_pvperl? I can't tell where power to
the slot is turned on.
- Where does mediatek enforce the PCIe sec 6.6.1 delay after
deasserting PERST# and before config requests?
- Does either apple or mediatek support speeds greater than 5 GT/s,
and if so, shouldn't we start the sec 6.6.1 100ms delay *after*
Link training completes?
> drivers/pci/controller/pcie-mediatek.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 2f3f974977a3..a61ea3940471 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -702,6 +702,13 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> */
> writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
>
> + /*
> + * Described in PCIe CEM specification sections 2.2 (PERST# Signal) and
> + * 2.2.1 (Initial Power-Up (G3 to S0)). The deassertion of PERST# should
> + * be delayed 100ms (TPVPERL) for the power and clock to become stable.
> + */
> + msleep(100);
> +
> /* De-assert PHY, PE, PIPE, MAC and configuration reset */
> val = readl(port->base + PCIE_RST_CTRL);
> val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
> --
> 2.25.1
>
On Tue, Dec 07, 2021 at 11:54:16AM -0600, Bjorn Helgaas wrote: > [+cc Marc, Alyssa, Mark, Luca for reset timing questions] > > On Tue, Dec 07, 2021 at 04:41:53PM +0800, qizhong cheng wrote: > > Described in PCIe CEM specification sections 2.2 (PERST# Signal) and > > 2.2.1 (Initial Power-Up (G3 to S0)). The deassertion of PERST# should > > be delayed 100ms (TPVPERL) for the power and clock to become stable. > > > > Signed-off-by: qizhong cheng <qizhong.cheng@mediatek.com> > > Acked-by: Pali Roh�r <pali@kernel.org> > > --- > > > > v2: > > - Typo fix. > > - Rewrap into one paragraph. > > 1) If you change something, even in the commit log or comments, it is > a new version, not a "RESEND". A "RESEND" means "I sent this quite a > while ago and didn't hear anything, so I'm sending the exact same > thing again in case the first one got lost." > > 2) I suggested a subject line update, which apparently got missed. > Here's a better one: > > PCI: mediatek: Assert PERST# for 100ms for power and clock to stabilize > > 3) Most importantly, this needs to be reconciled with the similar > change to the apple driver: > > https://lore.kernel.org/r/20211123180636.80558-2-maz@kernel.org > > In the apple driver, we're doing: > > - Assert PERST# > - Set up REFCLK > - Sleep 100us (T_perst-clk, CEM r5 2.2, 2.9.2) > - Deassert PERST# > - Sleep 100ms (not sure there's a name? PCIe r5 6.6.1) > > But here in mediatek, we're doing: > > - Assert PERST# > - Sleep 100ms (T_pvperl, CEM r5 2.2, 2.2.1, 2.9.2) > - Deassert PERST# > > My questions: > > - Where does apple enforce T_pvperl? I can't tell where power to > the slot is turned on. > > - Where does mediatek enforce the PCIe sec 6.6.1 delay after > deasserting PERST# and before config requests? > > - Does either apple or mediatek support speeds greater than 5 GT/s, > and if so, shouldn't we start the sec 6.6.1 100ms delay *after* > Link training completes? I dropped this patch from my pci/mediatek branch, waiting for clarification. Thanks, Lorenzo > > drivers/pci/controller/pcie-mediatek.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > > index 2f3f974977a3..a61ea3940471 100644 > > --- a/drivers/pci/controller/pcie-mediatek.c > > +++ b/drivers/pci/controller/pcie-mediatek.c > > @@ -702,6 +702,13 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port) > > */ > > writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL); > > > > + /* > > + * Described in PCIe CEM specification sections 2.2 (PERST# Signal) and > > + * 2.2.1 (Initial Power-Up (G3 to S0)). The deassertion of PERST# should > > + * be delayed 100ms (TPVPERL) for the power and clock to become stable. > > + */ > > + msleep(100); > > + > > /* De-assert PHY, PE, PIPE, MAC and configuration reset */ > > val = readl(port->base + PCIE_RST_CTRL); > > val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB | > > -- > > 2.25.1 > >
> Date: Tue, 7 Dec 2021 11:54:16 -0600 > From: Bjorn Helgaas <helgaas@kernel.org> > > [+cc Marc, Alyssa, Mark, Luca for reset timing questions] Hi Bjorn, > On Tue, Dec 07, 2021 at 04:41:53PM +0800, qizhong cheng wrote: > > Described in PCIe CEM specification sections 2.2 (PERST# Signal) and > > 2.2.1 (Initial Power-Up (G3 to S0)). The deassertion of PERST# should > > be delayed 100ms (TPVPERL) for the power and clock to become stable. > > > > Signed-off-by: qizhong cheng <qizhong.cheng@mediatek.com> > > Acked-by: Pali Rohár <pali@kernel.org> > > --- > > > > v2: > > - Typo fix. > > - Rewrap into one paragraph. > > 1) If you change something, even in the commit log or comments, it is > a new version, not a "RESEND". A "RESEND" means "I sent this quite a > while ago and didn't hear anything, so I'm sending the exact same > thing again in case the first one got lost." > > 2) I suggested a subject line update, which apparently got missed. > Here's a better one: > > PCI: mediatek: Assert PERST# for 100ms for power and clock to stabilize > > 3) Most importantly, this needs to be reconciled with the similar > change to the apple driver: > > https://lore.kernel.org/r/20211123180636.80558-2-maz@kernel.org > > In the apple driver, we're doing: > > - Assert PERST# > - Set up REFCLK > - Sleep 100us (T_perst-clk, CEM r5 2.2, 2.9.2) > - Deassert PERST# > - Sleep 100ms (not sure there's a name? PCIe r5 6.6.1) > > But here in mediatek, we're doing: > > - Assert PERST# > - Sleep 100ms (T_pvperl, CEM r5 2.2, 2.2.1, 2.9.2) > - Deassert PERST# > > My questions: My understanding of the the Apple PCIe hardware is somewhat limited but: > - Where does apple enforce T_pvperl? I can't tell where power to > the slot is turned on. So far all available machines only have PCIe devices that are soldered onto the motherboard, so there are no "real" slots. As far as we can tell the PCIe power domain is already powered on at the point where the m1n1 bootloader takes control. There is a GPIO that controls power to some devices (WiFi, SDHC on the M1 Pro/Max laptops) and those devices are initially powered off. The Linux driver doesn't currently attempt to power these devices on, but U-Boot will power them on if the appropriate GPIO is defined in the device tree. The way this is specified in the device tree is still under discussion. > - Where does mediatek enforce the PCIe sec 6.6.1 delay after > deasserting PERST# and before config requests? > > - Does either apple or mediatek support speeds greater than 5 GT/s, > and if so, shouldn't we start the sec 6.6.1 100ms delay *after* > Link training completes? The Apple hardware advertises support for 8 GT/s, but all the devices integrated on the Mac mini support only 2.5 GT/s or 5 GT/s. Hope this helps, Mark
On Tue, Dec 07, 2021 at 10:00:43PM +0100, Mark Kettenis wrote: > > Date: Tue, 7 Dec 2021 11:54:16 -0600 > > From: Bjorn Helgaas <helgaas@kernel.org> > > > > [+cc Marc, Alyssa, Mark, Luca for reset timing questions] > > Hi Bjorn, > > > On Tue, Dec 07, 2021 at 04:41:53PM +0800, qizhong cheng wrote: > > > Described in PCIe CEM specification sections 2.2 (PERST# Signal) and > > > 2.2.1 (Initial Power-Up (G3 to S0)). The deassertion of PERST# should > > > be delayed 100ms (TPVPERL) for the power and clock to become stable. > > > > > > Signed-off-by: qizhong cheng <qizhong.cheng@mediatek.com> > > > Acked-by: Pali Rohár <pali@kernel.org> > > ... > > 3) Most importantly, this needs to be reconciled with the similar > > change to the apple driver: > > > > https://lore.kernel.org/r/20211123180636.80558-2-maz@kernel.org > > > > In the apple driver, we're doing: > > > > - Assert PERST# > > - Set up REFCLK > > - Sleep 100us (T_perst-clk, CEM r5 2.2, 2.9.2) > > - Deassert PERST# > > - Sleep 100ms (not sure there's a name? PCIe r5 6.6.1) > > > > But here in mediatek, we're doing: > > > > - Assert PERST# > > - Sleep 100ms (T_pvperl, CEM r5 2.2, 2.2.1, 2.9.2) > > - Deassert PERST# > > > > My questions: > > My understanding of the the Apple PCIe hardware is somewhat limited but: > > > - Where does apple enforce T_pvperl? I can't tell where power to > > the slot is turned on. > > So far all available machines only have PCIe devices that are soldered > onto the motherboard, so there are no "real" slots. As far as we can > tell the PCIe power domain is already powered on at the point where > the m1n1 bootloader takes control. There is a GPIO that controls > power to some devices (WiFi, SDHC on the M1 Pro/Max laptops) and those > devices are initially powered off. The Linux driver doesn't currently > attempt to power these devices on, but U-Boot will power them on if > the appropriate GPIO is defined in the device tree. The way this is > specified in the device tree is still under discussion. Does this mean we basically assume that m1n1 and early Linux boot takes at least the 100ms T_pvperl required by CEM sec 2.2, but we take pains to delay the 100us T_perst-clk? That seems a little weird, but I guess it is clear that REFCLK is *not* enabled before we enable it, so we do need at least the 100us there. It also niggles at me a little that the spec says T_pvperl starts from *power stable* (not from power enable) and T_perst-clk starts from *REFCLK stable* (not REFCLK enable). Since we don't know the time from enable to stable, it seems like native drivers should add some circuit-specific constants to the spec values. > > - Where does mediatek enforce the PCIe sec 6.6.1 delay after > > deasserting PERST# and before config requests? > > > > - Does either apple or mediatek support speeds greater than 5 GT/s, > > and if so, shouldn't we start the sec 6.6.1 100ms delay *after* > > Link training completes? > > The Apple hardware advertises support for 8 GT/s, but all the devices > integrated on the Mac mini support only 2.5 GT/s or 5 GT/s. The spec doesn't say anything about what the downstream devices support (obviously it can't because we don't *know* what those devices are until after we enumerate them). So to be pedantically correct, I'd argue that we should pay attention to what the Root Port advertises. Of course, I don't think we do this correctly *anywhere* today. Bjorn
© 2016 - 2024 Red Hat, Inc.