From: Geert Uytterhoeven <geert@linux-m68k.org>
To: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: Mateusz Holenko <mholenko@antmicro.com>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jiri Slaby <jslaby@suse.com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
Stafford Horne <shorne@gmail.com>,
Karol Gugala <kgugala@antmicro.com>,
Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
"David S. Miller" <davem@davemloft.net>,
"Paul E. McKenney" <paulmck@linux.ibm.com>,
Filip Kokosinski <fkokosinski@antmicro.com>,
Pawel Czarnecki <pczarnecki@internships.antmicro.com>,
Joel Stanley <joel@jms.id.au>,
Jonathan Cameron <Jonathan.Cameron@huawei.com>,
Maxime Ripard <mripard@kernel.org>,
Shawn Guo <shawnguo@kernel.org>, Heiko Stuebner <heiko@sntech.de>,
Sam Ravnborg <sam@ravnborg.org>, Icenowy Zheng <icenowy@aosc.io>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Florent Kermarrec <f.kermarrec@gmail.com>
Subject: Re: [PATCH v11 3/5] drivers/soc/litex: add LiteX SoC Controller driver
Date: Wed, 30 Sep 2020 09:32:21 +0200 [thread overview]
Message-ID: <CAMuHMdXvimaMROGFsLwC6COU4EPGden7w58r+N8JHA93EB6=+Q@mail.gmail.com> (raw)
In-Reply-To: <20200925150607.GB470906@errol.ini.cmu.edu>
Hi Gabriel,
On Fri, Sep 25, 2020 at 5:06 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote:
> On Fri, Sep 25, 2020 at 03:16:02PM +0200, Geert Uytterhoeven wrote:
> > On Wed, Sep 23, 2020 at 12:10 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > > + */
> > > +#define LITEX_REG_SIZE 0x4
> > > +#define LITEX_SUBREG_SIZE 0x1
> > > +#define LITEX_SUBREG_SIZE_BIT (LITEX_SUBREG_SIZE * 8)
> > > +
> > > +static DEFINE_SPINLOCK(csr_lock);
> > > +
> > > +/*
> > > + * LiteX SoC Generator, depending on the configuration,
> > > + * can split a single logical CSR (Control & Status Register)
> > > + * into a series of consecutive physical registers.
> > > + *
> > > + * For example, in the configuration with 8-bit CSR Bus,
> > > + * 32-bit aligned (the default one for 32-bit CPUs) a 32-bit
> > > + * logical CSR will be generated as four 32-bit physical registers,
> > > + * each one containing one byte of meaningful data.
> > > + *
> > > + * For details see: https://github.com/enjoy-digital/litex/wiki/CSR-Bus
> > > + *
> > > + * 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.
> > > + */
> > > +void litex_set_reg(void __iomem *reg, unsigned long reg_size,
> > > + unsigned long val)
> > > +{
> > > + unsigned long shifted_data, shift, i;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&csr_lock, flags);
> > > +
> > > + for (i = 0; i < reg_size; ++i) {
> > > + shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> > > + shifted_data = val >> shift;
> > > +
> > > + writel((u32 __force)cpu_to_le32(shifted_data), reg + (LITEX_REG_SIZE * i));
> > > + }
> > > +
> > > + spin_unlock_irqrestore(&csr_lock, flags);
> > > +}
> > > +EXPORT_SYMBOL_GPL(litex_set_reg);
> >
> > I'm still wondering about the overhead of loops and multiple accesses,
> > and the need for them (see also BenH's earlier comment).
> > If e.g. the register widths change for LiteUART (currently they're
> > hardcoded to one), would you still consider it using the same
> > programming interface, and thus compatible with "litex,liteuart"?
>
> There's been talk within the LiteX dev community to standardize on a
> LITEX_SUBREG_SIZE of 0x4 (i.e., using all 32 bits of a 32-bit
> (LITEX_REG_SIZE) aligned MMIO location). Early 32-bit (vexriscv based)
> Linux capable LiteX designs started out with only the 8 LSBits used
> within a 32-bit MMIO location, but 64-bit (Rocket chip) based LiteX SoCs
> use 4-byte aligned, fully populated MMIO registers (i.e., both
> LITEX_SUBREG_SIZE *and* LITEX_REG_SIZE are 4). There's also been talk of
> deprecating LITEX_SUBREG_SIZE == 0x1 for "linux-capable LiteX builds",
> but nothing definitive yet AFAIK.
That sounds like a good idea to me.
Having 8-bit accesses may be worthwhile on a small microcontroller, but a
full-fledge Linux system can use more and wider MMIO.
> Geert: note that LiteX has wider-than-32-bit registers spread across
> multiple 32-bit aligned, 8- or 32-bit wide "subregisters", so looping
> and shifting will still be necessary, even with LITEX_SUBREG_SIZE 0x4.
Can these be different than 64-bit (and 128-bit)?
That's not unlike accessors on other 32-bit platforms.
Still, no loop needed, just doing two (or four) 32-bit accesses in a row
is fine (but requires using inlines instead of your current single
out-of-line function).
> > > --- /dev/null
> > > +++ b/include/linux/litex.h
> > > +void litex_set_reg(void __iomem *reg, unsigned long reg_sz, unsigned long val);
> > > +
> > > +unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_sz);
> >
> > Perhaps you can add static inline litex_{read,write}{8,16,32}() wrappers,
> > so drivers don't have to pass the reg_sz parameter explicitly,
> > and to make it look more like accessors of other bus types?
>
> Seconded -- perhaps simply cut'n'paste and/or adapt from
> https://github.com/litex-hub/linux/blob/litex-rocket-rebase/include/linux/litex.h#L78
> (from the 64-bit port of the LiteX linux patch set)
Yes, you definitely want the 32-bit and 64-bit ports to agree ;-)
Note that these are using the "old" "bwlq" convention (with "l"
predating 64-bit long on 64-bit platforms) instead of the more modern
explicit {8,16,32,64}, but that's a minor detail.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
next prev parent reply other threads:[~2020-09-30 7:32 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-23 10:08 [PATCH v11 0/5] LiteX SoC controller and LiteUART serial driver Mateusz Holenko
2020-09-23 10:08 ` [PATCH v11 1/5] dt-bindings: vendor: add vendor prefix for LiteX Mateusz Holenko
2020-09-23 10:08 ` [PATCH v11 2/5] dt-bindings: soc: document LiteX SoC Controller bindings Mateusz Holenko
2020-09-25 12:48 ` Geert Uytterhoeven
2020-10-06 6:30 ` Mateusz Holenko
2020-09-23 10:09 ` [PATCH v11 3/5] drivers/soc/litex: add LiteX SoC Controller driver Mateusz Holenko
2020-09-23 11:56 ` Jonathan Cameron
2020-10-06 6:34 ` Mateusz Holenko
2020-09-25 13:16 ` Geert Uytterhoeven
2020-09-25 15:06 ` Gabriel L. Somlo
2020-09-30 7:32 ` Geert Uytterhoeven [this message]
2020-10-06 10:07 ` Mateusz Holenko
2020-10-06 8:02 ` Mateusz Holenko
2020-10-06 8:38 ` Geert Uytterhoeven
2020-10-06 13:29 ` Mateusz Holenko
2020-09-23 10:09 ` [PATCH v11 4/5] dt-bindings: serial: document LiteUART bindings Mateusz Holenko
2020-09-25 13:16 ` Geert Uytterhoeven
2020-10-06 7:00 ` Mateusz Holenko
2020-10-06 7:07 ` Geert Uytterhoeven
2020-09-23 10:09 ` [PATCH v11 5/5] drivers/tty/serial: add LiteUART driver Mateusz Holenko
2020-09-25 13:40 ` Geert Uytterhoeven
2020-10-06 7:11 ` Mateusz Holenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMuHMdXvimaMROGFsLwC6COU4EPGden7w58r+N8JHA93EB6=+Q@mail.gmail.com' \
--to=geert@linux-m68k.org \
--cc=Jonathan.Cameron@huawei.com \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=f.kermarrec@gmail.com \
--cc=fkokosinski@antmicro.com \
--cc=gregkh@linuxfoundation.org \
--cc=gsomlo@gmail.com \
--cc=heiko@sntech.de \
--cc=icenowy@aosc.io \
--cc=joel@jms.id.au \
--cc=jslaby@suse.com \
--cc=kgugala@antmicro.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mchehab+samsung@kernel.org \
--cc=mholenko@antmicro.com \
--cc=mripard@kernel.org \
--cc=paulmck@linux.ibm.com \
--cc=pczarnecki@internships.antmicro.com \
--cc=robh+dt@kernel.org \
--cc=sam@ravnborg.org \
--cc=shawnguo@kernel.org \
--cc=shorne@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).