Messages in this thread | | | Date | Mon, 24 Oct 2022 08:09:34 -0700 (PDT) | From | matthew.gerlach@linux ... | Subject | Re: [PATCH v4 3/4] fpga: dfl: add basic support DFHv1 |
| |
On Fri, 21 Oct 2022, Ilpo Järvinen wrote:
> On Thu, 20 Oct 2022, matthew.gerlach@linux.intel.com wrote: > >> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> >> Add generic support for MSI-X interrupts for DFL devices. >> >> The location of a feature's registers is explicitly >> described in DFHv1 and can be relative to the base of the DFHv1 >> or an absolute address. Parse the location and pass the information >> to DFL driver. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> --- >> v4: s/MSIX/MSI_X >> move kernel doc to implementation >> use structure assignment >> fix decode of absolute address >> clean up comment in parse_feature_irqs >> remove use of csr_res >> >> v3: remove unneeded blank line >> use clearer variable name >> pass finfo into parse_feature_irqs() >> refactor code for better indentation >> use switch statement for irq parsing >> squash in code parsing register location >> >> v2: fix kernel doc >> clarify use of DFH_VERSION field >> --- > >> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max) >> +{ >> + int size = 0; >> + u64 v, next; >> + >> + if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, >> + readq(dfh_base + DFHv1_CSR_SIZE_GRP))) >> + return 0; >> + >> + while (size + DFHv1_PARAM_HDR < max) { >> + v = readq(dfh_base + DFHv1_PARAM_HDR + size); >> + >> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v); >> + if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK)) > > In general, try to not use inverse logic for defining masks. However here, > just change DFHv1_PARAM_HDR_NEXT_OFFSET to not include any extra bits > (no rsvd nor eop) and you no longer need this extra masking.
I agree that defining the fields better and using FIELD_GET would make this code cleaner.
> >> + return -EINVAL; >> + >> + size += next & ~DFHv1_PARAM_HDR_NEXT_MASK; > > ...Then you can drop this anding too. > >> + >> + if (next & DFHv1_PARAM_HDR_NEXT_EOL) > > Your docs say EOP, but here you use EOL.
Thanks for catching the inconsistency.
> > Change DFHv1_PARAM_HDR_NEXT_EOL such that this is extracted directly from > v. > > -- > i. >
| |