Re: [PATCH v3 17/20] PCI: dw-rockchip: Switch to FIELD_PREP_WM16 macro

[Heiko Stuebner <heiko@sntech.de>]

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@google.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

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