[PATCH] soundwire: debugfs: use controller id instead of link_id

[PATCH] soundwire: debugfs: use controller id instead of link_id

[Srinivas Kandagatla]

link_id can be zero and if we have multiple controller instances
in a system like Qualcomm debugfs will end-up with duplicate namespace
resulting in incorrect debugfs entries.

Using id should give a unique debugfs directory entry and should fix below
warning too.
"debugfs: Directory 'master-0' with parent 'soundwire' already present!"

Fixes: bf03473d5bcc ("soundwire: add debugfs support")
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
index b6cad0d59b7b..5f9efa42bb25 100644
--- a/drivers/soundwire/debugfs.c
+++ b/drivers/soundwire/debugfs.c
@@ -19,7 +19,7 @@ void sdw_bus_debugfs_init(struct sdw_bus *bus)
 		return;
 
 	/* create the debugfs master-N */
-	snprintf(name, sizeof(name), "master-%d", bus->link_id);
+	snprintf(name, sizeof(name), "master-%d", bus->id);
 	bus->debugfs = debugfs_create_dir(name, sdw_debugfs_root);
 }
 
-- 
2.21.0

[PATCH] soundwire: debugfs: use controller id instead of link_id

[Srinivas Kandagatla]

link_id can be zero and if we have multiple controller instances
in a system like Qualcomm debugfs will end-up with duplicate namespace
resulting in incorrect debugfs entries.

Using id should give a unique debugfs directory entry and should fix below
warning too.
"debugfs: Directory 'master-0' with parent 'soundwire' already present!"

Fixes: bf03473d5bcc ("soundwire: add debugfs support")
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
index b6cad0d59b7b..5f9efa42bb25 100644
--- a/drivers/soundwire/debugfs.c
+++ b/drivers/soundwire/debugfs.c
@@ -19,7 +19,7 @@ void sdw_bus_debugfs_init(struct sdw_bus *bus)
 		return;
 
 	/* create the debugfs master-N */
-	snprintf(name, sizeof(name), "master-%d", bus->link_id);
+	snprintf(name, sizeof(name), "master-%d", bus->id);
 	bus->debugfs = debugfs_create_dir(name, sdw_debugfs_root);
 }
 
-- 
2.21.0

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Vinod Koul]

On 15-01-21, 16:25, Srinivas Kandagatla wrote:
> link_id can be zero and if we have multiple controller instances
> in a system like Qualcomm debugfs will end-up with duplicate namespace
> resulting in incorrect debugfs entries.
> 
> Using id should give a unique debugfs directory entry and should fix below
> warning too.
> "debugfs: Directory 'master-0' with parent 'soundwire' already present!"

Yeah id is guaranteed to be unique so this will work.

Applied, thanks

-- 
~Vinod

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Vinod Koul]

On 15-01-21, 16:25, Srinivas Kandagatla wrote:
> link_id can be zero and if we have multiple controller instances
> in a system like Qualcomm debugfs will end-up with duplicate namespace
> resulting in incorrect debugfs entries.
> 
> Using id should give a unique debugfs directory entry and should fix below
> warning too.
> "debugfs: Directory 'master-0' with parent 'soundwire' already present!"

Yeah id is guaranteed to be unique so this will work.

Applied, thanks

-- 
~Vinod

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Pierre-Louis Bossart]

On 1/19/21 8:52 AM, Vinod Koul wrote:
> On 15-01-21, 16:25, Srinivas Kandagatla wrote:
>> link_id can be zero and if we have multiple controller instances
>> in a system like Qualcomm debugfs will end-up with duplicate namespace
>> resulting in incorrect debugfs entries.
>>
>> Using id should give a unique debugfs directory entry and should fix below
>> warning too.
>> "debugfs: Directory 'master-0' with parent 'soundwire' already present!"
> 
> Yeah id is guaranteed to be unique so this will work.
> 
> Applied, thanks

This patch is a no-op for Intel, but I am not convinced it's the right 
fix or the definitions are not consistent.

  * @link_id: Link id number, can be 0 to N, unique for each Master
  * @id: bus system-wide unique id

In the MIPI/DisCo definitions, a controller can have multiple masters. 
if you have multiple controllers, each of them should have their own 
debugfs directory IMHO. It's totally fine to have multiple bus/masters 
with a link_id == 0.

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Pierre-Louis Bossart]

On 1/19/21 8:52 AM, Vinod Koul wrote:
> On 15-01-21, 16:25, Srinivas Kandagatla wrote:
>> link_id can be zero and if we have multiple controller instances
>> in a system like Qualcomm debugfs will end-up with duplicate namespace
>> resulting in incorrect debugfs entries.
>>
>> Using id should give a unique debugfs directory entry and should fix below
>> warning too.
>> "debugfs: Directory 'master-0' with parent 'soundwire' already present!"
> 
> Yeah id is guaranteed to be unique so this will work.
> 
> Applied, thanks

This patch is a no-op for Intel, but I am not convinced it's the right 
fix or the definitions are not consistent.

  * @link_id: Link id number, can be 0 to N, unique for each Master
  * @id: bus system-wide unique id

In the MIPI/DisCo definitions, a controller can have multiple masters. 
if you have multiple controllers, each of them should have their own 
debugfs directory IMHO. It's totally fine to have multiple bus/masters 
with a link_id == 0.

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Srinivas Kandagatla]

On 19/01/2021 15:54, Pierre-Louis Bossart wrote:
> 
> 
> On 1/19/21 8:52 AM, Vinod Koul wrote:
>> On 15-01-21, 16:25, Srinivas Kandagatla wrote:
>>> link_id can be zero and if we have multiple controller instances
>>> in a system like Qualcomm debugfs will end-up with duplicate namespace
>>> resulting in incorrect debugfs entries.
>>>
>>> Using id should give a unique debugfs directory entry and should fix 
>>> below
>>> warning too.
>>> "debugfs: Directory 'master-0' with parent 'soundwire' already present!"
>>
>> Yeah id is guaranteed to be unique so this will work.
>>
>> Applied, thanks
> 
> This patch is a no-op for Intel, but I am not convinced it's the right 
> fix or the definitions are not consistent.

It depends if the intention is to represent full Hierarchy in debugfs, 
then I agree. Its was consistent even before!

currently we have
/sys/kernel/debug/soundwire/master-*

Are you suggesting that we have something like this:

/sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ??
/sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID>/xyz-slave/master-<LINK-ID> 
??


Or may be something much simpler like:

/sys/kernel/debug/soundwire/master-<ID>.<LINK_ID>


--srini

> 
>   * @link_id: Link id number, can be 0 to N, unique for each Master
>   * @id: bus system-wide unique id
> 
> In the MIPI/DisCo definitions, a controller can have multiple masters. 
> if you have multiple controllers, each of them should have their own 
> debugfs directory IMHO. It's totally fine to have multiple bus/masters 
> with a link_id == 0.

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Srinivas Kandagatla]

On 19/01/2021 15:54, Pierre-Louis Bossart wrote:
> 
> 
> On 1/19/21 8:52 AM, Vinod Koul wrote:
>> On 15-01-21, 16:25, Srinivas Kandagatla wrote:
>>> link_id can be zero and if we have multiple controller instances
>>> in a system like Qualcomm debugfs will end-up with duplicate namespace
>>> resulting in incorrect debugfs entries.
>>>
>>> Using id should give a unique debugfs directory entry and should fix 
>>> below
>>> warning too.
>>> "debugfs: Directory 'master-0' with parent 'soundwire' already present!"
>>
>> Yeah id is guaranteed to be unique so this will work.
>>
>> Applied, thanks
> 
> This patch is a no-op for Intel, but I am not convinced it's the right 
> fix or the definitions are not consistent.

It depends if the intention is to represent full Hierarchy in debugfs, 
then I agree. Its was consistent even before!

currently we have
/sys/kernel/debug/soundwire/master-*

Are you suggesting that we have something like this:

/sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ??
/sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID>/xyz-slave/master-<LINK-ID> 
??


Or may be something much simpler like:

/sys/kernel/debug/soundwire/master-<ID>.<LINK_ID>


--srini

> 
>   * @link_id: Link id number, can be 0 to N, unique for each Master
>   * @id: bus system-wide unique id
> 
> In the MIPI/DisCo definitions, a controller can have multiple masters. 
> if you have multiple controllers, each of them should have their own 
> debugfs directory IMHO. It's totally fine to have multiple bus/masters 
> with a link_id == 0.

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Pierre-Louis Bossart]

>>>> link_id can be zero and if we have multiple controller instances
>>>> in a system like Qualcomm debugfs will end-up with duplicate namespace
>>>> resulting in incorrect debugfs entries.
>>>>
>>>> Using id should give a unique debugfs directory entry and should fix 
>>>> below
>>>> warning too.
>>>> "debugfs: Directory 'master-0' with parent 'soundwire' already 
>>>> present!"
>>>
>>> Yeah id is guaranteed to be unique so this will work.
>>>
>>> Applied, thanks
>>
>> This patch is a no-op for Intel, but I am not convinced it's the right 
>> fix or the definitions are not consistent.
> 
> It depends if the intention is to represent full Hierarchy in debugfs, 
> then I agree. Its was consistent even before!

Indeed, we don't currently have a notion of controller in debugfs.

> currently we have
> /sys/kernel/debug/soundwire/master-*
> 
> Are you suggesting that we have something like this:
> 
> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ??

Yes this is what I was thinking about.

> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID>/xyz-slave/master-<LINK-ID> 
> ??

This would be for a bridge which to the best of my knowledge hasn't been 
implemented by anyone (clocking and command/control timing issues).

> Or may be something much simpler like:
> 
> /sys/kernel/debug/soundwire/master-<ID>.<LINK_ID>

