Currently for ACPI-based FW we fail the probe for an unrecognised child
HID.
However, there is FW in the field with LPC child devices having fake HIDs,
namely "IPI0002", which was an IPMI device invented to support the
initial out-of-tree LPC host driver, different from the final mainline
version.
To provide compatibility support for these dodgy FWs, just discard the
unrecognised HIDs instead of failing the probe altogether.
Tested-by: Zengruan Ye <[email protected]>
Signed-off-by: John Garry <[email protected]>
diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index d5f8545..19d7b6f 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -522,10 +522,9 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
if (!found) {
dev_warn(hostdev,
- "could not find cell for child device (%s)\n",
+ "could not find cell for child device (%s), discarding\n",
hid);
- ret = -ENODEV;
- goto fail;
+ continue;
}
pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);
--
1.9.1
On Thu, Jan 03, 2019 at 07:57:02PM +0800, John Garry wrote:
> Currently for ACPI-based FW we fail the probe for an unrecognised child
> HID.
>
> However, there is FW in the field with LPC child devices having fake HIDs,
> namely "IPI0002", which was an IPMI device invented to support the
> initial out-of-tree LPC host driver, different from the final mainline
> version.
>
> To provide compatibility support for these dodgy FWs, just discard the
> unrecognised HIDs instead of failing the probe altogether.
>
> Tested-by: Zengruan Ye <[email protected]>
> Signed-off-by: John Garry <[email protected]>
>
> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> index d5f8545..19d7b6f 100644
> --- a/drivers/bus/hisi_lpc.c
> +++ b/drivers/bus/hisi_lpc.c
> @@ -522,10 +522,9 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>
> if (!found) {
> dev_warn(hostdev,
> - "could not find cell for child device (%s)\n",
> + "could not find cell for child device (%s), discarding\n",
> hid);
> - ret = -ENODEV;
> - goto fail;
> + continue;
> }
This driver is the equivalent of a board file. Wasn't ACPI supposed to
spare us from these platform device tables? It even has hardcoded clock
information in it. :(
Also, we were told that there'll be expectations for users to update
their ACPI tables if they're incompatible our out of date. Can that be done
here as well?
-Olof
On 04/01/2019 22:25, Olof Johansson wrote:
> On Thu, Jan 03, 2019 at 07:57:02PM +0800, John Garry wrote:
>> Currently for ACPI-based FW we fail the probe for an unrecognised child
>> HID.
>>
>> However, there is FW in the field with LPC child devices having fake HIDs,
>> namely "IPI0002", which was an IPMI device invented to support the
>> initial out-of-tree LPC host driver, different from the final mainline
>> version.
>>
>> To provide compatibility support for these dodgy FWs, just discard the
>> unrecognised HIDs instead of failing the probe altogether.
>>
>> Tested-by: Zengruan Ye <[email protected]>
>> Signed-off-by: John Garry <[email protected]>
>>
>> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
>> index d5f8545..19d7b6f 100644
>> --- a/drivers/bus/hisi_lpc.c
>> +++ b/drivers/bus/hisi_lpc.c
>> @@ -522,10 +522,9 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>>
>> if (!found) {
>> dev_warn(hostdev,
>> - "could not find cell for child device (%s)\n",
>> + "could not find cell for child device (%s), discarding\n",
>> hid);
>> - ret = -ENODEV;
>> - goto fail;
>> + continue;
>> }
>
Hi Olof,
> This driver is the equivalent of a board file. Wasn't ACPI supposed to
> spare us from these platform device tables? It even has hardcoded clock
> information in it. :(
For sure, we should not need a look-up table like this. The background
is complex. I think that if you check these thread+patches then you may
get a better idea of why we require the table:
https://lkml.org/lkml/2018/4/20/278
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/acpi/scan.c?h=v5.0-rc1&id=dfda4492322ed0a1eb9c4d4715c4b90c9af57352
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/bus/hisi_lpc.c?h=next-20190107&id=e0aa1563f8945d9b8f472426d100bed190a4308f
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/bus/hisi_lpc.c?h=next-20190107&id=adf3457b4ce6940885be3e5ee832c6949fba4166
To summarize:
For child devices, we use indirect-PIO method to access. In this, we
need to create a new device with updated resources (see call to
hisi_lpc_acpi_set_io_res() call and
drivers/acpi/scan.c:acpi_is_indirect_io_slave()) for logical PIO space.
One of the child devices is a 8250-compatible UART. For ACPI, we should
use the 8250 PNP driver for this device, i.e. use PNP0501. However PNP
code does not support this indirect-PIO child probe. So we use the
generic 8250 platform device driver instead. Hence the look-up table.
We saw this as the least disruptive method to support this legacy host
controller.
As for the clock info in the driver, we're just setting some driver wait
times depending on fixed clock information. So not configuring clocks or
the like.
>
> Also, we were told that there'll be expectations for users to update
> their ACPI tables if they're incompatible our out of date. Can that be done
> here as well?
If you're referring to this comment "To provide compatibility support
for these dodgy FWs", we're saying that if the FW has IPI0001 and
IPI0002 (this being invalid) child devices, then we can handle it by
just discarding IPI0002.
Thanks,
John
>
>
> -Olof
>
>
Hi John,
On 1/3/2019 11:57 AM, John Garry wrote:
> Currently for ACPI-based FW we fail the probe for an unrecognised child
> HID.
>
> However, there is FW in the field with LPC child devices having fake HIDs,
> namely "IPI0002", which was an IPMI device invented to support the
> initial out-of-tree LPC host driver, different from the final mainline
> version.
>
> To provide compatibility support for these dodgy FWs, just discard the
> unrecognised HIDs instead of failing the probe altogether.
>
> Tested-by: Zengruan Ye <[email protected]>
> Signed-off-by: John Garry <[email protected]>
Updated the subject as "bus: hisi_lpc: xxx" to follow the style and
applied to the hisilicon tree.
Thanks!
Best Regards,
Wei
>
> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> index d5f8545..19d7b6f 100644
> --- a/drivers/bus/hisi_lpc.c
> +++ b/drivers/bus/hisi_lpc.c
> @@ -522,10 +522,9 @@ static int hisi_lpc_acpi_probe(struct device *hostdev)
>
> if (!found) {
> dev_warn(hostdev,
> - "could not find cell for child device (%s)\n",
> + "could not find cell for child device (%s), discarding\n",
> hid);
> - ret = -ENODEV;
> - goto fail;
> + continue;
> }
>
> pdev = platform_device_alloc(cell->name, PLATFORM_DEVID_AUTO);
>