[v3] soundwire: SDCA: add helper macro to access controls

Commit Message

Bard Liao Oct. 29, 2020, 8:49 p.m. UTC

From: Pierre-Louis Bossart <[e-mail]>

The upcoming SDCA (SoundWire Device Class Audio) specification defines
a hierarchical encoding to interface with Class-defined capabilities.

The specification is not yet accessible to the general public but this
information is released with explicit permission from the MIPI Board
to avoid delays with SDCA support on Linux platforms.

A block of 64 MBytes of register addresses are allocated to SDCA
controls, starting at address 0x40000000. The 26 LSBs which identify
individual controls are set based on the following variables:

- Function Number. An SCDA device can be split in up to 8 independent
  Functions. Each of these Functions is described in the SDCA
  specification, e.g. Smart Amplifier, Smart Microphone, Simple
  Microphone, Jack codec, HID, etc.

- Entity Number.  Within each Function,  an Entity is  an identifiable
  block.  Up   to  127  Entities   are  connected  in   a  pre-defined
  graph  (similar to  USB), with  Entity0 reserved  for Function-level
  configurations.  In  contrast  to  USB, the  SDCA  spec  pre-defines
  Function Types, topologies, and allowed  options, i.e. the degree of
  freedom  is not  unlimited to  limit  the possibility  of errors  in
  descriptors leading to software quirks.

- Control Selector. Within each Entity, the SDCA specification defines
  48 controls such as Mute, Gain, AGC, etc, and 16 implementation
  defined ones. Some Control Selectors might be used for low-level
  platform setup, and other exposed to applications and users. Note
  that the same Control Selector capability, e.g. Latency control,
  might be located at different offsets in different entities, the
  Control Selector mapping is Entity-specific.

- Control Number. Some Control Selectors allow channel-specific values
  to be set, with up to 64 channels allowed. This is mostly used for
  volume control.

- Current/Next values. Some Control Selectors are
  'Dual-Ranked'. Software may either update the Current value directly
  for immediate effect. Alternatively, software may write into the
  'Next' values and update the SoundWire 1.2 'Commit Groups' register
  to copy 'Next' values into 'Current' ones in a synchronized
  manner. This is different from bank switching which is typically
  used to change the bus configuration only.

- MBQ. the Multi-Byte Quantity bit is used to provide atomic updates
  when accessing more that one byte, for example a 16-bit volume
  control would be updated consistently, the intermediate values
  mixing old MSB with new LSB are not applied.

These 6 parameters are used to build a 32-bit address to access the
desired Controls. Because of address range, paging is required, but
the most often used parameter values are placed in the lower 16 bits
of the address. This helps to keep the paging registers constant while
updating Controls for a specific Device/Function.

Reviewed-by: Rander Wang <[e-mail]>
Reviewed-by: Guennadi Liakhovetski <[e-mail]>
Reviewed-by: Kai Vehmanen <[e-mail]>
Signed-off-by: Pierre-Louis Bossart <[e-mail]>
Signed-off-by: Bard Liao <[e-mail]>
---
Changelog:

v2:
 - add SDW_SDCA_MBQ_CTL

v3:
 - add SDW_SDCA_NEXT_CTL

---
 include/linux/soundwire/sdw_registers.h | 32 +++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Greg KH Oct. 30, 2020, 9:36 a.m. UTC | #1

On Fri, Oct 30, 2020 at 04:49:55AM +0800, Bard Liao wrote:
> From: Pierre-Louis Bossart <[e-mail]>
> 
> The upcoming SDCA (SoundWire Device Class Audio) specification defines
> a hierarchical encoding to interface with Class-defined capabilities.
> 
> The specification is not yet accessible to the general public but this
> information is released with explicit permission from the MIPI Board
> to avoid delays with SDCA support on Linux platforms.
> 
> A block of 64 MBytes of register addresses are allocated to SDCA
> controls, starting at address 0x40000000. The 26 LSBs which identify
> individual controls are set based on the following variables:
> 
> - Function Number. An SCDA device can be split in up to 8 independent
>   Functions. Each of these Functions is described in the SDCA
>   specification, e.g. Smart Amplifier, Smart Microphone, Simple
>   Microphone, Jack codec, HID, etc.
> 
> - Entity Number.  Within each Function,  an Entity is  an identifiable
>   block.  Up   to  127  Entities   are  connected  in   a  pre-defined
>   graph  (similar to  USB), with  Entity0 reserved  for Function-level
>   configurations.  In  contrast  to  USB, the  SDCA  spec  pre-defines
>   Function Types, topologies, and allowed  options, i.e. the degree of
>   freedom  is not  unlimited to  limit  the possibility  of errors  in
>   descriptors leading to software quirks.
> 
> - Control Selector. Within each Entity, the SDCA specification defines
>   48 controls such as Mute, Gain, AGC, etc, and 16 implementation
>   defined ones. Some Control Selectors might be used for low-level
>   platform setup, and other exposed to applications and users. Note
>   that the same Control Selector capability, e.g. Latency control,
>   might be located at different offsets in different entities, the
>   Control Selector mapping is Entity-specific.
> 
> - Control Number. Some Control Selectors allow channel-specific values
>   to be set, with up to 64 channels allowed. This is mostly used for
>   volume control.
> 
> - Current/Next values. Some Control Selectors are
>   'Dual-Ranked'. Software may either update the Current value directly
>   for immediate effect. Alternatively, software may write into the
>   'Next' values and update the SoundWire 1.2 'Commit Groups' register
>   to copy 'Next' values into 'Current' ones in a synchronized
>   manner. This is different from bank switching which is typically
>   used to change the bus configuration only.
> 
> - MBQ. the Multi-Byte Quantity bit is used to provide atomic updates
>   when accessing more that one byte, for example a 16-bit volume
>   control would be updated consistently, the intermediate values
>   mixing old MSB with new LSB are not applied.
> 
> These 6 parameters are used to build a 32-bit address to access the
> desired Controls. Because of address range, paging is required, but
> the most often used parameter values are placed in the lower 16 bits
> of the address. This helps to keep the paging registers constant while
> updating Controls for a specific Device/Function.
> 
> Reviewed-by: Rander Wang <[e-mail]>
> Reviewed-by: Guennadi Liakhovetski <[e-mail]>
> Reviewed-by: Kai Vehmanen <[e-mail]>
> Signed-off-by: Pierre-Louis Bossart <[e-mail]>
> Signed-off-by: Bard Liao <[e-mail]>
> ---
> Changelog:
> 
> v2:
>  - add SDW_SDCA_MBQ_CTL
> 
> v3:
>  - add SDW_SDCA_NEXT_CTL
> 
> ---
>  include/linux/soundwire/sdw_registers.h | 32 +++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h
> index f420e8059779..e14dff9a9c7f 100644
> --- a/include/linux/soundwire/sdw_registers.h
> +++ b/include/linux/soundwire/sdw_registers.h
> @@ -298,4 +298,36 @@
>  #define SDW_CASC_PORT_MASK_INTSTAT3		1
>  #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3	2
>  
> +/*
> + * v1.2 device - SDCA address mapping
> + *
> + * Spec definition
> + *	Bits		Contents
> + *	31		0 (required by addressing range)
> + *	30:26		0b10000 (Control Prefix)
> + *	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 SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |			\
> +						 (((fun) & 0x7) << 22) |	\
> +						 (((ent) & 0x40) << 15) |	\
> +						 (((ent) & 0x3f) << 7) |	\
> +						 (((ctl) & 0x30) << 15) |	\
> +						 (((ctl) & 0x0f) << 3) |	\
> +						 (((ch) & 0x38) << 12) |	\
> +						 ((ch) & 0x07))
> +
> +#define SDW_SDCA_MBQ_CTL(reg)			((reg) | BIT(13))
> +#define SDW_SDCA_NEXT_CTL(reg)			((reg) | BIT(14))
> +
>  #endif /* __SDW_REGISTERS_H */


No users of these macros?

Liao, Bard Oct. 30, 2020, 11:24 a.m. UTC | #2

> -----Original Message-----
> From: Greg KH <[e-mail]>
> Sent: Friday, October 30, 2020 5:37 PM
> To: Bard Liao <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; pierre-
> [e-mail]; Kale, Sanyog R <[e-mail]>; Lin,
> Mengdong <[e-mail]>; Liao, Bard <[e-mail]>
> Subject: Re: [PATCH v3] soundwire: SDCA: add helper macro to access
> controls
> 
> On Fri, Oct 30, 2020 at 04:49:55AM +0800, Bard Liao wrote:
> > From: Pierre-Louis Bossart <[e-mail]>
> >
> > The upcoming SDCA (SoundWire Device Class Audio) specification defines
> > a hierarchical encoding to interface with Class-defined capabilities.
> >
> > The specification is not yet accessible to the general public but this
> > information is released with explicit permission from the MIPI Board
> > to avoid delays with SDCA support on Linux platforms.
> >
> > A block of 64 MBytes of register addresses are allocated to SDCA
> > controls, starting at address 0x40000000. The 26 LSBs which identify
> > individual controls are set based on the following variables:
> >
> > - Function Number. An SCDA device can be split in up to 8 independent
> >   Functions. Each of these Functions is described in the SDCA
> >   specification, e.g. Smart Amplifier, Smart Microphone, Simple
> >   Microphone, Jack codec, HID, etc.
> >
> > - Entity Number.  Within each Function,  an Entity is  an identifiable
> >   block.  Up   to  127  Entities   are  connected  in   a  pre-defined
> >   graph  (similar to  USB), with  Entity0 reserved  for Function-level
> >   configurations.  In  contrast  to  USB, the  SDCA  spec  pre-defines
> >   Function Types, topologies, and allowed  options, i.e. the degree of
> >   freedom  is not  unlimited to  limit  the possibility  of errors  in
> >   descriptors leading to software quirks.
> >
> > - Control Selector. Within each Entity, the SDCA specification defines
> >   48 controls such as Mute, Gain, AGC, etc, and 16 implementation
> >   defined ones. Some Control Selectors might be used for low-level
> >   platform setup, and other exposed to applications and users. Note
> >   that the same Control Selector capability, e.g. Latency control,
> >   might be located at different offsets in different entities, the
> >   Control Selector mapping is Entity-specific.
> >
> > - Control Number. Some Control Selectors allow channel-specific values
> >   to be set, with up to 64 channels allowed. This is mostly used for
> >   volume control.
> >
> > - Current/Next values. Some Control Selectors are
> >   'Dual-Ranked'. Software may either update the Current value directly
> >   for immediate effect. Alternatively, software may write into the
> >   'Next' values and update the SoundWire 1.2 'Commit Groups' register
> >   to copy 'Next' values into 'Current' ones in a synchronized
> >   manner. This is different from bank switching which is typically
> >   used to change the bus configuration only.
> >
> > - MBQ. the Multi-Byte Quantity bit is used to provide atomic updates
> >   when accessing more that one byte, for example a 16-bit volume
> >   control would be updated consistently, the intermediate values
> >   mixing old MSB with new LSB are not applied.
> >
> > These 6 parameters are used to build a 32-bit address to access the
> > desired Controls. Because of address range, paging is required, but
> > the most often used parameter values are placed in the lower 16 bits
> > of the address. This helps to keep the paging registers constant while
> > updating Controls for a specific Device/Function.
> >
> > Reviewed-by: Rander Wang <[e-mail]>
> > Reviewed-by: Guennadi Liakhovetski
> > <[e-mail]>
> > Reviewed-by: Kai Vehmanen <[e-mail]>
> > Signed-off-by: Pierre-Louis Bossart
> > <[e-mail]>
> > Signed-off-by: Bard Liao <[e-mail]>
> > ---
> > Changelog:
> >
> > v2:
> >  - add SDW_SDCA_MBQ_CTL
> >
> > v3:
> >  - add SDW_SDCA_NEXT_CTL
> >
> > ---
> >  include/linux/soundwire/sdw_registers.h | 32
> > +++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> >
> > diff --git a/include/linux/soundwire/sdw_registers.h
> > b/include/linux/soundwire/sdw_registers.h
> > index f420e8059779..e14dff9a9c7f 100644
> > --- a/include/linux/soundwire/sdw_registers.h
> > +++ b/include/linux/soundwire/sdw_registers.h
> > @@ -298,4 +298,36 @@
> >  #define SDW_CASC_PORT_MASK_INTSTAT3		1
> >  #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3	2
> >
> > +/*
> > + * v1.2 device - SDCA address mapping
> > + *
> > + * Spec definition
> > + *	Bits		Contents
> > + *	31		0 (required by addressing range)
> > + *	30:26		0b10000 (Control Prefix)
> > + *	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 SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |
> 		\
> > +						 (((fun) & 0x7) << 22) |	\
> > +						 (((ent) & 0x40) << 15) |	\
> > +						 (((ent) & 0x3f) << 7) |	\
> > +						 (((ctl) & 0x30) << 15) |	\
> > +						 (((ctl) & 0x0f) << 3) |	\
> > +						 (((ch) & 0x38) << 12) |	\
> > +						 ((ch) & 0x07))
> > +
> > +#define SDW_SDCA_MBQ_CTL(reg)			((reg) | BIT(13))
> > +#define SDW_SDCA_NEXT_CTL(reg)			((reg) | BIT(14))
> > +
> >  #endif /* __SDW_REGISTERS_H */
> 
> 
> No users of these macros?

SDW_SDCA_CTL is used in sdca codec drivers which are not upstream yet.
SDW_SDCA_MBQ_CTL will be used in a new regmap method.
SDW_SDCA_NEXT_CTL can be used in sdca codec drivers, too.

Takashi Iwai Oct. 30, 2020, 11:39 a.m. UTC | #3

On Fri, 30 Oct 2020 12:24:35 +0100,
Liao, Bard wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Greg KH <[e-mail]>
> > Sent: Friday, October 30, 2020 5:37 PM
> > To: Bard Liao <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; pierre-
> > [e-mail]; Kale, Sanyog R <[e-mail]>; Lin,
> > Mengdong <[e-mail]>; Liao, Bard <[e-mail]>
> > Subject: Re: [PATCH v3] soundwire: SDCA: add helper macro to access
> > controls
> > 
> > On Fri, Oct 30, 2020 at 04:49:55AM +0800, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <[e-mail]>
> > >
> > > The upcoming SDCA (SoundWire Device Class Audio) specification defines
> > > a hierarchical encoding to interface with Class-defined capabilities.
> > >
> > > The specification is not yet accessible to the general public but this
> > > information is released with explicit permission from the MIPI Board
> > > to avoid delays with SDCA support on Linux platforms.
> > >
> > > A block of 64 MBytes of register addresses are allocated to SDCA
> > > controls, starting at address 0x40000000. The 26 LSBs which identify
> > > individual controls are set based on the following variables:
> > >
> > > - Function Number. An SCDA device can be split in up to 8 independent
> > >   Functions. Each of these Functions is described in the SDCA
> > >   specification, e.g. Smart Amplifier, Smart Microphone, Simple
> > >   Microphone, Jack codec, HID, etc.
> > >
> > > - Entity Number.  Within each Function,  an Entity is  an identifiable
> > >   block.  Up   to  127  Entities   are  connected  in   a  pre-defined
> > >   graph  (similar to  USB), with  Entity0 reserved  for Function-level
> > >   configurations.  In  contrast  to  USB, the  SDCA  spec  pre-defines
> > >   Function Types, topologies, and allowed  options, i.e. the degree of
> > >   freedom  is not  unlimited to  limit  the possibility  of errors  in
> > >   descriptors leading to software quirks.
> > >
> > > - Control Selector. Within each Entity, the SDCA specification defines
> > >   48 controls such as Mute, Gain, AGC, etc, and 16 implementation
> > >   defined ones. Some Control Selectors might be used for low-level
> > >   platform setup, and other exposed to applications and users. Note
> > >   that the same Control Selector capability, e.g. Latency control,
> > >   might be located at different offsets in different entities, the
> > >   Control Selector mapping is Entity-specific.
> > >
> > > - Control Number. Some Control Selectors allow channel-specific values
> > >   to be set, with up to 64 channels allowed. This is mostly used for
> > >   volume control.
> > >
> > > - Current/Next values. Some Control Selectors are
> > >   'Dual-Ranked'. Software may either update the Current value directly
> > >   for immediate effect. Alternatively, software may write into the
> > >   'Next' values and update the SoundWire 1.2 'Commit Groups' register
> > >   to copy 'Next' values into 'Current' ones in a synchronized
> > >   manner. This is different from bank switching which is typically
> > >   used to change the bus configuration only.
> > >
> > > - MBQ. the Multi-Byte Quantity bit is used to provide atomic updates
> > >   when accessing more that one byte, for example a 16-bit volume
> > >   control would be updated consistently, the intermediate values
> > >   mixing old MSB with new LSB are not applied.
> > >
> > > These 6 parameters are used to build a 32-bit address to access the
> > > desired Controls. Because of address range, paging is required, but
> > > the most often used parameter values are placed in the lower 16 bits
> > > of the address. This helps to keep the paging registers constant while
> > > updating Controls for a specific Device/Function.
> > >
> > > Reviewed-by: Rander Wang <[e-mail]>
> > > Reviewed-by: Guennadi Liakhovetski
> > > <[e-mail]>
> > > Reviewed-by: Kai Vehmanen <[e-mail]>
> > > Signed-off-by: Pierre-Louis Bossart
> > > <[e-mail]>
> > > Signed-off-by: Bard Liao <[e-mail]>
> > > ---
> > > Changelog:
> > >
> > > v2:
> > >  - add SDW_SDCA_MBQ_CTL
> > >
> > > v3:
> > >  - add SDW_SDCA_NEXT_CTL
> > >
> > > ---
> > >  include/linux/soundwire/sdw_registers.h | 32
> > > +++++++++++++++++++++++++
> > >  1 file changed, 32 insertions(+)
> > >
> > > diff --git a/include/linux/soundwire/sdw_registers.h
> > > b/include/linux/soundwire/sdw_registers.h
> > > index f420e8059779..e14dff9a9c7f 100644
> > > --- a/include/linux/soundwire/sdw_registers.h
> > > +++ b/include/linux/soundwire/sdw_registers.h
> > > @@ -298,4 +298,36 @@
> > >  #define SDW_CASC_PORT_MASK_INTSTAT3		1
> > >  #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3	2
> > >
> > > +/*
> > > + * v1.2 device - SDCA address mapping
> > > + *
> > > + * Spec definition
> > > + *	Bits		Contents
> > > + *	31		0 (required by addressing range)
> > > + *	30:26		0b10000 (Control Prefix)
> > > + *	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 SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |
> > 		\
> > > +						 (((fun) & 0x7) << 22) |	\
> > > +						 (((ent) & 0x40) << 15) |	\
> > > +						 (((ent) & 0x3f) << 7) |	\
> > > +						 (((ctl) & 0x30) << 15) |	\
> > > +						 (((ctl) & 0x0f) << 3) |	\
> > > +						 (((ch) & 0x38) << 12) |	\
> > > +						 ((ch) & 0x07))
> > > +
> > > +#define SDW_SDCA_MBQ_CTL(reg)			((reg) | BIT(13))
> > > +#define SDW_SDCA_NEXT_CTL(reg)			((reg) | BIT(14))
> > > +
> > >  #endif /* __SDW_REGISTERS_H */
> > 
> > 
> > No users of these macros?
> 
> SDW_SDCA_CTL is used in sdca codec drivers which are not upstream yet.
> SDW_SDCA_MBQ_CTL will be used in a new regmap method.
> SDW_SDCA_NEXT_CTL can be used in sdca codec drivers, too.

Well, the point is that it's hard to review without seeing how the
code of actual users are.

BTW, the bit definitions can be simplified with GENMASK().
I personally don't think GENMASK() necessarily good, but it may fit
better in a case like this.


thanks,

Takashi

Greg KH Oct. 30, 2020, 11:56 a.m. UTC | #4

On Fri, Oct 30, 2020 at 11:24:35AM +0000, Liao, Bard wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH <[e-mail]>
> > Sent: Friday, October 30, 2020 5:37 PM
> > To: Bard Liao <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; pierre-
> > [e-mail]; Kale, Sanyog R <[e-mail]>; Lin,
> > Mengdong <[e-mail]>; Liao, Bard <[e-mail]>
> > Subject: Re: [PATCH v3] soundwire: SDCA: add helper macro to access
> > controls
> > 
> > On Fri, Oct 30, 2020 at 04:49:55AM +0800, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <[e-mail]>
> > >
> > > The upcoming SDCA (SoundWire Device Class Audio) specification defines
> > > a hierarchical encoding to interface with Class-defined capabilities.
> > >
> > > The specification is not yet accessible to the general public but this
> > > information is released with explicit permission from the MIPI Board
> > > to avoid delays with SDCA support on Linux platforms.
> > >
> > > A block of 64 MBytes of register addresses are allocated to SDCA
> > > controls, starting at address 0x40000000. The 26 LSBs which identify
> > > individual controls are set based on the following variables:
> > >
> > > - Function Number. An SCDA device can be split in up to 8 independent
> > >   Functions. Each of these Functions is described in the SDCA
> > >   specification, e.g. Smart Amplifier, Smart Microphone, Simple
> > >   Microphone, Jack codec, HID, etc.
> > >
> > > - Entity Number.  Within each Function,  an Entity is  an identifiable
> > >   block.  Up   to  127  Entities   are  connected  in   a  pre-defined
> > >   graph  (similar to  USB), with  Entity0 reserved  for Function-level
> > >   configurations.  In  contrast  to  USB, the  SDCA  spec  pre-defines
> > >   Function Types, topologies, and allowed  options, i.e. the degree of
> > >   freedom  is not  unlimited to  limit  the possibility  of errors  in
> > >   descriptors leading to software quirks.
> > >
> > > - Control Selector. Within each Entity, the SDCA specification defines
> > >   48 controls such as Mute, Gain, AGC, etc, and 16 implementation
> > >   defined ones. Some Control Selectors might be used for low-level
> > >   platform setup, and other exposed to applications and users. Note
> > >   that the same Control Selector capability, e.g. Latency control,
> > >   might be located at different offsets in different entities, the
> > >   Control Selector mapping is Entity-specific.
> > >
> > > - Control Number. Some Control Selectors allow channel-specific values
> > >   to be set, with up to 64 channels allowed. This is mostly used for
> > >   volume control.
> > >
> > > - Current/Next values. Some Control Selectors are
> > >   'Dual-Ranked'. Software may either update the Current value directly
> > >   for immediate effect. Alternatively, software may write into the
> > >   'Next' values and update the SoundWire 1.2 'Commit Groups' register
> > >   to copy 'Next' values into 'Current' ones in a synchronized
> > >   manner. This is different from bank switching which is typically
> > >   used to change the bus configuration only.
> > >
> > > - MBQ. the Multi-Byte Quantity bit is used to provide atomic updates
> > >   when accessing more that one byte, for example a 16-bit volume
> > >   control would be updated consistently, the intermediate values
> > >   mixing old MSB with new LSB are not applied.
> > >
> > > These 6 parameters are used to build a 32-bit address to access the
> > > desired Controls. Because of address range, paging is required, but
> > > the most often used parameter values are placed in the lower 16 bits
> > > of the address. This helps to keep the paging registers constant while
> > > updating Controls for a specific Device/Function.
> > >
> > > Reviewed-by: Rander Wang <[e-mail]>
> > > Reviewed-by: Guennadi Liakhovetski
> > > <[e-mail]>
> > > Reviewed-by: Kai Vehmanen <[e-mail]>
> > > Signed-off-by: Pierre-Louis Bossart
> > > <[e-mail]>
> > > Signed-off-by: Bard Liao <[e-mail]>
> > > ---
> > > Changelog:
> > >
> > > v2:
> > >  - add SDW_SDCA_MBQ_CTL
> > >
> > > v3:
> > >  - add SDW_SDCA_NEXT_CTL
> > >
> > > ---
> > >  include/linux/soundwire/sdw_registers.h | 32
> > > +++++++++++++++++++++++++
> > >  1 file changed, 32 insertions(+)
> > >
> > > diff --git a/include/linux/soundwire/sdw_registers.h
> > > b/include/linux/soundwire/sdw_registers.h
> > > index f420e8059779..e14dff9a9c7f 100644
> > > --- a/include/linux/soundwire/sdw_registers.h
> > > +++ b/include/linux/soundwire/sdw_registers.h
> > > @@ -298,4 +298,36 @@
> > >  #define SDW_CASC_PORT_MASK_INTSTAT3		1
> > >  #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3	2
> > >
> > > +/*
> > > + * v1.2 device - SDCA address mapping
> > > + *
> > > + * Spec definition
> > > + *	Bits		Contents
> > > + *	31		0 (required by addressing range)
> > > + *	30:26		0b10000 (Control Prefix)
> > > + *	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 SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |
> > 		\
> > > +						 (((fun) & 0x7) << 22) |	\
> > > +						 (((ent) & 0x40) << 15) |	\
> > > +						 (((ent) & 0x3f) << 7) |	\
> > > +						 (((ctl) & 0x30) << 15) |	\
> > > +						 (((ctl) & 0x0f) << 3) |	\
> > > +						 (((ch) & 0x38) << 12) |	\
> > > +						 ((ch) & 0x07))
> > > +
> > > +#define SDW_SDCA_MBQ_CTL(reg)			((reg) | BIT(13))
> > > +#define SDW_SDCA_NEXT_CTL(reg)			((reg) | BIT(14))
> > > +
> > >  #endif /* __SDW_REGISTERS_H */
> > 
> > 
> > No users of these macros?
> 
> SDW_SDCA_CTL is used in sdca codec drivers which are not upstream yet.
> SDW_SDCA_MBQ_CTL will be used in a new regmap method.
> SDW_SDCA_NEXT_CTL can be used in sdca codec drivers, too.

Then submit the users with the macro, otherwise it makes no sense to try
to review something that we do not see being used.

We do not add apis or code to the kernel that is not used, otherwise it
is impossible to maintain over time.

thanks,

greg k-h

Pierre-Louis Bossart Oct. 30, 2020, 3:52 p.m. UTC | #5

>>>> +#define SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |
>>> 		\
>>>> +						 (((fun) & 0x7) << 22) |	\
>>>> +						 (((ent) & 0x40) << 15) |	\
>>>> +						 (((ent) & 0x3f) << 7) |	\
>>>> +						 (((ctl) & 0x30) << 15) |	\
>>>> +						 (((ctl) & 0x0f) << 3) |	\
>>>> +						 (((ch) & 0x38) << 12) |	\
>>>> +						 ((ch) & 0x07))
>>>> +
>>>> +#define SDW_SDCA_MBQ_CTL(reg)			((reg) | BIT(13))
>>>> +#define SDW_SDCA_NEXT_CTL(reg)			((reg) | BIT(14))
>>>> +
>>>>   #endif /* __SDW_REGISTERS_H */
>>>
>>>
>>> No users of these macros?
>>
>> SDW_SDCA_CTL is used in sdca codec drivers which are not upstream yet.
>> SDW_SDCA_MBQ_CTL will be used in a new regmap method.
>> SDW_SDCA_NEXT_CTL can be used in sdca codec drivers, too.
> 
> Well, the point is that it's hard to review without seeing how the
> code of actual users are.

Agree, but our job is not made easy by the three-way dependency on 
regmap, SoundWire before we can submit ASoC codec drivers (developed by 
Realtek and tested by Intel).

If you prefer us to send all patches for SDCA codec support in one shot, 
that would be fine with us.
> BTW, the bit definitions can be simplified with GENMASK().
> I personally don't think GENMASK() necessarily good, but it may fit
> better in a case like this.

we use this macro in switch cases, e.g. for regmap properties to define 
read/volatile registers:

case SDW_SDCA_CTL(FUN_JACK_CODEC, RT711_SDCA_ENT_GE49, 
RT711_SDCA_CTL_SELECTED_MODE, 0):
	case SDW_SDCA_CTL(FUN_JACK_CODEC, RT711_SDCA_ENT_GE49, 
RT711_SDCA_CTL_DETECTED_MODE, 0):
	case SDW_SDCA_CTL(FUN_HID, RT711_SDCA_ENT_HID01, 
RT711_SDCA_CTL_HIDTX_CURRENT_OWNER, 0) ...
		SDW_SDCA_CTL(FUN_HID, RT711_SDCA_ENT_HID01, 
RT711_SDCA_CTL_HIDTX_MESSAGE_LENGTH, 0):
	case RT711_BUF_ADDR_HID1 ... RT711_BUF_ADDR_HID2:
		return true;

https://github.com/thesofproject/linux/blob/70fe32e776dafb4b03581d62a4569f65c2f13ada/sound/soc/codecs/rt711-sdca-sdw.c#L35

