From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 00E5FC32792 for ; Tue, 1 Oct 2019 02:32:59 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C9CB72075C for ; Tue, 1 Oct 2019 02:32:58 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="UjxRIbB1" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C9CB72075C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=mediatek.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Date:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=LsiR3nLsWK9QqRLItZRfjvF8CsBMK2P9IMBuF2mTiDA=; b=UjxRIbB1kcEz/5 0y9iEuoKufOu5y1wePGFwbWbcHYsNY5MDF6QmwZIfBAOb2RpQE23wkNW/9+LzNPV+smHEFFYVtuZK MYk/H221W0lW1du3vKImbHQKZ33KQj2tFwd3MU5PH3Nw6PktmR09r/gDvg3I4d6ZEiSP+FHLEJ+JZ 80Z5wh4cgGA2hK/VzlGt1k03L/jo5AEmkTwhOwhLRit2O1inbDfoOXQrEP79wLoI0ulgfp9IQKsuu dnt2yZ9eiEEZbahg2Mu4cMIosL0RTzlLFR/UYeSjnhnDrOfgw3Qrde1dbjCvXPF6fTPPHQ5d6qQym u2K4Ylx2h1ulD7Zajhlw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.2 #3 (Red Hat Linux)) id 1iF7y2-0003hK-J7; Tue, 01 Oct 2019 02:32:46 +0000 Received: from mailgw01.mediatek.com ([216.200.240.184]) by bombadil.infradead.org with esmtps (Exim 4.92.2 #3 (Red Hat Linux)) id 1iF7xw-0003g6-JX; Tue, 01 Oct 2019 02:32:42 +0000 X-UUID: dfdd7984197b424ca20651afe5af993d-20190930 X-UUID: dfdd7984197b424ca20651afe5af993d-20190930 Received: from mtkcas68.mediatek.inc [(172.29.94.19)] by mailgw01.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLS) with ESMTP id 2099448664; Mon, 30 Sep 2019 18:32:27 -0800 Received: from mtkmbs05n1.mediatek.inc (172.21.101.15) by MTKMBS62N1.mediatek.inc (172.29.193.41) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Mon, 30 Sep 2019 19:32:26 -0700 Received: from mtkcas08.mediatek.inc (172.21.101.126) by mtkmbs05n1.mediatek.inc (172.21.101.15) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 1 Oct 2019 10:17:22 +0800 Received: from [172.21.77.4] (172.21.77.4) by mtkcas08.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.0.1395.4 via Frontend Transport; Tue, 1 Oct 2019 10:17:22 +0800 Message-ID: <1569896243.21654.8.camel@mtksdaap41> Subject: Re: [PATCH v15 1/4] soc: mediatek: cmdq: define the instruction struct From: CK Hu To: Bibby Hsieh Date: Tue, 1 Oct 2019 10:17:23 +0800 In-Reply-To: <20190927114254.6258-2-bibby.hsieh@mediatek.com> References: <20190927114254.6258-1-bibby.hsieh@mediatek.com> <20190927114254.6258-2-bibby.hsieh@mediatek.com> X-Mailer: Evolution 3.10.4-0ubuntu2 MIME-Version: 1.0 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190930_193240_649135_CDA09BA4 X-CRM114-Status: GOOD ( 18.40 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, Nicolas Boichat , srv_heupstream@mediatek.com, linux-kernel@vger.kernel.org, Houlong Wei , Dennis-YC Hsieh , Rob Herring , linux-mediatek@lists.infradead.org, Matthias Brugger , linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, Bibby: On Fri, 2019-09-27 at 19:42 +0800, Bibby Hsieh wrote: > Define an instruction structure for gce driver to append command. > This structure can make the client's code more readability. > > Signed-off-by: Bibby Hsieh > Reviewed-by: CK Hu You've modified this patch in this version, so you should drop this 'Reviewed-by' tag. > Reviewed-by: Houlong Wei > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 106 +++++++++++++++++------ > include/linux/mailbox/mtk-cmdq-mailbox.h | 10 +++ > 2 files changed, 90 insertions(+), 26 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c > index 7aa0517ff2f3..7af327b98d25 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -9,12 +9,24 @@ > #include > #include > > -#define CMDQ_ARG_A_WRITE_MASK 0xffff > #define CMDQ_WRITE_ENABLE_MASK BIT(0) > #define CMDQ_EOC_IRQ_EN BIT(0) > #define CMDQ_EOC_CMD ((u64)((CMDQ_CODE_EOC << CMDQ_OP_CODE_SHIFT)) \ > << 32 | CMDQ_EOC_IRQ_EN) > > +struct cmdq_instruction { > + union { > + u32 value; > + u32 mask; > + }; > + union { > + u16 offset; > + u16 event; > + }; > + u8 subsys; > + u8 op; > +}; > + > static void cmdq_client_timeout(struct timer_list *t) > { > struct cmdq_client *client = from_timer(client, t, timer); > @@ -110,10 +122,10 @@ void cmdq_pkt_destroy(struct cmdq_pkt *pkt) > } > EXPORT_SYMBOL(cmdq_pkt_destroy); > > -static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code, > - u32 arg_a, u32 arg_b) > +static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, > + struct cmdq_instruction *inst) > { > - u64 *cmd_ptr; > + struct cmdq_instruction *cmd_ptr; > > if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) { > /* > @@ -129,8 +141,9 @@ static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code, > __func__, (u32)pkt->buf_size); > return -ENOMEM; > } > + > cmd_ptr = pkt->va_base + pkt->cmd_buf_size; > - (*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b; > + *cmd_ptr = *inst; > pkt->cmd_buf_size += CMDQ_INST_SIZE; > > return 0; > @@ -138,24 +151,42 @@ static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code, > > int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value) > { > - u32 arg_a = (offset & CMDQ_ARG_A_WRITE_MASK) | > - (subsys << CMDQ_SUBSYS_SHIFT); > + struct cmdq_instruction *inst = kzalloc(sizeof(*inst), GFP_KERNEL); Frequently allocate/free increase CPU loading. The simpler way is struct cmdq_instruction inst = { 0 }; cmdq_pkt_append_command(pkt, &inst); > + int err = 0; No need to assign initial value. > + > + if (!inst) > + return -ENOMEM; > + > + inst->op = CMDQ_CODE_WRITE; > + inst->value = value; > + inst->offset = offset; > + inst->subsys = subsys; > > - return cmdq_pkt_append_command(pkt, CMDQ_CODE_WRITE, arg_a, value); > + err = cmdq_pkt_append_command(pkt, inst); > + kfree(inst); > + > + return err; > } > EXPORT_SYMBOL(cmdq_pkt_write); > [snip] > > static int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > { > - int err; > + struct cmdq_instruction *inst = kzalloc(sizeof(*inst), GFP_KERNEL); > + int err = 0; > + > + if (!inst) > + return -ENOMEM; > > /* insert EOC and generate IRQ for each command iteration */ > - err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN); > + inst->op = CMDQ_CODE_EOC; > + inst->value = CMDQ_EOC_IRQ_EN; > + err = cmdq_pkt_append_command(pkt, inst); > > /* JUMP to end */ > - err |= cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS); > + inst->op = CMDQ_CODE_JUMP; > + inst->value = CMDQ_JUMP_PASS; > + err |= cmdq_pkt_append_command(pkt, inst); OR the err value looks strange. If you OR err 0x1 and err 0x10, you would get the new err 0x11. How do you know that err 0x11 is the combination of 0x1 and 0x10? This bug seems exist in previous patch, so I would like you to fix this bug first and then apply this patch. Regards, CK > + kfree(inst); > > return err; > } > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h > index e6f54ef6698b..678760548791 100644 > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h > @@ -20,6 +20,16 @@ > #define CMDQ_WFE_WAIT BIT(15) > #define CMDQ_WFE_WAIT_VALUE 0x1 > > +/* > + * WFE arg_b > + * bit 0-11: wait value > + * bit 15: 1 - wait, 0 - no wait > + * bit 16-27: update value > + * bit 31: 1 - update, 0 - no update > + */ > +#define CMDQ_WFE_OPTION (CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | \ > + CMDQ_WFE_WAIT_VALUE) > + > /** cmdq event maximum */ > #define CMDQ_MAX_EVENT 0x3ff > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel