Re: [PATCH v1 2/3] PCI: brcmstb: Clkreq# accomodations of downstream device

From: Bjorn Helgaas <helgaas@kernel.org>
To: Jim Quinlan <jim2101024@gmail.com>
Cc: linux-pci@vger.kernel.org,
	"Nicolas Saenz Julienne" <nsaenz@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Cyril Brulebois" <kibi@debian.org>,
	"Phil Elwell" <phil@raspberrypi.com>,
	bcm-kernel-feedback-list@broadcom.com,
	james.quinlan@broadcom.com,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Rob Herring" <robh@kernel.org>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
	<linux-rpi-kernel@lists.infradead.org>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
	<linux-arm-kernel@lists.infradead.org>,
	"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 2/3] PCI: brcmstb: Clkreq# accomodations of downstream device
Date: Thu, 6 Apr 2023 14:09:53 -0500	[thread overview]
Message-ID: <20230406190953.GA3723665@bhelgaas> (raw)
In-Reply-To: <20230406124625.41325-3-jim2101024@gmail.com>

On Thu, Apr 06, 2023 at 08:46:23AM -0400, Jim Quinlan wrote:
> The Broadcom STB/CM PCIe HW core, which is also used in RPi SOCs, may be
> set into three mutually exclusive modes:
> 
>   (a) No clkreq# expected or required, refclk is always available.
>   (b) Clkreq# is expected to be driven by downstream device when needed.
>   (c) Bidirectional clkreq# for L1SS capable devices.
> 
> Previously, only (b) was supported by the driver, as almost all STB boards
> operate in this mode.  But now there is interest in activating L1SS power
> savings from STB customers, and also interest in accomodating mode (a) for
> designs such as the RPi CM4 with IO board.
> 
> The HW can tell us when mode (a) mode is needed.  But there is no easy way
> to tell if L1SS mode is needed.  Unfortunately, getting this wrong causes a
> panic during boot time.  So we rely on the DT prop "brcm,enable-l1ss" to
> tell us when mode (c) is desired.  This property has already been in
> use by Raspian Linux, but this immplementation adds more details and
> discerns between (a) and (b) automatically.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217276
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>

> +	 * We have "seen" clkreq# if it is asserted or has been in the past.
> +	 * Note that the CLKREQ_IN_MASK is 1 if clkreq# is asserted.

"CLKREQ#" to match PCIe spec and comments below.

> +	if (l1ss && IS_ENABLED(CONFIG_PCIEASPM)) {
> +		/*
> +		 * Note: This (l1ss) mode may not meet requirement for
> +		 * Endpoints that require CLKREQ# assertion to clock active
> +		 * within 400ns.
> +		 */
> +		clkreq_set |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK;
> +		dev_info(pcie->dev, "bi-dir clkreq; l1ss-capable devs only\n");
> +		dev_info(pcie->dev, "ASPM policy must be set to powersupersave\n");

Seems problematic since L1SS can be enabled/disabled at run-time:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-bus-pci?id=v6.2#n420

The simplistic answer is to advertise L1SS support if and only if you
can safely support it.

I don't know why this is an issue for this device but not others.  Is
it because there's some problem in the way the board is designed?  Or
(after skimming the bugzilla) maybe a problem with the plug-in cards?

Bjorn