Enable upper layers using FPGA region w/o device tree

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.

Quoted 9 lines · Alan Tull

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.

Quoted 7 lines

* 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.

Do you think if having multiple fpga-* under one parent device is in the right direction?

If yes, shall we provide some more APIs which accept fpga_bridge (and same for fpga-mgr) as parameter instead of the parent device just like fpga-region?

Thanks
Hao

Quoted 20 lines · Wu Hao

Quoted 9 lines · Alan Tull

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.

Quoted 7 lines

* 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

Quoted 12 lines

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.

Quoted 4 lines

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

Quoted 7 lines

If yes, shall we provide some more APIs which accept fpga_bridge (and same for fpga-mgr) as parameter instead of the parent device just like fpga-region?

Thanks
Hao

Quoted 20 lines · Wu Hao

Quoted 9 lines · Alan Tull

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.

Quoted 7 lines

* 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

Quoted 12 lines

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.

Quoted 3 lines

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

Quoted 8 lines

If yes, shall we provide some more APIs which accept fpga_bridge (and same for fpga-mgr) as parameter instead of the parent device just like fpga-region?

Thanks
Hao

Quoted 34 lines · Wu Hao

Quoted 20 lines · Alan Tull

Quoted 9 lines

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.

Quoted 7 lines

* 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

Quoted 12 lines

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.

Quoted 8 lines

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:

Quoted 14 lines

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.

Quoted 3 lines

Do you think if having multiple fpga-* under one parent device is in the right direction?

Quoted 6 lines

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

Quoted 72 lines · Alan Tull

Quoted 34 lines · Wu Hao

Quoted 20 lines · Alan Tull

Quoted 9 lines

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.

Quoted 7 lines

* 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

Quoted 12 lines

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.

Quoted 8 lines

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:

Quoted 14 lines

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.

Quoted 3 lines

Do you think if having multiple fpga-* under one parent device is in the right direction?

Quoted 6 lines

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.

Quoted 1 line

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.

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.

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 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.

Thanks
Hao

Quoted 72 lines · Hao

Quoted 72 lines · Alan Tull

Quoted 34 lines · Wu Hao

Quoted 20 lines · Alan Tull

Quoted 9 lines

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.

Quoted 7 lines

* 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

Quoted 12 lines

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.

Quoted 8 lines

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:

Quoted 14 lines

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.

Quoted 3 lines

Do you think if having multiple fpga-* under one parent device is in the right direction?

Quoted 6 lines

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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 135 lines · Alan Tull

Quoted 72 lines · Hao

Quoted 72 lines · Alan Tull

Quoted 34 lines · Wu Hao

Quoted 20 lines · Alan Tull

Quoted 9 lines

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.

Quoted 7 lines

* 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

Quoted 12 lines

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.

Quoted 8 lines

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:

Quoted 14 lines

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.

Quoted 3 lines

Do you think if having multiple fpga-* under one parent device is in the right direction?

Quoted 6 lines

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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 9 lines

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.

The above sounds like a poster-child application for MFD. If you do it in a clever
way (i.e. write your platform drivers in a reusable way) you might be able to just reuse them on your next generation.

Cheers,

Moritz

Quoted 146 lines · Moritz Fischer

Quoted 135 lines · Alan Tull

Quoted 72 lines · Hao

Quoted 72 lines · Alan Tull

Quoted 34 lines · Wu Hao

Quoted 20 lines · Alan Tull

Quoted 9 lines

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.

Quoted 7 lines

* 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

Quoted 12 lines

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.

Quoted 8 lines

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:

Quoted 14 lines

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.

Quoted 3 lines

Do you think if having multiple fpga-* under one parent device is in the right direction?

Quoted 6 lines

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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 9 lines

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.

Quoted 4 lines

The above sounds like a poster-child application for MFD. If you do it in a clever
way (i.e. write your platform drivers in a reusable way) you might be able to just reuse them on your next generation.

Yes, I played with that with some test code. I wrote some test code
that allocates dummy mgr and bridge devices using mfd_add_devices(). I didn't see any way of getting access to the devices after creating them. Maybe I'm missing something. Neither the dev nor the platform_device is saved in the cell struct.

Hi Alan,

Quoted 152 lines · Alan Tull

Quoted 146 lines · Moritz Fischer

Quoted 135 lines · Alan Tull

Quoted 72 lines · Hao

Quoted 72 lines · Alan Tull

Quoted 34 lines · Wu Hao

