Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1321562 > unrolled thread

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

Started byDaniel Kurtz <djkurtz@chromium.org>
First post2016-01-29 09:50 +0100
Last post2016-02-02 17:30 +0100
Articles 9 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver Daniel Kurtz <djkurtz@chromium.org> - 2016-01-29 09:50 +0100
    Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2016-01-29 13:30 +0100
      Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver Daniel Kurtz <djkurtz@chromium.org> - 2016-01-29 14:20 +0100
        Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2016-02-01 03:10 +0100
          Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver Daniel Kurtz <djkurtz@chromium.org> - 2016-02-01 05:20 +0100
            Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2016-02-01 07:30 +0100
              Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver Daniel Kurtz <djkurtz@chromium.org> - 2016-02-01 11:30 +0100
                Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver Horng-Shyang Liao <hs.liao@mediatek.com> - 2016-02-02 07:50 +0100
                  Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver Daniel Kurtz <djkurtz@chromium.org> - 2016-02-02 17:30 +0100

#1321562 — Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver

FromDaniel Kurtz <djkurtz@chromium.org>
Date2016-01-29 09:50 +0100
SubjectRe: [RFC 3/3] CMDQ: Mediatek CMDQ driver
Message-ID<qWcUG-90-5@gated-at.bofh.it>
On Fri, Jan 29, 2016 at 3:39 PM, Horng-Shyang Liao <hs.liao@mediatek.com> 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@mediatek.com> wrote:
>> > From: HS Liao <hs.liao@mediatek.com>
>> >
>> > 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@mediatek.com>
>>
>> [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]
>>
>> > +/*
>> > + * Maximum prefetch buffer size.
>> > + * Unit is instructions.
>> > + */
>> > +#define CMDQ_MAX_PREFETCH_INSTUCTION   240
>>
>> INSTRUCTION
>
> will fix
>
>> [snip]
>>
>> > +
>> > +/* get lsb for subsys encoding in arg_a (range: 0 - 31) */
>> > +
>> > +enum cmdq_eng {
>> > +       CMDQ_ENG_DISP_UFOE = 0,
>> > +       CMDQ_ENG_DISP_AAL,
>> > +       CMDQ_ENG_DISP_COLOR0,
>> > +       CMDQ_ENG_DISP_COLOR1,
>> > +       CMDQ_ENG_DISP_RDMA0,
>> > +       CMDQ_ENG_DISP_RDMA1,
>> > +       CMDQ_ENG_DISP_RDMA2,
>> > +       CMDQ_ENG_DISP_WDMA0,
>> > +       CMDQ_ENG_DISP_WDMA1,
>> > +       CMDQ_ENG_DISP_OVL0,
>> > +       CMDQ_ENG_DISP_OVL1,
>> > +       CMDQ_ENG_DISP_GAMMA,
>> > +       CMDQ_ENG_DISP_DSI0_CMD,
>> > +       CMDQ_ENG_DISP_DSI1_CMD,
>>
>> Why do these last two have "_CMD" at the end?
>
> will remove
>
>> > +       CMDQ_MAX_ENGINE_COUNT   /* ALWAYS keep at the end */
>> > +};
>> > +
>> > +struct cmdq_command {
>> > +       struct cmdq             *cqctx;
>> > +       u32                     scenario;
>> > +       /* task priority (NOT HW thread priority) */
>> > +       u32                     priority;
>> > +       /* bit flag of used engines */
>> > +       u64                     engine_flag;
>> > +       /*
>> > +        * pointer of instruction buffer
>> > +        * This must point to an 64-bit aligned u32 array
>> > +        */
>> > +       u32                     *va_base;
>>
>> All of your "va" and "va_base" should be void *, not u32 *.
>
> will do
>
>> > +       /* size of instruction buffer, in bytes. */
>> > +       u32                     block_size;
>>
>> Better to use size_t for "size in bytes".
>
> will fix
>
>> > +};
>> > +
>> > +enum cmdq_code {
>> > +       /* These are actual HW op code. */
>> > +       CMDQ_CODE_MOVE = 0x02,
>> > +       CMDQ_CODE_WRITE = 0x04,
>> > +       CMDQ_CODE_JUMP = 0x10,
>> > +       CMDQ_CODE_WFE = 0x20,   /* wait for event (and clear) */
>> > +       CMDQ_CODE_CLEAR_EVENT = 0x21,   /* clear event */
>> > +       CMDQ_CODE_EOC = 0x40,   /* end of command */
>> > +};
>> > +
>> > +enum cmdq_task_state {
>> > +       TASK_STATE_IDLE,        /* free task */
>> > +       TASK_STATE_BUSY,        /* task running on a thread */
>> > +       TASK_STATE_KILLED,      /* task process being killed */
>> > +       TASK_STATE_ERROR,       /* task execution error */
>> > +       TASK_STATE_DONE,        /* task finished */
>> > +       TASK_STATE_WAITING,     /* allocated but waiting for available thread */
>> > +};
>> > +
>> > +struct cmdq_cmd_buf {
>> > +       atomic_t                used;
>> > +       void                    *va;
>> > +       dma_addr_t              pa;
>> > +};
>> > +
>> > +struct cmdq_task_cb {
>> > +       /* called by isr */
>> > +       cmdq_async_flush_cb     isr_cb;
>> > +       void                    *isr_data;
>> > +       /* called by releasing task */
>> > +       cmdq_async_flush_cb     done_cb;
>> > +       void                    *done_data;
>> > +};
>> > +
>> > +struct cmdq_task {
>> > +       struct cmdq             *cqctx;
>> > +       struct list_head        list_entry;
>> > +
>> > +       /* state for task life cycle */
>> > +       enum cmdq_task_state    task_state;
>> > +       /* virtual address of command buffer */
>> > +       u32                     *va_base;
>> > +       /* physical address of command buffer */
>> > +       dma_addr_t              mva_base;
>> > +       /* size of allocated command buffer */
>> > +       u32                     buf_size;
>>
>> size_t
>
> will fix
>
>> > +       /* It points to a cmdq_cmd_buf if this task use command buffer pool. */
>> > +       struct cmdq_cmd_buf     *cmd_buf;
>> > +
>> > +       int                     scenario;
>> > +       int                     priority;
>> > +       u64                     engine_flag;
>> > +       u32                     command_size;
>> > +       u32                     num_cmd;
>> > +       int                     reorder;
>> > +       /* HW thread ID; CMDQ_INVALID_THREAD if not running */
>> > +       int                     thread;
>>
>> I think this driver will be a lot more clear if you do this:
>>
>> struct cmdq_thread *thread;
>>
>> And then use "NULL" for "invalid thread" instead of CMDQ_INVALID_THREAD.
>
> I will try to use cmdq_thread instead of thread id if possible.
> If CMDQ_INVALID_THREAD id is not necessary any more, I will remove it.
>
>> > +       /* flag of IRQ received */
>> > +       int                     irq_flag;
>> > +       /* callback functions */
>> > +       struct cmdq_task_cb     cb;
>> > +       /* work item when auto release is used */
>> > +       struct work_struct      auto_release_work;
>> > +
>> > +       unsigned long long      submit; /* submit time */
>> > +
>> > +       pid_t                   caller_pid;
>> > +       char                    caller_name[TASK_COMM_LEN];
>> > +};
>> > +
>> > +struct cmdq_thread {
>> > +       u32                     task_count;
>> > +       u32                     wait_cookie;
>> > +       u32                     next_cookie;
>> > +       struct cmdq_task        *cur_task[CMDQ_MAX_TASK_IN_THREAD];
>> > +};
>> > +
>> > +struct cmdq {
>> > +       struct device           *dev;
>> > +       struct notifier_block   pm_notifier;
>> > +
>> > +       void __iomem            *base_va;
>> > +       unsigned long           base_pa;
>>
>> I think we can remove base_pa (it is only used in a debug print), and just call
>> "base_va" as "base".
>
> will do
>
>> > +       u32                     irq;
>> > +
>> > +       /*
>> > +        * task information
>> > +        * task_cache: struct cmdq_task object cache
>> > +        * task_free_list: unused free tasks
>> > +        * task_active_list: active tasks
>> > +        * task_consume_wait_queue_item: task consumption work item
>> > +        * task_auto_release_wq: auto-release workqueue
>> > +        * task_consume_wq: task consumption workqueue (for queued tasks)
>> > +        */
>> > +       struct kmem_cache       *task_cache;
>> > +       struct list_head        task_free_list;
>> > +       struct list_head        task_active_list;
>> > +       struct list_head        task_wait_list;
>> > +       struct work_struct      task_consume_wait_queue_item;
>> > +       struct workqueue_struct *task_auto_release_wq;
>> > +       struct workqueue_struct *task_consume_wq;
>> > +       u16                     task_count[CMDQ_MAX_THREAD_COUNT];
>>
>> AFAICT, this task_count is not used?
>
> will remove
>
>> > +
>> > +       struct cmdq_thread      thread[CMDQ_MAX_THREAD_COUNT];
>> > +
>> > +       /* mutex, spinlock, flag */
>> > +       struct mutex            task_mutex;     /* for task list */
>> > +       struct mutex            clock_mutex;    /* for clock operation */
>> > +       spinlock_t              thread_lock;    /* for cmdq hardware thread */
>> > +       int                     thread_usage;
>> > +       spinlock_t              exec_lock;      /* for exec task */
>> > +
>> > +       /* suspend */
>> > +       bool                    suspended;
>> > +
>> > +       /* command buffer pool */
>> > +       struct cmdq_cmd_buf     cmd_buf_pool[CMDQ_CMD_BUF_POOL_BUF_NUM];
>> > +
>> > +       /*
>> > +        * notification
>> > +        * wait_queue: for task done
>> > +        * thread_dispatch_queue: for thread acquiring
>> > +        */
>> > +       wait_queue_head_t       wait_queue[CMDQ_MAX_THREAD_COUNT];
>> > +       wait_queue_head_t       thread_dispatch_queue;
>> > +
>> > +       /* ccf */
>> > +       struct clk              *clock;
>> > +};
>> > +
>> > +struct cmdq_event_name {
>> > +       enum cmdq_event event;
>> > +       char            *name;
>>
>> const char *
>
> will do
>
>> > +};
>> > +
>> > +struct cmdq_subsys {
>> > +       u32     base_addr;
>> > +       int     id;
>> > +       char    *grp_name;
>>
>> const char *
>
> will do
>
>> > +};
>> > +
>> > +static const struct cmdq_event_name g_event_name[] = {
>> > +       /* Display start of frame(SOF) events */
>> > +       {CMDQ_EVENT_DISP_OVL0_SOF, "CMDQ_EVENT_DISP_OVL0_SOF",},
>>
>> You can drop the "," inside "}".
>
> will do
>
>> > +       {CMDQ_EVENT_DISP_OVL1_SOF, "CMDQ_EVENT_DISP_OVL1_SOF",},
>> > +       {CMDQ_EVENT_DISP_RDMA0_SOF, "CMDQ_EVENT_DISP_RDMA0_SOF",},
>> > +       {CMDQ_EVENT_DISP_RDMA1_SOF, "CMDQ_EVENT_DISP_RDMA1_SOF",},
>> > +       {CMDQ_EVENT_DISP_RDMA2_SOF, "CMDQ_EVENT_DISP_RDMA2_SOF",},
>> > +       {CMDQ_EVENT_DISP_WDMA0_SOF, "CMDQ_EVENT_DISP_WDMA0_SOF",},
>> > +       {CMDQ_EVENT_DISP_WDMA1_SOF, "CMDQ_EVENT_DISP_WDMA1_SOF",},
>> > +       /* Display end of frame(EOF) events */
>> > +       {CMDQ_EVENT_DISP_OVL0_EOF, "CMDQ_EVENT_DISP_OVL0_EOF",},
>> > +       {CMDQ_EVENT_DISP_OVL1_EOF, "CMDQ_EVENT_DISP_OVL1_EOF",},
>> > +       {CMDQ_EVENT_DISP_RDMA0_EOF, "CMDQ_EVENT_DISP_RDMA0_EOF",},
>> > +       {CMDQ_EVENT_DISP_RDMA1_EOF, "CMDQ_EVENT_DISP_RDMA1_EOF",},
>> > +       {CMDQ_EVENT_DISP_RDMA2_EOF, "CMDQ_EVENT_DISP_RDMA2_EOF",},
>> > +       {CMDQ_EVENT_DISP_WDMA0_EOF, "CMDQ_EVENT_DISP_WDMA0_EOF",},
>> > +       {CMDQ_EVENT_DISP_WDMA1_EOF, "CMDQ_EVENT_DISP_WDMA1_EOF",},
>> > +       /* Mutex end of frame(EOF) events */
>> > +       {CMDQ_EVENT_MUTEX0_STREAM_EOF, "CMDQ_EVENT_MUTEX0_STREAM_EOF",},
>> > +       {CMDQ_EVENT_MUTEX1_STREAM_EOF, "CMDQ_EVENT_MUTEX1_STREAM_EOF",},
>> > +       {CMDQ_EVENT_MUTEX2_STREAM_EOF, "CMDQ_EVENT_MUTEX2_STREAM_EOF",},
>> > +       {CMDQ_EVENT_MUTEX3_STREAM_EOF, "CMDQ_EVENT_MUTEX3_STREAM_EOF",},
>> > +       {CMDQ_EVENT_MUTEX4_STREAM_EOF, "CMDQ_EVENT_MUTEX4_STREAM_EOF",},
>> > +       /* Display underrun events */
>> > +       {CMDQ_EVENT_DISP_RDMA0_UNDERRUN, "CMDQ_EVENT_DISP_RDMA0_UNDERRUN",},
>> > +       {CMDQ_EVENT_DISP_RDMA1_UNDERRUN, "CMDQ_EVENT_DISP_RDMA1_UNDERRUN",},
>> > +       {CMDQ_EVENT_DISP_RDMA2_UNDERRUN, "CMDQ_EVENT_DISP_RDMA2_UNDERRUN",},
>> > +       /* Keep this at the end of HW events */
>> > +       {CMDQ_MAX_HW_EVENT_COUNT, "CMDQ_MAX_HW_EVENT_COUNT",},
>> > +       /* GPR events */
>>
>> What are "GPR" events?
>
> They are events of General Purpose Register of GCE.
> I will remove them if driver doesn't need to take care of them.
>
>> > +       {CMDQ_SYNC_TOKEN_GPR_SET_0, "CMDQ_SYNC_TOKEN_GPR_SET_0",},
>> > +       {CMDQ_SYNC_TOKEN_GPR_SET_1, "CMDQ_SYNC_TOKEN_GPR_SET_1",},
>> > +       {CMDQ_SYNC_TOKEN_GPR_SET_2, "CMDQ_SYNC_TOKEN_GPR_SET_2",},
>> > +       {CMDQ_SYNC_TOKEN_GPR_SET_3, "CMDQ_SYNC_TOKEN_GPR_SET_3",},
>> > +       {CMDQ_SYNC_TOKEN_GPR_SET_4, "CMDQ_SYNC_TOKEN_GPR_SET_4",},
>> > +       /* This is max event and also can be used as mask. */
>> > +       {CMDQ_SYNC_TOKEN_MAX, "CMDQ_SYNC_TOKEN_MAX",},
>> > +       /* Invalid event */
>> > +       {CMDQ_SYNC_TOKEN_INVALID, "CMDQ_SYNC_TOKEN_INVALID",},
>> > +};
>> > +
>> > +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

