Re: [PATCH v2 2/3] soundwire: SDCA: add helper macro to access controls

[Vinod Koul <vkoul@kernel.org>]

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