Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function
Hi Matthias, thanks for your comment. On Mon, 2020-06-22 at 13:07 +0200, Matthias Brugger wrote: > > On 21/06/2020 16:18, Dennis YC Hsieh wrote: > > add write_s function in cmdq helper functions which > > writes value contains in internal register to address > > with large dma access support. > > > > Signed-off-by: Dennis YC Hsieh > > --- > > drivers/soc/mediatek/mtk-cmdq-helper.c | 19 +++ > > include/linux/mailbox/mtk-cmdq-mailbox.h |1 + > > include/linux/soc/mediatek/mtk-cmdq.h| 19 +++ > > 3 files changed, 39 insertions(+) > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > > b/drivers/soc/mediatek/mtk-cmdq-helper.c > > index bf32e3b2ca6c..817a5a97dbe5 100644 > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > > @@ -18,6 +18,10 @@ struct cmdq_instruction { > > union { > > u32 value; > > u32 mask; > > + struct { > > + u16 arg_c; > > + u16 src_reg; > > + }; > > }; > > union { > > u16 offset; > > @@ -222,6 +226,21 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 > > subsys, > > } > > EXPORT_SYMBOL(cmdq_pkt_write_mask); > > > > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > > +u16 addr_low, u16 src_reg_idx) > > +{ > > Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and > CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and addr_low > respectively? > > In that case I think a better interface would be to pass the address and do > the > high/low calculation in the cmdq_pkt_write_s Not exactly. The high_addr_reg_idx parameter is index of internal register (which store address bit[47:16]), not result of CMDQ_ADDR_HIGH(addr). The CMDQ_ADDR_HIGH macro use in patch 02/11 cmdq_pkt_assign() api. This api helps assign address bit[47:16] into one of internal register by index. And same index could be use in cmdq_pkt_write_s(). The gce combine bit[47:16] in internal register and bit[15:0] in addr_low parameter to final address. So it is better to keep interface in this way. Regards, Dennis > > Regards, > Matthias > > > + struct cmdq_instruction inst = {}; > > + > > + inst.op = CMDQ_CODE_WRITE_S; > > + inst.src_t = CMDQ_REG_TYPE; > > + inst.sop = high_addr_reg_idx; > > + inst.offset = addr_low; > > + inst.src_reg = src_reg_idx; > > + > > + return cmdq_pkt_append_command(pkt, inst); > > +} > > +EXPORT_SYMBOL(cmdq_pkt_write_s); > > + > > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > > { > > struct cmdq_instruction inst = { {0} }; > > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h > > b/include/linux/mailbox/mtk-cmdq-mailbox.h > > index 121c3bb6d3de..ee67dd3b86f5 100644 > > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h > > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h > > @@ -59,6 +59,7 @@ enum cmdq_code { > > CMDQ_CODE_JUMP = 0x10, > > CMDQ_CODE_WFE = 0x20, > > CMDQ_CODE_EOC = 0x40, > > + CMDQ_CODE_WRITE_S = 0x90, > > CMDQ_CODE_LOGIC = 0xa0, > > }; > > > > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h > > b/include/linux/soc/mediatek/mtk-cmdq.h > > index 83340211e1d3..e1c5a7549b4f 100644 > > --- a/include/linux/soc/mediatek/mtk-cmdq.h > > +++ b/include/linux/soc/mediatek/mtk-cmdq.h > > @@ -12,6 +12,8 @@ > > #include > > > > #define CMDQ_NO_TIMEOUT0xu > > +#define CMDQ_ADDR_HIGH(addr) ((u32)(((addr) >> 16) & GENMASK(31, 0))) > > +#define CMDQ_ADDR_LOW(addr)((u16)(addr) | BIT(1)) > > > > struct cmdq_pkt; > > > > @@ -103,6 +105,23 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 > > subsys, > > u16 offset, u32 value, u32 mask); > > > > /** > > + * cmdq_pkt_write_s() - append write_s command to the CMDQ packet > > + * @pkt: the CMDQ packet > > + * @high_addr_reg_idx: internal register ID which contains high > > address of pa > > + * @addr_low: low address of pa > > + * @src_reg_idx: the CMDQ internal register ID which cache source value > > + * > > + * Return: 0 for success; else the error code is returned > > + * > > + * Support write value to physical address without subsys. Use > > CMDQ_ADDR_HIGH() > > + * to get high address and call cmdq_pkt_assign() to assign value into > > internal > > + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low parameter > > when > > + * call to this function. > > + */ > > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > > +u16 addr_low, u16 src_reg_idx); > > + > > +/** > > * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet > > * @pkt: the CMDQ packet > > * @event: the desired event type to "wait and CLEAR" > > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function
Hi Matthias, On Mon, 2020-06-22 at 17:54 +0200, Matthias Brugger wrote: > > On 22/06/2020 17:36, Dennis-YC Hsieh wrote: > > Hi Matthias, > > > > thanks for your comment. > > > > On Mon, 2020-06-22 at 13:07 +0200, Matthias Brugger wrote: > >> > >> On 21/06/2020 16:18, Dennis YC Hsieh wrote: > >>> add write_s function in cmdq helper functions which > >>> writes value contains in internal register to address > >>> with large dma access support. > >>> > >>> Signed-off-by: Dennis YC Hsieh > >>> --- > >>> drivers/soc/mediatek/mtk-cmdq-helper.c | 19 +++ > >>> include/linux/mailbox/mtk-cmdq-mailbox.h |1 + > >>> include/linux/soc/mediatek/mtk-cmdq.h| 19 +++ > >>> 3 files changed, 39 insertions(+) > >>> > >>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > >>> b/drivers/soc/mediatek/mtk-cmdq-helper.c > >>> index bf32e3b2ca6c..817a5a97dbe5 100644 > >>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > >>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > >>> @@ -18,6 +18,10 @@ struct cmdq_instruction { > >>> union { > >>> u32 value; > >>> u32 mask; > >>> + struct { > >>> + u16 arg_c; > >>> + u16 src_reg; > >>> + }; > >>> }; > >>> union { > >>> u16 offset; > >>> @@ -222,6 +226,21 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 > >>> subsys, > >>> } > >>> EXPORT_SYMBOL(cmdq_pkt_write_mask); > >>> > >>> +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > >>> + u16 addr_low, u16 src_reg_idx) > >>> +{ > >> > >> Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and > >> CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and > >> addr_low > >> respectively? > >> > >> In that case I think a better interface would be to pass the address and > >> do the > >> high/low calculation in the cmdq_pkt_write_s > > > > Not exactly. The high_addr_reg_idx parameter is index of internal > > register (which store address bit[47:16]), not result of > > CMDQ_ADDR_HIGH(addr). > > > > The CMDQ_ADDR_HIGH macro use in patch 02/11 cmdq_pkt_assign() api. This > > api helps assign address bit[47:16] into one of internal register by > > index. And same index could be use in cmdq_pkt_write_s(). The gce > > combine bit[47:16] in internal register and bit[15:0] in addr_low > > parameter to final address. So it is better to keep interface in this > > way. > > > > Got it, but then why don't we call cmdq_pkt_assign() in cmdq_pkt_write_s()? > This > way we would get a clean API for what we want to do. > Do we expect other users of cmdq_pkt_assign()? Otherwise we could keep it > private the this file and don't export it. Considering this case: write 2 register 0xaabb00c0 0xaabb00d0. If we call assign inside write_s api it will be: assign aabb to internal reg 0 write reg 0 + 0x00c0 assign aabb to internal reg 0 write reg 0 + 0x00d0 But if we let client decide timing to call assign, it will be like: assign aabb to internal reg 0 write reg 0 + 0x00c0 write reg 0 + 0x00d0 The first way uses 4 command and second one uses only 3 command. Thus it is better to let client call assign explicitly. > > By the way, why do you postfix the _s, I understand that it reflects the large > DMA access but I wonder why you choose '_s'. > The name of this command is "write_s" which is hardware spec. I'm just following it since it is a common language between gce sw/hw designers. Regards, Dennis > Regards, > Matthias > > > > > Regards, > > Dennis > > > >> > >> Regards, > >> Matthias > >> > >>> + struct cmdq_instruction inst = {}; > >>> + > >>> + inst.op = CMDQ_CODE_WRITE_S; > >>> + inst.src_t = CMDQ_REG_TYPE; > >>> + inst.sop = high_addr_reg_idx; > >>> + inst.offset = addr_low; > >>> + inst.src_reg = src_reg_idx; > >>> + > >>> + return cmdq_pkt_append_command(pkt, inst); > >>> +} > >>> +EXPORT_SYMBOL(cmdq_pkt_write_s); > >>> + > >>> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > >>> { > >>> struct cmdq_instruction inst = { {0} }; > >>> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h > >>> b/include/linux/mailbox/mtk-cmdq-mailbox.h > >>> index 121c3bb6d3de..ee67dd3b86f5 100644 > >>> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h > >>> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h > >>> @@ -59,6 +59,7 @@ enum cmdq_code { > >>> CMDQ_CODE_JUMP = 0x10, > >>> CMDQ_CODE_WFE = 0x20, > >>> CMDQ_CODE_EOC = 0x40, > >>> + CMDQ_CODE_WRITE_S = 0x90, > >>> CMDQ_CODE_LOGIC = 0xa0, > >>> }; > >>> > >>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h > >>> b/include/linux/soc/mediatek/mtk-cmdq.h > >>> index 83340211e1d3..e1c5a7549b4f 100644 > >>> --- a/include/linux/soc/mediatek/mtk-cmdq.h > >>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h > >>> @@ -12,6 +12,8 @@ > >>> #include > >>> > >>> #define CMDQ_NO_TIMEOUT 0xu > >>> +#define CMDQ_ADDR_HIGH(addr) ((u32)(((addr) >> 16) & GENMASK(31, 0))) > >>> +#define CMDQ_ADDR_LOW
Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function
Hi Matthias, On Mon, 2020-06-22 at 19:08 +0200, Matthias Brugger wrote: > > On 22/06/2020 18:12, Dennis-YC Hsieh wrote: > > Hi Matthias, > > > > On Mon, 2020-06-22 at 17:54 +0200, Matthias Brugger wrote: > >> > >> On 22/06/2020 17:36, Dennis-YC Hsieh wrote: > >>> Hi Matthias, > >>> > >>> thanks for your comment. > >>> > >>> On Mon, 2020-06-22 at 13:07 +0200, Matthias Brugger wrote: > > On 21/06/2020 16:18, Dennis YC Hsieh wrote: > > add write_s function in cmdq helper functions which > > writes value contains in internal register to address > > with large dma access support. > > > > Signed-off-by: Dennis YC Hsieh > > --- > > drivers/soc/mediatek/mtk-cmdq-helper.c | 19 +++ > > include/linux/mailbox/mtk-cmdq-mailbox.h |1 + > > include/linux/soc/mediatek/mtk-cmdq.h| 19 +++ > > 3 files changed, 39 insertions(+) > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > > b/drivers/soc/mediatek/mtk-cmdq-helper.c > > index bf32e3b2ca6c..817a5a97dbe5 100644 > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > > @@ -18,6 +18,10 @@ struct cmdq_instruction { > > union { > > u32 value; > > u32 mask; > > + struct { > > + u16 arg_c; > > + u16 src_reg; > > + }; > > }; > > union { > > u16 offset; > > @@ -222,6 +226,21 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 > > subsys, > > } > > EXPORT_SYMBOL(cmdq_pkt_write_mask); > > > > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > > +u16 addr_low, u16 src_reg_idx) > > +{ > > Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and > CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and > addr_low > respectively? > > In that case I think a better interface would be to pass the address and > do the > high/low calculation in the cmdq_pkt_write_s > >>> > >>> Not exactly. The high_addr_reg_idx parameter is index of internal > >>> register (which store address bit[47:16]), not result of > >>> CMDQ_ADDR_HIGH(addr). > >>> > >>> The CMDQ_ADDR_HIGH macro use in patch 02/11 cmdq_pkt_assign() api. This > >>> api helps assign address bit[47:16] into one of internal register by > >>> index. And same index could be use in cmdq_pkt_write_s(). The gce > >>> combine bit[47:16] in internal register and bit[15:0] in addr_low > >>> parameter to final address. So it is better to keep interface in this > >>> way. > >>> > >> > >> Got it, but then why don't we call cmdq_pkt_assign() in > >> cmdq_pkt_write_s()? This > >> way we would get a clean API for what we want to do. > >> Do we expect other users of cmdq_pkt_assign()? Otherwise we could keep it > >> private the this file and don't export it. > > > > Considering this case: write 2 register 0xaabb00c0 0xaabb00d0. > > > > If we call assign inside write_s api it will be: > > assign aabb to internal reg 0 > > write reg 0 + 0x00c0 > > assign aabb to internal reg 0 > > write reg 0 + 0x00d0 > > > > > > But if we let client decide timing to call assign, it will be like: > > assign aabb to internal reg 0 > > write reg 0 + 0x00c0 > > write reg 0 + 0x00d0 > > > > Ok, thanks for clarification. Is this something you exepect to see in the gce > consumer driver? > yes it is, less command means better performance and save memory, so it is a good practice for consumer. > > > > The first way uses 4 command and second one uses only 3 command. > > Thus it is better to let client call assign explicitly. > > > >> > >> By the way, why do you postfix the _s, I understand that it reflects the > >> large > >> DMA access but I wonder why you choose '_s'. > >> > > > > The name of this command is "write_s" which is hardware spec. > > I'm just following it since it is a common language between gce sw/hw > > designers. > > > > Ok, I will probably have to look that up every time have a look at the driver, > but that's OK. > ok thanks for your comment Regards, Dennis > Regards, > Matthias > > > > > Regards, > > Dennis > > > >> Regards, > >> Matthias > >> > >>> > >>> Regards, > >>> Dennis > >>> > > Regards, > Matthias > > > + struct cmdq_instruction inst = {}; > > + > > + inst.op = CMDQ_CODE_WRITE_S; > > + inst.src_t = CMDQ_REG_TYPE; > > + inst.sop = high_addr_reg_idx; > > + inst.offset = addr_low; > > + inst.src_reg = src_reg_idx; > > + > > + return cmdq_pkt_append_command(pkt, inst); > > +} > > +EXPORT_SYMBOL(cmdq_pkt_write_s); > > + > > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > > { > > struct cmdq_instr
Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function
On 22/06/2020 18:12, Dennis-YC Hsieh wrote: > Hi Matthias, > > On Mon, 2020-06-22 at 17:54 +0200, Matthias Brugger wrote: >> >> On 22/06/2020 17:36, Dennis-YC Hsieh wrote: >>> Hi Matthias, >>> >>> thanks for your comment. >>> >>> On Mon, 2020-06-22 at 13:07 +0200, Matthias Brugger wrote: On 21/06/2020 16:18, Dennis YC Hsieh wrote: > add write_s function in cmdq helper functions which > writes value contains in internal register to address > with large dma access support. > > Signed-off-by: Dennis YC Hsieh > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 19 +++ > include/linux/mailbox/mtk-cmdq-mailbox.h |1 + > include/linux/soc/mediatek/mtk-cmdq.h| 19 +++ > 3 files changed, 39 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > b/drivers/soc/mediatek/mtk-cmdq-helper.c > index bf32e3b2ca6c..817a5a97dbe5 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -18,6 +18,10 @@ struct cmdq_instruction { > union { > u32 value; > u32 mask; > + struct { > + u16 arg_c; > + u16 src_reg; > + }; > }; > union { > u16 offset; > @@ -222,6 +226,21 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 > subsys, > } > EXPORT_SYMBOL(cmdq_pkt_write_mask); > > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > + u16 addr_low, u16 src_reg_idx) > +{ Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and addr_low respectively? In that case I think a better interface would be to pass the address and do the high/low calculation in the cmdq_pkt_write_s >>> >>> Not exactly. The high_addr_reg_idx parameter is index of internal >>> register (which store address bit[47:16]), not result of >>> CMDQ_ADDR_HIGH(addr). >>> >>> The CMDQ_ADDR_HIGH macro use in patch 02/11 cmdq_pkt_assign() api. This >>> api helps assign address bit[47:16] into one of internal register by >>> index. And same index could be use in cmdq_pkt_write_s(). The gce >>> combine bit[47:16] in internal register and bit[15:0] in addr_low >>> parameter to final address. So it is better to keep interface in this >>> way. >>> >> >> Got it, but then why don't we call cmdq_pkt_assign() in cmdq_pkt_write_s()? >> This >> way we would get a clean API for what we want to do. >> Do we expect other users of cmdq_pkt_assign()? Otherwise we could keep it >> private the this file and don't export it. > > Considering this case: write 2 register 0xaabb00c0 0xaabb00d0. > > If we call assign inside write_s api it will be: > assign aabb to internal reg 0 > write reg 0 + 0x00c0 > assign aabb to internal reg 0 > write reg 0 + 0x00d0 > > > But if we let client decide timing to call assign, it will be like: > assign aabb to internal reg 0 > write reg 0 + 0x00c0 > write reg 0 + 0x00d0 > Ok, thanks for clarification. Is this something you exepect to see in the gce consumer driver? > > The first way uses 4 command and second one uses only 3 command. > Thus it is better to let client call assign explicitly. > >> >> By the way, why do you postfix the _s, I understand that it reflects the >> large >> DMA access but I wonder why you choose '_s'. >> > > The name of this command is "write_s" which is hardware spec. > I'm just following it since it is a common language between gce sw/hw > designers. > Ok, I will probably have to look that up every time have a look at the driver, but that's OK. Regards, Matthias > > Regards, > Dennis > >> Regards, >> Matthias >> >>> >>> Regards, >>> Dennis >>> Regards, Matthias > + struct cmdq_instruction inst = {}; > + > + inst.op = CMDQ_CODE_WRITE_S; > + inst.src_t = CMDQ_REG_TYPE; > + inst.sop = high_addr_reg_idx; > + inst.offset = addr_low; > + inst.src_reg = src_reg_idx; > + > + return cmdq_pkt_append_command(pkt, inst); > +} > +EXPORT_SYMBOL(cmdq_pkt_write_s); > + > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > { > struct cmdq_instruction inst = { {0} }; > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h > b/include/linux/mailbox/mtk-cmdq-mailbox.h > index 121c3bb6d3de..ee67dd3b86f5 100644 > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h > @@ -59,6 +59,7 @@ enum cmdq_code { > CMDQ_CODE_JUMP = 0x10, > CMDQ_CODE_WFE = 0x20, > CMDQ_CODE_EOC = 0x40, > + CMDQ_CODE_WRITE_S = 0x90, > CMDQ_CODE_LOGIC = 0xa0, > }; > > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h > b/include/linux/soc/mediatek/mtk-cmdq.h > index 83340211e1d3..e1
Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function
On 22/06/2020 17:36, Dennis-YC Hsieh wrote: > Hi Matthias, > > thanks for your comment. > > On Mon, 2020-06-22 at 13:07 +0200, Matthias Brugger wrote: >> >> On 21/06/2020 16:18, Dennis YC Hsieh wrote: >>> add write_s function in cmdq helper functions which >>> writes value contains in internal register to address >>> with large dma access support. >>> >>> Signed-off-by: Dennis YC Hsieh >>> --- >>> drivers/soc/mediatek/mtk-cmdq-helper.c | 19 +++ >>> include/linux/mailbox/mtk-cmdq-mailbox.h |1 + >>> include/linux/soc/mediatek/mtk-cmdq.h| 19 +++ >>> 3 files changed, 39 insertions(+) >>> >>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c >>> b/drivers/soc/mediatek/mtk-cmdq-helper.c >>> index bf32e3b2ca6c..817a5a97dbe5 100644 >>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c >>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c >>> @@ -18,6 +18,10 @@ struct cmdq_instruction { >>> union { >>> u32 value; >>> u32 mask; >>> + struct { >>> + u16 arg_c; >>> + u16 src_reg; >>> + }; >>> }; >>> union { >>> u16 offset; >>> @@ -222,6 +226,21 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 >>> subsys, >>> } >>> EXPORT_SYMBOL(cmdq_pkt_write_mask); >>> >>> +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, >>> +u16 addr_low, u16 src_reg_idx) >>> +{ >> >> Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and >> CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and addr_low >> respectively? >> >> In that case I think a better interface would be to pass the address and do >> the >> high/low calculation in the cmdq_pkt_write_s > > Not exactly. The high_addr_reg_idx parameter is index of internal > register (which store address bit[47:16]), not result of > CMDQ_ADDR_HIGH(addr). > > The CMDQ_ADDR_HIGH macro use in patch 02/11 cmdq_pkt_assign() api. This > api helps assign address bit[47:16] into one of internal register by > index. And same index could be use in cmdq_pkt_write_s(). The gce > combine bit[47:16] in internal register and bit[15:0] in addr_low > parameter to final address. So it is better to keep interface in this > way. > Got it, but then why don't we call cmdq_pkt_assign() in cmdq_pkt_write_s()? This way we would get a clean API for what we want to do. Do we expect other users of cmdq_pkt_assign()? Otherwise we could keep it private the this file and don't export it. By the way, why do you postfix the _s, I understand that it reflects the large DMA access but I wonder why you choose '_s'. Regards, Matthias > > Regards, > Dennis > >> >> Regards, >> Matthias >> >>> + struct cmdq_instruction inst = {}; >>> + >>> + inst.op = CMDQ_CODE_WRITE_S; >>> + inst.src_t = CMDQ_REG_TYPE; >>> + inst.sop = high_addr_reg_idx; >>> + inst.offset = addr_low; >>> + inst.src_reg = src_reg_idx; >>> + >>> + return cmdq_pkt_append_command(pkt, inst); >>> +} >>> +EXPORT_SYMBOL(cmdq_pkt_write_s); >>> + >>> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) >>> { >>> struct cmdq_instruction inst = { {0} }; >>> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h >>> b/include/linux/mailbox/mtk-cmdq-mailbox.h >>> index 121c3bb6d3de..ee67dd3b86f5 100644 >>> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h >>> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h >>> @@ -59,6 +59,7 @@ enum cmdq_code { >>> CMDQ_CODE_JUMP = 0x10, >>> CMDQ_CODE_WFE = 0x20, >>> CMDQ_CODE_EOC = 0x40, >>> + CMDQ_CODE_WRITE_S = 0x90, >>> CMDQ_CODE_LOGIC = 0xa0, >>> }; >>> >>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h >>> b/include/linux/soc/mediatek/mtk-cmdq.h >>> index 83340211e1d3..e1c5a7549b4f 100644 >>> --- a/include/linux/soc/mediatek/mtk-cmdq.h >>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h >>> @@ -12,6 +12,8 @@ >>> #include >>> >>> #define CMDQ_NO_TIMEOUT0xu >>> +#define CMDQ_ADDR_HIGH(addr) ((u32)(((addr) >> 16) & GENMASK(31, 0))) >>> +#define CMDQ_ADDR_LOW(addr)((u16)(addr) | BIT(1)) >>> >>> struct cmdq_pkt; >>> >>> @@ -103,6 +105,23 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 >>> subsys, >>> u16 offset, u32 value, u32 mask); >>> >>> /** >>> + * cmdq_pkt_write_s() - append write_s command to the CMDQ packet >>> + * @pkt: the CMDQ packet >>> + * @high_addr_reg_idx: internal register ID which contains high >>> address of pa >>> + * @addr_low: low address of pa >>> + * @src_reg_idx: the CMDQ internal register ID which cache source value >>> + * >>> + * Return: 0 for success; else the error code is returned >>> + * >>> + * Support write value to physical address without subsys. Use >>> CMDQ_ADDR_HIGH() >>> + * to get high address and call cmdq_pkt_assign() to assign value into >>> internal >>> + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low parameter >>> when >>> + * call to this fu
Re: [PATCH v1 03/11] soc: mediatek: cmdq: add write_s function
On 21/06/2020 16:18, Dennis YC Hsieh wrote: > add write_s function in cmdq helper functions which > writes value contains in internal register to address > with large dma access support. > > Signed-off-by: Dennis YC Hsieh > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 19 +++ > include/linux/mailbox/mtk-cmdq-mailbox.h |1 + > include/linux/soc/mediatek/mtk-cmdq.h| 19 +++ > 3 files changed, 39 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > b/drivers/soc/mediatek/mtk-cmdq-helper.c > index bf32e3b2ca6c..817a5a97dbe5 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -18,6 +18,10 @@ struct cmdq_instruction { > union { > u32 value; > u32 mask; > + struct { > + u16 arg_c; > + u16 src_reg; > + }; > }; > union { > u16 offset; > @@ -222,6 +226,21 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys, > } > EXPORT_SYMBOL(cmdq_pkt_write_mask); > > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > + u16 addr_low, u16 src_reg_idx) > +{ Do I understand correctly that we use CMDQ_ADDR_HIGH(addr) and CMDQ_ADDR_LOW(addr) to calculate in the client high_addr_reg_idx and addr_low respectively? In that case I think a better interface would be to pass the address and do the high/low calculation in the cmdq_pkt_write_s Regards, Matthias > + struct cmdq_instruction inst = {}; > + > + inst.op = CMDQ_CODE_WRITE_S; > + inst.src_t = CMDQ_REG_TYPE; > + inst.sop = high_addr_reg_idx; > + inst.offset = addr_low; > + inst.src_reg = src_reg_idx; > + > + return cmdq_pkt_append_command(pkt, inst); > +} > +EXPORT_SYMBOL(cmdq_pkt_write_s); > + > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > { > struct cmdq_instruction inst = { {0} }; > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h > b/include/linux/mailbox/mtk-cmdq-mailbox.h > index 121c3bb6d3de..ee67dd3b86f5 100644 > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h > @@ -59,6 +59,7 @@ enum cmdq_code { > CMDQ_CODE_JUMP = 0x10, > CMDQ_CODE_WFE = 0x20, > CMDQ_CODE_EOC = 0x40, > + CMDQ_CODE_WRITE_S = 0x90, > CMDQ_CODE_LOGIC = 0xa0, > }; > > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h > b/include/linux/soc/mediatek/mtk-cmdq.h > index 83340211e1d3..e1c5a7549b4f 100644 > --- a/include/linux/soc/mediatek/mtk-cmdq.h > +++ b/include/linux/soc/mediatek/mtk-cmdq.h > @@ -12,6 +12,8 @@ > #include > > #define CMDQ_NO_TIMEOUT 0xu > +#define CMDQ_ADDR_HIGH(addr) ((u32)(((addr) >> 16) & GENMASK(31, 0))) > +#define CMDQ_ADDR_LOW(addr) ((u16)(addr) | BIT(1)) > > struct cmdq_pkt; > > @@ -103,6 +105,23 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys, > u16 offset, u32 value, u32 mask); > > /** > + * cmdq_pkt_write_s() - append write_s command to the CMDQ packet > + * @pkt: the CMDQ packet > + * @high_addr_reg_idx: internal register ID which contains high > address of pa > + * @addr_low:low address of pa > + * @src_reg_idx: the CMDQ internal register ID which cache source value > + * > + * Return: 0 for success; else the error code is returned > + * > + * Support write value to physical address without subsys. Use > CMDQ_ADDR_HIGH() > + * to get high address and call cmdq_pkt_assign() to assign value into > internal > + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low parameter > when > + * call to this function. > + */ > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > + u16 addr_low, u16 src_reg_idx); > + > +/** > * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet > * @pkt: the CMDQ packet > * @event: the desired event type to "wait and CLEAR" > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1 03/11] soc: mediatek: cmdq: add write_s function
add write_s function in cmdq helper functions which writes value contains in internal register to address with large dma access support. Signed-off-by: Dennis YC Hsieh --- drivers/soc/mediatek/mtk-cmdq-helper.c | 19 +++ include/linux/mailbox/mtk-cmdq-mailbox.h |1 + include/linux/soc/mediatek/mtk-cmdq.h| 19 +++ 3 files changed, 39 insertions(+) diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c index bf32e3b2ca6c..817a5a97dbe5 100644 --- a/drivers/soc/mediatek/mtk-cmdq-helper.c +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c @@ -18,6 +18,10 @@ struct cmdq_instruction { union { u32 value; u32 mask; + struct { + u16 arg_c; + u16 src_reg; + }; }; union { u16 offset; @@ -222,6 +226,21 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys, } EXPORT_SYMBOL(cmdq_pkt_write_mask); +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, +u16 addr_low, u16 src_reg_idx) +{ + struct cmdq_instruction inst = {}; + + inst.op = CMDQ_CODE_WRITE_S; + inst.src_t = CMDQ_REG_TYPE; + inst.sop = high_addr_reg_idx; + inst.offset = addr_low; + inst.src_reg = src_reg_idx; + + return cmdq_pkt_append_command(pkt, inst); +} +EXPORT_SYMBOL(cmdq_pkt_write_s); + int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) { struct cmdq_instruction inst = { {0} }; diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h index 121c3bb6d3de..ee67dd3b86f5 100644 --- a/include/linux/mailbox/mtk-cmdq-mailbox.h +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h @@ -59,6 +59,7 @@ enum cmdq_code { CMDQ_CODE_JUMP = 0x10, CMDQ_CODE_WFE = 0x20, CMDQ_CODE_EOC = 0x40, + CMDQ_CODE_WRITE_S = 0x90, CMDQ_CODE_LOGIC = 0xa0, }; diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h index 83340211e1d3..e1c5a7549b4f 100644 --- a/include/linux/soc/mediatek/mtk-cmdq.h +++ b/include/linux/soc/mediatek/mtk-cmdq.h @@ -12,6 +12,8 @@ #include #define CMDQ_NO_TIMEOUT0xu +#define CMDQ_ADDR_HIGH(addr) ((u32)(((addr) >> 16) & GENMASK(31, 0))) +#define CMDQ_ADDR_LOW(addr)((u16)(addr) | BIT(1)) struct cmdq_pkt; @@ -103,6 +105,23 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value, u32 mask); /** + * cmdq_pkt_write_s() - append write_s command to the CMDQ packet + * @pkt: the CMDQ packet + * @high_addr_reg_idx: internal register ID which contains high address of pa + * @addr_low: low address of pa + * @src_reg_idx: the CMDQ internal register ID which cache source value + * + * Return: 0 for success; else the error code is returned + * + * Support write value to physical address without subsys. Use CMDQ_ADDR_HIGH() + * to get high address and call cmdq_pkt_assign() to assign value into internal + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low parameter when + * call to this function. + */ +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, +u16 addr_low, u16 src_reg_idx); + +/** * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet * @pkt: the CMDQ packet * @event: the desired event type to "wait and CLEAR" -- 1.7.9.5 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel