Messages in this thread | | | Date | Thu, 12 Jan 2023 07:36:29 -0800 (PST) | From | matthew.gerlach@linux ... | Subject | Re: [PATCH v10 3/4] fpga: dfl: add basic support for DFHv1 |
| |
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.
Thanks, Matthew Gerlach
> >> And why users/drivers need to care about the alignment of the parameter >> block? >> >>> follows: > > -- > With Best Regards, > Andy Shevchenko > > >
| |