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

SOF topologies hard-code the MCLK used for SSP connections. That was a
bad idea in hindsight, this information should really come from BIOS
and/or machine driver.

This patch introduces a helper to scan all SSP endpoints connected to
a codec, and all formats to see what MCLK is used. When BIT(0) of the
mdivc offset if set in the SSP blob, MCLK0 is used, and likewise when
BIT(1) is set MCLK1 is used.

The case where both MCLKs are used is possible but has never been seen
in practice so should be treated as an error by the caller.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 include/sound/intel-nhlt.h |  7 +++++
 sound/hda/intel-nhlt.c     | 61 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

On Mon, 22 Aug 2022 20:59:09 +0200,
Pierre-Louis Bossart wrote:
> 
> +#define SSP_BLOB_V1_0_SIZE		84
> +#define SSP_BLOB_V1_0_MDIVC_OFFSET	19 /* offset in u32 */
> +#define SSP_BLOB_V1_5_SIZE		96
> +#define SSP_BLOB_V1_5_MDIVC_OFFSET	21 /* offset in u32 */

This is 84 in bytes, which is equal with SSP_BLOB_V1_0_size.
So...

> +			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;

... the size check is >= 84.  If cfg->config.size==84, it may be an
out-of-bound read at blob[SSP_BLOB_V1_5_MDIVC_OFFSET]?

I don't think this would really matter in practice, but it's better to
have a proper check, of course.


thanks,

Takashi

Hi Amadeusz,

>> +int intel_nhlt_ssp_mclk_mask(struct nhlt_acpi_table *nhlt, int ssp_num)
>> +{
>> +    struct nhlt_endpoint *epnt;
>> +    struct nhlt_fmt *fmt;
>> +    struct nhlt_fmt_cfg *cfg;
>> +    int mclk_mask = 0;
>> +    int i, j;
>> +
>> +    if (!nhlt)
>> +        return 0;
>> +
>> +    epnt = (struct nhlt_endpoint *)nhlt->desc;
>> +    for (i = 0; i < nhlt->endpoint_count; i++) {
>> +
>> +        /* we only care about endpoints connected to an audio codec
>> over SSP */
>> +        if (epnt->linktype == NHLT_LINK_SSP &&
>> +            epnt->device_type == NHLT_DEVICE_I2S &&
>> +            epnt->virtual_bus_id == ssp_num) {
> 
> if (epnt->linktype != NHLT_LINK_SSP ||
>     epnt->device_type != NHLT_DEVICE_I2S ||
>     epnt->virtual_bus_id != ssp_num)
>     continue;
> 
> and then you can remove one indentation level below?


Would that work? We still need to move the epnt pointer:

epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);

and moving this in the endpoint_count loop would be ugly as well.


>> +
>> +            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);
>> +                }
>> +
>> +                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.

> 
>> +}
>> +EXPORT_SYMBOL(intel_nhlt_ssp_mclk_mask);
>> +
>>   static struct nhlt_specific_cfg *
>>   nhlt_get_specific_cfg(struct device *dev, struct nhlt_fmt *fmt, u8
>> num_ch,
>>                 u32 rate, u8 vbps, u8 bps)
>

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?

>>>>>> +                }
>>>>>> +
>>>>>> +                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.

Yeah, I had to ask multiple times as well. It's far from
self-explanatory and all the findings were based on NHLT shared with me.

See
https://github.com/thesofproject/linux/issues/3336#issuecomment-1206176141
for two examples with MCLK0 and MCLK1 used.

> 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?

Indeed it seems that depending on tools versions and targeted silicon,
MCLK1 may or may not be supported.

I guess it'd be fine to throw a big fat error in case this two-MCLK
configuration ever shows-up. That way we'll know for sure what was deployed.

Thanks for the feedback, I'll update this shortly.