Message-ID: <2d608273-e37d-cb2c-45da-3213aba55f8e@intel.com>
Date:   Mon, 22 Aug 2022 10:38:51 -0700
From:   Russ Weight <russell.h.weight@...el.com>
To:     Xu Yilun <yilun.xu@...el.com>
CC:     Peter Colberg <peter.colberg@...el.com>, Wu Hao <hao.wu@...el.com>,
        "Tom Rix" <trix@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <linux-fpga@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <matthew.gerlach@...ux.intel.com>,
        <basheer.ahmed.muddebihal@...el.com>, <tianfei.zhang@...el.com>,
        <marpagan@...hat.com>, <lgoncalv@...hat.com>
Subject: Re: [PATCH v1] uio: dfl: add IOPLL user-clock feature id



On 8/21/22 21:49, Xu Yilun wrote:
> On 2022-08-18 at 17:38:35 -0600, Russ Weight wrote: >> >> >> On 8/17/22 21:18, Xu Yilun wrote: >>> On 2022-08-17 at 17:37:46 -0400, Peter Colberg wrote: >>>> Add a Device Feature List (DFL) feature id for the >>>> configurable IOPLL user clock source, which can be used to >>>> configure the clock speeds that are used for RTL logic that is >>>> programmed into the Partial Reconfiguration (PR) region of an >>>> FPGA. >>> Why not use linux clock framework for this IOPLL? And let the PR >>> driver set it togeter with the RTL logic reporgramming? >> >> Hi Yilun, >> >> We previously explored the possibility of plugging into the linux >> clock framework. For this device, setting a desired frequency is >> heavily dependent on a table of values that must be programmed in >> order to achieve the desired clock speeds. >> >> Here is an example table, indexed by frequency. The first element >> in each entry is the frequency in kHz: >> >> https://github.com/OPAE/opae-sdk/blob/master/libraries/plugins/xfpga/usrclk/fpga_user_clk_freq.h >> >>
>> >> >> >> >> We previously experimented with a kernel-space driver. The
>> implementation exported a sysfs node into which the table values >> for the desired frequency would be written in order to set the >> desired frequency. The function of the driver was to execute the >> logic required to program the device. We did not think this >> implementation should be up-streamed. >> >> It isn't practical to upstream the frequency tables as they are >> subject to change for future devices. For example, if the >> reference frequency changed in a future device, a whole new table >> of values would have to be added for the new device. In a recent >> transition to a new device, the range of frequencies was increased >> which required an extension to an existing table. > > Making a table for the inputs & outputs is always a easier way to > get things done, but the trade off is, as you said, extension to the > table every time for new outputs. > > So do we really need all parameters to be in a table, or these are > actually the outcome of some calculation? Is it possible just > Implementing the calculation.
For each desired frequency, the table values are produced by calling
the quartus tool, the same tool that generates the IOPLL RTL logic.
The quartus tool allows the RTL designer to select different options
which can affect the table values. For example, the current IOPLL
used in OFS has two frequency outputs and the desired relationship
between the two frequencies is 1x/2x until the 2x frequency reaches
a threshold (about 800MHz) and then the relationship is modified.

To convert this process into an algorithm would require reverse
engineering the quartus algorithm for the set of variables and
clock relationships in a specific implementation. The resulting
algorithm would have a very narrow application; we would have to
upstream additional algorithms for future, modified implementations.
Also, customers have the ability to modify the IOPLL implementation
if they choose. A table driven driver enables customers to easily
adapt the driver to their implementation.

We think a userspace table-driven driver is the best approach for
supporting the user clock.

- Russ

> > > If I remember correctly, linux clk framework enables a generic clk > caculation mechanism. It encourages people to model the internal > refclk, plls (and deviders?) separately and construct the clk tree. > Then the specified calculation could be simpler for each clk driver. > > I'm not sure the clk framework fits all your need, but please > investigate it firstly. > >> >> A previous implementation of the user clock was also implemented >> in user-space. The kernel driver exported each of the registers, >> but all of the logic was implemented in user-space. The kernel >> portion can be viewed here: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/dfl-afu-main.c#n380 >> >> >> >> >> >> >> This is our reasoning in choosing to implement this driver in
>> user-space. Would you consider a uio based user-space driver to be >> acceptable for in this case? > > As usual, we firstly make clear why existing framework cannot fit > the case and should be implemented in userspace, then everything > would be OK. > > Thanks, Yilun > >> >> - Russ >> >> >>> >>> Thanks, Yilun >>> >>>> The DFL feature id table can be found at: >>>> https://github.com/OPAE/dfl-feature-id >>>> >>>> Signed-off-by: Peter Colberg <peter.colberg@...el.com> --- >>>> drivers/uio/uio_dfl.c | 2 ++ 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/uio/uio_dfl.c b/drivers/uio/uio_dfl.c >>>> index 8f39cc8bb034..69e93f3e7faf 100644 --- >>>> a/drivers/uio/uio_dfl.c +++ b/drivers/uio/uio_dfl.c @@ -46,10 >>>> +46,12 @@ static int uio_dfl_probe(struct dfl_device *ddev) >>>> >>>> #define FME_FEATURE_ID_ETH_GROUP 0x10 #define >>>> FME_FEATURE_ID_HSSI_SUBSYS 0x15 +#define >>>> PORT_FEATURE_ID_IOPLL_USRCLK 0x14 >>>> >>>> static const struct dfl_device_id uio_dfl_ids[] = { { FME_ID, >>>> FME_FEATURE_ID_ETH_GROUP }, { FME_ID, >>>>
FME_FEATURE_ID_HSSI_SUBSYS }, + { PORT_ID, >>>> PORT_FEATURE_ID_IOPLL_USRCLK }, { } }; MODULE_DEVICE_TABLE(dfl, >>>> uio_dfl_ids); -- 2.28.0 >>>> >>

Powered by blists - more mailing lists