Re: [PATCH] drm/radeon: Prefer pcie_capability_read_word()

From: Bjorn Helgaas
Date: Thu Jul 18 2019 - 08:57:04 EST

Next message: Christoph Hellwig: "Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n"
Previous message: Sudip Mukherjee: "Re: regression with napi/softirq ?"
In reply to: Frederick Lawler: "[PATCH] drm/radeon: Prefer pcie_capability_read_word()"
Next in thread: Frederick Lawler: "[PATCH] igc: Prefer pcie_capability_read_word()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Jul 17, 2019 at 9:08 PM Frederick Lawler <fred@xxxxxxxxxxxx> wrote:
>
> Commit 8c0d3a02c130 ("PCI: Add accessors for PCI Express Capability")
> added accessors for the PCI Express Capability so that drivers didn't
> need to be aware of differences between v1 and v2 of the PCI
> Express Capability.
>
> Replace pci_read_config_word() and pci_write_config_word() calls with
> pcie_capability_read_word() and pcie_capability_write_word().
>
> Signed-off-by: Frederick Lawler <fred@xxxxxxxxxxxx>
> ---
> drivers/gpu/drm/radeon/cik.c | 70 +++++++++++++++++++++-------------
> drivers/gpu/drm/radeon/si.c | 73 +++++++++++++++++++++++-------------
> 2 files changed, 90 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
> index ab7b4e2ffcd2..f6c91ac5427a 100644
> --- a/drivers/gpu/drm/radeon/cik.c
> +++ b/drivers/gpu/drm/radeon/cik.c
> @@ -9500,7 +9500,6 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
> {
> struct pci_dev *root = rdev->pdev->bus->self;
> enum pci_bus_speed speed_cap;
> - int bridge_pos, gpu_pos;
> u32 speed_cntl, current_data_rate;
> int i;
> u16 tmp16;
> @@ -9542,12 +9541,7 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
> DRM_INFO("enabling PCIE gen 2 link speeds, disable with radeon.pcie_gen2=0\n");
> }
>
> - bridge_pos = pci_pcie_cap(root);
> - if (!bridge_pos)
> - return;
> -
> - gpu_pos = pci_pcie_cap(rdev->pdev);
> - if (!gpu_pos)
> + if (!pci_is_pcie(root) || !pci_is_pcie(rdev->pdev))
> return;
>
> if (speed_cap == PCIE_SPEED_8_0GT) {
> @@ -9557,14 +9551,17 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
> u16 bridge_cfg2, gpu_cfg2;
> u32 max_lw, current_lw, tmp;
>
> - pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL, &bridge_cfg);
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL, &gpu_cfg);
> + pcie_capability_read_word(root, PCI_EXP_LNKCTL,
> + &bridge_cfg);
> + pcie_capability_read_word(rdev->pdev, PCI_EXP_LNKCTL,
> + &gpu_cfg);
>
> tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD;
> - pci_write_config_word(root, bridge_pos + PCI_EXP_LNKCTL, tmp16);
> + pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16);
>
> tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD;
> - pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL, tmp16);
> + pcie_capability_write_word(rdev->pdev, PCI_EXP_LNKCTL,
> + tmp16);
>
> tmp = RREG32_PCIE_PORT(PCIE_LC_STATUS1);
> max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) >> LC_DETECTED_LINK_WIDTH_SHIFT;
> @@ -9582,15 +9579,23 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
>
> for (i = 0; i < 10; i++) {
> /* check status */
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_DEVSTA, &tmp16);
> + pcie_capability_read_word(rdev->pdev,
> + PCI_EXP_DEVSTA,
> + &tmp16);
> if (tmp16 & PCI_EXP_DEVSTA_TRPND)
> break;
>
> - pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL, &bridge_cfg);
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL, &gpu_cfg);
> + pcie_capability_read_word(root, PCI_EXP_LNKCTL,
> + &bridge_cfg);
> + pcie_capability_read_word(rdev->pdev,
> + PCI_EXP_LNKCTL,
> + &gpu_cfg);
>
> - pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, &bridge_cfg2);
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &gpu_cfg2);
> + pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
> + &bridge_cfg2);
> + pcie_capability_read_word(rdev->pdev,
> + PCI_EXP_LNKCTL2,
> + &gpu_cfg2);
>
> tmp = RREG32_PCIE_PORT(PCIE_LC_CNTL4);
> tmp |= LC_SET_QUIESCE;
> @@ -9603,26 +9608,39 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
> msleep(100);
>
> /* linkctl */
> - pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL, &tmp16);
> + pcie_capability_read_word(root, PCI_EXP_LNKCTL,
> + &tmp16);
> tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
> tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD);
> - pci_write_config_word(root, bridge_pos + PCI_EXP_LNKCTL, tmp16);
> + pcie_capability_write_word(root, PCI_EXP_LNKCTL,
> + tmp16);
>
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL, &tmp16);
> + pcie_capability_read_word(rdev->pdev,
> + PCI_EXP_LNKCTL,
> + &tmp16);
> tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
> tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD);
> - pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL, tmp16);
> + pcie_capability_write_word(rdev->pdev,
> + PCI_EXP_LNKCTL,
> + tmp16);
>
> /* linkctl2 */
> - pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, &tmp16);
> + pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
> + &tmp16);
> tmp16 &= ~((1 << 4) | (7 << 9));
> tmp16 |= (bridge_cfg2 & ((1 << 4) | (7 << 9)));

