On 30/06/16 12:43, Nipun Gupta wrote: > > >> -----Original Message----- >> From: Robin Murphy [mailto:[email protected]] >> Sent: Thursday, June 30, 2016 16:26 >> To: Nipun Gupta <[email protected]>; [email protected]; Stuart Yoder >> <[email protected]>; [email protected]; linux-arm- >> [email protected] >> Cc: Bharat Bhushan <[email protected]> >> 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 <[email protected]> >>> Signed-off-by: Bharat Bhushan <[email protected]> >>> --- >>> 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 <[email protected]> >>> + * >>> + * 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_ */ >>> > _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
