Re: [PATCH 05/11] soundwire: bus: update multi-link definition with hw sync details

[Vinod Koul]

On 18-08-20, 10:41, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>
> ---
>  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

-- 
~Vinod