Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver

[Horng-Shyang Liao]

On Fri, 2016-01-29 at 21:15 +0800, Daniel Kurtz wrote:
> On Fri, Jan 29, 2016 at 8:24 PM, Horng-Shyang Liao <hs.liao@xxxxxxxxxxxx> wrote:
> > On Fri, 2016-01-29 at 16:42 +0800, Daniel Kurtz wrote:
> >> On Fri, Jan 29, 2016 at 3:39 PM, Horng-Shyang Liao <hs.liao@xxxxxxxxxxxx> wrote:
> >> > Hi Dan,
> >> >
> >> > Many thanks for your comments and time.
> >> > I reply my plan inline.
> >> >
> >> >
> >> > On Thu, 2016-01-28 at 12:49 +0800, Daniel Kurtz wrote:
> >> >> Hi HS,
> >> >>
> >> >> Sorry for the delay.  It is hard to find time to review a >3700 line
> >> >> driver :-o in detail....
> >> >>
> >> >> Some review comments inline, although I still do not completely
> >> >> understand how all that this driver does and how it works.
> >> >> I'll try to find time to go through this driver in detail again next
> >> >> time you post it for review.
> >> >>
> >> >> On Tue, Jan 19, 2016 at 9:14 PM,  <hs.liao@xxxxxxxxxxxx> wrote:
> >> >> > From: HS Liao <hs.liao@xxxxxxxxxxxx>
> >> >> >
> >> >> > This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> >> >> > CMDQ is used to help read/write registers with critical time limitation,
> >> >> > such as updating display configuration during the vblank. It controls
> >> >> > Global Command Engine (GCE) hardware to achieve this requirement.
> >> >> > Currently, CMDQ only supports display related hardwares, but we expect
> >> >> > it can be extended to other hardwares for future requirements.
> >> >> >
> >> >> > Signed-off-by: HS Liao <hs.liao@xxxxxxxxxxxx>
> >> >>
> >> >> [snip]
> >> >>
> >> >> > diff --git a/drivers/soc/mediatek/mtk-cmdq.c b/drivers/soc/mediatek/mtk-cmdq.c
> >> >> > new file mode 100644
> >> >> > index 0000000..7570f00
> >> >> > --- /dev/null
> >> >> > +++ b/drivers/soc/mediatek/mtk-cmdq.c
> >
> > [snip]
> >
> >> >> > +static const struct cmdq_subsys g_subsys[] = {
> >> >> > +       {0x1400, 1, "MMSYS"},
> >> >> > +       {0x1401, 2, "DISP"},
> >> >> > +       {0x1402, 3, "DISP"},
> >> >>
> >> >> This isn't going to scale.  These addresses could be different on
> >> >> different chips.
> >> >> Instead of a static table like this, we probably need specify to the
> >> >> connection between gce and other devices via devicetree phandles, and
> >> >> then use the phandles to lookup the corresponding device address
> >> >> range.
> >> >
> >> > I will define them in device tree.
> >> > E.g.
> >> > cmdq {
> >> >   reg_domain = 0x14000000, 0x14010000, 0x14020000
> >> > }
> >>
> >> The devicetree should only model hardware relationships, not software
> >> considerations.
> >>
> >> Is the hardware constraint here for using gce with various other
> >> hardware blocks?  I think we already model this by only providing a
> >> gce phandle in the device tree nodes for those devices that can use
> >> gce.
> >>
> >> Looking at the driver closer, as far as I can tell, the whole subsys
> >> concept is a purely software abstraction, and only used to debug the
> >> CMDQ_CODE_WRITE command.  In fact, AFAICT, everything would work fine
> >> if we just completely removed the 'subsys' concept, and just passed
> >> through the raw address provided by the driver.
> >>
> >> So, I recommend just removing 'subsys' completely from the driver -
> >> from this array, and in the masks.
> >>
> >> Instead, if there is an error on the write command, just print the
> >> address that fails.  There are other ways to deduce the subsystem from
> >> a physical address.
> >>
> >> Thanks,
> >>
> >> -Dan
> >
> > Hi Dan,
> >
> > Subsys is not just for debug.
> > Its main purpose is to transfer CPU address to GCE address.
> > Let me explain it by "write" op,
> > I list a code segment from cmdq_rec_append_command().
> >
> >         case CMDQ_CODE_WRITE:
> >                 subsys = cmdq_subsys_from_phys_addr(cqctx, arg_a);
> >                 if (subsys < 0) {
> >                         dev_err(dev,
> >                                 "unsupported memory base address 0x%08x\n",
> >                                 arg_a);
> >                         return -EFAULT;
> >                 }
> >
> >                 *cmd_ptr++ = arg_b;
> >                 *cmd_ptr++ = (CMDQ_CODE_WRITE << CMDQ_OP_CODE_SHIFT) |
> >                              (arg_a & CMDQ_ARG_A_WRITE_MASK) |
> >                              ((subsys & CMDQ_SUBSYS_MASK) << CMDQ_SUBSYS_SHIFT);
> >                 break;
> >
> > Subsys is mapped from physical address via cmdq_subsys_from_phys_addr(),
> > and then it becomes part of GCE command via ((subsys & CMDQ_SUBSYS_MASK)
> > << CMDQ_SUBSYS_SHIFT) .
> > Only low bits of physical address are the same as GCE address.
> > We can get it by (arg_a & CMDQ_ARG_A_WRITE_MASK).
> > MASK is used to define how many bits are valid for this op.
> > So, GCE address = subsys + valid low bits.
> 
> How are these upper bits of the "GCE address" defined?
> In other words, for a given SoC, how is the mapping between physical
> io addresses to GCE addresses defined?
> Is this mapping fixed by hardware?

Yes, this mapping is fixed by hardware.

> Does it vary for different SoCs?

Yes, it varies for different SoCs.

> 
> -Dan
> 
> > That's why we need to know the mapping between the range of physical
> > address and subsys.
> > Please guide us a better way to code such requirement.
> > Thanks for your help.
> >
> > Thanks,
> > HS Liao
> >

Thanks,
HS Liao