Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Ley Foon Tan]

On Mon, Oct 12, 2015 at 8:03 PM, Arnd Bergmann  wrote:
>
> On Friday 09 October 2015 18:15:40 Bjorn Helgaas wrote:
> >
> > I don't know if this should be a kernel taint, a simple warning in
> > dmesg, or what.  I guess the tainting mechanism is probably too
> > general-purpose for this, and add_taint() doesn't give any dmesg
> > indication.  We wouldn't see the taint unless the problem actually
> > caused an oops or panic.  In this case, I think I want a clue in dmesg
> > so we have a chance of seeing it even if there is no oops.  So
> > probably something like a dev_warn("non-compliant config accesses")
> > would work.
> >
> > You really should double-check with the hardware guys, because it's
> > pretty obvious that the PCI spec requires 1- and 2-byte config
> > accesses to work correctly.  For example, if you read/modify/write to
> > update PCI_COMMAND, you will inadvertently clear the RW1C bits in
> > PCI_STATUS.
>
> Would it help to require a DT property here that flags the device
> as having a broken config space?
>
> Then we could implement both in the driver, and only use the
> RMW based implementation if the firmware describes the device
> as "altera,broken-pci-config-space".
>

I have checked the PCI/TLP specification, the address needs to be
4-byte aligned. But, we can use "byte enable" field to update specific
bytes.
For example, if byte enable is 0x3 (0011b), that mean it only update
lower 2 bytes. By doing this, we can resolve the RW1C issue here.
I will update the driver with this in next revision.

Thanks for reviewing.

Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Ley Foon Tan]

On Mon, Oct 12, 2015 at 8:03 PM, Arnd Bergmann  wrote:
>
> On Friday 09 October 2015 18:15:40 Bjorn Helgaas wrote:
> >
> > I don't know if this should be a kernel taint, a simple warning in
> > dmesg, or what.  I guess the tainting mechanism is probably too
> > general-purpose for this, and add_taint() doesn't give any dmesg
> > indication.  We wouldn't see the taint unless the problem actually
> > caused an oops or panic.  In this case, I think I want a clue in dmesg
> > so we have a chance of seeing it even if there is no oops.  So
> > probably something like a dev_warn("non-compliant config accesses")
> > would work.
> >
> > You really should double-check with the hardware guys, because it's
> > pretty obvious that the PCI spec requires 1- and 2-byte config
> > accesses to work correctly.  For example, if you read/modify/write to
> > update PCI_COMMAND, you will inadvertently clear the RW1C bits in
> > PCI_STATUS.
>
> Would it help to require a DT property here that flags the device
> as having a broken config space?
>
> Then we could implement both in the driver, and only use the
> RMW based implementation if the firmware describes the device
> as "altera,broken-pci-config-space".
>

I have checked the PCI/TLP specification, the address needs to be
4-byte aligned. But, we can use "byte enable" field to update specific
bytes.
For example, if byte enable is 0x3 (0011b), that mean it only update
lower 2 bytes. By doing this, we can resolve the RW1C issue here.
I will update the driver with this in next revision.

Thanks for reviewing.

Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Arnd Bergmann]

On Friday 09 October 2015 18:15:40 Bjorn Helgaas wrote:
> 
> I don't know if this should be a kernel taint, a simple warning in
> dmesg, or what.  I guess the tainting mechanism is probably too
> general-purpose for this, and add_taint() doesn't give any dmesg
> indication.  We wouldn't see the taint unless the problem actually
> caused an oops or panic.  In this case, I think I want a clue in dmesg
> so we have a chance of seeing it even if there is no oops.  So
> probably something like a dev_warn("non-compliant config accesses")
> would work.
> 
> You really should double-check with the hardware guys, because it's
> pretty obvious that the PCI spec requires 1- and 2-byte config
> accesses to work correctly.  For example, if you read/modify/write to
> update PCI_COMMAND, you will inadvertently clear the RW1C bits in
> PCI_STATUS.

Would it help to require a DT property here that flags the device
as having a broken config space?

Then we could implement both in the driver, and only use the
RMW based implementation if the firmware describes the device
as "altera,broken-pci-config-space".

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Arnd Bergmann]

