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?
