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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 49F46C6FD19 for ; Mon, 13 Mar 2023 07:52:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=CF37B9GJEF5iafZuy1szkd2SnuO0EgpYu4Mth2F8A+4=; b=fe7AUGMqGZoxxt oi32NFGp5dXceVf9e6tRqZOUOLaDmVLLPb/V55d3Jt9bqhZt3Q6EiAl7wiowBtcuMVBBo0c15sW8Z +QxxJsSgUdA8Hc1s1cxZBacUxFyW/cmJgmoxuDjw9TFjrYyyymYAsRV0Rx/Xz2pbXn51e0/y65DKv xEJ0/eb9bIZODpxmXGde7rnkzLz5ctH82MgpETPg51Igwe3mgTUdrDX7ZwS37vLr87hrzjYXUJV26 Yml/SoSk1UElemwniRHj1niZDbH+V0AV5unnKdMURV+e0g6p8xliyRP3tUO4Fm217/bOafT3p53Er FHur8rJ50uK5g8J6izdw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pbcxz-004exf-Ul; Mon, 13 Mar 2023 07:51:36 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pbcxn-004et9-Rd for linux-arm-kernel@lists.infradead.org; Mon, 13 Mar 2023 07:51:31 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id D28A861124; Mon, 13 Mar 2023 07:51:19 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id AD111C433EF; Mon, 13 Mar 2023 07:51:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1678693879; bh=W0+XBjv+nrljobq/UfvhL1/USkBrUmYTR01ih/dZaiE=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=GtNR/gp4Et7JQ19cG1OhxKrtAbSQh4wVdLE9n7Xadw8FH1g9mO+kMMzuFRJaeYCT2 5UysR6IgAtEBJxhLtn4bk6RFwddPR1wBmdUO2r8znxGPDIRm1FderayEKHCtnYQzMN L1+4Lig/toHD9LkYi/NndLSv/NvehyIFe4ehjXrDw06VWreF63O7HNx2FH74hMOUyV vxjycwnkGa1hIxmZ2s1oxXFcsNYXi6cuuGU99ypf84PUVugwUETrf/gWC3Bx1QKza8 wcvWXnchebS6JWMlQTjTCaaBGhie8NWGQ0ihfPGuO2rxF8QfsrVThxxVTvZ+c4AbER oH/AFe507pZzw== Message-ID: Date: Mon, 13 Mar 2023 09:51:12 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [EXTERNAL] Re: [PATCH v3 3/6] soc: ti: pruss: Add pruss_cfg_read()/update() API To: Md Danish Anwar , MD Danish Anwar , "Andrew F. Davis" , Suman Anna , Vignesh Raghavendra , Mathieu Poirier , Bjorn Andersson , Santosh Shilimkar , Nishanth Menon Cc: linux-remoteproc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org, srk@ti.com, devicetree@vger.kernel.org, netdev@vger.kernel.org References: <20230306110934.2736465-1-danishanwar@ti.com> <20230306110934.2736465-4-danishanwar@ti.com> <7076208d-7dca-6980-5399-498e55648740@kernel.org> <367f6b50-e4cc-c3eb-e8e9-dabd4e044530@ti.com> <46415d8e-3c92-d489-3f44-01a586160082@kernel.org> <1c1e67fd-1eaa-30f5-8b2a-41a7e3ff664a@ti.com> Content-Language: en-US From: Roger Quadros In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230313_005124_270395_3826E1D2 X-CRM114-Status: GOOD ( 22.30 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 13/03/2023 07:01, Md Danish Anwar wrote: > Hi Roger > > On 11/03/23 17:36, Roger Quadros wrote: >> Hi Danish, >> >> On 10/03/2023 17:36, Md Danish Anwar wrote: >>> Hi Roger, >>> >>> On 10/03/23 18:53, Roger Quadros wrote: >>>> Hi Danish, >>>> >>>> On 10/03/2023 13:53, Md Danish Anwar wrote: >>>>> Hi Roger, >>>>> >>>>> On 09/03/23 17:00, Md Danish Anwar wrote: >>>>>> Hi Roger, >>>>>> >>>>>> On 08/03/23 17:12, Roger Quadros wrote: >>>>>>> >>>>>>> >>>>>>> On 08/03/2023 13:36, Md Danish Anwar wrote: >>>>>>>> Hi Roger, >>>>>>>> >>>>>>>> On 08/03/23 13:57, Roger Quadros wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On 06/03/2023 13:09, MD Danish Anwar wrote: >>>>>>>>>> From: Suman Anna >>>>>>>>>> >>>>>>>>>> Add two new generic API pruss_cfg_read() and pruss_cfg_update() to >>>>>>>>>> the PRUSS platform driver to allow other drivers to read and program >>>>>>>>>> respectively a register within the PRUSS CFG sub-module represented >>>>>>>>>> by a syscon driver. This interface provides a simple way for client >>>>>>>>> >>>>>>>>> Do you really need these 2 functions to be public? >>>>>>>>> I see that later patches (4-6) add APIs for doing specific things >>>>>>>>> and that should be sufficient than exposing entire CFG space via >>>>>>>>> pruss_cfg_read/update(). >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> I think the intention here is to keep this APIs pruss_cfg_read() and >>>>>>>> pruss_cfg_update() public so that other drivers can read / modify PRUSS config >>>>>>>> when needed. >>>>>>> >>>>>>> Where are these other drivers? If they don't exist then let's not make provision >>>>>>> for it now. >>>>>>> We can provide necessary API helpers when needed instead of letting client drivers >>>>>>> do what they want as they can be misused and hard to debug. >>>>>>> >>>>>> >>>>>> The ICSSG Ethernet driver uses pruss_cfg_update() API. It is posted upstream in >>>>>> the series [1]. The ethernet driver series is dependent on this series. In >>>>>> series [1] we are using pruss_cfg_update() in icssg_config.c file, >>>>>> icssg_config() API. >>>> >>>> You can instead add a new API on what exactly you want it to do rather than exposing >>>> entire CFG space. >>>> >>> >>> Sure. >>> >>> In icssg_config.c, a call to pruss_cfg_update() is made to enable XFR shift for >>> PRU and RTU, >>> >>> /* enable XFR shift for PRU and RTU */ >>> mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN; >>> pruss_cfg_update(prueth->pruss, PRUSS_CFG_SPP, mask, mask); >>> >>> I will add the below API as part of Patch 4 of the series. We'll call this API >>> and entire CFG space will not be exposed. >>> >>> /** >>> * pruss_cfg_xfr_pru_rtu_enable() - Enable/disable XFR shift for PRU and RTU >>> * @pruss: the pruss instance >>> * @enable: enable/disable >>> * >>> * Return: 0 on success, or an error code otherwise >>> */ >>> static inline int pruss_cfg_xfr_pru_rtu_enable(struct pruss *pruss, bool enable) >>> { >>> u32 mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN; >>> u32 set = enable ? mask : 0; >>> >>> return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set); >>> } >> >> I would suggest to make separate APIs for PRU XFR vs RTU XFR. >> > > How about making only one API for XFR shift and passing PRU or RTU as argument > to the API. The API along with struct pruss and bool enable will take another > argument u32 mask. > > mask = PRUSS_SPP_XFER_SHIFT_EN for PRU > mask = PRUSS_SPP_RTU_XFR_SHIFT_EN for RTU > mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN for PRU and RTU > > So one API will be able to do all three jobs. > > How does this seem? Yes, that is also fine. cheers, -roger _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel