| Message ID | 1583664775-19382-10-git-send-email-dennis-yc.hsieh@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support gce on mt6779 platform | expand |
On 08/03/2020 11:52, Dennis YC Hsieh wrote: > add write_s function in cmdq helper functions which > writes a constant value to address with large dma > access support. > > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> > Reviewed-by: CK Hu <ck.hu@mediatek.com> > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++ > include/linux/soc/mediatek/mtk-cmdq.h | 14 ++++++++++++++ > 2 files changed, 40 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c > index 03c129230cd7..a9ebbabb7439 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > } > EXPORT_SYMBOL(cmdq_pkt_write_s); > > +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > + u16 addr_low, u32 value, u32 mask) > +{ > + struct cmdq_instruction inst = { {0} }; > + int err; > + > + if (mask != U32_MAX) { > + inst.op = CMDQ_CODE_MASK; > + inst.mask = ~mask; > + err = cmdq_pkt_append_command(pkt, inst); > + if (err < 0) > + return err; > + > + inst.op = CMDQ_CODE_WRITE_S_MASK; > + } else { > + inst.op = CMDQ_CODE_WRITE_S; > + } > + > + inst.sop = high_addr_reg_idx; Writing u16 value in a 5 bit wide variable? > + inst.offset = addr_low; > + inst.value = value; > + > + return cmdq_pkt_append_command(pkt, inst); > +} > +EXPORT_SYMBOL(cmdq_pkt_write_s_value); > + > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > { > struct cmdq_instruction inst = { {0} }; > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h > index 01b4184af310..fec292aac83c 100644 > --- a/include/linux/soc/mediatek/mtk-cmdq.h > +++ b/include/linux/soc/mediatek/mtk-cmdq.h > @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low, > int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > u16 addr_low, u16 src_reg_idx, u32 mask); > > +/** > + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ > + * packet which write value to a physical address > + * @pkt: the CMDQ packet > + * @high_addr_reg_idx: internal regisger ID which contains high address of pa register > + * @addr_low: low address of pa > + * @value: the specified target value > + * @mask: the specified target mask > + * > + * Return: 0 for success; else the error code is returned > + */ > +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > + u16 addr_low, u32 value, u32 mask); > + > /** > * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet > * @pkt: the CMDQ packet >
Hi Matthias, Thanks for your comment. On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote: > > On 08/03/2020 11:52, Dennis YC Hsieh wrote: > > add write_s function in cmdq helper functions which > > writes a constant value to address with large dma > > access support. > > > > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> > > Reviewed-by: CK Hu <ck.hu@mediatek.com> > > --- > > drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++ > > include/linux/soc/mediatek/mtk-cmdq.h | 14 ++++++++++++++ > > 2 files changed, 40 insertions(+) > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c > > index 03c129230cd7..a9ebbabb7439 100644 > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > > @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > > } > > EXPORT_SYMBOL(cmdq_pkt_write_s); > > > > +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > > + u16 addr_low, u32 value, u32 mask) > > +{ > > + struct cmdq_instruction inst = { {0} }; > > + int err; > > + > > + if (mask != U32_MAX) { > > + inst.op = CMDQ_CODE_MASK; > > + inst.mask = ~mask; > > + err = cmdq_pkt_append_command(pkt, inst); > > + if (err < 0) > > + return err; > > + > > + inst.op = CMDQ_CODE_WRITE_S_MASK; > > + } else { > > + inst.op = CMDQ_CODE_WRITE_S; > > + } > > + > > + inst.sop = high_addr_reg_idx; > > Writing u16 value in a 5 bit wide variable? We need only 5 bits in this case. I'll change high_addr_reg_idx parameter to u8. > > > + inst.offset = addr_low; > > + inst.value = value; > > + > > + return cmdq_pkt_append_command(pkt, inst); > > +} > > +EXPORT_SYMBOL(cmdq_pkt_write_s_value); > > + > > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > > { > > struct cmdq_instruction inst = { {0} }; > > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h > > index 01b4184af310..fec292aac83c 100644 > > --- a/include/linux/soc/mediatek/mtk-cmdq.h > > +++ b/include/linux/soc/mediatek/mtk-cmdq.h > > @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low, > > int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > > u16 addr_low, u16 src_reg_idx, u32 mask); > > > > +/** > > + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ > > + * packet which write value to a physical address > > + * @pkt: the CMDQ packet > > + * @high_addr_reg_idx: internal regisger ID which contains high address of pa > > register will fix Regards, Dennis > > > + * @addr_low: low address of pa > > + * @value: the specified target value > > + * @mask: the specified target mask > > + * > > + * Return: 0 for success; else the error code is returned > > + */ > > +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > > + u16 addr_low, u32 value, u32 mask); > > + > > /** > > * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet > > * @pkt: the CMDQ packet > >
On 24/05/2020 19:31, Dennis-YC Hsieh wrote: > Hi Matthias, > > Thanks for your comment. > > On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote: >> >> On 08/03/2020 11:52, Dennis YC Hsieh wrote: >>> add write_s function in cmdq helper functions which >>> writes a constant value to address with large dma >>> access support. >>> >>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> >>> Reviewed-by: CK Hu <ck.hu@mediatek.com> >>> --- >>> drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++ >>> include/linux/soc/mediatek/mtk-cmdq.h | 14 ++++++++++++++ >>> 2 files changed, 40 insertions(+) >>> >>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c >>> index 03c129230cd7..a9ebbabb7439 100644 >>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c >>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c >>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, >>> } >>> EXPORT_SYMBOL(cmdq_pkt_write_s); >>> >>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, >>> + u16 addr_low, u32 value, u32 mask) >>> +{ >>> + struct cmdq_instruction inst = { {0} }; >>> + int err; >>> + >>> + if (mask != U32_MAX) { >>> + inst.op = CMDQ_CODE_MASK; >>> + inst.mask = ~mask; >>> + err = cmdq_pkt_append_command(pkt, inst); >>> + if (err < 0) >>> + return err; >>> + >>> + inst.op = CMDQ_CODE_WRITE_S_MASK; >>> + } else { >>> + inst.op = CMDQ_CODE_WRITE_S; >>> + } >>> + >>> + inst.sop = high_addr_reg_idx; >> >> Writing u16 value in a 5 bit wide variable? > > We need only 5 bits in this case. I'll change high_addr_reg_idx > parameter to u8. > Ok, please make sure to mask the value, so that it's explicit in the code that we only use the lowest 5 bits of high_addr_reg_idx. Regards, Matthias >> >>> + inst.offset = addr_low; >>> + inst.value = value; >>> + >>> + return cmdq_pkt_append_command(pkt, inst); >>> +} >>> +EXPORT_SYMBOL(cmdq_pkt_write_s_value); >>> + >>> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) >>> { >>> struct cmdq_instruction inst = { {0} }; >>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h >>> index 01b4184af310..fec292aac83c 100644 >>> --- a/include/linux/soc/mediatek/mtk-cmdq.h >>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h >>> @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low, >>> int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, >>> u16 addr_low, u16 src_reg_idx, u32 mask); >>> >>> +/** >>> + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ >>> + * packet which write value to a physical address >>> + * @pkt: the CMDQ packet >>> + * @high_addr_reg_idx: internal regisger ID which contains high address of pa >> >> register > > will fix > > > Regards, > Dennis > >> >>> + * @addr_low: low address of pa >>> + * @value: the specified target value >>> + * @mask: the specified target mask >>> + * >>> + * Return: 0 for success; else the error code is returned >>> + */ >>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, >>> + u16 addr_low, u32 value, u32 mask); >>> + >>> /** >>> * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet >>> * @pkt: the CMDQ packet >>> >
On Sun, 2020-05-24 at 20:13 +0200, Matthias Brugger wrote: > > On 24/05/2020 19:31, Dennis-YC Hsieh wrote: > > Hi Matthias, > > > > Thanks for your comment. > > > > On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote: > >> > >> On 08/03/2020 11:52, Dennis YC Hsieh wrote: > >>> add write_s function in cmdq helper functions which > >>> writes a constant value to address with large dma > >>> access support. > >>> > >>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> > >>> Reviewed-by: CK Hu <ck.hu@mediatek.com> > >>> --- > >>> drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++ > >>> include/linux/soc/mediatek/mtk-cmdq.h | 14 ++++++++++++++ > >>> 2 files changed, 40 insertions(+) > >>> > >>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c > >>> index 03c129230cd7..a9ebbabb7439 100644 > >>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > >>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > >>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > >>> } > >>> EXPORT_SYMBOL(cmdq_pkt_write_s); > >>> > >>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > >>> + u16 addr_low, u32 value, u32 mask) > >>> +{ > >>> + struct cmdq_instruction inst = { {0} }; > >>> + int err; > >>> + > >>> + if (mask != U32_MAX) { > >>> + inst.op = CMDQ_CODE_MASK; > >>> + inst.mask = ~mask; > >>> + err = cmdq_pkt_append_command(pkt, inst); > >>> + if (err < 0) > >>> + return err; > >>> + > >>> + inst.op = CMDQ_CODE_WRITE_S_MASK; > >>> + } else { > >>> + inst.op = CMDQ_CODE_WRITE_S; > >>> + } > >>> + > >>> + inst.sop = high_addr_reg_idx; > >> > >> Writing u16 value in a 5 bit wide variable? > > > > We need only 5 bits in this case. I'll change high_addr_reg_idx > > parameter to u8. > > > > Ok, please make sure to mask the value, so that it's explicit in the code that > we only use the lowest 5 bits of high_addr_reg_idx. Is it necessary to mask the value? Since sop already defined as "u8 sop:5;", I thought it is explicit that only use 5 bits and compiler should do the rest jobs. Regards, Dennis > > Regards, > Matthias > > >> > >>> + inst.offset = addr_low; > >>> + inst.value = value; > >>> + > >>> + return cmdq_pkt_append_command(pkt, inst); > >>> +} > >>> +EXPORT_SYMBOL(cmdq_pkt_write_s_value); > >>> + > >>> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > >>> { > >>> struct cmdq_instruction inst = { {0} }; > >>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h > >>> index 01b4184af310..fec292aac83c 100644 > >>> --- a/include/linux/soc/mediatek/mtk-cmdq.h > >>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h > >>> @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low, > >>> int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > >>> u16 addr_low, u16 src_reg_idx, u32 mask); > >>> > >>> +/** > >>> + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ > >>> + * packet which write value to a physical address > >>> + * @pkt: the CMDQ packet > >>> + * @high_addr_reg_idx: internal regisger ID which contains high address of pa > >> > >> register > > > > will fix > > > > > > Regards, > > Dennis > > > >> > >>> + * @addr_low: low address of pa > >>> + * @value: the specified target value > >>> + * @mask: the specified target mask > >>> + * > >>> + * Return: 0 for success; else the error code is returned > >>> + */ > >>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > >>> + u16 addr_low, u32 value, u32 mask); > >>> + > >>> /** > >>> * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet > >>> * @pkt: the CMDQ packet > >>> > >
On 25/05/2020 04:27, Dennis-YC Hsieh wrote: > > On Sun, 2020-05-24 at 20:13 +0200, Matthias Brugger wrote: >> >> On 24/05/2020 19:31, Dennis-YC Hsieh wrote: >>> Hi Matthias, >>> >>> Thanks for your comment. >>> >>> On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote: >>>> >>>> On 08/03/2020 11:52, Dennis YC Hsieh wrote: >>>>> add write_s function in cmdq helper functions which >>>>> writes a constant value to address with large dma >>>>> access support. >>>>> >>>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> >>>>> Reviewed-by: CK Hu <ck.hu@mediatek.com> >>>>> --- >>>>> drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++ >>>>> include/linux/soc/mediatek/mtk-cmdq.h | 14 ++++++++++++++ >>>>> 2 files changed, 40 insertions(+) >>>>> >>>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c >>>>> index 03c129230cd7..a9ebbabb7439 100644 >>>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c >>>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c >>>>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, >>>>> } >>>>> EXPORT_SYMBOL(cmdq_pkt_write_s); >>>>> >>>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, >>>>> + u16 addr_low, u32 value, u32 mask) >>>>> +{ >>>>> + struct cmdq_instruction inst = { {0} }; >>>>> + int err; >>>>> + >>>>> + if (mask != U32_MAX) { >>>>> + inst.op = CMDQ_CODE_MASK; >>>>> + inst.mask = ~mask; >>>>> + err = cmdq_pkt_append_command(pkt, inst); >>>>> + if (err < 0) >>>>> + return err; >>>>> + >>>>> + inst.op = CMDQ_CODE_WRITE_S_MASK; >>>>> + } else { >>>>> + inst.op = CMDQ_CODE_WRITE_S; >>>>> + } >>>>> + >>>>> + inst.sop = high_addr_reg_idx; >>>> >>>> Writing u16 value in a 5 bit wide variable? >>> >>> We need only 5 bits in this case. I'll change high_addr_reg_idx >>> parameter to u8. >>> >> >> Ok, please make sure to mask the value, so that it's explicit in the code that >> we only use the lowest 5 bits of high_addr_reg_idx. > > Is it necessary to mask the value? > Since sop already defined as "u8 sop:5;", I thought it is explicit that > only use 5 bits and compiler should do the rest jobs. Yes but it makes the code more explicit if we have a inst.sop = high_addr_reg_idx & 0x1f; What do you think? Regards, Matthias > > > Regards, > Dennis > >> >> Regards, >> Matthias >> >>>> >>>>> + inst.offset = addr_low; >>>>> + inst.value = value; >>>>> + >>>>> + return cmdq_pkt_append_command(pkt, inst); >>>>> +} >>>>> +EXPORT_SYMBOL(cmdq_pkt_write_s_value); >>>>> + >>>>> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) >>>>> { >>>>> struct cmdq_instruction inst = { {0} }; >>>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h >>>>> index 01b4184af310..fec292aac83c 100644 >>>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h >>>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h >>>>> @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low, >>>>> int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, >>>>> u16 addr_low, u16 src_reg_idx, u32 mask); >>>>> >>>>> +/** >>>>> + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ >>>>> + * packet which write value to a physical address >>>>> + * @pkt: the CMDQ packet >>>>> + * @high_addr_reg_idx: internal regisger ID which contains high address of pa >>>> >>>> register >>> >>> will fix >>> >>> >>> Regards, >>> Dennis >>> >>>> >>>>> + * @addr_low: low address of pa >>>>> + * @value: the specified target value >>>>> + * @mask: the specified target mask >>>>> + * >>>>> + * Return: 0 for success; else the error code is returned >>>>> + */ >>>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, >>>>> + u16 addr_low, u32 value, u32 mask); >>>>> + >>>>> /** >>>>> * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet >>>>> * @pkt: the CMDQ packet >>>>> >>> >
On Mon, 2020-05-25 at 10:39 +0200, Matthias Brugger wrote: > > On 25/05/2020 04:27, Dennis-YC Hsieh wrote: > > > > On Sun, 2020-05-24 at 20:13 +0200, Matthias Brugger wrote: > >> > >> On 24/05/2020 19:31, Dennis-YC Hsieh wrote: > >>> Hi Matthias, > >>> > >>> Thanks for your comment. > >>> > >>> On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote: > >>>> > >>>> On 08/03/2020 11:52, Dennis YC Hsieh wrote: > >>>>> add write_s function in cmdq helper functions which > >>>>> writes a constant value to address with large dma > >>>>> access support. > >>>>> > >>>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> > >>>>> Reviewed-by: CK Hu <ck.hu@mediatek.com> > >>>>> --- > >>>>> drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++ > >>>>> include/linux/soc/mediatek/mtk-cmdq.h | 14 ++++++++++++++ > >>>>> 2 files changed, 40 insertions(+) > >>>>> > >>>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c > >>>>> index 03c129230cd7..a9ebbabb7439 100644 > >>>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > >>>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > >>>>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > >>>>> } > >>>>> EXPORT_SYMBOL(cmdq_pkt_write_s); > >>>>> > >>>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > >>>>> + u16 addr_low, u32 value, u32 mask) > >>>>> +{ > >>>>> + struct cmdq_instruction inst = { {0} }; > >>>>> + int err; > >>>>> + > >>>>> + if (mask != U32_MAX) { > >>>>> + inst.op = CMDQ_CODE_MASK; > >>>>> + inst.mask = ~mask; > >>>>> + err = cmdq_pkt_append_command(pkt, inst); > >>>>> + if (err < 0) > >>>>> + return err; > >>>>> + > >>>>> + inst.op = CMDQ_CODE_WRITE_S_MASK; > >>>>> + } else { > >>>>> + inst.op = CMDQ_CODE_WRITE_S; > >>>>> + } > >>>>> + > >>>>> + inst.sop = high_addr_reg_idx; > >>>> > >>>> Writing u16 value in a 5 bit wide variable? > >>> > >>> We need only 5 bits in this case. I'll change high_addr_reg_idx > >>> parameter to u8. > >>> > >> > >> Ok, please make sure to mask the value, so that it's explicit in the code that > >> we only use the lowest 5 bits of high_addr_reg_idx. > > > > Is it necessary to mask the value? > > Since sop already defined as "u8 sop:5;", I thought it is explicit that > > only use 5 bits and compiler should do the rest jobs. > > Yes but it makes the code more explicit if we have a > inst.sop = high_addr_reg_idx & 0x1f; > > What do you think? The value assign to sop will restrict by hardware spec. Clients call this function will define constant value and use it as parameter. So I think we don't worry about client call this api with wrong value. Regards, Dennis > > Regards, > Matthias > > > > > > > Regards, > > Dennis > > > >> > >> Regards, > >> Matthias > >> > >>>> > >>>>> + inst.offset = addr_low; > >>>>> + inst.value = value; > >>>>> + > >>>>> + return cmdq_pkt_append_command(pkt, inst); > >>>>> +} > >>>>> +EXPORT_SYMBOL(cmdq_pkt_write_s_value); > >>>>> + > >>>>> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > >>>>> { > >>>>> struct cmdq_instruction inst = { {0} }; > >>>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h > >>>>> index 01b4184af310..fec292aac83c 100644 > >>>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h > >>>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h > >>>>> @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low, > >>>>> int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > >>>>> u16 addr_low, u16 src_reg_idx, u32 mask); > >>>>> > >>>>> +/** > >>>>> + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ > >>>>> + * packet which write value to a physical address > >>>>> + * @pkt: the CMDQ packet > >>>>> + * @high_addr_reg_idx: internal regisger ID which contains high address of pa > >>>> > >>>> register > >>> > >>> will fix > >>> > >>> > >>> Regards, > >>> Dennis > >>> > >>>> > >>>>> + * @addr_low: low address of pa > >>>>> + * @value: the specified target value > >>>>> + * @mask: the specified target mask > >>>>> + * > >>>>> + * Return: 0 for success; else the error code is returned > >>>>> + */ > >>>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, > >>>>> + u16 addr_low, u32 value, u32 mask); > >>>>> + > >>>>> /** > >>>>> * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet > >>>>> * @pkt: the CMDQ packet > >>>>> > >>> > >
On 25/05/2020 12:38, Dennis-YC Hsieh wrote: > > On Mon, 2020-05-25 at 10:39 +0200, Matthias Brugger wrote: >> >> On 25/05/2020 04:27, Dennis-YC Hsieh wrote: >>> >>> On Sun, 2020-05-24 at 20:13 +0200, Matthias Brugger wrote: >>>> >>>> On 24/05/2020 19:31, Dennis-YC Hsieh wrote: >>>>> Hi Matthias, >>>>> >>>>> Thanks for your comment. >>>>> >>>>> On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote: >>>>>> >>>>>> On 08/03/2020 11:52, Dennis YC Hsieh wrote: >>>>>>> add write_s function in cmdq helper functions which >>>>>>> writes a constant value to address with large dma >>>>>>> access support. >>>>>>> >>>>>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com> >>>>>>> Reviewed-by: CK Hu <ck.hu@mediatek.com> >>>>>>> --- >>>>>>> drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++ >>>>>>> include/linux/soc/mediatek/mtk-cmdq.h | 14 ++++++++++++++ >>>>>>> 2 files changed, 40 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c >>>>>>> index 03c129230cd7..a9ebbabb7439 100644 >>>>>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c >>>>>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c >>>>>>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, >>>>>>> } >>>>>>> EXPORT_SYMBOL(cmdq_pkt_write_s); >>>>>>> >>>>>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, >>>>>>> + u16 addr_low, u32 value, u32 mask) >>>>>>> +{ >>>>>>> + struct cmdq_instruction inst = { {0} }; >>>>>>> + int err; >>>>>>> + >>>>>>> + if (mask != U32_MAX) { >>>>>>> + inst.op = CMDQ_CODE_MASK; >>>>>>> + inst.mask = ~mask; >>>>>>> + err = cmdq_pkt_append_command(pkt, inst); >>>>>>> + if (err < 0) >>>>>>> + return err; >>>>>>> + >>>>>>> + inst.op = CMDQ_CODE_WRITE_S_MASK; >>>>>>> + } else { >>>>>>> + inst.op = CMDQ_CODE_WRITE_S; >>>>>>> + } >>>>>>> + >>>>>>> + inst.sop = high_addr_reg_idx; >>>>>> >>>>>> Writing u16 value in a 5 bit wide variable? >>>>> >>>>> We need only 5 bits in this case. I'll change high_addr_reg_idx >>>>> parameter to u8. >>>>> >>>> >>>> Ok, please make sure to mask the value, so that it's explicit in the code that >>>> we only use the lowest 5 bits of high_addr_reg_idx. >>> >>> Is it necessary to mask the value? >>> Since sop already defined as "u8 sop:5;", I thought it is explicit that >>> only use 5 bits and compiler should do the rest jobs. >> >> Yes but it makes the code more explicit if we have a >> inst.sop = high_addr_reg_idx & 0x1f; >> >> What do you think? > > The value assign to sop will restrict by hardware spec. Clients call > this function will define constant value and use it as parameter. So I > think we don't worry about client call this api with wrong value. > Ok, then let's change the parameter to u8 and don't add a mask. Regards, Matthias
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c index 03c129230cd7..a9ebbabb7439 100644 --- a/drivers/soc/mediatek/mtk-cmdq-helper.c +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, } EXPORT_SYMBOL(cmdq_pkt_write_s); +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, + u16 addr_low, u32 value, u32 mask) +{ + struct cmdq_instruction inst = { {0} }; + int err; + + if (mask != U32_MAX) { + inst.op = CMDQ_CODE_MASK; + inst.mask = ~mask; + err = cmdq_pkt_append_command(pkt, inst); + if (err < 0) + return err; + + inst.op = CMDQ_CODE_WRITE_S_MASK; + } else { + inst.op = CMDQ_CODE_WRITE_S; + } + + inst.sop = high_addr_reg_idx; + inst.offset = addr_low; + inst.value = value; + + return cmdq_pkt_append_command(pkt, inst); +} +EXPORT_SYMBOL(cmdq_pkt_write_s_value); + int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) { struct cmdq_instruction inst = { {0} }; diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h index 01b4184af310..fec292aac83c 100644 --- a/include/linux/soc/mediatek/mtk-cmdq.h +++ b/include/linux/soc/mediatek/mtk-cmdq.h @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low, int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low, u16 src_reg_idx, u32 mask); +/** + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ + * packet which write value to a physical address + * @pkt: the CMDQ packet + * @high_addr_reg_idx: internal regisger ID which contains high address of pa + * @addr_low: low address of pa + * @value: the specified target value + * @mask: the specified target mask + * + * Return: 0 for success; else the error code is returned + */ +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, + u16 addr_low, u32 value, u32 mask); + /** * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet * @pkt: the CMDQ packet