Received-SPF: pass (google.com: domain of linux-kernel+bounces-132070-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 4 Apr 2024 22:01:29 +0200
From: Cyril Brulebois <kibi@debian.org>
To: Jim Quinlan <james.quinlan@broadcom.com>
Cc: linux-pci@vger.kernel.org, Nicolas Saenz Julienne <nsaenz@kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Phil Elwell <phil@raspberrypi.com>,
	bcm-kernel-feedback-list@broadcom.com,
	Conor Dooley <conor+dt@kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>,
	Florian Fainelli <florian.fainelli@broadcom.com>,
	Jim Quinlan <jim2101024@gmail.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org>,
	open list <linux-kernel@vger.kernel.org>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@lists.infradead.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Rob Herring <robh@kernel.org>
Subject: Re: [PATCH v9 0/4] PCI: brcmstb: Configure appropriate HW CLKREQ#
 mode
Message-ID: <20240404200129.3qp4qs6zklbk2prl@mraw.org>
Organization: Debian
References: <20240403213902.26391-1-james.quinlan@broadcom.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="4mlfdkp3wjmc3cs3"
Content-Disposition: inline
In-Reply-To: <20240403213902.26391-1-james.quinlan@broadcom.com>


--4mlfdkp3wjmc3cs3
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi Jim,

Jim Quinlan <james.quinlan@broadcom.com> (2024-04-03):
> v9 -- v8 was setting an internal bus timeout to accomodate large L1 exit
>       latencies.  After meeting the PCIe HW team it was revealed that the
>       HW default timeout value was set low for the purposes of HW debuggi=
ng
>       convenience; for nominal operation it needs to be set to a higher
>       value independent of this submission's purpose.  This is now a
>       separate commit.
>=20
>    -- With v8, Bjorne asked what was preventing a device from exceeding t=
he
>       time required for the above internal bus timeout.  The answer to th=
is
>       is for us to set the endpoints' max latency {no-,}snoop value to
>       something below this internal timeout value.  If the endpoint
>       respects this value as it should, it will not send an LTR request
>       with a larger latency value and not put itself in a situation
>       that requires more latency than is possible for the platform.
>=20
>       Typically, ACPI or FW sets these max latency values.  In most of our
>       systems we do not have this happening so it is up to the RC driver =
to
>       set these values in the endpoint devices.  If the endpoints already
>       have non-zero values that are lower than what we are setting, we let
>       them be, as it is possible ACPI or FW set them and knows something
>       that we do not.
>=20
>    -- The "clkreq" commit has only been changed to remove the code that w=
as
>       setting the timeout value, as this code is now its own commit.

Given the bot's feedback, I took the liberty of running tests on your
patch series except with an extra =E2=80=9Cstatic=E2=80=9D keyword. On my b=
uild system,
gcc 12 wasn't complaining about it but I didn't spend time trying to
find the right options, or trying a switch to clang to confirm the
before/after situation:

    -void brcm_set_downstream_devs_ltr_max(struct brcm_pcie *pcie)
    +static void brcm_set_downstream_devs_ltr_max(struct brcm_pcie *pcie)


Anyway, this is still:

Tested-by: Cyril Brulebois <cyril@debamax.com>


Test setup:
-----------

 - using a $CM with the 20230111 EEPROM
 - on the same CM4 IO Board
 - with a $PCIE board (PCIe to multiple USB ports)
 - and the same Samsung USB flash drive + Logitech keyboard.

where $CM is one of:

 - CM4 Lite Rev 1.0
 - CM4 8/32 Rev 1.0
 - CM4 4/32 Rev 1.1

and $PCIE is one of:

 - SupaHub PCE6U1C-R02, VER 006
 - SupaHub PCE6U1C-R02, VER 006S
 - Waveshare VIA VL805/806-based


Results:
--------

 1. Given this is already v9, and given I don't see how this could have
    possibly changed, I didn't build or tested an unpatched kernel,
    which I would still expect to produce either a successful boot
    *without* seeing the devices plugged on the PCIe-to-USB board or the
    dreaded SError in most cases.

 2. With a patched kernel (v6.7-562-g9f8413c4a66f2 + this series +
    =E2=80=9Cstatic=E2=80=9D in front of brcm_set_downstream_devs_ltr_max()=
), for all
    $CM/$PCIE combinations, I'm getting a system that boots, sees the
    flash drive, and gives decent read/write performance on it (plus a
    functional keyboard).


Cheers,
--=20
Cyril Brulebois (kibi@debian.org)            <https://debamax.com/>
D-I release manager -- Release team member -- Freelance Consultant

--4mlfdkp3wjmc3cs3
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEtg6/KYRFPHDXTPR4/5FK8MKzVSAFAmYPBxYACgkQ/5FK8MKz
VSCIpw//YwRVw8lAie3wKx/qgQbpoFTlOXGLNFVwWKZFPhIgx5EQg+iWTKffg3EE
yoq8uWofgddD3Rcmhspu5a/Af/DX7s7RkrVT3BH3PfwJ+WaAm/60vpVxx8vhNl8r
kQvc1mwzuyyqh4BgmpcC2vYSn7ECJNSdawcY0bi4lJvCHmU60Bv1Lg4SQ0cqVKs/
OSjZdwyeLOTfovDQevKD0xRV52WxlrMZg7dhPG2DcUytrKl+36PPEpO6LdzxibCy
LPAsklzDMOTgfF/7jTZWgAxyL4c5UslXk1X6zkKyWJTmABlvu2RBt7dgdINTKH68
g+5y5vApXegkbkZDFvFes25QvOZMFNx3JcmHVO4qp2EvsVv4krhAIyFb4BX0Zdjw
ORHv2KJQD/x56JweQ/iX72oMz2PKZCpLSTExhLerF2lGrBlubqMLmZqII1MwL6T0
IMwEo1PB4UkBo8wWDZPUhk786Oxp6jNxdNtNRa3caD4KwvQOwP0nbGgSoDN91LHY
tTb1cjdE03S0fmihIdIdfBM+dHZGKbQY6SoPGZ+xtyW98Moc6VpI8O0uAIJYO5gP
FdQSn0NEOTFYlVDZvldQMkgPaAUi4fRmNOBQu7WWvaUHa+nlSFG7TpjqWJKY+CYJ
ekw3PxfOn7Z677QjnBeXPr99kCAqiZBR/KEzs+USaGStNcObWns=
=LON4
-----END PGP SIGNATURE-----

--4mlfdkp3wjmc3cs3--