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 dennis-yc.hsieh@mediatek.com
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;};@@ -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_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 <linux/timer.h>
#define CMDQ_NO_TIMEOUT 0xffffffffu +#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"