Re: [alsa-devel] [RFC PATCH 25/40] soundwire: intel: use BIOS information to set clock dividers

[Pierre-Louis Bossart]

On 8/5/19 5:28 AM, Sanyog Kale wrote:
> On Thu, Jul 25, 2019 at 06:40:17PM -0500, Pierre-Louis Bossart wrote:
> > The BIOS provides an Intel-specific property, let's use it to avoid
> > hard-coded clock dividers.
> >
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/soundwire/cadence_master.c | 26 ++++++++++++++++++++++----
> >   drivers/soundwire/intel.c          | 26 ++++++++++++++++++++++++++
> >   include/linux/soundwire/sdw.h      |  2 ++
> >   3 files changed, 50 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> > index d84344e29f71..10ebcef2e84e 100644
> > --- a/drivers/soundwire/cadence_master.c
> > +++ b/drivers/soundwire/cadence_master.c
> > @@ -173,8 +173,6 @@
> >   #define CDNS_PDI_CONFIG_PORT			GENMASK(4, 0)
> >   
> >   /* Driver defaults */
> > -
> > -#define CDNS_DEFAULT_CLK_DIVIDER		0
> >   #define CDNS_DEFAULT_SSP_INTERVAL		0x18
> >   #define CDNS_TX_TIMEOUT				2000
> >   
> > @@ -973,7 +971,10 @@ static u32 cdns_set_default_frame_shape(int n_rows, int n_cols)
> >    */
> >   int sdw_cdns_init(struct sdw_cdns *cdns)
> >   {
> > +	struct sdw_bus *bus = &cdns->bus;
> > +	struct sdw_master_prop *prop = &bus->prop;
> >   	u32 val;
> > +	int divider;
> >   	int ret;
> >   
> >   	/* Exit clock stop */
> > @@ -985,9 +986,17 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
> >   	}
> >   
> >   	/* Set clock divider */
> > +	divider	= (prop->mclk_freq / prop->max_clk_freq) - 1;
>
> Do you expect mclk_freq and max_clk_freq to be same?

Nope. For Icelake the MCLK is 38.4 MHz and is, but the max_clk needs to 
be 9.6 max (can't be higher per the SoundWire spec).

The max_clk_freq may even be lower thank 9.6 MHz due to specific 
topologies where the higher frequencies are problematic if the trace 
lengths are too long.

For CNL/CML the MCLK is 24 MHz but ironically we run the bus at 12 MHz, 
so the divider is smaller.

> >   	val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);
> > -	val |= CDNS_DEFAULT_CLK_DIVIDER;
> > +	val |= divider;
> >   	cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
> > +	cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
> > +
> > +	pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
> > +	       prop->mclk_freq,
> > +	       prop->max_clk_freq,
> > +	       divider,
> > +	       val);
>
> This can be removed.

yes, done already.

> >   
> >   	/* Set the default frame shape */
> >   	val = cdns_set_default_frame_shape(prop->default_row,
> > @@ -1035,6 +1044,7 @@ EXPORT_SYMBOL(sdw_cdns_init);
> >   
> >   int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params)
> >   {
> > +	struct sdw_master_prop *prop = &bus->prop;
> >   	struct sdw_cdns *cdns = bus_to_cdns(bus);
> >   	int mcp_clkctrl_off, mcp_clkctrl;
> >   	int divider;
> > @@ -1044,7 +1054,9 @@ int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params)
> >   		return -EINVAL;
> >   	}
> >   
> > -	divider	= (params->max_dr_freq / params->curr_dr_freq) - 1;
> > +	divider	= prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR /
>
> What is the reason for not using max_dr_freq? Its precomputed as
> prop->max_clk_freq * SDW_DOUBLE_RATE_FACTOR;

no, as explained above the divider needs to start from the clock 
provided to the IP, which is different from the max frequency clock the 
bus operates at. the MCLK is a fixed value for all platforms using the 
same SOC/PCH, the max_clk is platform-dependent and its value is 
provided by the firmware (BIOS/DT).