From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pierre-Louis Bossart Subject: Re: [alsa-devel] [RFC PATCH 25/40] soundwire: intel: use BIOS information to set clock dividers Date: Mon, 5 Aug 2019 10:40:13 -0500 Message-ID: References: <20190725234032.21152-1-pierre-louis.bossart@linux.intel.com> <20190725234032.21152-26-pierre-louis.bossart@linux.intel.com> <20190805102828.GF22437@buildpc-HP-Z230> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20190805102828.GF22437@buildpc-HP-Z230> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Sanyog Kale Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, tiwai@suse.de, broonie@kernel.org, vkoul@kernel.org, gregkh@linuxfoundation.org, jank@cadence.com, srinivas.kandagatla@linaro.org, slawomir.blauciak@intel.com List-Id: alsa-devel@alsa-project.org 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 >> --- >> 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).