Re: [Xen-devel] [RFC/WIP] xen: clk: introudce pvclk for device passthrough

[Peng Fan <van.freenix@xxxxxxxxx>]

Hello Ian,

On Mon, Jan 18, 2016 at 12:41:59PM +0000, Ian Campbell wrote:
>On Mon, 2016-01-18 at 11:24 +0000, David Vrabel wrote:
>> On 16/01/16 05:22, Peng Fan wrote:
>> > This patch was just a initial patch, not sure whether this way
>> > is ok from you side for handlding clk when doing platform device
>> > passhthrough. Any comments are appreciated, and your comments may
>> > give me a better direction.
>> 
>> There's no documentation on the interface, which makes it difficult to
>> review.  At a first look it looks very specific to the particular Linux
>> implementation of a clk subsystem.
>> 
>> > --- /dev/null
>> > +++ b/include/xen/interface/io/clkif.h
>> > @@ -0,0 +1,41 @@
>> > +/*
>> > + * The code contained herein is licensed under the GNU General Public
>> > + * License. You may obtain a copy of the GNU General Public License
>> > + * Version 2 or later at the following locations:
>> > + *
>> > + * http://www.opensource.org/licenses/gpl-license.html
>> > + * http://www.gnu.org/copyleft/gpl.html
>> > + */
>> 
>> ABIs should be under a more permissive license so they can be used by
>> other (non-GPLv2) operating systems.
>
>... along the same lines proposals for new ABIs should be made in the form
>of patches to xen.git:xen/include/public/io/ before being submitted as an
>implementation for one particular os.

I had no idea about this before. Do you mean that before I implement
pvclk for linux, I need to first post the clkif part to xen devel?

If it is, I'll split the interface part and send this part to
xen-devel@xxxxxxxxxxxxxxxxxxxx for review.

>
>
>> > +  unsigned long rate;
>> > +  char clk_name[32];
>> 
>> Where does the frontend get these names from?  31 character names seems
>> rather limiting.
>
>Indeed.
>
>At a guess I would assume they come from the device-tree given to the guest
>and tie into the host device tree.

Yeah. the clk_name is got from DomU dts, and in Dom0 there is also a same
name.

>
>I think a better model might be for each clk to have it's own subdirectory
>under the overall clock bus node, e.g. something like:
>
>/local/domain/<...>/clk/0/nr-clks = 4
>/local/domain/<...>/clk/0/clk-0/ ...
>/local/domain/<...>/clk/0/clk-1/ ...
>/lo
>cal/domain/<...>/clk/0/clk-2/ ...
>/local/domain/<...>/clk/0/clk-3/ ...
>
>and for each subdirectory to contain the a node containing the corresponding 
>firmware table identifier (so path in the DT case), which the toolstack knows, 
>so it can setup the f/e and b/e to correspond as necessary, and the f/e device 
>needn't necessarily use the same name as the backend/host).
>
>The request would then include the index and not the name (and as David 
>observes the response only needs the id).

For now, I have not began the userspace libxl part for pvclk, I use the libxl 
pvusb code for test (:
The id acctually means what operation is needed, such as CLK_PREPARE, 
CLK_UNPREPARE, CLK_SET_RATE, CLK_GET_RATE. I'll add more text to document this 
in V2.

>
>As well as properly documenting the meaning of the operations 
>the clkif.h should also define the xenstore nodes and contain the binary 
>layouts of the req/rsp structs (see netif.h for examples of both, blkif.h also 
>includes examples of the former).

ok. I'll take netif/blkif for reference.

>
>I'd also like to see a description of the DT bindings, which I assume must be 
>needed such that the devices clocks property has something to refer to. For 
>example maybe it doesn't make sense for xenstore to contain the path, but for 
>the pvclk node in xenstore to contain the index.

The DT bindings for xen pvclk, I use this:
"
                clks: clks {
                        compatible = "xen,xen-clk";
                        #clock-cells = <1>;
                        clock-output-names = "uart2_root_clk";
                };
"
the clock-output-names will be parsed and be registered as clk root. The device 
will use index to refer to the clk, as the following:
"
                clocks = <&clks 0>, <&clks 0>;
                clock-names = "ipg", "per";
"
0 means the first one in clock-output-names.

To the xenstore part, I am not sure whether need to expose the clock relate 
info to xenstore. I just want to store the clock names in xenstore to let
user know what clocks are now handled by DomU.

How about the following?

doamin1:
/local/domain/1/device/vclk/nr-clks
/local/domain/1/device/vclk/clk-0/name
/local/domain/1/device/vclk/clk-1/name

domain2:
/local/domain/2/device/vclk/nr-clks
/local/domain/2/device/vclk/clk-0/name
/local/domain/2/device/vclk/clk-1/name

domain0:
/local/domain/0/backend/vclk/1/0/nr-clks
/local/domain/0/backend/vclk/1/0/clk-0/name
/local/domain/0/backend/vclk/1/0/clk-1/name
/local/domain/0/backend/vclk/1/1/nr-clks
/local/domain/0/backend/vclk/1/1/clk-0/name
/local/domain/0/backend/vclk/1/1/clk-1/name

Or do not store the name and nr-clks in domain0?

Thanks,
Peng.

>
>> > +
>> > +DEFINE_RING_TYPES(xen_clkif, struct xen_clkif_request, struct
>> > xen_clkif_response);
>> > +#define XEN_CLK_RING_SIZE __CONST_RING_SIZE(xen_clkif, PAGE_SIZE)
>> > +
>> > +#endif
>> > 
>> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel