Message ID | 20171017212031.3770-14-atull@kernel.org (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Hi Alan, I have a little confused on the fpga_region_register() interface. > -static int of_fpga_region_probe(struct platform_device *pdev) > +int fpga_region_register(struct device *dev, struct fpga_region > +*region) > { >... > device_initialize(®ion->dev); > region->dev.class = fpga_region_class; > region->dev.parent = dev; > - region->dev.of_node = np; > + region->dev.of_node = dev->of_node; > region->dev.id = id; > dev_set_drvdata(dev, region); For the device tree version, it is clear that each fpga region will have one of_node, and the fpga_region is registered to it with the **dev->driver_data** point to the fpga_region struct. But for non- device tree version (e.g. pcie-attached fpga in x86), I cannot just call "fpga_region_register(&pdev->dev, region[i])" multiple times if I want to register multiple regions. Because the register interface use "pdev->dev.driver_data" to store reference to fpga_region. So, I have to separate it to 2 steps: - create one sub-device(maybe platform_dev?) for each region, - and register fpga_region to these sub-devices instead of the pci_dev. Is there any way to omit these extra sub-devices? Thank you. Jiuyue Ma -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jiuyue, On Fri, Oct 20, 2017 at 07:23:31AM +0000, majiuyue wrote: > Hi Alan, > > I have a little confused on the fpga_region_register() interface. > > > -static int of_fpga_region_probe(struct platform_device *pdev) > > +int fpga_region_register(struct device *dev, struct fpga_region > > +*region) > > { > >... > > device_initialize(®ion->dev); > > region->dev.class = fpga_region_class; > > region->dev.parent = dev; > > - region->dev.of_node = np; > > + region->dev.of_node = dev->of_node; > > region->dev.id = id; > > dev_set_drvdata(dev, region); > > For the device tree version, it is clear that each fpga region will have one of_node, > and the fpga_region is registered to it with the **dev->driver_data** point to the fpga_region struct. > > But for non- device tree version (e.g. pcie-attached fpga in x86), > I cannot just call "fpga_region_register(&pdev->dev, region[i])" multiple times if I want to register > multiple regions. Because the register interface use "pdev->dev.driver_data" to store reference to fpga_region. > > So, I have to separate it to 2 steps: > > - create one sub-device(maybe platform_dev?) for each region, This step doesn't seem to crazy to me, because most likely you wanna be able to have region specific behavior on a per subdevice basis? One big issue is that we don't have a whole lot of PCIe hardware to look. If you have a project that requires this, sharing some code (snippets) might allow for better architecture and ultimately make your job easier ;-) > - and register fpga_region to these sub-devices instead of the pci_dev. > > Is there any way to omit these extra sub-devices? We can probably work around this, again, a specific usecase would help. > > > Thank you. > > Jiuyue Ma > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fpga" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Thanks, Moritz -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 22, 2017 at 4:43 AM, Moritz Fischer <mdf@kernel.org> wrote: Hi Jiuyue, > Hi Jiuyue, > > On Fri, Oct 20, 2017 at 07:23:31AM +0000, majiuyue wrote: >> Hi Alan, >> >> I have a little confused on the fpga_region_register() interface. >> >> > -static int of_fpga_region_probe(struct platform_device *pdev) >> > +int fpga_region_register(struct device *dev, struct fpga_region >> > +*region) >> > { >> >... >> > device_initialize(®ion->dev); >> > region->dev.class = fpga_region_class; >> > region->dev.parent = dev; >> > - region->dev.of_node = np; >> > + region->dev.of_node = dev->of_node; >> > region->dev.id = id; >> > dev_set_drvdata(dev, region); Thanks for catching this. This dev_set_drvdata is only needed for the platform_get_drvdata that's in of_fpga_region_remove. So the dev_set_drvdta should be taken from fpga_region_register and moved to of_fpga_region_probe. How does that sound? Alan >> >> For the device tree version, it is clear that each fpga region will have one of_node, >> and the fpga_region is registered to it with the **dev->driver_data** point to the fpga_region struct. >> >> But for non- device tree version (e.g. pcie-attached fpga in x86), >> I cannot just call "fpga_region_register(&pdev->dev, region[i])" multiple times if I want to register >> multiple regions. Because the register interface use "pdev->dev.driver_data" to store reference to fpga_region. >> >> So, I have to separate it to 2 steps: >> >> - create one sub-device(maybe platform_dev?) for each region, > > This step doesn't seem to crazy to me, because most likely you wanna be > able to have region specific behavior on a per subdevice basis? > > One big issue is that we don't have a whole lot of PCIe > hardware to look. If you have a project that requires this, sharing some > code (snippets) might allow for better architecture and ultimately make your job > easier ;-) > >> - and register fpga_region to these sub-devices instead of the pci_dev. >> >> Is there any way to omit these extra sub-devices? > > We can probably work around this, again, a specific usecase would help. >> >> >> Thank you. >> >> Jiuyue Ma >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > Thanks, > Moritz -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, > >> > + region->dev.of_node = dev->of_node; > >> > region->dev.id = id; > >> > dev_set_drvdata(dev, region); > > Thanks for catching this. This dev_set_drvdata is only needed for the > platform_get_drvdata that's in of_fpga_region_remove. So the > dev_set_drvdta should be taken from fpga_region_register and moved to > of_fpga_region_probe. > > How does that sound? That looks perfect :) Then I should be able to register both fpga_mgr and fpga_region(s) to the same parent device (e.g. pci_dev). Thank you. ---- Jiuyue
Hi Moritz, Sorry for the late reply, because I'm still trying to figure out the differences between a device-tree based system and a pcie base system. In my mind, the main differences between them are: - In a of_* based system, the kernel will create platform_device objects for all of_node when system is boot up. Because these platform_device(s) are already there, a driver only need to match them and do the probe/register works. - But, for pcie based system, it is the driver's responsibility to discover all the sub-features/sub-devices for a pci_dev. So the driver have the right to decide how to construct the device hierarchy below the parent pci_dev. We (the fpga framework) should not make any restrictions/assumptions on this, such as: * "fpga_mgr/region/bridge are stored in pdev->driver_data" --> the pci driver may use it to store private data * "try_module_get(dev->parent->driver->owner)" --> the mgr/region/bridge's parent driver may not exist, because they are registered by parent's parent driver. So I'm not agree with your point: > >This step doesn't seem to crazy to me, because most likely you wanna be able to have region specific behavior on a per subdevice basis? > And surely, we are very glad to share our usecase to the fpga framework and make it better :) Thanks. ---- Jiuyue -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jiuyue, On Thu, Oct 26, 2017 at 02:14:56AM +0000, majiuyue wrote: > Hi Moritz, > > Sorry for the late reply, because I'm still trying to figure out the differences between a device-tree based system and a pcie base system. > > In my mind, the main differences between them are: > > - In a of_* based system, the kernel will create platform_device objects for all of_node when system is boot up. > Because these platform_device(s) are already there, a driver only need to match them and do the probe/register works. > > - But, for pcie based system, it is the driver's responsibility to discover all the sub-features/sub-devices for a pci_dev. > So the driver have the right to decide how to construct the device hierarchy below the parent pci_dev. > We (the fpga framework) should not make any restrictions/assumptions on this, such as: > * "fpga_mgr/region/bridge are stored in pdev->driver_data" --> the pci driver may use it to store private data > * "try_module_get(dev->parent->driver->owner)" --> the mgr/region/bridge's parent driver may not exist, because they are registered by parent's parent driver. > > So I'm not agree with your point: > > > > >This step doesn't seem to crazy to me, because most likely you wanna be able to have region specific behavior on a per subdevice basis? > > You are right, I had misunderstood what you where trying to do. > > And surely, we are very glad to share our usecase to the fpga framework and make it better :) > Sounds good, > > Thanks. > > ---- > Jiuyue Cheers, Moritz -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 26, 2017 at 2:08 AM, Moritz Fischer <mdf@kernel.org> wrote: Hi Jiuyue, > Hi Jiuyue, > > On Thu, Oct 26, 2017 at 02:14:56AM +0000, majiuyue wrote: >> Hi Moritz, >> >> Sorry for the late reply, because I'm still trying to figure out the differences between a device-tree based system and a pcie base system. >> >> In my mind, the main differences between them are: >> >> - In a of_* based system, the kernel will create platform_device objects for all of_node when system is boot up. >> Because these platform_device(s) are already there, a driver only need to match them and do the probe/register works. >> >> - But, for pcie based system, it is the driver's responsibility to discover all the sub-features/sub-devices for a pci_dev. >> So the driver have the right to decide how to construct the device hierarchy below the parent pci_dev. >> We (the fpga framework) should not make any restrictions/assumptions on this, such as: >> * "fpga_mgr/region/bridge are stored in pdev->driver_data" --> the pci driver may use it to store private data I'll have to look at this more. The code that uses drvdata that way may need to move out of the fpga-bridge/mgr/region framework register functions to the calling probe functions. Alan >> >> So I'm not agree with your point: >> >> > >> >This step doesn't seem to crazy to me, because most likely you wanna be able to have region specific behavior on a per subdevice basis? >> > > > You are right, I had misunderstood what you where trying to do. >> >> And surely, we are very glad to share our usecase to the fpga framework and make it better :) >> > > Sounds good, >> >> Thanks. >> >> ---- >> Jiuyue > > Cheers, > > Moritz -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index 92ab216..76db81d 100644 --- a/drivers/fpga/fpga-region.c +++ b/drivers/fpga/fpga-region.c @@ -143,7 +143,6 @@ static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np) /** * of_fpga_region_get_bridges - create a list of bridges * @region: FPGA region - * @info: FPGA image info * * Create a list of bridges including the parent bridge and the bridges * specified by "fpga-bridges" property. Note that the @@ -156,11 +155,11 @@ static struct fpga_manager *of_fpga_region_get_mgr(struct device_node *np) * Return 0 for success (even if there are no bridges specified) * or -EBUSY if any of the bridges are in use. */ -static int of_fpga_region_get_bridges(struct fpga_region *region, - struct fpga_image_info *info) +static int of_fpga_region_get_bridges(struct fpga_region *region) { struct device *dev = ®ion->dev; struct device_node *region_np = dev->of_node; + struct fpga_image_info *info = region->info; struct device_node *br, *np, *parent_br = NULL; int i, ret; @@ -192,7 +191,7 @@ static int of_fpga_region_get_bridges(struct fpga_region *region, continue; /* If node is a bridge, get it and add to list */ - ret = of_fpga_bridge_get_to_list(br, region->info, + ret = of_fpga_bridge_get_to_list(br, info, ®ion->bridge_list); /* If any of the bridges are in use, give up */ @@ -229,10 +228,16 @@ int fpga_region_program_fpga(struct fpga_region *region) goto err_put_region; } - ret = of_fpga_region_get_bridges(region, info); - if (ret) { - dev_err(dev, "failed to get FPGA bridges\n"); - goto err_unlock_mgr; + /* + * In some cases, we already have a list of bridges in the + * fpga region struct. Or we don't have any bridges. + */ + if (region->get_bridges) { + ret = region->get_bridges(region); + if (ret) { + dev_err(dev, "failed to get fpga region bridges\n"); + goto err_unlock_mgr; + } } ret = fpga_bridges_disable(®ion->bridge_list); @@ -259,7 +264,8 @@ int fpga_region_program_fpga(struct fpga_region *region) return 0; err_put_br: - fpga_bridges_put(®ion->bridge_list); + if (region->get_bridges) + fpga_bridges_put(®ion->bridge_list); err_unlock_mgr: fpga_mgr_unlock(region->mgr); err_put_region: @@ -522,39 +528,20 @@ static struct notifier_block fpga_region_of_nb = { .notifier_call = of_fpga_region_notify, }; -static int of_fpga_region_probe(struct platform_device *pdev) +int fpga_region_register(struct device *dev, struct fpga_region *region) { - struct device *dev = &pdev->dev; - struct device_node *np = dev->of_node; - struct fpga_region *region; - struct fpga_manager *mgr; int id, ret = 0; - mgr = of_fpga_region_get_mgr(np); - if (IS_ERR(mgr)) - return -EPROBE_DEFER; - - region = kzalloc(sizeof(*region), GFP_KERNEL); - if (!region) { - ret = -ENOMEM; - goto err_put_mgr; - } - - region->mgr = mgr; - id = ida_simple_get(&fpga_region_ida, 0, 0, GFP_KERNEL); - if (id < 0) { - ret = id; - goto err_kfree; - } + if (id < 0) + return id; mutex_init(®ion->mutex); INIT_LIST_HEAD(®ion->bridge_list); - device_initialize(®ion->dev); region->dev.class = fpga_region_class; region->dev.parent = dev; - region->dev.of_node = np; + region->dev.of_node = dev->of_node; region->dev.id = id; dev_set_drvdata(dev, region); @@ -566,19 +553,58 @@ static int of_fpga_region_probe(struct platform_device *pdev) if (ret) goto err_remove; + return 0; + +err_remove: + ida_simple_remove(&fpga_region_ida, id); + return ret; +} +EXPORT_SYMBOL_GPL(fpga_region_register); + +int fpga_region_unregister(struct fpga_region *region) +{ + device_unregister(®ion->dev); + + return 0; +} +EXPORT_SYMBOL_GPL(fpga_region_unregister); + +static int of_fpga_region_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct fpga_region *region; + struct fpga_manager *mgr; + int ret; + + /* Find the FPGA mgr specified by region or parent region. */ + mgr = of_fpga_region_get_mgr(np); + if (IS_ERR(mgr)) + return -EPROBE_DEFER; + + region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL); + if (!region) { + ret = -ENOMEM; + goto eprobe_mgr_put; + } + + region->mgr = mgr; + + /* Specify how to get bridges for this type of region. */ + region->get_bridges = of_fpga_region_get_bridges; + + ret = fpga_region_register(dev, region); + if (ret) + goto eprobe_mgr_put; + of_platform_populate(np, fpga_region_of_match, NULL, ®ion->dev); dev_info(dev, "FPGA Region probed\n"); return 0; -err_remove: - ida_simple_remove(&fpga_region_ida, id); -err_kfree: - kfree(region); -err_put_mgr: +eprobe_mgr_put: fpga_mgr_put(mgr); - return ret; } @@ -586,7 +612,7 @@ static int of_fpga_region_remove(struct platform_device *pdev) { struct fpga_region *region = platform_get_drvdata(pdev); - device_unregister(®ion->dev); + fpga_region_unregister(region); fpga_mgr_put(region->mgr); return 0; @@ -606,7 +632,6 @@ static void fpga_region_dev_release(struct device *dev) struct fpga_region *region = to_fpga_region(dev); ida_simple_remove(&fpga_region_ida, region->dev.id); - kfree(region); } /** diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h index 8a35517..8c8a3249f 100644 --- a/include/linux/fpga/fpga-region.h +++ b/include/linux/fpga/fpga-region.h @@ -12,6 +12,8 @@ * @bridge_list: list of FPGA bridges specified in region * @mgr: FPGA manager * @info: FPGA image info + * @priv: private data + * @get_bridges: optional function to get bridges to a list */ struct fpga_region { struct device dev; @@ -19,10 +21,15 @@ struct fpga_region { struct list_head bridge_list; struct fpga_manager *mgr; struct fpga_image_info *info; + void *priv; + int (*get_bridges)(struct fpga_region *region); }; #define to_fpga_region(d) container_of(d, struct fpga_region, dev) int fpga_region_program_fpga(struct fpga_region *region); +int fpga_region_register(struct device *dev, struct fpga_region *region); +int fpga_region_unregister(struct fpga_region *region); + #endif /* _FPGA_REGION_H */
Another step in separating common code from device tree specific code for FPGA regions. * add FPGA region register/unregister functions. * add the register/unregister functions to the header * use devm_kzalloc to alloc the region. * add a method for getting bridges to the region struct * add priv to the region struct * use region->info in of_fpga_region_get_bridges Signed-off-by: Alan Tull <atull@kernel.org> --- v2: split out from another patch v3: use region->info, remove info param where applicable v4: no change to this patch in this version of patchset v5: add back in fpga_mgr_put in of_fpga_region_remove --- drivers/fpga/fpga-region.c | 105 ++++++++++++++++++++++++--------------- include/linux/fpga/fpga-region.h | 7 +++ 2 files changed, 72 insertions(+), 40 deletions(-)