* [PATCH v2] drivers/soc/litex: remove 8-bit subregister option
@ 2021-05-26 10:51 Gabriel Somlo
2021-05-27 2:12 ` Joel Stanley
0 siblings, 1 reply; 4+ messages in thread
From: Gabriel Somlo @ 2021-05-26 10:51 UTC (permalink / raw)
To: linux-kernel
Cc: shorne, kgugala, mholenko, pczarnecki, davidgow, florent, joel
Since upstream LiteX recommends that Linux support be limited to
designs configured with 32-bit CSR subregisters (see commit a2b71fde
in upstream LiteX, https://github.com/enjoy-digital/litex), remove
the option to select 8-bit subregisters, significantly reducing the
complexity of LiteX CSR (MMIO register) accessor methods.
NOTE: for details on the underlying mechanics of LiteX CSR registers,
see https://github.com/enjoy-digital/litex/wiki/CSR-Bus or the original
LiteX accessors (litex/soc/software/include/hw/common.h in the upstream
repository).
Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
Cc: Stafford Horne <shorne@gmail.com>
Cc: Florent Kermarrec <florent@enjoy-digital.fr>
Cc: Mateusz Holenko <mholenko@antmicro.com>
Cc: Joel Stanley <joel@jms.id.au>
---
Changes since v1:
- remove LITEX_SUBREG_* macros as suggested by Stafford Horne
drivers/soc/litex/Kconfig | 12 ----
drivers/soc/litex/litex_soc_ctrl.c | 3 +-
include/linux/litex.h | 103 +++++------------------------
3 files changed, 16 insertions(+), 102 deletions(-)
diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
index e7011d665b15..e6ba3573a772 100644
--- a/drivers/soc/litex/Kconfig
+++ b/drivers/soc/litex/Kconfig
@@ -17,16 +17,4 @@ config LITEX_SOC_CONTROLLER
All drivers that use functions from litex.h must depend on
LITEX.
-config LITEX_SUBREG_SIZE
- int "Size of a LiteX CSR subregister, in bytes"
- depends on LITEX
- range 1 4
- default 4
- help
- LiteX MMIO registers (referred to as Configuration and Status
- registers, or CSRs) are spread across adjacent 8- or 32-bit
- subregisters, located at 32-bit aligned MMIO addresses. Use
- this to select the appropriate size (1 or 4 bytes) matching
- your particular LiteX build.
-
endmenu
diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
index c3e379a990f2..f75790091d38 100644
--- a/drivers/soc/litex/litex_soc_ctrl.c
+++ b/drivers/soc/litex/litex_soc_ctrl.c
@@ -62,8 +62,7 @@ static int litex_check_csr_access(void __iomem *reg_addr)
/* restore original value of the SCRATCH register */
litex_write32(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_VALUE);
- pr_info("LiteX SoC Controller driver initialized: subreg:%d, align:%d",
- LITEX_SUBREG_SIZE, LITEX_SUBREG_ALIGN);
+ pr_info("LiteX SoC Controller driver initialized");
return 0;
}
diff --git a/include/linux/litex.h b/include/linux/litex.h
index 5ea9ccf5cce4..f2edb86d5f44 100644
--- a/include/linux/litex.h
+++ b/include/linux/litex.h
@@ -11,18 +11,6 @@
#include <linux/io.h>
-/* LiteX SoCs support 8- or 32-bit CSR Bus data width (i.e., subreg. size) */
-#if defined(CONFIG_LITEX_SUBREG_SIZE) && \
- (CONFIG_LITEX_SUBREG_SIZE == 1 || CONFIG_LITEX_SUBREG_SIZE == 4)
-#define LITEX_SUBREG_SIZE CONFIG_LITEX_SUBREG_SIZE
-#else
-#error LiteX subregister size (LITEX_SUBREG_SIZE) must be 4 or 1!
-#endif
-#define LITEX_SUBREG_SIZE_BIT (LITEX_SUBREG_SIZE * 8)
-
-/* LiteX subregisters of any width are always aligned on a 4-byte boundary */
-#define LITEX_SUBREG_ALIGN 0x4
-
static inline void _write_litex_subregister(u32 val, void __iomem *addr)
{
writel((u32 __force)cpu_to_le32(val), addr);
@@ -42,115 +30,54 @@ static inline u32 _read_litex_subregister(void __iomem *addr)
* 32-bit wide logical CSR will be laid out as four 32-bit physical
* subregisters, each one containing one byte of meaningful data.
*
+ * For Linux support, upstream LiteX enforces a 32-bit wide CSR bus, which
+ * means that only larger-than-32-bit CSRs will be split across multiple
+ * subregisters (e.g., a 64-bit CSR will be spread across two consecutive
+ * 32-bit subregisters).
+ *
* For details see: https://github.com/enjoy-digital/litex/wiki/CSR-Bus
*/
-/* number of LiteX subregisters needed to store a register of given reg_size */
-#define _litex_num_subregs(reg_size) \
- (((reg_size) - 1) / LITEX_SUBREG_SIZE + 1)
-
-/*
- * since the number of 4-byte aligned subregisters required to store a single
- * LiteX CSR (MMIO) register varies with LITEX_SUBREG_SIZE, the offset of the
- * next adjacent LiteX CSR register w.r.t. the offset of the current one also
- * depends on how many subregisters the latter is spread across
- */
-#define _next_reg_off(off, size) \
- ((off) + _litex_num_subregs(size) * LITEX_SUBREG_ALIGN)
-
-/*
- * 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 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)).
- */
-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;
-
- while (shift > 0) {
- shift -= LITEX_SUBREG_SIZE_BIT;
- _write_litex_subregister(val >> shift, reg);
- reg += LITEX_SUBREG_ALIGN;
- }
-}
-
-/**
- * _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
- *
- * Return: Value read from the CSR
- *
- * 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)).
- */
-static inline u64 _litex_get_reg(void __iomem *reg, size_t reg_size)
-{
- u64 r;
- u8 i;
-
- r = _read_litex_subregister(reg);
- for (i = 1; i < _litex_num_subregs(reg_size); i++) {
- r <<= LITEX_SUBREG_SIZE_BIT;
- reg += LITEX_SUBREG_ALIGN;
- r |= _read_litex_subregister(reg);
- }
- return r;
-}
-
static inline void litex_write8(void __iomem *reg, u8 val)
{
- _litex_set_reg(reg, sizeof(u8), val);
+ _write_litex_subregister(val, reg);
}
static inline void litex_write16(void __iomem *reg, u16 val)
{
- _litex_set_reg(reg, sizeof(u16), val);
+ _write_litex_subregister(val, reg);
}
static inline void litex_write32(void __iomem *reg, u32 val)
{
- _litex_set_reg(reg, sizeof(u32), val);
+ _write_litex_subregister(val, reg);
}
static inline void litex_write64(void __iomem *reg, u64 val)
{
- _litex_set_reg(reg, sizeof(u64), val);
+ _write_litex_subregister(val >> 32, reg);
+ _write_litex_subregister(val, reg + 4);
}
static inline u8 litex_read8(void __iomem *reg)
{
- return _litex_get_reg(reg, sizeof(u8));
+ return _read_litex_subregister(reg);
}
static inline u16 litex_read16(void __iomem *reg)
{
- return _litex_get_reg(reg, sizeof(u16));
+ return _read_litex_subregister(reg);
}
static inline u32 litex_read32(void __iomem *reg)
{
- return _litex_get_reg(reg, sizeof(u32));
+ return _read_litex_subregister(reg);
}
static inline u64 litex_read64(void __iomem *reg)
{
- return _litex_get_reg(reg, sizeof(u64));
+ return ((u64)_read_litex_subregister(reg) << 32) |
+ _read_litex_subregister(reg + 4);
}
#endif /* _LINUX_LITEX_H */
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] drivers/soc/litex: remove 8-bit subregister option
2021-05-26 10:51 [PATCH v2] drivers/soc/litex: remove 8-bit subregister option Gabriel Somlo
@ 2021-05-27 2:12 ` Joel Stanley
2021-05-27 3:17 ` Gabriel L. Somlo
0 siblings, 1 reply; 4+ messages in thread
From: Joel Stanley @ 2021-05-27 2:12 UTC (permalink / raw)
To: Gabriel Somlo
Cc: Linux Kernel Mailing List, Stafford Horne, Karol Gugala,
Mateusz Holenko, pczarnecki, David Gow, Florent Kermarrec
On Wed, 26 May 2021 at 10:55, Gabriel Somlo <gsomlo@gmail.com> wrote:
>
> Since upstream LiteX recommends that Linux support be limited to
> designs configured with 32-bit CSR subregisters (see commit a2b71fde
> in upstream LiteX, https://github.com/enjoy-digital/litex), remove
> the option to select 8-bit subregisters, significantly reducing the
> complexity of LiteX CSR (MMIO register) accessor methods.
>
> NOTE: for details on the underlying mechanics of LiteX CSR registers,
> see https://github.com/enjoy-digital/litex/wiki/CSR-Bus or the original
> LiteX accessors (litex/soc/software/include/hw/common.h in the upstream
> repository).
>
> Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
I like how this simplifies things.
Reviewed-by: Joel Stanley <joel@jms.id.au>
With this change, is there any need to keep the litex acessors around?
There's the 64 bit case, but we don't have any 64 bit CSR registers do we?
> Cc: Stafford Horne <shorne@gmail.com>
> Cc: Florent Kermarrec <florent@enjoy-digital.fr>
> Cc: Mateusz Holenko <mholenko@antmicro.com>
> Cc: Joel Stanley <joel@jms.id.au>
> ---
> Changes since v1:
> - remove LITEX_SUBREG_* macros as suggested by Stafford Horne
>
> drivers/soc/litex/Kconfig | 12 ----
> drivers/soc/litex/litex_soc_ctrl.c | 3 +-
> include/linux/litex.h | 103 +++++------------------------
> 3 files changed, 16 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> index e7011d665b15..e6ba3573a772 100644
> --- a/drivers/soc/litex/Kconfig
> +++ b/drivers/soc/litex/Kconfig
> @@ -17,16 +17,4 @@ config LITEX_SOC_CONTROLLER
> All drivers that use functions from litex.h must depend on
> LITEX.
>
> -config LITEX_SUBREG_SIZE
> - int "Size of a LiteX CSR subregister, in bytes"
> - depends on LITEX
> - range 1 4
> - default 4
> - help
> - LiteX MMIO registers (referred to as Configuration and Status
> - registers, or CSRs) are spread across adjacent 8- or 32-bit
> - subregisters, located at 32-bit aligned MMIO addresses. Use
> - this to select the appropriate size (1 or 4 bytes) matching
> - your particular LiteX build.
> -
> endmenu
> diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> index c3e379a990f2..f75790091d38 100644
> --- a/drivers/soc/litex/litex_soc_ctrl.c
> +++ b/drivers/soc/litex/litex_soc_ctrl.c
> @@ -62,8 +62,7 @@ static int litex_check_csr_access(void __iomem *reg_addr)
> /* restore original value of the SCRATCH register */
> litex_write32(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_VALUE);
>
> - pr_info("LiteX SoC Controller driver initialized: subreg:%d, align:%d",
> - LITEX_SUBREG_SIZE, LITEX_SUBREG_ALIGN);
> + pr_info("LiteX SoC Controller driver initialized");
>
> return 0;
> }
> diff --git a/include/linux/litex.h b/include/linux/litex.h
> index 5ea9ccf5cce4..f2edb86d5f44 100644
> --- a/include/linux/litex.h
> +++ b/include/linux/litex.h
> @@ -11,18 +11,6 @@
>
> #include <linux/io.h>
>
> -/* LiteX SoCs support 8- or 32-bit CSR Bus data width (i.e., subreg. size) */
> -#if defined(CONFIG_LITEX_SUBREG_SIZE) && \
> - (CONFIG_LITEX_SUBREG_SIZE == 1 || CONFIG_LITEX_SUBREG_SIZE == 4)
> -#define LITEX_SUBREG_SIZE CONFIG_LITEX_SUBREG_SIZE
> -#else
> -#error LiteX subregister size (LITEX_SUBREG_SIZE) must be 4 or 1!
> -#endif
> -#define LITEX_SUBREG_SIZE_BIT (LITEX_SUBREG_SIZE * 8)
> -
> -/* LiteX subregisters of any width are always aligned on a 4-byte boundary */
> -#define LITEX_SUBREG_ALIGN 0x4
> -
> static inline void _write_litex_subregister(u32 val, void __iomem *addr)
> {
> writel((u32 __force)cpu_to_le32(val), addr);
> @@ -42,115 +30,54 @@ static inline u32 _read_litex_subregister(void __iomem *addr)
> * 32-bit wide logical CSR will be laid out as four 32-bit physical
> * subregisters, each one containing one byte of meaningful data.
> *
> + * For Linux support, upstream LiteX enforces a 32-bit wide CSR bus, which
> + * means that only larger-than-32-bit CSRs will be split across multiple
> + * subregisters (e.g., a 64-bit CSR will be spread across two consecutive
> + * 32-bit subregisters).
> + *
> * For details see: https://github.com/enjoy-digital/litex/wiki/CSR-Bus
> */
>
> -/* number of LiteX subregisters needed to store a register of given reg_size */
> -#define _litex_num_subregs(reg_size) \
> - (((reg_size) - 1) / LITEX_SUBREG_SIZE + 1)
> -
> -/*
> - * since the number of 4-byte aligned subregisters required to store a single
> - * LiteX CSR (MMIO) register varies with LITEX_SUBREG_SIZE, the offset of the
> - * next adjacent LiteX CSR register w.r.t. the offset of the current one also
> - * depends on how many subregisters the latter is spread across
> - */
> -#define _next_reg_off(off, size) \
> - ((off) + _litex_num_subregs(size) * LITEX_SUBREG_ALIGN)
> -
> -/*
> - * 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 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)).
> - */
> -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;
> -
> - while (shift > 0) {
> - shift -= LITEX_SUBREG_SIZE_BIT;
> - _write_litex_subregister(val >> shift, reg);
> - reg += LITEX_SUBREG_ALIGN;
> - }
> -}
> -
> -/**
> - * _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
> - *
> - * Return: Value read from the CSR
> - *
> - * 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)).
> - */
> -static inline u64 _litex_get_reg(void __iomem *reg, size_t reg_size)
> -{
> - u64 r;
> - u8 i;
> -
> - r = _read_litex_subregister(reg);
> - for (i = 1; i < _litex_num_subregs(reg_size); i++) {
> - r <<= LITEX_SUBREG_SIZE_BIT;
> - reg += LITEX_SUBREG_ALIGN;
> - r |= _read_litex_subregister(reg);
> - }
> - return r;
> -}
> -
> static inline void litex_write8(void __iomem *reg, u8 val)
> {
> - _litex_set_reg(reg, sizeof(u8), val);
> + _write_litex_subregister(val, reg);
> }
>
> static inline void litex_write16(void __iomem *reg, u16 val)
> {
> - _litex_set_reg(reg, sizeof(u16), val);
> + _write_litex_subregister(val, reg);
> }
>
> static inline void litex_write32(void __iomem *reg, u32 val)
> {
> - _litex_set_reg(reg, sizeof(u32), val);
> + _write_litex_subregister(val, reg);
> }
>
> static inline void litex_write64(void __iomem *reg, u64 val)
> {
> - _litex_set_reg(reg, sizeof(u64), val);
> + _write_litex_subregister(val >> 32, reg);
> + _write_litex_subregister(val, reg + 4);
> }
>
> static inline u8 litex_read8(void __iomem *reg)
> {
> - return _litex_get_reg(reg, sizeof(u8));
> + return _read_litex_subregister(reg);
> }
>
> static inline u16 litex_read16(void __iomem *reg)
> {
> - return _litex_get_reg(reg, sizeof(u16));
> + return _read_litex_subregister(reg);
> }
>
> static inline u32 litex_read32(void __iomem *reg)
> {
> - return _litex_get_reg(reg, sizeof(u32));
> + return _read_litex_subregister(reg);
> }
>
> static inline u64 litex_read64(void __iomem *reg)
> {
> - return _litex_get_reg(reg, sizeof(u64));
> + return ((u64)_read_litex_subregister(reg) << 32) |
> + _read_litex_subregister(reg + 4);
> }
>
> #endif /* _LINUX_LITEX_H */
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] drivers/soc/litex: remove 8-bit subregister option
2021-05-27 2:12 ` Joel Stanley
@ 2021-05-27 3:17 ` Gabriel L. Somlo
2021-06-10 19:45 ` Stafford Horne
0 siblings, 1 reply; 4+ messages in thread
From: Gabriel L. Somlo @ 2021-05-27 3:17 UTC (permalink / raw)
To: Joel Stanley
Cc: Linux Kernel Mailing List, Stafford Horne, Karol Gugala,
Mateusz Holenko, pczarnecki, David Gow, Florent Kermarrec
On Thu, May 27, 2021 at 02:12:40AM +0000, Joel Stanley wrote:
> On Wed, 26 May 2021 at 10:55, Gabriel Somlo <gsomlo@gmail.com> wrote:
> >
> > Since upstream LiteX recommends that Linux support be limited to
> > designs configured with 32-bit CSR subregisters (see commit a2b71fde
> > in upstream LiteX, https://github.com/enjoy-digital/litex), remove
> > the option to select 8-bit subregisters, significantly reducing the
> > complexity of LiteX CSR (MMIO register) accessor methods.
> >
> > NOTE: for details on the underlying mechanics of LiteX CSR registers,
> > see https://github.com/enjoy-digital/litex/wiki/CSR-Bus or the original
> > LiteX accessors (litex/soc/software/include/hw/common.h in the upstream
> > repository).
> >
> > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
>
> I like how this simplifies things.
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
Thanks!
> With this change, is there any need to keep the litex acessors around?
>
> There's the 64 bit case, but we don't have any 64 bit CSR registers do we?
Off the top of my head, the LiteSDcard driver uses 64-bit CSR writes:
https://github.com/litex-hub/linux/blob/litex-rebase/drivers/mmc/host/litex_mmc.c#L403
and
https://github.com/litex-hub/linux/blob/litex-rebase/drivers/mmc/host/litex_mmc.c#L421
These are both DMA base-address registers that support rv64*.
Thanks,
--Gabriel
> > Cc: Stafford Horne <shorne@gmail.com>
> > Cc: Florent Kermarrec <florent@enjoy-digital.fr>
> > Cc: Mateusz Holenko <mholenko@antmicro.com>
> > Cc: Joel Stanley <joel@jms.id.au>
> > ---
> > Changes since v1:
> > - remove LITEX_SUBREG_* macros as suggested by Stafford Horne
> >
> > drivers/soc/litex/Kconfig | 12 ----
> > drivers/soc/litex/litex_soc_ctrl.c | 3 +-
> > include/linux/litex.h | 103 +++++------------------------
> > 3 files changed, 16 insertions(+), 102 deletions(-)
> >
> > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> > index e7011d665b15..e6ba3573a772 100644
> > --- a/drivers/soc/litex/Kconfig
> > +++ b/drivers/soc/litex/Kconfig
> > @@ -17,16 +17,4 @@ config LITEX_SOC_CONTROLLER
> > All drivers that use functions from litex.h must depend on
> > LITEX.
> >
> > -config LITEX_SUBREG_SIZE
> > - int "Size of a LiteX CSR subregister, in bytes"
> > - depends on LITEX
> > - range 1 4
> > - default 4
> > - help
> > - LiteX MMIO registers (referred to as Configuration and Status
> > - registers, or CSRs) are spread across adjacent 8- or 32-bit
> > - subregisters, located at 32-bit aligned MMIO addresses. Use
> > - this to select the appropriate size (1 or 4 bytes) matching
> > - your particular LiteX build.
> > -
> > endmenu
> > diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> > index c3e379a990f2..f75790091d38 100644
> > --- a/drivers/soc/litex/litex_soc_ctrl.c
> > +++ b/drivers/soc/litex/litex_soc_ctrl.c
> > @@ -62,8 +62,7 @@ static int litex_check_csr_access(void __iomem *reg_addr)
> > /* restore original value of the SCRATCH register */
> > litex_write32(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_VALUE);
> >
> > - pr_info("LiteX SoC Controller driver initialized: subreg:%d, align:%d",
> > - LITEX_SUBREG_SIZE, LITEX_SUBREG_ALIGN);
> > + pr_info("LiteX SoC Controller driver initialized");
> >
> > return 0;
> > }
> > diff --git a/include/linux/litex.h b/include/linux/litex.h
> > index 5ea9ccf5cce4..f2edb86d5f44 100644
> > --- a/include/linux/litex.h
> > +++ b/include/linux/litex.h
> > @@ -11,18 +11,6 @@
> >
> > #include <linux/io.h>
> >
> > -/* LiteX SoCs support 8- or 32-bit CSR Bus data width (i.e., subreg. size) */
> > -#if defined(CONFIG_LITEX_SUBREG_SIZE) && \
> > - (CONFIG_LITEX_SUBREG_SIZE == 1 || CONFIG_LITEX_SUBREG_SIZE == 4)
> > -#define LITEX_SUBREG_SIZE CONFIG_LITEX_SUBREG_SIZE
> > -#else
> > -#error LiteX subregister size (LITEX_SUBREG_SIZE) must be 4 or 1!
> > -#endif
> > -#define LITEX_SUBREG_SIZE_BIT (LITEX_SUBREG_SIZE * 8)
> > -
> > -/* LiteX subregisters of any width are always aligned on a 4-byte boundary */
> > -#define LITEX_SUBREG_ALIGN 0x4
> > -
> > static inline void _write_litex_subregister(u32 val, void __iomem *addr)
> > {
> > writel((u32 __force)cpu_to_le32(val), addr);
> > @@ -42,115 +30,54 @@ static inline u32 _read_litex_subregister(void __iomem *addr)
> > * 32-bit wide logical CSR will be laid out as four 32-bit physical
> > * subregisters, each one containing one byte of meaningful data.
> > *
> > + * For Linux support, upstream LiteX enforces a 32-bit wide CSR bus, which
> > + * means that only larger-than-32-bit CSRs will be split across multiple
> > + * subregisters (e.g., a 64-bit CSR will be spread across two consecutive
> > + * 32-bit subregisters).
> > + *
> > * For details see: https://github.com/enjoy-digital/litex/wiki/CSR-Bus
> > */
> >
> > -/* number of LiteX subregisters needed to store a register of given reg_size */
> > -#define _litex_num_subregs(reg_size) \
> > - (((reg_size) - 1) / LITEX_SUBREG_SIZE + 1)
> > -
> > -/*
> > - * since the number of 4-byte aligned subregisters required to store a single
> > - * LiteX CSR (MMIO) register varies with LITEX_SUBREG_SIZE, the offset of the
> > - * next adjacent LiteX CSR register w.r.t. the offset of the current one also
> > - * depends on how many subregisters the latter is spread across
> > - */
> > -#define _next_reg_off(off, size) \
> > - ((off) + _litex_num_subregs(size) * LITEX_SUBREG_ALIGN)
> > -
> > -/*
> > - * 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 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)).
> > - */
> > -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;
> > -
> > - while (shift > 0) {
> > - shift -= LITEX_SUBREG_SIZE_BIT;
> > - _write_litex_subregister(val >> shift, reg);
> > - reg += LITEX_SUBREG_ALIGN;
> > - }
> > -}
> > -
> > -/**
> > - * _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
> > - *
> > - * Return: Value read from the CSR
> > - *
> > - * 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)).
> > - */
> > -static inline u64 _litex_get_reg(void __iomem *reg, size_t reg_size)
> > -{
> > - u64 r;
> > - u8 i;
> > -
> > - r = _read_litex_subregister(reg);
> > - for (i = 1; i < _litex_num_subregs(reg_size); i++) {
> > - r <<= LITEX_SUBREG_SIZE_BIT;
> > - reg += LITEX_SUBREG_ALIGN;
> > - r |= _read_litex_subregister(reg);
> > - }
> > - return r;
> > -}
> > -
> > static inline void litex_write8(void __iomem *reg, u8 val)
> > {
> > - _litex_set_reg(reg, sizeof(u8), val);
> > + _write_litex_subregister(val, reg);
> > }
> >
> > static inline void litex_write16(void __iomem *reg, u16 val)
> > {
> > - _litex_set_reg(reg, sizeof(u16), val);
> > + _write_litex_subregister(val, reg);
> > }
> >
> > static inline void litex_write32(void __iomem *reg, u32 val)
> > {
> > - _litex_set_reg(reg, sizeof(u32), val);
> > + _write_litex_subregister(val, reg);
> > }
> >
> > static inline void litex_write64(void __iomem *reg, u64 val)
> > {
> > - _litex_set_reg(reg, sizeof(u64), val);
> > + _write_litex_subregister(val >> 32, reg);
> > + _write_litex_subregister(val, reg + 4);
> > }
> >
> > static inline u8 litex_read8(void __iomem *reg)
> > {
> > - return _litex_get_reg(reg, sizeof(u8));
> > + return _read_litex_subregister(reg);
> > }
> >
> > static inline u16 litex_read16(void __iomem *reg)
> > {
> > - return _litex_get_reg(reg, sizeof(u16));
> > + return _read_litex_subregister(reg);
> > }
> >
> > static inline u32 litex_read32(void __iomem *reg)
> > {
> > - return _litex_get_reg(reg, sizeof(u32));
> > + return _read_litex_subregister(reg);
> > }
> >
> > static inline u64 litex_read64(void __iomem *reg)
> > {
> > - return _litex_get_reg(reg, sizeof(u64));
> > + return ((u64)_read_litex_subregister(reg) << 32) |
> > + _read_litex_subregister(reg + 4);
> > }
> >
> > #endif /* _LINUX_LITEX_H */
> > --
> > 2.31.1
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] drivers/soc/litex: remove 8-bit subregister option
2021-05-27 3:17 ` Gabriel L. Somlo
@ 2021-06-10 19:45 ` Stafford Horne
0 siblings, 0 replies; 4+ messages in thread
From: Stafford Horne @ 2021-06-10 19:45 UTC (permalink / raw)
To: Gabriel L. Somlo
Cc: Joel Stanley, Linux Kernel Mailing List, Karol Gugala,
Mateusz Holenko, pczarnecki, David Gow, Florent Kermarrec
On Wed, May 26, 2021 at 11:17:36PM -0400, Gabriel L. Somlo wrote:
> On Thu, May 27, 2021 at 02:12:40AM +0000, Joel Stanley wrote:
> > On Wed, 26 May 2021 at 10:55, Gabriel Somlo <gsomlo@gmail.com> wrote:
> > >
> > > Since upstream LiteX recommends that Linux support be limited to
> > > designs configured with 32-bit CSR subregisters (see commit a2b71fde
> > > in upstream LiteX, https://github.com/enjoy-digital/litex), remove
> > > the option to select 8-bit subregisters, significantly reducing the
> > > complexity of LiteX CSR (MMIO register) accessor methods.
> > >
> > > NOTE: for details on the underlying mechanics of LiteX CSR registers,
> > > see https://github.com/enjoy-digital/litex/wiki/CSR-Bus or the original
> > > LiteX accessors (litex/soc/software/include/hw/common.h in the upstream
> > > repository).
> > >
> > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>
> >
> > I like how this simplifies things.
> >
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> Thanks!
>
> > With this change, is there any need to keep the litex acessors around?
> >
> > There's the 64 bit case, but we don't have any 64 bit CSR registers do we?
>
> Off the top of my head, the LiteSDcard driver uses 64-bit CSR writes:
> https://github.com/litex-hub/linux/blob/litex-rebase/drivers/mmc/host/litex_mmc.c#L403
> and
> https://github.com/litex-hub/linux/blob/litex-rebase/drivers/mmc/host/litex_mmc.c#L421
>
> These are both DMA base-address registers that support rv64*.
>
I have pushed version to to for-next which should show up in linux-next today or
next week.
-Stafford
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-10 19:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-26 10:51 [PATCH v2] drivers/soc/litex: remove 8-bit subregister option Gabriel Somlo
2021-05-27 2:12 ` Joel Stanley
2021-05-27 3:17 ` Gabriel L. Somlo
2021-06-10 19:45 ` Stafford Horne
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).