Quoted 20 lines · Alan Tull

Quoted 9 lines

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.

Quoted 7 lines

* 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

Quoted 12 lines

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.

Quoted 8 lines

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:

Quoted 14 lines

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.

Quoted 3 lines

Do you think if having multiple fpga-* under one parent device is in the right direction?

Quoted 6 lines

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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 9 lines

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.

Quoted 4 lines

The above sounds like a poster-child application for MFD. If you do it in a clever
way (i.e. write your platform drivers in a reusable way) you might be able to just reuse them on your next generation.

Quoted 5 lines

Yes, I played with that with some test code. I wrote some test code
that allocates dummy mgr and bridge devices using mfd_add_devices(). I didn't see any way of getting access to the devices after creating them. Maybe I'm missing something. Neither the dev nor the platform_device is saved in the cell struct.

Currently working on some MFD stuff for an RTC, which device are you trying to get to?

The parent from subdevice (fpga mgr?) you can get by something like:

foo_fpga_mgr_probe(struct platform_device *pdev)

{
    struct foo_parent *parent = dev_get_drvdata(dev->parent);
}

Or are you talking about the other way round (i.e. get access to children from parent device) ?

Which functionality would that achieve?

Cheers,

Moritz

Quoted 166 lines · Moritz Fischer

Hi Alan,

Quoted 152 lines · Alan Tull

Quoted 146 lines · Moritz Fischer

Quoted 135 lines · Alan Tull

Quoted 72 lines · Hao

Quoted 72 lines · Alan Tull

Quoted 34 lines · Wu Hao

Quoted 20 lines · Alan Tull

Quoted 9 lines

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.

Quoted 7 lines

* 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

Quoted 12 lines

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.

Quoted 8 lines

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:

Quoted 14 lines

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.

Quoted 3 lines

Do you think if having multiple fpga-* under one parent device is in the right direction?

Quoted 6 lines

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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 9 lines

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.

Quoted 4 lines

The above sounds like a poster-child application for MFD. If you do it in a clever
way (i.e. write your platform drivers in a reusable way) you might be able to just reuse them on your next generation.

Quoted 5 lines

Yes, I played with that with some test code. I wrote some test code
that allocates dummy mgr and bridge devices using mfd_add_devices(). I didn't see any way of getting access to the devices after creating them. Maybe I'm missing something. Neither the dev nor the platform_device is saved in the cell struct.

Currently working on some MFD stuff for an RTC, which device are you trying to get to?

The parent from subdevice (fpga mgr?) you can get by something like:

Quoted 7 lines

foo_fpga_mgr_probe(struct platform_device *pdev)

{
    struct foo_parent *parent = dev_get_drvdata(dev->parent);
}

Or are you talking about the other way round (i.e. get access to children from parent device) ?

That's it.

Quoted 1 line

Which functionality would that achieve?

Suppose someone (non-DT case) is enumerating some hardware in the FPGA that includes a mgr and some bridges. So they create the mgr device. Then for each bridge and they want to create a bridge device and a region device and let the region know what mgr and bridge to use. The main enumerating device keeps track of all these regions so if stuff gets unloaded, it can destroy it all properly.

Quoted 72 lines · Hao

Quoted 72 lines · Alan Tull

Quoted 34 lines · Wu Hao

Quoted 20 lines · Alan Tull

Quoted 9 lines

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.

Quoted 7 lines

* 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

Quoted 12 lines

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.

Quoted 8 lines

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:

Quoted 14 lines

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.

Quoted 3 lines

Do you think if having multiple fpga-* under one parent device is in the right direction?

Quoted 6 lines

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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Remove of_node_get/put in fpga_region_get/put. Not needed and will get in the way when I separate out the common FPGA region code from Device Tree support code.

Signed-off-by Alan Tull <[email protected]>

---
v2: split out from another patch

---
 drivers/fpga/fpga-region.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/fpga/fpga-region.c +0 -4

index eaea9d4..6784357 100644
@@ -94,9 +94,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region)
        }
 
        get_device(dev);
-       of_node_get(dev->of_node);
        if (!try_module_get(dev->parent->driver->owner)) {
-               of_node_put(dev->of_node);
                put_device(dev);
                mutex_unlock(&region->mutex);
                return ERR_PTR(-ENODEV);

@@ -119,7 +117,6 @@ static void fpga_region_put(struct fpga_region *region)
        dev_dbg(dev, "put\n");
 
        module_put(dev->parent->driver->owner);
-       of_node_put(dev->of_node);
        put_device(dev);
        mutex_unlock(&region->mutex);
 }
