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 > <linux@xxxxxxxxxxxxxxxx> 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, &data32); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + switch (size) { > > > + case 1: > > > + data32 = (data32 & ~(0xff << shift)) | > > > + ((value & 0xff) << shift); > > > + break; > > > + case 2: > > > + data32 = (data32 & ~(0xffff << shift)) | > > > + ((value & 0xffff) << 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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html