[PATCH] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[PATCH] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Pali Rohár]

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;
 	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. */
+	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

Re: [PATCH] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Pali Rohár]

PING?

On Sunday 03 July 2022 12:46:27 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;
>  	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. */
> +	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
> 

Re: [PATCH] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Bjorn Helgaas]

On Thu, Aug 18, 2022 at 03:50:54PM +0200, Pali Rohár wrote:
> PING?

No need to shout :)

> On Sunday 03 July 2022 12:46:27 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.

Just curious since I haven't read the patch, and Lorenzo will take
care of this anyway, but does this fix a bug, i.e., does something
work when it didn't work before?  Or does everything *work* without
this patch, but lspci reports capabilities at different offsets than
U-Boot?

> > 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;
> >  	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. */
> > +	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
> > 

Re: [PATCH] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Pali Rohár]

On Thursday 18 August 2022 17:31:36 Bjorn Helgaas wrote:
> On Thu, Aug 18, 2022 at 03:50:54PM +0200, Pali Rohár wrote:
> > PING?
> 
> No need to shout :)

Ok :)

> > On Sunday 03 July 2022 12:46:27 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.
> 
> Just curious since I haven't read the patch, and Lorenzo will take
> care of this anyway, but does this fix a bug, i.e., does something
> work when it didn't work before?  Or does everything *work* without
> this patch, but lspci reports capabilities at different offsets than
> U-Boot?

The last sentence is correct. Everything works with and also without
this patch. Just without this patch lspci reports capabilities at
different offsets than what HW reports and what U-Boot reports (U-Boot
already reports offsets same as in HW).

So lets say, that with this patch, it is easier to compare pci config
space dump from u-boot and linux. And this simplify debugging.

> > > 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;
> > >  	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. */
> > > +	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
> > > 

Re: [PATCH] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Bjorn Helgaas]

On Fri, Aug 19, 2022 at 12:36:36AM +0200, Pali Rohár wrote:
> On Thursday 18 August 2022 17:31:36 Bjorn Helgaas wrote:
> > On Thu, Aug 18, 2022 at 03:50:54PM +0200, Pali Rohár wrote:
> > > On Sunday 03 July 2022 12:46:27 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.
> > 
> > Just curious since I haven't read the patch, and Lorenzo will take
> > care of this anyway, but does this fix a bug, i.e., does something
> > work when it didn't work before?  Or does everything *work* without
> > this patch, but lspci reports capabilities at different offsets than
> > U-Boot?
> 
> The last sentence is correct. Everything works with and also without
> this patch. Just without this patch lspci reports capabilities at
> different offsets than what HW reports and what U-Boot reports (U-Boot
> already reports offsets same as in HW).
> 
> So lets say, that with this patch, it is easier to compare pci config
> space dump from u-boot and linux. And this simplify debugging.

I agree, that's a really annoying difference.  Seems like a good thing
to fix (again, not having looked at the patch, I have no opinion on
the implementation :)).

Bjorn

Re: [PATCH] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Lorenzo Pieralisi]

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
> 

Re: [PATCH] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Pali Rohár]

On Tuesday 23 August 2022 11:06:22 Lorenzo Pieralisi wrote:
> 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.

Macro is without _OFF suffix. I will fix it in v2.

> >  	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 ?

The point is that it is this bridge emulation code which construct
linked list of PCI capabilities, it has full control over placement of
registers and host controller drivers just implement callbacks for
particular capability. Callbacks use register offset relative to the
start of capability, so bridge can put pcie_start and ssid_start to any
position in config space which does not conflict with other things.

And usage of lowest possible value is just some canonical choice where
to put it. It was also before this change. Now host controller drivers
can say that they want to put those capabilities at specific location
and not at location chosen by bridge emulation code.

> 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
> > 

[PATCH v2] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Pali Rohár]

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>
---
Changes in v2:
* Rebase on top of v6.0-rc1, fix usage of PCIE_CAP_PCIEXP
---
 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 966c8b48bd96..4834198cc86b 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1078,6 +1078,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 af915c951f06..0fdbb5585fec 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;
 	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. */
+	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

Re: [PATCH v2] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Lorenzo Pieralisi]

On Tue, Aug 23, 2022 at 12:14:39PM +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>
> ---
> Changes in v2:
> * Rebase on top of v6.0-rc1, fix usage of PCIE_CAP_PCIEXP
> ---
>  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 966c8b48bd96..4834198cc86b 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1078,6 +1078,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 af915c951f06..0fdbb5585fec 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;
>  	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. */
> +	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;

Right. So here the PCI express capability should be made to point to
subsystem ID vendor capability but I don't see anything in the code
that makes it happen. I am just trying to understand what this patch
is changing and AFAICS this code path is not triggered with current
code but it may well be in the future.

In other words: what if PCI express capability is lower in the address
space (config) than the subsystem ID vendor capability ?

I am just trying to understand the patch, so forgive me if the question
is already addressed in the code.

Thanks,
Lorenzo

> +		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
> 

Re: [PATCH v2] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Pali Rohár]

On Tuesday 23 August 2022 17:55:41 Lorenzo Pieralisi wrote:
> On Tue, Aug 23, 2022 at 12:14:39PM +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>
> > ---
> > Changes in v2:
> > * Rebase on top of v6.0-rc1, fix usage of PCIE_CAP_PCIEXP
> > ---
> >  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 966c8b48bd96..4834198cc86b 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -1078,6 +1078,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 af915c951f06..0fdbb5585fec 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;
> >  	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. */
> > +	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;
> 
> Right. So here the PCI express capability should be made to point to
> subsystem ID vendor capability but I don't see anything in the code
> that makes it happen. I am just trying to understand what this patch
> is changing and AFAICS this code path is not triggered with current
> code but it may well be in the future.

This top level branch is called when emulated PCI bridge is PCIe device
(has_pcie is truth) and PCIe host controller driver did not want to put
PCI express capability at any specific offset (pcie_start address is not
filled, is zero), but want to put SSID capability at specific offset
(ssid_start is filled).

Second inner branch is called when between PCI standard capability and
SSID capability is enough place for putting PCI express capability.

So in this case offset for PCI express capability (pcie_start) is set
immediate after the PCI standard capability.

All these conditions (toplevel + inner branch) are not currently called,
because both aardvark and mvebu drivers do not match those conditions.
I put this code here to ensure that if somebody adds a new driver which
will use pci emul bridge, it will work and does not crash kernel because
values pcie_start or ssid_start are not filled but capabilities are
required.

> In other words: what if PCI express capability is lower in the address
> space (config) than the subsystem ID vendor capability ?

Current code expects that if host controller driver sets both pcie_start
and ssid_start, those values are correct, non-overlapping and can be
handled correctly.

And if offset to PCI express capability is lower than offset to SSID
capability then there should not be any issue. First capability is
correctly set into capabilities_pointer (via min function) and then
pci_bridge_emul_conf_read() should handle it.

The whole my idea is to construct capabilities linked list structure
correctly based on input requirements (e.g. fixed location of some
capability, etc).

> I am just trying to understand the patch, so forgive me if the question
> is already addressed in the code.
> 
> Thanks,
> Lorenzo
> 
> > +		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
> > 

Re: [PATCH v2] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Lorenzo Pieralisi]

[...]

> > In other words: what if PCI express capability is lower in the address
> > space (config) than the subsystem ID vendor capability ?
> 
> Current code expects that if host controller driver sets both pcie_start
> and ssid_start, those values are correct, non-overlapping and can be
> handled correctly.
> 
> And if offset to PCI express capability is lower than offset to SSID
> capability then there should not be any issue. First capability is
> correctly set into capabilities_pointer (via min function) and then
> pci_bridge_emul_conf_read() should handle it.

I don't understand how the pointer to the SSID cap is set in this
specific case, I don't see any code doing that but that's most certainly
because I don't know in details the emul bridge internals.

IIUC, in pci_bridge_emul_read_ssid(), we set the next cap pointer if
the PCI express capability is at an address higher (current case) than
the SSID capability otherwise we set it to 0 (end of the list).

