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.
--
~Vinod
