thesofproject / linux

Linux kernel source tree
Other
91 stars 133 forks source link

SoundWire: improve LunarLake tests #5062

Closed plbossart closed 5 months ago

plbossart commented 5 months ago

Make sure the bus does not run too fast and apply minor corrections found in documented programming flows.

plbossart commented 5 months ago

the bank switch error is back @bardliao, see https://sof-ci.01.org/linuxpr/PR5062/build3628/devicetest/index.html?model=LNLM_SDW_AIOC&testcase=multiple-pause-resume-50

I'll drop all my other patches, maybe something gets in the way.

plbossart commented 5 months ago

I don't know what is going on, this PR is supposed to be a cleaner version of https://github.com/thesofproject/linux/pull/5059

But test results tell me otherwise, same FAILs as before.

plbossart commented 5 months ago

well, d'uh, this piece of code is broken as well

/**
 * sdw_compute_bus_params: Compute bus parameters
 *
 * @bus: SDW Bus instance
 */
static int sdw_compute_bus_params(struct sdw_bus *bus)
{
    unsigned int curr_dr_freq = 0;
    struct sdw_master_prop *mstr_prop = &bus->prop;
    int i, clk_values, ret;
    bool is_gear = false;
    u32 *clk_buf;

    if (mstr_prop->num_clk_gears) {
        clk_values = mstr_prop->num_clk_gears;
        clk_buf = mstr_prop->clk_gears;
        is_gear = true;
    } else if (mstr_prop->num_clk_freq) {
        clk_values = mstr_prop->num_clk_freq;
        clk_buf = mstr_prop->clk_freq;
    } else {
        clk_values = 1;
        clk_buf = NULL;
    }

    for (i = 0; i < clk_values; i++) {
        if (!clk_buf)
            curr_dr_freq = bus->params.max_dr_freq;
        else
            curr_dr_freq = (is_gear) ?
                (bus->params.max_dr_freq >>  clk_buf[i]) :
                clk_buf[i] * SDW_DOUBLE_RATE_FACTOR;

        if (curr_dr_freq <= bus->params.bandwidth)
            continue;

        break;

Even though we start the bus at the default frequency, this again selects the max. This is just wrong. I guess this is related to what @bardliao wanted to fix in https://github.com/thesofproject/linux/pull/5063, but the reality is that we don't support clock scaling just yet, so this code should be revisited back to single frequency.

bardliao commented 5 months ago

well, d'uh, this piece of code is broken as well

/**
 * sdw_compute_bus_params: Compute bus parameters
 *
 * @bus: SDW Bus instance
 */
static int sdw_compute_bus_params(struct sdw_bus *bus)
{
  unsigned int curr_dr_freq = 0;
  struct sdw_master_prop *mstr_prop = &bus->prop;
  int i, clk_values, ret;
  bool is_gear = false;
  u32 *clk_buf;

  if (mstr_prop->num_clk_gears) {
      clk_values = mstr_prop->num_clk_gears;
      clk_buf = mstr_prop->clk_gears;
      is_gear = true;
  } else if (mstr_prop->num_clk_freq) {
      clk_values = mstr_prop->num_clk_freq;
      clk_buf = mstr_prop->clk_freq;
  } else {
      clk_values = 1;
      clk_buf = NULL;
  }

  for (i = 0; i < clk_values; i++) {
      if (!clk_buf)
          curr_dr_freq = bus->params.max_dr_freq;
      else
          curr_dr_freq = (is_gear) ?
              (bus->params.max_dr_freq >>  clk_buf[i]) :
              clk_buf[i] * SDW_DOUBLE_RATE_FACTOR;

      if (curr_dr_freq <= bus->params.bandwidth)
          continue;

      break;

Even though we start the bus at the default frequency, this again selects the max. This is just wrong. I guess this is related to what @bardliao wanted to fix in #5063, but the reality is that we don't support clock scaling just yet, so this code should be revisited back to single frequency.

@plbossart I think your PR works, it selects the default the default value. "soundwire sdw-master-0-0: sdw_compute_bus_params: plb: i 0 curr_dr_freq 9600000 bandwidth 2304000" where 9600000 is because 4800000 * SDW_DOUBLE_RATE_FACTOR. But I think what really matter to the bank switch is prop->max_clk_freq. https://github.com/thesofproject/linux/pull/5059 set prop->num_clk_freq = 1. So that the first supported frequency will be max_clk_freq, and cdns_init_clock_ctrl() will set the divider based on mclk and max_clk_freq. The bank switch timed out issue happens when the divider is 1.