| Message ID | 20200311221026.18174-5-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SoundWire: intel: fix SHIM programming sequences | expand |
On 11-03-20, 17:10, Pierre-Louis Bossart wrote: > We want to make sure SHIM register fields such as SYNCPRD are only > programmed once. Since we don't have a controller-level driver, we > need master-level drivers to collaborate: the registers will only be > programmed when the first link is powered-up. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/intel.h | 2 ++ > include/linux/soundwire/sdw_intel.h | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h > index 568c84a80d79..cfc83120b8f9 100644 > --- a/drivers/soundwire/intel.h > +++ b/drivers/soundwire/intel.h > @@ -16,6 +16,7 @@ > * @ops: Shim callback ops > * @dev: device implementing hw_params and free callbacks > * @shim_lock: mutex to handle access to shared SHIM registers > + * @shim_mask: global pointer to check SHIM register initialization > */ > struct sdw_intel_link_res { > struct platform_device *pdev; > @@ -27,6 +28,7 @@ struct sdw_intel_link_res { > const struct sdw_intel_ops *ops; > struct device *dev; > struct mutex *shim_lock; /* protect shared registers */ > + u32 *shim_mask; You have a pointer, okay where is it initialized > }; > > #endif /* __SDW_INTEL_LOCAL_H */ > diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h > index 979b41b5dcb4..120ffddc03d2 100644 > --- a/include/linux/soundwire/sdw_intel.h > +++ b/include/linux/soundwire/sdw_intel.h > @@ -115,6 +115,7 @@ struct sdw_intel_slave_id { > * links > * @link_list: list to handle interrupts across all links > * @shim_lock: mutex to handle concurrent rmw access to shared SHIM registers. > + * @shim_mask: flags to track initialization of SHIM shared registers > */ > struct sdw_intel_ctx { > int count; > @@ -126,6 +127,7 @@ struct sdw_intel_ctx { > struct sdw_intel_slave_id *ids; > struct list_head link_list; > struct mutex shim_lock; /* lock for access to shared SHIM registers */ > + u32 shim_mask; And a integer, question: why do you need pointer and integer, why not use only one..?
>> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h >> index 568c84a80d79..cfc83120b8f9 100644 >> --- a/drivers/soundwire/intel.h >> +++ b/drivers/soundwire/intel.h >> @@ -16,6 +16,7 @@ >> * @ops: Shim callback ops >> * @dev: device implementing hw_params and free callbacks >> * @shim_lock: mutex to handle access to shared SHIM registers >> + * @shim_mask: global pointer to check SHIM register initialization >> */ >> struct sdw_intel_link_res { >> struct platform_device *pdev; >> @@ -27,6 +28,7 @@ struct sdw_intel_link_res { >> const struct sdw_intel_ops *ops; >> struct device *dev; >> struct mutex *shim_lock; /* protect shared registers */ >> + u32 *shim_mask; > > You have a pointer, okay where is it initialized same answer as shim_lock, it's initialized at the higher level https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L252 >> }; >> >> #endif /* __SDW_INTEL_LOCAL_H */ >> diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h >> index 979b41b5dcb4..120ffddc03d2 100644 >> --- a/include/linux/soundwire/sdw_intel.h >> +++ b/include/linux/soundwire/sdw_intel.h >> @@ -115,6 +115,7 @@ struct sdw_intel_slave_id { >> * links >> * @link_list: list to handle interrupts across all links >> * @shim_lock: mutex to handle concurrent rmw access to shared SHIM registers. >> + * @shim_mask: flags to track initialization of SHIM shared registers >> */ >> struct sdw_intel_ctx { >> int count; >> @@ -126,6 +127,7 @@ struct sdw_intel_ctx { >> struct sdw_intel_slave_id *ids; >> struct list_head link_list; >> struct mutex shim_lock; /* lock for access to shared SHIM registers */ >> + u32 shim_mask; > > And a integer, question: why do you need pointer and integer, why not > use only one..? You may have missed that the structures are different. the sdw_intel_ctx is global for all links, so that the shim_mask integer value. the sdw_intel_link_res is link-specific and use a pointer to the same shim_mask value, protected by shim_lock.
On 20-03-20, 09:13, Pierre-Louis Bossart wrote: > > > > diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h > > > index 568c84a80d79..cfc83120b8f9 100644 > > > --- a/drivers/soundwire/intel.h > > > +++ b/drivers/soundwire/intel.h > > > @@ -16,6 +16,7 @@ > > > * @ops: Shim callback ops > > > * @dev: device implementing hw_params and free callbacks > > > * @shim_lock: mutex to handle access to shared SHIM registers > > > + * @shim_mask: global pointer to check SHIM register initialization > > > */ > > > struct sdw_intel_link_res { > > > struct platform_device *pdev; > > > @@ -27,6 +28,7 @@ struct sdw_intel_link_res { > > > const struct sdw_intel_ops *ops; > > > struct device *dev; > > > struct mutex *shim_lock; /* protect shared registers */ > > > + u32 *shim_mask; > > > > You have a pointer, okay where is it initialized > > same answer as shim_lock, it's initialized at the higher level > > https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L252 Why can't it be done here, what stops you? You really need to keep initialzation and usage in same patch :(
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index 568c84a80d79..cfc83120b8f9 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -16,6 +16,7 @@ * @ops: Shim callback ops * @dev: device implementing hw_params and free callbacks * @shim_lock: mutex to handle access to shared SHIM registers + * @shim_mask: global pointer to check SHIM register initialization */ struct sdw_intel_link_res { struct platform_device *pdev; @@ -27,6 +28,7 @@ struct sdw_intel_link_res { const struct sdw_intel_ops *ops; struct device *dev; struct mutex *shim_lock; /* protect shared registers */ + u32 *shim_mask; }; #endif /* __SDW_INTEL_LOCAL_H */ diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index 979b41b5dcb4..120ffddc03d2 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -115,6 +115,7 @@ struct sdw_intel_slave_id { * links * @link_list: list to handle interrupts across all links * @shim_lock: mutex to handle concurrent rmw access to shared SHIM registers. + * @shim_mask: flags to track initialization of SHIM shared registers */ struct sdw_intel_ctx { int count; @@ -126,6 +127,7 @@ struct sdw_intel_ctx { struct sdw_intel_slave_id *ids; struct list_head link_list; struct mutex shim_lock; /* lock for access to shared SHIM registers */ + u32 shim_mask; }; /**
We want to make sure SHIM register fields such as SYNCPRD are only programmed once. Since we don't have a controller-level driver, we need master-level drivers to collaborate: the registers will only be programmed when the first link is powered-up. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/intel.h | 2 ++ include/linux/soundwire/sdw_intel.h | 2 ++ 2 files changed, 4 insertions(+)