Re: [PATCH v15 8/9] HISI LPC: Add ACPI support

[Andy Shevchenko]

On Fri, 2018-03-02 at 10:19 +, John Garry wrote:
> On 01/03/2018 19:50, Andy Shevchenko wrote:
> > On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote:

> > Ok, this needs to be thought about a bit more.
> > 
> > I guess I understand what's is the problem with PNP IDs in the
> > driver.
> > 
> > You probe your LPC device quite late.
> > One option is to move from classical probe to a event-driven model,
> > i.e.
> > via registering a notifier (see acpi_lpss.c), preparing necessary
> > stuff
> > at earlier stages and then register devices by their enumeration and
> > appearance.
> > 
> > Though, if I would be you I would seek a opinion from Rafael and
> > Mika
> > (maybe others as well).
> 
> I would like to give a bit more background on this HW. This HW is now 
> for us a legacy device. It will be used in no more chipsets. It is
> only 
> used in hip06 and hip07 chipsets on our D03 and D05 boards, 
> respectively. On these boards we have the following LPC slave devices
> only:
> D03: UART, IPMI
> D05: IPMI
> 
> Supporting IPMI for D05 is required. Supporting legacy D03 and the
> UART 
> is a "nice-to-have". But it is definitely ok for us to not support
> this 
> device.

I see. But it raises another question to the whole series, why do we
introduce a generic indirect IO for only two devices ever?

Can't it be done in the (MFD) driver itself?

Possible another option, is to introduce a specific regmap for that and
use it in the drivers (though it might be not so trivial with existing
ones).

> Our previous ACPI support solution did use a scan handler for this
> host, 
> but there was some sensible pushback on this - please check this if
> not 
> familiar: https://lkml.org/lkml/2018/2/14/532

> Overall it does not make sense to try to move this back to
> drivers/acpi 
> and receive more pushback from that direction,

Ah, I think the ARM people just worried mostly about arm64 in the
pathnames, though your case is very similar to our LPSS for only few
SoCs.

I would rather think if you move it directly to drivers/acpi it would be
fine.

>  and only delay 
> indefinitely upstreaming this driver (which is now running at ~27
> months 
> since v1) to just support a PNP-compatible device which we don't care 
> too much about.

Ouch!

As I suggested before, you would better to get an input from
maintainers.

My personal opinion that the handler approach is cleaner, though it's
still non-generic stuff. I think that this is what Mika referred to in
his mail.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v15 8/9] HISI LPC: Add ACPI support

[Andy Shevchenko]

On Fri, 2018-03-02 at 10:19 +, John Garry wrote:
> On 01/03/2018 19:50, Andy Shevchenko wrote:
> > On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote:

> > Ok, this needs to be thought about a bit more.
> > 
> > I guess I understand what's is the problem with PNP IDs in the
> > driver.
> > 
> > You probe your LPC device quite late.
> > One option is to move from classical probe to a event-driven model,
> > i.e.
> > via registering a notifier (see acpi_lpss.c), preparing necessary
> > stuff
> > at earlier stages and then register devices by their enumeration and
> > appearance.
> > 
> > Though, if I would be you I would seek a opinion from Rafael and
> > Mika
> > (maybe others as well).
> 
> I would like to give a bit more background on this HW. This HW is now 
> for us a legacy device. It will be used in no more chipsets. It is
> only 
> used in hip06 and hip07 chipsets on our D03 and D05 boards, 
> respectively. On these boards we have the following LPC slave devices
> only:
> D03: UART, IPMI
> D05: IPMI
> 
> Supporting IPMI for D05 is required. Supporting legacy D03 and the
> UART 
> is a "nice-to-have". But it is definitely ok for us to not support
> this 
> device.

I see. But it raises another question to the whole series, why do we
introduce a generic indirect IO for only two devices ever?

Can't it be done in the (MFD) driver itself?

Possible another option, is to introduce a specific regmap for that and
use it in the drivers (though it might be not so trivial with existing
ones).

> Our previous ACPI support solution did use a scan handler for this
> host, 
> but there was some sensible pushback on this - please check this if
> not 
> familiar: https://lkml.org/lkml/2018/2/14/532