On Friday 09 October 2015 18:15:40 Bjorn Helgaas wrote:
> 
> I don't know if this should be a kernel taint, a simple warning in
> dmesg, or what.  I guess the tainting mechanism is probably too
> general-purpose for this, and add_taint() doesn't give any dmesg
> indication.  We wouldn't see the taint unless the problem actually
> caused an oops or panic.  In this case, I think I want a clue in dmesg
> so we have a chance of seeing it even if there is no oops.  So
> probably something like a dev_warn("non-compliant config accesses")
> would work.
> 
> You really should double-check with the hardware guys, because it's
> pretty obvious that the PCI spec requires 1- and 2-byte config
> accesses to work correctly.  For example, if you read/modify/write to
> update PCI_COMMAND, you will inadvertently clear the RW1C bits in
> PCI_STATUS.

Would it help to require a DT property here that flags the device
as having a broken config space?

Then we could implement both in the driver, and only use the
RMW based implementation if the firmware describes the device
as "altera,broken-pci-config-space".

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Bjorn Helgaas]

On Thu, Oct 08, 2015 at 06:03:24PM +0800, Ley Foon Tan wrote:
> On Thu, Oct 8, 2015 at 5:47 PM, Russell King - ARM Linux
>  wrote:
> >
> > On Thu, Oct 08, 2015 at 05:43:11PM +0800, Ley Foon Tan wrote:
> > > +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
> > > +  int where, int size, u32 value)
> > > +{
> > > + struct altera_pcie *pcie = bus->sysdata;
> > > + u32 data32;
> > > + u32 shift = 8 * (where & 3);
> > > + int ret;
> > > +
> > > + if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
> > > + return PCIBIOS_DEVICE_NOT_FOUND;
> > > +
> > > + /* write partial */
> > > + if (size != sizeof(u32)) {
> > > + ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
> > > +  where & ~DWORD_MASK, );
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + switch (size) {
> > > + case 1:
> > > + data32 = (data32 & ~(0xff << shift)) |
> > > + ((value & 0xff) << shift);
> > > + break;
> > > + case 2:
> > > + data32 = (data32 & ~(0x << shift)) |
> > > + ((value & 0x) << shift);
> > > + break;
> > > + default:
> > > + data32 = value;
> >
> > Can you generate proper 1, 2 and 4 byte configuration accesses?  That
> > is much preferred over the above read-modify-write, as there are
> > registers in PCI and PCIe that are read/write-1-to-clear.  The above
> > has the effect of inadvertently clearing those RW1C bits.
> No, hardware can only access 4-byte aligned address.

This is non-spec compliant, and we really should have some way of
flagging that because it may break things in ways that would be very
difficult to debug, e.g., we can lose RW1C status bits when writing an
adjacent register, so they would just silently disappear.

I don't know if this should be a kernel taint, a simple warning in
dmesg, or what.  I guess the tainting mechanism is probably too
general-purpose for this, and add_taint() doesn't give any dmesg
indication.  We wouldn't see the taint unless the problem actually
caused an oops or panic.  In this case, I think I want a clue in dmesg
so we have a chance of seeing it even if there is no oops.  So
probably something like a dev_warn("non-compliant config accesses")
would work.

You really should double-check with the hardware guys, because it's
pretty obvious that the PCI spec requires 1- and 2-byte config
accesses to work correctly.  For example, if you read/modify/write to
update PCI_COMMAND, you will inadvertently clear the RW1C bits in
PCI_STATUS.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Bjorn Helgaas]

On Thu, Oct 08, 2015 at 06:03:24PM +0800, Ley Foon Tan wrote:
> On Thu, Oct 8, 2015 at 5:47 PM, Russell King - ARM Linux
>  wrote:
> >
> > On Thu, Oct 08, 2015 at 05:43:11PM +0800, Ley Foon Tan wrote:
> > > +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
> > > +  int where, int size, u32 value)
> > > +{
> > > + struct altera_pcie *pcie = bus->sysdata;
> > > + u32 data32;
> > > + u32 shift = 8 * (where & 3);
> > > + int ret;
> > > +
> > > + if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
> > > + return PCIBIOS_DEVICE_NOT_FOUND;
> > > +
> > > + /* write partial */
> > > + if (size != sizeof(u32)) {
> > > + ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
> > > +  where & ~DWORD_MASK, );
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + switch (size) {
> > > + case 1:
> > > + data32 = (data32 & ~(0xff << shift)) |
> > > + ((value & 0xff) << shift);
> > > + break;
> > > + case 2:
> > > + data32 = (data32 & ~(0x << shift)) |
> > > + ((value & 0x) << shift);
> > > + break;
> > > + default:
> > > + data32 = value;
> >
> > Can you generate proper 1, 2 and 4 byte configuration accesses?  That
> > is much preferred over the above read-modify-write, as there are
> > registers in PCI and PCIe that are read/write-1-to-clear.  The above
> > has the effect of inadvertently clearing those RW1C bits.
> No, hardware can only access 4-byte aligned address.

This is non-spec compliant, and we really should have some way of
flagging that because it may break things in ways that would be very
difficult to debug, e.g., we can lose RW1C status bits when writing an
adjacent register, so they would just silently disappear.

I don't know if this should be a kernel taint, a simple warning in
dmesg, or what.  I guess the tainting mechanism is probably too
general-purpose for this, and add_taint() doesn't give any dmesg
indication.  We wouldn't see the taint unless the problem actually
caused an oops or panic.  In this case, I think I want a clue in dmesg
so we have a chance of seeing it even if there is no oops.  So
probably something like a dev_warn("non-compliant config accesses")
would work.

You really should double-check with the hardware guys, because it's
pretty obvious that the PCI spec requires 1- and 2-byte config
accesses to work correctly.  For example, if you read/modify/write to
update PCI_COMMAND, you will inadvertently clear the RW1C bits in
PCI_STATUS.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[kbuild test robot]

Hi Ley,

[auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: sparc-allmodconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc 

All errors (new ones prefixed by >>):

   drivers/pci/host/pcie-altera.c: In function 'tlp_cfg_dword_read':
   drivers/pci/host/pcie-altera.c:243:12: warning: large integer implicitly 
truncated to unsigned type [-Woverflow]
  *value = ~0UL; /* return 0x if error */
   ^
   drivers/pci/host/pcie-altera.c: In function 'altera_pcie_cfg_read':
   drivers/pci/host/pcie-altera.c:291:12: warning: large integer implicitly 
truncated to unsigned type [-Woverflow]
  *value = ~0UL;
   ^
   drivers/pci/host/pcie-altera.c: In function 
'altera_pcie_parse_request_of_pci_ranges':
>> drivers/pci/host/pcie-altera.c:410:2: error: implicit declaration of 
>> function 'of_pci_get_host_bridge_resources' 
>> [-Werror=implicit-function-declaration]
 err = of_pci_get_host_bridge_resources(np, 0, 0xff, >resources,
 ^
   cc1: some warnings being treated as errors

vim +/of_pci_get_host_bridge_resources +410 drivers/pci/host/pcie-altera.c

   237  headers[2] = TLP_CFG_DW2(bus, devfn, where);
   238  
   239  tlp_write_packet(pcie, headers, 0, false);
   240  
   241  ret = tlp_read_packet(pcie, value);
   242  if (ret != PCIBIOS_SUCCESSFUL)
 > 243  *value = ~0UL;  /* return 0x if error */
   244  
   245  return ret;
   246  }
   247  
   248  static int tlp_cfg_dword_write(struct altera_pcie *pcie, u8 bus, u32 
devfn,
   249 int where, u32 value)
   250  {
   251  u32 headers[TLP_HDR_SIZE];
   252  int ret;
   253  
   254  if (bus == pcie->root_bus_nr)
   255  headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR0);
   256  else
   257  headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR1);
   258  
   259  headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
   260  TLP_WRITE_TAG);
   261  headers[2] = TLP_CFG_DW2(bus, devfn, where);
   262  
   263  /* check alignment to Qword */
   264  if ((where & 0x7) == 0)
   265  tlp_write_packet(pcie, headers, value, true);
   266  else
   267  tlp_write_packet(pcie, headers, value, false);
   268  
   269  ret = tlp_read_packet(pcie, NULL);
   270  if (ret != PCIBIOS_SUCCESSFUL)
   271  return ret;
   272  
   273  /*
   274   * Monitoring changes to PCI_PRIMARY_BUS register on root port 
and update
   275   * local copy of root bus number accordingly.
   276   */
   277  if ((bus == pcie->root_bus_nr) && (where == PCI_PRIMARY_BUS))
   278  pcie->root_bus_nr = (u8)(value);
   279  
   280  return PCIBIOS_SUCCESSFUL;
   281  }
   282  
   283  static int altera_pcie_cfg_read(struct pci_bus *bus, unsigned int devfn,
   284  int where, int size, u32 *value)
   285  {
   286  struct altera_pcie *pcie = bus->sysdata;
   287  int ret;
   288  u32 data;
   289  
   290  if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn))) {
   291  *value = ~0UL;
   292  return PCIBIOS_DEVICE_NOT_FOUND;
   293  }
   294  
   295  ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
   296   (where & ~DWORD_MASK), );
   297  if (ret != PCIBIOS_SUCCESSFUL)
   298  return ret;
   299  
   300  switch (size) {
   301  case 1:
   302  *value = (data >> (8 * (where & 0x3))) & 0xff;
   303  break;
   304  case 2:
   305  *value = (data >> (8 * (where & 0x2))) & 0x;
   306  break;
   307  default:
   308  *value = data;
   309  break;
   310  }
   311  
   312  return PCIBIOS_SUCCESSFUL;
   313  }
   314  
   315  static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int 
devfn,
   316   int where, int size, u32 value)
   317  {
   318  struct altera_pcie *pcie = bus->sysdata;
   319  u32 data32;
   320  u32 shift = 8 * (where & 3);
   321  int ret;
   322  
   323  if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
   324  return PCIBIOS_DEVICE_NOT_FOUND;
   325  
   326  /* write partial */
   327  if (size != sizeof(u32)) {
   328  ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
 

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[kbuild test robot]

Hi Ley,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please 
ignore]

config: x86_64-allmodconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/pci/host/pcie-altera.c: In function 'tlp_cfg_dword_read':
>> drivers/pci/host/pcie-altera.c:243:12: warning: large integer implicitly 
>> truncated to unsigned type [-Woverflow]
  *value = ~0UL; /* return 0x if error */
   ^
   drivers/pci/host/pcie-altera.c: In function 'altera_pcie_cfg_read':
   drivers/pci/host/pcie-altera.c:291:12: warning: large integer implicitly 
truncated to unsigned type [-Woverflow]
  *value = ~0UL;
   ^

vim +243 drivers/pci/host/pcie-altera.c

   227  int ret;
   228  u32 headers[TLP_HDR_SIZE];
   229  
   230  if (bus == pcie->root_bus_nr)
   231  headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD0);
   232  else
   233  headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD1);
   234  
   235  headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
   236  TLP_READ_TAG);
   237  headers[2] = TLP_CFG_DW2(bus, devfn, where);
   238  
   239  tlp_write_packet(pcie, headers, 0, false);
   240  
   241  ret = tlp_read_packet(pcie, value);
   242  if (ret != PCIBIOS_SUCCESSFUL)
 > 243  *value = ~0UL;  /* return 0x if error */
   244  
   245  return ret;
   246  }
   247  
   248  static int tlp_cfg_dword_write(struct altera_pcie *pcie, u8 bus, u32 
devfn,
   249 int where, u32 value)
   250  {
   251  u32 headers[TLP_HDR_SIZE];

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Ley Foon Tan]

