* [PATCH] soundwire: debugfs: use controller id instead of link_id
@ 2021-01-15 16:25 Srinivas Kandagatla
2021-01-19 14:52 ` Vinod Koul
0 siblings, 1 reply; 15+ messages in thread
From: Srinivas Kandagatla @ 2021-01-15 16:25 UTC (permalink / raw)
To: vkoul, yung-chuan.liao
Cc: pierre-louis.bossart, sanyog.r.kale, gregkh, alsa-devel,
linux-kernel, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-01-15 16:25 [PATCH] soundwire: debugfs: use controller id instead of link_id Srinivas Kandagatla
@ 2021-01-19 14:52 ` Vinod Koul
2021-01-19 15:54 ` Pierre-Louis Bossart
0 siblings, 1 reply; 15+ messages in thread
From: Vinod Koul @ 2021-01-19 14:52 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale, gregkh,
alsa-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-01-19 14:52 ` Vinod Koul
@ 2021-01-19 15:54 ` Pierre-Louis Bossart
2021-01-19 17:17 ` Srinivas Kandagatla
0 siblings, 1 reply; 15+ messages in thread
From: Pierre-Louis Bossart @ 2021-01-19 15:54 UTC (permalink / raw)
To: Vinod Koul, Srinivas Kandagatla
Cc: alsa-devel, gregkh, linux-kernel, sanyog.r.kale, yung-chuan.liao
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-01-19 15:54 ` Pierre-Louis Bossart
@ 2021-01-19 17:17 ` Srinivas Kandagatla
2021-01-19 19:09 ` Pierre-Louis Bossart
0 siblings, 1 reply; 15+ messages in thread
From: Srinivas Kandagatla @ 2021-01-19 17:17 UTC (permalink / raw)
To: Pierre-Louis Bossart, Vinod Koul
Cc: alsa-devel, gregkh, linux-kernel, sanyog.r.kale, yung-chuan.liao
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-01-19 17:17 ` Srinivas Kandagatla
@ 2021-01-19 19:09 ` Pierre-Louis Bossart
2021-01-21 12:03 ` Srinivas Kandagatla
0 siblings, 1 reply; 15+ messages in thread
From: Pierre-Louis Bossart @ 2021-01-19 19:09 UTC (permalink / raw)
To: Srinivas Kandagatla, Vinod Koul
Cc: alsa-devel, gregkh, linux-kernel, sanyog.r.kale, yung-chuan.liao
>>>> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-01-19 19:09 ` Pierre-Louis Bossart
@ 2021-01-21 12:03 ` Srinivas Kandagatla
2021-01-21 15:12 ` Pierre-Louis Bossart
0 siblings, 1 reply; 15+ messages in thread
From: Srinivas Kandagatla @ 2021-01-21 12:03 UTC (permalink / raw)
To: Pierre-Louis Bossart, Vinod Koul
Cc: alsa-devel, gregkh, linux-kernel, sanyog.r.kale, yung-chuan.liao
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-01-21 12:03 ` Srinivas Kandagatla
@ 2021-01-21 15:12 ` Pierre-Louis Bossart
2021-01-21 17:23 ` Srinivas Kandagatla
0 siblings, 1 reply; 15+ messages in thread
From: Pierre-Louis Bossart @ 2021-01-21 15:12 UTC (permalink / raw)
To: Srinivas Kandagatla, Vinod Koul
Cc: gregkh, alsa-devel, yung-chuan.liao, linux-kernel, sanyog.r.kale
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-01-21 15:12 ` Pierre-Louis Bossart
@ 2021-01-21 17:23 ` Srinivas Kandagatla
2021-01-21 18:22 ` Pierre-Louis Bossart
2021-02-01 10:14 ` Vinod Koul
0 siblings, 2 replies; 15+ messages in thread
From: Srinivas Kandagatla @ 2021-01-21 17:23 UTC (permalink / raw)
To: Pierre-Louis Bossart, Vinod Koul
Cc: gregkh, alsa-devel, yung-chuan.liao, linux-kernel, sanyog.r.kale
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-01-21 17:23 ` Srinivas Kandagatla
@ 2021-01-21 18:22 ` Pierre-Louis Bossart
2021-02-01 10:14 ` Vinod Koul
1 sibling, 0 replies; 15+ messages in thread
From: Pierre-Louis Bossart @ 2021-01-21 18:22 UTC (permalink / raw)
To: Srinivas Kandagatla, Vinod Koul
Cc: gregkh, alsa-devel, yung-chuan.liao, linux-kernel, sanyog.r.kale
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-01-21 17:23 ` Srinivas Kandagatla
2021-01-21 18:22 ` Pierre-Louis Bossart
@ 2021-02-01 10:14 ` Vinod Koul
2021-02-01 16:10 ` Pierre-Louis Bossart
1 sibling, 1 reply; 15+ messages in thread
From: Vinod Koul @ 2021-02-01 10:14 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Pierre-Louis Bossart, gregkh, alsa-devel, yung-chuan.liao,
linux-kernel, sanyog.r.kale
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-02-01 10:14 ` Vinod Koul
@ 2021-02-01 16:10 ` Pierre-Louis Bossart
2021-02-02 4:18 ` Vinod Koul
0 siblings, 1 reply; 15+ messages in thread
From: Pierre-Louis Bossart @ 2021-02-01 16:10 UTC (permalink / raw)
To: Vinod Koul, Srinivas Kandagatla
Cc: gregkh, alsa-devel, yung-chuan.liao, linux-kernel, sanyog.r.kale
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);
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-02-01 16:10 ` Pierre-Louis Bossart
@ 2021-02-02 4:18 ` Vinod Koul
2021-02-02 16:43 ` Pierre-Louis Bossart
0 siblings, 1 reply; 15+ messages in thread
From: Vinod Koul @ 2021-02-02 4:18 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Srinivas Kandagatla, gregkh, alsa-devel, yung-chuan.liao,
linux-kernel, sanyog.r.kale
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-02-02 4:18 ` Vinod Koul
@ 2021-02-02 16:43 ` Pierre-Louis Bossart
2021-02-03 11:14 ` Vinod Koul
0 siblings, 1 reply; 15+ messages in thread
From: Pierre-Louis Bossart @ 2021-02-02 16:43 UTC (permalink / raw)
To: Vinod Koul
Cc: alsa-devel, gregkh, linux-kernel, Srinivas Kandagatla,
sanyog.r.kale, yung-chuan.liao
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-02-02 16:43 ` Pierre-Louis Bossart
@ 2021-02-03 11:14 ` Vinod Koul
2021-02-06 10:22 ` Vinod Koul
0 siblings, 1 reply; 15+ messages in thread
From: Vinod Koul @ 2021-02-03 11:14 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, gregkh, linux-kernel, Srinivas Kandagatla,
sanyog.r.kale, yung-chuan.liao
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] soundwire: debugfs: use controller id instead of link_id
2021-02-03 11:14 ` Vinod Koul
@ 2021-02-06 10:22 ` Vinod Koul
0 siblings, 0 replies; 15+ messages in thread
From: Vinod Koul @ 2021-02-06 10:22 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: alsa-devel, gregkh, linux-kernel, Srinivas Kandagatla,
sanyog.r.kale, yung-chuan.liao
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-02-06 10:23 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-15 16:25 [PATCH] soundwire: debugfs: use controller id instead of link_id Srinivas Kandagatla
2021-01-19 14:52 ` Vinod Koul
2021-01-19 15:54 ` Pierre-Louis Bossart
2021-01-19 17:17 ` Srinivas Kandagatla
2021-01-19 19:09 ` Pierre-Louis Bossart
2021-01-21 12:03 ` Srinivas Kandagatla
2021-01-21 15:12 ` Pierre-Louis Bossart
2021-01-21 17:23 ` Srinivas Kandagatla
2021-01-21 18:22 ` Pierre-Louis Bossart
2021-02-01 10:14 ` Vinod Koul
2021-02-01 16:10 ` Pierre-Louis Bossart
2021-02-02 4:18 ` Vinod Koul
2021-02-02 16:43 ` Pierre-Louis Bossart
2021-02-03 11:14 ` Vinod Koul
2021-02-06 10:22 ` Vinod Koul
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).