> Overall it does not make sense to try to move this back to
> drivers/acpi 
> and receive more pushback from that direction,

Ah, I think the ARM people just worried mostly about arm64 in the
pathnames, though your case is very similar to our LPSS for only few
SoCs.

I would rather think if you move it directly to drivers/acpi it would be
fine.

>  and only delay 
> indefinitely upstreaming this driver (which is now running at ~27
> months 
> since v1) to just support a PNP-compatible device which we don't care 
> too much about.

Ouch!

As I suggested before, you would better to get an input from
maintainers.

My personal opinion that the handler approach is cleaner, though it's
still non-generic stuff. I think that this is what Mika referred to in
his mail.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v15 8/9] HISI LPC: Add ACPI support

[John Garry]

On 01/03/2018 19:50, Andy Shevchenko wrote:

On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote:

Based on the previous patches, this patch supports the
LPC host on hip06/hip07 for ACPI FW.

It is the responsibility of the LPC host driver to
enumerate the child devices, as the ACPI scan code will
not enumerate children of "indirect IO" hosts.

The ACPI table for the LPC host controller and the child
devices is in the following format:
  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 resources of the child devices need to be
translated from LPC bus addresses to logical PIO addresses,
and we shouldn't modify the resources of the devices
generated in the FW scan, a per-child MFD is created as
a substitute. The MFD IO resources will be the translated
bus addresses of the ACPI child.




Hi Andy,


Ok, this needs to be thought about a bit more.

I guess I understand what's is the problem with PNP IDs in the driver.

You probe your LPC device quite late.
One option is to move from classical probe to a event-driven model, i.e.
via registering a notifier (see acpi_lpss.c), preparing necessary stuff
at earlier stages and then register devices by their enumeration and
appearance.

Though, if I would be you I would seek a opinion from Rafael and Mika
(maybe others as well).


I would like to give a bit more background on this HW. This HW is now 
for us a legacy device. It will be used in no more chipsets. It is only 
used in hip06 and hip07 chipsets on our D03 and D05 boards, 
respectively. On these boards we have the following LPC slave devices only:

D03: UART, IPMI
D05: IPMI

Supporting IPMI for D05 is required. Supporting legacy D03 and the UART 
is a "nice-to-have". But it is definitely ok for us to not support this 
device.


Our previous ACPI support solution did use a scan handler for this host, 
but there was some sensible pushback on this - please check this if not 
familiar: https://lkml.org/lkml/2018/2/14/532


Overall it does not make sense to try to move this back to drivers/acpi 
and receive more pushback from that direction, and only delay 
indefinitely upstreaming this driver (which is now running at ~27 months 
since v1) to just support a PNP-compatible device which we don't care 
too much about.




See also some comments below.


+#ifdef CONFIG_ACPI
+#define MFD_CHILD_NAME_PREFIX DRV_NAME"-"



+#define MFD_CHILD_NAME_LEN (ACPI_ID_LEN +
sizeof(MFD_CHILD_NAME_PREFIX))


..._PREFIX) - 1)


I didn't think so. But now I have noticed that ACPI_ID_LEN is 9 (which 
of course it needs to be for pnpid name), and not 8. I was thinking that 
the sizeof(MFD_CHILD_NAME_PREFIX) was providing the extra byte for the 
NULL terminator I required.




?


+static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev,
+struct acpi_device *host,
+struct resource *res)
+{
+   unsigned long sys_port;



+   resource_size_t len = res->end - res->start;


resource_size()


should be ok




+   return 0;
+}




+static int hisi_lpc_acpi_set_io_res(struct device *child,
+   struct device *hostdev,
+   const struct resource **res,
+   int *num_res)
+{



+   /*
+* The following code segment to retrieve the resources is
common to
+* acpi_create_platform_device(), so consider a common helper
function
+* in future.
+*/
+   count = acpi_dev_get_resources(adev, _list, NULL,
NULL);
+   if (count <= 0) {
+   dev_dbg(child, "failed to get resources\n");



+   return count ? count : -EIO;


count == 0 --> return 0;



The idea is that having no IO resources is a failure for a slave device 
on this bus. So then, if count == 0, we should error.



Is it by design? (I didn't check acpi_create_platform_device() though)


+   }
+
+   resources = devm_kcalloc(hostdev, count, sizeof(*resources),
+GFP_KERNEL);
+   if (!resources) {
+   dev_warn(hostdev, "could not allocate memory for %d
resources\n",
+count);
+   acpi_dev_free_resource_list(_list);
+   return -ENOMEM;
+   }
+   count = 0;
+   list_for_each_entry(rentry, _list, node)
+   resources[count++] = *rentry->res;
+
+   

Re: [PATCH v15 8/9] HISI LPC: Add ACPI support

[John Garry]

On 01/03/2018 19:50, Andy Shevchenko wrote:

On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote:

Based on the previous patches, this patch supports the
LPC host on hip06/hip07 for ACPI FW.

It is the responsibility of the LPC host driver to
enumerate the child devices, as the ACPI scan code will
not enumerate children of "indirect IO" hosts.

The ACPI table for the LPC host controller and the child
devices is in the following format:
  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 resources of the child devices need to be
translated from LPC bus addresses to logical PIO addresses,
and we shouldn't modify the resources of the devices
generated in the FW scan, a per-child MFD is created as
a substitute. The MFD IO resources will be the translated
bus addresses of the ACPI child.




Hi Andy,


Ok, this needs to be thought about a bit more.

I guess I understand what's is the problem with PNP IDs in the driver.

You probe your LPC device quite late.
One option is to move from classical probe to a event-driven model, i.e.
via registering a notifier (see acpi_lpss.c), preparing necessary stuff
at earlier stages and then register devices by their enumeration and
appearance.

Though, if I would be you I would seek a opinion from Rafael and Mika
(maybe others as well).


I would like to give a bit more background on this HW. This HW is now 
for us a legacy device. It will be used in no more chipsets. It is only 
used in hip06 and hip07 chipsets on our D03 and D05 boards, 
respectively. On these boards we have the following LPC slave devices only:

D03: UART, IPMI
D05: IPMI

Supporting IPMI for D05 is required. Supporting legacy D03 and the UART 
is a "nice-to-have". But it is definitely ok for us to not support this 
device.


Our previous ACPI support solution did use a scan handler for this host, 
but there was some sensible pushback on this - please check this if not 
familiar: https://lkml.org/lkml/2018/2/14/532


Overall it does not make sense to try to move this back to drivers/acpi 
and receive more pushback from that direction, and only delay 
indefinitely upstreaming this driver (which is now running at ~27 months 
since v1) to just support a PNP-compatible device which we don't care 
too much about.




See also some comments below.


+#ifdef CONFIG_ACPI
+#define MFD_CHILD_NAME_PREFIX DRV_NAME"-"



+#define MFD_CHILD_NAME_LEN (ACPI_ID_LEN +
sizeof(MFD_CHILD_NAME_PREFIX))


..._PREFIX) - 1)


I didn't think so. But now I have noticed that ACPI_ID_LEN is 9 (which 
of course it needs to be for pnpid name), and not 8. I was thinking that 
the sizeof(MFD_CHILD_NAME_PREFIX) was providing the extra byte for the 
NULL terminator I required.




?


+static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev,
+struct acpi_device *host,
+struct resource *res)
+{
+   unsigned long sys_port;



+   resource_size_t len = res->end - res->start;


resource_size()


should be ok




+   return 0;
+}




