clk-fixed-rate clock driver usage with fpgas was: Re: Fpga manager help

[Michal Simek <monstr@monstr.eu>]

[-- Attachment #1.1: Type: text/plain, Size: 6392 bytes --]

Hi, +linux-clk

On 21.2.2018 20:25, Alan Tull wrote:
> On Wed, Feb 21, 2018 at 6:33 AM, Michal Simek <monstr@monstr.eu> wrote:
>> Hi, +Grant and Pantelis,
> 
> Hi Shubhrajyoti,
> 
> I want to make sure I understand your situation here.  So I'll ask
> some questions, please let me know if I'm misunderstanding.
> 
>>
>> On 21.2.2018 06:36, Shubhrajyoti Datta wrote:
>>> Hi Alan,
>>> Thanks for the help.
>>>
>>>
>>> On Tue, Feb 20, 2018 at 9:54 PM, Alan Tull <atull@kernel.org> wrote:
>>>> On Mon, Feb 19, 2018 at 1:09 AM, Shubhrajyoti Datta
>>>> <shubhrajyoti.datta@gmail.com> wrote:
>>>>
>>>> Hi Shubhrajyoti,
>>>>
>>>>> Hi ,
>>>>>
>>>>> Some of the FPGA has a  clock  rate that it needs to operate.
> 
> The FPGA device needs a clock, right?  This isn't dependent on what
> FPGA image is loaded; this is the clock that makes the FPGA device
> happy, right?

There are several scenarios how to use these clocks but let me describe
HW. Xilinx fpgas have some clock lines coming from fixed part(PS) to
programmable logic(PL). Clock rate is setup by initial device
programming (bootloader) but that's just fixed for the whole live cycle.

To be more flexible there is a need to have an option to change clock
rate depending on bitstream you want to program (It doesn't matter if
this is full bitstream or partial bitstream).
For example: That means one set of bitstreams with dt overlay where one
require 25MHz on clock line and another 50MHz.


> If that is the case, this is platform specific setup, which can be
> done by adding an overlay or it could have been in the base device
> tree, right?  The second overlay can do the FPGA programming.

I don't think that make sense to have more overlays and mess up things
just because of setting up proper clock rate for specific PL.

Anyway concept which I am talking about looks like this in DT description.

/dts-v1/;
/plugin/;

/ {
  fragment {
    target = <&fpga_full>;
    __overlay__ {
      #address-cells = <1>;
      #size-cells = <1>;
      firmware-name = "download.bin"; /* bitstream */
      fpga-bridges = <&... &...>; /* some bridges */

      pl1: clock {
	compatible = "fixed-factor-clock";
	clocks = <&clk 71>;
	#clock-cells = <0>;
	clock-frequency = <25000000>; /* missing property */
	clock-div = <1>;
	clock-mult = <1>;
	clock-output-names = "pl1"
      };

      /* The same as above but with different driver */
      pl1-alt: clock {
	compatible = "fixed-clock";
	clocks = <&clk 71>; /* missing property */
	#clock-cells = <0>;
	clock-frequency = <25000000>;
	clock-output-names = "pl1";
      };
      /* or completely new driver based on above two. */

      gpio_on: gpio@XXX {
	compatible = "xlnx,xps-gpio-1.00.a";
	gpio-controller ;
	clocks = <&pl1>; /* or pl1-alt or new driver */
      };
    };
  };
};

It means download.bit is loaded and overlay applied. clock driver is
probed and gpio driver is waiting for it (-EDEFER if not ready).

This concept should ensure that no driver is probed without setting up
proper clock rate which bitstream was built for.

As you see above I have added pl1 or pl1-alt nodes which are using
current drivers but we are missing functionality there.

1) In fixed-factor-clock we are missing option to setup clock rate
directly. That the driver will ask parent for setting up rate. Driver
contain support added with "allwinner,sun4i-a10-pll3-2x-clk"
(e6cbf9984ee73) by Maxime Ripard which enables changing rate from childs
but that's different functionality than we need.

2) in fixed-clock driver we are missing connection with parent to ask
for setting up described clock.

There is also an option to write new driver. Based on my look the driver
would be almost the same as above with just small difference. That's why
I think that will be the best to add this functionality to current
driver instead of creating similar one.

fixed-factor-clk seems to be better because there is already patch from
Maxime for passing setting up rate from childs but I would like hear
your opinion.
The patch would like just like this

clock-frequency - DT optional parameter.

And in driver:

of_property_read_u32(node, "clock-frequency", &rate);
if (rate)
	clk_set_rate(..., rate);

Please let me what you think.

Thanks,
Michal

> 
>>>>> Wanted some help in having suggestions as to  how should this be done.
>>>>>
>>>>>
>>>>> I tried the bridge approach.
>>>>> It worked however since having the possible rate have precompiled is difficult
>>>>> because of the wide range of possible frequency.
>>>>>
>>>>> So i have a first overlay that updates the rate and second overlay
>>>>> actually apply.
>>>>
>>>> IIUC, that sounds right to me.  The base overlay sets up bridges and
>>>> fpga regions.  The overlay after that does the programming.  That's
>>>> consistent with the fpga-region device tree binding documentation.
>>>
>>> My options were
>>> 1) a first overlay that updates the rate and second overlay
>>>  actually apply.
> 
> So that's what I'm saying above, IIUC
> 
>>> 2) overlay has a pseudo clock driver that sets the rate and acts
>>> as a provider of clocks so that other drivers are EPROBE_DEFERED.
>>>
>>> Do you think I should prefer option 1)
>> I would prefer option too.
> 
> Option two?
> 
> What does option 2 give us that option 1 doesn't?  Seems like more
> work, unless there's something in this setup I don't know about, which
> seems pretty likely right now.
> 
>> If you want to use option 1 then still
>> devices in second overlay should reference clock setup in the first.
> 
> Now that makes it sound like we're talking about clocks for the
> hardware that is in FPGA fabric?
> 
>>
>> Thanks,
>> Michal
>>
>>
>> --
>> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
>> w: www.monstr.eu p: +42-0-721842854
>> Maintainer of Linux kernel - Xilinx Microblaze
>> Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
>> U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs
>>
>>


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]