Message-ID: <5730130.X9hSmTKtgW@phil>
Date: Wed, 27 Aug 2025 09:58:04 +0200
From: Heiko Stuebner <heiko@...ech.de>
To: Yury Norov <yury.norov@...il.com>,
 Rasmus Villemoes <linux@...musvillemoes.dk>,
 Jaehoon Chung <jh80.chung@...sung.com>, Ulf Hansson <ulf.hansson@...aro.org>,
 Shreeya Patel <shreeya.patel@...labora.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Sandy Huang <hjc@...k-chips.com>,
 Andy Yan <andy.yan@...k-chips.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
 Nicolas Frattaroli <frattaroli.nicolas@...il.com>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Shawn Lin <shawn.lin@...k-chips.com>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>,
 Manivannan Sadhasivam <mani@...nel.org>, Rob Herring <robh@...nel.org>,
 Bjorn Helgaas <bhelgaas@...gle.com>, Chanwoo Choi <cw00.choi@...sung.com>,
 MyungJoo Ham <myungjoo.ham@...sung.com>,
 Kyungmin Park <kyungmin.park@...sung.com>, Qin Jian <qinjian@...lus1.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
 Nathan Chancellor <nathan@...nel.org>,
 Nick Desaulniers <nick.desaulniers+lkml@...il.com>,
 Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
 Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Cc: kernel@...labora.com, linux-kernel@...r.kernel.org,
 linux-mmc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-media@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, linux-phy@...ts.infradead.org,
 linux-sound@...r.kernel.org, netdev@...r.kernel.org,
 linux-stm32@...md-mailman.stormreply.com, linux-pci@...r.kernel.org,
 linux-pm@...r.kernel.org, linux-clk@...r.kernel.org, llvm@...ts.linux.dev,
 Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Subject:
 Re: [PATCH v3 17/20] PCI: dw-rockchip: Switch to FIELD_PREP_WM16 macro

Am Montag, 25. August 2025, 10:28:37 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> The era of hand-rolled HIWORD_UPDATE macros is over.
> 
> Like many other Rockchip drivers, pcie-dw-rockchip brings with it its
> very own flavour of HIWORD_UPDATE. It's occasionally used without a
> constant mask, which complicates matters. HIWORD_UPDATE_BIT is a
> confusingly named addition, as it doesn't update the bit, it actually
> sets all bits in the value to 1. HIWORD_DISABLE_BIT is similarly
> confusing; it disables several bits at once by using the value as a mask
> and the inverse of value as the value, and the "disabling only these"
> effect comes from the hardware actually using the mask. The more obvious
> approach would've been HIWORD_UPDATE(val, 0) in my opinion.
> 
> This is part of the motivation why this patch uses hw_bitfield.h's
> FIELD_PREP_WM16 instead, where possible. FIELD_PREP_WM16 requires a
> constant bit mask, which isn't possible where the irq number is used to
> generate a bit mask. For that purpose, we replace it with a more robust
> macro than what was there but that should also bring close to zero
> runtime overhead: we actually mask the IRQ number to make sure we're not
> writing garbage.
> 
> For the remaining bits, there also are some caveats. For starters, the
> PCIE_CLIENT_ENABLE_LTSSM and PCIE_CLIENT_DISABLE_LTSSM were named in a
> manner that isn't quite truthful to what they do. Their modification
> actually spans not just the LTSSM bit but also another bit, flipping
> only the LTSSM one, but keeping the other (which according to the TRM
> has a reset value of 0) always enabled. This other bit is reserved as of
> the IP version RK3588 uses at least, and I have my doubts as to whether
> it was meant to be set, and whether it was meant to be set in that code
> path. Either way, it's confusing.
> 
> Replace it with just writing either 1 or 0 to the LTSSM bit, using the
> new FIELD_PREP_WM16 macro from hw_bitfield.h, which grants us the
> benefit of better compile-time error checking.
> 
> The change of no longer setting the reserved bit doesn't appear to
> change the behaviour on RK3568 in RC mode, where it's not marked as
> reserved.
> 
> PCIE_CLIENT_RC_MODE/PCIE_CLIENT_EP_MODE was another field that wasn't
> super clear on what the bit field modification actually is. As far as I
> can tell, switching to RC mode doesn't actually write the correct value
> to the field if any of its bits have been set previously, as it only
> updates one bit of a 4 bit field.
> 
> Replace it by actually writing the full values to the field, using the
> new FIELD_PREP_WM16 macro, which grants us the benefit of better
> compile-time error checking.
> 
> This patch was tested on RK3588 (PCIe3 x4 controller), RK3576 (PCIe2 x1
> controller) and RK3568 (PCIe x2 controller), all in RC mode.
> 
> Acked-by: Bjorn Helgaas <bhelgaas@...gle.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>

Reviewed-by: Heiko Stuebner <heiko@...ech.de>

Powered by blists - more mailing lists