--

2.7.4

During a device tree overlay pre-apply notification, the check for child FPGA regions can happen slightly earlier. This saves us from allocating the FPGA image info that just gets thrown away.

This is a baby step in refactoring the FPGA region code to separate out common FPGA region code from FPGA region Device Tree overlay support.

Signed-off-by Alan Tull <[email protected]>

---
v2: split out from another patch

---
 drivers/fpga/fpga-region.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

drivers/fpga/fpga-region.c +9 -6

index 7ff5426..c2730a8 100644
@@ -355,15 +355,19 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
        const char *firmware_name;
        int ret;
 
-       info = fpga_image_info_alloc(&region->dev);
-       if (!info)
-               return -ENOMEM;
-
-       /* Reject overlay if child FPGA Regions have firmware-name property */
+       /*
+        * Reject overlay if child FPGA Regions added in the overlay have
+        * firmware-name property (would mean that an FPGA region that has
+        * not been added to the live tree yet is doing FPGA programming).
+        */
        ret = child_regions_with_firmware(nd->overlay);
        if (ret)
                return ret;
 
+       info = fpga_image_info_alloc(dev);
+       if (!info)
+               return -ENOMEM;
+
        /* Read FPGA region properties from the overlay */
        if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
                info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
--

2.7.4

Quoted 5 lines · Alan Tull

During a device tree overlay pre-apply notification, the check for child FPGA regions can happen slightly earlier. This saves us from allocating the FPGA image info that just gets thrown away.

Quoted 5 lines

This is a baby step in refactoring the FPGA region code to separate out common FPGA region code from FPGA region Device Tree overlay support.

Signed-off-by Alan Tull <[email protected]>

Acked-by Moritz Fischer <[email protected]>

Use FPGA image info as a parameter when region code is programming the FPGA.

This is a baby step in refactoring the FPGA region code to separate out common FPGA region code from FPGA region Device Tree overlay support.

Signed-off-by Alan Tull <[email protected]>

---
v2: split out from another patch

---
 drivers/fpga/fpga-region.c    | 26 ++++++++++++++++----------
 include/linux/fpga/fpga-mgr.h |  4 ++++
 2 files changed, 20 insertions(+), 10 deletions(-)

drivers/fpga/fpga-region.c +2 -3

index d6f2313..1892126 100644
@@ -223,16 +223,21 @@ static int fpga_region_get_bridges(struct fpga_region *region,
 /**
  * fpga_region_program_fpga - program FPGA
  * @region: FPGA region
- * @overlay: device node of the overlay
- * Program an FPGA using information in the region's fpga image info.
+ * @info: FPGA image info
+ * Program an FPGA using fpga image info.
  * Return 0 for success or negative error code.
  */
 static int fpga_region_program_fpga(struct fpga_region *region,
-                                   struct device_node *overlay)

drivers/fpga/fpga-region.c +8 -7

        region = fpga_region_get(region);
        if (IS_ERR(region)) {
                dev_err(dev, "failed to get FPGA region\n");

@@ -245,7 +250,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
                goto err_put_region;
        }
 
-       ret = fpga_region_get_bridges(region, overlay);
+       ret = fpga_region_get_bridges(region, info->overlay);
        if (ret) {
                dev_err(dev, "failed to get FPGA bridges\n");
                goto err_unlock_mgr;

@@ -257,7 +262,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
                goto err_put_br;
        }
 
-       ret = fpga_mgr_load(region->mgr, region->info);
+       ret = fpga_mgr_load(region->mgr, info);
        if (ret) {
                dev_err(dev, "failed to load FPGA image\n");
                goto err_put_br;

@@ -269,6 +274,8 @@ static int fpga_region_program_fpga(struct fpga_region *region,
                goto err_put_br;
        }
 
+       region->info = info;
+
        fpga_mgr_unlock(region->mgr);
        fpga_region_put(region);
 

@@ -373,6 +380,8 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
        if (!info)
                return -ENOMEM;
 
+       info->overlay = nd->overlay;
+
        /* Read FPGA region properties from the overlay */
        if (of_property_read_bool(nd->overlay, "partial-fpga-config"))
                info->flags |= FPGA_MGR_PARTIAL_RECONFIG;

@@ -420,12 +429,9 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
                return -EINVAL;
        }
 
-       region->info = info;
-       ret = fpga_region_program_fpga(region, nd->overlay);
-       if (ret) {
+       ret = fpga_region_program_fpga(region, info);
+       if (ret)
                fpga_image_info_free(&region->dev, info);
-               region->info = NULL;
-       }
 
        return ret;
 }