the bus->id is an IDA, which is useful for to avoid conflicts, but it's 
not really meaningful for debugfs. I remember seeing a case where we had 
links 2 and 4 enabled, and the bus->id were 0 and 1, a completely 
artificial value that doesn't really help in debugging.

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Pierre-Louis Bossart]

>>>> link_id can be zero and if we have multiple controller instances
>>>> in a system like Qualcomm debugfs will end-up with duplicate namespace
>>>> resulting in incorrect debugfs entries.
>>>>
>>>> Using id should give a unique debugfs directory entry and should fix 
>>>> below
>>>> warning too.
>>>> "debugfs: Directory 'master-0' with parent 'soundwire' already 
>>>> present!"
>>>
>>> Yeah id is guaranteed to be unique so this will work.
>>>
>>> Applied, thanks
>>
>> This patch is a no-op for Intel, but I am not convinced it's the right 
>> fix or the definitions are not consistent.
> 
> It depends if the intention is to represent full Hierarchy in debugfs, 
> then I agree. Its was consistent even before!

Indeed, we don't currently have a notion of controller in debugfs.

> currently we have
> /sys/kernel/debug/soundwire/master-*
> 
> Are you suggesting that we have something like this:
> 
> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ??

Yes this is what I was thinking about.

> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID>/xyz-slave/master-<LINK-ID> 
> ??

This would be for a bridge which to the best of my knowledge hasn't been 
implemented by anyone (clocking and command/control timing issues).

> Or may be something much simpler like:
> 
> /sys/kernel/debug/soundwire/master-<ID>.<LINK_ID>

the bus->id is an IDA, which is useful for to avoid conflicts, but it's 
not really meaningful for debugfs. I remember seeing a case where we had 
links 2 and 4 enabled, and the bus->id were 0 and 1, a completely 
artificial value that doesn't really help in debugging.

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Srinivas Kandagatla]

On 19/01/2021 19:09, Pierre-Louis Bossart wrote:
> 
>> currently we have
>> /sys/kernel/debug/soundwire/master-*
>>
>> Are you suggesting that we have something like this:
>>
>> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ??
> 
> Yes this is what I was thinking about.

Vinod/Pierre,

One Question here,

Why is link_id part of "struct sdw_bus", should it not be part of 
"struct sdw_master_device " ?

Given that "There is one Link per each Master"

--srini

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Srinivas Kandagatla]

On 19/01/2021 19:09, Pierre-Louis Bossart wrote:
> 
>> currently we have
>> /sys/kernel/debug/soundwire/master-*
>>
>> Are you suggesting that we have something like this:
>>
>> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ??
> 
> Yes this is what I was thinking about.

Vinod/Pierre,

One Question here,

Why is link_id part of "struct sdw_bus", should it not be part of 
"struct sdw_master_device " ?

Given that "There is one Link per each Master"

--srini

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Pierre-Louis Bossart]

On 1/21/21 6:03 AM, Srinivas Kandagatla wrote:
> 
> 
> On 19/01/2021 19:09, Pierre-Louis Bossart wrote:
>>
>>> currently we have
>>> /sys/kernel/debug/soundwire/master-*
>>>
>>> Are you suggesting that we have something like this:
>>>
>>> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ??
>>
>> Yes this is what I was thinking about.
> 
> Vinod/Pierre,
> 
> One Question here,
> 
> Why is link_id part of "struct sdw_bus", should it not be part of 
> "struct sdw_master_device " ?
> 
> Given that "There is one Link per each Master"

it's true that link == master == bus at the concept level.

but we have an existing code base with different structures and we can't 
break too many things at once.

In the existing flow, the 'bus' is created and setup first, the 
sdw_bus_master_add() routine takes a 'bus' argument, and the link_id is 
already set. This routine only creates a device and in the rest of the 
code we keep using the 'bus' pointer, so there's no real short-term 
scope for moving the information into the 'sdw_master_device' structure 
- that would be a lot of surgery when nothing is really broken.

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Srinivas Kandagatla]

On 21/01/2021 15:12, Pierre-Louis Bossart wrote:
> 
> 
> On 1/21/21 6:03 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 19/01/2021 19:09, Pierre-Louis Bossart wrote:
>>>
>>>> currently we have
>>>> /sys/kernel/debug/soundwire/master-*
>>>>
>>>> Are you suggesting that we have something like this:
>>>>
>>>> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ??
>>>
>>> Yes this is what I was thinking about.
>>
>> Vinod/Pierre,
>>
>> One Question here,
>>
>> Why is link_id part of "struct sdw_bus", should it not be part of 
>> "struct sdw_master_device " ?
>>
>> Given that "There is one Link per each Master"
> 
> it's true that link == master == bus at the concept level.
> 
> but we have an existing code base with different structures and we can't 
> break too many things at once.
> 
> In the existing flow, the 'bus' is created and setup first, the 
> sdw_bus_master_add() routine takes a 'bus' argument, and the link_id is 
> already set. This routine only creates a device and in the rest of the 
> code we keep using the 'bus' pointer, so there's no real short-term 
> scope for moving the information into the 'sdw_master_device' structure 
> - that would be a lot of surgery when nothing is really broken.