On Thu, Oct 8, 2015 at 5:47 PM, Russell King - ARM Linux
 wrote:
>
> On Thu, Oct 08, 2015 at 05:43:11PM +0800, Ley Foon Tan wrote:
> > +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
> > +  int where, int size, u32 value)
> > +{
> > + struct altera_pcie *pcie = bus->sysdata;
> > + u32 data32;
> > + u32 shift = 8 * (where & 3);
> > + int ret;
> > +
> > + if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
> > + return PCIBIOS_DEVICE_NOT_FOUND;
> > +
> > + /* write partial */
> > + if (size != sizeof(u32)) {
> > + ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
> > +  where & ~DWORD_MASK, );
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + switch (size) {
> > + case 1:
> > + data32 = (data32 & ~(0xff << shift)) |
> > + ((value & 0xff) << shift);
> > + break;
> > + case 2:
> > + data32 = (data32 & ~(0x << shift)) |
> > + ((value & 0x) << shift);
> > + break;
> > + default:
> > + data32 = value;
>
> Can you generate proper 1, 2 and 4 byte configuration accesses?  That
> is much preferred over the above read-modify-write, as there are
> registers in PCI and PCIe that are read/write-1-to-clear.  The above
> has the effect of inadvertently clearing those RW1C bits.
No, hardware can only access 4-byte aligned address.

Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Russell King - ARM Linux]

On Thu, Oct 08, 2015 at 05:43:11PM +0800, Ley Foon Tan wrote:
> +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
> +  int where, int size, u32 value)
> +{
> + struct altera_pcie *pcie = bus->sysdata;
> + u32 data32;
> + u32 shift = 8 * (where & 3);
> + int ret;
> +
> + if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
> + return PCIBIOS_DEVICE_NOT_FOUND;
> +
> + /* write partial */
> + if (size != sizeof(u32)) {
> + ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
> +  where & ~DWORD_MASK, );
> + if (ret)
> + return ret;
> + }
> +
> + switch (size) {
> + case 1:
> + data32 = (data32 & ~(0xff << shift)) |
> + ((value & 0xff) << shift);
> + break;
> + case 2:
> + data32 = (data32 & ~(0x << shift)) |
> + ((value & 0x) << shift);
> + break;
> + default:
> + data32 = value;

Can you generate proper 1, 2 and 4 byte configuration accesses?  That
is much preferred over the above read-modify-write, as there are
registers in PCI and PCIe that are read/write-1-to-clear.  The above
has the effect of inadvertently clearing those RW1C bits.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Ley Foon Tan]

