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

[Xu Yilun]

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.

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