From: Philipp Stanner <pstanner@redhat.com>
To: Geraldo Nascimento <geraldogabriel@gmail.com>,
linux-rockchip@lists.infradead.org
Cc: "Shawn Lin" <shawn.lin@rock-chips.com>,
"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
"Krzysztof Wilczyński" <kw@linux.com>,
"Manivannan Sadhasivam" <mani@kernel.org>,
"Rob Herring" <robh@kernel.org>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Heiko Stuebner" <heiko@sntech.de>,
"Vinod Koul" <vkoul@kernel.org>,
"Kishon Vijay Abraham I" <kishon@kernel.org>,
"Rick wertenbroek" <rick.wertenbroek@gmail.com>,
"Neil Armstrong" <neil.armstrong@linaro.org>,
"Valmantas Paliksa" <walmis@gmail.com>,
linux-phy@lists.infradead.org, linux-pci@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [RESEND PATCH v9 1/4] PCI: rockchip: Use standard PCIe defines
Date: Tue, 01 Jul 2025 09:54:51 +0200 [thread overview]
Message-ID: <ce0fdbf2753075056c227ce7567a69386a5aa0a3.camel@redhat.com> (raw)
In-Reply-To: <e81700ef4b49f584bc8834bfb07b6d8995fc1f42.1751322015.git.geraldogabriel@gmail.com>
On Mon, 2025-06-30 at 19:24 -0300, Geraldo Nascimento wrote:
> Current code uses custom-defined register offsets and bitfields for
> standard PCIe registers. Change to using standard PCIe defines. Since
> we are now using standard PCIe defines, drop unused custom-defined
> ones,
> which are now referenced from offset at added Capabilities Register.
This could be phrased a bit more cleanly. At least I don't get exactly
what "from offset" means. You mean you replace the unused custom ones?
But if they're unused, why are they even being replaced?
>
> Suggested-By: Bjorn Helgaas <bhelgaas@google.com>
s/By/by
P.
> Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> ---
> drivers/pci/controller/pcie-rockchip-ep.c | 4 +-
> drivers/pci/controller/pcie-rockchip-host.c | 44 ++++++++++---------
> --
> drivers/pci/controller/pcie-rockchip.h | 12 +-----
> 3 files changed, 25 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c
> b/drivers/pci/controller/pcie-rockchip-ep.c
> index 55416b8311dd..300cd85fa035 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -518,9 +518,9 @@ static void rockchip_pcie_ep_retrain_link(struct
> rockchip_pcie *rockchip)
> {
> u32 status;
>
> - status = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_LCS);
> + status = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
> PCI_EXP_LNKCTL);
> status |= PCI_EXP_LNKCTL_RL;
> - rockchip_pcie_write(rockchip, status, PCIE_EP_CONFIG_LCS);
> + rockchip_pcie_write(rockchip, status, PCIE_EP_CONFIG_BASE +
> PCI_EXP_LNKCTL);
> }
>
> static bool rockchip_pcie_ep_link_up(struct rockchip_pcie *rockchip)
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c
> b/drivers/pci/controller/pcie-rockchip-host.c
> index b9e7a8710cf0..65653218b9ab 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -40,18 +40,18 @@ static void rockchip_pcie_enable_bw_int(struct
> rockchip_pcie *rockchip)
> {
> u32 status;
>
> - status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> + status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR +
> PCI_EXP_LNKCTL);
> status |= (PCI_EXP_LNKCTL_LBMIE | PCI_EXP_LNKCTL_LABIE);
> - rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> + rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR +
> PCI_EXP_LNKCTL);
> }
>
> static void rockchip_pcie_clr_bw_int(struct rockchip_pcie *rockchip)
> {
> u32 status;
>
> - status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> + status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR +
> PCI_EXP_LNKCTL);
> status |= (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_LABS) << 16;
> - rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> + rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR +
> PCI_EXP_LNKCTL);
> }
>
> static void rockchip_pcie_update_txcredit_mui(struct rockchip_pcie
> *rockchip)
> @@ -269,7 +269,7 @@ static void rockchip_pcie_set_power_limit(struct
> rockchip_pcie *rockchip)
> scale = 3; /* 0.001x */
> curr = curr / 1000; /* convert to mA */
> power = (curr * 3300) / 1000; /* milliwatt */
> - while (power > PCIE_RC_CONFIG_DCR_CSPL_LIMIT) {
> + while (power > FIELD_MAX(PCI_EXP_DEVCAP_PWR_VAL)) {
> if (!scale) {
> dev_warn(rockchip->dev, "invalid power
> supply\n");
> return;
> @@ -278,10 +278,10 @@ static void
> rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
> power = power / 10;
> }
>
> - status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DCR);
> - status |= (power << PCIE_RC_CONFIG_DCR_CSPL_SHIFT) |
> - (scale << PCIE_RC_CONFIG_DCR_CPLS_SHIFT);
> - rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCR);
> + status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR +
> PCI_EXP_DEVCAP);
> + status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_VAL, power);
> + status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_SCL, scale);
> + rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR +
> PCI_EXP_DEVCAP);
> }
>
> /**
> @@ -309,14 +309,14 @@ static int rockchip_pcie_host_init_port(struct
> rockchip_pcie *rockchip)
> rockchip_pcie_set_power_limit(rockchip);
>
> /* Set RC's clock architecture as common clock */
> - status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> + status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR +
> PCI_EXP_LNKCTL);
> status |= PCI_EXP_LNKSTA_SLC << 16;
> - rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> + rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR +
> PCI_EXP_LNKCTL);
>
> /* Set RC's RCB to 128 */
> - status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> + status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR +
> PCI_EXP_LNKCTL);
> status |= PCI_EXP_LNKCTL_RCB;
> - rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> + rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR +
> PCI_EXP_LNKCTL);
>
> /* Enable Gen1 training */
> rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
> @@ -341,9 +341,9 @@ static int rockchip_pcie_host_init_port(struct
> rockchip_pcie *rockchip)
> * Enable retrain for gen2. This should be
> configured only after
> * gen1 finished.
> */
> - status = rockchip_pcie_read(rockchip,
> PCIE_RC_CONFIG_LCS);
> + status = rockchip_pcie_read(rockchip,
> PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
> status |= PCI_EXP_LNKCTL_RL;
> - rockchip_pcie_write(rockchip, status,
> PCIE_RC_CONFIG_LCS);
> + rockchip_pcie_write(rockchip, status,
> PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>
> err = readl_poll_timeout(rockchip->apb_base +
> PCIE_CORE_CTRL,
> status,
> PCIE_LINK_IS_GEN2(status), 20,
> @@ -380,15 +380,15 @@ static int rockchip_pcie_host_init_port(struct
> rockchip_pcie *rockchip)
>
> /* Clear L0s from RC's link cap */
> if (of_property_read_bool(dev->of_node, "aspm-no-l0s")) {
> - status = rockchip_pcie_read(rockchip,
> PCIE_RC_CONFIG_LINK_CAP);
> - status &= ~PCIE_RC_CONFIG_LINK_CAP_L0S;
> - rockchip_pcie_write(rockchip, status,
> PCIE_RC_CONFIG_LINK_CAP);
> + status = rockchip_pcie_read(rockchip,
> PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP);
> + status &= ~PCI_EXP_LNKCAP_ASPM_L0S;
> + rockchip_pcie_write(rockchip, status,
> PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP);
> }
>
> - status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DCSR);
> - status &= ~PCIE_RC_CONFIG_DCSR_MPS_MASK;
> - status |= PCIE_RC_CONFIG_DCSR_MPS_256;
> - rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCSR);
> + status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR +
> PCI_EXP_DEVCTL);
> + status &= ~PCI_EXP_DEVCTL_PAYLOAD;
> + status |= PCI_EXP_DEVCTL_PAYLOAD_256B;
> + rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR +
> PCI_EXP_DEVCTL);
>
> return 0;
> err_power_off_phy:
> diff --git a/drivers/pci/controller/pcie-rockchip.h
> b/drivers/pci/controller/pcie-rockchip.h
> index 5864a20323f2..f5cbf3c9d2d9 100644
> --- a/drivers/pci/controller/pcie-rockchip.h
> +++ b/drivers/pci/controller/pcie-rockchip.h
> @@ -155,17 +155,7 @@
> #define PCIE_EP_CONFIG_DID_VID (PCIE_EP_CONFIG_BASE + 0x00)
> #define PCIE_EP_CONFIG_LCS (PCIE_EP_CONFIG_BASE + 0xd0)
> #define PCIE_RC_CONFIG_RID_CCR (PCIE_RC_CONFIG_BASE + 0x08)
> -#define PCIE_RC_CONFIG_DCR (PCIE_RC_CONFIG_BASE + 0xc4)
> -#define PCIE_RC_CONFIG_DCR_CSPL_SHIFT 18
> -#define PCIE_RC_CONFIG_DCR_CSPL_LIMIT 0xff
> -#define PCIE_RC_CONFIG_DCR_CPLS_SHIFT 26
> -#define PCIE_RC_CONFIG_DCSR (PCIE_RC_CONFIG_BASE + 0xc8)
> -#define PCIE_RC_CONFIG_DCSR_MPS_MASK GENMASK(7, 5)
> -#define PCIE_RC_CONFIG_DCSR_MPS_256 (0x1 << 5)
> -#define PCIE_RC_CONFIG_LINK_CAP (PCIE_RC_CONFIG_BASE
> + 0xcc)
> -#define PCIE_RC_CONFIG_LINK_CAP_L0S BIT(10)
> -#define PCIE_RC_CONFIG_LCS (PCIE_RC_CONFIG_BASE + 0xd0)
> -#define PCIE_EP_CONFIG_LCS (PCIE_EP_CONFIG_BASE + 0xd0)
> +#define PCIE_RC_CONFIG_CR (PCIE_RC_CONFIG_BASE + 0xc0)
> #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE +
> 0x90c)
> #define PCIE_RC_CONFIG_THP_CAP (PCIE_RC_CONFIG_BASE +
> 0x274)
> #define PCIE_RC_CONFIG_THP_CAP_NEXT_MASK GENMASK(31, 20)
next prev parent reply other threads:[~2025-07-01 7:55 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-30 22:24 [RESEND PATCH v9 0/4] PCI: rockchip: Improve driver quality Geraldo Nascimento
2025-06-30 22:24 ` [RESEND PATCH v9 1/4] PCI: rockchip: Use standard PCIe defines Geraldo Nascimento
2025-07-01 7:54 ` Philipp Stanner [this message]
2025-07-01 12:03 ` Geraldo Nascimento
2025-07-01 12:44 ` Manivannan Sadhasivam
2025-07-07 22:22 ` Bjorn Helgaas
2025-07-07 23:04 ` Geraldo Nascimento
2025-06-30 22:24 ` [RESEND PATCH v9 2/4] PCI: rockchip: Set Target Link Speed before retraining Geraldo Nascimento
2025-06-30 22:25 ` [RESEND PATCH v9 3/4] phy: rockchip-pcie: Enable all four lanes if required Geraldo Nascimento
2025-06-30 22:25 ` [RESEND PATCH v9 4/4] phy: rockchip-pcie: Properly disable TEST_WRITE strobe signal Geraldo Nascimento
2025-07-01 7:43 ` (subset) [RESEND PATCH v9 0/4] PCI: rockchip: Improve driver quality Manivannan Sadhasivam
2025-07-22 13:37 ` Vinod Koul
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ce0fdbf2753075056c227ce7567a69386a5aa0a3.camel@redhat.com \
--to=pstanner@redhat.com \
--cc=bhelgaas@google.com \
--cc=geraldogabriel@gmail.com \
--cc=heiko@sntech.de \
--cc=kishon@kernel.org \
--cc=kw@linux.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-phy@lists.infradead.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=lpieralisi@kernel.org \
--cc=mani@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=rick.wertenbroek@gmail.com \
--cc=robh@kernel.org \
--cc=shawn.lin@rock-chips.com \
--cc=vkoul@kernel.org \
--cc=walmis@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).