| Message ID | 20211111153549.29111-7-kabel@kernel.org |
|---|---|
| State | Accepted |
| Commit | a48e4287d61a08663bb95bc515c751c342e0ffb6 |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | PCI mvebu and aardvark changes | expand |
On 11/11/21 16:35, Marek Behún wrote: > From: Pali Rohár <pali@kernel.org> > > The PCI Bridge which represents mvebu PCIe Root Port has Expansion ROM > Base Address register at offset 0x30 but its meaning is different that > of PCI's Expansion ROM BAR register, although the address format of > the register is the same. > > In reality, this device does not have any configurable PCI BARs. So > ensure that write operation into BARs (including Expansion ROM BAR) is a > noop and registers always contain zero address which indicates that BARs > are unsupported. > > Fixes: a7b61ab58d5d ("pci: pci_mvebu: Properly configure and use PCI Bridge (PCIe Root Port)") > Signed-off-by: Pali Rohár <pali@kernel.org> > Signed-off-by: Marek Behún <marek.behun@nic.cz> Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > --- > drivers/pci/pci_mvebu.c | 55 +++++++++++++++++++++++------------------ > 1 file changed, 31 insertions(+), 24 deletions(-) > > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c > index b545e62689..701a17dfb7 100644 > --- a/drivers/pci/pci_mvebu.c > +++ b/drivers/pci/pci_mvebu.c > @@ -93,7 +93,7 @@ struct mvebu_pcie { > unsigned int mem_attr; > unsigned int io_target; > unsigned int io_attr; > - u32 cfgcache[(0x34 - 0x10) / 4]; > + u32 cfgcache[(0x3c - 0x10) / 4]; > }; > > /* > @@ -189,20 +189,20 @@ static int mvebu_pcie_read_config(const struct udevice *bus, pci_dev_t bdf, > } > > /* > - * mvebu has different internal registers mapped into PCI config space > - * in range 0x10-0x34 for PCI bridge, so do not access PCI config space > - * for this range and instead read content from driver virtual cfgcache > + * The configuration space of the PCI Bridge on primary (first) bus is > + * of Type 0 but the BAR registers (including ROM BAR) don't have the > + * same meaning as in the PCIe specification. Therefore do not access > + * BAR registers and non-common registers (those which have different > + * meaning for Type 0 and Type 1 config space) of the PCI Bridge and > + * instead read their content from driver virtual cfgcache[]. > */ > - if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) { > + if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) || > + (offset >= 0x38 && offset < 0x3c))) { > data = pcie->cfgcache[(offset - 0x10) / 4]; > debug("(addr,size,val)=(0x%04x, %d, 0x%08x) from cfgcache\n", > offset, size, data); > *valuep = pci_conv_32_to_size(data, offset, size); > return 0; > - } else if (busno == pcie->first_busno && > - (offset & ~3) == PCI_ROM_ADDRESS1) { > - /* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */ > - offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF; > } > > /* > @@ -269,17 +269,21 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf, > } > > /* > - * mvebu has different internal registers mapped into PCI config space > - * in range 0x10-0x34 for PCI bridge, so do not access PCI config space > - * for this range and instead write content to driver virtual cfgcache > + * As explained in mvebu_pcie_read_config(), PCI Bridge Type 1 specific > + * config registers are not available, so we write their content only > + * into driver virtual cfgcache[]. > + * And as explained in mvebu_pcie_probe(), mvebu has its own specific > + * way for configuring primary and secondary bus numbers. > */ > - if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) { > + if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) || > + (offset >= 0x38 && offset < 0x3c))) { > debug("Writing to cfgcache only\n"); > data = pcie->cfgcache[(offset - 0x10) / 4]; > data = pci_conv_size_to_32(data, value, offset, size); > /* mvebu PCI bridge does not have configurable bars */ > if ((offset & ~3) == PCI_BASE_ADDRESS_0 || > - (offset & ~3) == PCI_BASE_ADDRESS_1) > + (offset & ~3) == PCI_BASE_ADDRESS_1 || > + (offset & ~3) == PCI_ROM_ADDRESS1) > data = 0x0; > pcie->cfgcache[(offset - 0x10) / 4] = data; > /* mvebu has its own way how to set PCI primary bus number */ > @@ -297,10 +301,6 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf, > pcie->sec_busno); > } > return 0; > - } else if (busno == pcie->first_busno && > - (offset & ~3) == PCI_ROM_ADDRESS1) { > - /* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */ > - offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF; > } > > /* > @@ -424,13 +424,20 @@ static int mvebu_pcie_probe(struct udevice *dev) > * U-Boot cannot recognize as P2P Bridge. > * > * Note that this mvebu PCI Bridge does not have compliant Type 1 > - * Configuration Space. Header Type is reported as Type 0 and in > - * range 0x10-0x34 it has aliased internal mvebu registers 0x10-0x34 > - * (e.g. PCIE_BAR_LO_OFF) and register 0x38 is reserved. > + * Configuration Space. Header Type is reported as Type 0 and it > + * has format of Type 0 config space. > * > - * Driver for this range redirects access to virtual cfgcache[] buffer > - * which avoids changing internal mvebu registers. And changes Header > - * Type response value to Type 1. > + * Moreover Type 0 BAR registers (ranges 0x10 - 0x28 and 0x30 - 0x34) > + * have the same format in Marvell's specification as in PCIe > + * specification, but their meaning is totally different and they do > + * different things: they are aliased into internal mvebu registers > + * (e.g. PCIE_BAR_LO_OFF) and these should not be changed or > + * reconfigured by pci device drivers. > + * > + * So our driver converts Type 0 config space to Type 1 and reports > + * Header Type as Type 1. Access to BAR registers and to non-existent > + * Type 1 registers is redirected to the virtual cfgcache[] buffer, > + * which avoids changing unrelated registers. > */ > reg = readl(pcie->base + PCIE_DEV_REV_OFF); > reg &= ~0xffffff00; > Viele Grüße, Stefan Roese
On 11/11/21 16:35, Marek Behún wrote: > From: Pali Rohár <pali@kernel.org> > > The PCI Bridge which represents mvebu PCIe Root Port has Expansion ROM > Base Address register at offset 0x30 but its meaning is different that > of PCI's Expansion ROM BAR register, although the address format of > the register is the same. > > In reality, this device does not have any configurable PCI BARs. So > ensure that write operation into BARs (including Expansion ROM BAR) is a > noop and registers always contain zero address which indicates that BARs > are unsupported. > > Fixes: a7b61ab58d5d ("pci: pci_mvebu: Properly configure and use PCI Bridge (PCIe Root Port)") > Signed-off-by: Pali Rohár <pali@kernel.org> > Signed-off-by: Marek Behún <marek.behun@nic.cz> Applied to u-boot-marvell/master Thanks, Stefan > --- > drivers/pci/pci_mvebu.c | 55 +++++++++++++++++++++++------------------ > 1 file changed, 31 insertions(+), 24 deletions(-) > > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c > index b545e62689..701a17dfb7 100644 > --- a/drivers/pci/pci_mvebu.c > +++ b/drivers/pci/pci_mvebu.c > @@ -93,7 +93,7 @@ struct mvebu_pcie { > unsigned int mem_attr; > unsigned int io_target; > unsigned int io_attr; > - u32 cfgcache[(0x34 - 0x10) / 4]; > + u32 cfgcache[(0x3c - 0x10) / 4]; > }; > > /* > @@ -189,20 +189,20 @@ static int mvebu_pcie_read_config(const struct udevice *bus, pci_dev_t bdf, > } > > /* > - * mvebu has different internal registers mapped into PCI config space > - * in range 0x10-0x34 for PCI bridge, so do not access PCI config space > - * for this range and instead read content from driver virtual cfgcache > + * The configuration space of the PCI Bridge on primary (first) bus is > + * of Type 0 but the BAR registers (including ROM BAR) don't have the > + * same meaning as in the PCIe specification. Therefore do not access > + * BAR registers and non-common registers (those which have different > + * meaning for Type 0 and Type 1 config space) of the PCI Bridge and > + * instead read their content from driver virtual cfgcache[]. > */ > - if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) { > + if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) || > + (offset >= 0x38 && offset < 0x3c))) { > data = pcie->cfgcache[(offset - 0x10) / 4]; > debug("(addr,size,val)=(0x%04x, %d, 0x%08x) from cfgcache\n", > offset, size, data); > *valuep = pci_conv_32_to_size(data, offset, size); > return 0; > - } else if (busno == pcie->first_busno && > - (offset & ~3) == PCI_ROM_ADDRESS1) { > - /* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */ > - offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF; > } > > /* > @@ -269,17 +269,21 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf, > } > > /* > - * mvebu has different internal registers mapped into PCI config space > - * in range 0x10-0x34 for PCI bridge, so do not access PCI config space > - * for this range and instead write content to driver virtual cfgcache > + * As explained in mvebu_pcie_read_config(), PCI Bridge Type 1 specific > + * config registers are not available, so we write their content only > + * into driver virtual cfgcache[]. > + * And as explained in mvebu_pcie_probe(), mvebu has its own specific > + * way for configuring primary and secondary bus numbers. > */ > - if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) { > + if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) || > + (offset >= 0x38 && offset < 0x3c))) { > debug("Writing to cfgcache only\n"); > data = pcie->cfgcache[(offset - 0x10) / 4]; > data = pci_conv_size_to_32(data, value, offset, size); > /* mvebu PCI bridge does not have configurable bars */ > if ((offset & ~3) == PCI_BASE_ADDRESS_0 || > - (offset & ~3) == PCI_BASE_ADDRESS_1) > + (offset & ~3) == PCI_BASE_ADDRESS_1 || > + (offset & ~3) == PCI_ROM_ADDRESS1) > data = 0x0; > pcie->cfgcache[(offset - 0x10) / 4] = data; > /* mvebu has its own way how to set PCI primary bus number */ > @@ -297,10 +301,6 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf, > pcie->sec_busno); > } > return 0; > - } else if (busno == pcie->first_busno && > - (offset & ~3) == PCI_ROM_ADDRESS1) { > - /* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */ > - offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF; > } > > /* > @@ -424,13 +424,20 @@ static int mvebu_pcie_probe(struct udevice *dev) > * U-Boot cannot recognize as P2P Bridge. > * > * Note that this mvebu PCI Bridge does not have compliant Type 1 > - * Configuration Space. Header Type is reported as Type 0 and in > - * range 0x10-0x34 it has aliased internal mvebu registers 0x10-0x34 > - * (e.g. PCIE_BAR_LO_OFF) and register 0x38 is reserved. > + * Configuration Space. Header Type is reported as Type 0 and it > + * has format of Type 0 config space. > * > - * Driver for this range redirects access to virtual cfgcache[] buffer > - * which avoids changing internal mvebu registers. And changes Header > - * Type response value to Type 1. > + * Moreover Type 0 BAR registers (ranges 0x10 - 0x28 and 0x30 - 0x34) > + * have the same format in Marvell's specification as in PCIe > + * specification, but their meaning is totally different and they do > + * different things: they are aliased into internal mvebu registers > + * (e.g. PCIE_BAR_LO_OFF) and these should not be changed or > + * reconfigured by pci device drivers. > + * > + * So our driver converts Type 0 config space to Type 1 and reports > + * Header Type as Type 1. Access to BAR registers and to non-existent > + * Type 1 registers is redirected to the virtual cfgcache[] buffer, > + * which avoids changing unrelated registers. > */ > reg = readl(pcie->base + PCIE_DEV_REV_OFF); > reg &= ~0xffffff00; > Viele Grüße, Stefan Roese
diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c index b545e62689..701a17dfb7 100644 --- a/drivers/pci/pci_mvebu.c +++ b/drivers/pci/pci_mvebu.c @@ -93,7 +93,7 @@ struct mvebu_pcie { unsigned int mem_attr; unsigned int io_target; unsigned int io_attr; - u32 cfgcache[(0x34 - 0x10) / 4]; + u32 cfgcache[(0x3c - 0x10) / 4]; }; /* @@ -189,20 +189,20 @@ static int mvebu_pcie_read_config(const struct udevice *bus, pci_dev_t bdf, } /* - * mvebu has different internal registers mapped into PCI config space - * in range 0x10-0x34 for PCI bridge, so do not access PCI config space - * for this range and instead read content from driver virtual cfgcache + * The configuration space of the PCI Bridge on primary (first) bus is + * of Type 0 but the BAR registers (including ROM BAR) don't have the + * same meaning as in the PCIe specification. Therefore do not access + * BAR registers and non-common registers (those which have different + * meaning for Type 0 and Type 1 config space) of the PCI Bridge and + * instead read their content from driver virtual cfgcache[]. */ - if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) { + if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) || + (offset >= 0x38 && offset < 0x3c))) { data = pcie->cfgcache[(offset - 0x10) / 4]; debug("(addr,size,val)=(0x%04x, %d, 0x%08x) from cfgcache\n", offset, size, data); *valuep = pci_conv_32_to_size(data, offset, size); return 0; - } else if (busno == pcie->first_busno && - (offset & ~3) == PCI_ROM_ADDRESS1) { - /* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */ - offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF; } /* @@ -269,17 +269,21 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf, } /* - * mvebu has different internal registers mapped into PCI config space - * in range 0x10-0x34 for PCI bridge, so do not access PCI config space - * for this range and instead write content to driver virtual cfgcache + * As explained in mvebu_pcie_read_config(), PCI Bridge Type 1 specific + * config registers are not available, so we write their content only + * into driver virtual cfgcache[]. + * And as explained in mvebu_pcie_probe(), mvebu has its own specific + * way for configuring primary and secondary bus numbers. */ - if (busno == pcie->first_busno && offset >= 0x10 && offset < 0x34) { + if (busno == pcie->first_busno && ((offset >= 0x10 && offset < 0x34) || + (offset >= 0x38 && offset < 0x3c))) { debug("Writing to cfgcache only\n"); data = pcie->cfgcache[(offset - 0x10) / 4]; data = pci_conv_size_to_32(data, value, offset, size); /* mvebu PCI bridge does not have configurable bars */ if ((offset & ~3) == PCI_BASE_ADDRESS_0 || - (offset & ~3) == PCI_BASE_ADDRESS_1) + (offset & ~3) == PCI_BASE_ADDRESS_1 || + (offset & ~3) == PCI_ROM_ADDRESS1) data = 0x0; pcie->cfgcache[(offset - 0x10) / 4] = data; /* mvebu has its own way how to set PCI primary bus number */ @@ -297,10 +301,6 @@ static int mvebu_pcie_write_config(struct udevice *bus, pci_dev_t bdf, pcie->sec_busno); } return 0; - } else if (busno == pcie->first_busno && - (offset & ~3) == PCI_ROM_ADDRESS1) { - /* mvebu has Expansion ROM Base Address (0x38) at offset 0x30 */ - offset -= PCI_ROM_ADDRESS1 - PCIE_EXP_ROM_BAR_OFF; } /* @@ -424,13 +424,20 @@ static int mvebu_pcie_probe(struct udevice *dev) * U-Boot cannot recognize as P2P Bridge. * * Note that this mvebu PCI Bridge does not have compliant Type 1 - * Configuration Space. Header Type is reported as Type 0 and in - * range 0x10-0x34 it has aliased internal mvebu registers 0x10-0x34 - * (e.g. PCIE_BAR_LO_OFF) and register 0x38 is reserved. + * Configuration Space. Header Type is reported as Type 0 and it + * has format of Type 0 config space. * - * Driver for this range redirects access to virtual cfgcache[] buffer - * which avoids changing internal mvebu registers. And changes Header - * Type response value to Type 1. + * Moreover Type 0 BAR registers (ranges 0x10 - 0x28 and 0x30 - 0x34) + * have the same format in Marvell's specification as in PCIe + * specification, but their meaning is totally different and they do + * different things: they are aliased into internal mvebu registers + * (e.g. PCIE_BAR_LO_OFF) and these should not be changed or + * reconfigured by pci device drivers. + * + * So our driver converts Type 0 config space to Type 1 and reports + * Header Type as Type 1. Access to BAR registers and to non-existent + * Type 1 registers is redirected to the virtual cfgcache[] buffer, + * which avoids changing unrelated registers. */ reg = readl(pcie->base + PCIE_DEV_REV_OFF); reg &= ~0xffffff00;