The other way around, I don't see the PCI express next cap pointer
being set anywhere (where, IIUC, it should be set to point to the
SSID cap) - I am not sure you are handling it.

That's the only question I have on this patch.

Lorenzo

> The whole my idea is to construct capabilities linked list structure
> correctly based on input requirements (e.g. fixed location of some
> capability, etc).
> 
> > I am just trying to understand the patch, so forgive me if the question
> > is already addressed in the code.
> > 
> > Thanks,
> > Lorenzo
> > 
> > > +		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
> > > 

Re: [PATCH v2] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Pali Rohár]

On Wednesday 24 August 2022 09:25:15 Lorenzo Pieralisi wrote:
> [...]
> 
> > > In other words: what if PCI express capability is lower in the address
> > > space (config) than the subsystem ID vendor capability ?
> > 
> > Current code expects that if host controller driver sets both pcie_start
> > and ssid_start, those values are correct, non-overlapping and can be
> > handled correctly.
> > 
> > And if offset to PCI express capability is lower than offset to SSID
> > capability then there should not be any issue. First capability is
> > correctly set into capabilities_pointer (via min function) and then
> > pci_bridge_emul_conf_read() should handle it.
> 
> I don't understand how the pointer to the SSID cap is set in this
> specific case, I don't see any code doing that but that's most certainly
> because I don't know in details the emul bridge internals.
> 
> IIUC, in pci_bridge_emul_read_ssid(), we set the next cap pointer if
> the PCI express capability is at an address higher (current case) than
> the SSID capability otherwise we set it to 0 (end of the list).
> 
> The other way around, I don't see the PCI express next cap pointer
> being set anywhere (where, IIUC, it should be set to point to the
> SSID cap) - I am not sure you are handling it.

Ou, I see. This is really missing! It should be set when initializing
pcie_conf.cap_id and pcie_conf.cap at this place:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/pci/pci-bridge-emul.c?h=v6.0-rc2#n379

I will fix it and send a new version of this patch.

> That's the only question I have on this patch.
> 
> Lorenzo
> 
> > The whole my idea is to construct capabilities linked list structure
> > correctly based on input requirements (e.g. fixed location of some
> > capability, etc).
> > 
> > > I am just trying to understand the patch, so forgive me if the question
> > > is already addressed in the code.
> > > 
> > > Thanks,
> > > Lorenzo
> > > 
> > > > +		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
> > > > 

[PATCH v3] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Pali Rohár]

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>
---
Changes in v3:
* Correctly initialize pcie_conf.next pointer for SSID capability (if it is
  after the PCIe capability)

Changes in v2:
* Rebase on top of v6.0-rc1, fix usage of PCIE_CAP_PCIEXP
---
 drivers/pci/controller/pci-aardvark.c |  1 +
 drivers/pci/controller/pci-mvebu.c    |  1 +
 drivers/pci/pci-bridge-emul.c         | 48 +++++++++++++++++----------
 drivers/pci/pci-bridge-emul.h         |  2 ++
 4 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 966c8b48bd96..4834198cc86b 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1078,6 +1078,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 af915c951f06..0fdbb5585fec 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;
 	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..9334b2dd4764 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,18 +361,33 @@ 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. */
+	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);
 
 	if (bridge->has_pcie) {
 		bridge->pcie_conf.cap_id = PCI_CAP_ID_EXP;
+		bridge->pcie_conf.next = (bridge->ssid_start > bridge->pcie_start) ?
+					 bridge->ssid_start : 0;
 		bridge->pcie_conf.cap |= cpu_to_le16(PCI_EXP_TYPE_ROOT_PORT << 4);
 		bridge->pcie_cap_regs_behavior =
 			kmemdup(pcie_cap_regs_behavior,
@@ -459,15 +470,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 +551,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

Re: [PATCH v3] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value

[Lorenzo Pieralisi]

On Wed, 24 Aug 2022 13:21:24 +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.
> 
> [...]

Applied to pci/bridge-emul, thanks!

[1/1] PCI: pci-bridge-emul: Set position of PCI capabilities to real HW value
      https://git.kernel.org/lpieralisi/pci/c/658aea35ab88

Thanks,
Lorenzo