Message-ID: <CANk1AXSbR037T9jzcs3DtduOiy8Pt-dDcesEnJuY9LTa9vF_og@mail.gmail.com>
Date:   Wed, 5 Apr 2017 10:26:50 -0500
From:   Alan Tull <atull@...nel.org>
To:     "Wu, Hao" <hao.wu@...el.com>
Cc:     Moritz Fischer <moritz.fischer@...us.com>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "Kang, Luwei" <luwei.kang@...el.com>,
        "Zhang, Yi Z" <yi.z.zhang@...el.com>,
        "Whisonant, Tim" <tim.whisonant@...el.com>,
        "Luebbers, Enno" <enno.luebbers@...el.com>,
        "Rao, Shiva" <shiva.rao@...el.com>,
        "Rauer, Christopher" <christopher.rauer@...el.com>,
        "Tull, Alan" <alan.tull@...el.com>,
        Xiao Guangrong <guangrong.xiao@...ux.intel.com>
Subject: Re: [PATCH 11/16] fpga: intel: fme: add partial reconfiguration sub
 feature support

On Wed, Apr 5, 2017 at 6:40 AM, Wu, Hao <hao.wu@...el.com> wrote:
>> >> The  fpga_image_info struct started life as just image specific info,
>> >> but I want it to go in the direction of including parameters needed to
>> >> program it this specific time. Otherwise we are stuck having to keep
>> >> adding parameters as our use of FPGA develops.  It probably could be
>> >> documented better as 'information needed to program a FPGA image'
>> >> rather than strictly 'information about this particular FPGA image'.
>> >> My patch "fpga-mgr: pass parameters for loading fpga in image info"
>> >> goes in this direction by having the buf, firmware name, or sg list
>> >> passed in the info for the added fpga_mgr_load() function.  Actually I
>> >> should probably simplify the API and get rid of fpga_mgr_buf_load,
>> >> fpga_mgr_buf_load_sg, and fpga_mgr_firmware_load and require people to
>> >> use fpga_mgr_load (passing all parameters in fpga_image_info).
>> >>
>> >
>> > Make sense.
>> >
>> >> > It may be a
>> >> > little confusing. One rough idea is that keep this info under fpga region
>> >> > (maybe its private data), and pass the fpga-region to fpga_mgr_buf_load,
>> >>
>> >> Yes, keep this info in fpga-region.  When the region wants to program
>> >> using fpga-mgr, add the region id to fpga_image_info.  I propose
>> >> calling it region_id.
>> >
>> > Hm.. Do we need a function which moves info from region to image info?
>>
>> No, just code that sets that variable in the struct before calling the
>> fpga_region_program_fpga function.
>>
>> >
>> > Another idea is, add a priv to fpga_image_info, and use a common function
>> > to pass the fpga_region's priv to fpga_image_info's priv before PR.
>> > fpga-mgr then knows fpga_region priv info from the fpga_image_info.
>> >
>>
>> Adding priv would make the interface for fpga-mgr non-uniform.  The point
>> of having a fpga-mgr framework is that there
>> is the potential of the upper layers working for different FPGA devices.
>> If the interface for each FPGA device were different, that would then
>> be broken.
>>
>
> I mean drivers can register their own fpga-mgr ops, and handle priv of
> fpga_image_info in driver specific way for pr (e.g write_init function).
> We don't need to change the any upper layer interfaces.

I'm trying to avoid driver specific ways of doing things.  Think of this
all as a set of blocks and we want to be able to switch out individual
blocks in the future.  It's future-proofing and also making code more
generally usable.

fpga_mgr_info is part of the interface for calls to fpga-mgr to do
reprogramming.  My patchset will push it further in that direction
as pointers to the image are added to fpga_mgr_info.

Adding 'priv' to fpga_mgr_info makes that interface specific to a this driver.
It's better to add a region_id variable to fpga_mgr_info that may not be used by
all fpga-mgr drivers.  The current model is that a fpga-mgr driver checks
fpga_mgr_info flags to see if its correct.  The fpga-mgr driver can check
any other needed fpga_mgr_info variables and return error if the params
look invalid.  And ignore any it doesn't need.

I don't think priv belongs in fpga_image_info.  priv tends to be information
for a specific instance of a driver that can have several instances.  priv
usually stores a driver's memory mappings, interrupts, etc for that instance.
It's called private info as it is info that other blocks don't need to know
and don't get to look at.  It's private.  So priv as in interface strikes me as
not private any more and is sort of a red flag.

Alan

>
> If you prefer the region_id for fpga_image_info, we can go with region_id
> for sure. : )
>
> Thanks
> Hao

Powered by blists - more mailing lists