Re: [PATCH v1] uio: dfl: add IOPLL user-clock feature id

[Xu Yilun]

On 2022-08-22 at 10:38:51 -0700, Russ Weight wrote:
> 
> 
> 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.

OK, this is an important reason.

> 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

This makes sense to me.

> 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

That also makes sense to me.

> adapt the driver to their implementation.

So could you please add some brief description in commit message? The
code is good to me.

Thanks,
Yioun

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