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 <bhelg...@google.com> > Signed-off-by: Nicolas Frattaroli <nicolas.frattar...@collabora.com>
Reviewed-by: Heiko Stuebner <he...@sntech.de>
