From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753879AbcBBGsK (ORCPT ); Tue, 2 Feb 2016 01:48:10 -0500 Received: from mailgw02.mediatek.com ([210.61.82.184]:2390 "EHLO mailgw02.hq.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751464AbcBBGsI (ORCPT ); Tue, 2 Feb 2016 01:48:08 -0500 Message-ID: <1454395681.27603.13.camel@mtksdaap41> Subject: Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver From: Horng-Shyang Liao To: Daniel Kurtz CC: Rob Herring , Matthias Brugger , Sascha Hauer , "open list:OPEN FIRMWARE AND..." , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "moderated list:ARM/Mediatek SoC support" , srv_heupstream , Sascha Hauer , Philipp Zabel , Nicolas Boichat , CK HU , cawa cheng , Bibby Hsieh , YT Shen , Daoyuan Huang , "Damon Chu" , Josh-YC Liu , "Glory Hung" , Yong Wu Date: Tue, 2 Feb 2016 14:48:01 +0800 In-Reply-To: References: <1453266881-16849-1-git-send-email-hs.liao@mediatek.com> <1453266881-16849-4-git-send-email-hs.liao@mediatek.com> <1454053150.32413.52.camel@mtksdaap41> <1454070262.7401.21.camel@mtksdaap41> <1454292257.22270.3.camel@mtksdaap41> <1454307624.22441.18.camel@mtksdaap41> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2016-02-01 at 18:22 +0800, Daniel Kurtz wrote: > On Mon, Feb 1, 2016 at 2:20 PM, Horng-Shyang Liao 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 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 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 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, wrote: > >> > > >> >> > From: HS Liao > >> > > >> >> > > >> > > >> >> > 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 > >> > > >> >> > >> > > >> >> [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 = ; > > 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 > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Horng-Shyang Liao Subject: Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver Date: Tue, 2 Feb 2016 14:48:01 +0800 Message-ID: <1454395681.27603.13.camel@mtksdaap41> References: <1453266881-16849-1-git-send-email-hs.liao@mediatek.com> <1453266881-16849-4-git-send-email-hs.liao@mediatek.com> <1454053150.32413.52.camel@mtksdaap41> <1454070262.7401.21.camel@mtksdaap41> <1454292257.22270.3.camel@mtksdaap41> <1454307624.22441.18.camel@mtksdaap41> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Daniel Kurtz Cc: Rob Herring , Matthias Brugger , Sascha Hauer , "open list:OPEN FIRMWARE AND..." , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "moderated list:ARM/Mediatek SoC support" , srv_heupstream , Sascha Hauer , Philipp Zabel , Nicolas Boichat , CK HU , cawa cheng , Bibby Hsieh , YT Shen , Daoyuan Huang , Damon Chu , Josh-YC Liu , Glory Hung , Yong List-Id: devicetree@vger.kernel.org On Mon, 2016-02-01 at 18:22 +0800, Daniel Kurtz wrote: > On Mon, Feb 1, 2016 at 2:20 PM, Horng-Shyang Liao 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 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 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 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, wrote: > >> > > >> >> > From: HS Liao > >> > > >> >> > > >> > > >> >> > 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 > >> > > >> >> > >> > > >> >> [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 = ; > > 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 > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: hs.liao@mediatek.com (Horng-Shyang Liao) Date: Tue, 2 Feb 2016 14:48:01 +0800 Subject: [RFC 3/3] CMDQ: Mediatek CMDQ driver In-Reply-To: References: <1453266881-16849-1-git-send-email-hs.liao@mediatek.com> <1453266881-16849-4-git-send-email-hs.liao@mediatek.com> <1454053150.32413.52.camel@mtksdaap41> <1454070262.7401.21.camel@mtksdaap41> <1454292257.22270.3.camel@mtksdaap41> <1454307624.22441.18.camel@mtksdaap41> Message-ID: <1454395681.27603.13.camel@mtksdaap41> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, 2016-02-01 at 18:22 +0800, Daniel Kurtz wrote: > On Mon, Feb 1, 2016 at 2:20 PM, Horng-Shyang Liao 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 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 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 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, wrote: > >> > > >> >> > From: HS Liao > >> > > >> >> > > >> > > >> >> > 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 > >> > > >> >> > >> > > >> >> [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 = ; > > 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 at 14000000 > ovl0: ovl at 1400c000 > ovl1: ovl at 1400d000 > rdma0: rdma at 1400e000 > rdma1: rdma at 1400f000 > > @1401: > rdma2: rdma at 14010000 > wdma0: wdma at 14011000 > wdma1: wdma at 14012000 > color0: color at 14013000 > color1: color at 14014000 > aal at 14015000 > gamma at 14016000 > merge at 14017000 > split0: split at 14018000 > split1: split at 14019000 > ufoe at 1401a000 > dsi0: dsi at 1401b000 > dsi1: dsi at 1401c000 > dpi0: dpi at 1401d000 > pwm0: pwm at 1401e000 > pwm1: pwm at 1401f000 > > @1402: > mutex: mutex at 14020000 > od at 14023000 > larb0: larb at 14021000 > smi_common: smi at 14022000 > hdmi0: hdmi at 14025000 > larb4: larb at 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 > >