I totally agree!

If I understand it correctly in Intel case there will be only one Link 
ID per bus.


Does this change look good to you?

---------------->cut<---------------

diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
index b6cad0d59b7b..f22868614f09 100644
--- a/drivers/soundwire/debugfs.c
+++ b/drivers/soundwire/debugfs.c
@@ -19,13 +19,14 @@ void sdw_bus_debugfs_init(struct sdw_bus *bus)
                 return;

         /* create the debugfs master-N */
+       bus->controller_debugfs = debugfs_create_dir(dev_name(bus->dev), 
sdw_debugfs_root);
         snprintf(name, sizeof(name), "master-%d", bus->link_id);
-       bus->debugfs = debugfs_create_dir(name, sdw_debugfs_root);
+       bus->debugfs = debugfs_create_dir(name, bus->controller_debugfs);
  }

  void sdw_bus_debugfs_exit(struct sdw_bus *bus)
  {
-       debugfs_remove_recursive(bus->debugfs);
+       debugfs_remove_recursive(bus->controller_debugfs);
  }

  #define RD_BUF (3 * PAGE_SIZE)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index b198f471bea8..242bde30d8bd 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -877,6 +877,7 @@ struct sdw_bus {
         struct sdw_master_prop prop;
         struct list_head m_rt_list;
  #ifdef CONFIG_DEBUG_FS
+       struct dentry *controller_debugfs;
         struct dentry *debugfs;
  #endif
         struct sdw_defer defer_msg;

---------------->cut<---------------

With this change I get something like this on my board:

~# find /sys/kernel/debug/soundwire/
/sys/kernel/debug/soundwire/
/sys/kernel/debug/soundwire/sdw-master-2
/sys/kernel/debug/soundwire/sdw-master-2/master-0
/sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4
/sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4/registers
/sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3
/sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3/registers
/sys/kernel/debug/soundwire/sdw-master-1
/sys/kernel/debug/soundwire/sdw-master-1/master-0
/sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3
/sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3/registers
/sys/kernel/debug/soundwire/sdw-master-0
/sys/kernel/debug/soundwire/sdw-master-0/master-0
/sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4
/sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4/registers



Thanks,
srini

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Pierre-Louis Bossart]

> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> index b6cad0d59b7b..f22868614f09 100644
> --- a/drivers/soundwire/debugfs.c
> +++ b/drivers/soundwire/debugfs.c
> @@ -19,13 +19,14 @@ void sdw_bus_debugfs_init(struct sdw_bus *bus)
>                  return;
> 
>          /* create the debugfs master-N */
> +       bus->controller_debugfs = debugfs_create_dir(dev_name(bus->dev), 
> sdw_debugfs_root);

	bus->dev = &md->dev;

dev_name(bus->dev) does not describe a controller at all but an 
individual master.

We might as well just change the information as:

snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id);

you get the system unique ID and controller-relative ID, and you can 
decide to ignore one or the other on different platform.

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Vinod Koul]

On 21-01-21, 17:23, Srinivas Kandagatla wrote:
> 
> 
> On 21/01/2021 15:12, Pierre-Louis Bossart wrote:
> > 
> > 
> > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote:
> > > 
> > > 
> > > On 19/01/2021 19:09, Pierre-Louis Bossart wrote:
> > > > 
> > > > > currently we have
> > > > > /sys/kernel/debug/soundwire/master-*
> > > > > 
> > > > > Are you suggesting that we have something like this:
> > > > > 
> > > > > /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ??
> > > > 
> > > > Yes this is what I was thinking about.
> > > 
> > > Vinod/Pierre,
> > > 
> > > One Question here,
> > > 
> > > Why is link_id part of "struct sdw_bus", should it not be part of
> > > "struct sdw_master_device " ?
> > > 
> > > Given that "There is one Link per each Master"
> > 
> > it's true that link == master == bus at the concept level.
> > 
> > but we have an existing code base with different structures and we can't
> > break too many things at once.
> > 
> > In the existing flow, the 'bus' is created and setup first, the
> > sdw_bus_master_add() routine takes a 'bus' argument, and the link_id is
> > already set. This routine only creates a device and in the rest of the
> > code we keep using the 'bus' pointer, so there's no real short-term
> > scope for moving the information into the 'sdw_master_device' structure
> > - that would be a lot of surgery when nothing is really broken.
> 
> I totally agree!
> 
> If I understand it correctly in Intel case there will be only one Link ID
> per bus.

Yes IIUC there would be one link id per bus.

the ida approach gives us unique id for each master,bus I would like to
propose using that everywhere

