On 03/05/2016 07:54 AM, Yury Norov wrote:
[...]
+static u32 thunder_pem_bridge_w1c_bits(int where)
+{
+ u32 w1c_bits = 0;
+
+ switch (where & ~3) {
+ case 0x04: /* Command/Status */
+ case 0x1c: /* Base and I/O Limit/Secondary Status */
+ w1c_bits = 0xff000000;
+ break;
+ case 0x44: /* Power Management Control and Status */
+ w1c_bits = 0xfffffe00;
+ break;
+ case 0x78: /* Device Control/Device Status */
+ case 0x80: /* Link Control/Link Status */
+ case 0x88: /* Slot Control/Slot Status */
+ case 0x90: /* Root Status */
+ case 0xa0: /* Link Control 2 Registers/Link Status 2 */
+ w1c_bits = 0xffff0000;
+ break;
+ case 0x104: /* Uncorrectable Error Status */
+ case 0x110: /* Correctable Error Status */
+ case 0x130: /* Error Status */
+ case 0x160: /* Link Control 4 */
This patchset is full of magic numbers.
Yes. Did you notice that there is a comment with each one describing
what it is, or what it is doing?
For better readability
I disagree with that.
Doing a:
#define CN8890_PASS1_LINK_CONTROL_4_CONFIG_SPACE_OFFSET 0x160
and then later using the symbol adds clutter. The current code is
compact, and *more* readable than scattering the information across
multiple sites in the file.
and portability,
The whole point of this file is that we are fixing up accesses for a
very small and tightly constrained set of systems. It is not a general
purpose PCI root complex with standard bridges that will be used by
multiple vendors and architectures. Portability is not a big concern.
it's better to declare all that as macro:
#define LINK_CONTROL_4 0x160
If there's some specific reason to use numbers, I think, it should be
explained.
I had hoped that the changlog for the commit combined with the comments
in the file would be sufficient.
I try to explain in this e-mail my thoughts on some of the stylistic
choices I made while writing the code, but I don't plan on including
them into the patch itself.
+ w1c_bits = 0xffffffff;
+ break;
+ default:
+ break;
+ }
+ return w1c_bits;
+}
+
+static int thunder_pem_bridge_write(struct pci_bus *bus, unsigned int devfn,
+ int where, int size, u32 val)
+{
+ struct gen_pci *pci = bus->sysdata;
+ struct thunder_pem_pci *pem_pci;
+ u64 write_val, read_val;
+ u32 mask = 0;
+
+ pem_pci = container_of(pci, struct thunder_pem_pci, gen_pci);
+
+ if (devfn != 0 || where >= 2048)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+ /*
+ * 32-bit accesses only. If the write is for a size smaller
+ * than 32-bits, we must first read the 32-bit value and merge
+ * in the desired bits and then write the whole 32-bits back
+ * out.
+ */
+ switch (size) {
+ case 1:
+ read_val = where & ~3ull;
+ writeq(read_val, pem_pci->pem_reg_base + PEM_CFG_RD);
+ read_val = readq(pem_pci->pem_reg_base + PEM_CFG_RD);
+ read_val >>= 32;
+ mask = ~(0xff << (8 * (where & 3)));
I'm pretty sure, there's no any rocket science, but it's hard to
understand what happens here. What is 8? Bits in byte? Bytes in word?
Anything PCI-specific? Moreover, you repeat this line several times
here (though little modified). Maybe it deserves to be wrapped by some
explaining macro?
I tried to explain this in the comment above the switch statement.
I doubt breaking the masking operations out into out-of-line functions
would add to the readability.
+ read_val &= mask;
+ val = (val & 0xff) << (8 * (where & 3));
+ val |= (u32)read_val;
+ break;
+ case 2:
Case 1 and 2 are looking very similar. Is it possible to wrap them?
For now it looks like code duplication.
They are indeed similar, differing only in mask width.
If Bjorn insists, we could probably factor some of this code out into a
separate function. Personally, I don't think it is worthwhile, as doing
so would probably increase the number of lines in the file.
+ read_val = where & ~3ull;
+ writeq(read_val, pem_pci->pem_reg_base + PEM_CFG_RD);
+ read_val = readq(pem_pci->pem_reg_base + PEM_CFG_RD);
+ read_val >>= 32;
+ mask = ~(0xffff << (8 * (where & 3)));
+ read_val &= mask;
+ val = (val & 0xffff) << (8 * (where & 3));
+ val |= (u32)read_val;
+ break;
+ default:
+ break;
+ }
+
+ /*
+ * By expanding the write width to 32 bits, we may
+ * inadvertently hit some W1C bits that were not intended to
+ * be written. Calculate the mask that must be applied to the
+ * data to be written to avoid these cases.
+ */
+ if (mask) {
+ u32 w1c_bits = thunder_pem_bridge_w1c_bits(where);
+
+ if (w1c_bits) {
+ mask &= w1c_bits;
+ val &= ~mask;
+ }
+ }
+
+ /*
+ * Low order bits are the config address, the high order 32
+ * bits are the data to be written.
+ */
+ write_val = where & ~3ull;
+ write_val |= (((u64)val) << 32);
+ writeq(write_val, pem_pci->pem_reg_base + PEM_CFG_WR);
+ return PCIBIOS_SUCCESSFUL;
+}
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
