Re: [PATCH 17/20] PCI: dw-rockchip: switch to HWORD_UPDATE macro

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

Hello,

On Friday, 13 June 2025 11:45:22 Central European Summer Time Niklas Cassel wrote:
> Hello Nicolas,
> 
> On Thu, Jun 12, 2025 at 08:56:19PM +0200, Nicolas Frattaroli wrote:
> > 
> > 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 HWORD_UPDATE macro, which grants us the benefit of better
> > compile-time error checking.
> 
> The current code looks like this:
> #define  PCIE_CLIENT_RC_MODE            HIWORD_UPDATE_BIT(0x40)
> #define  PCIE_CLIENT_EP_MODE            HIWORD_UPDATE(0xf0, 0x0)
> 
> The device_type field is defined like this:
> 4'h0: PCI Express endpoint
> 4'h1: Legacy PCI Express endpoint
> 4'h4: Root port of PCI Express root complex
> 
> The reset value of the device_type field is 0x0 (EP mode).
> 
> So switching between RC mode / EP mode should be fine.
> 
> But I agree, theoretically there could be a bug if e.g. bootloader
> has set the device_type to 0x1 (Legacy EP).
> 
> So if you want, you could send a patch:
> -#define  PCIE_CLIENT_RC_MODE            HIWORD_UPDATE_BIT(0x40)
> +#define  PCIE_CLIENT_RC_MODE            HIWORD_UPDATE(0xf0, 0x40)
> 
> With:
> Fixes: 0e898eb8df4e ("PCI: rockchip-dwc: Add Rockchip RK356X host controller driver")
> 
> But I also think that your current patch is fine as-is.
> 
> I do however think that you can drop this line:
> +#define  PCIE_CLIENT_MODE_LEGACY       0x1U
> 
> Since the define is never used.

Will do

> 
> 
> Also, is there any point in adding the U suffix?
> 
> Usually you see UL or ULL suffix, when that is needed, but there actually
> seems to be extremely few hits of simply U suffix:
> $ git grep 0x1U | grep -v UL

Sort of. Literals without the U suffix are considered signed iirc, and
operating with them and then left-shifting the result can run into issues
if you shift their bits into the sign bit. In the patch at [1] I needed to
quell a compiler warning about signed long overflows with a U suffix. This
should only ever really be a problem for anything that gets shifted up to
bit index 31 I believe, and maybe there's a better way to handle this in
the macro itself with an explicit cast to unsigned, but explicit casts
give me the ick. I'm also open to changing it to an UL, which will have
the same effect, and has more precedent.

> 
> 
> Kind regards,
> Niklas
> 

Best Regards,
Nicolas Frattaroli

Link: https://lore.kernel.org/all/20250612-byeword-update-v1-7-f4afb8f6313f@collabora.com/ [1]