> 
> 
> Does this change look good to you?
> 
> ---------------->cut<---------------
> 
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> index b6cad0d59b7b..f22868614f09 100644
> --- a/drivers/soundwire/debugfs.c
> +++ b/drivers/soundwire/debugfs.c
> @@ -19,13 +19,14 @@ void sdw_bus_debugfs_init(struct sdw_bus *bus)
>                 return;
> 
>         /* create the debugfs master-N */
> +       bus->controller_debugfs = debugfs_create_dir(dev_name(bus->dev),
> sdw_debugfs_root);
>         snprintf(name, sizeof(name), "master-%d", bus->link_id);
> -       bus->debugfs = debugfs_create_dir(name, sdw_debugfs_root);
> +       bus->debugfs = debugfs_create_dir(name, bus->controller_debugfs);
>  }
> 
>  void sdw_bus_debugfs_exit(struct sdw_bus *bus)
>  {
> -       debugfs_remove_recursive(bus->debugfs);
> +       debugfs_remove_recursive(bus->controller_debugfs);
>  }
> 
>  #define RD_BUF (3 * PAGE_SIZE)
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index b198f471bea8..242bde30d8bd 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -877,6 +877,7 @@ struct sdw_bus {
>         struct sdw_master_prop prop;
>         struct list_head m_rt_list;
>  #ifdef CONFIG_DEBUG_FS
> +       struct dentry *controller_debugfs;
>         struct dentry *debugfs;
>  #endif
>         struct sdw_defer defer_msg;
> 
> ---------------->cut<---------------
> 
> With this change I get something like this on my board:
> 
> ~# find /sys/kernel/debug/soundwire/
> /sys/kernel/debug/soundwire/
> /sys/kernel/debug/soundwire/sdw-master-2
> /sys/kernel/debug/soundwire/sdw-master-2/master-0
> /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4
> /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4/registers
> /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3
> /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3/registers
> /sys/kernel/debug/soundwire/sdw-master-1
> /sys/kernel/debug/soundwire/sdw-master-1/master-0
> /sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3
> /sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3/registers
> /sys/kernel/debug/soundwire/sdw-master-0
> /sys/kernel/debug/soundwire/sdw-master-0/master-0
> /sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4
> /sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4/registers
> 
> 
> 
> Thanks,
> srini

-- 
~Vinod

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Vinod Koul]

On 21-01-21, 17:23, Srinivas Kandagatla wrote:
> 
> 
> On 21/01/2021 15:12, Pierre-Louis Bossart wrote:
> > 
> > 
> > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote:
> > > 
> > > 
> > > On 19/01/2021 19:09, Pierre-Louis Bossart wrote:
> > > > 
> > > > > currently we have
> > > > > /sys/kernel/debug/soundwire/master-*
> > > > > 
> > > > > Are you suggesting that we have something like this:
> > > > > 
> > > > > /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ??
> > > > 
> > > > Yes this is what I was thinking about.
> > > 
> > > Vinod/Pierre,
> > > 
> > > One Question here,
> > > 
> > > Why is link_id part of "struct sdw_bus", should it not be part of
> > > "struct sdw_master_device " ?
> > > 
> > > Given that "There is one Link per each Master"
> > 
> > it's true that link == master == bus at the concept level.
> > 
> > but we have an existing code base with different structures and we can't
> > break too many things at once.
> > 
> > In the existing flow, the 'bus' is created and setup first, the
> > sdw_bus_master_add() routine takes a 'bus' argument, and the link_id is
> > already set. This routine only creates a device and in the rest of the
> > code we keep using the 'bus' pointer, so there's no real short-term
> > scope for moving the information into the 'sdw_master_device' structure
> > - that would be a lot of surgery when nothing is really broken.
> 
> I totally agree!
> 
> If I understand it correctly in Intel case there will be only one Link ID
> per bus.

Yes IIUC there would be one link id per bus.

the ida approach gives us unique id for each master,bus I would like to
propose using that everywhere

> 
> 
> Does this change look good to you?
> 
> ---------------->cut<---------------
> 
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> index b6cad0d59b7b..f22868614f09 100644
> --- a/drivers/soundwire/debugfs.c
> +++ b/drivers/soundwire/debugfs.c
> @@ -19,13 +19,14 @@ void sdw_bus_debugfs_init(struct sdw_bus *bus)
>                 return;
> 
>         /* create the debugfs master-N */
> +       bus->controller_debugfs = debugfs_create_dir(dev_name(bus->dev),
> sdw_debugfs_root);
>         snprintf(name, sizeof(name), "master-%d", bus->link_id);
> -       bus->debugfs = debugfs_create_dir(name, sdw_debugfs_root);
> +       bus->debugfs = debugfs_create_dir(name, bus->controller_debugfs);
>  }
> 
>  void sdw_bus_debugfs_exit(struct sdw_bus *bus)
>  {
> -       debugfs_remove_recursive(bus->debugfs);
> +       debugfs_remove_recursive(bus->controller_debugfs);
>  }
> 
>  #define RD_BUF (3 * PAGE_SIZE)
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index b198f471bea8..242bde30d8bd 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -877,6 +877,7 @@ struct sdw_bus {
>         struct sdw_master_prop prop;
>         struct list_head m_rt_list;
>  #ifdef CONFIG_DEBUG_FS
> +       struct dentry *controller_debugfs;
>         struct dentry *debugfs;
>  #endif
>         struct sdw_defer defer_msg;
> 
> ---------------->cut<---------------
> 
> With this change I get something like this on my board:
> 
> ~# find /sys/kernel/debug/soundwire/
> /sys/kernel/debug/soundwire/
> /sys/kernel/debug/soundwire/sdw-master-2
> /sys/kernel/debug/soundwire/sdw-master-2/master-0
> /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4
> /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:4/registers
> /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3
> /sys/kernel/debug/soundwire/sdw-master-2/master-0/sdw:0:217:2110:0:3/registers
> /sys/kernel/debug/soundwire/sdw-master-1
> /sys/kernel/debug/soundwire/sdw-master-1/master-0
> /sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3
> /sys/kernel/debug/soundwire/sdw-master-1/master-0/sdw:0:217:10d:0:3/registers
> /sys/kernel/debug/soundwire/sdw-master-0
> /sys/kernel/debug/soundwire/sdw-master-0/master-0
> /sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4
> /sys/kernel/debug/soundwire/sdw-master-0/master-0/sdw:0:217:10d:0:4/registers
> 
> 
> 
> Thanks,
> srini

-- 
~Vinod

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Pierre-Louis Bossart]

