| Message ID | 20200818024120.20721-6-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: intel: add multi-link support | expand |
On 18-08-20, 10:41, Bard Liao wrote: > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > Hardware-based synchronization is typically required when the > bus->multi_link flag is set. > > On Intel platforms, when the Cadence IP is configured in 'Multi Master > Mode', the hardware synchronization is required even when a stream > only uses a single segment. The existing code only deal with hardware > synchronization when a stream uses more than one segment so to remain > backwards compatible we add a configuration threshold. For Intel cases > this threshold will be set to one, other platforms may be able to use > the SSP-based sync in those cases. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> > --- > include/linux/soundwire/sdw.h | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h > index 76052f12c9f7..9adbe4fd7980 100644 > --- a/include/linux/soundwire/sdw.h > +++ b/include/linux/soundwire/sdw.h > @@ -827,6 +827,11 @@ struct sdw_master_ops { > * @multi_link: Store bus property that indicates if multi links > * are supported. This flag is populated by drivers after reading > * appropriate firmware (ACPI/DT). > + * @hw_sync_min_links: Number of links used by a stream above which > + * hardware-based synchronization is required. This value is only > + * meaningful if multi_link is set. If set to 1, hardware-based > + * synchronization will be used even if a stream only uses a single > + * SoundWire segment. Soundwire spec does not say anything about multi-link so this is left to implementer. Assuming that value of 1 would mean hw based sync will be used even for single stream does not make sense in generic terms. Maybe yes for Intel but may not be true for everyone? We already use m_rt_count in code for this, so the question is why is that not sufficient? > */ > struct sdw_bus { > struct device *dev; > @@ -850,6 +855,7 @@ struct sdw_bus { > unsigned int clk_stop_timeout; > u32 bank_switch_timeout; > bool multi_link; > + int hw_sync_min_links; > }; > > int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent, > -- > 2.17.1
>> + * @hw_sync_min_links: Number of links used by a stream above which >> + * hardware-based synchronization is required. This value is only >> + * meaningful if multi_link is set. If set to 1, hardware-based >> + * synchronization will be used even if a stream only uses a single >> + * SoundWire segment. > > Soundwire spec does not say anything about multi-link so this is left to > implementer. Assuming that value of 1 would mean hw based sync will > be used even for single stream does not make sense in generic terms. > Maybe yes for Intel but may not be true for everyone? hw-based sync is required for Intel even for single stream. It's been part of the recommended programming flows since the beginning but ignored so far. That said, this value is set by each master implementation, no one forces non-Intel users to implement an Intel-specific requirement. > We already use m_rt_count in code for this, so the question is why is > that not sufficient? Because as you rightly said above, Intel requires the hw_sync to be used even for single stream, but we didn't want others to be forced to use the hw-sync for single stream. the m_rt_count is not sufficient for Intel. I think we are in agreement on not forcing everyone to follow what is required by Intel, and that's precisely why we added this setting. If you set it to two you would only use hw_sync when two masters are used.
On 26-08-20, 09:09, Pierre-Louis Bossart wrote: > > > > > + * @hw_sync_min_links: Number of links used by a stream above which > > > + * hardware-based synchronization is required. This value is only > > > + * meaningful if multi_link is set. If set to 1, hardware-based > > > + * synchronization will be used even if a stream only uses a single > > > + * SoundWire segment. > > > > Soundwire spec does not say anything about multi-link so this is left to > > implementer. Assuming that value of 1 would mean hw based sync will > > be used even for single stream does not make sense in generic terms. > > Maybe yes for Intel but may not be true for everyone? > > hw-based sync is required for Intel even for single stream. It's been part > of the recommended programming flows since the beginning but ignored so far. > > That said, this value is set by each master implementation, no one forces > non-Intel users to implement an Intel-specific requirement. > > > We already use m_rt_count in code for this, so the question is why is > > that not sufficient? > > Because as you rightly said above, Intel requires the hw_sync to be used > even for single stream, but we didn't want others to be forced to use the > hw-sync for single stream. the m_rt_count is not sufficient for Intel. > > I think we are in agreement on not forcing everyone to follow what is > required by Intel, and that's precisely why we added this setting. If you > set it to two you would only use hw_sync when two masters are used. Okay, it would be better if we move it to intel driver, but I see it may not be trivial, so lets go with this approach.
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 76052f12c9f7..9adbe4fd7980 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -827,6 +827,11 @@ struct sdw_master_ops { * @multi_link: Store bus property that indicates if multi links * are supported. This flag is populated by drivers after reading * appropriate firmware (ACPI/DT). + * @hw_sync_min_links: Number of links used by a stream above which + * hardware-based synchronization is required. This value is only + * meaningful if multi_link is set. If set to 1, hardware-based + * synchronization will be used even if a stream only uses a single + * SoundWire segment. */ struct sdw_bus { struct device *dev; @@ -850,6 +855,7 @@ struct sdw_bus { unsigned int clk_stop_timeout; u32 bank_switch_timeout; bool multi_link; + int hw_sync_min_links; }; int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,