lkml.org 
[lkml]   [2023]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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
>>>
>>>
>>>
>

\
 
 \ /
  Last update: 2023-03-26 23:41    [W:0.048 / U:1.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site