include/linux/fpga/fpga-mgr.h +4 -1

index 4e14bdb..66d0e32 100644
@@ -84,6 +84,7 @@ enum fpga_mgr_states {
  * @sgt: scatter/gather table containing FPGA image
  * @buf: contiguous buffer containing FPGA image
  * @count: size of buf
+ * @overlay: Device Tree overlay
  */
 struct fpga_image_info {
        u32 flags;

@@ -94,6 +95,9 @@ struct fpga_image_info {
        struct sg_table *sgt;
        const char *buf;
        size_t count;
+#ifdef CONFIG_OF
+       struct device_node *overlay;
+#endif
 };
 
 /**
--

2.7.4

Add a function for searching the fpga-region class. This will be useful when device tree code is no longer in the same file that declares the fpga-region class. Another step in separating common FPGA region code from device tree support.

Signed-off-by Alan Tull <[email protected]>

---
v2: split out from another patch

---
 drivers/fpga/fpga-region.c       | 23 +++++++++++++++--------
 include/linux/fpga/fpga-region.h |  4 ++++
 2 files changed, 19 insertions(+), 8 deletions(-)

drivers/fpga/fpga-region.c +15 -8

index e2a3fe6..7ffb8c1 100644
@@ -30,6 +30,20 @@
 static DEFINE_IDA(fpga_region_ida);
 static struct class *fpga_region_class;
 
+struct fpga_region *fpga_region_class_find(
+       struct device *start, const void *data,
+       int (*match)(struct device *, const void *))
+{
+       struct device *dev;
+
+       dev = class_find_device(fpga_region_class, start, data, match);
+       if (!dev)
+               return NULL;
+
+       return to_fpga_region(dev);
+}
+EXPORT_SYMBOL_GPL(fpga_region_class_find);
+
 static const struct of_device_id fpga_region_of_match[] = {
        { .compatible = "fpga-region", },
        {},

@@ -51,14 +65,7 @@ static int fpga_region_of_node_match(struct device *dev, const void *data)
  */
 static struct fpga_region *of_fpga_region_find(struct device_node *np)
 {
-       struct device *dev;
-
-       dev = class_find_device(fpga_region_class, NULL, np,
-                               fpga_region_of_node_match);
-       if (!dev)
-               return NULL;
-
-       return to_fpga_region(dev);
+       return fpga_region_class_find(NULL, np, fpga_region_of_node_match);
 }
 
 /**

include/linux/fpga/fpga-region.h +4 -1

index 35e7e09..4966bb2 100644
@@ -28,6 +28,10 @@ struct fpga_region {
 
 #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
 
+struct fpga_region *fpga_region_class_find(
+       struct device *start, const void *data,
+       int (*match)(struct device *, const void *));
+
 int fpga_region_program_fpga(struct fpga_region *region,
                             struct fpga_image_info *image_info);
 
--

2.7.4

Quoted 7 lines · Alan Tull

Add a function for searching the fpga-region class. This will be useful when device tree code is no longer in the same file that declares the fpga-region class. Another step in separating common FPGA region code from device tree support.

Signed-off-by Alan Tull <[email protected]>

Acked-by Moritz Fischer <[email protected]>

Quoted 66 lines

---
v2: split out from another patch

---
 drivers/fpga/fpga-region.c       | 23 +++++++++++++++--------
 include/linux/fpga/fpga-region.h |  4 ++++
 2 files changed, 19 insertions(+), 8 deletions(-)

drivers/fpga/fpga-region.c +15 -8

index e2a3fe6..7ffb8c1 100644
@@ -30,6 +30,20 @@
 static DEFINE_IDA(fpga_region_ida);
 static struct class *fpga_region_class;
 
+struct fpga_region *fpga_region_class_find(
+       struct device *start, const void *data,
+       int (*match)(struct device *, const void *))
+{
+       struct device *dev;
+
+       dev = class_find_device(fpga_region_class, start, data, match);
+       if (!dev)
+               return NULL;
+
+       return to_fpga_region(dev);
+}
+EXPORT_SYMBOL_GPL(fpga_region_class_find);
+
 static const struct of_device_id fpga_region_of_match[] = {
        { .compatible = "fpga-region", },
        {},

@@ -51,14 +65,7 @@ static int fpga_region_of_node_match(struct device *dev, const void *data)
  */
 static struct fpga_region *of_fpga_region_find(struct device_node *np)
 {
-       struct device *dev;
-
-       dev = class_find_device(fpga_region_class, NULL, np,
-                               fpga_region_of_node_match);
-       if (!dev)
-               return NULL;
-
-       return to_fpga_region(dev);
+       return fpga_region_class_find(NULL, np, fpga_region_of_node_match);
 }
 
 /**

include/linux/fpga/fpga-region.h +4 -1

index 35e7e09..4966bb2 100644
@@ -28,6 +28,10 @@ struct fpga_region {
 
 #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
 
+struct fpga_region *fpga_region_class_find(
+       struct device *start, const void *data,
+       int (*match)(struct device *, const void *));
+
 int fpga_region_program_fpga(struct fpga_region *region,
                             struct fpga_image_info *image_info);
 
--

2.7.4

Create of-fpga-region.c

Move the following functions without modification from fpga-region.c to of-fpga-region.c:

* of_fpga_region_find
* of_fpga_region_get_mgr
* of_fpga_region_get_bridges
* child_regions_with_firmware
* of_fpga_region_parse_ov
* of_fpga_region_notify_pre_apply
* of_fpga_region_notify_post_remove
* of_fpga_region_notify
* of_fpga_region_probe
* of_fpga_region_remove

Create two new function with some code from fpga_region_init/exit.

* of_fpga_region_init
* of_fpga_region_exit

Signed-off-by Alan Tull <[email protected]>

---
v2: split out code changes into other patches, only move code here

---
 drivers/fpga/Kconfig          |  13 +-
 drivers/fpga/Makefile         |   1 +
 drivers/fpga/fpga-region.c    | 449 +-------------------------------------
 drivers/fpga/of-fpga-region.c | 493 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 505 insertions(+), 451 deletions(-)
 create mode 100644 drivers/fpga/of-fpga-region.c

drivers/fpga/Kconfig +0 -0

index 161ba9d..394c141 100644

Quoted 37 lines · Alan Tull

Create of-fpga-region.c

Move the following functions without modification from fpga-region.c to of-fpga-region.c:

* of_fpga_region_find
* of_fpga_region_get_mgr
* of_fpga_region_get_bridges
* child_regions_with_firmware
* of_fpga_region_parse_ov
* of_fpga_region_notify_pre_apply
* of_fpga_region_notify_post_remove
* of_fpga_region_notify
* of_fpga_region_probe
* of_fpga_region_remove

Create two new function with some code from fpga_region_init/exit.

* of_fpga_region_init
* of_fpga_region_exit

Signed-off-by Alan Tull <[email protected]>

---
v2: split out code changes into other patches, only move code here

---
 drivers/fpga/Kconfig          |  13 +-
 drivers/fpga/Makefile         |   1 +
 drivers/fpga/fpga-region.c    | 449 +-------------------------------------
 drivers/fpga/of-fpga-region.c | 493 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 505 insertions(+), 451 deletions(-)
 create mode 100644 drivers/fpga/of-fpga-region.c

drivers/fpga/Kconfig +0 -0

index 161ba9d..394c141 100644

drivers/fpga/Kconfig if FPGA · line 23

Quoted 10 lines

drivers/fpga/Kconfig +6 -1

@@ -15,10 +15,17 @@ if FPGA
 
 config FPGA_REGION
      tristate "FPGA Region"
-     depends on OF && FPGA_BRIDGE
+     depends on FPGA_BRIDGE
+     help
+       FPGA Region common code.  A FPGA Region controls a FPGA Manager
+       and the FPGA Bridges associated with either a reconfigurable
+       region of an FPGA or a whole FPGA.
+

Hi Alan

As FPGA_BRIDGE depends on OF, so FPGA_REGION still can't be selected without OF. Should we remove the OF dependency for FPGA_BRIDGE as well?

Thanks
Hao

The current FPGA layer has two problems which this patchset addresses:

* We now have 3 functions for programming a FPGA, depending on whether the image is in a sg list, a buffer, or firmware. So callers have to decide where the image will be or have to write extra code to maintain flexibilty.

* users who aren't using device tree are left to write their own code that is essentially a rewrite of FPGA region.

The major things this patchset accomplishes is: