Message-ID: <alpine.DEB.2.22.394.2301131056020.1326035@rhweight-WRK1>
Date:   Fri, 13 Jan 2023 11:05:06 -0800 (PST)
From:   matthew.gerlach@...ux.intel.com
To:     Xu Yilun <yilun.xu@...el.com>
cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        hao.wu@...el.com, russell.h.weight@...el.com,
        basheer.ahmed.muddebihal@...el.com, trix@...hat.com,
        mdf@...nel.org, linux-fpga@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        tianfei.zhang@...el.com, corbet@....net,
        gregkh@...uxfoundation.org, linux-serial@...r.kernel.org,
        jirislaby@...nel.org, geert+renesas@...der.be,
        niklas.soderlund+renesas@...natech.se, macro@...am.me.uk,
        johan@...nel.org, lukas@...ner.de, ilpo.jarvinen@...ux.intel.com,
        marpagan@...hat.com, bagasdotme@...il.com
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@...ux.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@...ux.intel.com wrote:
>>>>> On Tue, 10 Jan 2023, Andy Shevchenko wrote:
>>>>>> On Mon, Jan 09, 2023 at 04:30:28PM -0800, matthew.gerlach@...ux.intel.com wrote:
>>>>>>> From: Matthew Gerlach <matthew.gerlach@...ux.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
>>>
>>>
>>>
>

Powered by blists - more mailing lists