On Mon, May 08, 2017 at 03:44:12PM -0500, Alan Tull wrote: > On Mon, May 8, 2017 at 3:44 AM, Wu, Hao <hao...@intel.com> wrote: > >> On Wed, May 3, 2017 at 3:07 PM, Alan Tull <at...@kernel.org> wrote: > >> > On Wed, May 3, 2017 at 6:58 AM, Wu Hao <hao...@intel.com> wrote: > >> >> On Thu, Apr 20, 2017 at 09:09:47AM -0500, Alan Tull wrote: > >> >>> Add two functions for getting the FPGA bridge from the device > >> >>> rather than device tree node. This is to enable writing code > >> >>> that will support using FPGA bridges without device tree. > >> >>> Rename one old function to make it clear that it is device > >> >>> tree-ish. This leaves us with 3 functions for getting a bridge: > >> >>> > >> >>> * fpga_bridge_get > >> >>> Get the bridge given the device. > >> >>> > >> >>> * fpga_bridges_get_to_list > >> >>> Given the device, get the bridge and add it to a list. > >> >>> > >> >>> * of_fpga_bridges_get_to_list > >> >>> Renamed from priviously existing fpga_bridges_get_to_list. > >> >>> Given the device node, get the bridge and add it to a list. > >> >>> > >> >> > >> >> Hi Alan > >> >> > >> >> Thanks a lot for providing this patch set for non device tree support. > >> >> :) > >> >> Actually I am reworking the Intel FPGA device drivers based on this > >> >> patch > >> >> set, and I find some problems with the existing APIs including fpga > >> >> bridge > >> >> and manager. My idea is to create all fpga bridges/regions/manager under > >> >> the same platform device (FME), it allows FME driver to establish the > >> >> relationship for the bridges/regions/managers it creates in an easy way. > >> >> But I found current fpga class API doesn't support this very well. > >> >> e.g fpga_bridge_get/get_to_list only accept parent device as the input > >> >> parameter, but it doesn't work if we have multiple bridges (and > >> >> regions/manager) under the same platform device. fpga_mgr has similar > >> >> issue, but fpga_region APIs work better, as they accept fpga_region as > >> >> parameter not the shared parent device. > >> > > >> > That's good feedback. I can post a couple patches that apply on top > >> > of that patchset to add the APIs you need. > >> > > >> > Probably what I'll do is add > >> > > >> > struct fpga_manager *fpga_mgr_get(struct fpga_manager *mgr); > >> > > >> > And rename fpga_bridge_get() to fpga_bridge_dev_get() and add the > >> following: > >> > > >> > struct fpga_bridge *fpga_bridge_get(struct fpga_bridge *br, > >> > struct fpga_image_info *info); > >> > > >> > int of_fpga_bridge_get_to_list(struct fpga_bridge *br, > >> > struct fpga_image_info *info, > >> > struct list_head *bridge_list); > >> > > >> > Working on it now. > >> > > >> >> > >> >> Do you think if having multiple fpga-* under one parent device is in the > >> >> right direction? > >> > > >> > That should be fine as long as it's coded with an eye on making things > >> > reusable and seeing beyond the current project. Just thinking of the > >> > future and of what can be of general usefulness for others. And there > >> > will be others interested in reusing this. > >> > > >> > Alan > >> > >> Actually, I don't think you will need the additional APIs we were > >> just discussing after all. What you have is a multifunction device > >> (single piece of hardware, multi functions such as in drivers/mfd). > >> It will have child devices for the mgr, bridges, and regions. When > >> registering the mgr and bridges you will need to allocate child > >> devices and use them to create the mgr and bridges. > >> > >> Alan > > > > Hi Alan > > > > I tried to create child devices as the parent device for the mgr and > > bridges in fme platform driver module. If only creates the device without > > driver, it doesn't work as try_module_get(dev->parent->driver->owner) > > always failed in mgr_get and bridge_get functions. > > I tried it and it wasn't hard. > > Each mgr or bridge driver should be a separate file which registers > its driver using 'module_platform_driver". That way the drivers are > registered with the kernel in a normal fashion. The thing we want > here is to not bypass the kernel driver model. > > You'll need to keep the platform_device pointers in private data somewhere. > > For each child platform device, do a platform_device_alloc and > platform_device_add. > > Then to get the manager, you can do > > mgr = fpga_mgr_get(&priv->mgr_pdev->dev); > > If this is in your probe function, you can use -EPROBE_DEFER if > platform_device_alloc or fpga_mgr_get fail. Then you could destroy > whatever you've created and return -EPROBE_DEFER to wait for the > drivers you need to be registered and ready for devices to be added. > > > > > If it creates platform devices as child devices, and introduce new platform > > device drivers for bridge and mgr, then it will be difficult to establish > > the > > relationship for region/mgr/bridges (e.g when should region->mgr be > > configured and cleared, as mgr is created/destroyed when mgr parent > > device platform driver module is loaded/unload), and it maybe not really > > necessary to introduce more different driver modules here. > > It should be pretty easy to create/destroy child devices as shown > above. The kernel does this all the time. > > > > > But if it allows multiple fpga-* created under one device in one device > > driver, it will be much easier to avoid above problems. So I asked if it > > is possible to create multiple fpga-* under one parent device, > > I think it's fine for your FME to create child platform devices. It's > similar to a mfd, but the mfd framework hides the platform devices > from the module that creates them, unfortunately. > > > I feel > > this will not impact to current fpga drivers a lot, but provide more > > flexibility for drivers to use fpga-region/bridge/manager to create > > the topology in a device specific way, especially for non device > > tree case. > > > > I would like to see most of this code as FME enumeration code + a mgr > driver + a bridge driver + a region driver. If the FME and the > enumeration code can be separate files, so much the better for general > usability. > > The enumeration code can build a set of regions by doing something like this: > 1. figure out what type of mgr and bridges your hardware FME has. > 2. do platform_device_alloc and platform_device_add to create the mgr > device, save a pointer to its platform_device in your FME driver's > private data. > 2. For each port, create a region and a bridge device. Save the > region's platform device or struct in a list in your FME driver's > priv. > 3. then you can create the sub function devices.
Thanks Alan for the suggestion. I will follow this direction to rework the FME driver. In above step 2, I will put fpga-mgr/bridge platform device info into the platform data of the fpga-region platform device, then fpga-region knows which mgr and bridge to use. Thanks Hao
