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 F40EEC54EE9 for ; Wed, 7 Sep 2022 13:19:51 +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:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=plkYRy4dF3bW1f+UO8ZfngEM391fZ6tnZxKz1j2eyGs=; b=rVahPR2buJ1EoD pGCdCvG0frfe3oug6eQ4O5v3c0hTRZREpmw4mbxbuuuBJrwg3QC1x+OWfVJHB3uGrWfJgYr5hEvHU Q3LwnQSUlMhOf5agmJiOnC+XCqjCBBx6F4jrES674jx5fvYK1elY+wN2MpGJL077kCrDV9bcHrpfw LVUsADzDfgyNEk8jETQzvw9vW+scn/ocz7eRlLSSaJaoLHaDot+hIsx7EgKXeiOggHdS5d0Uwyl6p GtIYSduJ0baF2QyOXwg7QpC1rO9elgdDR2m4d12xQQ+jPecj0qPNsdy78lzZ2ARtipJ9xRZUKAX5E eXRPBffX4s9W77H90LPA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oVux1-006QtF-Lp; Wed, 07 Sep 2022 13:18:44 +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 1oVuws-006Qpg-Cp for linux-arm-kernel@lists.infradead.org; Wed, 07 Sep 2022 13:18:38 +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 3C0336190B; Wed, 7 Sep 2022 13:18:31 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 425ADC433D7; Wed, 7 Sep 2022 13:18:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1662556710; bh=vfGds+IHhiq5Jfaq54EwwCVAb4B+ErClkegWiHk+7UQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=MG9l7KiTqdxYExU+toJC8MSzf5EAzXbdQLiHk9BwAd5zmRxrQv6bCvSLfx4zxVL/S XNwlv+5E3b7qHzpgDuQ1gYxk4OwbCQc3qouzcTRrFvC9Z3F9dMu0YLQgMOuRmAJNaY gIRwnApYUy6K7Q8SYzHTRg3L4tyQFpPR8AmWG45n8vLBfduRJ9ILU963bBWOTEr7or ORWYekYBuddCabPssufALWr5++hvRCrY9ZKCa6/8aKJFiPDDkxx/g5TeE0hDNi6mLP YgeIHoAYsSxEeWTvMpVbuF4ttl42ZA3qciIUf3R2Nq0ZWfDgbZt4wtPGGUzRJSLddE XimGqmLoqr9JA== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oVuwl-008eKg-VP; Wed, 07 Sep 2022 14:18:28 +0100 Date: Wed, 07 Sep 2022 14:18:27 +0100 Message-ID: <87h71juxuk.wl-maz@kernel.org> From: Marc Zyngier To: Nipun Gupta Cc: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [RFC PATCH v3 4/7] bus/cdx: add cdx-MSI domain with gic-its domain as parent In-Reply-To: <20220906134801.4079497-5-nipun.gupta@amd.com> References: <20220803122655.100254-1-nipun.gupta@amd.com> <20220906134801.4079497-1-nipun.gupta@amd.com> <20220906134801.4079497-5-nipun.gupta@amd.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: nipun.gupta@amd.com, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, gregkh@linuxfoundation.org, rafael@kernel.org, eric.auger@redhat.com, alex.williamson@redhat.com, cohuck@redhat.com, puneet.gupta@amd.com, song.bao.hua@hisilicon.com, mchehab+huawei@kernel.org, f.fainelli@gmail.com, jeffrey.l.hugo@gmail.com, saravanak@google.com, Michael.Srba@seznam.cz, mani@kernel.org, yishaih@nvidia.com, jgg@ziepe.ca, jgg@nvidia.com, robin.murphy@arm.com, will@kernel.org, joro@8bytes.org, masahiroy@kernel.org, ndesaulniers@google.com, linux-arm-kernel@lists.infradead.org, linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, kvm@vger.kernel.org, okaya@kernel.org, harpreet.anand@amd.com, nikhil.agarwal@amd.com, michal.simek@amd.com, aleksandar.radovanovic@amd.com, git@amd.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220907_061834_541419_8952BFBF X-CRM114-Status: GOOD ( 48.77 ) 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 Tue, 06 Sep 2022 14:47:58 +0100, Nipun Gupta wrote: > > Devices on cdx bus are dynamically detected and registered using > platform_device_register API. As these devices are not linked to > of node they need a separate MSI domain for handling device ID > to be provided to the GIC ITS domain. > > This also introduces APIs to alloc and free IRQs for CDX domain. > > Signed-off-by: Nipun Gupta > Signed-off-by: Nikhil Agarwal > --- > drivers/bus/cdx/cdx.c | 18 +++ > drivers/bus/cdx/cdx.h | 19 +++ > drivers/bus/cdx/cdx_msi.c | 236 +++++++++++++++++++++++++++++++++++ > drivers/bus/cdx/mcdi_stubs.c | 1 + > include/linux/cdx/cdx_bus.h | 19 +++ > 5 files changed, 293 insertions(+) > create mode 100644 drivers/bus/cdx/cdx_msi.c > > diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c > index fc417c32c59b..02ececce1c84 100644 > --- a/drivers/bus/cdx/cdx.c > +++ b/drivers/bus/cdx/cdx.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > > #include "cdx.h" > @@ -236,6 +237,7 @@ static int cdx_device_add(struct device *parent, > struct cdx_dev_params_t *dev_params) > { > struct cdx_device *cdx_dev; > + struct irq_domain *cdx_msi_domain; > int ret; > > cdx_dev = kzalloc(sizeof(*cdx_dev), GFP_KERNEL); > @@ -252,6 +254,7 @@ static int cdx_device_add(struct device *parent, > > /* Populate CDX dev params */ > cdx_dev->req_id = dev_params->req_id; > + cdx_dev->num_msi = dev_params->num_msi; > cdx_dev->vendor = dev_params->vendor; > cdx_dev->device = dev_params->device; > cdx_dev->bus_id = dev_params->bus_id; > @@ -269,6 +272,21 @@ static int cdx_device_add(struct device *parent, > dev_set_name(&cdx_dev->dev, "cdx-%02x:%02x", cdx_dev->bus_id, > cdx_dev->func_id); > > + /* If CDX MSI domain is not created, create one. */ > + cdx_msi_domain = cdx_find_msi_domain(parent); Why do we need such a wrapper around find_host_domain()? > + if (!cdx_msi_domain) { > + cdx_msi_domain = cdx_msi_domain_init(parent); This is racy. If device are populated in parallel, bad things will happen. > + if (!cdx_msi_domain) { > + dev_err(&cdx_dev->dev, > + "cdx_msi_domain_init() failed: %d", ret); > + kfree(cdx_dev); > + return -1; Use standard error codes. > + } > + } > + > + /* Set the MSI domain */ > + dev_set_msi_domain(&cdx_dev->dev, cdx_msi_domain); > + > ret = device_add(&cdx_dev->dev); > if (ret != 0) { > dev_err(&cdx_dev->dev, > diff --git a/drivers/bus/cdx/cdx.h b/drivers/bus/cdx/cdx.h > index db0569431c10..95df440ebd73 100644 > --- a/drivers/bus/cdx/cdx.h > +++ b/drivers/bus/cdx/cdx.h > @@ -20,6 +20,7 @@ > * @res: array of MMIO region entries > * @res_count: number of valid MMIO regions > * @req_id: Requestor ID associated with CDX device > + * @num_msi: Number of MSI's supported by the device > */ > struct cdx_dev_params_t { > u16 vendor; > @@ -29,6 +30,24 @@ struct cdx_dev_params_t { > struct resource res[MAX_CDX_DEV_RESOURCES]; > u8 res_count; > u32 req_id; > + u32 num_msi; > }; > > +/** > + * cdx_msi_domain_init - Init the CDX bus MSI domain. > + * @dev: Device of the CDX bus controller > + * > + * Return CDX MSI domain, NULL on failure > + */ > +struct irq_domain *cdx_msi_domain_init(struct device *dev); > + > +/** > + * cdx_find_msi_domain - Get the CDX-MSI domain. > + * @dev: CDX controller generic device > + * > + * Return CDX MSI domain, NULL on error or if CDX-MSI domain is > + * not yet created. > + */ > +struct irq_domain *cdx_find_msi_domain(struct device *parent); > + > #endif /* _CDX_H_ */ > diff --git a/drivers/bus/cdx/cdx_msi.c b/drivers/bus/cdx/cdx_msi.c > new file mode 100644 > index 000000000000..2fb7bac18393 > --- /dev/null > +++ b/drivers/bus/cdx/cdx_msi.c > @@ -0,0 +1,236 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * AMD CDX bus driver MSI support > + * > + * Copyright (C) 2022, Advanced Micro Devices, Inc. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "cdx.h" > + > +#ifdef GENERIC_MSI_DOMAIN_OPS > +/* > + * Generate a unique ID identifying the interrupt (only used within the MSI > + * irqdomain. Combine the req_id with the interrupt index. > + */ > +static irq_hw_number_t cdx_domain_calc_hwirq(struct cdx_device *dev, > + struct msi_desc *desc) > +{ > + /* > + * Make the base hwirq value for req_id*10000 so it is readable > + * as a decimal value in /proc/interrupts. > + */ > + return (irq_hw_number_t)(desc->msi_index + (dev->req_id * 10000)); No, please. Use shifts, and use a script if decimal conversion fails you. We're not playing these games. And the cast is pointless. Yes, you have lifted it from the FSL code, bad move. /me makes a note to go and clean-up this crap. > +} > + > +static void cdx_msi_set_desc(msi_alloc_info_t *arg, > + struct msi_desc *desc) > +{ > + arg->desc = desc; > + arg->hwirq = cdx_domain_calc_hwirq(to_cdx_device(desc->dev), desc); > +} > +#else > +#define cdx_msi_set_desc NULL Why the ifdefery? This should *only* be supported with GENERIC_MSI_DOMAIN_OPS. > +#endif > + > +static void cdx_msi_update_dom_ops(struct msi_domain_info *info) > +{ > + struct msi_domain_ops *ops = info->ops; > + > + if (!ops) > + return; > + > + /* set_desc should not be set by the caller */ > + if (!ops->set_desc) > + ops->set_desc = cdx_msi_set_desc; Then why are you allowing this to be overridden? > +} > + > +static void cdx_msi_write_msg(struct irq_data *irq_data, > + struct msi_msg *msg) > +{ > + /* > + * Do nothing as CDX devices have these pre-populated > + * in the hardware itself. > + */ We talked about this in a separate thread. This is a major problem. > +} > + > +static void cdx_msi_update_chip_ops(struct msi_domain_info *info) > +{ > + struct irq_chip *chip = info->chip; > + > + if (!chip) > + return; > + > + /* > + * irq_write_msi_msg should not be set by the caller > + */ > + if (!chip->irq_write_msi_msg) > + chip->irq_write_msi_msg = cdx_msi_write_msg; Then why the check? > +} > +/** > + * cdx_msi_create_irq_domain - Create a CDX MSI interrupt domain > + * @fwnode: Optional firmware node of the interrupt controller > + * @info: MSI domain info > + * @parent: Parent irq domain > + * > + * Updates the domain and chip ops and creates a CDX MSI > + * interrupt domain. > + * > + * Returns: > + * A domain pointer or NULL in case of failure. > + */ > +static struct irq_domain *cdx_msi_create_irq_domain(struct fwnode_handle *fwnode, > + struct msi_domain_info *info, > + struct irq_domain *parent) > +{ > + if (WARN_ON((info->flags & MSI_FLAG_LEVEL_CAPABLE))) > + info->flags &= ~MSI_FLAG_LEVEL_CAPABLE; No. Just fail the domain creation. We shouldn't paper over these things. > + if (info->flags & MSI_FLAG_USE_DEF_DOM_OPS) > + cdx_msi_update_dom_ops(info); > + if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS) > + cdx_msi_update_chip_ops(info); Under what circumstances would the default ops not be used? The only caller is in this file and has pre-computed values. This looks like a copy/paste from platform-msi.c. > + info->flags |= MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS | MSI_FLAG_FREE_MSI_DESCS; > + > + return msi_create_irq_domain(fwnode, info, parent); This whole function makes no sense. You should move everything to the relevant structures, and simply call msi_create_irq_domain() from the sole caller of this function. > +} > + > +int cdx_msi_domain_alloc_irqs(struct device *dev, unsigned int irq_count) > +{ > + struct irq_domain *msi_domain; > + int ret; > + > + msi_domain = dev_get_msi_domain(dev); > + if (!msi_domain) { How can that happen? > + dev_err(dev, "msi domain get failed\n"); > + return -EINVAL; > + } > + > + ret = msi_setup_device_data(dev); > + if (ret) { > + dev_err(dev, "msi setup device failed: %d\n", ret); > + return ret; > + } > + > + msi_lock_descs(dev); > + if (msi_first_desc(dev, MSI_DESC_ALL)) > + ret = -EINVAL; > + msi_unlock_descs(dev); > + if (ret) { > + dev_err(dev, "msi setup device failed: %d\n", ret); Same message twice, not very useful. Consider grouping these things at the end of the function and make use of a (oh Gawd) goto... > + return ret; > + } > + > + ret = msi_domain_alloc_irqs(msi_domain, dev, irq_count); > + if (ret) > + dev_err(dev, "Failed to allocate IRQs\n"); > + > + return ret; > +} > +EXPORT_SYMBOL(cdx_msi_domain_alloc_irqs); EXPORT_SYMBOL_GPL(), please, for all the exports. > + > +void cdx_msi_domain_free_irqs(struct device *dev) > +{ > + struct irq_domain *msi_domain; > + > + msi_domain = dev_get_msi_domain(dev); > + if (!msi_domain) Again, how can that happen? > + return; > + > + msi_domain_free_irqs(msi_domain, dev); > +} > +EXPORT_SYMBOL(cdx_msi_domain_free_irqs); This feels like a very pointless helper, and again a copy/paste from the FSL code. I'd rather you change msi_domain_free_irqs() to only take a device and use the implicit MSI domain. > + > +static struct irq_chip cdx_msi_irq_chip = { > + .name = "CDX-MSI", > + .irq_mask = irq_chip_mask_parent, > + .irq_unmask = irq_chip_unmask_parent, > + .irq_eoi = irq_chip_eoi_parent, > + .irq_set_affinity = msi_domain_set_affinity nit: please align things vertically. > +}; > + > +static int cdx_msi_prepare(struct irq_domain *msi_domain, > + struct device *dev, > + int nvec, msi_alloc_info_t *info) > +{ > + struct cdx_device *cdx_dev = to_cdx_device(dev); > + struct msi_domain_info *msi_info; > + struct device *parent = dev->parent; > + u32 dev_id; > + int ret; > + > + /* Retrieve device ID from requestor ID using parent device */ > + ret = of_map_id(parent->of_node, cdx_dev->req_id, "msi-map", > + "msi-map-mask", NULL, &dev_id); > + if (ret) { > + dev_err(dev, "of_map_id failed for MSI: %d\n", ret); > + return ret; > + } > + > + /* Set the device Id to be passed to the GIC-ITS */ > + info->scratchpad[0].ul = dev_id; > + > + msi_info = msi_get_domain_info(msi_domain->parent); > + > + /* Allocate at least 32 MSIs, and always as a power of 2 */ Where is this requirement coming from? > + nvec = max_t(int, 32, roundup_pow_of_two(nvec)); > + return msi_info->ops->msi_prepare(msi_domain->parent, dev, nvec, info); > +} > + > +static struct msi_domain_ops cdx_msi_ops __ro_after_init = { > + .msi_prepare = cdx_msi_prepare, > +}; > + > +static struct msi_domain_info cdx_msi_domain_info = { > + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS), > + .ops = &cdx_msi_ops, > + .chip = &cdx_msi_irq_chip, > +}; > + > +struct irq_domain *cdx_msi_domain_init(struct device *dev) > +{ > + struct irq_domain *parent; > + struct irq_domain *cdx_msi_domain; > + struct fwnode_handle *fwnode_handle; > + struct device_node *parent_node; > + struct device_node *np = dev->of_node; > + > + fwnode_handle = of_node_to_fwnode(np); > + > + parent_node = of_parse_phandle(np, "msi-map", 1); Huh. This only works because you are stuck with a single ITS per system. > + if (!parent_node) { > + dev_err(dev, "msi-map not present on cdx controller\n"); > + return NULL; > + } > + > + parent = irq_find_matching_fwnode(of_node_to_fwnode(parent_node), > + DOMAIN_BUS_NEXUS); > + if (!parent || !msi_get_domain_info(parent)) { > + dev_err(dev, "unable to locate ITS domain\n"); > + return NULL; > + } > + > + cdx_msi_domain = cdx_msi_create_irq_domain(fwnode_handle, > + &cdx_msi_domain_info, parent); > + if (!cdx_msi_domain) { > + dev_err(dev, "unable to create CDX-MSI domain\n"); > + return NULL; > + } > + > + dev_dbg(dev, "CDX-MSI domain created\n"); > + > + return cdx_msi_domain; > +} > + > +struct irq_domain *cdx_find_msi_domain(struct device *parent) > +{ > + return irq_find_host(parent->of_node); > +} > diff --git a/drivers/bus/cdx/mcdi_stubs.c b/drivers/bus/cdx/mcdi_stubs.c > index cc9d30fa02f8..2c8db1f5a057 100644 > --- a/drivers/bus/cdx/mcdi_stubs.c > +++ b/drivers/bus/cdx/mcdi_stubs.c > @@ -45,6 +45,7 @@ int cdx_mcdi_get_func_config(struct cdx_mcdi_t *cdx_mcdi, > dev_params->res_count = 2; > > dev_params->req_id = 0x250; > + dev_params->num_msi = 4; Why the hardcoded 4? Is that part of the firmware emulation stuff? M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel