[PATCH] usb: typec: intel_pmc_mux: Add new ACPI ID for Meteor Lake IOM device

[PATCH] usb: typec: intel_pmc_mux: Add new ACPI ID for Meteor Lake IOM device

[Utkarsh Patel]

Intel Meteor Lake IOM uses 64bit IOM BASE address than previous Intel
Generations which use 32bit.

Added code to support 64bit IOM BASE address change with necessary ACPI
resource extraction support.

Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/mux/intel_pmc_mux.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
index 47b733f78fb0..a8e273fe204a 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -571,9 +571,11 @@ static int pmc_usb_register_port(struct pmc_usb *pmc, int index,
 
 static int is_memory(struct acpi_resource *res, void *data)
 {
-	struct resource r;
+	struct resource_win win = {};
+	struct resource *r = &win.res;
 
-	return !acpi_dev_resource_memory(res, &r);
+	return !(acpi_dev_resource_memory(res, r) ||
+		 acpi_dev_resource_address_space(res, &win));
 }
 
 /* IOM ACPI IDs and IOM_PORT_STATUS_OFFSET */
@@ -583,6 +585,9 @@ static const struct acpi_device_id iom_acpi_ids[] = {
 
 	/* AlderLake */
 	{ "INTC1079", 0x160, },
+
+	/* Meteor Lake */
+	{ "INTC107A", 0x160, },
 	{}
 };
 
-- 
2.25.1

Re: [PATCH] usb: typec: intel_pmc_mux: Add new ACPI ID for Meteor Lake IOM device

[Greg KH]

On Thu, Jul 28, 2022 at 05:30:33PM -0700, Utkarsh Patel wrote:
> Intel Meteor Lake IOM uses 64bit IOM BASE address than previous Intel
> Generations which use 32bit.

I can not parse this sentence, sorry.

> Added code to support 64bit IOM BASE address change with necessary ACPI
> resource extraction support.

Again, I do not understand :(

> 
> Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

No need to backport this to stable kernels?

> ---
>  drivers/usb/typec/mux/intel_pmc_mux.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> index 47b733f78fb0..a8e273fe204a 100644
> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> @@ -571,9 +571,11 @@ static int pmc_usb_register_port(struct pmc_usb *pmc, int index,
>  
>  static int is_memory(struct acpi_resource *res, void *data)
>  {
> -	struct resource r;
> +	struct resource_win win = {};
> +	struct resource *r = &win.res;
>  
> -	return !acpi_dev_resource_memory(res, &r);
> +	return !(acpi_dev_resource_memory(res, r) ||
> +		 acpi_dev_resource_address_space(res, &win));

I don't understand, what is the extra check here doing?  Why is that
needed and why aren't you triggering off of a platform type?

thanks,

greg k-h

Re: [PATCH] usb: typec: intel_pmc_mux: Add new ACPI ID for Meteor Lake IOM device

[Heikki Krogerus]

Hi Utkarsh,

On Thu, Jul 28, 2022 at 05:30:33PM -0700, Utkarsh Patel wrote:
> Intel Meteor Lake IOM uses 64bit IOM BASE address than previous Intel
> Generations which use 32bit.
> 
> Added code to support 64bit IOM BASE address change with necessary ACPI
> resource extraction support.
> 
> Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/typec/mux/intel_pmc_mux.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> index 47b733f78fb0..a8e273fe204a 100644
> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> @@ -571,9 +571,11 @@ static int pmc_usb_register_port(struct pmc_usb *pmc, int index,
>  
>  static int is_memory(struct acpi_resource *res, void *data)
>  {
> -	struct resource r;
> +	struct resource_win win = {};
> +	struct resource *r = &win.res;
>  
> -	return !acpi_dev_resource_memory(res, &r);
> +	return !(acpi_dev_resource_memory(res, r) ||
> +		 acpi_dev_resource_address_space(res, &win));
>  }

I realised that now that is_memory() function is basically just a copy
of the is_memory() function that's in drivers/acpi/resources.c, so I
think we need to handle this a bit differently. There are a few places
in kernel that have that same check.

One way would be to just export the is_memory() function that's in
drivers/acpi/resources.c, but since we have already a wrapper function
acpi_dev_get_dma_resources() for DMA resouces, I think we could have a
similar wrapper for common memory resources.

I'll prepare a patch(s) where I'll propose a new wrapper function
acpi_dev_get_memory_resources() that will take care of the is_memory()
check, and then convert the users (including this driver). After that,
this patch only needs to add the ID.


thanks,


>  /* IOM ACPI IDs and IOM_PORT_STATUS_OFFSET */
> @@ -583,6 +585,9 @@ static const struct acpi_device_id iom_acpi_ids[] = {
>  
>  	/* AlderLake */
>  	{ "INTC1079", 0x160, },
> +
> +	/* Meteor Lake */
> +	{ "INTC107A", 0x160, },
>  	{}
>  };
>  
> -- 
> 2.25.1

-- 
heikki

RE: [PATCH] usb: typec: intel_pmc_mux: Add new ACPI ID for Meteor Lake IOM device

[Patel, Utkarsh H]

Hi Heikki,

Thank you for the review and feedback. 

> >  drivers/usb/typec/mux/intel_pmc_mux.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c
> > b/drivers/usb/typec/mux/intel_pmc_mux.c
> > index 47b733f78fb0..a8e273fe204a 100644
> > --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> > @@ -571,9 +571,11 @@ static int pmc_usb_register_port(struct pmc_usb
> > *pmc, int index,
> >
> >  static int is_memory(struct acpi_resource *res, void *data)  {
> > -	struct resource r;
> > +	struct resource_win win = {};
> > +	struct resource *r = &win.res;
> >
> > -	return !acpi_dev_resource_memory(res, &r);
> > +	return !(acpi_dev_resource_memory(res, r) ||
> > +		 acpi_dev_resource_address_space(res, &win));
> >  }
> 
> I realised that now that is_memory() function is basically just a copy of the
> is_memory() function that's in drivers/acpi/resources.c, so I think we need to
> handle this a bit differently. There are a few places in kernel that have that
> same check.
> 
> One way would be to just export the is_memory() function that's in
> drivers/acpi/resources.c, but since we have already a wrapper function
> acpi_dev_get_dma_resources() for DMA resouces, I think we could have a
> similar wrapper for common memory resources.
> 
> I'll prepare a patch(s) where I'll propose a new wrapper function
> acpi_dev_get_memory_resources() that will take care of the is_memory()
> check, and then convert the users (including this driver). After that, this patch
> only needs to add the ID.

I will wait for your changes then. 

Sincerely,
Utkarsh Patel.