| Message ID | 20210422140945.4131092-2-daniel@0x0f.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ARM: mstar: Internal bus madness | expand |
On Thu, Apr 22, 2021 at 4:11 PM Daniel Palmer <daniel@0x0f.com> wrote: > + > +#include <linux/io.h> > + > +static inline u32 riu_readl(__iomem void *base, unsigned int offset) The __iomem token comes after the type, so this should be 'void __iomem *'. > + return readw_relaxed(reg + 4) << 16 | readw_relaxed(reg); This should probably be using 'readw' instead of 'readw_relaxed'. If you absolutely need to use one of the relaxed accessors somewhere, better add both sets and make sure drivers use the non-relaxed version by default. Maybe both types of accessors can be in a single header. Arnd
Hi Arnd, On Fri, 23 Apr 2021 at 22:48, Arnd Bergmann <arnd@kernel.org> wrote: > > The __iomem token comes after the type, so this should be 'void __iomem *'. > Bit of copy/paste fail. Fixed. > > + return readw_relaxed(reg + 4) << 16 | readw_relaxed(reg); > > This should probably be using 'readw' instead of 'readw_relaxed'. If you > absolutely need to use one of the relaxed accessors somewhere, > better add both sets and make sure drivers use the non-relaxed version > by default. I'll add a relaxed/non-relaxed version of each. Because of the heavy memory barrier to access one 32 bit register we'll hit the barrier twice in the non-relaxed version. And we don't need to hit the barrier at all because it doesn't actually matter for IO. Is there something better I can do there? > Maybe both types of accessors can be in a single header. That makes sense. I'll merge them. Would this header be something that could go in alone without anything that uses them in mainline right now? Thanks, Daniel
On Fri, Apr 23, 2021 at 4:03 PM Daniel Palmer <daniel@0x0f.com> wrote: > On Fri, 23 Apr 2021 at 22:48, Arnd Bergmann <arnd@kernel.org> wrote: > > > > The __iomem token comes after the type, so this should be 'void __iomem *'. > > > > Bit of copy/paste fail. Fixed. > > > > + return readw_relaxed(reg + 4) << 16 | readw_relaxed(reg); > > > > This should probably be using 'readw' instead of 'readw_relaxed'. If you > > absolutely need to use one of the relaxed accessors somewhere, > > better add both sets and make sure drivers use the non-relaxed version > > by default. > > I'll add a relaxed/non-relaxed version of each. > Because of the heavy memory barrier to access one 32 bit register > we'll hit the barrier twice in the non-relaxed version. > And we don't need to hit the barrier at all because it doesn't > actually matter for IO. Is there something better I can do there? I think you can do the heavy barrier only once in this case. For writel, the barrier comes first, so you can do writel();write_relaxed(), and the reverse for the read side, doing readl_relaxed(); readl();. > > Maybe both types of accessors can be in a single header. > > That makes sense. I'll merge them. Would this header be something that > could go in alone without anything that uses them in mainline right > now? I don't care much, I can provide an Acked-by for merging it along with whatever driver change first needs it, or I can merge it after 5.13-rc1 through the soc tree. Arnd
diff --git a/MAINTAINERS b/MAINTAINERS index 19dc2eb0d93b..9600291e73a7 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2155,6 +2155,7 @@ F: drivers/gpio/gpio-msc313.c F: drivers/pinctrl/pinctrl-msc313.c F: include/dt-bindings/clock/mstar-* F: include/dt-bindings/gpio/msc313-gpio.h +F: include/soc/mstar/ ARM/NEC MOBILEPRO 900/c MACHINE SUPPORT M: Michael Petchkovsky <mkpetch@internode.on.net> diff --git a/include/soc/mstar/riu.h b/include/soc/mstar/riu.h new file mode 100644 index 000000000000..5aeea9c1e7eb --- /dev/null +++ b/include/soc/mstar/riu.h @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ + +#ifndef _SOC_MSTAR_RIU_H_ +#define _SOC_MSTAR_RIU_H_ + +#include <linux/io.h> + +static inline u32 riu_readl(__iomem void *base, unsigned int offset) +{ + __iomem void *reg = base + (offset * 2); + + return readw_relaxed(reg + 4) << 16 | readw_relaxed(reg); +} + +static inline void riu_writel(__iomem void *base, unsigned int offset, u32 value) +{ + __iomem void *reg = base + (offset * 2); + + /* + * Do not change this order. For EMAC at least + * the write order must be the lower half and then + * the upper half otherwise it doesn't work. + */ + writew_relaxed(value, reg); + writew_relaxed(value >> 16, reg + 4); +} + +#endif
Registers connected to the CPU via "RIU" (Maybe Register Interface Unit) are 16bits wide with a 32bit stride. For IPs that came from 3rd parties that have natively 32bit registers they are annoyingly mapped with the 32bit register split into two 16bit registers. This means that any existing driver (i.e. the usb and ethernet) cannot be used as is and needs to use a special readl()/writel() to fix up the address of the register that needs to be accessed, do two readw()/writel()s and stitch the values together. To avoid having this code in every driver add a header with an implementation of readl()/writel() that patches over the insanity. Signed-off-by: Daniel Palmer <daniel@0x0f.com> --- MAINTAINERS | 1 + include/soc/mstar/riu.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 include/soc/mstar/riu.h