Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 16/02/2018 14:42, Andy Shevchenko wrote: On Thu, Feb 15, 2018 at 7:07 PM, John Garrywrote: On 14/02/2018 16:16, Andy Shevchenko wrote: + list_for_each_entry(rentry, _list, node) + resources[count++] = *rentry->res; It has similarities with acpi_create_platform_device(). I guess we can utilize existing code. For sure, this particular segment is effectively same as part of acpi_create_platform_device(): Not the same, acpi_create_platform_device() does a bit more than copying the resources. If it indeed makes no hurt... list_for_each_entry(rentry, _list, node) acpi_platform_fill_resource(adev, rentry->res, [count++]); So is your idea to refactor this common segment into a helper function? ...I would go with helper. Hi Andy, Since the plan now is that this code is no longer going to be added to drivers/acpi, but instead pushed to the LLDD, I am pondering whether we should still factor out of this common code. Opinion? I would still go with a common helper. Though, as first step, we can make it lazy, i.e. put a comment in your code, like a todo notice (w/o TODO word :-) ) to consider a common helper. Fine, I was also thinking that I don't want to do this now as it could make merging the patchset more complex. For now, the ACPI change I plan creates no dependencies. Cheers, John
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 16/02/2018 14:42, Andy Shevchenko wrote: On Thu, Feb 15, 2018 at 7:07 PM, John Garry wrote: On 14/02/2018 16:16, Andy Shevchenko wrote: + list_for_each_entry(rentry, _list, node) + resources[count++] = *rentry->res; It has similarities with acpi_create_platform_device(). I guess we can utilize existing code. For sure, this particular segment is effectively same as part of acpi_create_platform_device(): Not the same, acpi_create_platform_device() does a bit more than copying the resources. If it indeed makes no hurt... list_for_each_entry(rentry, _list, node) acpi_platform_fill_resource(adev, rentry->res, [count++]); So is your idea to refactor this common segment into a helper function? ...I would go with helper. Hi Andy, Since the plan now is that this code is no longer going to be added to drivers/acpi, but instead pushed to the LLDD, I am pondering whether we should still factor out of this common code. Opinion? I would still go with a common helper. Though, as first step, we can make it lazy, i.e. put a comment in your code, like a todo notice (w/o TODO word :-) ) to consider a common helper. Fine, I was also thinking that I don't want to do this now as it could make merging the patchset more complex. For now, the ACPI change I plan creates no dependencies. Cheers, John
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Thu, Feb 15, 2018 at 7:07 PM, John Garrywrote: > On 14/02/2018 16:16, Andy Shevchenko wrote: > >>> + list_for_each_entry(rentry, _list, node) > >>> + resources[count++] = *rentry->res; >> It has similarities with acpi_create_platform_device(). >> I guess we can utilize existing code. >>> >>> > For sure, this particular segment is effectively same as part of >>> > acpi_create_platform_device(): >> >> Not the same, acpi_create_platform_device() does a bit more than >> copying the resources. If it indeed makes no hurt... >> >>> > list_for_each_entry(rentry, _list, node) >>> > acpi_platform_fill_resource(adev, rentry->res, >>> > [count++]); >>> > So is your idea to refactor this common segment into a helper function? >> >> ...I would go with helper. >> > > Hi Andy, > > Since the plan now is that this code is no longer going to be added to > drivers/acpi, but instead pushed to the LLDD, I am pondering whether we > should still factor out of this common code. Opinion? I would still go with a common helper. Though, as first step, we can make it lazy, i.e. put a comment in your code, like a todo notice (w/o TODO word :-) ) to consider a common helper. -- With Best Regards, Andy Shevchenko
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Thu, Feb 15, 2018 at 7:07 PM, John Garry wrote: > On 14/02/2018 16:16, Andy Shevchenko wrote: > >>> + list_for_each_entry(rentry, _list, node) > >>> + resources[count++] = *rentry->res; >> It has similarities with acpi_create_platform_device(). >> I guess we can utilize existing code. >>> >>> > For sure, this particular segment is effectively same as part of >>> > acpi_create_platform_device(): >> >> Not the same, acpi_create_platform_device() does a bit more than >> copying the resources. If it indeed makes no hurt... >> >>> > list_for_each_entry(rentry, _list, node) >>> > acpi_platform_fill_resource(adev, rentry->res, >>> > [count++]); >>> > So is your idea to refactor this common segment into a helper function? >> >> ...I would go with helper. >> > > Hi Andy, > > Since the plan now is that this code is no longer going to be added to > drivers/acpi, but instead pushed to the LLDD, I am pondering whether we > should still factor out of this common code. Opinion? I would still go with a common helper. Though, as first step, we can make it lazy, i.e. put a comment in your code, like a todo notice (w/o TODO word :-) ) to consider a common helper. -- With Best Regards, Andy Shevchenko
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 14/02/2018 16:16, Andy Shevchenko wrote: Another approach is to use ~0UL if that is preferable. >>> + list_for_each_entry(rentry, _list, node) >>> + resources[count++] = *rentry->res; >> It has similarities with acpi_create_platform_device(). >> I guess we can utilize existing code. > For sure, this particular segment is effectively same as part of > acpi_create_platform_device(): Not the same, acpi_create_platform_device() does a bit more than copying the resources. If it indeed makes no hurt... > list_for_each_entry(rentry, _list, node) > acpi_platform_fill_resource(adev, rentry->res, > [count++]); > So is your idea to refactor this common segment into a helper function? ...I would go with helper. Hi Andy, Since the plan now is that this code is no longer going to be added to drivers/acpi, but instead pushed to the LLDD, I am pondering whether we should still factor out of this common code. Opinion? Thanks, John >>> + char *name = _indirect_io_mfd_cell[count].name[0];
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 14/02/2018 16:16, Andy Shevchenko wrote: Another approach is to use ~0UL if that is preferable. >>> + list_for_each_entry(rentry, _list, node) >>> + resources[count++] = *rentry->res; >> It has similarities with acpi_create_platform_device(). >> I guess we can utilize existing code. > For sure, this particular segment is effectively same as part of > acpi_create_platform_device(): Not the same, acpi_create_platform_device() does a bit more than copying the resources. If it indeed makes no hurt... > list_for_each_entry(rentry, _list, node) > acpi_platform_fill_resource(adev, rentry->res, > [count++]); > So is your idea to refactor this common segment into a helper function? ...I would go with helper. Hi Andy, Since the plan now is that this code is no longer going to be added to drivers/acpi, but instead pushed to the LLDD, I am pondering whether we should still factor out of this common code. Opinion? Thanks, John >>> + char *name = _indirect_io_mfd_cell[count].name[0];
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 15/02/2018 11:47, Rafael J. Wysocki wrote: On Thu, Feb 15, 2018 at 12:19 PM, John Garrywrote: Nothing apart from only being used by arm64 platforms today, which is circumstantial. I understand you need to find a place to add the: acpi_indirect_io_scan_init() to be called from core ACPI code because ACPI can't handle probe dependencies in any other way but other than that this patch is a Hisilicon ACPI driver - there is nothing generic in it (or at least there are no standard bindings to make it so). Whether a callback from ACPI core code (acpi_scan_init()) to a driver specific hook is sane or not that's the question and the only reason why you want to add this in drivers/acpi/arm64 rather than, say, drivers/bus (as you do for the DT driver). I do not know Rafael's opinion on the above, I would like to help you make forward progress but please understand my concerns, mostly on FW side. I did mention an alternative in my "ping" in v12 patch 7/9 (Feb 1), but no response to this specific note so I kept on the same path. Here's what I then wrote: "I think another solution - which you may prefer - is to avoid adding this scan handler (and all this other scan code) and add a check like acpi_is_serial_bus_slave() [which checks the device parent versus a list of known indirectIO hosts] to not enumerate these children, and do it from the LLDD host probe instead (https://lkml.org/lkml/2017/6/16/250)" Hi Rafael, Lorenzo, I can avoid adding the scan handler in acpi_indirectio.c by skipping the child enumeration, like with this change in scan.c: Hi Rafael, +static const struct acpi_device_id indirect_io_hosts[] = { +{"HISI0191", 0},/* HiSilicon LPC host */ +{}, +}; + +static bool acpi_is_indirect_io_slave(struct acpi_device *device) +{ Why don't you put the table definition here? I can do. +struct acpi_device *parent = dev->parent; + +if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) +return false; + +return true; return parent && !acpi_match_device_ids(parent, indirect_io_hosts); Fine, a bit more concise +} + static bool acpi_is_serial_bus_slave(struct acpi_device *device) { struct list_head resource_list; bool is_serial_bus_slave = false; +if (acpi_is_indirect_io_slave(device)) +return true; + /* Macs use device properties in lieu of _CRS resources */ This means I can move all this scan code into the LLDD. What do you think? Please let me know. If Lorenzo agrees, that will be fine by me modulo the above remarks. . I see Lorenzo also finds this ok, so I'll go with that. Thanks to all, John
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 15/02/2018 11:47, Rafael J. Wysocki wrote: On Thu, Feb 15, 2018 at 12:19 PM, John Garry wrote: Nothing apart from only being used by arm64 platforms today, which is circumstantial. I understand you need to find a place to add the: acpi_indirect_io_scan_init() to be called from core ACPI code because ACPI can't handle probe dependencies in any other way but other than that this patch is a Hisilicon ACPI driver - there is nothing generic in it (or at least there are no standard bindings to make it so). Whether a callback from ACPI core code (acpi_scan_init()) to a driver specific hook is sane or not that's the question and the only reason why you want to add this in drivers/acpi/arm64 rather than, say, drivers/bus (as you do for the DT driver). I do not know Rafael's opinion on the above, I would like to help you make forward progress but please understand my concerns, mostly on FW side. I did mention an alternative in my "ping" in v12 patch 7/9 (Feb 1), but no response to this specific note so I kept on the same path. Here's what I then wrote: "I think another solution - which you may prefer - is to avoid adding this scan handler (and all this other scan code) and add a check like acpi_is_serial_bus_slave() [which checks the device parent versus a list of known indirectIO hosts] to not enumerate these children, and do it from the LLDD host probe instead (https://lkml.org/lkml/2017/6/16/250)" Hi Rafael, Lorenzo, I can avoid adding the scan handler in acpi_indirectio.c by skipping the child enumeration, like with this change in scan.c: Hi Rafael, +static const struct acpi_device_id indirect_io_hosts[] = { +{"HISI0191", 0},/* HiSilicon LPC host */ +{}, +}; + +static bool acpi_is_indirect_io_slave(struct acpi_device *device) +{ Why don't you put the table definition here? I can do. +struct acpi_device *parent = dev->parent; + +if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) +return false; + +return true; return parent && !acpi_match_device_ids(parent, indirect_io_hosts); Fine, a bit more concise +} + static bool acpi_is_serial_bus_slave(struct acpi_device *device) { struct list_head resource_list; bool is_serial_bus_slave = false; +if (acpi_is_indirect_io_slave(device)) +return true; + /* Macs use device properties in lieu of _CRS resources */ This means I can move all this scan code into the LLDD. What do you think? Please let me know. If Lorenzo agrees, that will be fine by me modulo the above remarks. . I see Lorenzo also finds this ok, so I'll go with that. Thanks to all, John
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Thu, Feb 15, 2018 at 2:52 PM, John Garrywrote: > On 15/02/2018 12:22, Andy Shevchenko wrote: >> On Thu, Feb 15, 2018 at 1:19 PM, John Garry wrote: >>> +static const struct acpi_device_id indirect_io_hosts[] = { >>> +{"HISI0191", 0},/* HiSilicon LPC host */ >>> +{}, >> >> >> Just a nit. >> > > I noticed that I have this in the LLDD also. I think that this may be a > force of habit. >> It seems lately this happens more often than usual, I mean a comma in >> the terminator line. >> >> If we remove it. we terminate not only at runtime, but at compile >> time, which is slightly better. > > > I grepped for "{}," in the drivers folder and it gives many results (I do > accept that some are not sentinels): Yes, in old code it might be, but why to cargo-culting bad (okay, less worth) practices? >>grep -R "{}," * | tail Btw, `git grep ...` is much faster. > watchdog/gef_wdt.c:{}, > watchdog/asm9260_wdt.c:{}, > watchdog/bcm2835_wdt.c:{}, > watchdog/digicolor_wdt.c:{}, > watchdog/bcm7038_wdt.c:{}, > watchdog/ath79_wdt.c:{}, > watchdog/riowd.c:{}, > watchdog/sbsa_gwdt.c:{}, > watchdog/sbsa_gwdt.c:{}, > watchdog/bcm_kona_wdt.c:{}, -- With Best Regards, Andy Shevchenko
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Thu, Feb 15, 2018 at 2:52 PM, John Garry wrote: > On 15/02/2018 12:22, Andy Shevchenko wrote: >> On Thu, Feb 15, 2018 at 1:19 PM, John Garry wrote: >>> +static const struct acpi_device_id indirect_io_hosts[] = { >>> +{"HISI0191", 0},/* HiSilicon LPC host */ >>> +{}, >> >> >> Just a nit. >> > > I noticed that I have this in the LLDD also. I think that this may be a > force of habit. >> It seems lately this happens more often than usual, I mean a comma in >> the terminator line. >> >> If we remove it. we terminate not only at runtime, but at compile >> time, which is slightly better. > > > I grepped for "{}," in the drivers folder and it gives many results (I do > accept that some are not sentinels): Yes, in old code it might be, but why to cargo-culting bad (okay, less worth) practices? >>grep -R "{}," * | tail Btw, `git grep ...` is much faster. > watchdog/gef_wdt.c:{}, > watchdog/asm9260_wdt.c:{}, > watchdog/bcm2835_wdt.c:{}, > watchdog/digicolor_wdt.c:{}, > watchdog/bcm7038_wdt.c:{}, > watchdog/ath79_wdt.c:{}, > watchdog/riowd.c:{}, > watchdog/sbsa_gwdt.c:{}, > watchdog/sbsa_gwdt.c:{}, > watchdog/bcm_kona_wdt.c:{}, -- With Best Regards, Andy Shevchenko
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 15/02/2018 12:22, Andy Shevchenko wrote: On Thu, Feb 15, 2018 at 1:19 PM, John Garrywrote: Hi Andy, +static const struct acpi_device_id indirect_io_hosts[] = { +{"HISI0191", 0},/* HiSilicon LPC host */ +{}, Just a nit. I noticed that I have this in the LLDD also. I think that this may be a force of habit. It seems lately this happens more often than usual, I mean a comma in the terminator line. If we remove it. we terminate not only at runtime, but at compile time, which is slightly better. I grepped for "{}," in the drivers folder and it gives many results (I do accept that some are not sentinels): >grep -R "{}," * | tail watchdog/gef_wdt.c:{}, watchdog/asm9260_wdt.c:{}, watchdog/bcm2835_wdt.c:{}, watchdog/digicolor_wdt.c:{}, watchdog/bcm7038_wdt.c:{}, watchdog/ath79_wdt.c:{}, watchdog/riowd.c:{}, watchdog/sbsa_gwdt.c:{}, watchdog/sbsa_gwdt.c:{}, watchdog/bcm_kona_wdt.c:{}, Much appreciated, John
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 15/02/2018 12:22, Andy Shevchenko wrote: On Thu, Feb 15, 2018 at 1:19 PM, John Garry wrote: Hi Andy, +static const struct acpi_device_id indirect_io_hosts[] = { +{"HISI0191", 0},/* HiSilicon LPC host */ +{}, Just a nit. I noticed that I have this in the LLDD also. I think that this may be a force of habit. It seems lately this happens more often than usual, I mean a comma in the terminator line. If we remove it. we terminate not only at runtime, but at compile time, which is slightly better. I grepped for "{}," in the drivers folder and it gives many results (I do accept that some are not sentinels): >grep -R "{}," * | tail watchdog/gef_wdt.c:{}, watchdog/asm9260_wdt.c:{}, watchdog/bcm2835_wdt.c:{}, watchdog/digicolor_wdt.c:{}, watchdog/bcm7038_wdt.c:{}, watchdog/ath79_wdt.c:{}, watchdog/riowd.c:{}, watchdog/sbsa_gwdt.c:{}, watchdog/sbsa_gwdt.c:{}, watchdog/bcm_kona_wdt.c:{}, Much appreciated, John
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Thu, Feb 15, 2018 at 12:47:25PM +0100, Rafael J. Wysocki wrote: > On Thu, Feb 15, 2018 at 12:19 PM, John Garrywrote: > >> Nothing apart from only being used by arm64 platforms today, which is > >> circumstantial. > >> > >>> > >>> I understand you need to find a place to add the: > >>> > >>> acpi_indirect_io_scan_init() > >>> > >>> to be called from core ACPI code because ACPI can't handle probe > >>> dependencies in any other way but other than that this patch is > >>> a Hisilicon ACPI driver - there is nothing generic in it (or at > >>> least there are no standard bindings to make it so). > >>> > >>> Whether a callback from ACPI core code (acpi_scan_init()) to a driver > >>> specific hook is sane or not that's the question and the only reason > >>> why you want to add this in drivers/acpi/arm64 rather than, say, > >>> drivers/bus (as you do for the DT driver). > >>> > >>> I do not know Rafael's opinion on the above, I would like to help > >>> you make forward progress but please understand my concerns, mostly > >>> on FW side. > >>> > >> > >> I did mention an alternative in my "ping" in v12 patch 7/9 (Feb 1), but > >> no response to this specific note so I kept on the same path. > >> > >> Here's what I then wrote: > >> "I think another solution - which you may prefer - is to avoid adding > >> this scan handler (and all this other scan code) and add a check like > >> acpi_is_serial_bus_slave() [which checks the device parent versus a list > >> of known indirectIO hosts] to not enumerate these children, and do it > >> from the LLDD host probe instead (https://lkml.org/lkml/2017/6/16/250)" > >> > > > > Hi Rafael, Lorenzo, > > > > I can avoid adding the scan handler in acpi_indirectio.c by skipping the > > child enumeration, like with this change in scan.c: > > > > +static const struct acpi_device_id indirect_io_hosts[] = { > > +{"HISI0191", 0},/* HiSilicon LPC host */ > > +{}, > > +}; > > + > > +static bool acpi_is_indirect_io_slave(struct acpi_device *device) > > +{ > > Why don't you put the table definition here? > > > +struct acpi_device *parent = dev->parent; > > + > > +if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) > > +return false; > > + > > +return true; > > return parent && !acpi_match_device_ids(parent, indirect_io_hosts); > > > +} > > + > > static bool acpi_is_serial_bus_slave(struct acpi_device *device) > > { > > struct list_head resource_list; > > bool is_serial_bus_slave = false; > > > > +if (acpi_is_indirect_io_slave(device)) > > +return true; > > + > > /* Macs use device properties in lieu of _CRS resources */ > > > > > > This means I can move all this scan code into the LLDD. > > > > What do you think? Please let me know. > > If Lorenzo agrees, that will be fine by me modulo the above remarks. I agree and I thank you for accepting this in core ACPI code, I think that's much cleaner than a driver specific scan hook. It is a shame we do not have a generic identifier for such bus in ACPI but that won't happen overnight anyway (if ever, I think a binding for LPC in the ACPI specs is hard to justify). Thank you, Lorenzo
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Thu, Feb 15, 2018 at 12:47:25PM +0100, Rafael J. Wysocki wrote: > On Thu, Feb 15, 2018 at 12:19 PM, John Garry wrote: > >> Nothing apart from only being used by arm64 platforms today, which is > >> circumstantial. > >> > >>> > >>> I understand you need to find a place to add the: > >>> > >>> acpi_indirect_io_scan_init() > >>> > >>> to be called from core ACPI code because ACPI can't handle probe > >>> dependencies in any other way but other than that this patch is > >>> a Hisilicon ACPI driver - there is nothing generic in it (or at > >>> least there are no standard bindings to make it so). > >>> > >>> Whether a callback from ACPI core code (acpi_scan_init()) to a driver > >>> specific hook is sane or not that's the question and the only reason > >>> why you want to add this in drivers/acpi/arm64 rather than, say, > >>> drivers/bus (as you do for the DT driver). > >>> > >>> I do not know Rafael's opinion on the above, I would like to help > >>> you make forward progress but please understand my concerns, mostly > >>> on FW side. > >>> > >> > >> I did mention an alternative in my "ping" in v12 patch 7/9 (Feb 1), but > >> no response to this specific note so I kept on the same path. > >> > >> Here's what I then wrote: > >> "I think another solution - which you may prefer - is to avoid adding > >> this scan handler (and all this other scan code) and add a check like > >> acpi_is_serial_bus_slave() [which checks the device parent versus a list > >> of known indirectIO hosts] to not enumerate these children, and do it > >> from the LLDD host probe instead (https://lkml.org/lkml/2017/6/16/250)" > >> > > > > Hi Rafael, Lorenzo, > > > > I can avoid adding the scan handler in acpi_indirectio.c by skipping the > > child enumeration, like with this change in scan.c: > > > > +static const struct acpi_device_id indirect_io_hosts[] = { > > +{"HISI0191", 0},/* HiSilicon LPC host */ > > +{}, > > +}; > > + > > +static bool acpi_is_indirect_io_slave(struct acpi_device *device) > > +{ > > Why don't you put the table definition here? > > > +struct acpi_device *parent = dev->parent; > > + > > +if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) > > +return false; > > + > > +return true; > > return parent && !acpi_match_device_ids(parent, indirect_io_hosts); > > > +} > > + > > static bool acpi_is_serial_bus_slave(struct acpi_device *device) > > { > > struct list_head resource_list; > > bool is_serial_bus_slave = false; > > > > +if (acpi_is_indirect_io_slave(device)) > > +return true; > > + > > /* Macs use device properties in lieu of _CRS resources */ > > > > > > This means I can move all this scan code into the LLDD. > > > > What do you think? Please let me know. > > If Lorenzo agrees, that will be fine by me modulo the above remarks. I agree and I thank you for accepting this in core ACPI code, I think that's much cleaner than a driver specific scan hook. It is a shame we do not have a generic identifier for such bus in ACPI but that won't happen overnight anyway (if ever, I think a binding for LPC in the ACPI specs is hard to justify). Thank you, Lorenzo
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Thu, Feb 15, 2018 at 1:19 PM, John Garrywrote: > +static const struct acpi_device_id indirect_io_hosts[] = { > +{"HISI0191", 0},/* HiSilicon LPC host */ > +{}, Just a nit. It seems lately this happens more often than usual, I mean a comma in the terminator line. If we remove it. we terminate not only at runtime, but at compile time, which is slightly better. > +}; -- With Best Regards, Andy Shevchenko
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Thu, Feb 15, 2018 at 1:19 PM, John Garry wrote: > +static const struct acpi_device_id indirect_io_hosts[] = { > +{"HISI0191", 0},/* HiSilicon LPC host */ > +{}, Just a nit. It seems lately this happens more often than usual, I mean a comma in the terminator line. If we remove it. we terminate not only at runtime, but at compile time, which is slightly better. > +}; -- With Best Regards, Andy Shevchenko
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Thu, Feb 15, 2018 at 12:19 PM, John Garrywrote: >> Nothing apart from only being used by arm64 platforms today, which is >> circumstantial. >> >>> >>> I understand you need to find a place to add the: >>> >>> acpi_indirect_io_scan_init() >>> >>> to be called from core ACPI code because ACPI can't handle probe >>> dependencies in any other way but other than that this patch is >>> a Hisilicon ACPI driver - there is nothing generic in it (or at >>> least there are no standard bindings to make it so). >>> >>> Whether a callback from ACPI core code (acpi_scan_init()) to a driver >>> specific hook is sane or not that's the question and the only reason >>> why you want to add this in drivers/acpi/arm64 rather than, say, >>> drivers/bus (as you do for the DT driver). >>> >>> I do not know Rafael's opinion on the above, I would like to help >>> you make forward progress but please understand my concerns, mostly >>> on FW side. >>> >> >> I did mention an alternative in my "ping" in v12 patch 7/9 (Feb 1), but >> no response to this specific note so I kept on the same path. >> >> Here's what I then wrote: >> "I think another solution - which you may prefer - is to avoid adding >> this scan handler (and all this other scan code) and add a check like >> acpi_is_serial_bus_slave() [which checks the device parent versus a list >> of known indirectIO hosts] to not enumerate these children, and do it >> from the LLDD host probe instead (https://lkml.org/lkml/2017/6/16/250)" >> > > Hi Rafael, Lorenzo, > > I can avoid adding the scan handler in acpi_indirectio.c by skipping the > child enumeration, like with this change in scan.c: > > +static const struct acpi_device_id indirect_io_hosts[] = { > +{"HISI0191", 0},/* HiSilicon LPC host */ > +{}, > +}; > + > +static bool acpi_is_indirect_io_slave(struct acpi_device *device) > +{ Why don't you put the table definition here? > +struct acpi_device *parent = dev->parent; > + > +if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) > +return false; > + > +return true; return parent && !acpi_match_device_ids(parent, indirect_io_hosts); > +} > + > static bool acpi_is_serial_bus_slave(struct acpi_device *device) > { > struct list_head resource_list; > bool is_serial_bus_slave = false; > > +if (acpi_is_indirect_io_slave(device)) > +return true; > + > /* Macs use device properties in lieu of _CRS resources */ > > > This means I can move all this scan code into the LLDD. > > What do you think? Please let me know. If Lorenzo agrees, that will be fine by me modulo the above remarks.
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Thu, Feb 15, 2018 at 12:19 PM, John Garry wrote: >> Nothing apart from only being used by arm64 platforms today, which is >> circumstantial. >> >>> >>> I understand you need to find a place to add the: >>> >>> acpi_indirect_io_scan_init() >>> >>> to be called from core ACPI code because ACPI can't handle probe >>> dependencies in any other way but other than that this patch is >>> a Hisilicon ACPI driver - there is nothing generic in it (or at >>> least there are no standard bindings to make it so). >>> >>> Whether a callback from ACPI core code (acpi_scan_init()) to a driver >>> specific hook is sane or not that's the question and the only reason >>> why you want to add this in drivers/acpi/arm64 rather than, say, >>> drivers/bus (as you do for the DT driver). >>> >>> I do not know Rafael's opinion on the above, I would like to help >>> you make forward progress but please understand my concerns, mostly >>> on FW side. >>> >> >> I did mention an alternative in my "ping" in v12 patch 7/9 (Feb 1), but >> no response to this specific note so I kept on the same path. >> >> Here's what I then wrote: >> "I think another solution - which you may prefer - is to avoid adding >> this scan handler (and all this other scan code) and add a check like >> acpi_is_serial_bus_slave() [which checks the device parent versus a list >> of known indirectIO hosts] to not enumerate these children, and do it >> from the LLDD host probe instead (https://lkml.org/lkml/2017/6/16/250)" >> > > Hi Rafael, Lorenzo, > > I can avoid adding the scan handler in acpi_indirectio.c by skipping the > child enumeration, like with this change in scan.c: > > +static const struct acpi_device_id indirect_io_hosts[] = { > +{"HISI0191", 0},/* HiSilicon LPC host */ > +{}, > +}; > + > +static bool acpi_is_indirect_io_slave(struct acpi_device *device) > +{ Why don't you put the table definition here? > +struct acpi_device *parent = dev->parent; > + > +if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) > +return false; > + > +return true; return parent && !acpi_match_device_ids(parent, indirect_io_hosts); > +} > + > static bool acpi_is_serial_bus_slave(struct acpi_device *device) > { > struct list_head resource_list; > bool is_serial_bus_slave = false; > > +if (acpi_is_indirect_io_slave(device)) > +return true; > + > /* Macs use device properties in lieu of _CRS resources */ > > > This means I can move all this scan code into the LLDD. > > What do you think? Please let me know. If Lorenzo agrees, that will be fine by me modulo the above remarks.
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
Nothing apart from only being used by arm64 platforms today, which is circumstantial. I understand you need to find a place to add the: acpi_indirect_io_scan_init() to be called from core ACPI code because ACPI can't handle probe dependencies in any other way but other than that this patch is a Hisilicon ACPI driver - there is nothing generic in it (or at least there are no standard bindings to make it so). Whether a callback from ACPI core code (acpi_scan_init()) to a driver specific hook is sane or not that's the question and the only reason why you want to add this in drivers/acpi/arm64 rather than, say, drivers/bus (as you do for the DT driver). I do not know Rafael's opinion on the above, I would like to help you make forward progress but please understand my concerns, mostly on FW side. I did mention an alternative in my "ping" in v12 patch 7/9 (Feb 1), but no response to this specific note so I kept on the same path. Here's what I then wrote: "I think another solution - which you may prefer - is to avoid adding this scan handler (and all this other scan code) and add a check like acpi_is_serial_bus_slave() [which checks the device parent versus a list of known indirectIO hosts] to not enumerate these children, and do it from the LLDD host probe instead (https://lkml.org/lkml/2017/6/16/250)" Hi Rafael, Lorenzo, I can avoid adding the scan handler in acpi_indirectio.c by skipping the child enumeration, like with this change in scan.c: +static const struct acpi_device_id indirect_io_hosts[] = { +{"HISI0191", 0},/* HiSilicon LPC host */ +{}, +}; + +static bool acpi_is_indirect_io_slave(struct acpi_device *device) +{ +struct acpi_device *parent = dev->parent; + +if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) +return false; + +return true; +} + static bool acpi_is_serial_bus_slave(struct acpi_device *device) { struct list_head resource_list; bool is_serial_bus_slave = false; +if (acpi_is_indirect_io_slave(device)) +return true; + /* Macs use device properties in lieu of _CRS resources */ This means I can move all this scan code into the LLDD. What do you think? Please let me know. John
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
Nothing apart from only being used by arm64 platforms today, which is circumstantial. I understand you need to find a place to add the: acpi_indirect_io_scan_init() to be called from core ACPI code because ACPI can't handle probe dependencies in any other way but other than that this patch is a Hisilicon ACPI driver - there is nothing generic in it (or at least there are no standard bindings to make it so). Whether a callback from ACPI core code (acpi_scan_init()) to a driver specific hook is sane or not that's the question and the only reason why you want to add this in drivers/acpi/arm64 rather than, say, drivers/bus (as you do for the DT driver). I do not know Rafael's opinion on the above, I would like to help you make forward progress but please understand my concerns, mostly on FW side. I did mention an alternative in my "ping" in v12 patch 7/9 (Feb 1), but no response to this specific note so I kept on the same path. Here's what I then wrote: "I think another solution - which you may prefer - is to avoid adding this scan handler (and all this other scan code) and add a check like acpi_is_serial_bus_slave() [which checks the device parent versus a list of known indirectIO hosts] to not enumerate these children, and do it from the LLDD host probe instead (https://lkml.org/lkml/2017/6/16/250)" Hi Rafael, Lorenzo, I can avoid adding the scan handler in acpi_indirectio.c by skipping the child enumeration, like with this change in scan.c: +static const struct acpi_device_id indirect_io_hosts[] = { +{"HISI0191", 0},/* HiSilicon LPC host */ +{}, +}; + +static bool acpi_is_indirect_io_slave(struct acpi_device *device) +{ +struct acpi_device *parent = dev->parent; + +if (!parent || acpi_match_device_ids(parent, indirect_io_hosts)) +return false; + +return true; +} + static bool acpi_is_serial_bus_slave(struct acpi_device *device) { struct list_head resource_list; bool is_serial_bus_slave = false; +if (acpi_is_indirect_io_slave(device)) +return true; + /* Macs use device properties in lieu of _CRS resources */ This means I can move all this scan code into the LLDD. What do you think? Please let me know. John
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 14/02/2018 16:16, Lorenzo Pieralisi wrote: On Wed, Feb 14, 2018 at 01:45:31AM +0800, John Garry wrote: On some platforms (such as arm64-based hip06/hip07), access to legacy ISA/LPC devices through access IO space is required, similar to x86 platforms. As the I/O for these devices are not memory mapped like PCI/PCIE MMIO host bridges, they require special low-level device operations through some host to generate IO accesses, i.e. a non- transparent bridge. Through the logical PIO framework, hosts are able to register address ranges in the logical PIO space for IO accesses. For hosts which require a LLDD to generate the IO accesses, through the logical PIO framework the host also registers accessors as a backend to generate the physical bus transactions for IO space accesses (called indirect IO). When describing the indirect IO child device in APCI tables, the IO resource is the host-specific address for the child (generally a bus address). An example is as follows: Device (LPC0) { Name (_HID, "HISI0191") // HiSi LPC Name (_CRS, ResourceTemplate () { Memory32Fixed (ReadWrite, 0xa01b, 0x1000) }) } Device (LPC0.IPMI) { Name (_HID, "IPI0001") Name (LORS, ResourceTemplate() { QWordIO ( ResourceConsumer, MinNotFixed, // _MIF MaxNotFixed, // _MAF PosDecode, EntireRange, 0x0, // _GRA 0xe4,// _MIN 0x3fff, // _MAX 0x0, // _TRA 0x04,// _LEN , , BTIO ) }) Since the IO resource for the child is a host-specific address, special translation are required to retrieve the logical PIO address for that child. Hi Lorenzo, The problem I have with this patchset and with pretending that the ACPI bits are generic is that the rules used to translate resources (I am referring to LPC0.IPMI above) are documented _nowhere_ which means that making this series generic code is just wishful thinking - there are no bindings backing it, it will never ever be used on a platform different from the one you are pushing this code for and I stated this already. Right, it is working on the presumption that this is how all "indirectio IO" hosts and children should/would be described in DSDT. Reworded differently - this is a Hisilicon driver it is not generic ACPI code; I can't see how it can be used on a multitude of platforms unless you specify FW level bindings. To overcome the problem of associating this logical PIO address with the child device, a scan handler is added to scan the ACPI namespace for known indirect IO hosts. This scan handler creates an MFD per child with the translated logical PIO address as it's IO resource, as a substitute for the normal platform device which ACPI would create during device enumeration. Signed-off-by: John GarrySigned-off-by: Zhichang Yuan Signed-off-by: Gabriele Paoloni --- drivers/acpi/arm64/Makefile | 1 + drivers/acpi/arm64/acpi_indirectio.c | 250 +++ See above (and I do not understand what arm64 has to do with it). Nothing apart from only being used by arm64 platforms today, which is circumstantial. I understand you need to find a place to add the: acpi_indirect_io_scan_init() to be called from core ACPI code because ACPI can't handle probe dependencies in any other way but other than that this patch is a Hisilicon ACPI driver - there is nothing generic in it (or at least there are no standard bindings to make it so). Whether a callback from ACPI core code (acpi_scan_init()) to a driver specific hook is sane or not that's the question and the only reason why you want to add this in drivers/acpi/arm64 rather than, say, drivers/bus (as you do for the DT driver). I do not know Rafael's opinion on the above, I would like to help you make forward progress but please understand my concerns, mostly on FW side. I did mention an alternative in my "ping" in v12 patch 7/9 (Feb 1), but no response to this specific note so I kept on the same path. Here's what I then wrote: "I think another solution - which you may prefer - is to avoid adding this scan handler (and all this other scan code) and add a check like acpi_is_serial_bus_slave() [which checks the device parent versus a list of known indirectIO hosts] to not enumerate these children, and do it from the LLDD host probe instead (https://lkml.org/lkml/2017/6/16/250)" Please consider this. Thanks, Lorenzo drivers/acpi/internal.h | 5 + drivers/acpi/scan.c | 1 + 4 files changed, 257 insertions(+) create mode 100644 drivers/acpi/arm64/acpi_indirectio.c Cheers, John
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 14/02/2018 16:16, Lorenzo Pieralisi wrote: On Wed, Feb 14, 2018 at 01:45:31AM +0800, John Garry wrote: On some platforms (such as arm64-based hip06/hip07), access to legacy ISA/LPC devices through access IO space is required, similar to x86 platforms. As the I/O for these devices are not memory mapped like PCI/PCIE MMIO host bridges, they require special low-level device operations through some host to generate IO accesses, i.e. a non- transparent bridge. Through the logical PIO framework, hosts are able to register address ranges in the logical PIO space for IO accesses. For hosts which require a LLDD to generate the IO accesses, through the logical PIO framework the host also registers accessors as a backend to generate the physical bus transactions for IO space accesses (called indirect IO). When describing the indirect IO child device in APCI tables, the IO resource is the host-specific address for the child (generally a bus address). An example is as follows: Device (LPC0) { Name (_HID, "HISI0191") // HiSi LPC Name (_CRS, ResourceTemplate () { Memory32Fixed (ReadWrite, 0xa01b, 0x1000) }) } Device (LPC0.IPMI) { Name (_HID, "IPI0001") Name (LORS, ResourceTemplate() { QWordIO ( ResourceConsumer, MinNotFixed, // _MIF MaxNotFixed, // _MAF PosDecode, EntireRange, 0x0, // _GRA 0xe4,// _MIN 0x3fff, // _MAX 0x0, // _TRA 0x04,// _LEN , , BTIO ) }) Since the IO resource for the child is a host-specific address, special translation are required to retrieve the logical PIO address for that child. Hi Lorenzo, The problem I have with this patchset and with pretending that the ACPI bits are generic is that the rules used to translate resources (I am referring to LPC0.IPMI above) are documented _nowhere_ which means that making this series generic code is just wishful thinking - there are no bindings backing it, it will never ever be used on a platform different from the one you are pushing this code for and I stated this already. Right, it is working on the presumption that this is how all "indirectio IO" hosts and children should/would be described in DSDT. Reworded differently - this is a Hisilicon driver it is not generic ACPI code; I can't see how it can be used on a multitude of platforms unless you specify FW level bindings. To overcome the problem of associating this logical PIO address with the child device, a scan handler is added to scan the ACPI namespace for known indirect IO hosts. This scan handler creates an MFD per child with the translated logical PIO address as it's IO resource, as a substitute for the normal platform device which ACPI would create during device enumeration. Signed-off-by: John Garry Signed-off-by: Zhichang Yuan Signed-off-by: Gabriele Paoloni --- drivers/acpi/arm64/Makefile | 1 + drivers/acpi/arm64/acpi_indirectio.c | 250 +++ See above (and I do not understand what arm64 has to do with it). Nothing apart from only being used by arm64 platforms today, which is circumstantial. I understand you need to find a place to add the: acpi_indirect_io_scan_init() to be called from core ACPI code because ACPI can't handle probe dependencies in any other way but other than that this patch is a Hisilicon ACPI driver - there is nothing generic in it (or at least there are no standard bindings to make it so). Whether a callback from ACPI core code (acpi_scan_init()) to a driver specific hook is sane or not that's the question and the only reason why you want to add this in drivers/acpi/arm64 rather than, say, drivers/bus (as you do for the DT driver). I do not know Rafael's opinion on the above, I would like to help you make forward progress but please understand my concerns, mostly on FW side. I did mention an alternative in my "ping" in v12 patch 7/9 (Feb 1), but no response to this specific note so I kept on the same path. Here's what I then wrote: "I think another solution - which you may prefer - is to avoid adding this scan handler (and all this other scan code) and add a check like acpi_is_serial_bus_slave() [which checks the device parent versus a list of known indirectIO hosts] to not enumerate these children, and do it from the LLDD host probe instead (https://lkml.org/lkml/2017/6/16/250)" Please consider this. Thanks, Lorenzo drivers/acpi/internal.h | 5 + drivers/acpi/scan.c | 1 + 4 files changed, 257 insertions(+) create mode 100644 drivers/acpi/arm64/acpi_indirectio.c Cheers, John
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Wed, Feb 14, 2018 at 5:33 PM, John Garrywrote: > On 14/02/2018 13:53, Andy Shevchenko wrote: >> On Tue, Feb 13, 2018 at 7:45 PM, John Garry wrote: >>> + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, >>> len); >>> + if (sys_port == -1UL) >> Wouldn't it be better to compare with ULONG_MAX? > Could do, being the same thing. Maybe people prefer -1UL as it saves having > to figure out what ULONG_MAX is :) -1UL looks confusing. Another approach is to use ~0UL if that is preferable. >>> + list_for_each_entry(rentry, _list, node) >>> + resources[count++] = *rentry->res; >> It has similarities with acpi_create_platform_device(). >> I guess we can utilize existing code. > For sure, this particular segment is effectively same as part of > acpi_create_platform_device(): Not the same, acpi_create_platform_device() does a bit more than copying the resources. If it indeed makes no hurt... > list_for_each_entry(rentry, _list, node) > acpi_platform_fill_resource(adev, rentry->res, > [count++]); > So is your idea to refactor this common segment into a helper function? ...I would go with helper. >>> + char *name = _indirect_io_mfd_cell[count].name[0]; >>> + char *pnpid = _indirect_io_mfd_cell[count].pnpid[0]; >> >> >> Plain x is equivalent to [0]. > Right, but I thought for arrays that we should use address of x rather than > x itself, no? x is addressed by it's beginning. -- With Best Regards, Andy Shevchenko
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Wed, Feb 14, 2018 at 5:33 PM, John Garry wrote: > On 14/02/2018 13:53, Andy Shevchenko wrote: >> On Tue, Feb 13, 2018 at 7:45 PM, John Garry wrote: >>> + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, >>> len); >>> + if (sys_port == -1UL) >> Wouldn't it be better to compare with ULONG_MAX? > Could do, being the same thing. Maybe people prefer -1UL as it saves having > to figure out what ULONG_MAX is :) -1UL looks confusing. Another approach is to use ~0UL if that is preferable. >>> + list_for_each_entry(rentry, _list, node) >>> + resources[count++] = *rentry->res; >> It has similarities with acpi_create_platform_device(). >> I guess we can utilize existing code. > For sure, this particular segment is effectively same as part of > acpi_create_platform_device(): Not the same, acpi_create_platform_device() does a bit more than copying the resources. If it indeed makes no hurt... > list_for_each_entry(rentry, _list, node) > acpi_platform_fill_resource(adev, rentry->res, > [count++]); > So is your idea to refactor this common segment into a helper function? ...I would go with helper. >>> + char *name = _indirect_io_mfd_cell[count].name[0]; >>> + char *pnpid = _indirect_io_mfd_cell[count].pnpid[0]; >> >> >> Plain x is equivalent to [0]. > Right, but I thought for arrays that we should use address of x rather than > x itself, no? x is addressed by it's beginning. -- With Best Regards, Andy Shevchenko
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Wed, Feb 14, 2018 at 01:45:31AM +0800, John Garry wrote: > On some platforms (such as arm64-based hip06/hip07), access to legacy > ISA/LPC devices through access IO space is required, similar to x86 > platforms. As the I/O for these devices are not memory mapped like > PCI/PCIE MMIO host bridges, they require special low-level device > operations through some host to generate IO accesses, i.e. a non- > transparent bridge. > > Through the logical PIO framework, hosts are able to register address > ranges in the logical PIO space for IO accesses. For hosts which require > a LLDD to generate the IO accesses, through the logical PIO framework > the host also registers accessors as a backend to generate the physical > bus transactions for IO space accesses (called indirect IO). > > When describing the indirect IO child device in APCI tables, the IO > resource is the host-specific address for the child (generally a > bus address). > An example is as follows: > Device (LPC0) { > Name (_HID, "HISI0191") // HiSi LPC > Name (_CRS, ResourceTemplate () { > Memory32Fixed (ReadWrite, 0xa01b, 0x1000) > }) > } > > Device (LPC0.IPMI) { > Name (_HID, "IPI0001") > Name (LORS, ResourceTemplate() { > QWordIO ( > ResourceConsumer, > MinNotFixed, // _MIF > MaxNotFixed, // _MAF > PosDecode, > EntireRange, > 0x0, // _GRA > 0xe4,// _MIN > 0x3fff, // _MAX > 0x0, // _TRA > 0x04,// _LEN > , , > BTIO > ) > }) > > Since the IO resource for the child is a host-specific address, > special translation are required to retrieve the logical PIO address > for that child. The problem I have with this patchset and with pretending that the ACPI bits are generic is that the rules used to translate resources (I am referring to LPC0.IPMI above) are documented _nowhere_ which means that making this series generic code is just wishful thinking - there are no bindings backing it, it will never ever be used on a platform different from the one you are pushing this code for and I stated this already. Reworded differently - this is a Hisilicon driver it is not generic ACPI code; I can't see how it can be used on a multitude of platforms unless you specify FW level bindings. > To overcome the problem of associating this logical PIO address > with the child device, a scan handler is added to scan the ACPI > namespace for known indirect IO hosts. This scan handler creates an > MFD per child with the translated logical PIO address as it's IO > resource, as a substitute for the normal platform device which ACPI > would create during device enumeration. > > Signed-off-by: John Garry> Signed-off-by: Zhichang Yuan > Signed-off-by: Gabriele Paoloni > --- > drivers/acpi/arm64/Makefile | 1 + > drivers/acpi/arm64/acpi_indirectio.c | 250 > +++ See above (and I do not understand what arm64 has to do with it). I understand you need to find a place to add the: acpi_indirect_io_scan_init() to be called from core ACPI code because ACPI can't handle probe dependencies in any other way but other than that this patch is a Hisilicon ACPI driver - there is nothing generic in it (or at least there are no standard bindings to make it so). Whether a callback from ACPI core code (acpi_scan_init()) to a driver specific hook is sane or not that's the question and the only reason why you want to add this in drivers/acpi/arm64 rather than, say, drivers/bus (as you do for the DT driver). I do not know Rafael's opinion on the above, I would like to help you make forward progress but please understand my concerns, mostly on FW side. Thanks, Lorenzo > drivers/acpi/internal.h | 5 + > drivers/acpi/scan.c | 1 + > 4 files changed, 257 insertions(+) > create mode 100644 drivers/acpi/arm64/acpi_indirectio.c > > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > index 1017def..f4a7f46 100644 > --- a/drivers/acpi/arm64/Makefile > +++ b/drivers/acpi/arm64/Makefile > @@ -1,2 +1,3 @@ > obj-$(CONFIG_ACPI_IORT) += iort.o > obj-$(CONFIG_ACPI_GTDT) += gtdt.o > +obj-$(CONFIG_INDIRECT_PIO) += acpi_indirectio.o > diff --git a/drivers/acpi/arm64/acpi_indirectio.c > b/drivers/acpi/arm64/acpi_indirectio.c > new file mode 100644 > index 000..51a1b92 > --- /dev/null > +++ b/drivers/acpi/arm64/acpi_indirectio.c > @@ -0,0 +1,250 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. > + * Author: Gabriele Paoloni > + * Author: Zhichang Yuan > + * Author: John Garry > + * > + * This file implements functunality to scan the ACPI namespace and config > + * devices
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Wed, Feb 14, 2018 at 01:45:31AM +0800, John Garry wrote: > On some platforms (such as arm64-based hip06/hip07), access to legacy > ISA/LPC devices through access IO space is required, similar to x86 > platforms. As the I/O for these devices are not memory mapped like > PCI/PCIE MMIO host bridges, they require special low-level device > operations through some host to generate IO accesses, i.e. a non- > transparent bridge. > > Through the logical PIO framework, hosts are able to register address > ranges in the logical PIO space for IO accesses. For hosts which require > a LLDD to generate the IO accesses, through the logical PIO framework > the host also registers accessors as a backend to generate the physical > bus transactions for IO space accesses (called indirect IO). > > When describing the indirect IO child device in APCI tables, the IO > resource is the host-specific address for the child (generally a > bus address). > An example is as follows: > Device (LPC0) { > Name (_HID, "HISI0191") // HiSi LPC > Name (_CRS, ResourceTemplate () { > Memory32Fixed (ReadWrite, 0xa01b, 0x1000) > }) > } > > Device (LPC0.IPMI) { > Name (_HID, "IPI0001") > Name (LORS, ResourceTemplate() { > QWordIO ( > ResourceConsumer, > MinNotFixed, // _MIF > MaxNotFixed, // _MAF > PosDecode, > EntireRange, > 0x0, // _GRA > 0xe4,// _MIN > 0x3fff, // _MAX > 0x0, // _TRA > 0x04,// _LEN > , , > BTIO > ) > }) > > Since the IO resource for the child is a host-specific address, > special translation are required to retrieve the logical PIO address > for that child. The problem I have with this patchset and with pretending that the ACPI bits are generic is that the rules used to translate resources (I am referring to LPC0.IPMI above) are documented _nowhere_ which means that making this series generic code is just wishful thinking - there are no bindings backing it, it will never ever be used on a platform different from the one you are pushing this code for and I stated this already. Reworded differently - this is a Hisilicon driver it is not generic ACPI code; I can't see how it can be used on a multitude of platforms unless you specify FW level bindings. > To overcome the problem of associating this logical PIO address > with the child device, a scan handler is added to scan the ACPI > namespace for known indirect IO hosts. This scan handler creates an > MFD per child with the translated logical PIO address as it's IO > resource, as a substitute for the normal platform device which ACPI > would create during device enumeration. > > Signed-off-by: John Garry > Signed-off-by: Zhichang Yuan > Signed-off-by: Gabriele Paoloni > --- > drivers/acpi/arm64/Makefile | 1 + > drivers/acpi/arm64/acpi_indirectio.c | 250 > +++ See above (and I do not understand what arm64 has to do with it). I understand you need to find a place to add the: acpi_indirect_io_scan_init() to be called from core ACPI code because ACPI can't handle probe dependencies in any other way but other than that this patch is a Hisilicon ACPI driver - there is nothing generic in it (or at least there are no standard bindings to make it so). Whether a callback from ACPI core code (acpi_scan_init()) to a driver specific hook is sane or not that's the question and the only reason why you want to add this in drivers/acpi/arm64 rather than, say, drivers/bus (as you do for the DT driver). I do not know Rafael's opinion on the above, I would like to help you make forward progress but please understand my concerns, mostly on FW side. Thanks, Lorenzo > drivers/acpi/internal.h | 5 + > drivers/acpi/scan.c | 1 + > 4 files changed, 257 insertions(+) > create mode 100644 drivers/acpi/arm64/acpi_indirectio.c > > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > index 1017def..f4a7f46 100644 > --- a/drivers/acpi/arm64/Makefile > +++ b/drivers/acpi/arm64/Makefile > @@ -1,2 +1,3 @@ > obj-$(CONFIG_ACPI_IORT) += iort.o > obj-$(CONFIG_ACPI_GTDT) += gtdt.o > +obj-$(CONFIG_INDIRECT_PIO) += acpi_indirectio.o > diff --git a/drivers/acpi/arm64/acpi_indirectio.c > b/drivers/acpi/arm64/acpi_indirectio.c > new file mode 100644 > index 000..51a1b92 > --- /dev/null > +++ b/drivers/acpi/arm64/acpi_indirectio.c > @@ -0,0 +1,250 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. > + * Author: Gabriele Paoloni > + * Author: Zhichang Yuan > + * Author: John Garry > + * > + * This file implements functunality to scan the ACPI namespace and config > + * devices under "indirect IO" hosts. An "indirect IO" host allows child > + * devices to use logical IO accesses when the host, itself, does not provide > + * a
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 14/02/2018 13:53, Andy Shevchenko wrote: On Tue, Feb 13, 2018 at 7:45 PM, John Garrywrote: On some platforms (such as arm64-based hip06/hip07), access to legacy ISA/LPC devices through access IO space is required, similar to x86 platforms. As the I/O for these devices are not memory mapped like PCI/PCIE MMIO host bridges, they require special low-level device operations through some host to generate IO accesses, i.e. a non- transparent bridge. Through the logical PIO framework, hosts are able to register address ranges in the logical PIO space for IO accesses. For hosts which require a LLDD to generate the IO accesses, through the logical PIO framework the host also registers accessors as a backend to generate the physical bus transactions for IO space accesses (called indirect IO). When describing the indirect IO child device in APCI tables, the IO resource is the host-specific address for the child (generally a bus address). An example is as follows: Device (LPC0) { Name (_HID, "HISI0191") // HiSi LPC Name (_CRS, ResourceTemplate () { Memory32Fixed (ReadWrite, 0xa01b, 0x1000) }) } Device (LPC0.IPMI) { Name (_HID, "IPI0001") Name (LORS, ResourceTemplate() { QWordIO ( ResourceConsumer, MinNotFixed, // _MIF MaxNotFixed, // _MAF PosDecode, EntireRange, 0x0, // _GRA 0xe4,// _MIN 0x3fff, // _MAX 0x0, // _TRA 0x04,// _LEN , , BTIO ) }) Since the IO resource for the child is a host-specific address, special translation are required to retrieve the logical PIO address for that child. To overcome the problem of associating this logical PIO address with the child device, a scan handler is added to scan the ACPI namespace for known indirect IO hosts. This scan handler creates an MFD per child with the translated logical PIO address as it's IO resource, as a substitute for the normal platform device which ACPI would create during device enumeration. Hi Andy, + unsigned long sys_port; + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, len); + if (sys_port == -1UL) Wouldn't it be better to compare with ULONG_MAX? Could do, being the same thing. Maybe people prefer -1UL as it saves having to figure out what ULONG_MAX is :) + return -EFAULT; +/* Shouldn't be a kernel-doc? Right, I'll make it /** + * acpi_indirect_io_set_res - set the resources for a child device + * (MFD) of an "indirect IO" host. In that case this would be one line w/o period at the end. + * @child: the device node to be updated the I/O resource + * @hostdev: the device node associated with the "indirect IO" host + * @res: double pointer to be set to the address of translated resources + * @num_res: pointer to variable to hold the number of translated resources + * + * Returns 0 when successful, and a negative value for failure. + * + * For a given "indirect IO" host, each child device will have associated + * host-relevative address resource. This function will return the translated + * logical PIO addresses for each child devices resources. + */ +static int acpi_indirect_io_set_res(struct device *child, + struct device *hostdev, + const struct resource **res, + int *num_res) +{ + struct acpi_device *adev; + struct acpi_device *host; + struct resource_entry *rentry; + LIST_HEAD(resource_list); + struct resource *resources; + int count; + int i; + int ret = -EIO; + + if (!child || !hostdev) + return -EINVAL; + + host = to_acpi_device(hostdev); + adev = to_acpi_device(child); *** + count = acpi_dev_get_resources(adev, _list, NULL, NULL); + if (count <= 0) { + dev_err(child, "failed to get resources\n"); + return count ? count : -EIO; + } + + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); + if (!resources) { + acpi_dev_free_resource_list(_list); + return -ENOMEM; + } + count = 0; + list_for_each_entry(rentry, _list, node) + resources[count++] = *rentry->res; + + acpi_dev_free_resource_list(_list); It has similarities with acpi_create_platform_device(). I guess we can utilize existing code. For sure, this particular segment is effectively same as part of acpi_create_platform_device(): struct platform_device *acpi_create_platform_device(struct acpi_device *adev, struct property_entry *properties) { struct platform_device *pdev = NULL; struct platform_device_info pdevinfo; struct resource_entry *rentry; struct list_head resource_list; struct resource *resources = NULL;
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On 14/02/2018 13:53, Andy Shevchenko wrote: On Tue, Feb 13, 2018 at 7:45 PM, John Garry wrote: On some platforms (such as arm64-based hip06/hip07), access to legacy ISA/LPC devices through access IO space is required, similar to x86 platforms. As the I/O for these devices are not memory mapped like PCI/PCIE MMIO host bridges, they require special low-level device operations through some host to generate IO accesses, i.e. a non- transparent bridge. Through the logical PIO framework, hosts are able to register address ranges in the logical PIO space for IO accesses. For hosts which require a LLDD to generate the IO accesses, through the logical PIO framework the host also registers accessors as a backend to generate the physical bus transactions for IO space accesses (called indirect IO). When describing the indirect IO child device in APCI tables, the IO resource is the host-specific address for the child (generally a bus address). An example is as follows: Device (LPC0) { Name (_HID, "HISI0191") // HiSi LPC Name (_CRS, ResourceTemplate () { Memory32Fixed (ReadWrite, 0xa01b, 0x1000) }) } Device (LPC0.IPMI) { Name (_HID, "IPI0001") Name (LORS, ResourceTemplate() { QWordIO ( ResourceConsumer, MinNotFixed, // _MIF MaxNotFixed, // _MAF PosDecode, EntireRange, 0x0, // _GRA 0xe4,// _MIN 0x3fff, // _MAX 0x0, // _TRA 0x04,// _LEN , , BTIO ) }) Since the IO resource for the child is a host-specific address, special translation are required to retrieve the logical PIO address for that child. To overcome the problem of associating this logical PIO address with the child device, a scan handler is added to scan the ACPI namespace for known indirect IO hosts. This scan handler creates an MFD per child with the translated logical PIO address as it's IO resource, as a substitute for the normal platform device which ACPI would create during device enumeration. Hi Andy, + unsigned long sys_port; + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, len); + if (sys_port == -1UL) Wouldn't it be better to compare with ULONG_MAX? Could do, being the same thing. Maybe people prefer -1UL as it saves having to figure out what ULONG_MAX is :) + return -EFAULT; +/* Shouldn't be a kernel-doc? Right, I'll make it /** + * acpi_indirect_io_set_res - set the resources for a child device + * (MFD) of an "indirect IO" host. In that case this would be one line w/o period at the end. + * @child: the device node to be updated the I/O resource + * @hostdev: the device node associated with the "indirect IO" host + * @res: double pointer to be set to the address of translated resources + * @num_res: pointer to variable to hold the number of translated resources + * + * Returns 0 when successful, and a negative value for failure. + * + * For a given "indirect IO" host, each child device will have associated + * host-relevative address resource. This function will return the translated + * logical PIO addresses for each child devices resources. + */ +static int acpi_indirect_io_set_res(struct device *child, + struct device *hostdev, + const struct resource **res, + int *num_res) +{ + struct acpi_device *adev; + struct acpi_device *host; + struct resource_entry *rentry; + LIST_HEAD(resource_list); + struct resource *resources; + int count; + int i; + int ret = -EIO; + + if (!child || !hostdev) + return -EINVAL; + + host = to_acpi_device(hostdev); + adev = to_acpi_device(child); *** + count = acpi_dev_get_resources(adev, _list, NULL, NULL); + if (count <= 0) { + dev_err(child, "failed to get resources\n"); + return count ? count : -EIO; + } + + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); + if (!resources) { + acpi_dev_free_resource_list(_list); + return -ENOMEM; + } + count = 0; + list_for_each_entry(rentry, _list, node) + resources[count++] = *rentry->res; + + acpi_dev_free_resource_list(_list); It has similarities with acpi_create_platform_device(). I guess we can utilize existing code. For sure, this particular segment is effectively same as part of acpi_create_platform_device(): struct platform_device *acpi_create_platform_device(struct acpi_device *adev, struct property_entry *properties) { struct platform_device *pdev = NULL; struct platform_device_info pdevinfo; struct resource_entry *rentry; struct list_head resource_list; struct resource *resources = NULL; int count; /* If
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Tue, Feb 13, 2018 at 7:45 PM, John Garrywrote: > On some platforms (such as arm64-based hip06/hip07), access to legacy > ISA/LPC devices through access IO space is required, similar to x86 > platforms. As the I/O for these devices are not memory mapped like > PCI/PCIE MMIO host bridges, they require special low-level device > operations through some host to generate IO accesses, i.e. a non- > transparent bridge. > > Through the logical PIO framework, hosts are able to register address > ranges in the logical PIO space for IO accesses. For hosts which require > a LLDD to generate the IO accesses, through the logical PIO framework > the host also registers accessors as a backend to generate the physical > bus transactions for IO space accesses (called indirect IO). > > When describing the indirect IO child device in APCI tables, the IO > resource is the host-specific address for the child (generally a > bus address). > An example is as follows: > Device (LPC0) { > Name (_HID, "HISI0191") // HiSi LPC > Name (_CRS, ResourceTemplate () { > Memory32Fixed (ReadWrite, 0xa01b, 0x1000) > }) > } > > Device (LPC0.IPMI) { > Name (_HID, "IPI0001") > Name (LORS, ResourceTemplate() { > QWordIO ( > ResourceConsumer, > MinNotFixed, // _MIF > MaxNotFixed, // _MAF > PosDecode, > EntireRange, > 0x0, // _GRA > 0xe4,// _MIN > 0x3fff, // _MAX > 0x0, // _TRA > 0x04,// _LEN > , , > BTIO > ) > }) > > Since the IO resource for the child is a host-specific address, > special translation are required to retrieve the logical PIO address > for that child. > > To overcome the problem of associating this logical PIO address > with the child device, a scan handler is added to scan the ACPI > namespace for known indirect IO hosts. This scan handler creates an > MFD per child with the translated logical PIO address as it's IO > resource, as a substitute for the normal platform device which ACPI > would create during device enumeration. > + unsigned long sys_port; > + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, len); > + if (sys_port == -1UL) Wouldn't it be better to compare with ULONG_MAX? > + return -EFAULT; > +/* Shouldn't be a kernel-doc? > + * acpi_indirect_io_set_res - set the resources for a child device > + * (MFD) of an "indirect IO" host. In that case this would be one line w/o period at the end. > + * @child: the device node to be updated the I/O resource > + * @hostdev: the device node associated with the "indirect IO" host > + * @res: double pointer to be set to the address of translated resources > + * @num_res: pointer to variable to hold the number of translated resources > + * > + * Returns 0 when successful, and a negative value for failure. > + * > + * For a given "indirect IO" host, each child device will have associated > + * host-relevative address resource. This function will return the translated > + * logical PIO addresses for each child devices resources. > + */ > +static int acpi_indirect_io_set_res(struct device *child, > + struct device *hostdev, > + const struct resource **res, > + int *num_res) > +{ > + struct acpi_device *adev; > + struct acpi_device *host; > + struct resource_entry *rentry; > + LIST_HEAD(resource_list); > + struct resource *resources; > + int count; > + int i; > + int ret = -EIO; > + > + if (!child || !hostdev) > + return -EINVAL; > + > + host = to_acpi_device(hostdev); > + adev = to_acpi_device(child); > + count = acpi_dev_get_resources(adev, _list, NULL, NULL); > + if (count <= 0) { > + dev_err(child, "failed to get resources\n"); > + return count ? count : -EIO; > + } > + > + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); > + if (!resources) { > + acpi_dev_free_resource_list(_list); > + return -ENOMEM; > + } > + count = 0; > + list_for_each_entry(rentry, _list, node) > + resources[count++] = *rentry->res; > + > + acpi_dev_free_resource_list(_list); It has similarities with acpi_create_platform_device(). I guess we can utilize existing code. > + /* translate the I/O resources */ > + for (i = 0; i < count; i++) { > + if (!(resources[i].flags & IORESOURCE_IO)) > + continue; > + ret = acpi_indirect_io_xlat_res(adev, host, [i]); > + if (ret) { > + kfree(resources); > + dev_err(child, "translate IO range failed(%d)\n", > ret); > + return ret; > +
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Tue, Feb 13, 2018 at 7:45 PM, John Garry wrote: > On some platforms (such as arm64-based hip06/hip07), access to legacy > ISA/LPC devices through access IO space is required, similar to x86 > platforms. As the I/O for these devices are not memory mapped like > PCI/PCIE MMIO host bridges, they require special low-level device > operations through some host to generate IO accesses, i.e. a non- > transparent bridge. > > Through the logical PIO framework, hosts are able to register address > ranges in the logical PIO space for IO accesses. For hosts which require > a LLDD to generate the IO accesses, through the logical PIO framework > the host also registers accessors as a backend to generate the physical > bus transactions for IO space accesses (called indirect IO). > > When describing the indirect IO child device in APCI tables, the IO > resource is the host-specific address for the child (generally a > bus address). > An example is as follows: > Device (LPC0) { > Name (_HID, "HISI0191") // HiSi LPC > Name (_CRS, ResourceTemplate () { > Memory32Fixed (ReadWrite, 0xa01b, 0x1000) > }) > } > > Device (LPC0.IPMI) { > Name (_HID, "IPI0001") > Name (LORS, ResourceTemplate() { > QWordIO ( > ResourceConsumer, > MinNotFixed, // _MIF > MaxNotFixed, // _MAF > PosDecode, > EntireRange, > 0x0, // _GRA > 0xe4,// _MIN > 0x3fff, // _MAX > 0x0, // _TRA > 0x04,// _LEN > , , > BTIO > ) > }) > > Since the IO resource for the child is a host-specific address, > special translation are required to retrieve the logical PIO address > for that child. > > To overcome the problem of associating this logical PIO address > with the child device, a scan handler is added to scan the ACPI > namespace for known indirect IO hosts. This scan handler creates an > MFD per child with the translated logical PIO address as it's IO > resource, as a substitute for the normal platform device which ACPI > would create during device enumeration. > + unsigned long sys_port; > + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, len); > + if (sys_port == -1UL) Wouldn't it be better to compare with ULONG_MAX? > + return -EFAULT; > +/* Shouldn't be a kernel-doc? > + * acpi_indirect_io_set_res - set the resources for a child device > + * (MFD) of an "indirect IO" host. In that case this would be one line w/o period at the end. > + * @child: the device node to be updated the I/O resource > + * @hostdev: the device node associated with the "indirect IO" host > + * @res: double pointer to be set to the address of translated resources > + * @num_res: pointer to variable to hold the number of translated resources > + * > + * Returns 0 when successful, and a negative value for failure. > + * > + * For a given "indirect IO" host, each child device will have associated > + * host-relevative address resource. This function will return the translated > + * logical PIO addresses for each child devices resources. > + */ > +static int acpi_indirect_io_set_res(struct device *child, > + struct device *hostdev, > + const struct resource **res, > + int *num_res) > +{ > + struct acpi_device *adev; > + struct acpi_device *host; > + struct resource_entry *rentry; > + LIST_HEAD(resource_list); > + struct resource *resources; > + int count; > + int i; > + int ret = -EIO; > + > + if (!child || !hostdev) > + return -EINVAL; > + > + host = to_acpi_device(hostdev); > + adev = to_acpi_device(child); > + count = acpi_dev_get_resources(adev, _list, NULL, NULL); > + if (count <= 0) { > + dev_err(child, "failed to get resources\n"); > + return count ? count : -EIO; > + } > + > + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); > + if (!resources) { > + acpi_dev_free_resource_list(_list); > + return -ENOMEM; > + } > + count = 0; > + list_for_each_entry(rentry, _list, node) > + resources[count++] = *rentry->res; > + > + acpi_dev_free_resource_list(_list); It has similarities with acpi_create_platform_device(). I guess we can utilize existing code. > + /* translate the I/O resources */ > + for (i = 0; i < count; i++) { > + if (!(resources[i].flags & IORESOURCE_IO)) > + continue; > + ret = acpi_indirect_io_xlat_res(adev, host, [i]); > + if (ret) { > + kfree(resources); > + dev_err(child, "translate IO range failed(%d)\n", > ret); > + return ret; > + } > +
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
Signed-off-by: John GarrySigned-off-by: Zhichang Yuan Signed-off-by: Gabriele Paoloni Hi Rafael, Thanks for checking again. Just a few minor nits below. --- drivers/acpi/arm64/Makefile | 1 + drivers/acpi/arm64/acpi_indirectio.c | 250 +++ drivers/acpi/internal.h | 5 + drivers/acpi/scan.c | 1 + 4 files changed, 257 insertions(+) create mode 100644 drivers/acpi/arm64/acpi_indirectio.c diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile index 1017def..f4a7f46 100644 --- a/drivers/acpi/arm64/Makefile +++ b/drivers/acpi/arm64/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_ACPI_IORT)+= iort.o obj-$(CONFIG_ACPI_GTDT)+= gtdt.o +obj-$(CONFIG_INDIRECT_PIO) += acpi_indirectio.o diff --git a/drivers/acpi/arm64/acpi_indirectio.c b/drivers/acpi/arm64/acpi_indirectio.c new file mode 100644 index 000..51a1b92 --- /dev/null +++ b/drivers/acpi/arm64/acpi_indirectio.c @@ -0,0 +1,250 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. + * Author: Gabriele Paoloni + * Author: Zhichang Yuan + * Author: John Garry + * + * This file implements functunality to scan the ACPI namespace and config + * devices under "indirect IO" hosts. An "indirect IO" host allows child + * devices to use logical IO accesses when the host, itself, does not provide + * a transparent bridge. The device setup creates a per-child MFD with a + * logical port IO resource. + */ + +#include +#include +#include +#include + +ACPI_MODULE_NAME("indirect IO"); + +#define ACPI_INDIRECT_IO_NAME_LEN 255 + +struct acpi_indirect_io_mfd_cell { + struct mfd_cell_acpi_match acpi_match; + char name[ACPI_INDIRECT_IO_NAME_LEN]; + char pnpid[ACPI_INDIRECT_IO_NAME_LEN]; +}; + +static int acpi_indirect_io_xlat_res(struct acpi_device *adev, +struct acpi_device *host, +struct resource *res) +{ + unsigned long sys_port; + resource_size_t len = res->end - res->start; + + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, len); + if (sys_port == -1UL) + return -EFAULT; + + res->start = sys_port; + res->end = sys_port + len; + + return 0; +} + +/* + * acpi_indirect_io_set_res - set the resources for a child device + * (MFD) of an "indirect IO" host. The above should fit into a single line. I'd make it something like "acpi_indirect_io_set_res - set "indirect IO" host child (MFD) resources" and it already is explained in the comment below. Fine + * @child: the device node to be updated the I/O resource + * @hostdev: the device node associated with the "indirect IO" host + * @res: double pointer to be set to the address of translated resources + * @num_res: pointer to variable to hold the number of translated resources + * + * Returns 0 when successful, and a negative value for failure. + * + * For a given "indirect IO" host, each child device will have associated + * host-relevative address resource. This function will return the translated host-relative right + * logical PIO addresses for each child devices resources. + */ +static int acpi_indirect_io_set_res(struct device *child, + struct device *hostdev, + const struct resource **res, + int *num_res) +{ + struct acpi_device *adev; + struct acpi_device *host; + struct resource_entry *rentry; + LIST_HEAD(resource_list); + struct resource *resources; + int count; + int i; + int ret = -EIO; + + if (!child || !hostdev) + return -EINVAL; + + host = to_acpi_device(hostdev); + adev = to_acpi_device(child); + + /* check the device state */ + if (!adev->status.present) { + dev_info(child, "device is not present\n"); dev_dbg()? + return 0; + } + /* whether the child had been enumerated? */ + if (acpi_device_enumerated(adev)) { + dev_info(child, "had been enumerated\n"); Again, dev_dbg()? sure, I think that these can be downgraded + return 0; + } + + count = acpi_dev_get_resources(adev, _list, NULL, NULL); + if (count <= 0) { + dev_err(child, "failed to get resources\n"); I'd use dev_dbg() here too (the message may not even be meaningful to a user). I think that it could be ok - having no resources is not really an "error". + return count ? count : -EIO; + } + + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); + if (!resources) { And you don't print anything here, I
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
Signed-off-by: John Garry Signed-off-by: Zhichang Yuan Signed-off-by: Gabriele Paoloni Hi Rafael, Thanks for checking again. Just a few minor nits below. --- drivers/acpi/arm64/Makefile | 1 + drivers/acpi/arm64/acpi_indirectio.c | 250 +++ drivers/acpi/internal.h | 5 + drivers/acpi/scan.c | 1 + 4 files changed, 257 insertions(+) create mode 100644 drivers/acpi/arm64/acpi_indirectio.c diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile index 1017def..f4a7f46 100644 --- a/drivers/acpi/arm64/Makefile +++ b/drivers/acpi/arm64/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_ACPI_IORT)+= iort.o obj-$(CONFIG_ACPI_GTDT)+= gtdt.o +obj-$(CONFIG_INDIRECT_PIO) += acpi_indirectio.o diff --git a/drivers/acpi/arm64/acpi_indirectio.c b/drivers/acpi/arm64/acpi_indirectio.c new file mode 100644 index 000..51a1b92 --- /dev/null +++ b/drivers/acpi/arm64/acpi_indirectio.c @@ -0,0 +1,250 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. + * Author: Gabriele Paoloni + * Author: Zhichang Yuan + * Author: John Garry + * + * This file implements functunality to scan the ACPI namespace and config + * devices under "indirect IO" hosts. An "indirect IO" host allows child + * devices to use logical IO accesses when the host, itself, does not provide + * a transparent bridge. The device setup creates a per-child MFD with a + * logical port IO resource. + */ + +#include +#include +#include +#include + +ACPI_MODULE_NAME("indirect IO"); + +#define ACPI_INDIRECT_IO_NAME_LEN 255 + +struct acpi_indirect_io_mfd_cell { + struct mfd_cell_acpi_match acpi_match; + char name[ACPI_INDIRECT_IO_NAME_LEN]; + char pnpid[ACPI_INDIRECT_IO_NAME_LEN]; +}; + +static int acpi_indirect_io_xlat_res(struct acpi_device *adev, +struct acpi_device *host, +struct resource *res) +{ + unsigned long sys_port; + resource_size_t len = res->end - res->start; + + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, len); + if (sys_port == -1UL) + return -EFAULT; + + res->start = sys_port; + res->end = sys_port + len; + + return 0; +} + +/* + * acpi_indirect_io_set_res - set the resources for a child device + * (MFD) of an "indirect IO" host. The above should fit into a single line. I'd make it something like "acpi_indirect_io_set_res - set "indirect IO" host child (MFD) resources" and it already is explained in the comment below. Fine + * @child: the device node to be updated the I/O resource + * @hostdev: the device node associated with the "indirect IO" host + * @res: double pointer to be set to the address of translated resources + * @num_res: pointer to variable to hold the number of translated resources + * + * Returns 0 when successful, and a negative value for failure. + * + * For a given "indirect IO" host, each child device will have associated + * host-relevative address resource. This function will return the translated host-relative right + * logical PIO addresses for each child devices resources. + */ +static int acpi_indirect_io_set_res(struct device *child, + struct device *hostdev, + const struct resource **res, + int *num_res) +{ + struct acpi_device *adev; + struct acpi_device *host; + struct resource_entry *rentry; + LIST_HEAD(resource_list); + struct resource *resources; + int count; + int i; + int ret = -EIO; + + if (!child || !hostdev) + return -EINVAL; + + host = to_acpi_device(hostdev); + adev = to_acpi_device(child); + + /* check the device state */ + if (!adev->status.present) { + dev_info(child, "device is not present\n"); dev_dbg()? + return 0; + } + /* whether the child had been enumerated? */ + if (acpi_device_enumerated(adev)) { + dev_info(child, "had been enumerated\n"); Again, dev_dbg()? sure, I think that these can be downgraded + return 0; + } + + count = acpi_dev_get_resources(adev, _list, NULL, NULL); + if (count <= 0) { + dev_err(child, "failed to get resources\n"); I'd use dev_dbg() here too (the message may not even be meaningful to a user). I think that it could be ok - having no resources is not really an "error". + return count ? count : -EIO; + } + + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); + if (!resources) { And you don't print anything here, I wonder why? As I see, we generally don't print out-of-memory failure as we expect the alloc code to do it. But I agree it's useful as we know the point of
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Tue, Feb 13, 2018 at 6:45 PM, John Garrywrote: > On some platforms (such as arm64-based hip06/hip07), access to legacy > ISA/LPC devices through access IO space is required, similar to x86 > platforms. As the I/O for these devices are not memory mapped like > PCI/PCIE MMIO host bridges, they require special low-level device > operations through some host to generate IO accesses, i.e. a non- > transparent bridge. > > Through the logical PIO framework, hosts are able to register address > ranges in the logical PIO space for IO accesses. For hosts which require > a LLDD to generate the IO accesses, through the logical PIO framework > the host also registers accessors as a backend to generate the physical > bus transactions for IO space accesses (called indirect IO). > > When describing the indirect IO child device in APCI tables, the IO > resource is the host-specific address for the child (generally a > bus address). > An example is as follows: > Device (LPC0) { > Name (_HID, "HISI0191") // HiSi LPC > Name (_CRS, ResourceTemplate () { > Memory32Fixed (ReadWrite, 0xa01b, 0x1000) > }) > } > > Device (LPC0.IPMI) { > Name (_HID, "IPI0001") > Name (LORS, ResourceTemplate() { > QWordIO ( > ResourceConsumer, > MinNotFixed, // _MIF > MaxNotFixed, // _MAF > PosDecode, > EntireRange, > 0x0, // _GRA > 0xe4,// _MIN > 0x3fff, // _MAX > 0x0, // _TRA > 0x04,// _LEN > , , > BTIO > ) > }) > > Since the IO resource for the child is a host-specific address, > special translation are required to retrieve the logical PIO address > for that child. > > To overcome the problem of associating this logical PIO address > with the child device, a scan handler is added to scan the ACPI > namespace for known indirect IO hosts. This scan handler creates an > MFD per child with the translated logical PIO address as it's IO > resource, as a substitute for the normal platform device which ACPI > would create during device enumeration. > > Signed-off-by: John Garry > Signed-off-by: Zhichang Yuan > Signed-off-by: Gabriele Paoloni Just a few minor nits below. > --- > drivers/acpi/arm64/Makefile | 1 + > drivers/acpi/arm64/acpi_indirectio.c | 250 > +++ > drivers/acpi/internal.h | 5 + > drivers/acpi/scan.c | 1 + > 4 files changed, 257 insertions(+) > create mode 100644 drivers/acpi/arm64/acpi_indirectio.c > > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > index 1017def..f4a7f46 100644 > --- a/drivers/acpi/arm64/Makefile > +++ b/drivers/acpi/arm64/Makefile > @@ -1,2 +1,3 @@ > obj-$(CONFIG_ACPI_IORT)+= iort.o > obj-$(CONFIG_ACPI_GTDT)+= gtdt.o > +obj-$(CONFIG_INDIRECT_PIO) += acpi_indirectio.o > diff --git a/drivers/acpi/arm64/acpi_indirectio.c > b/drivers/acpi/arm64/acpi_indirectio.c > new file mode 100644 > index 000..51a1b92 > --- /dev/null > +++ b/drivers/acpi/arm64/acpi_indirectio.c > @@ -0,0 +1,250 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. > + * Author: Gabriele Paoloni > + * Author: Zhichang Yuan > + * Author: John Garry > + * > + * This file implements functunality to scan the ACPI namespace and config > + * devices under "indirect IO" hosts. An "indirect IO" host allows child > + * devices to use logical IO accesses when the host, itself, does not provide > + * a transparent bridge. The device setup creates a per-child MFD with a > + * logical port IO resource. > + */ > + > +#include > +#include > +#include > +#include > + > +ACPI_MODULE_NAME("indirect IO"); > + > +#define ACPI_INDIRECT_IO_NAME_LEN 255 > + > +struct acpi_indirect_io_mfd_cell { > + struct mfd_cell_acpi_match acpi_match; > + char name[ACPI_INDIRECT_IO_NAME_LEN]; > + char pnpid[ACPI_INDIRECT_IO_NAME_LEN]; > +}; > + > +static int acpi_indirect_io_xlat_res(struct acpi_device *adev, > +struct acpi_device *host, > +struct resource *res) > +{ > + unsigned long sys_port; > + resource_size_t len = res->end - res->start; > + > + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, len); > + if (sys_port == -1UL) > + return -EFAULT; > + > + res->start = sys_port; > + res->end = sys_port + len; > + > + return 0; > +} > + > +/* > + * acpi_indirect_io_set_res - set the resources for a child device > + * (MFD) of an "indirect IO" host. The above should fit into a single line. I'd make it something like "acpi_indirect_io_set_res -
Re: [PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On Tue, Feb 13, 2018 at 6:45 PM, John Garry wrote: > On some platforms (such as arm64-based hip06/hip07), access to legacy > ISA/LPC devices through access IO space is required, similar to x86 > platforms. As the I/O for these devices are not memory mapped like > PCI/PCIE MMIO host bridges, they require special low-level device > operations through some host to generate IO accesses, i.e. a non- > transparent bridge. > > Through the logical PIO framework, hosts are able to register address > ranges in the logical PIO space for IO accesses. For hosts which require > a LLDD to generate the IO accesses, through the logical PIO framework > the host also registers accessors as a backend to generate the physical > bus transactions for IO space accesses (called indirect IO). > > When describing the indirect IO child device in APCI tables, the IO > resource is the host-specific address for the child (generally a > bus address). > An example is as follows: > Device (LPC0) { > Name (_HID, "HISI0191") // HiSi LPC > Name (_CRS, ResourceTemplate () { > Memory32Fixed (ReadWrite, 0xa01b, 0x1000) > }) > } > > Device (LPC0.IPMI) { > Name (_HID, "IPI0001") > Name (LORS, ResourceTemplate() { > QWordIO ( > ResourceConsumer, > MinNotFixed, // _MIF > MaxNotFixed, // _MAF > PosDecode, > EntireRange, > 0x0, // _GRA > 0xe4,// _MIN > 0x3fff, // _MAX > 0x0, // _TRA > 0x04,// _LEN > , , > BTIO > ) > }) > > Since the IO resource for the child is a host-specific address, > special translation are required to retrieve the logical PIO address > for that child. > > To overcome the problem of associating this logical PIO address > with the child device, a scan handler is added to scan the ACPI > namespace for known indirect IO hosts. This scan handler creates an > MFD per child with the translated logical PIO address as it's IO > resource, as a substitute for the normal platform device which ACPI > would create during device enumeration. > > Signed-off-by: John Garry > Signed-off-by: Zhichang Yuan > Signed-off-by: Gabriele Paoloni Just a few minor nits below. > --- > drivers/acpi/arm64/Makefile | 1 + > drivers/acpi/arm64/acpi_indirectio.c | 250 > +++ > drivers/acpi/internal.h | 5 + > drivers/acpi/scan.c | 1 + > 4 files changed, 257 insertions(+) > create mode 100644 drivers/acpi/arm64/acpi_indirectio.c > > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > index 1017def..f4a7f46 100644 > --- a/drivers/acpi/arm64/Makefile > +++ b/drivers/acpi/arm64/Makefile > @@ -1,2 +1,3 @@ > obj-$(CONFIG_ACPI_IORT)+= iort.o > obj-$(CONFIG_ACPI_GTDT)+= gtdt.o > +obj-$(CONFIG_INDIRECT_PIO) += acpi_indirectio.o > diff --git a/drivers/acpi/arm64/acpi_indirectio.c > b/drivers/acpi/arm64/acpi_indirectio.c > new file mode 100644 > index 000..51a1b92 > --- /dev/null > +++ b/drivers/acpi/arm64/acpi_indirectio.c > @@ -0,0 +1,250 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. > + * Author: Gabriele Paoloni > + * Author: Zhichang Yuan > + * Author: John Garry > + * > + * This file implements functunality to scan the ACPI namespace and config > + * devices under "indirect IO" hosts. An "indirect IO" host allows child > + * devices to use logical IO accesses when the host, itself, does not provide > + * a transparent bridge. The device setup creates a per-child MFD with a > + * logical port IO resource. > + */ > + > +#include > +#include > +#include > +#include > + > +ACPI_MODULE_NAME("indirect IO"); > + > +#define ACPI_INDIRECT_IO_NAME_LEN 255 > + > +struct acpi_indirect_io_mfd_cell { > + struct mfd_cell_acpi_match acpi_match; > + char name[ACPI_INDIRECT_IO_NAME_LEN]; > + char pnpid[ACPI_INDIRECT_IO_NAME_LEN]; > +}; > + > +static int acpi_indirect_io_xlat_res(struct acpi_device *adev, > +struct acpi_device *host, > +struct resource *res) > +{ > + unsigned long sys_port; > + resource_size_t len = res->end - res->start; > + > + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, len); > + if (sys_port == -1UL) > + return -EFAULT; > + > + res->start = sys_port; > + res->end = sys_port + len; > + > + return 0; > +} > + > +/* > + * acpi_indirect_io_set_res - set the resources for a child device > + * (MFD) of an "indirect IO" host. The above should fit into a single line. I'd make it something like "acpi_indirect_io_set_res - set "indirect IO" host child (MFD) resources" and it already is explained in the comment below. > + * @child: the device node to be updated the I/O resource > + * @hostdev: the
[PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On some platforms (such as arm64-based hip06/hip07), access to legacy ISA/LPC devices through access IO space is required, similar to x86 platforms. As the I/O for these devices are not memory mapped like PCI/PCIE MMIO host bridges, they require special low-level device operations through some host to generate IO accesses, i.e. a non- transparent bridge. Through the logical PIO framework, hosts are able to register address ranges in the logical PIO space for IO accesses. For hosts which require a LLDD to generate the IO accesses, through the logical PIO framework the host also registers accessors as a backend to generate the physical bus transactions for IO space accesses (called indirect IO). When describing the indirect IO child device in APCI tables, the IO resource is the host-specific address for the child (generally a bus address). An example is as follows: Device (LPC0) { Name (_HID, "HISI0191") // HiSi LPC Name (_CRS, ResourceTemplate () { Memory32Fixed (ReadWrite, 0xa01b, 0x1000) }) } Device (LPC0.IPMI) { Name (_HID, "IPI0001") Name (LORS, ResourceTemplate() { QWordIO ( ResourceConsumer, MinNotFixed, // _MIF MaxNotFixed, // _MAF PosDecode, EntireRange, 0x0, // _GRA 0xe4,// _MIN 0x3fff, // _MAX 0x0, // _TRA 0x04,// _LEN , , BTIO ) }) Since the IO resource for the child is a host-specific address, special translation are required to retrieve the logical PIO address for that child. To overcome the problem of associating this logical PIO address with the child device, a scan handler is added to scan the ACPI namespace for known indirect IO hosts. This scan handler creates an MFD per child with the translated logical PIO address as it's IO resource, as a substitute for the normal platform device which ACPI would create during device enumeration. Signed-off-by: John GarrySigned-off-by: Zhichang Yuan Signed-off-by: Gabriele Paoloni --- drivers/acpi/arm64/Makefile | 1 + drivers/acpi/arm64/acpi_indirectio.c | 250 +++ drivers/acpi/internal.h | 5 + drivers/acpi/scan.c | 1 + 4 files changed, 257 insertions(+) create mode 100644 drivers/acpi/arm64/acpi_indirectio.c diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile index 1017def..f4a7f46 100644 --- a/drivers/acpi/arm64/Makefile +++ b/drivers/acpi/arm64/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_ACPI_IORT)+= iort.o obj-$(CONFIG_ACPI_GTDT)+= gtdt.o +obj-$(CONFIG_INDIRECT_PIO) += acpi_indirectio.o diff --git a/drivers/acpi/arm64/acpi_indirectio.c b/drivers/acpi/arm64/acpi_indirectio.c new file mode 100644 index 000..51a1b92 --- /dev/null +++ b/drivers/acpi/arm64/acpi_indirectio.c @@ -0,0 +1,250 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. + * Author: Gabriele Paoloni + * Author: Zhichang Yuan + * Author: John Garry + * + * This file implements functunality to scan the ACPI namespace and config + * devices under "indirect IO" hosts. An "indirect IO" host allows child + * devices to use logical IO accesses when the host, itself, does not provide + * a transparent bridge. The device setup creates a per-child MFD with a + * logical port IO resource. + */ + +#include +#include +#include +#include + +ACPI_MODULE_NAME("indirect IO"); + +#define ACPI_INDIRECT_IO_NAME_LEN 255 + +struct acpi_indirect_io_mfd_cell { + struct mfd_cell_acpi_match acpi_match; + char name[ACPI_INDIRECT_IO_NAME_LEN]; + char pnpid[ACPI_INDIRECT_IO_NAME_LEN]; +}; + +static int acpi_indirect_io_xlat_res(struct acpi_device *adev, +struct acpi_device *host, +struct resource *res) +{ + unsigned long sys_port; + resource_size_t len = res->end - res->start; + + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, len); + if (sys_port == -1UL) + return -EFAULT; + + res->start = sys_port; + res->end = sys_port + len; + + return 0; +} + +/* + * acpi_indirect_io_set_res - set the resources for a child device + * (MFD) of an "indirect IO" host. + * @child: the device node to be updated the I/O resource + * @hostdev: the device node associated with the "indirect IO" host + * @res: double pointer to be set to the address of translated resources + * @num_res: pointer to variable to hold the number of translated resources + * + * Returns 0 when successful, and a negative value for failure. + * + * For a given "indirect IO" host, each child device will have associated + * host-relevative address
[PATCH v13 7/9] ACPI: Translate the I/O range of non-MMIO devices before scanning
On some platforms (such as arm64-based hip06/hip07), access to legacy ISA/LPC devices through access IO space is required, similar to x86 platforms. As the I/O for these devices are not memory mapped like PCI/PCIE MMIO host bridges, they require special low-level device operations through some host to generate IO accesses, i.e. a non- transparent bridge. Through the logical PIO framework, hosts are able to register address ranges in the logical PIO space for IO accesses. For hosts which require a LLDD to generate the IO accesses, through the logical PIO framework the host also registers accessors as a backend to generate the physical bus transactions for IO space accesses (called indirect IO). When describing the indirect IO child device in APCI tables, the IO resource is the host-specific address for the child (generally a bus address). An example is as follows: Device (LPC0) { Name (_HID, "HISI0191") // HiSi LPC Name (_CRS, ResourceTemplate () { Memory32Fixed (ReadWrite, 0xa01b, 0x1000) }) } Device (LPC0.IPMI) { Name (_HID, "IPI0001") Name (LORS, ResourceTemplate() { QWordIO ( ResourceConsumer, MinNotFixed, // _MIF MaxNotFixed, // _MAF PosDecode, EntireRange, 0x0, // _GRA 0xe4,// _MIN 0x3fff, // _MAX 0x0, // _TRA 0x04,// _LEN , , BTIO ) }) Since the IO resource for the child is a host-specific address, special translation are required to retrieve the logical PIO address for that child. To overcome the problem of associating this logical PIO address with the child device, a scan handler is added to scan the ACPI namespace for known indirect IO hosts. This scan handler creates an MFD per child with the translated logical PIO address as it's IO resource, as a substitute for the normal platform device which ACPI would create during device enumeration. Signed-off-by: John Garry Signed-off-by: Zhichang Yuan Signed-off-by: Gabriele Paoloni --- drivers/acpi/arm64/Makefile | 1 + drivers/acpi/arm64/acpi_indirectio.c | 250 +++ drivers/acpi/internal.h | 5 + drivers/acpi/scan.c | 1 + 4 files changed, 257 insertions(+) create mode 100644 drivers/acpi/arm64/acpi_indirectio.c diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile index 1017def..f4a7f46 100644 --- a/drivers/acpi/arm64/Makefile +++ b/drivers/acpi/arm64/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_ACPI_IORT)+= iort.o obj-$(CONFIG_ACPI_GTDT)+= gtdt.o +obj-$(CONFIG_INDIRECT_PIO) += acpi_indirectio.o diff --git a/drivers/acpi/arm64/acpi_indirectio.c b/drivers/acpi/arm64/acpi_indirectio.c new file mode 100644 index 000..51a1b92 --- /dev/null +++ b/drivers/acpi/arm64/acpi_indirectio.c @@ -0,0 +1,250 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2017 HiSilicon Limited, All Rights Reserved. + * Author: Gabriele Paoloni + * Author: Zhichang Yuan + * Author: John Garry + * + * This file implements functunality to scan the ACPI namespace and config + * devices under "indirect IO" hosts. An "indirect IO" host allows child + * devices to use logical IO accesses when the host, itself, does not provide + * a transparent bridge. The device setup creates a per-child MFD with a + * logical port IO resource. + */ + +#include +#include +#include +#include + +ACPI_MODULE_NAME("indirect IO"); + +#define ACPI_INDIRECT_IO_NAME_LEN 255 + +struct acpi_indirect_io_mfd_cell { + struct mfd_cell_acpi_match acpi_match; + char name[ACPI_INDIRECT_IO_NAME_LEN]; + char pnpid[ACPI_INDIRECT_IO_NAME_LEN]; +}; + +static int acpi_indirect_io_xlat_res(struct acpi_device *adev, +struct acpi_device *host, +struct resource *res) +{ + unsigned long sys_port; + resource_size_t len = res->end - res->start; + + sys_port = logic_pio_trans_hwaddr(>fwnode, res->start, len); + if (sys_port == -1UL) + return -EFAULT; + + res->start = sys_port; + res->end = sys_port + len; + + return 0; +} + +/* + * acpi_indirect_io_set_res - set the resources for a child device + * (MFD) of an "indirect IO" host. + * @child: the device node to be updated the I/O resource + * @hostdev: the device node associated with the "indirect IO" host + * @res: double pointer to be set to the address of translated resources + * @num_res: pointer to variable to hold the number of translated resources + * + * Returns 0 when successful, and a negative value for failure. + * + * For a given "indirect IO" host, each child device will have associated + * host-relevative address resource. This function will return the translated + * logical PIO addresses for each child devices resources. + */ +static int acpi_indirect_io_set_res(struct