[PATCH 1/7] PL330: Add common core driver\
Ben Dooks
ben-linux at fluff.org
Mon Apr 26 03:09:24 EDT 2010
On Mon, Apr 26, 2010 at 03:20:59PM +0900, jassi brar wrote:
> On Mon, Apr 26, 2010 at 1:54 PM, Ben Dooks <ben-linux at fluff.org> wrote:
> > On Wed, Apr 21, 2010 at 04:27:28PM +0900, jassisinghbrar at gmail.com wrote:
> >> From: Jassi Brar <jassi.brar at samsung.com>
>
> .....
>
> >> +
> >> +#define _FTC 0x40
> >> +#define FTC(n) (_FTC + (n)*0x4)
> >> +
> >> +#define _CS 0x100
> >> +#define CS(n) (_CS + (n)*0x8)
> >> +#define CS_CNS (1 << 21)
> >> +
> >> +#define _CPC 0x104
> >> +#define CPC(n) (_CPC + (n)*0x8)
> >> +
> >> +#define _SA 0x400
> >> +#define SA(n) (_SA + (n)*0x20)
> >> +
> >> +#define _DA 0x404
> >> +#define DA(n) (_DA + (n)*0x20)
> >> +
> >> +#define _CC 0x408
> >> +#define CC(n) (_CC + (n)*0x20)
> >> +
> >> +#define CC_SRCINC (1 << 0)
> >> +#define CC_DSTINC (1 << 14)
> >> +#define CC_SRCPRI (1 << 8)
> >> +#define CC_DSTPRI (1 << 22)
> >> +#define CC_SRCNS (1 << 9)
> >> +#define CC_DSTNS (1 << 23)
> >> +#define CC_SRCIA (1 << 10)
> >> +#define CC_DSTIA (1 << 24)
> >> +#define CC_SRCBRSTLEN_SHFT 4
> >> +#define CC_DSTBRSTLEN_SHFT 18
> >> +#define CC_SRCBRSTSIZE_SHFT 1
> >> +#define CC_DSTBRSTSIZE_SHFT 15
> >> +#define CC_SRCCCTRL_SHFT 11
> >> +#define CC_SRCCCTRL_MASK 0x7
> >> +#define CC_DSTCCTRL_SHFT 25
> >> +#define CC_DRCCCTRL_MASK 0x7
> >> +#define CC_SWAP_SHFT 28
> >> +
> >> +#define _LC0 0x40c
> >> +#define LC0(n) (_LC0 + (n)*0x20)
> >> +
> >> +#define _LC1 0x410
> >> +#define LC1(n) (_LC1 + (n)*0x20)
> >
> > do we really need the _ versions of each of these, or can
> > they be merged into thier non _ counterparts?
> Yes, they could be merged.
>
> .....
>
> >> +#define CR0_PERIPH_REQ_SET (1 << 0)
> >> +#define CR0_BOOT_EN_SET (1 << 1)
> >> +#define CR0_BOOT_MAN_NS (1 << 2)
> >> +#define CR0_NUM_CHANS_SHIFT 4
> >> +#define CR0_NUM_CHANS_MASK 0x7
> >> +#define CR0_NUM_PERIPH_SHIFT 12
> >> +#define CR0_NUM_PERIPH_MASK 0x1f
> >> +#define CR0_NUM_EVENTS_SHIFT 17
> >> +#define CR0_NUM_EVENTS_MASK 0x1f
> >> +
> >> +#define CR1_ICACHE_LEN_SHIFT 0
> >> +#define CR1_ICACHE_LEN_MASK 0x7
> >> +#define CR1_NUM_ICACHELINES_SHIFT 4
> >> +#define CR1_NUM_ICACHELINES_MASK 0xf
> >
> > are these aligned, or is it my mail client ignoring the formatting?
> They are aligned. Maybe it's your mail client.
Ok, hope to sort this out once the computer is fixed.
> ........
>
> >> +
> >> +/*
> >> + * Mark a _pl330_req as free.
> >> + * We do it by writing DMAEND as the first instruction
> >> + * because no valid request is going to have DMAEND as
> >> + * its first instruction to execute.
> >> + */
> >> +#define MARK_FREE(req) do { \
> >> + _emit_END(0, (req)->mc_cpu); \
> >> + (req)->mc_len = 0; \
> >> + } while (0)
> >
> > I think inline functions for these would be easier to read.
> you mean alignment that we see here? it's actually correctly
> aligned in an editor.
firstly, on the grounds it avoids the whole \
and secondly on the grounds inline functions get checked by the compiler
whether they're used or not.
I belive any macro over a couple of lines is probably doing too much
work.
> .....
>
> >> +struct _xfer_spec {
> >> + u32 ccr;
> >> + struct pl330_req *r;
> >> + struct pl330_xfer *x;
> >> +};
> >
> > would have been nice to kerneldoc this or at-least document it.
> It is totally an internal and temporary thing made only to have to
> not pass three args. Except for the func that use it, it doesn't matter
> to any other part.
>
> .....
>
> >> +static inline bool _queue_empty(struct pl330_thread *thrd)
> >> +{
> >> + return (IS_FREE(&thrd->req[0]) && IS_FREE(&thrd->req[1]))
> >> + ? true : false;
> >> +}
> >> +
> >> +static inline bool _queue_full(struct pl330_thread *thrd)
> >> +{
> >> + return (IS_FREE(&thrd->req[0]) || IS_FREE(&thrd->req[1]))
> >> + ? false : true;
> >> +}
> >
> > return !(IS_FREE(&thrd->req[0]) || IS_FREE(&thrd->req[1]) should
> > also work here.
> Of course.
> sometimes some people have an itch to return a 'true' or 'false'
> for a function that returns bool :)
ok, but that's one strange itch. you should go see a doctor.
> .....
>
> >> +
> >> +static inline u32 _emit_ADDH(unsigned dry_run, u8 buf[],
> >> + enum pl330_dst da, u16 val)
> >> +{
> >
> > think that should be u8 *buf.
> The 'buf' in all _emit_XXX functions is passed as an offset on
> dma consistent buffer and hence always contiguous.
> Since, it is easier to, and I do, manipulate that buffer like an
> array, they are taken as array.
> Or am i overlooking something?
u8 *ptr; ptr[x] is valid.
> .....
>
> >> +static inline u32 _emit_LD(unsigned dry_run, u8 buf[], enum pl330_cond cond)
> >> +{
> >> + if (dry_run)
> >> + return SZ_DMALD;
> >> +
> >> + buf[0] = CMD_DMALD;
> >> +
> >> + if (cond == SINGLE)
> >> + buf[0] |= (0 << 1) | (1 << 0);
> >> + else if (cond == BURST)
> >> + buf[0] |= (1 << 1) | (1 << 0);
> >
> > easir to merge CMD_DMALD into these to and just set buf[0] to the value
> > in the first place.
> Ok.
>
> ......
>
> >> + PL330_DBGCMD_DUMP(SZ_DMALD, "\tDMALD%c\n",
> >> + cond == SINGLE ? 'S' : (cond == BURST ? 'B' : 'A'));
> >> +
> >> + return SZ_DMALD;
> >> +}
> >> +
> >> +static inline u32 _emit_LDP(unsigned dry_run, u8 buf[],
> >> + enum pl330_cond cond, u8 peri)
> >> +{
> >> + if (dry_run)
> >> + return SZ_DMALDP;
> >> +
> >> + buf[0] = CMD_DMALDP;
> >> +
> >> + if (cond == BURST)
> >> + buf[0] |= (1 << 1);
> >> +
> >> + peri &= 0x1f;
> >> + peri <<= 3;
> >> + buf[1] = peri;
> >> +
> >> + PL330_DBGCMD_DUMP(SZ_DMALDP, "\tDMALDP%c %u\n",
> >> + cond == SINGLE ? 'S' : 'B', peri >> 3);
> >> +
> >> + return SZ_DMALDP;
> >> +}
> >> +
> >> +static inline u32 _emit_LP(unsigned dry_run, u8 buf[],
> >> + unsigned loop, u8 cnt)
> >> +{
> > could loop be a bool? dry_run certainly should be.
> I'd better do
>
> buf[0] |= (loop << 1);
ok, as long as you ensure loop is always 0 or 1. you could also
then merge in the original instruction.
> rather than
> if (loop)
> buf[0] |= (1 << 1);
>
> Yes, dry_run should be bool.
>
> .......
>
> >> + if (dry_run)
> >> + return SZ_DMALP;
> >
> > I'm just wondering if these shouldn't be if (!dry_run) { } return SZ_;\
> Just wanted to avoid another level of indent.
>
> ........
>
> >> + buf[0] = CMD_DMALP;
> >> +
> >> + if (loop)
> >> + buf[0] |= (1 << 1);
> >
> > again, you could probably do
> >
> > buf[0] = loop ? CMD_DMALP | (1 << 1) : CMD_DMALP;
> Since loop is the index, it'd better be
> buf[0] = CMD_DMALP | (loop << 1) ;
ok, seems sensible.
> .......
>
> >> +static inline u32 _emit_SEV(unsigned dry_run, u8 buf[], u8 ev)
> >> +{
> >> + if (dry_run)
> >> + return SZ_DMASEV;
> >> +
> >> + buf[0] = CMD_DMASEV;
> >> +
> >> + ev &= 0x1f;
> >> + ev <<= 3;
> >
> > is this some common op that should have some form of macro/inline defn?
> Strictly speaking, we could define a macro for preparing such mask for a few
> different instructions which happen to do the same thing.
I'm also wodnering if you could have a overall generate instruction with
this form of arguments, instead of having a whole pile of really similar
code.
> ......
>
> >> +static inline u32 _emit_GO(unsigned dry_run, u8 buf[],
> >> + const struct _arg_GO *arg)
> >> +{
> >> + u8 chan = arg->chan;
> >> + u32 addr = arg->addr;
> >> + unsigned ns = arg->ns;
> >> +
> >> + if (dry_run)
> >> + return SZ_DMAGO;
> >> +
> >> + buf[0] = CMD_DMAGO;
> >> + buf[0] |= (ns << 1);
> >
> > again, you could have probably merged these two into one.
> Ok
>
> ......
>
> >> +static inline u32 _state(struct pl330_thread *thrd)
> >> +{
> >> + void __iomem *regs = thrd->dmac->pinfo->base;
> >> + u32 val;
> >> +
> >> + if (is_manager(thrd))
> >> + val = readl(regs + DS) & 0xf;
> >> + else
> >> + val = readl(regs + CS(thrd->id)) & 0xf;
> >> +
> >> + switch (val) {
> >> + case DS_ST_STOP:
> >> + return PL330_STATE_STOPPED;
> >> + case DS_ST_EXEC:
> >> + return PL330_STATE_EXECUTING;
> >> + case DS_ST_CMISS:
> >> + return PL330_STATE_CACHEMISS;
> >> + case DS_ST_UPDTPC:
> >> + return PL330_STATE_UPDTPC;
> >> + case DS_ST_WFE:
> >> + return PL330_STATE_WFE;
> >> + case DS_ST_FAULT:
> >> + return PL330_STATE_FAULTING;
> >> + case DS_ST_ATBRR:
> >> + if (is_manager(thrd))
> >> + return PL330_STATE_INVALID;
> >
> > might be more code efficient to break the case into two
> > and have a is_manager() check between the two switches.
> ok
>
> ......
>
> >> +
> >> +/* If the request 'req' of thread 'thrd' is currently active */
> >> +static inline bool _req_active(struct pl330_thread *thrd,
> >> + struct _pl330_req *req)
> >> +{
> >> + void __iomem *regs = thrd->dmac->pinfo->base;
> >> + u32 buf = req->mc_bus, pc = readl(regs + CPC(thrd->id));
> >> +
> >> + if (IS_FREE(req))
> >> + return false;
> >> +
> >> + return (pc >= buf && pc <= buf + req->mc_len) ? true : false;
> >> +}
> >
> > hmm, doesn't linux bool allow just a
> > return (pc >= buf && pc <= buf + req->mc_len); ?
> remember my itch ? :)
brainscrub, isle 3.
> .....
>
> >> +updt_exit:
> >> + spin_unlock_irqrestore(&pl330->lock, flags);
> >> +
> >> + if (pl330->dmac_tbd.reset_dmac
> >> + || pl330->dmac_tbd.reset_mngr
> >> + || pl330->dmac_tbd.reset_chan) {
> >
> > iirc, style is:
> > if (a ||
> > b ||
> > c)
> >
> > could be wrong, definetly prefer the above.
> Ok, though there doesn't seem to be any hard-fast rule.
this is why I wasn't being definite.
> .......
>
> >> + if (!pl330->mcode_cpu) {
> >> + dev_err(pi->dev, "%s:%d Can't allocate memory!\n",
> >
> > no need to add an ! at the end of the line.
> Ok.
again, not a rule as such, just genrally accepted practice.
> ........
>
> >> +
> >> + /* Check if we can handle this DMAC */
> >> + if (get_id(pi, PERIPH_ID) != PERIPH_ID_VAL
> >> + || get_id(pi, PCELL_ID) != PCELL_ID_VAL) {
> >> + dev_err(pi->dev, "PERIPH_ID 0x%x, PCELL_ID 0x%x !\n",
> >> + readl(regs + PERIPH_ID), readl(regs + PCELL_ID));
> >
> > get_id and readl() ?
> should be only get_id
did seem wrong.
> > Also, please don;'t keep getting suprised by errors, erro messages don't
> > need a ! in them.
> ok
>
> ......
>
> >> +/* Populated by the PL330 core driver for DMA API driver's info */
> >> +struct pl330_config {
> >> + u32 periph_id;
> >> + u32 pcell_id;
> >> +#define DMAC_MODE_NS (1 << 0)
> >> + unsigned int mode;
> >> + unsigned int data_bus_width:10; /* In number of bits */
> >> + unsigned int data_buf_dep:10;
> >> + unsigned int num_chan:4;
> >> + unsigned int num_peri:6;
> >> + u32 peri_ns;
> >> + unsigned int num_events:6;
> >> + u32 irq_ns;
> >> +};
> >
> > If this is matching an in-memory structure, then please don't use :x to
> > define these. Also you'll need to ensure it is packed.
> >
> > If not matching an in memory structure, then unsigned char or u8s would
> > probably generate more efficient code without sacrificing a lot of memory
> > space.
> It is not matching any memory structure. I used fields only to stress the values
> they can take.
> Ok, will convert to u8
>
> .......
>
> >> + * The Client may want to provide this info only for the
> >> + * first request and a request with new settings.
> >> + */
> >> +struct pl330_reqcfg {
> >> + /* Address Incrementing */
> >> + unsigned dst_inc:1;
> >> + unsigned src_inc:1;
> >> +
> >> + /*
> >> + * For now, the SRC & DST protection levels
> >> + * and burst size/length are assumed same.
> >> + */
> >> + bool nonsecure;
> >> + bool privileged;
> >> + bool insnaccess;
> >> + unsigned brst_len:5;
> >> + unsigned brst_size:3; /* in power of 2 */
> >
> > see previous comments.
> ok
>
> Thanks.
good to see things moving along.
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
More information about the linux-arm-kernel
mailing list