From: Lorenzo Pieralisi <lpieralisi@kernel.org>
To: "Pali Rohár" <pali@kernel.org>
Cc: "Bjorn Helgaas" <bhelgaas@google.com>,
"Krzysztof Wilczyński" <kw@linux.com>,
"Rob Herring" <robh@kernel.org>,
"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
"Marek Behún" <kabel@kernel.org>,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value
Date: Tue, 23 Aug 2022 11:06:22 +0200 [thread overview]
Message-ID: <YwSYjmJrTDW0T7nf@lpieralisi> (raw)
In-Reply-To: <20220703104627.27058-1-pali@kernel.org>
On Sun, Jul 03, 2022 at 12:46:27PM +0200, Pali Rohár wrote:
> mvebu and aardvark HW have PCIe capabilities on different offset in PCI
> config space. Extend pci-bridge-emul.c code to allow setting custom driver
> custom value where PCIe capabilities starts.
>
> With this change PCIe capabilities of both drivers are reported at the same
> location as where they are reported by U-Boot - in their real HW offset.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> drivers/pci/controller/pci-aardvark.c | 1 +
> drivers/pci/controller/pci-mvebu.c | 1 +
> drivers/pci/pci-bridge-emul.c | 46 +++++++++++++++++----------
> drivers/pci/pci-bridge-emul.h | 2 ++
> 4 files changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index ffec82c8a523..32f97e71e0ca 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -984,6 +984,7 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> bridge->pcie_conf.rootcap = cpu_to_le16(PCI_EXP_RTCAP_CRSVIS);
>
> bridge->has_pcie = true;
> + bridge->pcie_start = PCIE_CORE_PCIEXP_CAP;
> bridge->data = pcie;
> bridge->ops = &advk_pci_bridge_emul_ops;
>
> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index c1ffdb06c971..cb7cf3f4802f 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -946,6 +946,7 @@ static int mvebu_pci_bridge_emul_init(struct mvebu_pcie_port *port)
> bridge->subsystem_vendor_id = ssdev_id & 0xffff;
> bridge->subsystem_id = ssdev_id >> 16;
> bridge->has_pcie = true;
> + bridge->pcie_start = PCIE_CAP_PCIEXP_OFF;
Is this patch to be applied against v6.0-rc1 ? Just asking, can't
find this define.
> bridge->data = port;
> bridge->ops = &mvebu_pci_bridge_emul_ops;
>
> diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
> index 9c2ca28e3ecf..dfbbe43ef518 100644
> --- a/drivers/pci/pci-bridge-emul.c
> +++ b/drivers/pci/pci-bridge-emul.c
> @@ -22,11 +22,7 @@
>
> #define PCI_BRIDGE_CONF_END PCI_STD_HEADER_SIZEOF
> #define PCI_CAP_SSID_SIZEOF (PCI_SSVID_DEVICE_ID + 2)
> -#define PCI_CAP_SSID_START PCI_BRIDGE_CONF_END
> -#define PCI_CAP_SSID_END (PCI_CAP_SSID_START + PCI_CAP_SSID_SIZEOF)
> #define PCI_CAP_PCIE_SIZEOF (PCI_EXP_SLTSTA2 + 2)
> -#define PCI_CAP_PCIE_START PCI_CAP_SSID_END
> -#define PCI_CAP_PCIE_END (PCI_CAP_PCIE_START + PCI_CAP_PCIE_SIZEOF)
>
> /**
> * struct pci_bridge_reg_behavior - register bits behaviors
> @@ -324,7 +320,7 @@ pci_bridge_emul_read_ssid(struct pci_bridge_emul *bridge, int reg, u32 *value)
> switch (reg) {
> case PCI_CAP_LIST_ID:
> *value = PCI_CAP_ID_SSVID |
> - (bridge->has_pcie ? (PCI_CAP_PCIE_START << 8) : 0);
> + ((bridge->pcie_start > bridge->ssid_start) ? (bridge->pcie_start << 8) : 0);
> return PCI_BRIDGE_EMUL_HANDLED;
>
> case PCI_SSVID_VENDOR_ID:
> @@ -365,12 +361,25 @@ int pci_bridge_emul_init(struct pci_bridge_emul *bridge,
> if (!bridge->pci_regs_behavior)
> return -ENOMEM;
>
> - if (bridge->subsystem_vendor_id)
> - bridge->conf.capabilities_pointer = PCI_CAP_SSID_START;
> - else if (bridge->has_pcie)
> - bridge->conf.capabilities_pointer = PCI_CAP_PCIE_START;
> - else
> - bridge->conf.capabilities_pointer = 0;
> + /* If ssid_start and pcie_start were not specified then choose the lowest possible value. */
Is this an assumption ? I don't see why this logic belongs in the
generic bridge emulation, I'd say that ssid_start, pcie_start and
the capabilities pointer should be set in the respective host
controller drivers, I don't think this belong in generic bridge
emulation code.
Thoughts ?
Lorenzo
> + if (!bridge->ssid_start && !bridge->pcie_start) {
> + if (bridge->subsystem_vendor_id)
> + bridge->ssid_start = PCI_BRIDGE_CONF_END;
> + if (bridge->has_pcie)
> + bridge->pcie_start = bridge->ssid_start + PCI_CAP_SSID_SIZEOF;
> + } else if (!bridge->ssid_start && bridge->subsystem_vendor_id) {
> + if (bridge->pcie_start - PCI_BRIDGE_CONF_END >= PCI_CAP_SSID_SIZEOF)
> + bridge->ssid_start = PCI_BRIDGE_CONF_END;
> + else
> + bridge->ssid_start = bridge->pcie_start + PCI_CAP_PCIE_SIZEOF;
> + } else if (!bridge->pcie_start && bridge->has_pcie) {
> + if (bridge->ssid_start - PCI_BRIDGE_CONF_END >= PCI_CAP_PCIE_SIZEOF)
> + bridge->pcie_start = PCI_BRIDGE_CONF_END;
> + else
> + bridge->pcie_start = bridge->ssid_start + PCI_CAP_SSID_SIZEOF;
> + }
> +
> + bridge->conf.capabilities_pointer = min(bridge->ssid_start, bridge->pcie_start);
>
> if (bridge->conf.capabilities_pointer)
> bridge->conf.status |= cpu_to_le16(PCI_STATUS_CAP_LIST);
> @@ -459,15 +468,17 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul *bridge, int where,
> read_op = bridge->ops->read_base;
> cfgspace = (__le32 *) &bridge->conf;
> behavior = bridge->pci_regs_behavior;
> - } else if (reg >= PCI_CAP_SSID_START && reg < PCI_CAP_SSID_END && bridge->subsystem_vendor_id) {
> + } else if (reg >= bridge->ssid_start && reg < bridge->ssid_start + PCI_CAP_SSID_SIZEOF &&
> + bridge->subsystem_vendor_id) {
> /* Emulated PCI Bridge Subsystem Vendor ID capability */
> - reg -= PCI_CAP_SSID_START;
> + reg -= bridge->ssid_start;
> read_op = pci_bridge_emul_read_ssid;
> cfgspace = NULL;
> behavior = NULL;
> - } else if (reg >= PCI_CAP_PCIE_START && reg < PCI_CAP_PCIE_END && bridge->has_pcie) {
> + } else if (reg >= bridge->pcie_start && reg < bridge->pcie_start + PCI_CAP_PCIE_SIZEOF &&
> + bridge->has_pcie) {
> /* Our emulated PCIe capability */
> - reg -= PCI_CAP_PCIE_START;
> + reg -= bridge->pcie_start;
> read_op = bridge->ops->read_pcie;
> cfgspace = (__le32 *) &bridge->pcie_conf;
> behavior = bridge->pcie_cap_regs_behavior;
> @@ -538,9 +549,10 @@ int pci_bridge_emul_conf_write(struct pci_bridge_emul *bridge, int where,
> write_op = bridge->ops->write_base;
> cfgspace = (__le32 *) &bridge->conf;
> behavior = bridge->pci_regs_behavior;
> - } else if (reg >= PCI_CAP_PCIE_START && reg < PCI_CAP_PCIE_END && bridge->has_pcie) {
> + } else if (reg >= bridge->pcie_start && reg < bridge->pcie_start + PCI_CAP_PCIE_SIZEOF &&
> + bridge->has_pcie) {
> /* Our emulated PCIe capability */
> - reg -= PCI_CAP_PCIE_START;
> + reg -= bridge->pcie_start;
> write_op = bridge->ops->write_pcie;
> cfgspace = (__le32 *) &bridge->pcie_conf;
> behavior = bridge->pcie_cap_regs_behavior;
> diff --git a/drivers/pci/pci-bridge-emul.h b/drivers/pci/pci-bridge-emul.h
> index 71392b67471d..2a0e59c7f0d9 100644
> --- a/drivers/pci/pci-bridge-emul.h
> +++ b/drivers/pci/pci-bridge-emul.h
> @@ -131,6 +131,8 @@ struct pci_bridge_emul {
> struct pci_bridge_reg_behavior *pci_regs_behavior;
> struct pci_bridge_reg_behavior *pcie_cap_regs_behavior;
> void *data;
> + u8 pcie_start;
> + u8 ssid_start;
> bool has_pcie;
> u16 subsystem_vendor_id;
> u16 subsystem_id;
> --
> 2.20.1
>
next prev parent reply other threads:[~2022-08-23 10:43 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-03 10:46 [PATCH] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value Pali Rohár
2022-08-18 13:50 ` Pali Rohár
2022-08-18 22:31 ` Bjorn Helgaas
2022-08-18 22:36 ` Pali Rohár
2022-08-18 23:02 ` Bjorn Helgaas
2022-08-23 9:06 ` Lorenzo Pieralisi [this message]
2022-08-23 9:25 ` Pali Rohár
2022-08-23 10:14 ` [PATCH v2] " Pali Rohár
2022-08-23 15:55 ` Lorenzo Pieralisi
2022-08-23 16:31 ` Pali Rohár
2022-08-24 7:25 ` Lorenzo Pieralisi
2022-08-24 10:33 ` Pali Rohár
2022-08-24 11:21 ` [PATCH v3] " Pali Rohár
2022-08-25 10:09 ` Lorenzo Pieralisi
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=YwSYjmJrTDW0T7nf@lpieralisi \
--to=lpieralisi@kernel.org \
--cc=bhelgaas@google.com \
--cc=kabel@kernel.org \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=pali@kernel.org \
--cc=robh@kernel.org \
--cc=thomas.petazzoni@bootlin.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).