Messages in this thread | | | Date | Fri, 13 Jan 2023 11:05:06 -0800 (PST) | From | matthew.gerlach@linux ... | Subject | Re: [PATCH v10 3/4] fpga: dfl: add basic support for DFHv1 |
| |
On Fri, 13 Jan 2023, Xu Yilun wrote:
> On 2023-01-12 at 07:36:29 -0800, matthew.gerlach@linux.intel.com wrote: >> >> >> On Thu, 12 Jan 2023, Andy Shevchenko wrote: >> >>> On Wed, Jan 11, 2023 at 10:13:31AM +0800, Xu Yilun wrote: >>>> On 2023-01-10 at 14:07:16 -0800, matthew.gerlach@linux.intel.com wrote: >>>>> On Tue, 10 Jan 2023, Andy Shevchenko wrote: >>>>>> On Mon, Jan 09, 2023 at 04:30:28PM -0800, matthew.gerlach@linux.intel.com wrote: >>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>> >>> ... >>> >>>>>>> v10: change dfh_find_param to return size of parameter data in bytes >>>>>> >>>>>> The problem that might occur with this approach is byte ordering. >>>>>> When we have u64 items, we know that they all are placed in CPU >>>>>> ordering by the bottom layer. What's the contract now? Can it be >>>>>> a problematic? Please double check this (always keep in mind BE32 >>>>>> as most interesting case for u64/unsigned long representation and >>>>>> other possible byte ordering outcomes). >>>>> >>>>> A number of u64 items certainly states explicit alignment of the memory, but >>>>> I think byte ordering is a different issue. >>>>> >>>>> The bottom layer, by design, is still enforcing a number u64 items under the >>>>> hood. So the contract has not changed. Changing units of size from u64s to >>>>> bytes was suggested to match the general practice of size of memory being in >>>>> bytes. I think the suggestion was made because the return type for >>>>> dfh_find_param() changed from u64* to void* in version 9, when indirectly >>>>> returning the size of the parameter data was introduced. So a void * with a >>>>> size in bytes makes sense. On the other hand, returning a u64 * is a more >>>>> precise reflection of the data alignment. I think the API should be as >>>> >>>> I prefer (void *) + bytes. The properties in the parameter block are not >>>> guarateed to be u64 for each, e.g. the REG_LAYOUT, so (void *) could better >>>> indicate it is not. It is just a block of data unknown to DFL core and to >>>> be parsed by drivers. >>> >>> If the hardware / protocol is capable of communicating the arbitrary lengths >>> of parameters, then yes, bytes make sense. But this should be clear what byte >>> ordering is there if the items can be words / dwords / qwords. >> >> The hardware does communicate the arbitrary lengths of the parameter data; >> so bytes make sense. I will update Documentation/fpga/dfl.rst to explicitly >> say that multi-byte quantities are little-endian. >> >>> >>> TL;DR: The Q is: Is the parameter block a byte stream? If yes, then your >>> proposal is okay. If no, no void * should be used. In the latter it should >>> be union of possible items or a like as defined by a protocol. >> >> The parameter block is not a byte stream; so void * should be used. > > Mm.. I think Andy's idea is, if the parameter block is not a byte stream, > void * should NOT be used. > > My understanding is, The parameter block is not a byte stream in HW, it is > some items (or properties) of various lengths. They are compacted in the > parameter block. But the layout is not generally defined, each parameter > block could have its own layout.
Your understanding is correct that the parameter block is a set of items (or properties) of variouse lengths in HW. The parameter blocks are comparable to PCI capabilities in PCI config space. Each capability has its own defined stucture.
> > The definition and layout of the parameter block is specific to each device, > that is, people design the parameter block for the device when they design > the device. So DFL core doesn't try to generalize all the layouts, they > are unlimited. DFL core just see it as a block of untouched data to be parsed > by each driver. So from DFL core's perspective, it is a byte stream.
Yes, from the DFL core's perspective, the parameter blocks are opaque chunks of data. This would affirm your preference of using (void *) and byte size in the API for the function, dfh_find_param.
Thanks, Matthew Gerlach
> Thanks, > Yilun > >> >> Thanks, >> Matthew Gerlach >> >> >>> >>>> And why users/drivers need to care about the alignment of the parameter >>>> block? >>>> >>>>> follows: >>> >>> -- >>> With Best Regards, >>> Andy Shevchenko >>> >>> >>> >
| |