On 21/11/2019 9:07 pm, Jim Quinlan wrote:
[...]
As for [...]_NUM_MASK_BITS I'm looking for a smart/generic way to calculate it
from the actual mask. No luck so far. If not, I think I'll simply leave it as
is for now.
HWEIGHT()?
FYI, What's happening here is that we have to save the CPU address range
(which
is already shifted right 20 positions) in two parts, the lower 12 bits go
into
PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT while the higher 8 bits go into
PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI or
PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI.
The hardware spec require bits 31:20 of the address, and the high registers
require 39:32 right?
Yes, that's it.
(Apologies, the indirection by the WR_FLD_** macros easily confuses me. These
type of macros are helpful, or rather would be if the whole kernel used them.
I think they can add confusion when each driver has its own set of similar
macros. This is why its *really* helpful to use any existing macros in the
kernel - and only invent new ones if needed).
I agree it's pretty confusing, I think v3, using bitfield.h as much as
possible, looks substantially more welcoming.
The reason we use custom macros is because we'd like to keep the
register names the same as the HW declares and our internal tools
support. As you may have noticed, our register names are unusually
long and it is hard to fit a simple read or write field assignment
within 80 columns w/o using custom macros tailored to our register
names' format.
Perhaps Nicolas can pull a rabbit out of a hat and use Linux macros
while keeping our long register names, but if he has to use his own
shorter register names it will become harder for Broadcom developers
to debug this driver.
Regardless of the length of the names, the standard bitfield helpers can
still make things easier to reason about - in this particular case I
think you could lose some boilerplate and indirection with essentially
no change to the readability you're concerned for - compare:
#define REG_NAME ...
#define REG_NAME_FIELD_NAME_MASK ...
#define REG_NAME_FIELD_NAME_SHIFT ...
val = RD_FIELD(base, REG_NAME,
FIELD_NAME);
vs.
#define REG_NAME ...
#define FIELD_NAME ...
reg = bcm_readl(base + REG_NAME);
val = FIELD_GET(FIELD_NAME, reg);
Robin.
