Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

PCI: dw-rockchip: Switch to FIELD_PREP_WM16 macro

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>
Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>

authored by

Nicolas Frattaroli and committed by
Yury Norov
30e91957 eb0bf4f0

+26 -16
+26 -16
drivers/pci/controller/dwc/pcie-dw-rockchip.c
··· 11 11 #include <linux/bitfield.h> 12 12 #include <linux/clk.h> 13 13 #include <linux/gpio/consumer.h> 14 + #include <linux/hw_bitfield.h> 14 15 #include <linux/irqchip/chained_irq.h> 15 16 #include <linux/irqdomain.h> 16 17 #include <linux/mfd/syscon.h> ··· 30 29 * The upper 16 bits of PCIE_CLIENT_CONFIG are a write 31 30 * mask for the lower 16 bits. 32 31 */ 33 - #define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val)) 34 - #define HIWORD_UPDATE_BIT(val) HIWORD_UPDATE(val, val) 35 - #define HIWORD_DISABLE_BIT(val) HIWORD_UPDATE(val, ~val) 36 32 37 33 #define to_rockchip_pcie(x) dev_get_drvdata((x)->dev) 38 34 39 35 /* General Control Register */ 40 36 #define PCIE_CLIENT_GENERAL_CON 0x0 41 - #define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40) 42 - #define PCIE_CLIENT_EP_MODE HIWORD_UPDATE(0xf0, 0x0) 43 - #define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc) 44 - #define PCIE_CLIENT_DISABLE_LTSSM HIWORD_UPDATE(0x0c, 0x8) 37 + #define PCIE_CLIENT_MODE_MASK GENMASK(7, 4) 38 + #define PCIE_CLIENT_MODE_EP 0x0UL 39 + #define PCIE_CLIENT_MODE_RC 0x4UL 40 + #define PCIE_CLIENT_SET_MODE(x) FIELD_PREP_WM16(PCIE_CLIENT_MODE_MASK, (x)) 41 + #define PCIE_CLIENT_LD_RQ_RST_GRT FIELD_PREP_WM16(BIT(3), 1) 42 + #define PCIE_CLIENT_ENABLE_LTSSM FIELD_PREP_WM16(BIT(2), 1) 43 + #define PCIE_CLIENT_DISABLE_LTSSM FIELD_PREP_WM16(BIT(2), 0) 45 44 46 45 /* Interrupt Status Register Related to Legacy Interrupt */ 47 46 #define PCIE_CLIENT_INTR_STATUS_LEGACY 0x8 ··· 53 52 54 53 /* Interrupt Mask Register Related to Legacy Interrupt */ 55 54 #define PCIE_CLIENT_INTR_MASK_LEGACY 0x1c 55 + #define PCIE_INTR_MASK GENMASK(7, 0) 56 + #define PCIE_INTR_CLAMP(_x) ((BIT((_x)) & PCIE_INTR_MASK)) 57 + #define PCIE_INTR_LEGACY_MASK(x) (PCIE_INTR_CLAMP((x)) | \ 58 + (PCIE_INTR_CLAMP((x)) << 16)) 59 + #define PCIE_INTR_LEGACY_UNMASK(x) (PCIE_INTR_CLAMP((x)) << 16) 56 60 57 61 /* Interrupt Mask Register Related to Miscellaneous Operation */ 58 62 #define PCIE_CLIENT_INTR_MASK_MISC 0x24 ··· 122 116 static void rockchip_intx_mask(struct irq_data *data) 123 117 { 124 118 rockchip_pcie_writel_apb(irq_data_get_irq_chip_data(data), 125 - HIWORD_UPDATE_BIT(BIT(data->hwirq)), 119 + PCIE_INTR_LEGACY_MASK(data->hwirq), 126 120 PCIE_CLIENT_INTR_MASK_LEGACY); 127 121 }; 128 122 129 123 static void rockchip_intx_unmask(struct irq_data *data) 130 124 { 131 125 rockchip_pcie_writel_apb(irq_data_get_irq_chip_data(data), 132 - HIWORD_DISABLE_BIT(BIT(data->hwirq)), 126 + PCIE_INTR_LEGACY_UNMASK(data->hwirq), 133 127 PCIE_CLIENT_INTR_MASK_LEGACY); 134 128 }; 135 129 ··· 495 489 dev_dbg(dev, "hot reset or link-down reset\n"); 496 490 dw_pcie_ep_linkdown(&pci->ep); 497 491 /* Stop delaying link training. */ 498 - val = HIWORD_UPDATE_BIT(PCIE_LTSSM_APP_DLY2_DONE); 492 + val = FIELD_PREP_WM16(PCIE_LTSSM_APP_DLY2_DONE, 1); 499 493 rockchip_pcie_writel_apb(rockchip, val, 500 494 PCIE_CLIENT_HOT_RESET_CTRL); 501 495 } ··· 534 528 } 535 529 536 530 /* LTSSM enable control mode */ 537 - val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); 531 + val = FIELD_PREP_WM16(PCIE_LTSSM_ENABLE_ENHANCE, 1); 538 532 rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); 539 533 540 - rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, 534 + rockchip_pcie_writel_apb(rockchip, 535 + PCIE_CLIENT_SET_MODE(PCIE_CLIENT_MODE_RC), 541 536 PCIE_CLIENT_GENERAL_CON); 542 537 543 538 pp = &rockchip->pci.pp; ··· 552 545 } 553 546 554 547 /* unmask DLL up/down indicator */ 555 - val = HIWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED, 0); 548 + val = FIELD_PREP_WM16(PCIE_RDLH_LINK_UP_CHGED, 0); 556 549 rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC); 557 550 558 551 return ret; ··· 584 577 * LTSSM enable control mode, and automatically delay link training on 585 578 * hot reset/link-down reset. 586 579 */ 587 - val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE | PCIE_LTSSM_APP_DLY2_EN); 580 + val = FIELD_PREP_WM16(PCIE_LTSSM_ENABLE_ENHANCE, 1) | 581 + FIELD_PREP_WM16(PCIE_LTSSM_APP_DLY2_EN, 1); 588 582 rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); 589 583 590 - rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_EP_MODE, 584 + rockchip_pcie_writel_apb(rockchip, 585 + PCIE_CLIENT_SET_MODE(PCIE_CLIENT_MODE_EP), 591 586 PCIE_CLIENT_GENERAL_CON); 592 587 593 588 rockchip->pci.ep.ops = &rockchip_pcie_ep_ops; ··· 613 604 pci_epc_init_notify(rockchip->pci.ep.epc); 614 605 615 606 /* unmask DLL up/down indicator and hot reset/link-down reset */ 616 - val = HIWORD_UPDATE(PCIE_RDLH_LINK_UP_CHGED | PCIE_LINK_REQ_RST_NOT_INT, 0); 607 + val = FIELD_PREP_WM16(PCIE_RDLH_LINK_UP_CHGED, 0) | 608 + FIELD_PREP_WM16(PCIE_LINK_REQ_RST_NOT_INT, 0); 617 609 rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC); 618 610 619 611 return ret;