Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 13 Jan 2023 10:22:39 +0800
From:   Xu Yilun <yilun.xu@intel.com>
To:     matthew.gerlach@linux.intel.com
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        hao.wu@intel.com, russell.h.weight@intel.com,
        basheer.ahmed.muddebihal@intel.com, trix@redhat.com,
        mdf@kernel.org, linux-fpga@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        tianfei.zhang@intel.com, corbet@lwn.net,
        gregkh@linuxfoundation.org, linux-serial@vger.kernel.org,
        jirislaby@kernel.org, geert+renesas@glider.be,
        niklas.soderlund+renesas@ragnatech.se, macro@orcam.me.uk,
        johan@kernel.org, lukas@wunner.de, ilpo.jarvinen@linux.intel.com,
        marpagan@redhat.com, bagasdotme@gmail.com
Subject: Re: [PATCH v10 3/4] fpga: dfl: add basic support for DFHv1
Message-ID: <Y8DAb3R3bP/M7f0v@yilunxu-OptiPlex-7050>
References: <20230110003029.806022-1-matthew.gerlach@linux.intel.com>
 <20230110003029.806022-4-matthew.gerlach@linux.intel.com>
 <Y708L2rRc1RDVkui@smile.fi.intel.com>
 <alpine.DEB.2.22.394.2301101310150.815911@rhweight-WRK1>
 <Y74bSzUBLYH4cLDh@yilunxu-OptiPlex-7050>
 <Y7/ggajPS2WNrPPU@smile.fi.intel.com>
 <alpine.DEB.2.22.394.2301120732500.845139@rhweight-WRK1>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.22.394.2301120732500.845139@rhweight-WRK1>
Precedence: bulk

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.

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.

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
> > 
> > 
> >