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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9D103C61DA4 for ; Mon, 13 Mar 2023 07:51:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229964AbjCMHvX (ORCPT ); Mon, 13 Mar 2023 03:51:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49068 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229648AbjCMHvV (ORCPT ); Mon, 13 Mar 2023 03:51:21 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4354E28D1D; Mon, 13 Mar 2023 00:51:20 -0700 (PDT) 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 D295F6112C; 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: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.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