On 2/1/21 4:14 AM, Vinod Koul wrote:
> On 21-01-21, 17:23, Srinivas Kandagatla wrote:
>>
>>
>> On 21/01/2021 15:12, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 1/21/21 6:03 AM, Srinivas Kandagatla wrote:
>>>>
>>>>
>>>> On 19/01/2021 19:09, Pierre-Louis Bossart wrote:
>>>>>
>>>>>> currently we have
>>>>>> /sys/kernel/debug/soundwire/master-*
>>>>>>
>>>>>> Are you suggesting that we have something like this:
>>>>>>
>>>>>> /sys/kernel/debug/soundwire/xyz-controller/master-<LINK-ID> ??
>>>>>
>>>>> Yes this is what I was thinking about.
>>>>
>>>> Vinod/Pierre,
>>>>
>>>> One Question here,
>>>>
>>>> Why is link_id part of "struct sdw_bus", should it not be part of
>>>> "struct sdw_master_device " ?
>>>>
>>>> Given that "There is one Link per each Master"
>>>
>>> it's true that link == master == bus at the concept level.
>>>
>>> but we have an existing code base with different structures and we can't
>>> break too many things at once.
>>>
>>> In the existing flow, the 'bus' is created and setup first, the
>>> sdw_bus_master_add() routine takes a 'bus' argument, and the link_id is
>>> already set. This routine only creates a device and in the rest of the
>>> code we keep using the 'bus' pointer, so there's no real short-term
>>> scope for moving the information into the 'sdw_master_device' structure
>>> - that would be a lot of surgery when nothing is really broken.
>>
>> I totally agree!
>>
>> If I understand it correctly in Intel case there will be only one Link ID
>> per bus.
> 
> Yes IIUC there would be one link id per bus.
> 
> the ida approach gives us unique id for each master,bus I would like to
> propose using that everywhere

We have cases where link2 is not used but link0, 1 and 3 are.
Using the IDA would result in master-0,1,2 being shown, that would throw 
the integrator off. the link_id is related to hardware and can tolerate 
gaps, the IDA is typically always increasing and is across the system, 
not controller specific.

We can debate forever but both pieces of information are useful, so my 
recommendation is to use both:

snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id);

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Vinod Koul]

On 01-02-21, 10:10, Pierre-Louis Bossart wrote:
> On 2/1/21 4:14 AM, Vinod Koul wrote:
> > On 21-01-21, 17:23, Srinivas Kandagatla wrote:
> > > On 21/01/2021 15:12, Pierre-Louis Bossart wrote:
> > > > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote:

> > > I totally agree!
> > > 
> > > If I understand it correctly in Intel case there will be only one Link ID
> > > per bus.
> > 
> > Yes IIUC there would be one link id per bus.
> > 
> > the ida approach gives us unique id for each master,bus I would like to
> > propose using that everywhere
> 
> We have cases where link2 is not used but link0, 1 and 3 are.
> Using the IDA would result in master-0,1,2 being shown, that would throw the
> integrator off. the link_id is related to hardware and can tolerate gaps,
> the IDA is typically always increasing and is across the system, not
> controller specific.
> 
> We can debate forever but both pieces of information are useful, so my
> recommendation is to use both:
> 
> snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id);

I agree we should use both, but does it really make sense for naming? We
can keep name in ida and expose the link_id as a parameter for
integrators to see in sysfs.

Also, even in intel case you would run into issue if you have two
independent controllers, am not sure if we ever get to see that, but I
think link_id is unique for a controller and not across system, right?

Thanks
-- 
~Vinod

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Vinod Koul]

On 01-02-21, 10:10, Pierre-Louis Bossart wrote:
> On 2/1/21 4:14 AM, Vinod Koul wrote:
> > On 21-01-21, 17:23, Srinivas Kandagatla wrote:
> > > On 21/01/2021 15:12, Pierre-Louis Bossart wrote:
> > > > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote:

> > > I totally agree!
> > > 
> > > If I understand it correctly in Intel case there will be only one Link ID
> > > per bus.
> > 
> > Yes IIUC there would be one link id per bus.
> > 
> > the ida approach gives us unique id for each master,bus I would like to
> > propose using that everywhere
> 
> We have cases where link2 is not used but link0, 1 and 3 are.
> Using the IDA would result in master-0,1,2 being shown, that would throw the
> integrator off. the link_id is related to hardware and can tolerate gaps,
> the IDA is typically always increasing and is across the system, not
> controller specific.
> 
> We can debate forever but both pieces of information are useful, so my
> recommendation is to use both:
> 
> snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id);

I agree we should use both, but does it really make sense for naming? We
can keep name in ida and expose the link_id as a parameter for
integrators to see in sysfs.

Also, even in intel case you would run into issue if you have two
independent controllers, am not sure if we ever get to see that, but I
think link_id is unique for a controller and not across system, right?

Thanks
-- 
~Vinod

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Pierre-Louis Bossart]

On 2/1/21 10:18 PM, Vinod Koul wrote:
> On 01-02-21, 10:10, Pierre-Louis Bossart wrote:
>> On 2/1/21 4:14 AM, Vinod Koul wrote:
>>> On 21-01-21, 17:23, Srinivas Kandagatla wrote:
>>>> On 21/01/2021 15:12, Pierre-Louis Bossart wrote:
>>>>> On 1/21/21 6:03 AM, Srinivas Kandagatla wrote:
> 
>>>> I totally agree!
>>>>
>>>> If I understand it correctly in Intel case there will be only one Link ID
>>>> per bus.
>>>
>>> Yes IIUC there would be one link id per bus.
>>>
>>> the ida approach gives us unique id for each master,bus I would like to
>>> propose using that everywhere
>>
>> We have cases where link2 is not used but link0, 1 and 3 are.
>> Using the IDA would result in master-0,1,2 being shown, that would throw the
>> integrator off. the link_id is related to hardware and can tolerate gaps,
>> the IDA is typically always increasing and is across the system, not
>> controller specific.
>>
>> We can debate forever but both pieces of information are useful, so my
>> recommendation is to use both:
>>
>> snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id);
> 
> I agree we should use both, but does it really make sense for naming? We
> can keep name in ida and expose the link_id as a parameter for
> integrators to see in sysfs.

That would mean changing the meaning of sysfs properties:

/*
  * The sysfs for properties reflects the MIPI description as given
  * in the MIPI DisCo spec
  *
  * Base file is:
  *	sdw-master-N
  *      |---- revision
  *      |---- clk_stop_modes
  *      |---- max_clk_freq
  *      |---- clk_freq
  *      |---- clk_gears
  *      |---- default_row
  *      |---- default_col
  *      |---- dynamic_shape
  *      |---- err_threshold
  */

N is the link ID in the spec. I am not convinced we'd do the community a 
service by unilaterally changing what an external spec means, or add a 
property that's kernel-defined while the rest is supposed to come from 
firmware. If you want to change the spec then you can contribute 
feedback in MIPI circles (MIPI have a mechanism for maintainers to 
provide such feedback without company/employer membership requirements)

So either we add a sysfs layer that represents a controller (better in 
my opinion so that we can show the link/master count), or keep the 
existing hierarchy but expand the name with a unique ID so that Qualcomm 
don't get errors with duplicate sysfs link0 entries.

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Vinod Koul]

On 02-02-21, 10:43, Pierre-Louis Bossart wrote:
> 
> 
> On 2/1/21 10:18 PM, Vinod Koul wrote:
> > On 01-02-21, 10:10, Pierre-Louis Bossart wrote:
> > > On 2/1/21 4:14 AM, Vinod Koul wrote:
> > > > On 21-01-21, 17:23, Srinivas Kandagatla wrote:
> > > > > On 21/01/2021 15:12, Pierre-Louis Bossart wrote:
> > > > > > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote:
> > 
> > > > > I totally agree!
> > > > > 
> > > > > If I understand it correctly in Intel case there will be only one Link ID
> > > > > per bus.
> > > > 
> > > > Yes IIUC there would be one link id per bus.
> > > > 
> > > > the ida approach gives us unique id for each master,bus I would like to
> > > > propose using that everywhere
> > > 
> > > We have cases where link2 is not used but link0, 1 and 3 are.
> > > Using the IDA would result in master-0,1,2 being shown, that would throw the
> > > integrator off. the link_id is related to hardware and can tolerate gaps,
> > > the IDA is typically always increasing and is across the system, not
> > > controller specific.
> > > 
> > > We can debate forever but both pieces of information are useful, so my
> > > recommendation is to use both:
> > > 
> > > snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id);
> > 
> > I agree we should use both, but does it really make sense for naming? We
> > can keep name in ida and expose the link_id as a parameter for
> > integrators to see in sysfs.
> 
> That would mean changing the meaning of sysfs properties:
> 
> /*
>  * The sysfs for properties reflects the MIPI description as given
>  * in the MIPI DisCo spec
>  *
>  * Base file is:
>  *	sdw-master-N

Key is "The sysfs for properties" is for property files. I am not sure
how this implies for a number above. I was thinking of using ID for N
here and add a link_id file below which represents the link-id property

>  *      |---- revision
>  *      |---- clk_stop_modes
>  *      |---- max_clk_freq
>  *      |---- clk_freq
>  *      |---- clk_gears
>  *      |---- default_row
>  *      |---- default_col
>  *      |---- dynamic_shape
>  *      |---- err_threshold
>  */
> 
> N is the link ID in the spec. I am not convinced we'd do the community a
> service by unilaterally changing what an external spec means, or add a
> property that's kernel-defined while the rest is supposed to come from
> firmware. If you want to change the spec then you can contribute feedback in
> MIPI circles (MIPI have a mechanism for maintainers to provide such feedback
> without company/employer membership requirements)
> 
> So either we add a sysfs layer that represents a controller (better in my
> opinion so that we can show the link/master count), or keep the existing
> hierarchy but expand the name with a unique ID so that Qualcomm don't get
> errors with duplicate sysfs link0 entries.

Anyway we are late in cycle for this.. I am reverting this patch and we
can arrive at consensus and fix this for next cycle

Thanks
-- 
~Vinod

Re: [PATCH] soundwire: debugfs: use controller id instead of link_id

[Vinod Koul]

On 03-02-21, 16:44, Vinod Koul wrote:
> On 02-02-21, 10:43, Pierre-Louis Bossart wrote:
> > 
> > 
> > On 2/1/21 10:18 PM, Vinod Koul wrote:
> > > On 01-02-21, 10:10, Pierre-Louis Bossart wrote:
> > > > On 2/1/21 4:14 AM, Vinod Koul wrote:
> > > > > On 21-01-21, 17:23, Srinivas Kandagatla wrote:
> > > > > > On 21/01/2021 15:12, Pierre-Louis Bossart wrote:
> > > > > > > On 1/21/21 6:03 AM, Srinivas Kandagatla wrote:
> > > 
> > > > > > I totally agree!
> > > > > > 
> > > > > > If I understand it correctly in Intel case there will be only one Link ID
> > > > > > per bus.
> > > > > 
> > > > > Yes IIUC there would be one link id per bus.
> > > > > 
> > > > > the ida approach gives us unique id for each master,bus I would like to
> > > > > propose using that everywhere
> > > > 
> > > > We have cases where link2 is not used but link0, 1 and 3 are.
> > > > Using the IDA would result in master-0,1,2 being shown, that would throw the
> > > > integrator off. the link_id is related to hardware and can tolerate gaps,
> > > > the IDA is typically always increasing and is across the system, not
> > > > controller specific.
> > > > 
> > > > We can debate forever but both pieces of information are useful, so my
> > > > recommendation is to use both:
> > > > 
> > > > snprintf(name, sizeof(name), "master-%d-%d", bus_id, bus->link_id);
> > > 
> > > I agree we should use both, but does it really make sense for naming? We
> > > can keep name in ida and expose the link_id as a parameter for
> > > integrators to see in sysfs.
> > 
> > That would mean changing the meaning of sysfs properties:
> > 
> > /*
> >  * The sysfs for properties reflects the MIPI description as given
> >  * in the MIPI DisCo spec
> >  *
> >  * Base file is:
> >  *	sdw-master-N
> 
> Key is "The sysfs for properties" is for property files. I am not sure
> how this implies for a number above. I was thinking of using ID for N
> here and add a link_id file below which represents the link-id property
> 
> >  *      |---- revision
> >  *      |---- clk_stop_modes
> >  *      |---- max_clk_freq
> >  *      |---- clk_freq
> >  *      |---- clk_gears
> >  *      |---- default_row
> >  *      |---- default_col
> >  *      |---- dynamic_shape
> >  *      |---- err_threshold
> >  */
> > 
> > N is the link ID in the spec. I am not convinced we'd do the community a
> > service by unilaterally changing what an external spec means, or add a
> > property that's kernel-defined while the rest is supposed to come from
> > firmware. If you want to change the spec then you can contribute feedback in
> > MIPI circles (MIPI have a mechanism for maintainers to provide such feedback
> > without company/employer membership requirements)
> > 
> > So either we add a sysfs layer that represents a controller (better in my
> > opinion so that we can show the link/master count), or keep the existing
> > hierarchy but expand the name with a unique ID so that Qualcomm don't get
> > errors with duplicate sysfs link0 entries.
> 
> Anyway we are late in cycle for this.. I am reverting this patch and we
> can arrive at consensus and fix this for next cycle

Reverted and pushed out now

-- 
~Vinod