From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robin Murphy Subject: Re: [RFC PATCH 1/2] fsl-mc: Added header file to provide fsl-mc bus iommu group creation Date: Thu, 30 Jun 2016 15:14:17 +0100 Message-ID: <57752939.9010504@arm.com> References: <1467220448-16764-1-git-send-email-nipun.gupta@nxp.com> <5774FABE.5070707@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Nipun Gupta , "will.deacon-5wv7dgnIgG8@public.gmane.org" , Stuart Yoder , "iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" Cc: Bharat Bhushan List-Id: iommu@lists.linux-foundation.org On 30/06/16 12:43, Nipun Gupta wrote: > > >> -----Original Message----- >> From: Robin Murphy [mailto:robin.murphy-5wv7dgnIgG8@public.gmane.org] >> Sent: Thursday, June 30, 2016 16:26 >> To: Nipun Gupta ; will.deacon-5wv7dgnIgG8@public.gmane.org; Stuart Yoder >> ; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; linux-arm- >> kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org >> Cc: Bharat Bhushan >> Subject: Re: [RFC PATCH 1/2] fsl-mc: Added header file to provide fsl-mc bus >> iommu group creation >> >> On 29/06/16 18:14, Nipun Gupta wrote: >>> Added a header file fsl_mc_smmu.h to provide basic support of creating >>> an IOMMU group for a fsl-mc type device and also provide helper macro >>> to get the stream ID of fsl-mc tyoe device. >>> >>> Signed-off-by: Nipun Gupta >>> Signed-off-by: Bharat Bhushan >>> --- >>> drivers/staging/fsl-mc/include/fsl_mc_smmu.h | 45 >>> ++++++++++++++++++++++++++++ >>> 1 file changed, 45 insertions(+) >>> create mode 100644 drivers/staging/fsl-mc/include/fsl_mc_smmu.h >>> >>> diff --git a/drivers/staging/fsl-mc/include/fsl_mc_smmu.h >>> b/drivers/staging/fsl-mc/include/fsl_mc_smmu.h >>> new file mode 100644 >>> index 0000000..9dff5ba >>> --- /dev/null >>> +++ b/drivers/staging/fsl-mc/include/fsl_mc_smmu.h >>> @@ -0,0 +1,45 @@ >>> +/* >>> + * Copyright (C) 2016 Freescale Semiconductor, Inc. >>> + * Author: Nipun Gupta >>> + * >>> + * This file is licensed under the terms of the GNU General Public >>> + * License version 2. This program is licensed "as is" without any >>> + * warranty of any kind, whether express or implied. >>> + */ >>> + >>> +#ifndef _FSL_MC_SMMU_H_ >>> +#define _FSL_MC_SMMU_H_ >>> + >>> +#include <../drivers/staging/fsl-mc/include/mc.h> >>> + >>> +/* Macro to get the MC device ICID (Stream ID) */ #define >>> +fslmc_dev_streamid(_dev) (to_fsl_mc_device(_dev)->icid) >> >> Is the the full 15-bit ICID from the MC's point of view, just the 7 bits that are >> actually routed to the SMMU, or the actual stream ID seen by the SMMU? None >> of those three are necessarily the same, and unless it's the third then I don't see >> the point of patches adding incomplete code which isn't going to work. > > Hi Robin, > > It's the second one where just 7 bits are maintained in the 'fsl_mc_device--->icid' which is programmed in SMMU. Right, once the SMR masking support is there then only these patches would make more sense. > We have sent these patches to get some early comments and to have you guys well informed beforehand about the changes which we require in the SMMU driver specific to fsl-mc bus :). Indeed, it's just kinda hard to review incomplete code, especially when it's the parts which don't exist yet which are the most significant ;) Either way, I'm deep in the middle of reworking SMMUv2 generic bindings[1] based on the latest iteration of the core code and iommu_fwspec[2], which removes the need for nearly all the bus-specific business within the driver (it's getting too big to be 4.8 material now, but I'll have something to post shortly). In that context, if the fsl-mc driver could process an "iommu-map" property on the MC node to plumb the ICID-to-stream-ID relationship into of_xlate, and inherit bus->iommu_ops from its parent bus, there shouldn't be any need to directly touch the SMMU driver at all. Robin. [1]:http://thread.gmane.org/gmane.linux.kernel.iommu/12454 [2]:http://thread.gmane.org/gmane.linux.kernel.iommu/14303 > > Thanks, > Nipun > >> >>> +/* Macro to get the container device of a MC device */ #define >>> +fslmc_cont_dev(_dev) ((to_fsl_mc_device(dev)->flags & \ >>> + FSL_MC_IS_DPRC) ? (_dev) : (_dev->parent)) >>> + >>> +/* Macro to check if a device is a container device */ #define >>> +is_cont_dev(_dev) (to_fsl_mc_device(_dev)->flags & FSL_MC_IS_DPRC) >>> + >>> +static struct iommu_group *fslmc_device_group(struct device *dev) { >>> + struct device *cont_dev = fslmc_cont_dev(dev); >>> + struct iommu_group *group; >>> + >>> + /* Container device is responsible for creating the iommu group */ >>> + if (is_cont_dev(dev)) { >>> + group = iommu_group_alloc(); >>> + >>> + if (IS_ERR(group)) >>> + return NULL; >>> + } else { >>> + get_device(cont_dev); >>> + group = iommu_group_get(cont_dev); >>> + put_device(cont_dev); >>> + } >>> + >>> + return group; >>> +} >> >> In isolation, though, this part seems perfectly reasonable. >> >> Robin. >> >>> + >>> +#endif /* _FSL_MC_SMMU_H_ */ >>> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: robin.murphy@arm.com (Robin Murphy) Date: Thu, 30 Jun 2016 15:14:17 +0100 Subject: [RFC PATCH 1/2] fsl-mc: Added header file to provide fsl-mc bus iommu group creation In-Reply-To: References: <1467220448-16764-1-git-send-email-nipun.gupta@nxp.com> <5774FABE.5070707@arm.com> Message-ID: <57752939.9010504@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 30/06/16 12:43, Nipun Gupta wrote: > > >> -----Original Message----- >> From: Robin Murphy [mailto:robin.murphy at arm.com] >> Sent: Thursday, June 30, 2016 16:26 >> To: Nipun Gupta ; will.deacon at arm.com; Stuart Yoder >> ; iommu at lists.linux-foundation.org; linux-arm- >> kernel at lists.infradead.org >> Cc: Bharat Bhushan >> Subject: Re: [RFC PATCH 1/2] fsl-mc: Added header file to provide fsl-mc bus >> iommu group creation >> >> On 29/06/16 18:14, Nipun Gupta wrote: >>> Added a header file fsl_mc_smmu.h to provide basic support of creating >>> an IOMMU group for a fsl-mc type device and also provide helper macro >>> to get the stream ID of fsl-mc tyoe device. >>> >>> Signed-off-by: Nipun Gupta >>> Signed-off-by: Bharat Bhushan >>> --- >>> drivers/staging/fsl-mc/include/fsl_mc_smmu.h | 45 >>> ++++++++++++++++++++++++++++ >>> 1 file changed, 45 insertions(+) >>> create mode 100644 drivers/staging/fsl-mc/include/fsl_mc_smmu.h >>> >>> diff --git a/drivers/staging/fsl-mc/include/fsl_mc_smmu.h >>> b/drivers/staging/fsl-mc/include/fsl_mc_smmu.h >>> new file mode 100644 >>> index 0000000..9dff5ba >>> --- /dev/null >>> +++ b/drivers/staging/fsl-mc/include/fsl_mc_smmu.h >>> @@ -0,0 +1,45 @@ >>> +/* >>> + * Copyright (C) 2016 Freescale Semiconductor, Inc. >>> + * Author: Nipun Gupta >>> + * >>> + * This file is licensed under the terms of the GNU General Public >>> + * License version 2. This program is licensed "as is" without any >>> + * warranty of any kind, whether express or implied. >>> + */ >>> + >>> +#ifndef _FSL_MC_SMMU_H_ >>> +#define _FSL_MC_SMMU_H_ >>> + >>> +#include <../drivers/staging/fsl-mc/include/mc.h> >>> + >>> +/* Macro to get the MC device ICID (Stream ID) */ #define >>> +fslmc_dev_streamid(_dev) (to_fsl_mc_device(_dev)->icid) >> >> Is the the full 15-bit ICID from the MC's point of view, just the 7 bits that are >> actually routed to the SMMU, or the actual stream ID seen by the SMMU? None >> of those three are necessarily the same, and unless it's the third then I don't see >> the point of patches adding incomplete code which isn't going to work. > > Hi Robin, > > It's the second one where just 7 bits are maintained in the 'fsl_mc_device--->icid' which is programmed in SMMU. Right, once the SMR masking support is there then only these patches would make more sense. > We have sent these patches to get some early comments and to have you guys well informed beforehand about the changes which we require in the SMMU driver specific to fsl-mc bus :). Indeed, it's just kinda hard to review incomplete code, especially when it's the parts which don't exist yet which are the most significant ;) Either way, I'm deep in the middle of reworking SMMUv2 generic bindings[1] based on the latest iteration of the core code and iommu_fwspec[2], which removes the need for nearly all the bus-specific business within the driver (it's getting too big to be 4.8 material now, but I'll have something to post shortly). In that context, if the fsl-mc driver could process an "iommu-map" property on the MC node to plumb the ICID-to-stream-ID relationship into of_xlate, and inherit bus->iommu_ops from its parent bus, there shouldn't be any need to directly touch the SMMU driver at all. Robin. [1]:http://thread.gmane.org/gmane.linux.kernel.iommu/12454 [2]:http://thread.gmane.org/gmane.linux.kernel.iommu/14303 > > Thanks, > Nipun > >> >>> +/* Macro to get the container device of a MC device */ #define >>> +fslmc_cont_dev(_dev) ((to_fsl_mc_device(dev)->flags & \ >>> + FSL_MC_IS_DPRC) ? (_dev) : (_dev->parent)) >>> + >>> +/* Macro to check if a device is a container device */ #define >>> +is_cont_dev(_dev) (to_fsl_mc_device(_dev)->flags & FSL_MC_IS_DPRC) >>> + >>> +static struct iommu_group *fslmc_device_group(struct device *dev) { >>> + struct device *cont_dev = fslmc_cont_dev(dev); >>> + struct iommu_group *group; >>> + >>> + /* Container device is responsible for creating the iommu group */ >>> + if (is_cont_dev(dev)) { >>> + group = iommu_group_alloc(); >>> + >>> + if (IS_ERR(group)) >>> + return NULL; >>> + } else { >>> + get_device(cont_dev); >>> + group = iommu_group_get(cont_dev); >>> + put_device(cont_dev); >>> + } >>> + >>> + return group; >>> +} >> >> In isolation, though, this part seems perfectly reasonable. >> >> Robin. >> >>> + >>> +#endif /* _FSL_MC_SMMU_H_ */ >>> >