[toc] | [next] | [standalone]

#1321722

FromHorng-Shyang Liao <hs.liao@mediatek.com>
Date2016-01-29 13:30 +0100
Message-ID<qWglz-2Vi-1@gated-at.bofh.it>
In reply to#1321562
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@mediatek.com> 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@mediatek.com> wrote:
> >> > From: HS Liao <hs.liao@mediatek.com>
> >> >
> >> > 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@mediatek.com>
> >>
> >> [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.

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

[toc] | [prev] | [next] | [standalone]

#1321744

FromDaniel Kurtz <djkurtz@chromium.org>
Date2016-01-29 14:20 +0100
Message-ID<qWh7Y-3Bi-5@gated-at.bofh.it>
In reply to#1321722
On Fri, Jan 29, 2016 at 8:24 PM, Horng-Shyang Liao <hs.liao@mediatek.com> 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@mediatek.com> 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@mediatek.com> wrote:
>> >> > From: HS Liao <hs.liao@mediatek.com>
>> >> >
>> >> > 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@mediatek.com>
>> >>
>> >> [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?
Does it vary 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
>

[toc] | [prev] | [next] | [standalone]

#1322778

FromHorng-Shyang Liao <hs.liao@mediatek.com>
Date2016-02-01 03:10 +0100
Message-ID<qXc6d-42e-1@gated-at.bofh.it>
In reply to#1321744
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@mediatek.com> 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@mediatek.com> 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@mediatek.com> wrote:
> >> >> > From: HS Liao <hs.liao@mediatek.com>
> >> >> >
> >> >> > 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@mediatek.com>
> >> >>
> >> >> [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

[toc] | [prev] | [next] | [standalone]

#1322817

FromDaniel Kurtz <djkurtz@chromium.org>
Date2016-02-01 05:20 +0100
Message-ID<qXe83-5vJ-5@gated-at.bofh.it>
In reply to#1322778
On Mon, Feb 1, 2016 at 10:04 AM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
>
> 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@mediatek.com> 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@mediatek.com> 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@mediatek.com> wrote:
> > >> >> > From: HS Liao <hs.liao@mediatek.com>
> > >> >> >
> > >> >> > 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@mediatek.com>
> > >> >>
> > >> >> [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?

Please answer the detailed technical questions:

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?

(a) Does the GCE remap a continuous device IO address range?

AFAICT, the  defines an MT8173 specific mapping of:

For example, the g_subsys table above seems to imply that the MT8173
gce maps all of:
  0x1400ffff:0x141fffff => 0x010000:0x1fffff

(b) Or, are the upper 5 bits of the "gce address" significant, and via
hardware it can map a disjoint groups of device addresses into the
continuous GCE address space, and really there are 0x1f distinct 64k
mappings:

mmsys (1) : 0x14000000:0x1400ffff => 0x010000:0x01ffff
disp  (2) : 0x14010000:0x1401ffff => 0x020000:0x02ffff
disp  (3) : 0x14020000:0x1402ffff => 0x030000:0x03ffff
...
???? (1f) : 0x141fffff:0x141fffff => 0x1f0000:0x1fffff

If the mapping is fixed and continuous (a), then I think all we need
is a single dts entry for the gce node that describes how it performs
this mapping.  And then, the gce consumers can just pass in their
regular physical addresses, and the gce driver can remap them directly
to gce addresses.

WDYT?

-Dan

>
> 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
>

[toc] | [prev] | [next] | [standalone]

#1322837

FromHorng-Shyang Liao <hs.liao@mediatek.com>
Date2016-02-01 07:30 +0100
Message-ID<qXg9Q-6Zg-5@gated-at.bofh.it>
In reply to#1322817
On Mon, 2016-02-01 at 12:15 +0800, Daniel Kurtz wrote:
> On Mon, Feb 1, 2016 at 10:04 AM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> >
> > 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@mediatek.com> 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@mediatek.com> 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@mediatek.com> wrote:
> > > >> >> > From: HS Liao <hs.liao@mediatek.com>
> > > >> >> >
> > > >> >> > 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@mediatek.com>
> > > >> >>
> > > >> >> [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?
> 
> Please answer the detailed technical questions:
> 
> How are these upper bits of the "GCE address" defined?

A GCE command is arg_a + arg_b. Both of them have 32 bits length.
arg_a is op + subsys + addr, and arg_b is value.
subsys + addr is less than 32bits, so we need to map address range to
subsys.
The mapping rule is defined by hardware.

> In other words, for a given SoC, how is the mapping between physical
> io addresses to GCE addresses defined?

It is (b).

> 
> (a) Does the GCE remap a continuous device IO address range?
> 
> AFAICT, the  defines an MT8173 specific mapping of:
> 
> For example, the g_subsys table above seems to imply that the MT8173
> gce maps all of:
>   0x1400ffff:0x141fffff => 0x010000:0x1fffff
> 
> (b) Or, are the upper 5 bits of the "gce address" significant, and via
> hardware it can map a disjoint groups of device addresses into the
> continuous GCE address space, and really there are 0x1f distinct 64k
> mappings:
> 
> mmsys (1) : 0x14000000:0x1400ffff => 0x010000:0x01ffff
> disp  (2) : 0x14010000:0x1401ffff => 0x020000:0x02ffff
> disp  (3) : 0x14020000:0x1402ffff => 0x030000:0x03ffff
> ...
> ???? (1f) : 0x141fffff:0x141fffff => 0x1f0000:0x1fffff
> 
> If the mapping is fixed and continuous (a), then I think all we need
> is a single dts entry for the gce node that describes how it performs
> this mapping.  And then, the gce consumers can just pass in their
> regular physical addresses, and the gce driver can remap them directly
> to gce addresses.
> 
> WDYT?

How about this?
hardware_module = <address_base subsys_id mask>;
So, the result is
mmsys_config_base = <0x14000000 1 0xffff0000>;
disp_rdma_config_base = <0x14010000 2 0xffff0000>;
disp_mutex_config_base = <0x14020000 3 0xffff0000>;

> -Dan
> 
> >
> > 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
> >

Thanks,
HS Liao

[toc] | [prev] | [next] | [standalone]

#1322965

FromDaniel Kurtz <djkurtz@chromium.org>
Date2016-02-01 11:30 +0100
Message-ID<qXjU6-19m-5@gated-at.bofh.it>
In reply to#1322837
On Mon, Feb 1, 2016 at 2:20 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> On Mon, 2016-02-01 at 12:15 +0800, Daniel Kurtz wrote:
>> On Mon, Feb 1, 2016 at 10:04 AM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
>> >
>> > 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@mediatek.com> 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@mediatek.com> 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@mediatek.com> wrote:
>> > > >> >> > From: HS Liao <hs.liao@mediatek.com>
>> > > >> >> >
>> > > >> >> > 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@mediatek.com>
>> > > >> >>
>> > > >> >> [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?
>>
>> Please answer the detailed technical questions:
>>
>> How are these upper bits of the "GCE address" defined?
>
> A GCE command is arg_a + arg_b. Both of them have 32 bits length.
> arg_a is op + subsys + addr, and arg_b is value.
> subsys + addr is less than 32bits, so we need to map address range to
> subsys.
> The mapping rule is defined by hardware.
>
>> In other words, for a given SoC, how is the mapping between physical
>> io addresses to GCE addresses defined?
>
> It is (b).
>
>>
>> (a) Does the GCE remap a continuous device IO address range?
>>
>> AFAICT, the  defines an MT8173 specific mapping of:
>>
>> For example, the g_subsys table above seems to imply that the MT8173
>> gce maps all of:
>>   0x1400ffff:0x141fffff => 0x010000:0x1fffff
>>
>> (b) Or, are the upper 5 bits of the "gce address" significant, and via
>> hardware it can map a disjoint groups of device addresses into the
>> continuous GCE address space, and really there are 0x1f distinct 64k
>> mappings:
>>
>> mmsys (1) : 0x14000000:0x1400ffff => 0x010000:0x01ffff
>> disp  (2) : 0x14010000:0x1401ffff => 0x020000:0x02ffff
>> disp  (3) : 0x14020000:0x1402ffff => 0x030000:0x03ffff
>> ...
>> ???? (1f) : 0x141fffff:0x141fffff => 0x1f0000:0x1fffff
>>
>> If the mapping is fixed and continuous (a), then I think all we need
>> is a single dts entry for the gce node that describes how it performs
>> this mapping.  And then, the gce consumers can just pass in their
>> regular physical addresses, and the gce driver can remap them directly
>> to gce addresses.
>>
>> WDYT?
>
> How about this?
> hardware_module = <address_base subsys_id mask>;
> So, the result is
> mmsys_config_base = <0x14000000 1 0xffff0000>;
> disp_rdma_config_base = <0x14010000 2 0xffff0000>;
> disp_mutex_config_base = <0x14020000 3 0xffff0000>;

What uses ID 0 and 4 - 0x1f?

According to mt8173.dtsi, here are the blocks in the address ranges above:

@1400:
  mmsys: clock-controller@14000000
  ovl0: ovl@1400c000
  ovl1: ovl@1400d000
  rdma0: rdma@1400e000
  rdma1: rdma@1400f000

@1401:
  rdma2: rdma@14010000
  wdma0: wdma@14011000
  wdma1: wdma@14012000
  color0: color@14013000
  color1: color@14014000
  aal@14015000
  gamma@14016000
  merge@14017000
  split0: split@14018000
  split1: split@14019000
  ufoe@1401a000
  dsi0: dsi@1401b000
  dsi1: dsi@1401c000
  dpi0: dpi@1401d000
  pwm0: pwm@1401e000
  pwm1: pwm@1401f000

@1402:
  mutex: mutex@14020000
  od@14023000
  larb0: larb@14021000
  smi_common: smi@14022000
  hdmi0: hdmi@14025000
  larb4: larb@14027000

I assume that the gce will work with any of the devices in those
ranges, not just "mmsys", "rdma" and "mutex", right?   (Also, notice
there are two "rdma" in the @1400 range, so rdma is really not a good
name for @1401)

Further, it looks like the gce just maps a large device address range
starting at 0x14000000 to (21-bit) gce address 0x010000, rather than
31 individually addressable 64k "subsys" blocks.  Is there a counter
example that I am missing?

-Dan

>
>> -Dan
>>
>> >
>> > 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
>> >
>
> Thanks,
> HS Liao
>

[toc] | [prev] | [next] | [standalone]

#1323836

FromHorng-Shyang Liao <hs.liao@mediatek.com>
Date2016-02-02 07:50 +0100
Message-ID<qXCWK-6RP-11@gated-at.bofh.it>
In reply to#1322965
On Mon, 2016-02-01 at 18:22 +0800, Daniel Kurtz wrote:
> On Mon, Feb 1, 2016 at 2:20 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> > On Mon, 2016-02-01 at 12:15 +0800, Daniel Kurtz wrote:
> >> On Mon, Feb 1, 2016 at 10:04 AM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> >> >
> >> > 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@mediatek.com> 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@mediatek.com> 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@mediatek.com> wrote:
> >> > > >> >> > From: HS Liao <hs.liao@mediatek.com>
> >> > > >> >> >
> >> > > >> >> > 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@mediatek.com>
> >> > > >> >>
> >> > > >> >> [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?
> >>
> >> Please answer the detailed technical questions:
> >>
> >> How are these upper bits of the "GCE address" defined?
> >
> > A GCE command is arg_a + arg_b. Both of them have 32 bits length.
> > arg_a is op + subsys + addr, and arg_b is value.
> > subsys + addr is less than 32bits, so we need to map address range to
> > subsys.
> > The mapping rule is defined by hardware.
> >
> >> In other words, for a given SoC, how is the mapping between physical
> >> io addresses to GCE addresses defined?
> >
> > It is (b).
> >
> >>
> >> (a) Does the GCE remap a continuous device IO address range?
> >>
> >> AFAICT, the  defines an MT8173 specific mapping of:
> >>
> >> For example, the g_subsys table above seems to imply that the MT8173
> >> gce maps all of:
> >>   0x1400ffff:0x141fffff => 0x010000:0x1fffff
> >>
> >> (b) Or, are the upper 5 bits of the "gce address" significant, and via
> >> hardware it can map a disjoint groups of device addresses into the
> >> continuous GCE address space, and really there are 0x1f distinct 64k
> >> mappings:
> >>
> >> mmsys (1) : 0x14000000:0x1400ffff => 0x010000:0x01ffff
> >> disp  (2) : 0x14010000:0x1401ffff => 0x020000:0x02ffff
> >> disp  (3) : 0x14020000:0x1402ffff => 0x030000:0x03ffff
> >> ...
> >> ???? (1f) : 0x141fffff:0x141fffff => 0x1f0000:0x1fffff
> >>
> >> If the mapping is fixed and continuous (a), then I think all we need
> >> is a single dts entry for the gce node that describes how it performs
> >> this mapping.  And then, the gce consumers can just pass in their
> >> regular physical addresses, and the gce driver can remap them directly
> >> to gce addresses.
> >>
> >> WDYT?
> >
> > How about this?
> > hardware_module = <address_base subsys_id mask>;
> > So, the result is
> > mmsys_config_base = <0x14000000 1 0xffff0000>;
> > disp_rdma_config_base = <0x14010000 2 0xffff0000>;
> > disp_mutex_config_base = <0x14020000 3 0xffff0000>;
> 
> What uses ID 0 and 4 - 0x1f?

Subsys is defined by GCE hardware, and other IDs are reserved currently.

> According to mt8173.dtsi, here are the blocks in the address ranges above:
> 
> @1400:
>   mmsys: clock-controller@14000000
>   ovl0: ovl@1400c000
>   ovl1: ovl@1400d000
>   rdma0: rdma@1400e000
>   rdma1: rdma@1400f000
> 
> @1401:
>   rdma2: rdma@14010000
>   wdma0: wdma@14011000
>   wdma1: wdma@14012000
>   color0: color@14013000
>   color1: color@14014000
>   aal@14015000
>   gamma@14016000
>   merge@14017000
>   split0: split@14018000
>   split1: split@14019000
>   ufoe@1401a000
>   dsi0: dsi@1401b000
>   dsi1: dsi@1401c000
>   dpi0: dpi@1401d000
>   pwm0: pwm@1401e000
>   pwm1: pwm@1401f000
> 
> @1402:
>   mutex: mutex@14020000
>   od@14023000
>   larb0: larb@14021000
>   smi_common: smi@14022000
>   hdmi0: hdmi@14025000
>   larb4: larb@14027000
> 
> I assume that the gce will work with any of the devices in those
> ranges, not just "mmsys", "rdma" and "mutex", right?   (Also, notice

That's right.

> there are two "rdma" in the @1400 range, so rdma is really not a good
> name for @1401)

I think we can just use index.
disp0_config_base = <0x14000000 1 0xffff0000>;
disp1_config_base = <0x14010000 2 0xffff0000>;
disp2_config_base = <0x14020000 3 0xffff0000>;

> Further, it looks like the gce just maps a large device address range
> starting at 0x14000000 to (21-bit) gce address 0x010000, rather than
> 31 individually addressable 64k "subsys" blocks.  Is there a counter
> example that I am missing?

From GCE's point of view,
it's 32 (0x0~0x1f) individually addressable 64k "subsys" blocks.
Currently, we don't have a counter example since all display related
address are put together.

> -Dan

Thanks,
HS Liao

> >
> >> -Dan
> >>
> >> >
> >> > 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
> >> >
> >
> > Thanks,
> > HS Liao
> >

[toc] | [prev] | [next] | [standalone]

#1324252

FromDaniel Kurtz <djkurtz@chromium.org>
Date2016-02-02 17:30 +0100
Message-ID<qXM02-5FW-17@gated-at.bofh.it>
In reply to#1323836
On Tue, Feb 2, 2016 at 2:48 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> On Mon, 2016-02-01 at 18:22 +0800, Daniel Kurtz wrote:
>> On Mon, Feb 1, 2016 at 2:20 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
>> > On Mon, 2016-02-01 at 12:15 +0800, Daniel Kurtz wrote:
>> >> On Mon, Feb 1, 2016 at 10:04 AM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
>> >> >
>> >> > 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@mediatek.com> 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@mediatek.com> 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@mediatek.com> wrote:
>> >> > > >> >> > From: HS Liao <hs.liao@mediatek.com>
>> >> > > >> >> >
>> >> > > >> >> > 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@mediatek.com>
>> >> > > >> >>
>> >> > > >> >> [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?
>> >>
>> >> Please answer the detailed technical questions:
>> >>
>> >> How are these upper bits of the "GCE address" defined?
>> >
>> > A GCE command is arg_a + arg_b. Both of them have 32 bits length.
>> > arg_a is op + subsys + addr, and arg_b is value.
>> > subsys + addr is less than 32bits, so we need to map address range to
>> > subsys.
>> > The mapping rule is defined by hardware.
>> >
>> >> In other words, for a given SoC, how is the mapping between physical
>> >> io addresses to GCE addresses defined?
>> >
>> > It is (b).
>> >
>> >>
>> >> (a) Does the GCE remap a continuous device IO address range?
>> >>
>> >> AFAICT, the  defines an MT8173 specific mapping of:
>> >>
>> >> For example, the g_subsys table above seems to imply that the MT8173
>> >> gce maps all of:
>> >>   0x1400ffff:0x141fffff => 0x010000:0x1fffff
>> >>
>> >> (b) Or, are the upper 5 bits of the "gce address" significant, and via
>> >> hardware it can map a disjoint groups of device addresses into the
>> >> continuous GCE address space, and really there are 0x1f distinct 64k
>> >> mappings:
>> >>
>> >> mmsys (1) : 0x14000000:0x1400ffff => 0x010000:0x01ffff
>> >> disp  (2) : 0x14010000:0x1401ffff => 0x020000:0x02ffff
>> >> disp  (3) : 0x14020000:0x1402ffff => 0x030000:0x03ffff
>> >> ...
>> >> ???? (1f) : 0x141fffff:0x141fffff => 0x1f0000:0x1fffff
>> >>
>> >> If the mapping is fixed and continuous (a), then I think all we need
>> >> is a single dts entry for the gce node that describes how it performs
>> >> this mapping.  And then, the gce consumers can just pass in their
>> >> regular physical addresses, and the gce driver can remap them directly
>> >> to gce addresses.
>> >>
>> >> WDYT?
>> >
>> > How about this?
>> > hardware_module = <address_base subsys_id mask>;
>> > So, the result is
>> > mmsys_config_base = <0x14000000 1 0xffff0000>;
>> > disp_rdma_config_base = <0x14010000 2 0xffff0000>;
>> > disp_mutex_config_base = <0x14020000 3 0xffff0000>;
>>
>> What uses ID 0 and 4 - 0x1f?
>
> Subsys is defined by GCE hardware, and other IDs are reserved currently.
>
>> According to mt8173.dtsi, here are the blocks in the address ranges above:
>>
>> @1400:
>>   mmsys: clock-controller@14000000
>>   ovl0: ovl@1400c000
>>   ovl1: ovl@1400d000
>>   rdma0: rdma@1400e000
>>   rdma1: rdma@1400f000
>>
>> @1401:
>>   rdma2: rdma@14010000
>>   wdma0: wdma@14011000
>>   wdma1: wdma@14012000
>>   color0: color@14013000
>>   color1: color@14014000
>>   aal@14015000
>>   gamma@14016000
>>   merge@14017000
>>   split0: split@14018000
>>   split1: split@14019000
>>   ufoe@1401a000
>>   dsi0: dsi@1401b000
>>   dsi1: dsi@1401c000
>>   dpi0: dpi@1401d000
>>   pwm0: pwm@1401e000
>>   pwm1: pwm@1401f000
>>
>> @1402:
>>   mutex: mutex@14020000
>>   od@14023000
>>   larb0: larb@14021000
>>   smi_common: smi@14022000
>>   hdmi0: hdmi@14025000
>>   larb4: larb@14027000
>>
>> I assume that the gce will work with any of the devices in those
>> ranges, not just "mmsys", "rdma" and "mutex", right?   (Also, notice
>
> That's right.
>
>> there are two "rdma" in the @1400 range, so rdma is really not a good
>> name for @1401)
>
> I think we can just use index.
> disp0_config_base = <0x14000000 1 0xffff0000>;
> disp1_config_base = <0x14010000 2 0xffff0000>;
> disp2_config_base = <0x14020000 3 0xffff0000>;
>
>> Further, it looks like the gce just maps a large device address range
>> starting at 0x14000000 to (21-bit) gce address 0x010000, rather than
>> 31 individually addressable 64k "subsys" blocks.  Is there a counter
>> example that I am missing?
>
> From GCE's point of view,
> it's 32 (0x0~0x1f) individually addressable 64k "subsys" blocks.
> Currently, we don't have a counter example since all display related
> address are put together.

Ok, in this case, perhaps we should treat the GCE like an IOMMU, and
have its binding define 32 slots or channels.
Then, any device that wishes to send the GCE commands for its address
range should register a phandle to the gce, including the
corresponding slot.

For example:

include/.../gce.h
include/dt-bindings/../mediatek-gce.h
#define GCE_SLOT_1  1
...

arch/arm64/boot/dts/mediatek/mt8173.dtsi:

&ovl0: {
  mediatek,gce = <&gce GCE_SLOT_1>;
};

&ovl1: {
  mediatek,gce = <&gce GCE_SLOT_1>;
};

&rdma2: {
  mediatek,gce = <&gce GCE_SLOT_2>;
};

&mutex: {
  mediatek,gce = <&gce GCE_SLOT_3>;
};

&od: {
  mediatek,gce = <&gce GCE_SLOT_3>;
};

Then, as each platform driver is probed, it can use the phandle to
look up its corresponding gce slot instance, retrieving an (opaque)
pointer to a struct gce_slot.
The gce driver can have a set of constant tables matching the slots to
address ranges for particular per-soc compatibles, one of which is
loaded on probe.
Later, when the device (gce consumer) wants to send a gce write
command, it passes in the gce_slot as an argument, and the gce driver
can do the corresponding lookup of subsys value and mask out the
provided *device virtual* address.  In this way, you also no longer
need to convert the devices iomap'ed addresses into physical addresses
before passing them to the gce.

WDYT?

-Dan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web