This patch adds the Altera PCIe host controller driver.

Signed-off-by: Ley Foon Tan 
Reviewed-by: Marc Zyngier 
---
 drivers/pci/host/Kconfig   |   7 +
 drivers/pci/host/Makefile  |   1 +
 drivers/pci/host/pcie-altera.c | 588 +
 3 files changed, 596 insertions(+)
 create mode 100644 drivers/pci/host/pcie-altera.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index d5e58ba..08f2543 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -145,4 +145,11 @@ config PCIE_IPROC_BCMA
  Say Y here if you want to use the Broadcom iProc PCIe controller
  through the BCMA bus interface
 
+config PCIE_ALTERA
+   bool "Altera PCIe controller"
+   select PCI_DOMAINS
+   help
+ Say Y here if you want to enable PCIe controller support for Altera
+ SoCFPGA family of SoCs.
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 140d66f..6954f76 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
 obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
 obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
 obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
+obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
new file mode 100644
index 000..8554ff9
--- /dev/null
+++ b/drivers/pci/host/pcie-altera.c
@@ -0,0 +1,588 @@
+/*
+ * Copyright Altera Corporation (C) 2013-2015. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define A2P_ADDR_MAP_LO0   0x1000
+#define A2P_ADDR_MAP_HI0   0x1004
+#define RP_TX_REG0 0x2000
+#define RP_TX_REG1 0x2004
+#define RP_TX_CNTRL0x2008
+#define RP_TX_EOP  0x2
+#define RP_TX_SOP  0x1
+#define RP_RXCPL_STATUS0x2010
+#define RP_RXCPL_EOP   0x2
+#define RP_RXCPL_SOP   0x1
+#define RP_RXCPL_REG0  0x2014
+#define RP_RXCPL_REG1  0x2018
+#define P2A_INT_STATUS 0x3060
+#define P2A_INT_STS_ALL0xF
+#define P2A_INT_ENABLE 0x3070
+#define P2A_INT_ENA_ALL0xF
+#define RP_LTSSM   0x3C64
+#define LTSSM_L0   0xF
+
+/* TLP configuration type 0 and 1 */
+#define TLP_FMTTYPE_CFGRD0 0x04/* Configuration Read Type 0 */
+#define TLP_FMTTYPE_CFGWR0 0x44/* Configuration Write Type 0 */
+#define TLP_FMTTYPE_CFGRD1 0x05/* Configuration Read Type 1 */
+#define TLP_FMTTYPE_CFGWR1 0x45/* Configuration Write Type 1 */
+#define TLP_PAYLOAD_SIZE   0x01
+#define TLP_READ_TAG   0x1D
+#define TLP_WRITE_TAG  0x10
+#define TLP_CFG_DW0(fmttype)   (((fmttype) << 24) | TLP_PAYLOAD_SIZE)
+#define TLP_CFG_DW1(reqid, tag)(((reqid) << 16) | (tag << 8) | 
0xF)
+#define TLP_CFG_DW2(bus, devfn, offset)\
+   (((bus) << 24) | ((devfn) << 16) | (offset))
+#define TLP_REQ_ID(bus, devfn) (((bus) << 8) | (devfn))
+#define TLP_COMPL_STATUS(hdr)  (((hdr) & 0xE0) >> 13)
+#define TLP_HDR_SIZE   3
+#define TLP_LOOP   500
+
+#define INTX_NUM   4
+
+#define DWORD_MASK 3
+
+struct altera_pcie {
+   struct platform_device  *pdev;
+   void __iomem*cra_base;
+   int irq;
+   u8  root_bus_nr;
+   struct irq_domain   *irq_domain;
+   struct resource bus_range;
+   struct list_headresources;
+};
+
+struct tlp_rp_regpair_t {
+   u32 ctrl;
+   u32 reg0;
+   u32 reg1;
+};
+
+static void altera_pcie_retrain(struct pci_dev *dev)
+{
+   u16 linkcap, linkstat;
+
+   /*
+* Set the retrain bit if the PCIe rootport support > 2.5GB/s, but
+* current speed is 2.5 GB/s.
+*/
+   pcie_capability_read_word(dev, PCI_EXP_LNKCAP, );
+
+   if ((linkcap & PCI_EXP_LNKCAP_SLS) <= 

[PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Ley Foon Tan]

This patch adds the Altera PCIe host controller driver.

Signed-off-by: Ley Foon Tan 
Reviewed-by: Marc Zyngier 
---
 drivers/pci/host/Kconfig   |   7 +
 drivers/pci/host/Makefile  |   1 +
 drivers/pci/host/pcie-altera.c | 588 +
 3 files changed, 596 insertions(+)
 create mode 100644 drivers/pci/host/pcie-altera.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index d5e58ba..08f2543 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -145,4 +145,11 @@ config PCIE_IPROC_BCMA
  Say Y here if you want to use the Broadcom iProc PCIe controller
  through the BCMA bus interface
 
+config PCIE_ALTERA
+   bool "Altera PCIe controller"
+   select PCI_DOMAINS
+   help
+ Say Y here if you want to enable PCIe controller support for Altera
+ SoCFPGA family of SoCs.
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 140d66f..6954f76 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
 obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
 obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
 obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
+obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
new file mode 100644
index 000..8554ff9
--- /dev/null
+++ b/drivers/pci/host/pcie-altera.c
@@ -0,0 +1,588 @@
+/*
+ * Copyright Altera Corporation (C) 2013-2015. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define A2P_ADDR_MAP_LO0   0x1000
+#define A2P_ADDR_MAP_HI0   0x1004
+#define RP_TX_REG0 0x2000
+#define RP_TX_REG1 0x2004
+#define RP_TX_CNTRL0x2008
+#define RP_TX_EOP  0x2
+#define RP_TX_SOP  0x1
+#define RP_RXCPL_STATUS0x2010
+#define RP_RXCPL_EOP   0x2
+#define RP_RXCPL_SOP   0x1
+#define RP_RXCPL_REG0  0x2014
+#define RP_RXCPL_REG1  0x2018
+#define P2A_INT_STATUS 0x3060
+#define P2A_INT_STS_ALL0xF
+#define P2A_INT_ENABLE 0x3070
+#define P2A_INT_ENA_ALL0xF
+#define RP_LTSSM   0x3C64
+#define LTSSM_L0   0xF
+
+/* TLP configuration type 0 and 1 */
+#define TLP_FMTTYPE_CFGRD0 0x04/* Configuration Read Type 0 */
+#define TLP_FMTTYPE_CFGWR0 0x44/* Configuration Write Type 0 */
+#define TLP_FMTTYPE_CFGRD1 0x05/* Configuration Read Type 1 */
+#define TLP_FMTTYPE_CFGWR1 0x45/* Configuration Write Type 1 */
+#define TLP_PAYLOAD_SIZE   0x01
+#define TLP_READ_TAG   0x1D
+#define TLP_WRITE_TAG  0x10
+#define TLP_CFG_DW0(fmttype)   (((fmttype) << 24) | TLP_PAYLOAD_SIZE)
+#define TLP_CFG_DW1(reqid, tag)(((reqid) << 16) | (tag << 8) | 
0xF)
+#define TLP_CFG_DW2(bus, devfn, offset)\
+   (((bus) << 24) | ((devfn) << 16) | (offset))
+#define TLP_REQ_ID(bus, devfn) (((bus) << 8) | (devfn))
+#define TLP_COMPL_STATUS(hdr)  (((hdr) & 0xE0) >> 13)
+#define TLP_HDR_SIZE   3
+#define TLP_LOOP   500
+
+#define INTX_NUM   4
+
+#define DWORD_MASK 3
+
+struct altera_pcie {
+   struct platform_device  *pdev;
+   void __iomem*cra_base;
+   int irq;
+   u8  root_bus_nr;
+   struct irq_domain   *irq_domain;
+   struct resource bus_range;
+   struct list_headresources;
+};
+
+struct tlp_rp_regpair_t {
+   u32 ctrl;
+   u32 reg0;
+   u32 reg1;
+};
+
+static void altera_pcie_retrain(struct pci_dev *dev)
+{
+   u16 linkcap, linkstat;
+
+   /*
+* Set the retrain bit if the PCIe rootport support > 2.5GB/s, but
+* current speed is 2.5 GB/s.
+*/
+   pcie_capability_read_word(dev, PCI_EXP_LNKCAP, );
+
+   if 

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Russell King - ARM Linux]

On Thu, Oct 08, 2015 at 05:43:11PM +0800, Ley Foon Tan wrote:
> +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
> +  int where, int size, u32 value)
> +{
> + struct altera_pcie *pcie = bus->sysdata;
> + u32 data32;
> + u32 shift = 8 * (where & 3);
> + int ret;
> +
> + if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
> + return PCIBIOS_DEVICE_NOT_FOUND;
> +
> + /* write partial */
> + if (size != sizeof(u32)) {
> + ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
> +  where & ~DWORD_MASK, );
> + if (ret)
> + return ret;
> + }
> +
> + switch (size) {
> + case 1:
> + data32 = (data32 & ~(0xff << shift)) |
> + ((value & 0xff) << shift);
> + break;
> + case 2:
> + data32 = (data32 & ~(0x << shift)) |
> + ((value & 0x) << shift);
> + break;
> + default:
> + data32 = value;

Can you generate proper 1, 2 and 4 byte configuration accesses?  That
is much preferred over the above read-modify-write, as there are
registers in PCI and PCIe that are read/write-1-to-clear.  The above
has the effect of inadvertently clearing those RW1C bits.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[Ley Foon Tan]

On Thu, Oct 8, 2015 at 5:47 PM, Russell King - ARM Linux
 wrote:
>
> On Thu, Oct 08, 2015 at 05:43:11PM +0800, Ley Foon Tan wrote:
> > +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
> > +  int where, int size, u32 value)
> > +{
> > + struct altera_pcie *pcie = bus->sysdata;
> > + u32 data32;
> > + u32 shift = 8 * (where & 3);
> > + int ret;
> > +
> > + if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
> > + return PCIBIOS_DEVICE_NOT_FOUND;
> > +
> > + /* write partial */
> > + if (size != sizeof(u32)) {
> > + ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
> > +  where & ~DWORD_MASK, );
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + switch (size) {
> > + case 1:
> > + data32 = (data32 & ~(0xff << shift)) |
> > + ((value & 0xff) << shift);
> > + break;
> > + case 2:
> > + data32 = (data32 & ~(0x << shift)) |
> > + ((value & 0x) << shift);
> > + break;
> > + default:
> > + data32 = value;
>
> Can you generate proper 1, 2 and 4 byte configuration accesses?  That
> is much preferred over the above read-modify-write, as there are
> registers in PCI and PCIe that are read/write-1-to-clear.  The above
> has the effect of inadvertently clearing those RW1C bits.
No, hardware can only access 4-byte aligned address.

Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[kbuild test robot]

Hi Ley,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please 
ignore]

config: x86_64-allmodconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/pci/host/pcie-altera.c: In function 'tlp_cfg_dword_read':
>> drivers/pci/host/pcie-altera.c:243:12: warning: large integer implicitly 
>> truncated to unsigned type [-Woverflow]
  *value = ~0UL; /* return 0x if error */
   ^
   drivers/pci/host/pcie-altera.c: In function 'altera_pcie_cfg_read':
   drivers/pci/host/pcie-altera.c:291:12: warning: large integer implicitly 
truncated to unsigned type [-Woverflow]
  *value = ~0UL;
   ^

vim +243 drivers/pci/host/pcie-altera.c

   227  int ret;
   228  u32 headers[TLP_HDR_SIZE];
   229  
   230  if (bus == pcie->root_bus_nr)
   231  headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD0);
   232  else
   233  headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD1);
   234  
   235  headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
   236  TLP_READ_TAG);
   237  headers[2] = TLP_CFG_DW2(bus, devfn, where);
   238  
   239  tlp_write_packet(pcie, headers, 0, false);
   240  
   241  ret = tlp_read_packet(pcie, value);
   242  if (ret != PCIBIOS_SUCCESSFUL)
 > 243  *value = ~0UL;  /* return 0x if error */
   244  
   245  return ret;
   246  }
   247  
   248  static int tlp_cfg_dword_write(struct altera_pcie *pcie, u8 bus, u32 
devfn,
   249 int where, u32 value)
   250  {
   251  u32 headers[TLP_HDR_SIZE];

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH v8 3/6] pci:host: Add Altera PCIe host controller driver

