Hi Pierre,
On 11-09-20, 09:50, Pierre-Louis Bossart wrote:
> > > + * 25 0 (Reserved) > > > + * 24:22 Function Number [2:0] > > > + * 21 Entity[6] > > > + * 20:19 Control Selector[5:4] > > > + * 18 0 (Reserved) > > > + * 17:15 Control Number[5:3] > > > + * 14 Next > > > + * 13 MBQ > > > + * 12:7 Entity[5:0] > > > + * 6:3 Control Selector[3:0] > > > + * 2:0 Control Number[2:0]
[...]
#define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19) #define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4) #define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3) #define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)
I think I missed ORIG and DEST stuff, what does this mean here?
If you missed this, it means my explanations are not good enough and I need to make it clearer in the commit log/documentation. Point taken, I'll improve this for the next version.
Relooking at the bit definition, for example 'Control Number' is defined in both 17:15 as well as 2:0, why is that. Is it split?
How does one program a control number into this?
A Control Number is represented on 6 bits.
See the documentation above.
17:15 Control Selector[5:3] 2:0 Control Selector[2:0]
The 3 MSBs for into bits 17:15 of the address, and the 3 LSBs into bits 2:0 of the address. The second part is simpler for Control Number but for entities and control selectors the LSB positions don't match.
Yes it's convoluted but it was well-intended: in most cases, there is a limited number of entities, control selectors, channel numbers, and putting the LSBs together in the 16-LSB of the address helps avoid reprogramming paging registers: all the addresses for a given function typically map into the same page.
That said, I am not sure the optimization is that great in the end, because we end-up having to play with bits for each address. Fewer changes of the paging registers but tons of operations in the core.
I wasn't around when this mapping was defined, and it is what is is now. There's hardware built based on this formula so we have to make it work.
Does this clarify the usage?
Thanks, that is very helpful. I have overlooked this bit.
For LSB bits, I dont think this is an issue. I expect it to work, for example: #define CONTROL_LSB_MASK GENMASK(2, 0) foo |= u32_encode_bits(control, CONTROL_LSB_MASK);
would mask the control value and program that in specific bitfeild.
But for MSB bits, I am not sure above will work so, you may need to extract the bits and then use, for example: #define CONTROL_MSB_BITS GENMASK(5, 3) #define CONTROL_MSB_MASK GENMASK(17, 15)
control = FIELD_GET(CONTROL_MSB_BITS, control); foo |= u32_encode_bits(control, CONTROL_MSB_MASK);
If you have a better suggestion that the FIELD_PREP/FIELD_GET use, I am all ears. At the end of the day, the mapping is pre-defined and we don't have any degree of freedom. What I do want is that this macro/inline function is shared by all codec drivers so that we don't have different interpretations of how the address is constructed.
Absolutely, this need to be defined here and used by everyone else.