+static int hisi_lpc_acpi_set_io_res(struct device *child,
+   struct device *hostdev,
+   const struct resource **res,
+   int *num_res)
+{



+   /*
+* The following code segment to retrieve the resources is
common to
+* acpi_create_platform_device(), so consider a common helper
function
+* in future.
+*/
+   count = acpi_dev_get_resources(adev, _list, NULL,
NULL);
+   if (count <= 0) {
+   dev_dbg(child, "failed to get resources\n");



+   return count ? count : -EIO;


count == 0 --> return 0;



The idea is that having no IO resources is a failure for a slave device 
on this bus. So then, if count == 0, we should error.



Is it by design? (I didn't check acpi_create_platform_device() though)


+   }
+
+   resources = devm_kcalloc(hostdev, count, sizeof(*resources),
+GFP_KERNEL);
+   if (!resources) {
+   dev_warn(hostdev, "could not allocate memory for %d
resources\n",
+count);
+   acpi_dev_free_resource_list(_list);
+   return -ENOMEM;
+   }
+   count = 0;
+   list_for_each_entry(rentry, _list, node)
+   resources[count++] = *rentry->res;
+
+   

Re: [PATCH v15 8/9] HISI LPC: Add ACPI support

[Andy Shevchenko]

On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote:
> Based on the previous patches, this patch supports the
> LPC host on hip06/hip07 for ACPI FW.
> 
> It is the responsibility of the LPC host driver to
> enumerate the child devices, as the ACPI scan code will
> not enumerate children of "indirect IO" hosts.
> 
> The ACPI table for the LPC host controller and the child
> devices is in the following format:
>   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 resources of the child devices need to be
> translated from LPC bus addresses to logical PIO addresses,
> and we shouldn't modify the resources of the devices
> generated in the FW scan, a per-child MFD is created as
> a substitute. The MFD IO resources will be the translated
> bus addresses of the ACPI child.

Ok, this needs to be thought about a bit more.

I guess I understand what's is the problem with PNP IDs in the driver.

You probe your LPC device quite late.
One option is to move from classical probe to a event-driven model, i.e.
via registering a notifier (see acpi_lpss.c), preparing necessary stuff
at earlier stages and then register devices by their enumeration and
appearance.

Though, if I would be you I would seek a opinion from Rafael and Mika
(maybe others as well).

See also some comments below.

> +#ifdef CONFIG_ACPI
> +#define MFD_CHILD_NAME_PREFIX DRV_NAME"-"

> +#define MFD_CHILD_NAME_LEN (ACPI_ID_LEN +
> sizeof(MFD_CHILD_NAME_PREFIX))

..._PREFIX) - 1)

?

> +static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev,
> +  struct acpi_device *host,
> +  struct resource *res)
> +{
> + unsigned long sys_port;

> + resource_size_t len = res->end - res->start;

resource_size()

> + return 0;
> +}


> +static int hisi_lpc_acpi_set_io_res(struct device *child,
> + struct device *hostdev,
> + const struct resource **res,
> + int *num_res)
> +{

> + /*
> +  * The following code segment to retrieve the resources is
> common to
> +  * acpi_create_platform_device(), so consider a common helper
> function
> +  * in future.
> +  */
> + count = acpi_dev_get_resources(adev, _list, NULL,
> NULL);
> + if (count <= 0) {
> + dev_dbg(child, "failed to get resources\n");

> + return count ? count : -EIO;

count == 0 --> return 0;

Is it by design? (I didn't check acpi_create_platform_device() though)

> + }
> +
> + resources = devm_kcalloc(hostdev, count, sizeof(*resources),
> +  GFP_KERNEL);
> + if (!resources) {
> + dev_warn(hostdev, "could not allocate memory for %d
> resources\n",
> +  count);
> + 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);

> +
> + return 0;
> +}

> +
> +/*
> + * hisi_lpc_acpi_probe - probe children for ACPI FW
> + * @hostdev: LPC host device pointer
> + *
> + * Returns 0 when successful, and a negative value for failure.
> + *
> + * Scan all child devices and create a per-device MFD with
> + * logical PIO translated IO resources.
> + */
> +static int hisi_lpc_acpi_probe(struct device *hostdev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(hostdev);
> + struct hisi_lpc_mfd_cell *hisi_lpc_mfd_cells;
> + struct mfd_cell *mfd_cells;
> + struct acpi_device *child;
> + int size, ret, count = 0, cell_num = 0;
> +
> + list_for_each_entry(child, >children, node)
> + cell_num++;
> +
> + /* allocate the mfd cell and companion acpi info, one per
> child */
> + size = sizeof(*mfd_cells) + sizeof(*hisi_lpc_mfd_cells);
> + mfd_cells = devm_kcalloc(hostdev, cell_num, size,
> GFP_KERNEL);
> + if (!mfd_cells)
> + return -ENOMEM;
> +

> + hisi_lpc_mfd_cells = (struct hisi_lpc_mfd_cell *)
> + _cells[cell_num];

One line, please.

Just noticed that calloc() memory layout is not the same how you are
using it.

> + /* Only consider the children of the host */
> + list_for_each_entry(child, >children, node) {
> + struct mfd_cell 

Re: [PATCH v15 8/9] HISI LPC: Add ACPI support

[Andy Shevchenko]

On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote:
> Based on the previous patches, this patch supports the
> LPC host on hip06/hip07 for ACPI FW.
> 
> It is the responsibility of the LPC host driver to
> enumerate the child devices, as the ACPI scan code will
> not enumerate children of "indirect IO" hosts.
> 
> The ACPI table for the LPC host controller and the child
> devices is in the following format:
>   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 resources of the child devices need to be
> translated from LPC bus addresses to logical PIO addresses,
> and we shouldn't modify the resources of the devices
> generated in the FW scan, a per-child MFD is created as
> a substitute. The MFD IO resources will be the translated
> bus addresses of the ACPI child.

Ok, this needs to be thought about a bit more.

I guess I understand what's is the problem with PNP IDs in the driver.

You probe your LPC device quite late.
One option is to move from classical probe to a event-driven model, i.e.
via registering a notifier (see acpi_lpss.c), preparing necessary stuff
at earlier stages and then register devices by their enumeration and
appearance.

Though, if I would be you I would seek a opinion from Rafael and Mika
(maybe others as well).

See also some comments below.

> +#ifdef CONFIG_ACPI
> +#define MFD_CHILD_NAME_PREFIX DRV_NAME"-"

> +#define MFD_CHILD_NAME_LEN (ACPI_ID_LEN +
> sizeof(MFD_CHILD_NAME_PREFIX))

..._PREFIX) - 1)

