diff mbox series

[v2] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs

Message ID 20230414081910.1336405-1-rajat.khandelwal@linux.intel.com
State New
Headers
Series [v2] usb: typec: intel_pmc_mux: Expose IOM port status to debugfs |

Commit Message

Rajat Khandelwal April 14, 2023, 8:19 a.m. UTC 
  IOM status has a crucial role during debugging to check the
current state of the type-C port.
There are ways to fetch the status, but all those require the
IOM port status offset, which could change with platform.

Make a debugfs directory for intel_pmc_mux and expose the status
under it per port basis.

Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
---

v2:
1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
2. Remove explicitly defined one-liner functions

 drivers/usb/typec/mux/intel_pmc_mux.c | 34 +++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Comments

Greg KH April 15, 2023, 5:31 a.m. UTC | #1 
On Fri, Apr 14, 2023 at 01:49:10PM +0530, Rajat Khandelwal wrote:
> IOM status has a crucial role during debugging to check the
> current state of the type-C port.
> There are ways to fetch the status, but all those require the
> IOM port status offset, which could change with platform.
> 
> Make a debugfs directory for intel_pmc_mux and expose the status
> under it per port basis.
> 
> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> ---
> 
> v2:
> 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
> 2. Remove explicitly defined one-liner functions
> 
>  drivers/usb/typec/mux/intel_pmc_mux.c | 34 +++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> index 34e4188a40ff..1d43b111781e 100644
> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> @@ -15,6 +15,7 @@
>  #include <linux/usb/typec_mux.h>
>  #include <linux/usb/typec_dp.h>
>  #include <linux/usb/typec_tbt.h>
> +#include <linux/debugfs.h>
>  
>  #include <asm/intel_scu_ipc.h>
>  
> @@ -639,9 +640,34 @@ static int pmc_usb_probe_iom(struct pmc_usb *pmc)
>  	return 0;
>  }
>  
> +static int port_iom_status_show(struct seq_file *s, void *unused)
> +{
> +	struct pmc_usb_port *port = s->private;
> +
> +	update_port_status(port);
> +	seq_printf(s, "0x%08x\n", port->iom_status);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(port_iom_status);
> +
> +static void pmc_mux_port_debugfs_init(struct pmc_usb_port *port,
> +				      struct dentry *pmc_mux_debugfs_root)
> +{
> +	struct dentry *debugfs_dir;
> +	char name[6];
> +
> +	snprintf(name, sizeof(name), "port%d", port->usb3_port - 1);
> +
> +	debugfs_dir = debugfs_create_dir(name, pmc_mux_debugfs_root);
> +	debugfs_create_file("iom_status", 0400, debugfs_dir, port,
> +			    &port_iom_status_fops);
> +}
> +
>  static int pmc_usb_probe(struct platform_device *pdev)
>  {
>  	struct fwnode_handle *fwnode = NULL;
> +	struct dentry *pmc_mux_debugfs_root;
>  	struct pmc_usb *pmc;
>  	int i = 0;
>  	int ret;
> @@ -674,6 +700,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	pmc_mux_debugfs_root = debugfs_create_dir("intel_pmc_mux", NULL);

What happens when you have more than one device in the system at the
same time?

thanks,

greg k-h
Rajat Khandelwal April 17, 2023, 5:58 a.m. UTC | #2 
Hi,

On 4/15/2023 11:01 AM, Greg KH wrote:
> On Fri, Apr 14, 2023 at 01:49:10PM +0530, Rajat Khandelwal wrote:
>> IOM status has a crucial role during debugging to check the
>> current state of the type-C port.
>> There are ways to fetch the status, but all those require the
>> IOM port status offset, which could change with platform.
>>
>> Make a debugfs directory for intel_pmc_mux and expose the status
>> under it per port basis.
>>
>> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
>> ---
>>
>> v2:
>> 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
>> 2. Remove explicitly defined one-liner functions
>>
>>   drivers/usb/typec/mux/intel_pmc_mux.c | 34 +++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
>> index 34e4188a40ff..1d43b111781e 100644
>> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
>> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/usb/typec_mux.h>
>>   #include <linux/usb/typec_dp.h>
>>   #include <linux/usb/typec_tbt.h>
>> +#include <linux/debugfs.h>
>>   
>>   #include <asm/intel_scu_ipc.h>
>>   
>> @@ -639,9 +640,34 @@ static int pmc_usb_probe_iom(struct pmc_usb *pmc)
>>   	return 0;
>>   }
>>   
>> +static int port_iom_status_show(struct seq_file *s, void *unused)
>> +{
>> +	struct pmc_usb_port *port = s->private;
>> +
>> +	update_port_status(port);
>> +	seq_printf(s, "0x%08x\n", port->iom_status);
>> +
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(port_iom_status);
>> +
>> +static void pmc_mux_port_debugfs_init(struct pmc_usb_port *port,
>> +				      struct dentry *pmc_mux_debugfs_root)
>> +{
>> +	struct dentry *debugfs_dir;
>> +	char name[6];
>> +
>> +	snprintf(name, sizeof(name), "port%d", port->usb3_port - 1);
>> +
>> +	debugfs_dir = debugfs_create_dir(name, pmc_mux_debugfs_root);
>> +	debugfs_create_file("iom_status", 0400, debugfs_dir, port,
>> +			    &port_iom_status_fops);
>> +}
>> +
>>   static int pmc_usb_probe(struct platform_device *pdev)
>>   {
>>   	struct fwnode_handle *fwnode = NULL;
>> +	struct dentry *pmc_mux_debugfs_root;
>>   	struct pmc_usb *pmc;
>>   	int i = 0;
>>   	int ret;
>> @@ -674,6 +700,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		return ret;
>>   
>> +	pmc_mux_debugfs_root = debugfs_create_dir("intel_pmc_mux", NULL);
> What happens when you have more than one device in the system at the
> same time?

I'm sorry I didn't understand the question. We would have separate directories
for all the ports which would contain the 'iom_status' file, thus representing
status for all the ports individually.
Can you rephrase the question since I guess you had something else in mind?

Thanks
Rajat

>
> thanks,
>
> greg k-h
Greg KH April 17, 2023, 6:12 a.m. UTC | #3 
On Mon, Apr 17, 2023 at 11:28:18AM +0530, Rajat Khandelwal wrote:
> Hi,
> 
> On 4/15/2023 11:01 AM, Greg KH wrote:
> > On Fri, Apr 14, 2023 at 01:49:10PM +0530, Rajat Khandelwal wrote:
> > > IOM status has a crucial role during debugging to check the
> > > current state of the type-C port.
> > > There are ways to fetch the status, but all those require the
> > > IOM port status offset, which could change with platform.
> > > 
> > > Make a debugfs directory for intel_pmc_mux and expose the status
> > > under it per port basis.
> > > 
> > > Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> > > ---
> > > 
> > > v2:
> > > 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
> > > 2. Remove explicitly defined one-liner functions
> > > 
> > >   drivers/usb/typec/mux/intel_pmc_mux.c | 34 +++++++++++++++++++++++++++
> > >   1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > index 34e4188a40ff..1d43b111781e 100644
> > > --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> > > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > @@ -15,6 +15,7 @@
> > >   #include <linux/usb/typec_mux.h>
> > >   #include <linux/usb/typec_dp.h>
> > >   #include <linux/usb/typec_tbt.h>
> > > +#include <linux/debugfs.h>
> > >   #include <asm/intel_scu_ipc.h>
> > > @@ -639,9 +640,34 @@ static int pmc_usb_probe_iom(struct pmc_usb *pmc)
> > >   	return 0;
> > >   }
> > > +static int port_iom_status_show(struct seq_file *s, void *unused)
> > > +{
> > > +	struct pmc_usb_port *port = s->private;
> > > +
> > > +	update_port_status(port);
> > > +	seq_printf(s, "0x%08x\n", port->iom_status);
> > > +
> > > +	return 0;
> > > +}
> > > +DEFINE_SHOW_ATTRIBUTE(port_iom_status);
> > > +
> > > +static void pmc_mux_port_debugfs_init(struct pmc_usb_port *port,
> > > +				      struct dentry *pmc_mux_debugfs_root)
> > > +{
> > > +	struct dentry *debugfs_dir;
> > > +	char name[6];
> > > +
> > > +	snprintf(name, sizeof(name), "port%d", port->usb3_port - 1);
> > > +
> > > +	debugfs_dir = debugfs_create_dir(name, pmc_mux_debugfs_root);
> > > +	debugfs_create_file("iom_status", 0400, debugfs_dir, port,
> > > +			    &port_iom_status_fops);
> > > +}
> > > +
> > >   static int pmc_usb_probe(struct platform_device *pdev)
> > >   {
> > >   	struct fwnode_handle *fwnode = NULL;
> > > +	struct dentry *pmc_mux_debugfs_root;
> > >   	struct pmc_usb *pmc;
> > >   	int i = 0;
> > >   	int ret;
> > > @@ -674,6 +700,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
> > >   	if (ret)
> > >   		return ret;
> > > +	pmc_mux_debugfs_root = debugfs_create_dir("intel_pmc_mux", NULL);
> > What happens when you have more than one device in the system at the
> > same time?
> 
> I'm sorry I didn't understand the question. We would have separate directories
> for all the ports which would contain the 'iom_status' file, thus representing
> status for all the ports individually.
> Can you rephrase the question since I guess you had something else in mind?

Can you please show the output of the directory
/sys/kernel/debug/intel_pmc_mux/ with multiple pmc devices in the system
at the same time?

This code seems to want to create that directory for every platform
device that matches this signature, so that implies you can not have
more than one of them at a time, which is not good and an artificial
restriction you are placing on the driver.

thanks,

greg k-h
Rajat Khandelwal April 18, 2023, 9:40 a.m. UTC | #4 
Hi,

On 4/17/2023 11:42 AM, Greg KH wrote:
> On Mon, Apr 17, 2023 at 11:28:18AM +0530, Rajat Khandelwal wrote:
>> Hi,
>>
>> On 4/15/2023 11:01 AM, Greg KH wrote:
>>> On Fri, Apr 14, 2023 at 01:49:10PM +0530, Rajat Khandelwal wrote:
>>>> IOM status has a crucial role during debugging to check the
>>>> current state of the type-C port.
>>>> There are ways to fetch the status, but all those require the
>>>> IOM port status offset, which could change with platform.
>>>>
>>>> Make a debugfs directory for intel_pmc_mux and expose the status
>>>> under it per port basis.
>>>>
>>>> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
>>>> ---
>>>>
>>>> v2:
>>>> 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
>>>> 2. Remove explicitly defined one-liner functions
>>>>
>>>>    drivers/usb/typec/mux/intel_pmc_mux.c | 34 +++++++++++++++++++++++++++
>>>>    1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
>>>> index 34e4188a40ff..1d43b111781e 100644
>>>> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
>>>> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
>>>> @@ -15,6 +15,7 @@
>>>>    #include <linux/usb/typec_mux.h>
>>>>    #include <linux/usb/typec_dp.h>
>>>>    #include <linux/usb/typec_tbt.h>
>>>> +#include <linux/debugfs.h>
>>>>    #include <asm/intel_scu_ipc.h>
>>>> @@ -639,9 +640,34 @@ static int pmc_usb_probe_iom(struct pmc_usb *pmc)
>>>>    	return 0;
>>>>    }
>>>> +static int port_iom_status_show(struct seq_file *s, void *unused)
>>>> +{
>>>> +	struct pmc_usb_port *port = s->private;
>>>> +
>>>> +	update_port_status(port);
>>>> +	seq_printf(s, "0x%08x\n", port->iom_status);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +DEFINE_SHOW_ATTRIBUTE(port_iom_status);
>>>> +
>>>> +static void pmc_mux_port_debugfs_init(struct pmc_usb_port *port,
>>>> +				      struct dentry *pmc_mux_debugfs_root)
>>>> +{
>>>> +	struct dentry *debugfs_dir;
>>>> +	char name[6];
>>>> +
>>>> +	snprintf(name, sizeof(name), "port%d", port->usb3_port - 1);
>>>> +
>>>> +	debugfs_dir = debugfs_create_dir(name, pmc_mux_debugfs_root);
>>>> +	debugfs_create_file("iom_status", 0400, debugfs_dir, port,
>>>> +			    &port_iom_status_fops);
>>>> +}
>>>> +
>>>>    static int pmc_usb_probe(struct platform_device *pdev)
>>>>    {
>>>>    	struct fwnode_handle *fwnode = NULL;
>>>> +	struct dentry *pmc_mux_debugfs_root;
>>>>    	struct pmc_usb *pmc;
>>>>    	int i = 0;
>>>>    	int ret;
>>>> @@ -674,6 +700,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
>>>>    	if (ret)
>>>>    		return ret;
>>>> +	pmc_mux_debugfs_root = debugfs_create_dir("intel_pmc_mux", NULL);
>>> What happens when you have more than one device in the system at the
>>> same time?
>> I'm sorry I didn't understand the question. We would have separate directories
>> for all the ports which would contain the 'iom_status' file, thus representing
>> status for all the ports individually.
>> Can you rephrase the question since I guess you had something else in mind?
> Can you please show the output of the directory
> /sys/kernel/debug/intel_pmc_mux/ with multiple pmc devices in the system
> at the same time?

Sorry, I don't own a system with multiple PMCs. Anyways, do we even have
a system with such design?

Thanks
Rajat

>
> This code seems to want to create that directory for every platform
> device that matches this signature, so that implies you can not have
> more than one of them at a time, which is not good and an artificial
> restriction you are placing on the driver.
>
> thanks,
>
> greg k-h
Greg KH April 18, 2023, 10:18 a.m. UTC | #5 
On Tue, Apr 18, 2023 at 03:10:47PM +0530, Rajat Khandelwal wrote:
> Hi,
> 
> On 4/17/2023 11:42 AM, Greg KH wrote:
> > On Mon, Apr 17, 2023 at 11:28:18AM +0530, Rajat Khandelwal wrote:
> > > Hi,
> > > 
> > > On 4/15/2023 11:01 AM, Greg KH wrote:
> > > > On Fri, Apr 14, 2023 at 01:49:10PM +0530, Rajat Khandelwal wrote:
> > > > > IOM status has a crucial role during debugging to check the
> > > > > current state of the type-C port.
> > > > > There are ways to fetch the status, but all those require the
> > > > > IOM port status offset, which could change with platform.
> > > > > 
> > > > > Make a debugfs directory for intel_pmc_mux and expose the status
> > > > > under it per port basis.
> > > > > 
> > > > > Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> > > > > ---
> > > > > 
> > > > > v2:
> > > > > 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
> > > > > 2. Remove explicitly defined one-liner functions
> > > > > 
> > > > >    drivers/usb/typec/mux/intel_pmc_mux.c | 34 +++++++++++++++++++++++++++
> > > > >    1 file changed, 34 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > index 34e4188a40ff..1d43b111781e 100644
> > > > > --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > @@ -15,6 +15,7 @@
> > > > >    #include <linux/usb/typec_mux.h>
> > > > >    #include <linux/usb/typec_dp.h>
> > > > >    #include <linux/usb/typec_tbt.h>
> > > > > +#include <linux/debugfs.h>
> > > > >    #include <asm/intel_scu_ipc.h>
> > > > > @@ -639,9 +640,34 @@ static int pmc_usb_probe_iom(struct pmc_usb *pmc)
> > > > >    	return 0;
> > > > >    }
> > > > > +static int port_iom_status_show(struct seq_file *s, void *unused)
> > > > > +{
> > > > > +	struct pmc_usb_port *port = s->private;
> > > > > +
> > > > > +	update_port_status(port);
> > > > > +	seq_printf(s, "0x%08x\n", port->iom_status);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +DEFINE_SHOW_ATTRIBUTE(port_iom_status);
> > > > > +
> > > > > +static void pmc_mux_port_debugfs_init(struct pmc_usb_port *port,
> > > > > +				      struct dentry *pmc_mux_debugfs_root)
> > > > > +{
> > > > > +	struct dentry *debugfs_dir;
> > > > > +	char name[6];
> > > > > +
> > > > > +	snprintf(name, sizeof(name), "port%d", port->usb3_port - 1);
> > > > > +
> > > > > +	debugfs_dir = debugfs_create_dir(name, pmc_mux_debugfs_root);
> > > > > +	debugfs_create_file("iom_status", 0400, debugfs_dir, port,
> > > > > +			    &port_iom_status_fops);
> > > > > +}
> > > > > +
> > > > >    static int pmc_usb_probe(struct platform_device *pdev)
> > > > >    {
> > > > >    	struct fwnode_handle *fwnode = NULL;
> > > > > +	struct dentry *pmc_mux_debugfs_root;
> > > > >    	struct pmc_usb *pmc;
> > > > >    	int i = 0;
> > > > >    	int ret;
> > > > > @@ -674,6 +700,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
> > > > >    	if (ret)
> > > > >    		return ret;
> > > > > +	pmc_mux_debugfs_root = debugfs_create_dir("intel_pmc_mux", NULL);
> > > > What happens when you have more than one device in the system at the
> > > > same time?
> > > I'm sorry I didn't understand the question. We would have separate directories
> > > for all the ports which would contain the 'iom_status' file, thus representing
> > > status for all the ports individually.
> > > Can you rephrase the question since I guess you had something else in mind?
> > Can you please show the output of the directory
> > /sys/kernel/debug/intel_pmc_mux/ with multiple pmc devices in the system
> > at the same time?
> 
> Sorry, I don't own a system with multiple PMCs. Anyways, do we even have
> a system with such design?

You might tomorrow.  So please don't create problems like this when you
do not have to.

greg k-h
Rajat Khandelwal April 18, 2023, 10:23 a.m. UTC | #6 
Hi,

On 4/18/2023 3:48 PM, Greg KH wrote:
> On Tue, Apr 18, 2023 at 03:10:47PM +0530, Rajat Khandelwal wrote:
>> Hi,
>>
>> On 4/17/2023 11:42 AM, Greg KH wrote:
>>> On Mon, Apr 17, 2023 at 11:28:18AM +0530, Rajat Khandelwal wrote:
>>>> Hi,
>>>>
>>>> On 4/15/2023 11:01 AM, Greg KH wrote:
>>>>> On Fri, Apr 14, 2023 at 01:49:10PM +0530, Rajat Khandelwal wrote:
>>>>>> IOM status has a crucial role during debugging to check the
>>>>>> current state of the type-C port.
>>>>>> There are ways to fetch the status, but all those require the
>>>>>> IOM port status offset, which could change with platform.
>>>>>>
>>>>>> Make a debugfs directory for intel_pmc_mux and expose the status
>>>>>> under it per port basis.
>>>>>>
>>>>>> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
>>>>>> ---
>>>>>>
>>>>>> v2:
>>>>>> 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
>>>>>> 2. Remove explicitly defined one-liner functions
>>>>>>
>>>>>>     drivers/usb/typec/mux/intel_pmc_mux.c | 34 +++++++++++++++++++++++++++
>>>>>>     1 file changed, 34 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
>>>>>> index 34e4188a40ff..1d43b111781e 100644
>>>>>> --- a/drivers/usb/typec/mux/intel_pmc_mux.c
>>>>>> +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>>     #include <linux/usb/typec_mux.h>
>>>>>>     #include <linux/usb/typec_dp.h>
>>>>>>     #include <linux/usb/typec_tbt.h>
>>>>>> +#include <linux/debugfs.h>
>>>>>>     #include <asm/intel_scu_ipc.h>
>>>>>> @@ -639,9 +640,34 @@ static int pmc_usb_probe_iom(struct pmc_usb *pmc)
>>>>>>     	return 0;
>>>>>>     }
>>>>>> +static int port_iom_status_show(struct seq_file *s, void *unused)
>>>>>> +{
>>>>>> +	struct pmc_usb_port *port = s->private;
>>>>>> +
>>>>>> +	update_port_status(port);
>>>>>> +	seq_printf(s, "0x%08x\n", port->iom_status);
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +DEFINE_SHOW_ATTRIBUTE(port_iom_status);
>>>>>> +
>>>>>> +static void pmc_mux_port_debugfs_init(struct pmc_usb_port *port,
>>>>>> +				      struct dentry *pmc_mux_debugfs_root)
>>>>>> +{
>>>>>> +	struct dentry *debugfs_dir;
>>>>>> +	char name[6];
>>>>>> +
>>>>>> +	snprintf(name, sizeof(name), "port%d", port->usb3_port - 1);
>>>>>> +
>>>>>> +	debugfs_dir = debugfs_create_dir(name, pmc_mux_debugfs_root);
>>>>>> +	debugfs_create_file("iom_status", 0400, debugfs_dir, port,
>>>>>> +			    &port_iom_status_fops);
>>>>>> +}
>>>>>> +
>>>>>>     static int pmc_usb_probe(struct platform_device *pdev)
>>>>>>     {
>>>>>>     	struct fwnode_handle *fwnode = NULL;
>>>>>> +	struct dentry *pmc_mux_debugfs_root;
>>>>>>     	struct pmc_usb *pmc;
>>>>>>     	int i = 0;
>>>>>>     	int ret;
>>>>>> @@ -674,6 +700,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
>>>>>>     	if (ret)
>>>>>>     		return ret;
>>>>>> +	pmc_mux_debugfs_root = debugfs_create_dir("intel_pmc_mux", NULL);
>>>>> What happens when you have more than one device in the system at the
>>>>> same time?
>>>> I'm sorry I didn't understand the question. We would have separate directories
>>>> for all the ports which would contain the 'iom_status' file, thus representing
>>>> status for all the ports individually.
>>>> Can you rephrase the question since I guess you had something else in mind?
>>> Can you please show the output of the directory
>>> /sys/kernel/debug/intel_pmc_mux/ with multiple pmc devices in the system
>>> at the same time?
>> Sorry, I don't own a system with multiple PMCs. Anyways, do we even have
>> a system with such design?
> You might tomorrow.  So please don't create problems like this when you
> do not have to.

We still didn't arrive on a culminating note. :)
What do you suggest I could do for now? This would be a useful debug tool
for us since we deal with it quite often.

If we don't have anything constraining this for now, can we keep it like
this? Let's ameliorate it once such systems actually arrive.
Suggestions?

Thanks
Rajat

>
> greg k-h
Heikki Krogerus April 18, 2023, 10:37 a.m. UTC | #7 
On Tue, Apr 18, 2023 at 03:53:46PM +0530, Rajat Khandelwal wrote:
> Hi,
> 
> On 4/18/2023 3:48 PM, Greg KH wrote:
> > On Tue, Apr 18, 2023 at 03:10:47PM +0530, Rajat Khandelwal wrote:
> > > Hi,
> > > 
> > > On 4/17/2023 11:42 AM, Greg KH wrote:
> > > > On Mon, Apr 17, 2023 at 11:28:18AM +0530, Rajat Khandelwal wrote:
> > > > > Hi,
> > > > > 
> > > > > On 4/15/2023 11:01 AM, Greg KH wrote:
> > > > > > On Fri, Apr 14, 2023 at 01:49:10PM +0530, Rajat Khandelwal wrote:
> > > > > > > IOM status has a crucial role during debugging to check the
> > > > > > > current state of the type-C port.
> > > > > > > There are ways to fetch the status, but all those require the
> > > > > > > IOM port status offset, which could change with platform.
> > > > > > > 
> > > > > > > Make a debugfs directory for intel_pmc_mux and expose the status
> > > > > > > under it per port basis.
> > > > > > > 
> > > > > > > Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > v2:
> > > > > > > 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
> > > > > > > 2. Remove explicitly defined one-liner functions
> > > > > > > 
> > > > > > >     drivers/usb/typec/mux/intel_pmc_mux.c | 34 +++++++++++++++++++++++++++
> > > > > > >     1 file changed, 34 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > > > index 34e4188a40ff..1d43b111781e 100644
> > > > > > > --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > > > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > > > @@ -15,6 +15,7 @@
> > > > > > >     #include <linux/usb/typec_mux.h>
> > > > > > >     #include <linux/usb/typec_dp.h>
> > > > > > >     #include <linux/usb/typec_tbt.h>
> > > > > > > +#include <linux/debugfs.h>
> > > > > > >     #include <asm/intel_scu_ipc.h>
> > > > > > > @@ -639,9 +640,34 @@ static int pmc_usb_probe_iom(struct pmc_usb *pmc)
> > > > > > >     	return 0;
> > > > > > >     }
> > > > > > > +static int port_iom_status_show(struct seq_file *s, void *unused)
> > > > > > > +{
> > > > > > > +	struct pmc_usb_port *port = s->private;
> > > > > > > +
> > > > > > > +	update_port_status(port);
> > > > > > > +	seq_printf(s, "0x%08x\n", port->iom_status);
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +DEFINE_SHOW_ATTRIBUTE(port_iom_status);
> > > > > > > +
> > > > > > > +static void pmc_mux_port_debugfs_init(struct pmc_usb_port *port,
> > > > > > > +				      struct dentry *pmc_mux_debugfs_root)
> > > > > > > +{
> > > > > > > +	struct dentry *debugfs_dir;
> > > > > > > +	char name[6];
> > > > > > > +
> > > > > > > +	snprintf(name, sizeof(name), "port%d", port->usb3_port - 1);
> > > > > > > +
> > > > > > > +	debugfs_dir = debugfs_create_dir(name, pmc_mux_debugfs_root);
> > > > > > > +	debugfs_create_file("iom_status", 0400, debugfs_dir, port,
> > > > > > > +			    &port_iom_status_fops);
> > > > > > > +}
> > > > > > > +
> > > > > > >     static int pmc_usb_probe(struct platform_device *pdev)
> > > > > > >     {
> > > > > > >     	struct fwnode_handle *fwnode = NULL;
> > > > > > > +	struct dentry *pmc_mux_debugfs_root;
> > > > > > >     	struct pmc_usb *pmc;
> > > > > > >     	int i = 0;
> > > > > > >     	int ret;
> > > > > > > @@ -674,6 +700,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
> > > > > > >     	if (ret)
> > > > > > >     		return ret;
> > > > > > > +	pmc_mux_debugfs_root = debugfs_create_dir("intel_pmc_mux", NULL);
> > > > > > What happens when you have more than one device in the system at the
> > > > > > same time?
> > > > > I'm sorry I didn't understand the question. We would have separate directories
> > > > > for all the ports which would contain the 'iom_status' file, thus representing
> > > > > status for all the ports individually.
> > > > > Can you rephrase the question since I guess you had something else in mind?
> > > > Can you please show the output of the directory
> > > > /sys/kernel/debug/intel_pmc_mux/ with multiple pmc devices in the system
> > > > at the same time?
> > > Sorry, I don't own a system with multiple PMCs. Anyways, do we even have
> > > a system with such design?
> > You might tomorrow.  So please don't create problems like this when you
> > do not have to.
> 
> We still didn't arrive on a culminating note. :)
> What do you suggest I could do for now? This would be a useful debug tool
> for us since we deal with it quite often.
> 
> If we don't have anything constraining this for now, can we keep it like
> this? Let's ameliorate it once such systems actually arrive.
> Suggestions?

You can use the device name. Please check for example how
drivers/usb/typec/tcpm/fusb302.c creates the directory.

thanks,
Greg KH April 18, 2023, 10:40 a.m. UTC | #8 
On Tue, Apr 18, 2023 at 03:53:46PM +0530, Rajat Khandelwal wrote:
> Hi,
> 
> On 4/18/2023 3:48 PM, Greg KH wrote:
> > On Tue, Apr 18, 2023 at 03:10:47PM +0530, Rajat Khandelwal wrote:
> > > Hi,
> > > 
> > > On 4/17/2023 11:42 AM, Greg KH wrote:
> > > > On Mon, Apr 17, 2023 at 11:28:18AM +0530, Rajat Khandelwal wrote:
> > > > > Hi,
> > > > > 
> > > > > On 4/15/2023 11:01 AM, Greg KH wrote:
> > > > > > On Fri, Apr 14, 2023 at 01:49:10PM +0530, Rajat Khandelwal wrote:
> > > > > > > IOM status has a crucial role during debugging to check the
> > > > > > > current state of the type-C port.
> > > > > > > There are ways to fetch the status, but all those require the
> > > > > > > IOM port status offset, which could change with platform.
> > > > > > > 
> > > > > > > Make a debugfs directory for intel_pmc_mux and expose the status
> > > > > > > under it per port basis.
> > > > > > > 
> > > > > > > Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > v2:
> > > > > > > 1. Remove static declaration of the debugfs root for 'intel_pmc_mux'
> > > > > > > 2. Remove explicitly defined one-liner functions
> > > > > > > 
> > > > > > >     drivers/usb/typec/mux/intel_pmc_mux.c | 34 +++++++++++++++++++++++++++
> > > > > > >     1 file changed, 34 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > > > index 34e4188a40ff..1d43b111781e 100644
> > > > > > > --- a/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > > > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c
> > > > > > > @@ -15,6 +15,7 @@
> > > > > > >     #include <linux/usb/typec_mux.h>
> > > > > > >     #include <linux/usb/typec_dp.h>
> > > > > > >     #include <linux/usb/typec_tbt.h>
> > > > > > > +#include <linux/debugfs.h>
> > > > > > >     #include <asm/intel_scu_ipc.h>
> > > > > > > @@ -639,9 +640,34 @@ static int pmc_usb_probe_iom(struct pmc_usb *pmc)
> > > > > > >     	return 0;
> > > > > > >     }
> > > > > > > +static int port_iom_status_show(struct seq_file *s, void *unused)
> > > > > > > +{
> > > > > > > +	struct pmc_usb_port *port = s->private;
> > > > > > > +
> > > > > > > +	update_port_status(port);
> > > > > > > +	seq_printf(s, "0x%08x\n", port->iom_status);
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +DEFINE_SHOW_ATTRIBUTE(port_iom_status);
> > > > > > > +
> > > > > > > +static void pmc_mux_port_debugfs_init(struct pmc_usb_port *port,
> > > > > > > +				      struct dentry *pmc_mux_debugfs_root)
> > > > > > > +{
> > > > > > > +	struct dentry *debugfs_dir;
> > > > > > > +	char name[6];
> > > > > > > +
> > > > > > > +	snprintf(name, sizeof(name), "port%d", port->usb3_port - 1);
> > > > > > > +
> > > > > > > +	debugfs_dir = debugfs_create_dir(name, pmc_mux_debugfs_root);
> > > > > > > +	debugfs_create_file("iom_status", 0400, debugfs_dir, port,
> > > > > > > +			    &port_iom_status_fops);
> > > > > > > +}
> > > > > > > +
> > > > > > >     static int pmc_usb_probe(struct platform_device *pdev)
> > > > > > >     {
> > > > > > >     	struct fwnode_handle *fwnode = NULL;
> > > > > > > +	struct dentry *pmc_mux_debugfs_root;
> > > > > > >     	struct pmc_usb *pmc;
> > > > > > >     	int i = 0;
> > > > > > >     	int ret;
> > > > > > > @@ -674,6 +700,8 @@ static int pmc_usb_probe(struct platform_device *pdev)
> > > > > > >     	if (ret)
> > > > > > >     		return ret;
> > > > > > > +	pmc_mux_debugfs_root = debugfs_create_dir("intel_pmc_mux", NULL);
> > > > > > What happens when you have more than one device in the system at the
> > > > > > same time?
> > > > > I'm sorry I didn't understand the question. We would have separate directories
> > > > > for all the ports which would contain the 'iom_status' file, thus representing
> > > > > status for all the ports individually.
> > > > > Can you rephrase the question since I guess you had something else in mind?
> > > > Can you please show the output of the directory
> > > > /sys/kernel/debug/intel_pmc_mux/ with multiple pmc devices in the system
> > > > at the same time?
> > > Sorry, I don't own a system with multiple PMCs. Anyways, do we even have
> > > a system with such design?
> > You might tomorrow.  So please don't create problems like this when you
> > do not have to.
> 
> We still didn't arrive on a culminating note. :)
> What do you suggest I could do for now? This would be a useful debug tool
> for us since we deal with it quite often.

Then do it properly please.  Look at the /sys/kernel/debug/usb/
directory for examples, specifically /sys/kernel/debug/xhci/.

> If we don't have anything constraining this for now, can we keep it like
> this?

No.
diff mbox series

Patch

diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c
index 34e4188a40ff..1d43b111781e 100644
--- a/drivers/usb/typec/mux/intel_pmc_mux.c
+++ b/drivers/usb/typec/mux/intel_pmc_mux.c
@@ -15,6 +15,7 @@ 
 #include <linux/usb/typec_mux.h>
 #include <linux/usb/typec_dp.h>
 #include <linux/usb/typec_tbt.h>
+#include <linux/debugfs.h>
 
 #include <asm/intel_scu_ipc.h>
 
@@ -639,9 +640,34 @@  static int pmc_usb_probe_iom(struct pmc_usb *pmc)
 	return 0;
 }
 
+static int port_iom_status_show(struct seq_file *s, void *unused)
+{
+	struct pmc_usb_port *port = s->private;
+
+	update_port_status(port);
+	seq_printf(s, "0x%08x\n", port->iom_status);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(port_iom_status);
+
+static void pmc_mux_port_debugfs_init(struct pmc_usb_port *port,
+				      struct dentry *pmc_mux_debugfs_root)
+{
+	struct dentry *debugfs_dir;
+	char name[6];
+
+	snprintf(name, sizeof(name), "port%d", port->usb3_port - 1);
+
+	debugfs_dir = debugfs_create_dir(name, pmc_mux_debugfs_root);
+	debugfs_create_file("iom_status", 0400, debugfs_dir, port,
+			    &port_iom_status_fops);
+}
+
 static int pmc_usb_probe(struct platform_device *pdev)
 {
 	struct fwnode_handle *fwnode = NULL;
+	struct dentry *pmc_mux_debugfs_root;
 	struct pmc_usb *pmc;
 	int i = 0;
 	int ret;
@@ -674,6 +700,8 @@  static int pmc_usb_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	pmc_mux_debugfs_root = debugfs_create_dir("intel_pmc_mux", NULL);
+
 	/*
 	 * For every physical USB connector (USB2 and USB3 combo) there is a
 	 * child ACPI device node under the PMC mux ACPI device object.
@@ -688,6 +716,8 @@  static int pmc_usb_probe(struct platform_device *pdev)
 			fwnode_handle_put(fwnode);
 			goto err_remove_ports;
 		}
+
+		pmc_mux_port_debugfs_init(&pmc->port[i], pmc_mux_debugfs_root);
 	}
 
 	platform_set_drvdata(pdev, pmc);
@@ -703,6 +733,8 @@  static int pmc_usb_probe(struct platform_device *pdev)
 
 	acpi_dev_put(pmc->iom_adev);
 
+	debugfs_lookup_and_remove("intel_pmc_mux", NULL);
+
 	return ret;
 }
 
@@ -719,6 +751,8 @@  static int pmc_usb_remove(struct platform_device *pdev)
 
 	acpi_dev_put(pmc->iom_adev);
 
+	debugfs_lookup_and_remove("intel_pmc_mux", NULL);
+
 	return 0;
 }