Re: [PATCH v7 2/3] pci, pci-thunder-pem: Add PCIe host driver for ThunderX processors.

[David Daney]

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;
> > +}
[...]