?

> +static int hisi_lpc_acpi_xlat_io_res(struct acpi_device *adev,
> +  struct acpi_device *host,
> +  struct resource *res)
> +{
> + unsigned long sys_port;

> + resource_size_t len = res->end - res->start;

resource_size()

> + return 0;
> +}


> +static int hisi_lpc_acpi_set_io_res(struct device *child,
> + struct device *hostdev,
> + const struct resource **res,
> + int *num_res)
> +{

> + /*
> +  * The following code segment to retrieve the resources is
> common to
> +  * acpi_create_platform_device(), so consider a common helper
> function
> +  * in future.
> +  */
> + count = acpi_dev_get_resources(adev, _list, NULL,
> NULL);
> + if (count <= 0) {
> + dev_dbg(child, "failed to get resources\n");

> + return count ? count : -EIO;

count == 0 --> return 0;

Is it by design? (I didn't check acpi_create_platform_device() though)

> + }
> +
> + resources = devm_kcalloc(hostdev, count, sizeof(*resources),
> +  GFP_KERNEL);
> + if (!resources) {
> + dev_warn(hostdev, "could not allocate memory for %d
> resources\n",
> +  count);
> + 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);

> +
> + return 0;
> +}

> +
> +/*
> + * hisi_lpc_acpi_probe - probe children for ACPI FW
> + * @hostdev: LPC host device pointer
> + *
> + * Returns 0 when successful, and a negative value for failure.
> + *
> + * Scan all child devices and create a per-device MFD with
> + * logical PIO translated IO resources.
> + */
> +static int hisi_lpc_acpi_probe(struct device *hostdev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(hostdev);
> + struct hisi_lpc_mfd_cell *hisi_lpc_mfd_cells;
> + struct mfd_cell *mfd_cells;
> + struct acpi_device *child;
> + int size, ret, count = 0, cell_num = 0;
> +
> + list_for_each_entry(child, >children, node)
> + cell_num++;
> +
> + /* allocate the mfd cell and companion acpi info, one per
> child */
> + size = sizeof(*mfd_cells) + sizeof(*hisi_lpc_mfd_cells);
> + mfd_cells = devm_kcalloc(hostdev, cell_num, size,
> GFP_KERNEL);
> + if (!mfd_cells)
> + return -ENOMEM;
> +

> + hisi_lpc_mfd_cells = (struct hisi_lpc_mfd_cell *)
> + _cells[cell_num];

One line, please.

Just noticed that calloc() memory layout is not the same how you are
using it.

> + /* Only consider the children of the host */
> + list_for_each_entry(child, >children, node) {
> + struct mfd_cell