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=-10.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham 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 C630FC433B4 for ; Fri, 2 Apr 2021 00:50:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8DFB961104 for ; Fri, 2 Apr 2021 00:50:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233870AbhDBAum (ORCPT ); Thu, 1 Apr 2021 20:50:42 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38972 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233849AbhDBAul (ORCPT ); Thu, 1 Apr 2021 20:50:41 -0400 Received: from mail-pf1-x433.google.com (mail-pf1-x433.google.com [IPv6:2607:f8b0:4864:20::433]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 65A16C06178A for ; Thu, 1 Apr 2021 17:50:41 -0700 (PDT) Received: by mail-pf1-x433.google.com with SMTP id c204so2630401pfc.4 for ; Thu, 01 Apr 2021 17:50:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:content-transfer-encoding:in-reply-to:references :subject:from:cc:to:date:message-id:user-agent; bh=zdktSwchFSboaBqEuqtV3+3dzGZphF0+Oj1BTU7ioco=; b=BZvz8eqSSrf3c0ECoTcsde49xUb8HcYYXq9LoxwzYeeTKH4pNE9MNRmvio0+pZitx3 BiskEPtHTdlZawCF66kP6KqdNzyO2YL+HSd/fIHF6zSFQNtUeNDg0SP6lb0Qq1WV7aVJ QLR21CLgNNn5e4e+TzKHShB1tqfYXxmf90WtY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:content-transfer-encoding :in-reply-to:references:subject:from:cc:to:date:message-id :user-agent; bh=zdktSwchFSboaBqEuqtV3+3dzGZphF0+Oj1BTU7ioco=; b=T+HKqrPSzwkksPvfxFexsruG7EIkyQL/5h+3XGElVS5B/fVHjSZs7CuwH/eevsO/zL ipJR3NSo8KF2yQnPNxt71PJDLrrshPsRNIMJvJqyVow5F9Yz2OT8wP4631s/PiuP/xg/ wTN1w7jdNiLS+YWADTucDmPNytD/1ujisZYEdSa/yNR3zFYphtt+2VEoFpWQ/ySvVdLs NTAV9JTiQql4z6OITBzNYvbHhrb8TIK8QxCINT8Z8xwD7Z8+88mYaO+ROehapFaSU738 kxDvD9C1CkcL8UF0mVSPvFYCgI3uDgmzDYXLC7AIHjHFNRRNUaZKk1O5cwx3IaDOTw/L 75Cw== X-Gm-Message-State: AOAM531502dTpDIDdypMelDJWQ72xSiVrd1W5joVJFL/DkNpVe2oXaUD 8UHV4/4qtxXWKdKiRj1+UbJk56fehmnvxA== X-Google-Smtp-Source: ABdhPJwKwvpel8DWpoky99VHW3S9BdUssReEoPqpOgNcTckoZYDEcEmMak13iPP/AjC+LOHroYI14g== X-Received: by 2002:a63:5a48:: with SMTP id k8mr9830070pgm.308.1617324640692; Thu, 01 Apr 2021 17:50:40 -0700 (PDT) Received: from chromium.org ([2620:15c:202:201:450:f2a9:b3ca:879f]) by smtp.gmail.com with ESMTPSA id t6sm6454560pjs.26.2021.04.01.17.50.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Apr 2021 17:50:39 -0700 (PDT) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable In-Reply-To: <326da78181b566ee7d48683d68bb89d7@codeaurora.org> References: <161704834593.3012082.17486072850156076295@swboyd.mtv.corp.google.com> <326da78181b566ee7d48683d68bb89d7@codeaurora.org> Subject: Re: [PATCH V2 2/5] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC) From: Stephen Boyd Cc: Andy Gross , Bjorn Andersson , Rob Herring , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, Sai Prakash Ranjan , Sibi Sankar , Rajendra Nayak , vkoul@kernel.org To: schowdhu@codeaurora.org Date: Thu, 01 Apr 2021 17:50:38 -0700 Message-ID: <161732463842.2260335.16117110131113613939@swboyd.mtv.corp.google.com> User-Agent: alot/0.9.1 Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org Quoting schowdhu@codeaurora.org (2021-04-01 07:04:07) > On 2021-03-30 01:35, Stephen Boyd wrote: > > Quoting Souradeep Chowdhury (2021-03-25 01:02:33) > >> diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c > >> new file mode 100644 > >> index 0000000..a55d8ca7 > >> --- /dev/null > >> +++ b/drivers/soc/qcom/dcc.c > >> @@ -0,0 +1,1549 @@ [..] > >=20 > >> + void __iomem *base; > >> + u32 reg_size; > >> + struct device *dev; > >> + struct mutex mutex; > >=20 > > In particular what this mutex is protecting. >=20 > Ack. The mutex is used to protect the access as well as manipulation of=20 > the main instance of dcc_drvdata structure > initialized during probe time. This structure contains the useful driver = > data information and is set using the call > platform_set_drvdata(pdev, drvdata) which links this data to the=20 > platform device and hence needs to be protected via > mutex locks. The same convention is followed across other similar=20 > drivers exposing userspace like the llcc driver. The region that the mutex is protecting seems quite large. That's probably because I don't understand the driver. > >=20 > >> + > >> + mutex_lock(&drvdata->mutex); > >> + > >> + for (curr_list =3D 0; curr_list < drvdata->nr_link_list;=20 > >> curr_list++) { > >> + if (!drvdata->enable[curr_list]) > >> + continue; > >> + ll_cfg =3D dcc_readl(drvdata, DCC_LL_CFG(curr_list)); > >> + tmp_ll_cfg =3D ll_cfg & ~BIT(9); > >> + dcc_writel(drvdata, tmp_ll_cfg,=20 > >> DCC_LL_CFG(curr_list)); > >> + dcc_writel(drvdata, 1, DCC_LL_SW_TRIGGER(curr_list)); > >> + dcc_writel(drvdata, ll_cfg, DCC_LL_CFG(curr_list)); > >> + } > >=20 > > Does the mutex need to be held while waiting for ready? >=20 > Yes, to maintain consistency because inside the dcc_ready function,=20 > there is access to dcc_drvdata structure set > on the platform device. Is the drvdata going to be modified somewhere else? > >> + > >> + dev_info(drvdata->dev, "All values written to=20 > >> enable.\n"); > >=20 > > Debug print? >=20 > Ack >=20 > >=20 > >> + /* Make sure all config is written in sram */ > >> + mb(); > >=20 > > This won't work as intended. >=20 > This was called to prevent instruction reordering if the driver runs on=20 > multiple > CPU cores. As the hardware manipulation has to be done sequentially=20 > before the > trigger is set. Kindly let me know the concern in this case. Device I/O with the proper accessors is sequential even if the process moves to a different CPU. Is that what you're worried about? The comment says "make sure it is written to sram", which should be achieved by reading some register back from the device after all the writes so that the driver knows the writes have been posted to the device. I believe this mb() is doing nothing. >=20 > >=20 > >> + > >> + drvdata->enable[list] =3D true; > >> + > >> + /* 5. Configure trigger */ > >> + dcc_writel(drvdata, BIT(9), DCC_LL_CFG(list)); > >> + } > >> + > >> +err: > >> + mutex_unlock(&drvdata->mutex); > >> + return ret; > >> +} > >> + > >> +static void dcc_disable(struct dcc_drvdata *drvdata) > >> +{ > >> + int curr_list; > >> + > >> + mutex_lock(&drvdata->mutex); > >> + > >> + if (!dcc_ready(drvdata)) > >> + dev_err(drvdata->dev, "DCC is not ready Disabling=20 > >> DCC...\n"); > >=20 > > Is that two sentences? And a debug print? >=20 > Ack. >=20 > >=20 > >> + > >> + for (curr_list =3D 0; curr_list < drvdata->nr_link_list;=20 > >> curr_list++) { > >> + if (!drvdata->enable[curr_list]) > >> + continue; > >> + dcc_writel(drvdata, 0, DCC_LL_CFG(curr_list)); > >> + dcc_writel(drvdata, 0, DCC_LL_BASE(curr_list)); > >> + dcc_writel(drvdata, 0, DCC_FD_BASE(curr_list)); > >> + dcc_writel(drvdata, 0, DCC_LL_LOCK(curr_list)); > >> + drvdata->enable[curr_list] =3D false; > >> + } > >> + memset_io(drvdata->ram_base, 0, drvdata->ram_size); > >> + drvdata->ram_cfg =3D 0; > >> + drvdata->ram_start =3D 0; > >> + mutex_unlock(&drvdata->mutex); > >> +} > >> + > >> +static ssize_t curr_list_show(struct device *dev, > >> + struct device_attribute *attr, char *buf) > >> +{ > >> + int ret; > >> + struct dcc_drvdata *drvdata =3D dev_get_drvdata(dev); > >> + > >> + mutex_lock(&drvdata->mutex); > >> + if (drvdata->curr_list =3D=3D DCC_INVALID_LINK_LIST) { > >> + dev_err(dev, "curr_list is not set.\n"); > >> + ret =3D -EINVAL; > >> + goto err; > >> + } > >> + > >> + ret =3D scnprintf(buf, PAGE_SIZE, "%d\n", drvdata->curr_list); > >> +err: > >> + mutex_unlock(&drvdata->mutex); > >> + return ret; > >> +} > >> + > >> +static ssize_t curr_list_store(struct device *dev, > >> + struct=20 > >> device_attribute *attr, > >> + const char *buf,=20 > >> size_t size) > >> +{ > >> + struct dcc_drvdata *drvdata =3D dev_get_drvdata(dev); > >> + unsigned long val; > >> + u32 lock_reg; > >> + bool dcc_enable =3D false; > >> + > >> + if (kstrtoul(buf, 16, &val)) > >> + return -EINVAL; > >> + > >> + if (val >=3D drvdata->nr_link_list) > >> + return -EINVAL; > >> + > >> + mutex_lock(&drvdata->mutex); > >> + > >> + dcc_enable =3D is_dcc_enabled(drvdata); > >> + if (drvdata->curr_list !=3D DCC_INVALID_LINK_LIST && dcc_enabl= e)=20 > >> { > >> + dev_err(drvdata->dev, "DCC is enabled, please disable = > >> it first.\n"); > >> + mutex_unlock(&drvdata->mutex); > >> + return -EINVAL; > >> + } > >> + > >> + lock_reg =3D dcc_readl(drvdata, DCC_LL_LOCK(val)); > >> + if (lock_reg & 0x1) { > >> + dev_err(drvdata->dev, "DCC linked list is already=20 > >> configured\n"); > >> + mutex_unlock(&drvdata->mutex); > >> + return -EINVAL; > >> + } > >> + drvdata->curr_list =3D val; > >> + mutex_unlock(&drvdata->mutex); > >> + > >> + return size; > >> +} > >> + > >> +static DEVICE_ATTR_RW(curr_list); > >> + > >> + > >> +static ssize_t trigger_store(struct device *dev, > >> + struct device_attribute *attr, > >> + const char *buf, size_t size) > >> +{ > >> + int ret =3D 0; > >> + unsigned long val; > >> + struct dcc_drvdata *drvdata =3D dev_get_drvdata(dev); > >> + > >> + if (kstrtoul(buf, 16, &val)) > >> + return -EINVAL; > >> + if (val !=3D 1) > >> + return -EINVAL; > >> + > >> + ret =3D dcc_sw_trigger(drvdata); > >> + if (!ret) > >> + ret =3D size; > >> + > >> + return ret; > >> +} > >> +static DEVICE_ATTR_WO(trigger); > >> + > >> +static ssize_t enable_show(struct device *dev, > >> + struct device_attribute *attr, char *buf) > >> +{ > >> + int ret; > >> + bool dcc_enable =3D false; > >> + struct dcc_drvdata *drvdata =3D dev_get_drvdata(dev); > >> + > >> + mutex_lock(&drvdata->mutex); > >> + if (drvdata->curr_list >=3D drvdata->nr_link_list) { > >> + dev_err(dev, "Select link list to program using=20 > >> curr_list\n"); > >> + ret =3D -EINVAL; > >> + goto err; > >> + } > >> + > >> + dcc_enable =3D is_dcc_enabled(drvdata); > >> + > >> + ret =3D scnprintf(buf, PAGE_SIZE, "%u\n", > >> + (unsigned int)dcc_enable); > >> +err: > >> + mutex_unlock(&drvdata->mutex); > >=20 > > What does the mutex being held serve here? >=20 > As mentioned earlier, the mutex has been used while accessing=20 > dcc_drvdata structure. >=20 And what purpose does it serve? I suppose curr_list can be modified? But then when this function returns it could be disabled before userspace sees the value so I'm still lost why we care to hold the lock this long. > >=20 > >> + return ret; > >> +} > >> + > >> +static ssize_t enable_store(struct device *dev, > >> + struct device_attribute *attr, > >> + const char *buf, size_t size) > >> +{ > >> + int ret =3D 0; > >> + unsigned long val; > >> + struct dcc_drvdata *drvdata =3D dev_get_drvdata(dev); > >> + > >> + if (kstrtoul(buf, 16, &val)) > >> + return -EINVAL; > >> + > >> + if (val) > >> + ret =3D dcc_enable(drvdata); > >> + else > >> + dcc_disable(drvdata); > >> + > >> + if (!ret) > >> + ret =3D size; > >> + > >> + return ret; > >> + > >> +} > >> + > >> +static DEVICE_ATTR_RW(enable); > >> + > >> +static ssize_t config_show(struct device *dev, > >> + struct device_attribute *attr, char *buf) > >> +{ > >> + struct dcc_drvdata *drvdata =3D dev_get_drvdata(dev); > >> + struct dcc_config_entry *entry; > >> + char local_buf[64]; > >> + int len =3D 0, count =3D 0; > >> + > >> + buf[0] =3D '\0'; > >=20 > > Why? >=20 > The strlcat function is used here to concatenate the buffer with the=20 > config values. > The strlcat function in C needs a NULL terminated string both as it's=20 > source and > destination. That's why this has been initialized with NULL termination. >=20 sysfs files shall be one value per file, i.e. something that a machine reads. This function looks like a debugfs function. >=20 > >=20 > >> + > >> + mutex_lock(&drvdata->mutex); > >> + if (drvdata->curr_list >=3D drvdata->nr_link_list) { > >> + dev_err(dev, "Select link list to program using=20 > >> curr_list\n"); > >> + count =3D -EINVAL; > >> + goto err; > >> + } > >> + > >> + list_for_each_entry(entry, > >> + &drvdata->cfg_head[drvdata->curr_list], list) { > >> + switch (entry->desc_type) { > >> + case DCC_READ_WRITE_TYPE: > >> + len =3D snprintf(local_buf, 64, "Index: 0x%x, = > >> mask: 0x%x, val: 0x%x\n", > >> + entry->index, entry->mask,=20 > >> entry->write_val); > >> + break; > >> + case DCC_LOOP_TYPE: > >> + len =3D snprintf(local_buf, 64, "Index: 0x%x, = > >> Loop: %d\n", > >> + entry->index, entry->loop_cnt); > >> + break; > >> + case DCC_WRITE_TYPE: > >> + len =3D snprintf(local_buf, 64, > >> + "Write Index: 0x%x, Base: 0x%x,=20 > >> Offset: 0x%x, len: 0x%x APB: %d\n", > >> + entry->index, entry->base,=20 > >> entry->offset, entry->len, > >> + entry->apb_bus); > >> + break; > >> + default: > >> + len =3D snprintf(local_buf, 64, > >> + "Read Index: 0x%x, Base: 0x%x, Offset:= =20 > >> 0x%x, len: 0x%x APB: %d\n", > >> + entry->index, entry->base,=20 > >> entry->offset, > >> + entry->len, entry->apb_bus); > >> + } > >> + > >> + if ((count + len) > PAGE_SIZE) { > >> + dev_err(dev, "DCC: Couldn't write complete=20 > >> config\n"); > >> + break; > >> + } > >> + strlcat(buf, local_buf, PAGE_SIZE); > >> + count +=3D len; > >> + } > >> + > >> +err: > >> + mutex_unlock(&drvdata->mutex); > >> + return count; > >> +} > >=20 > >> + > >> + /* EOF check */ > >> + if (drvdata->ram_size <=3D *ppos) > >> + return 0; > >> + > >> + if ((*ppos + len) > drvdata->ram_size) > >> + len =3D (drvdata->ram_size - *ppos); > >> + > >> + buf =3D kzalloc(len, GFP_KERNEL); > >> + if (!buf) > >> + return -ENOMEM; > >> + > >> + memcpy_fromio(buf, drvdata->ram_base + *ppos, len); > >> + > >> + if (copy_to_user(data, buf, len)) { > >=20 > > Is there any sort of memcpy_fromio_to_user() API? That would avoid the > > extra buffer allocation by copying to userspace in the readl loop. >=20 > No. For directly copying io data to userspace we need to use direct I/O=20 > which is used for > special cases like tape drivers. In this case the complexity of using it = > outweighs it's > advantages. Also this is a fixed transfer of data in the form of=20 > dcc_sram content rather > than bulk transfers. Tape drivers? Huh? Can you please look into adding a memcpy_fromio_to_user() API that does this without allocating memory for a buffer? >=20 > >=20 > >> + dcc->loopoff =3D DCC_FIX_LOOP_OFFSET; > >> + else > >> + dcc->loopoff =3D get_bitmask_order((dcc->ram_size + > >> + dcc->ram_offset) / 4 - 1); > >> + > >> + mutex_init(&dcc->mutex); > >> + dcc->enable =3D devm_kcalloc(dev, dcc->nr_link_list, > >> + sizeof(bool), GFP_KERNEL); > >> + if (!dcc->enable) > >> + return -ENOMEM; > >> + > >> + dcc->configured =3D devm_kcalloc(dev, dcc->nr_link_list, > >> + sizeof(bool), GFP_KERNEL); > >> + if (!dcc->configured) > >> + return -ENOMEM; > >> + > >> + dcc->nr_config =3D devm_kcalloc(dev, dcc->nr_link_list, > >> + sizeof(u32), GFP_KERNEL); > >> + if (!dcc->nr_config) > >> + return -ENOMEM; > >> + > >> + dcc->cfg_head =3D devm_kcalloc(dev, dcc->nr_link_list, > >> + sizeof(struct list_head), GFP_KERNEL); > >> + if (!dcc->cfg_head) > >> + return -ENOMEM; > >=20 > > These are a lot of allocations. Any chance we can do one instead of=20 > > this > > many? >=20 > All these variable have predefined requirement of sizes > so they need to be allocated separately. Gather requirements, do some addition, and then allocate one chunk of memory? 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.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham 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 D1A38C433ED for ; Fri, 2 Apr 2021 00:56:52 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (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 419EB610A5 for ; Fri, 2 Apr 2021 00:56:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 419EB610A5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+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=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Message-ID:Date:To:Cc:From:Subject:References: In-Reply-To:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=mbuJXuWdjS0w9B+9iujST08gDZcWkm/giytPw0WdO0U=; b=kOn+Ej68/ZPyPtYqN+RRD2M4f 1Z+BfTFmIVIicRAsKW/DOgWfhLsiNNtZxzdn2Ll66gwqx5NiLyeEFpxg6BAr1sNXgHS3IxgNsbqv7 wA/byX4vVGFWHjt4PIyp8d64s6/qjRYP3iAbNlRScpB+sUGOtLYVaPV2JJq2A4ThjducULPIzTnJT jGd4xuUuEZhF+cgkklN11Q2fip+xw/PFGm8574hP7AhiEB2TpUK0+2uS00qjrJnKic7IdAx+DByNj QES2hEOI0Ljj3lGiai07UrnE38miimyMUoXTrEyr5bOsRQatyz3rHrXcyIhODgLDH1RNOowduie7H fJMKsCEzw==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lS81P-00BYF9-Qr; Fri, 02 Apr 2021 00:50:48 +0000 Received: from mail-pf1-x42b.google.com ([2607:f8b0:4864:20::42b]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lS81K-00BYDp-QY for linux-arm-kernel@lists.infradead.org; Fri, 02 Apr 2021 00:50:45 +0000 Received: by mail-pf1-x42b.google.com with SMTP id v10so2628811pfn.5 for ; Thu, 01 Apr 2021 17:50:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:content-transfer-encoding:in-reply-to:references :subject:from:cc:to:date:message-id:user-agent; bh=zdktSwchFSboaBqEuqtV3+3dzGZphF0+Oj1BTU7ioco=; b=BZvz8eqSSrf3c0ECoTcsde49xUb8HcYYXq9LoxwzYeeTKH4pNE9MNRmvio0+pZitx3 BiskEPtHTdlZawCF66kP6KqdNzyO2YL+HSd/fIHF6zSFQNtUeNDg0SP6lb0Qq1WV7aVJ QLR21CLgNNn5e4e+TzKHShB1tqfYXxmf90WtY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:content-transfer-encoding :in-reply-to:references:subject:from:cc:to:date:message-id :user-agent; bh=zdktSwchFSboaBqEuqtV3+3dzGZphF0+Oj1BTU7ioco=; b=kZmuhDdGYoEuoZ3M6l0uLF/B8c34hFIe6BwlyF+DnnWGRrS/7fwTgOyyUbo1mMYy25 MB8YYFVC0MRK1MybMBZZixAPjIyy7x65L7q0/AKykPTp4zch9LElJSAwkXwVV6839lTy tvLT0yZV1Xx/TJ54KkyBn6BFsdEAx1MfWkamV5ccRaYIVbUqqih9S9rIM/Zzqc741HZ8 KBnibcVoyGEuWXvYGdKeREnPsr1wyrhQassofVfrGRvFlWrHvnSyVaQMJbWOFG1MNbxX 1HchpmgfX4bTfDkLDWqKbV95CW20XtmWDM029QVatGgm8cbRyKoM9xvX9YGsKLUlFKSW BiBA== X-Gm-Message-State: AOAM532lzWcwln6DLnB81DKRPqzghgOLOgw55E9cIKIWiTPtZdN4N7eh hurt7HpR5RzSj742kwyhgYj6SA== X-Google-Smtp-Source: ABdhPJwKwvpel8DWpoky99VHW3S9BdUssReEoPqpOgNcTckoZYDEcEmMak13iPP/AjC+LOHroYI14g== X-Received: by 2002:a63:5a48:: with SMTP id k8mr9830070pgm.308.1617324640692; Thu, 01 Apr 2021 17:50:40 -0700 (PDT) Received: from chromium.org ([2620:15c:202:201:450:f2a9:b3ca:879f]) by smtp.gmail.com with ESMTPSA id t6sm6454560pjs.26.2021.04.01.17.50.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Apr 2021 17:50:39 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <326da78181b566ee7d48683d68bb89d7@codeaurora.org> References: <161704834593.3012082.17486072850156076295@swboyd.mtv.corp.google.com> <326da78181b566ee7d48683d68bb89d7@codeaurora.org> Subject: Re: [PATCH V2 2/5] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC) From: Stephen Boyd Cc: Andy Gross , Bjorn Andersson , Rob Herring , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, Sai Prakash Ranjan , Sibi Sankar , Rajendra Nayak , vkoul@kernel.org To: schowdhu@codeaurora.org Date: Thu, 01 Apr 2021 17:50:38 -0700 Message-ID: <161732463842.2260335.16117110131113613939@swboyd.mtv.corp.google.com> User-Agent: alot/0.9.1 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210402_015043_171190_2A6E70F9 X-CRM114-Status: GOOD ( 40.87 ) 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 Quoting schowdhu@codeaurora.org (2021-04-01 07:04:07) > On 2021-03-30 01:35, Stephen Boyd wrote: > > Quoting Souradeep Chowdhury (2021-03-25 01:02:33) > >> diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c > >> new file mode 100644 > >> index 0000000..a55d8ca7 > >> --- /dev/null > >> +++ b/drivers/soc/qcom/dcc.c > >> @@ -0,0 +1,1549 @@ [..] > > > >> + void __iomem *base; > >> + u32 reg_size; > >> + struct device *dev; > >> + struct mutex mutex; > > > > In particular what this mutex is protecting. > > Ack. The mutex is used to protect the access as well as manipulation of > the main instance of dcc_drvdata structure > initialized during probe time. This structure contains the useful driver > data information and is set using the call > platform_set_drvdata(pdev, drvdata) which links this data to the > platform device and hence needs to be protected via > mutex locks. The same convention is followed across other similar > drivers exposing userspace like the llcc driver. The region that the mutex is protecting seems quite large. That's probably because I don't understand the driver. > > > >> + > >> + mutex_lock(&drvdata->mutex); > >> + > >> + for (curr_list = 0; curr_list < drvdata->nr_link_list; > >> curr_list++) { > >> + if (!drvdata->enable[curr_list]) > >> + continue; > >> + ll_cfg = dcc_readl(drvdata, DCC_LL_CFG(curr_list)); > >> + tmp_ll_cfg = ll_cfg & ~BIT(9); > >> + dcc_writel(drvdata, tmp_ll_cfg, > >> DCC_LL_CFG(curr_list)); > >> + dcc_writel(drvdata, 1, DCC_LL_SW_TRIGGER(curr_list)); > >> + dcc_writel(drvdata, ll_cfg, DCC_LL_CFG(curr_list)); > >> + } > > > > Does the mutex need to be held while waiting for ready? > > Yes, to maintain consistency because inside the dcc_ready function, > there is access to dcc_drvdata structure set > on the platform device. Is the drvdata going to be modified somewhere else? > >> + > >> + dev_info(drvdata->dev, "All values written to > >> enable.\n"); > > > > Debug print? > > Ack > > > > >> + /* Make sure all config is written in sram */ > >> + mb(); > > > > This won't work as intended. > > This was called to prevent instruction reordering if the driver runs on > multiple > CPU cores. As the hardware manipulation has to be done sequentially > before the > trigger is set. Kindly let me know the concern in this case. Device I/O with the proper accessors is sequential even if the process moves to a different CPU. Is that what you're worried about? The comment says "make sure it is written to sram", which should be achieved by reading some register back from the device after all the writes so that the driver knows the writes have been posted to the device. I believe this mb() is doing nothing. > > > > >> + > >> + drvdata->enable[list] = true; > >> + > >> + /* 5. Configure trigger */ > >> + dcc_writel(drvdata, BIT(9), DCC_LL_CFG(list)); > >> + } > >> + > >> +err: > >> + mutex_unlock(&drvdata->mutex); > >> + return ret; > >> +} > >> + > >> +static void dcc_disable(struct dcc_drvdata *drvdata) > >> +{ > >> + int curr_list; > >> + > >> + mutex_lock(&drvdata->mutex); > >> + > >> + if (!dcc_ready(drvdata)) > >> + dev_err(drvdata->dev, "DCC is not ready Disabling > >> DCC...\n"); > > > > Is that two sentences? And a debug print? > > Ack. > > > > >> + > >> + for (curr_list = 0; curr_list < drvdata->nr_link_list; > >> curr_list++) { > >> + if (!drvdata->enable[curr_list]) > >> + continue; > >> + dcc_writel(drvdata, 0, DCC_LL_CFG(curr_list)); > >> + dcc_writel(drvdata, 0, DCC_LL_BASE(curr_list)); > >> + dcc_writel(drvdata, 0, DCC_FD_BASE(curr_list)); > >> + dcc_writel(drvdata, 0, DCC_LL_LOCK(curr_list)); > >> + drvdata->enable[curr_list] = false; > >> + } > >> + memset_io(drvdata->ram_base, 0, drvdata->ram_size); > >> + drvdata->ram_cfg = 0; > >> + drvdata->ram_start = 0; > >> + mutex_unlock(&drvdata->mutex); > >> +} > >> + > >> +static ssize_t curr_list_show(struct device *dev, > >> + struct device_attribute *attr, char *buf) > >> +{ > >> + int ret; > >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev); > >> + > >> + mutex_lock(&drvdata->mutex); > >> + if (drvdata->curr_list == DCC_INVALID_LINK_LIST) { > >> + dev_err(dev, "curr_list is not set.\n"); > >> + ret = -EINVAL; > >> + goto err; > >> + } > >> + > >> + ret = scnprintf(buf, PAGE_SIZE, "%d\n", drvdata->curr_list); > >> +err: > >> + mutex_unlock(&drvdata->mutex); > >> + return ret; > >> +} > >> + > >> +static ssize_t curr_list_store(struct device *dev, > >> + struct > >> device_attribute *attr, > >> + const char *buf, > >> size_t size) > >> +{ > >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev); > >> + unsigned long val; > >> + u32 lock_reg; > >> + bool dcc_enable = false; > >> + > >> + if (kstrtoul(buf, 16, &val)) > >> + return -EINVAL; > >> + > >> + if (val >= drvdata->nr_link_list) > >> + return -EINVAL; > >> + > >> + mutex_lock(&drvdata->mutex); > >> + > >> + dcc_enable = is_dcc_enabled(drvdata); > >> + if (drvdata->curr_list != DCC_INVALID_LINK_LIST && dcc_enable) > >> { > >> + dev_err(drvdata->dev, "DCC is enabled, please disable > >> it first.\n"); > >> + mutex_unlock(&drvdata->mutex); > >> + return -EINVAL; > >> + } > >> + > >> + lock_reg = dcc_readl(drvdata, DCC_LL_LOCK(val)); > >> + if (lock_reg & 0x1) { > >> + dev_err(drvdata->dev, "DCC linked list is already > >> configured\n"); > >> + mutex_unlock(&drvdata->mutex); > >> + return -EINVAL; > >> + } > >> + drvdata->curr_list = val; > >> + mutex_unlock(&drvdata->mutex); > >> + > >> + return size; > >> +} > >> + > >> +static DEVICE_ATTR_RW(curr_list); > >> + > >> + > >> +static ssize_t trigger_store(struct device *dev, > >> + struct device_attribute *attr, > >> + const char *buf, size_t size) > >> +{ > >> + int ret = 0; > >> + unsigned long val; > >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev); > >> + > >> + if (kstrtoul(buf, 16, &val)) > >> + return -EINVAL; > >> + if (val != 1) > >> + return -EINVAL; > >> + > >> + ret = dcc_sw_trigger(drvdata); > >> + if (!ret) > >> + ret = size; > >> + > >> + return ret; > >> +} > >> +static DEVICE_ATTR_WO(trigger); > >> + > >> +static ssize_t enable_show(struct device *dev, > >> + struct device_attribute *attr, char *buf) > >> +{ > >> + int ret; > >> + bool dcc_enable = false; > >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev); > >> + > >> + mutex_lock(&drvdata->mutex); > >> + if (drvdata->curr_list >= drvdata->nr_link_list) { > >> + dev_err(dev, "Select link list to program using > >> curr_list\n"); > >> + ret = -EINVAL; > >> + goto err; > >> + } > >> + > >> + dcc_enable = is_dcc_enabled(drvdata); > >> + > >> + ret = scnprintf(buf, PAGE_SIZE, "%u\n", > >> + (unsigned int)dcc_enable); > >> +err: > >> + mutex_unlock(&drvdata->mutex); > > > > What does the mutex being held serve here? > > As mentioned earlier, the mutex has been used while accessing > dcc_drvdata structure. > And what purpose does it serve? I suppose curr_list can be modified? But then when this function returns it could be disabled before userspace sees the value so I'm still lost why we care to hold the lock this long. > > > >> + return ret; > >> +} > >> + > >> +static ssize_t enable_store(struct device *dev, > >> + struct device_attribute *attr, > >> + const char *buf, size_t size) > >> +{ > >> + int ret = 0; > >> + unsigned long val; > >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev); > >> + > >> + if (kstrtoul(buf, 16, &val)) > >> + return -EINVAL; > >> + > >> + if (val) > >> + ret = dcc_enable(drvdata); > >> + else > >> + dcc_disable(drvdata); > >> + > >> + if (!ret) > >> + ret = size; > >> + > >> + return ret; > >> + > >> +} > >> + > >> +static DEVICE_ATTR_RW(enable); > >> + > >> +static ssize_t config_show(struct device *dev, > >> + struct device_attribute *attr, char *buf) > >> +{ > >> + struct dcc_drvdata *drvdata = dev_get_drvdata(dev); > >> + struct dcc_config_entry *entry; > >> + char local_buf[64]; > >> + int len = 0, count = 0; > >> + > >> + buf[0] = '\0'; > > > > Why? > > The strlcat function is used here to concatenate the buffer with the > config values. > The strlcat function in C needs a NULL terminated string both as it's > source and > destination. That's why this has been initialized with NULL termination. > sysfs files shall be one value per file, i.e. something that a machine reads. This function looks like a debugfs function. > > > > >> + > >> + mutex_lock(&drvdata->mutex); > >> + if (drvdata->curr_list >= drvdata->nr_link_list) { > >> + dev_err(dev, "Select link list to program using > >> curr_list\n"); > >> + count = -EINVAL; > >> + goto err; > >> + } > >> + > >> + list_for_each_entry(entry, > >> + &drvdata->cfg_head[drvdata->curr_list], list) { > >> + switch (entry->desc_type) { > >> + case DCC_READ_WRITE_TYPE: > >> + len = snprintf(local_buf, 64, "Index: 0x%x, > >> mask: 0x%x, val: 0x%x\n", > >> + entry->index, entry->mask, > >> entry->write_val); > >> + break; > >> + case DCC_LOOP_TYPE: > >> + len = snprintf(local_buf, 64, "Index: 0x%x, > >> Loop: %d\n", > >> + entry->index, entry->loop_cnt); > >> + break; > >> + case DCC_WRITE_TYPE: > >> + len = snprintf(local_buf, 64, > >> + "Write Index: 0x%x, Base: 0x%x, > >> Offset: 0x%x, len: 0x%x APB: %d\n", > >> + entry->index, entry->base, > >> entry->offset, entry->len, > >> + entry->apb_bus); > >> + break; > >> + default: > >> + len = snprintf(local_buf, 64, > >> + "Read Index: 0x%x, Base: 0x%x, Offset: > >> 0x%x, len: 0x%x APB: %d\n", > >> + entry->index, entry->base, > >> entry->offset, > >> + entry->len, entry->apb_bus); > >> + } > >> + > >> + if ((count + len) > PAGE_SIZE) { > >> + dev_err(dev, "DCC: Couldn't write complete > >> config\n"); > >> + break; > >> + } > >> + strlcat(buf, local_buf, PAGE_SIZE); > >> + count += len; > >> + } > >> + > >> +err: > >> + mutex_unlock(&drvdata->mutex); > >> + return count; > >> +} > > > >> + > >> + /* EOF check */ > >> + if (drvdata->ram_size <= *ppos) > >> + return 0; > >> + > >> + if ((*ppos + len) > drvdata->ram_size) > >> + len = (drvdata->ram_size - *ppos); > >> + > >> + buf = kzalloc(len, GFP_KERNEL); > >> + if (!buf) > >> + return -ENOMEM; > >> + > >> + memcpy_fromio(buf, drvdata->ram_base + *ppos, len); > >> + > >> + if (copy_to_user(data, buf, len)) { > > > > Is there any sort of memcpy_fromio_to_user() API? That would avoid the > > extra buffer allocation by copying to userspace in the readl loop. > > No. For directly copying io data to userspace we need to use direct I/O > which is used for > special cases like tape drivers. In this case the complexity of using it > outweighs it's > advantages. Also this is a fixed transfer of data in the form of > dcc_sram content rather > than bulk transfers. Tape drivers? Huh? Can you please look into adding a memcpy_fromio_to_user() API that does this without allocating memory for a buffer? > > > > >> + dcc->loopoff = DCC_FIX_LOOP_OFFSET; > >> + else > >> + dcc->loopoff = get_bitmask_order((dcc->ram_size + > >> + dcc->ram_offset) / 4 - 1); > >> + > >> + mutex_init(&dcc->mutex); > >> + dcc->enable = devm_kcalloc(dev, dcc->nr_link_list, > >> + sizeof(bool), GFP_KERNEL); > >> + if (!dcc->enable) > >> + return -ENOMEM; > >> + > >> + dcc->configured = devm_kcalloc(dev, dcc->nr_link_list, > >> + sizeof(bool), GFP_KERNEL); > >> + if (!dcc->configured) > >> + return -ENOMEM; > >> + > >> + dcc->nr_config = devm_kcalloc(dev, dcc->nr_link_list, > >> + sizeof(u32), GFP_KERNEL); > >> + if (!dcc->nr_config) > >> + return -ENOMEM; > >> + > >> + dcc->cfg_head = devm_kcalloc(dev, dcc->nr_link_list, > >> + sizeof(struct list_head), GFP_KERNEL); > >> + if (!dcc->cfg_head) > >> + return -ENOMEM; > > > > These are a lot of allocations. Any chance we can do one instead of > > this > > many? > > All these variable have predefined requirement of sizes > so they need to be allocated separately. Gather requirements, do some addition, and then allocate one chunk of memory? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel