On Sun, Dec 27, 2020 at 11:13:20AM -0500, Gabriel Somlo wrote:
> The 'litex_[set|get]_reg()' methods use the 'reg_size' parameter to
> specify the width of the LiteX CSR (MMIO) register being accessed.
>
> Since 'u64' is the widest data being supported, the value of 'reg_size'
> MUST be between 1 and sizeof(u64), which SHOULD be checked at runtime
> if these methods are publicly available for use by other LiteX device
> drivers.
>
> At the same time, none of the existing (or foreseeable) LiteX device
> drivers have a need to access registers whose size is unknown during
> compilation. As such, all LiteX device drivers should use fixed-width
> accessor methods such as 'litex_[write|read][8|16|32|64]()'.
>
> This patch renames 'litex_[set|get]_reg()' to '_litex_[set|get]_reg()',
> indicating that they should NOT be directly called from outside of
> the 'include/linux/litex.h' header file.
>
> Signed-off-by: Gabriel Somlo <gso...@gmail.com>
> ---
> drivers/soc/litex/Kconfig | 2 +-
> include/linux/litex.h | 35 ++++++++++++++++++++---------------
> 2 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> index 973f8d2fe1a7..b9b3d51ea7df 100644
> --- a/drivers/soc/litex/Kconfig
> +++ b/drivers/soc/litex/Kconfig
> @@ -11,7 +11,7 @@ config LITEX_SOC_CONTROLLER
> select LITEX
> help
> This option enables the SoC Controller Driver which verifies
> - LiteX CSR access and provides common litex_get_reg/litex_set_reg
> + LiteX CSR access and provides common litex_[read|write]*
> accessors.
> All drivers that use functions from litex.h must depend on
> LITEX.
> diff --git a/include/linux/litex.h b/include/linux/litex.h
> index 3456d527f644..14748fa86a99 100644
> --- a/include/linux/litex.h
> +++ b/include/linux/litex.h
> @@ -59,21 +59,25 @@ static inline u32 _read_litex_subregister(void __iomem
> *addr)
> ((off) + _litex_num_subregs(size) * LITEX_SUBREG_ALIGN)
>
> /*
> - * The purpose of `litex_set_reg`/`litex_get_reg` is to implement the logic
> - * of writing to/reading from the LiteX CSR in a single place that can be
> - * then reused by all LiteX drivers.
> + * The purpose of `_litex_[set|get]_reg()` is to implement the logic of
> + * writing to/reading from the LiteX CSR in a single place that can be then
> + * reused by all LiteX drivers via the `litex_[write|read][8|16|32|64]()`
> + * accessors for the appropriate data width.
> + * NOTE: direct use of `_litex_[set|get]_reg()` by LiteX drivers is strongly
> + * discouraged, as they perform no error checking on the requested data
> width!
> */
>
> /**
> - * litex_set_reg() - Writes the value to the LiteX CSR (Control&Status
> Register)
> + * _litex_set_reg() - Writes a value to the LiteX CSR (Control&Status
> Register)
> * @reg: Address of the CSR
> * @reg_size: The width of the CSR expressed in the number of bytes
> * @val: Value to be written to the CSR
> *
> * This function splits a single (possibly multi-byte) LiteX CSR write into
> * a series of subregister writes with a proper offset.
> + * NOTE: caller is responsible for ensuring (0 < reg_size < sizeof(u64)).
This will be fixed in v6 to read "(0 < reg_size <= sizeof(u64))".
> */
> -static inline void litex_set_reg(void __iomem *reg, size_t reg_size, u64 val)
> +static inline void _litex_set_reg(void __iomem *reg, size_t reg_size, u64
> val)
> {
> u8 shift = _litex_num_subregs(reg_size) * LITEX_SUBREG_SIZE_BIT;
>
> @@ -85,7 +89,7 @@ static inline void litex_set_reg(void __iomem *reg, size_t
> reg_size, u64 val)
> }
>
> /**
> - * litex_get_reg() - Reads the value of the LiteX CSR (Control&Status
> Register)
> + * _litex_get_reg() - Reads a value of the LiteX CSR (Control&Status
> Register)
> * @reg: Address of the CSR
> * @reg_size: The width of the CSR expressed in the number of bytes
> *
> @@ -93,8 +97,9 @@ static inline void litex_set_reg(void __iomem *reg, size_t
> reg_size, u64 val)
> *
> * This function generates a series of subregister reads with a proper offset
> * and joins their results into a single (possibly multi-byte) LiteX CSR
> value.
> + * NOTE: caller is responsible for ensuring (0 < reg_size < sizeof(u64)).
Ditto.
Thanks,
--Gabriel
> */
> -static inline u64 litex_get_reg(void __iomem *reg, size_t reg_size)
> +static inline u64 _litex_get_reg(void __iomem *reg, size_t reg_size)
> {
> u64 r;
> u8 i;
> @@ -110,42 +115,42 @@ static inline u64 litex_get_reg(void __iomem *reg,
> size_t reg_size)
>
> static inline void litex_write8(void __iomem *reg, u8 val)
> {
> - litex_set_reg(reg, sizeof(u8), val);
> + _litex_set_reg(reg, sizeof(u8), val);
> }
>
> static inline void litex_write16(void __iomem *reg, u16 val)
> {
> - litex_set_reg(reg, sizeof(u16), val);
> + _litex_set_reg(reg, sizeof(u16), val);
> }
>
> static inline void litex_write32(void __iomem *reg, u32 val)
> {
> - litex_set_reg(reg, sizeof(u32), val);
> + _litex_set_reg(reg, sizeof(u32), val);
> }
>
> static inline void litex_write64(void __iomem *reg, u64 val)
> {
> - litex_set_reg(reg, sizeof(u64), val);
> + _litex_set_reg(reg, sizeof(u64), val);
> }
>
> static inline u8 litex_read8(void __iomem *reg)
> {
> - return litex_get_reg(reg, sizeof(u8));
> + return _litex_get_reg(reg, sizeof(u8));
> }
>
> static inline u16 litex_read16(void __iomem *reg)
> {
> - return litex_get_reg(reg, sizeof(u16));
> + return _litex_get_reg(reg, sizeof(u16));
> }
>
> static inline u32 litex_read32(void __iomem *reg)
> {
> - return litex_get_reg(reg, sizeof(u32));
> + return _litex_get_reg(reg, sizeof(u32));
> }
>
> static inline u64 litex_read64(void __iomem *reg)
> {
> - return litex_get_reg(reg, sizeof(u64));
> + return _litex_get_reg(reg, sizeof(u64));
> }
>
> #endif /* _LINUX_LITEX_H */
> --
> 2.26.2
>
