Re: [RFC PATCH 1/2] fsl-mc: Added header file to provide fsl-mc bus iommu group creation

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.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 <nipun.gupta-3arQi8VN3Tc@public.gmane.org>; will.deacon-5wv7dgnIgG8@public.gmane.org; Stuart Yoder
>> <stuart.yoder-3arQi8VN3Tc@public.gmane.org>; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; linux-arm-
>> kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> Cc: Bharat Bhushan <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>
>> 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 <nipun.gupta-3arQi8VN3Tc@public.gmane.org>
>>> Signed-off-by: Bharat Bhushan <bharat.bhushan-3arQi8VN3Tc@public.gmane.org>
>>> ---
>>>  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 <nipun.gupta-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>> + *
>>> + * 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_ */
>>>
>