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=-13.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 A729EC43461 for ; Wed, 14 Apr 2021 00:40:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6F18461249 for ; Wed, 14 Apr 2021 00:40:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1348828AbhDNAkp (ORCPT ); Tue, 13 Apr 2021 20:40:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50878 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1348224AbhDNAkn (ORCPT ); Tue, 13 Apr 2021 20:40:43 -0400 Received: from mail-ej1-x632.google.com (mail-ej1-x632.google.com [IPv6:2a00:1450:4864:20::632]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 77D94C06138C for ; Tue, 13 Apr 2021 17:40:22 -0700 (PDT) Received: by mail-ej1-x632.google.com with SMTP id u17so28758763ejk.2 for ; Tue, 13 Apr 2021 17:40:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Dgx0zg/uwSfJAIpLe+lA7Jwh4OGdZHvPB/sLQQcgrjw=; b=R6XABSlNkDS6hYye7g2llZd1hYxrne9xBJPTNCVDoBtSjuqR0OK83afzV2GazGIIU6 MluZRXzIpm827Lu6Dn4DKZEEPR/jCANNyPe0iT9+Vq6f94GfkqQvEi11uzypnOmxo9yP lbXWJt3b7cToel8jDlh3aNIfJnnIA3vFIArAOi1eSYL6lQZUDDq4MAC/trPWxlV49jUa DIZXsiA2F2I+EhfoumUq5JN1oys0VHr4J3zQfV0Q56pZ/F2FHkeYhVZ1/h/M6kKQ/s3M h6ILG+A+WTfg4hEEQA4kv4KnLbgmiEOhOHJnCt/AkE1s0Z87HZCY+sXtgPWrgBNIsZwO fK/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Dgx0zg/uwSfJAIpLe+lA7Jwh4OGdZHvPB/sLQQcgrjw=; b=sNsdHkWDfQqN4P+pe4K8VBfW23VjE4laIebk8nI5sUZOhUVw5bUXiAZt7k2cq4pU8O 90rW77qkywGr8mM8BU0nIURQIYkua5Ea4RKhGbKHgUVEpFlKAObZBuWmLKpJp1N1M2Nx NlnX7nN7awcmVys9yInani1C1glU2D808yhwCigch6VoU2kUVSjPdGsMgr9RBAs/VzAP /9F8zm6owIq++cerCblpvx+P+c3T5Soi5giW5yGIZxIV3nU1c8lg+DS5ARFfXGzySiZA D07nxxmYHvG+6ttMXe+6FfRQGqsO3dYMlee5kwtfDlA8Z8eYit+bzfkWF8vz8KAw9JFI u5NA== X-Gm-Message-State: AOAM530XBtsYgoGvBbppvH3mheeErgHhgPXTxp11gaLm2mIV3CoaXDZB pn8VsVuBf9hJGDEMJxfu7fk5FYXDNQ5h6EjPrYCivA== X-Google-Smtp-Source: ABdhPJxDntJqAjA21gPsoO6ArpWTc7tn3MSadgeJ586Ft88PaPvpOxexWca17ZT6xuXU3Dw1cpOBOSBP7RpLnLzSD3g= X-Received: by 2002:a17:907:7631:: with SMTP id jy17mr9626142ejc.418.1618360820549; Tue, 13 Apr 2021 17:40:20 -0700 (PDT) MIME-Version: 1.0 References: <161728744224.2474040.12854720917440712854.stgit@dwillia2-desk3.amr.corp.intel.com> <161728745324.2474040.14172040051810008737.stgit@dwillia2-desk3.amr.corp.intel.com> <20210406180037.00000474@Huawei.com> In-Reply-To: <20210406180037.00000474@Huawei.com> From: Dan Williams Date: Tue, 13 Apr 2021 17:40:16 -0700 Message-ID: Subject: Re: [PATCH v2 2/8] cxl/mem: Introduce 'struct cxl_regs' for "composable" CXL devices To: Jonathan Cameron Cc: linux-cxl@vger.kernel.org, Linux PCI , Linux ACPI , "Weiny, Ira" , Vishal L Verma , "Schofield, Alison" , Ben Widawsky , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 6, 2021 at 10:47 AM Jonathan Cameron wrote: > > On Thu, 1 Apr 2021 07:30:53 -0700 > Dan Williams wrote: > > > CXL MMIO register blocks are organized by device type and capabilities. > > There are Component registers, Device registers (yes, an ambiguous > > name), and Memory Device registers (a specific extension of Device > > registers). > > > > It is possible for a given device instance (endpoint or port) to > > implement register sets from multiple of the above categories. > > > > The driver code that enumerates and maps the registers is type specific > > so it is useful to have a dedicated type and helpers for each block > > type. > > > > At the same time, once the registers are mapped the origin type does not > > matter. It is overly pedantic to reference the register block type in > > code that is using the registers. > > > > In preparation for the endpoint driver to incorporate Component registers > > into its MMIO operations reorganize the registers to allow typed > > enumeration + mapping, but anonymous usage. With the end state of > > 'struct cxl_regs' to be: > > > > struct cxl_regs { > > union { > > struct { > > CXL_DEVICE_REGS(); > > }; > > struct cxl_device_regs device_regs; > > }; > > union { > > struct { > > CXL_COMPONENT_REGS(); > > }; > > struct cxl_component_regs component_regs; > > }; > > }; > > > > With this arrangement the driver can share component init code with > > ports, but when using the registers it can directly reference the > > component register block type by name without the 'component_regs' > > prefix. > > > > So, map + enumerate can be shared across drivers of different CXL > > classes e.g.: > > > > void cxl_setup_device_regs(struct device *dev, void __iomem *base, > > struct cxl_device_regs *regs); > > > > void cxl_setup_component_regs(struct device *dev, void __iomem *base, > > struct cxl_component_regs *regs); > > > > ...while inline usage in the driver need not indicate where the > > registers came from: > > > > readl(cxlm->regs.mbox + MBOX_OFFSET); > > readl(cxlm->regs.hdm + HDM_OFFSET); > > > > ...instead of: > > > > readl(cxlm->regs.device_regs.mbox + MBOX_OFFSET); > > readl(cxlm->regs.component_regs.hdm + HDM_OFFSET); > > > > This complexity of the definition in .h yields improvement in code > > readability in .c while maintaining type-safety for organization of > > setup code. It prepares the implementation to maintain organization in > > the face of CXL devices that compose register interfaces consisting of > > multiple types. > > > > Reviewed-by: Ben Widawsky > > Signed-off-by: Dan Williams > > A few minor things inline. > > > --- > > drivers/cxl/cxl.h | 33 +++++++++++++++++++++++++++++++++ > > drivers/cxl/mem.c | 44 ++++++++++++++++++++++++-------------------- > > drivers/cxl/mem.h | 13 +++++-------- > > 3 files changed, 62 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > > index 2e3bdacb32e7..37325e504fb7 100644 > > --- a/drivers/cxl/cxl.h > > +++ b/drivers/cxl/cxl.h > > @@ -34,5 +34,38 @@ > > #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18 > > #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20 > > > > +/* See note for 'struct cxl_regs' for the rationale of this organization */ > > +#define CXL_DEVICE_REGS() \ > > + void __iomem *status; \ > > + void __iomem *mbox; \ > > + void __iomem *memdev > > + > > +/** > > + * struct cxl_device_regs - Common container of CXL Device register > > + * block base pointers > > + * @status: CXL 2.0 8.2.8.3 Device Status Registers > > + * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers > > + * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers > > kernel-doc script is not going to be happy with documenting fields it can't see > + not documenting the CXL_DEVICE_REGS() field it can. > > I've no idea what the right way to handle this might be. Sure, I'll at least check that the tool does not complain, I might just make this not a kernel-doc and change the /** to plain /*. [..] > > diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h > > index daa9aba0e218..c247cf9c71af 100644 > > --- a/drivers/cxl/mem.h > > +++ b/drivers/cxl/mem.h > > @@ -53,10 +53,9 @@ struct cxl_memdev { > > /** > > * struct cxl_mem - A CXL memory device > > * @pdev: The PCI device associated with this CXL device. > > - * @regs: IO mappings to the device's MMIO > > - * @status_regs: CXL 2.0 8.2.8.3 Device Status Registers > > - * @mbox_regs: CXL 2.0 8.2.8.4 Mailbox Registers > > - * @memdev_regs: CXL 2.0 8.2.8.5 Memory Device Registers > > + * @base: IO mappings to the device's MMIO > > + * @cxlmd: Logical memory device chardev / interface > > Unrelated missing docs fix? Yeah, I'll declare that in the changelog. > > > + * @regs: Parsed register blocks > > * @payload_size: Size of space for payload > > * (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register) > > * @mbox_mutex: Mutex to synchronize mailbox access. > > @@ -67,12 +66,10 @@ struct cxl_memdev { > > */ > > struct cxl_mem { > > struct pci_dev *pdev; > > - void __iomem *regs; > > + void __iomem *base; > > Whilst I have no problem with the rename and fact you want to free it > up for other uses, perhaps call it out in the patch description? Sure.