Looks like we could use some new #defines for these LNKCTL2 bits (also below).

> - pci_write_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, tmp16);
> + pcie_capability_write_word(root,
> + PCI_EXP_LNKCTL2,
> + tmp16);
>
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
> + pcie_capability_read_word(rdev->pdev,
> + PCI_EXP_LNKCTL2,
> + &tmp16);
> tmp16 &= ~((1 << 4) | (7 << 9));
> tmp16 |= (gpu_cfg2 & ((1 << 4) | (7 << 9)));
> - pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);
> + pcie_capability_write_word(rdev->pdev,
> + PCI_EXP_LNKCTL2,
> + tmp16);
>
> tmp = RREG32_PCIE_PORT(PCIE_LC_CNTL4);
> tmp &= ~LC_SET_QUIESCE;
> @@ -9636,7 +9654,7 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
> speed_cntl &= ~LC_FORCE_DIS_SW_SPEED_CHANGE;
> WREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL, speed_cntl);
>
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
> + pcie_capability_read_word(rdev->pdev, PCI_EXP_LNKCTL2, &tmp16);
> tmp16 &= ~0xf;
> if (speed_cap == PCIE_SPEED_8_0GT)
> tmp16 |= 3; /* gen3 */
> @@ -9644,7 +9662,7 @@ static void cik_pcie_gen3_enable(struct radeon_device *rdev)
> tmp16 |= 2; /* gen2 */
> else
> tmp16 |= 1; /* gen1 */
> - pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);
> + pcie_capability_write_word(rdev->pdev, PCI_EXP_LNKCTL2, tmp16);
>
> speed_cntl = RREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL);
> speed_cntl |= LC_INITIATE_LINK_SPEED_CHANGE;
> diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
> index 841bc8bc333d..6916703d7899 100644
> --- a/drivers/gpu/drm/radeon/si.c
> +++ b/drivers/gpu/drm/radeon/si.c
> @@ -3253,7 +3253,7 @@ static void si_gpu_init(struct radeon_device *rdev)
> /* XXX what about 12? */
> rdev->config.si.tile_config |= (3 << 0);
> break;
> - }
> + }
> switch ((mc_arb_ramcfg & NOOFBANK_MASK) >> NOOFBANK_SHIFT) {
> case 0: /* four banks */
> rdev->config.si.tile_config |= 0 << 4;
> @@ -7083,7 +7083,6 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
> {
> struct pci_dev *root = rdev->pdev->bus->self;
> enum pci_bus_speed speed_cap;
> - int bridge_pos, gpu_pos;
> u32 speed_cntl, current_data_rate;
> int i;
> u16 tmp16;
> @@ -7125,12 +7124,7 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
> DRM_INFO("enabling PCIE gen 2 link speeds, disable with radeon.pcie_gen2=0\n");
> }
>
> - bridge_pos = pci_pcie_cap(root);
> - if (!bridge_pos)
> - return;
> -
> - gpu_pos = pci_pcie_cap(rdev->pdev);
> - if (!gpu_pos)
> + if (!pci_is_pcie(root) || !pci_is_pcie(rdev->pdev))
> return;
>
> if (speed_cap == PCIE_SPEED_8_0GT) {
> @@ -7140,14 +7134,17 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
> u16 bridge_cfg2, gpu_cfg2;
> u32 max_lw, current_lw, tmp;
>
> - pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL, &bridge_cfg);
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL, &gpu_cfg);
> + pcie_capability_read_word(root, PCI_EXP_LNKCTL,
> + &bridge_cfg);
> + pcie_capability_read_word(rdev->pdev, PCI_EXP_LNKCTL,
> + &gpu_cfg);
>
> tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD;
> - pci_write_config_word(root, bridge_pos + PCI_EXP_LNKCTL, tmp16);
> + pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16);
>
> tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD;
> - pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL, tmp16);
> + pcie_capability_write_word(rdev->pdev, PCI_EXP_LNKCTL,
> + tmp16);
>
> tmp = RREG32_PCIE(PCIE_LC_STATUS1);
> max_lw = (tmp & LC_DETECTED_LINK_WIDTH_MASK) >> LC_DETECTED_LINK_WIDTH_SHIFT;
> @@ -7165,15 +7162,23 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
>
> for (i = 0; i < 10; i++) {
> /* check status */
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_DEVSTA, &tmp16);
> + pcie_capability_read_word(rdev->pdev,
> + PCI_EXP_DEVSTA,
> + &tmp16);
> if (tmp16 & PCI_EXP_DEVSTA_TRPND)
> break;
>
> - pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL, &bridge_cfg);
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL, &gpu_cfg);
> + pcie_capability_read_word(root, PCI_EXP_LNKCTL,
> + &bridge_cfg);
> + pcie_capability_read_word(rdev->pdev,
> + PCI_EXP_LNKCTL,
> + &gpu_cfg);
>
> - pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, &bridge_cfg2);
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &gpu_cfg2);
> + pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
> + &bridge_cfg2);
> + pcie_capability_read_word(rdev->pdev,
> + PCI_EXP_LNKCTL2,
> + &gpu_cfg2);
>
> tmp = RREG32_PCIE_PORT(PCIE_LC_CNTL4);
> tmp |= LC_SET_QUIESCE;
> @@ -7186,26 +7191,40 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
> msleep(100);
>
> /* linkctl */
> - pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL, &tmp16);
> + pcie_capability_read_word(root, PCI_EXP_LNKCTL,
> + &tmp16);
> tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
> tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD);
> - pci_write_config_word(root, bridge_pos + PCI_EXP_LNKCTL, tmp16);
> + pcie_capability_write_word(root,
> + PCI_EXP_LNKCTL,
> + tmp16);
>
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL, &tmp16);
> + pcie_capability_read_word(rdev->pdev,
> + PCI_EXP_LNKCTL,
> + &tmp16);
> tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
> tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD);
> - pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL, tmp16);
> + pcie_capability_write_word(rdev->pdev,
> + PCI_EXP_LNKCTL,
> + tmp16);
>
> /* linkctl2 */
> - pci_read_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, &tmp16);
> + pcie_capability_read_word(root, PCI_EXP_LNKCTL2,
> + &tmp16);
> tmp16 &= ~((1 << 4) | (7 << 9));
> tmp16 |= (bridge_cfg2 & ((1 << 4) | (7 << 9)));
> - pci_write_config_word(root, bridge_pos + PCI_EXP_LNKCTL2, tmp16);
> + pcie_capability_write_word(root,
> + PCI_EXP_LNKCTL2,
> + tmp16);
>
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
> + pcie_capability_read_word(rdev->pdev,
> + PCI_EXP_LNKCTL2,
> + &tmp16);
> tmp16 &= ~((1 << 4) | (7 << 9));
> tmp16 |= (gpu_cfg2 & ((1 << 4) | (7 << 9)));
> - pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);
> + pcie_capability_write_word(rdev->pdev,
> + PCI_EXP_LNKCTL2,
> + tmp16);
>
> tmp = RREG32_PCIE_PORT(PCIE_LC_CNTL4);
> tmp &= ~LC_SET_QUIESCE;
> @@ -7219,7 +7238,7 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
> speed_cntl &= ~LC_FORCE_DIS_SW_SPEED_CHANGE;
> WREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL, speed_cntl);
>
> - pci_read_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, &tmp16);
> + pcie_capability_read_word(rdev->pdev, PCI_EXP_LNKCTL2, &tmp16);
> tmp16 &= ~0xf;
> if (speed_cap == PCIE_SPEED_8_0GT)
> tmp16 |= 3; /* gen3 */

PCI_EXP_LNKCTL2_TLS, PCI_EXP_LNKCTL2_TLS_8_0GT, etc.

> @@ -7227,7 +7246,7 @@ static void si_pcie_gen3_enable(struct radeon_device *rdev)
> tmp16 |= 2; /* gen2 */
> else
> tmp16 |= 1; /* gen1 */
> - pci_write_config_word(rdev->pdev, gpu_pos + PCI_EXP_LNKCTL2, tmp16);
> + pcie_capability_write_word(rdev->pdev, PCI_EXP_LNKCTL2, tmp16);
>
> speed_cntl = RREG32_PCIE_PORT(PCIE_LC_SPEED_CNTL);
> speed_cntl |= LC_INITIATE_LINK_SPEED_CHANGE;
> --
> 2.17.1
>

Next message: Christoph Hellwig: "Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n"
Previous message: Sudip Mukherjee: "Re: regression with napi/softirq ?"
In reply to: Frederick Lawler: "[PATCH] drm/radeon: Prefer pcie_capability_read_word()"
Next in thread: Frederick Lawler: "[PATCH] igc: Prefer pcie_capability_read_word()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]