<html><body><p> <pre> Hi CK, Thanks for the review. On Thu, 2024-11-21 at 07:10 +0000, CK Hu (胡俊光) wrote: > Hi, Jason: > > On Thu, 2024-11-21 at 14:38 +0800, CK Hu wrote: > > Hi, Jason: > > > > On Thu, 2024-11-21 at 12:25 +0800, Jason-JH.Lin wrote: > > > When GCE executes instructions, the corresponding hardware > > > register > > > can be found through the subsys ID. > > > For unsupported subsys ID hardware, the physical address need to > > > be used > > > to generate GCE instructions. > > > > > > Add the pa_base interface to the instruction programming flow for > > > these > > > unsupported subsys ID hardware. > > > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > > --- > > > > [snip] > > > > > -int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, > > > u32 value) > > > +int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u32 pa_base, > > > u16 offset, u32 value) > > > { > > > +struct cmdq_client *cl = (struct cmdq_client *)pkt->cl; > > > struct cmdq_instruction inst = { > > > .op = CMDQ_CODE_WRITE, > > > .value = value, > > > .offset = offset, > > > .subsys = subsys > > > }; > > > -return cmdq_pkt_append_command(pkt, inst); > > > +int err; > > > + > > > +if (!cl) { > > > +pr_err("%s %d: pkt->cl is NULL!\n", __func__, > > > __LINE__); > > > +return -EINVAL; > > > +} > > > + > > > +if (cmdq_subsys_is_valid(cl->chan, subsys)) { > > > > I would like to have a new API for no subsys. Maybe > > cmdq_pkt_write_pa(). > > If some client driver always have subsys, it could use > > cmdq_pkt_write(). > > If some client driver have no subsys, it could use > > cmdq_pkt_write_pa(). > > This would prevent frequently conditional jump in this function. > > If some client driver have subsys in some SoC and have no subsys in > > other SoC, > > let the conditional jump happen in that client driver. > > (The client driver could use 'likely' or 'unlikely' to uptimize) > > In the view point to let client driver have fine-grained control, > > maybe client could use cmdq_pkt_assign() and > > cmdq_pkt_write_s_value() to achieve this so it's not necessary to > > invent new API. > > For a client driver, the high address is usually a constant value. > So the client could have command like: > > cmdq_pkt_assign(pkt, 0, CMDQ_ADDR_HIGH(pa_base)); > > cmdq_pkt_write_s_value(pkt, 0, CMDQ_ADDR_LOW(offset1), value1); > cmdq_pkt_write_s_value(pkt, 0, CMDQ_ADDR_LOW(offset2), value2); > cmdq_pkt_write_s_value(pkt, 0, CMDQ_ADDR_LOW(offset3), value3); > cmdq_pkt_write_s_value(pkt, 0, CMDQ_ADDR_LOW(offset4), value4); > cmdq_pkt_write_s_value(pkt, 0, CMDQ_ADDR_LOW(offset5), value5); > cmdq_pkt_write_s_value(pkt, 0, CMDQ_ADDR_LOW(offset6), value6); > > This would reduce the command size. > GCE would execute more quickly. > > Regards, > CK > > > > > Regards, > > CK I think that might be a wide rage for changing all the client's code. But I'll try to implement that in client drivers and see if there is any problem and feedback to you. Regards, Jason-JH.Lin > > > > > +err = cmdq_pkt_append_command(pkt, inst); > > > +} else { > > > +err = cmdq_pkt_assign(pkt, 0, CMDQ_ADDR_HIGH(pa_base)); > > > +if (err < 0) > > > +return err; > > > + > > > +err = cmdq_pkt_write_s_value(pkt, 0, > > > CMDQ_ADDR_LOW(offset), value); > > > +} > > > + > > > +return err; > > > } > > > EXPORT_SYMBOL(cmdq_pkt_write); > > > > > > > </pre> </p></body></html><pre>************* MEDIATEK Confidentiality Notice ******************** The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you! </pre>