Re: [PATCH 2/4] ALSA: hda: intel-nhlt: add intel_nhlt_ssp_mclk_mask()

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 8/23/2022 5:18 PM, Pierre-Louis Bossart wrote:
> 
>>>>> +
>>>>> +            fmt = (struct nhlt_fmt *)(epnt->config.caps +
>>>>> epnt->config.size);
>>>>> +            cfg = fmt->fmt_config;
>>>>> +
>>>>> +            /*
>>>>> +             * In theory all formats should use the same MCLK but it
>>>>> doesn't hurt to
>>>>> +             * double-check that the configuration is consistent
>>>>> +             */
>>>>> +            for (j = 0; j < fmt->fmt_count; j++) {
>>>>> +                u32 *blob;
>>>>> +                int mdivc_offset;
>>>>> +
>>>>> +                if (cfg->config.size >= SSP_BLOB_V1_0_SIZE) {
>>>>> +                    blob = (u32 *)cfg->config.caps;
>>>>> +
>>>>> +                    if (blob[1] == SSP_BLOB_VER_2_0)
>>>>> +                        mdivc_offset = SSP_BLOB_V2_0_MDIVC_OFFSET;
>>>>> +                    else if (blob[1] == SSP_BLOB_VER_1_5)
>>>>> +                        mdivc_offset = SSP_BLOB_V1_5_MDIVC_OFFSET;
>>>>> +                    else
>>>>> +                        mdivc_offset = SSP_BLOB_V1_0_MDIVC_OFFSET;
>>>>> +
>>>>> +                    mclk_mask |=  blob[mdivc_offset] & GENMASK(1, 0);
>>
>> One more thing, where does this GENMASK come from, as far as I can tell
>> HW specifies and FW uses one bit field to signal that MCLK is enabled?
>> (mdivc is simply a value written to HW register to configure it).
> 
> There are two MCLK signals, that's the point of this patch. We need to
> find which one is used. Platforms typically use MCLK0 except when they
> don't..
> 
> BIT(0) set in mdivc enables MCLK0
> BIT(1) set in mdivc enabled MCLK1
> 
> see
> https://github.com/thesofproject/sof/blob/44a5200c87625588f0028aa08d560e68f2b8dc82/src/drivers/intel/ssp/mn.c#L150
> 
>>>>> +                }
>>>>> +
>>>>> +                cfg = (struct nhlt_fmt_cfg *)(cfg->config.caps +
>>>>> cfg->config.size);
>>>>> +            }
>>>>> +        }
>>>>> +        epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);
>>>>> +    }
>>>>> +
>>>>> +    return mclk_mask;
>>>>
>>>> Although I understand that it is relegated to the caller, but if both
>>>> mclk being set is considered an error maybe add some kind of check here
>>>> instead and free callers from having to remember about it?
>>>>
>>>> if (hweight_long(mclk_mask) != 1)
>>>>       return -EINVAL;
>>>>
>>>> return mclk_mask;
>>>
>>> I went back and forth multiple times on this one. I can't figure out if
>>> this would be a bug or a feature, it could be e.g. a test capability and
>>> it's supported in hardware. I decided to make the decision in the caller
>>> rather than a lower level in the library.
>>>
>>> If the tools used to generate NHLT don't support this multi-MCLK mode
>>> then we could indeed move the test here.
>>>
>>
>> Considering comment I added above I've asked Czarek to also check this
>> series. I'm not sure it even makes sense to name the field "_mask" when
>> it is one bit...
> 
> it's two bits, see above.

So I've spend a bit talking with FW team, and you are right, I got 
confused by one of the tables and some code that specified it as 1 bit 
field and rest as reserved, while other documents do specify it as a 
variable range of bits.

Going back to return value, the tool I have access to only has support 
for MCLK0. I guess we can make the assumption for now that everyone 
connects codec to one clock source and if someone later implements HW 
where somehow 2 different clocks are used (depending on format) we can 
refine the check later?