From: Bjorn Helgaas <helgaas@kernel.org>
To: Geraldo Nascimento <geraldogabriel@gmail.com>
Cc: linux-rockchip@lists.infradead.org,
"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>,
linux-phy@lists.infradead.org, linux-pci@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines
Date: Fri, 13 Jun 2025 15:14:09 -0500 [thread overview]
Message-ID: <20250613201409.GA973486@bhelgaas> (raw)
In-Reply-To: <992ab6278af59b8f2f82521bf4611f69a916bbe1.1749827015.git.geraldogabriel@gmail.com>
On Fri, Jun 13, 2025 at 12:05:31PM -0300, Geraldo Nascimento wrote:
> Current code uses custom-defined register offsets
> and bitfields for standard PCIe registers. Change
> to using standard PCIe defines.
Wrap to fill 75 columns so there's space for "git log" to add
indentation.
> @@ -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;
It looks funny to write PCI_EXP_LNKCTL with bits from PCI_EXP_LNKSTA.
I guess this is because rockchip_pcie_write() does 32-bit writes, but
PCI_EXP_LNKCTL and PCI_EXP_LNKSTA are adjacent 16-bit registers.
If the hardware supports it, adding rockchip_pcie_readw() and
rockchip_pcie_writew() for 16-bit accesses would make this read
better.
Hopefully the hardware *does* support this (it's required per spec at
least for config accesses, which would be a different path in the
hardware). Doing the 32-bit write of PCI_EXP_LNKCTL above is
problematic because writes PCI_EXP_LNKSTA as well, and PCI_EXP_LNKSTA
includes some RW1C bits that may be unintentionally cleared.
> - 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);
This assumes the value you read from PCI_EXP_DEVCAP had zeroes in
these bits. It might, but it would look safer to do:
status &= ~(PCI_EXP_DEVCAP_PWR_VAL | PCI_EXP_DEVCAP_PWR_SCL);
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);
Similar problem here; PCI_EXP_DEVCTL is only 16 bits, and writing the
adjacent PCI_EXP_DEVSTA may clear RW1C bits you didn't want to clear.
Bjorn
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
next prev parent reply other threads:[~2025-06-13 20:16 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-13 15:05 [RESEND RFC PATCH v4 0/5] PCI: rockchip: Improve driver quality Geraldo Nascimento
2025-06-13 15:05 ` [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines Geraldo Nascimento
2025-06-13 20:14 ` Bjorn Helgaas [this message]
2025-06-13 20:26 ` Geraldo Nascimento
2025-06-13 20:50 ` Bjorn Helgaas
2025-06-13 21:01 ` Geraldo Nascimento
2025-06-14 2:31 ` Geraldo Nascimento
2025-06-14 1:38 ` Geraldo Nascimento
2025-06-13 15:05 ` [RESEND RFC PATCH v4 2/5] PCI: rockchip: Drop unused custom registers and bitfields Geraldo Nascimento
2025-06-13 15:06 ` [RESEND RFC PATCH v4 3/5] PCI: rockchip: Set Target Link Speed before retraining Geraldo Nascimento
2025-06-13 20:15 ` Bjorn Helgaas
2025-06-13 20:27 ` Geraldo Nascimento
2025-06-13 15:06 ` [RESEND RFC PATCH v4 4/5] phy: rockchip-pcie: Enable all four lanes Geraldo Nascimento
2025-06-13 15:06 ` [RESEND RFC PATCH v4 5/5] phy: rockchip-pcie: Adjust read mask and write Geraldo Nascimento
2025-06-13 20:20 ` Bjorn Helgaas
2025-06-13 20:32 ` Geraldo Nascimento
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=20250613201409.GA973486@bhelgaas \
--to=helgaas@kernel.org \
--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=robh@kernel.org \
--cc=shawn.lin@rock-chips.com \
--cc=vkoul@kernel.org \
/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).