and unfortunately all our attempts to use FIELD_PREP, FIELD_GET, 
u32_encode, as suggested by Vinod, failed for this case due to 
compilation issues (can't use these macros outside of a function scope). 
The errors were shared with Vinod.

That's why we went back to the initial suggestion to deal with the 
shifts/masks by hand. For now we don't have a better solution that works 
in all cases were the macro is used.

Thanks
-Pierre

Takashi Iwai Oct. 30, 2020, 4:36 p.m. UTC | #6

On Fri, 30 Oct 2020 16:52:24 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> >>>> +#define SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |
> >>> 		\
> >>>> +						 (((fun) & 0x7) << 22) |	\
> >>>> +						 (((ent) & 0x40) << 15) |	\
> >>>> +						 (((ent) & 0x3f) << 7) |	\
> >>>> +						 (((ctl) & 0x30) << 15) |	\
> >>>> +						 (((ctl) & 0x0f) << 3) |	\
> >>>> +						 (((ch) & 0x38) << 12) |	\
> >>>> +						 ((ch) & 0x07))
> >>>> +
> >>>> +#define SDW_SDCA_MBQ_CTL(reg)			((reg) | BIT(13))
> >>>> +#define SDW_SDCA_NEXT_CTL(reg)			((reg) | BIT(14))
> >>>> +
> >>>>   #endif /* __SDW_REGISTERS_H */
> >>>
> >>>
> >>> No users of these macros?
> >>
> >> SDW_SDCA_CTL is used in sdca codec drivers which are not upstream yet.
> >> SDW_SDCA_MBQ_CTL will be used in a new regmap method.
> >> SDW_SDCA_NEXT_CTL can be used in sdca codec drivers, too.
> >
> > Well, the point is that it's hard to review without seeing how the
> > code of actual users are.
> 
> Agree, but our job is not made easy by the three-way dependency on
> regmap, SoundWire before we can submit ASoC codec drivers (developed
> by Realtek and tested by Intel).
> 
> If you prefer us to send all patches for SDCA codec support in one
> shot, that would be fine with us.

It's not necessarily mandatory to send the whole series, but if a
relevant code is already available, mentioning a repo URL in the patch
description (or in the comment below the delimiter) would be helpful,
for example.

> > BTW, the bit definitions can be simplified with GENMASK().
> > I personally don't think GENMASK() necessarily good, but it may fit
> > better in a case like this.
> 
> we use this macro in switch cases, e.g. for regmap properties to
> define read/volatile registers:
> 
> case SDW_SDCA_CTL(FUN_JACK_CODEC, RT711_SDCA_ENT_GE49,
> RT711_SDCA_CTL_SELECTED_MODE, 0):
> 	case SDW_SDCA_CTL(FUN_JACK_CODEC, RT711_SDCA_ENT_GE49,
> RT711_SDCA_CTL_DETECTED_MODE, 0):
> 	case SDW_SDCA_CTL(FUN_HID, RT711_SDCA_ENT_HID01,
> RT711_SDCA_CTL_HIDTX_CURRENT_OWNER, 0) ...
> 		SDW_SDCA_CTL(FUN_HID, RT711_SDCA_ENT_HID01,
> RT711_SDCA_CTL_HIDTX_MESSAGE_LENGTH, 0):
> 	case RT711_BUF_ADDR_HID1 ... RT711_BUF_ADDR_HID2:
> 		return true;
> 
> https://github.com/thesofproject/linux/blob/70fe32e776dafb4b03581d62a4569f65c2f13ada/sound/soc/codecs/rt711-sdca-sdw.c#L35
> 
> and unfortunately all our attempts to use FIELD_PREP, FIELD_GET,
> u32_encode, as suggested by Vinod, failed for this case due to
> compilation issues (can't use these macros outside of a function
> scope). The errors were shared with Vinod.
> 
> That's why we went back to the initial suggestion to deal with the
> shifts/masks by hand. For now we don't have a better solution that
> works in all cases were the macro is used.

Hrm, OK, in this case the value is masked then shifted, so it's not
trivial to deal with a macro.


thanks,

Takashi

Patch

diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h
index f420e8059779..e14dff9a9c7f 100644
--- a/include/linux/soundwire/sdw_registers.h
+++ b/include/linux/soundwire/sdw_registers.h
@@ -298,4 +298,36 @@ 
 #define SDW_CASC_PORT_MASK_INTSTAT3		1
 #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3	2
 
+/*
+ * v1.2 device - SDCA address mapping
+ *
+ * Spec definition
+ *	Bits		Contents
+ *	31		0 (required by addressing range)
+ *	30:26		0b10000 (Control Prefix)
+ *	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 SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |			\
+						 (((fun) & 0x7) << 22) |	\
+						 (((ent) & 0x40) << 15) |	\
+						 (((ent) & 0x3f) << 7) |	\
+						 (((ctl) & 0x30) << 15) |	\
+						 (((ctl) & 0x0f) << 3) |	\
+						 (((ch) & 0x38) << 12) |	\
+						 ((ch) & 0x07))
+
+#define SDW_SDCA_MBQ_CTL(reg)			((reg) | BIT(13))
+#define SDW_SDCA_NEXT_CTL(reg)			((reg) | BIT(14))
+
 #endif /* __SDW_REGISTERS_H */