[kbuild test robot]

Hi Ley,

[auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: sparc-allmodconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc 

All errors (new ones prefixed by >>):

   drivers/pci/host/pcie-altera.c: In function 'tlp_cfg_dword_read':
   drivers/pci/host/pcie-altera.c:243:12: warning: large integer implicitly 
truncated to unsigned type [-Woverflow]
  *value = ~0UL; /* return 0x if error */
   ^
   drivers/pci/host/pcie-altera.c: In function 'altera_pcie_cfg_read':
   drivers/pci/host/pcie-altera.c:291:12: warning: large integer implicitly 
truncated to unsigned type [-Woverflow]
  *value = ~0UL;
   ^
   drivers/pci/host/pcie-altera.c: In function 
'altera_pcie_parse_request_of_pci_ranges':
>> drivers/pci/host/pcie-altera.c:410:2: error: implicit declaration of 
>> function 'of_pci_get_host_bridge_resources' 
>> [-Werror=implicit-function-declaration]
 err = of_pci_get_host_bridge_resources(np, 0, 0xff, >resources,
 ^
   cc1: some warnings being treated as errors

vim +/of_pci_get_host_bridge_resources +410 drivers/pci/host/pcie-altera.c

   237  headers[2] = TLP_CFG_DW2(bus, devfn, where);
   238  
   239  tlp_write_packet(pcie, headers, 0, false);
   240  
   241  ret = tlp_read_packet(pcie, value);
   242  if (ret != PCIBIOS_SUCCESSFUL)
 > 243  *value = ~0UL;  /* return 0x if error */
   244  
   245  return ret;
   246  }
   247  
   248  static int tlp_cfg_dword_write(struct altera_pcie *pcie, u8 bus, u32 
devfn,
   249 int where, u32 value)
   250  {
   251  u32 headers[TLP_HDR_SIZE];
   252  int ret;
   253  
   254  if (bus == pcie->root_bus_nr)
   255  headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR0);
   256  else
   257  headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR1);
   258  
   259  headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
   260  TLP_WRITE_TAG);
   261  headers[2] = TLP_CFG_DW2(bus, devfn, where);
   262  
   263  /* check alignment to Qword */
   264  if ((where & 0x7) == 0)
   265  tlp_write_packet(pcie, headers, value, true);
   266  else
   267  tlp_write_packet(pcie, headers, value, false);
   268  
   269  ret = tlp_read_packet(pcie, NULL);
   270  if (ret != PCIBIOS_SUCCESSFUL)
   271  return ret;
   272  
   273  /*
   274   * Monitoring changes to PCI_PRIMARY_BUS register on root port 
and update
   275   * local copy of root bus number accordingly.
   276   */
   277  if ((bus == pcie->root_bus_nr) && (where == PCI_PRIMARY_BUS))
   278  pcie->root_bus_nr = (u8)(value);
   279  
   280  return PCIBIOS_SUCCESSFUL;
   281  }
   282  
   283  static int altera_pcie_cfg_read(struct pci_bus *bus, unsigned int devfn,
   284  int where, int size, u32 *value)
   285  {
   286  struct altera_pcie *pcie = bus->sysdata;
   287  int ret;
   288  u32 data;
   289  
   290  if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn))) {
   291  *value = ~0UL;
   292  return PCIBIOS_DEVICE_NOT_FOUND;
   293  }
   294  
   295  ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
   296   (where & ~DWORD_MASK), );
   297  if (ret != PCIBIOS_SUCCESSFUL)
   298  return ret;
   299  
   300  switch (size) {
   301  case 1:
   302  *value = (data >> (8 * (where & 0x3))) & 0xff;
   303  break;
   304  case 2:
   305  *value = (data >> (8 * (where & 0x2))) & 0x;
   306  break;
   307  default:
   308  *value = data;
   309  break;
   310  }
   311  
   312  return PCIBIOS_SUCCESSFUL;
   313  }
   314  
   315  static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int 
devfn,
   316   int where, int size, u32 value)
   317  {
   318  struct altera_pcie *pcie = bus->sysdata;
   319  u32 data32;
   320  u32 shift = 8 * (where & 3);
   321  int ret;
   322  
   323  if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
   324  return PCIBIOS_DEVICE_NOT_FOUND;
   325  
   326  /* write partial */
   327  if (size != sizeof(u32)) {
   328  ret = tlp_cfg_dword_read(pcie, bus->number, devfn,