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 bitfield.h's
HWORD_UPDATE instead, where possible. HWORD_UPDATE 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 HWORD_UPDATE macro from 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 HWORD_UPDATE 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.
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
drivers/pci/controller/dwc/pcie-dw-rockchip.c | 39 ++++++++++++++++-----------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 93171a3928794915ad1e8c03c017ce0afc1f9169..29363346f2cd9774d8d2e06cd76f7f82e6a7fecf 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -29,18 +29,19 @@
* The upper 16 bits of PCIE_CLIENT_CONFIG are a write
* mask for the lower 16 bits.
*/
-#define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val))
-#define HIWORD_UPDATE_BIT(val) HIWORD_UPDATE(val, val)
-#define HIWORD_DISABLE_BIT(val) HIWORD_UPDATE(val, ~val)
#define to_rockchip_pcie(x) dev_get_drvdata((x)->dev)
/* General Control Register */
#define PCIE_CLIENT_GENERAL_CON 0x0
-#define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40)
-#define PCIE_CLIENT_EP_MODE HIWORD_UPDATE(0xf0, 0x0)
-#define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc)
-#define PCIE_CLIENT_DISABLE_LTSSM HIWORD_UPDATE(0x0c, 0x8)
+#define PCIE_CLIENT_MODE_MASK GENMASK(7, 4)
+#define PCIE_CLIENT_MODE_EP 0x0U
+#define PCIE_CLIENT_MODE_LEGACY 0x1U
+#define PCIE_CLIENT_MODE_RC 0x4U
+#define PCIE_CLIENT_SET_MODE(x) HWORD_UPDATE(PCIE_CLIENT_MODE_MASK, (x))
+#define PCIE_CLIENT_LD_RQ_RST_GRT HWORD_UPDATE(BIT(3), 1)
+#define PCIE_CLIENT_ENABLE_LTSSM HWORD_UPDATE(BIT(2), 1)
+#define PCIE_CLIENT_DISABLE_LTSSM HWORD_UPDATE(BIT(2), 0)
/* Interrupt Status Register Related to Legacy Interrupt */
#define PCIE_CLIENT_INTR_STATUS_LEGACY 0x8
@@ -52,6 +53,11 @@
/* Interrupt Mask Register Related to Legacy Interrupt */
#define PCIE_CLIENT_INTR_MASK_LEGACY 0x1c
+#define PCIE_INTR_MASK GENMASK(7, 0)
+#define PCIE_INTR_CLAMP(_x) ((BIT((_x)) & PCIE_INTR_MASK))
+#define PCIE_INTR_LEGACY_MASK(x) (PCIE_INTR_CLAMP((x)) | \
+ (PCIE_INTR_CLAMP((x)) << 16))
+#define PCIE_INTR_LEGACY_UNMASK(x) (PCIE_INTR_CLAMP((x)) << 16)
/* Interrupt Mask Register Related to Miscellaneous Operation */
#define PCIE_CLIENT_INTR_MASK_MISC 0x24
@@ -114,14 +120,14 @@ static void rockchip_pcie_intx_handler(struct irq_desc *desc)
static void rockchip_intx_mask(struct irq_data *data)
{
rockchip_pcie_writel_apb(irq_data_get_irq_chip_data(data),
- HIWORD_UPDATE_BIT(BIT(data->hwirq)),
+ PCIE_INTR_LEGACY_MASK(data->hwirq),
PCIE_CLIENT_INTR_MASK_LEGACY);
};
static void rockchip_intx_unmask(struct irq_data *data)
{
rockchip_pcie_writel_apb(irq_data_get_irq_chip_data(data),
- HIWORD_DISABLE_BIT(BIT(data->hwirq)),
+ PCIE_INTR_LEGACY_UNMASK(data->hwirq),
PCIE_CLIENT_INTR_MASK_LEGACY);
};
@@ -521,10 +527,11 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
}
/* LTSSM enable control mode */
- val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
+ val = HWORD_UPDATE(PCIE_LTSSM_ENABLE_ENHANCE, 1);
rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
- rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE,
+ rockchip_pcie_writel_apb(rockchip,
+ PCIE_CLIENT_SET_MODE(PCIE_CLIENT_MODE_RC),
PCIE_CLIENT_GENERAL_CON);
pp = &rockchip->pci.pp;
@@ -538,7 +545,7 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
}
/* unmask DLL up/down indicator */
- val = HIWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED, 0);
+ val = HWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED, 0);
rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
return ret;
@@ -567,10 +574,11 @@ static int rockchip_pcie_configure_ep(struct platform_device *pdev,
}
/* LTSSM enable control mode */
- val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
+ val = HWORD_UPDATE(PCIE_LTSSM_ENABLE_ENHANCE, 1);
rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
- rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_EP_MODE,
+ rockchip_pcie_writel_apb(rockchip,
+ PCIE_CLIENT_SET_MODE(PCIE_CLIENT_MODE_EP),
PCIE_CLIENT_GENERAL_CON);
rockchip->pci.ep.ops = &rockchip_pcie_ep_ops;
@@ -594,7 +602,8 @@ static int rockchip_pcie_configure_ep(struct platform_device *pdev,
pci_epc_init_notify(rockchip->pci.ep.epc);
/* unmask DLL up/down indicator and hot reset/link-down reset */
- val = HIWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED | PCIE_LINK_REQ_RST_NOT_INT, 0);
+ val = HWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED, 0) |
+ HWORD_UPDATE(PCIE_LINK_REQ_RST_NOT_INT, 0);
rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
return ret;
--
2.49.0Hello 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.
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
Kind regards,
Niklas
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]
On Thu, Jun 12, 2025 at 08:56:19PM +0200, Nicolas Frattaroli wrote:
> 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 bitfield.h's
> HWORD_UPDATE instead, where possible. HWORD_UPDATE 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 HWORD_UPDATE macro from 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 HWORD_UPDATE 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.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
PCI: dw-rockchip: Switch to HWORD_UPDATE macro
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 39 ++++++++++++++++-----------
> 1 file changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 93171a3928794915ad1e8c03c017ce0afc1f9169..29363346f2cd9774d8d2e06cd76f7f82e6a7fecf 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -29,18 +29,19 @@
> * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
> * mask for the lower 16 bits.
> */
> -#define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val))
> -#define HIWORD_UPDATE_BIT(val) HIWORD_UPDATE(val, val)
> -#define HIWORD_DISABLE_BIT(val) HIWORD_UPDATE(val, ~val)
>
> #define to_rockchip_pcie(x) dev_get_drvdata((x)->dev)
>
> /* General Control Register */
> #define PCIE_CLIENT_GENERAL_CON 0x0
> -#define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40)
> -#define PCIE_CLIENT_EP_MODE HIWORD_UPDATE(0xf0, 0x0)
> -#define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc)
> -#define PCIE_CLIENT_DISABLE_LTSSM HIWORD_UPDATE(0x0c, 0x8)
> +#define PCIE_CLIENT_MODE_MASK GENMASK(7, 4)
> +#define PCIE_CLIENT_MODE_EP 0x0U
> +#define PCIE_CLIENT_MODE_LEGACY 0x1U
> +#define PCIE_CLIENT_MODE_RC 0x4U
> +#define PCIE_CLIENT_SET_MODE(x) HWORD_UPDATE(PCIE_CLIENT_MODE_MASK, (x))
> +#define PCIE_CLIENT_LD_RQ_RST_GRT HWORD_UPDATE(BIT(3), 1)
> +#define PCIE_CLIENT_ENABLE_LTSSM HWORD_UPDATE(BIT(2), 1)
> +#define PCIE_CLIENT_DISABLE_LTSSM HWORD_UPDATE(BIT(2), 0)
>
> /* Interrupt Status Register Related to Legacy Interrupt */
> #define PCIE_CLIENT_INTR_STATUS_LEGACY 0x8
> @@ -52,6 +53,11 @@
>
> /* Interrupt Mask Register Related to Legacy Interrupt */
> #define PCIE_CLIENT_INTR_MASK_LEGACY 0x1c
> +#define PCIE_INTR_MASK GENMASK(7, 0)
> +#define PCIE_INTR_CLAMP(_x) ((BIT((_x)) & PCIE_INTR_MASK))
> +#define PCIE_INTR_LEGACY_MASK(x) (PCIE_INTR_CLAMP((x)) | \
> + (PCIE_INTR_CLAMP((x)) << 16))
> +#define PCIE_INTR_LEGACY_UNMASK(x) (PCIE_INTR_CLAMP((x)) << 16)
>
> /* Interrupt Mask Register Related to Miscellaneous Operation */
> #define PCIE_CLIENT_INTR_MASK_MISC 0x24
> @@ -114,14 +120,14 @@ static void rockchip_pcie_intx_handler(struct irq_desc *desc)
> static void rockchip_intx_mask(struct irq_data *data)
> {
> rockchip_pcie_writel_apb(irq_data_get_irq_chip_data(data),
> - HIWORD_UPDATE_BIT(BIT(data->hwirq)),
> + PCIE_INTR_LEGACY_MASK(data->hwirq),
> PCIE_CLIENT_INTR_MASK_LEGACY);
> };
>
> static void rockchip_intx_unmask(struct irq_data *data)
> {
> rockchip_pcie_writel_apb(irq_data_get_irq_chip_data(data),
> - HIWORD_DISABLE_BIT(BIT(data->hwirq)),
> + PCIE_INTR_LEGACY_UNMASK(data->hwirq),
> PCIE_CLIENT_INTR_MASK_LEGACY);
> };
>
> @@ -521,10 +527,11 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
> }
>
> /* LTSSM enable control mode */
> - val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
> + val = HWORD_UPDATE(PCIE_LTSSM_ENABLE_ENHANCE, 1);
> rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
>
> - rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE,
> + rockchip_pcie_writel_apb(rockchip,
> + PCIE_CLIENT_SET_MODE(PCIE_CLIENT_MODE_RC),
> PCIE_CLIENT_GENERAL_CON);
>
> pp = &rockchip->pci.pp;
> @@ -538,7 +545,7 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
> }
>
> /* unmask DLL up/down indicator */
> - val = HIWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED, 0);
> + val = HWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED, 0);
> rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
>
> return ret;
> @@ -567,10 +574,11 @@ static int rockchip_pcie_configure_ep(struct platform_device *pdev,
> }
>
> /* LTSSM enable control mode */
> - val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
> + val = HWORD_UPDATE(PCIE_LTSSM_ENABLE_ENHANCE, 1);
> rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
>
> - rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_EP_MODE,
> + rockchip_pcie_writel_apb(rockchip,
> + PCIE_CLIENT_SET_MODE(PCIE_CLIENT_MODE_EP),
> PCIE_CLIENT_GENERAL_CON);
>
> rockchip->pci.ep.ops = &rockchip_pcie_ep_ops;
> @@ -594,7 +602,8 @@ static int rockchip_pcie_configure_ep(struct platform_device *pdev,
> pci_epc_init_notify(rockchip->pci.ep.epc);
>
> /* unmask DLL up/down indicator and hot reset/link-down reset */
> - val = HIWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED | PCIE_LINK_REQ_RST_NOT_INT, 0);
> + val = HWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED, 0) |
> + HWORD_UPDATE(PCIE_LINK_REQ_RST_NOT_INT, 0);
> rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
>
> return ret;
>
> --
> 2.49.0
>
© 2016 - 2025 Red Hat, Inc.