Re: [PATCH 1/6] platform: specify the IOMMU physical translation bit

[Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 04/17/2015 02:26 PM, Alexandre Courbot wrote:
> On Thu, Apr 16, 2015 at 8:06 PM, Vince Hsu <vinceh@nvidia.com> wrote:
>> The IOMMU physical translation bit might vary with different SoCs. So add
>> a variable to specify this bit for GK20A.
>>
>> Signed-off-by: Vince Hsu <vinceh@nvidia.com>
>> ---
>>   drm/nouveau/nouveau_platform.c | 19 +++++++++++++++++++
>>   drm/nouveau/nouveau_platform.h |  1 +
>>   2 files changed, 20 insertions(+)
>>
>> diff --git a/drm/nouveau/nouveau_platform.c b/drm/nouveau/nouveau_platform.c
>> index 775277f1edb0..0d002f73e356 100644
>> --- a/drm/nouveau/nouveau_platform.c
>> +++ b/drm/nouveau/nouveau_platform.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/of.h>
>> +#include <linux/of_device.h>
>>   #include <linux/reset.h>
>>   #include <linux/regulator/consumer.h>
>>   #include <linux/iommu.h>
>> @@ -92,6 +93,22 @@ static int nouveau_platform_power_down(struct nouveau_platform_gpu *gpu)
>>          return 0;
>>   }
>>
>> +static unsigned long nouveau_platform_get_iommu_bit(struct device *dev)
>> +{
>> +       const struct of_device_id *match;
>> +
>> +       match = of_match_device(dev->driver->of_match_table, dev);
>> +       if (!match) {
>> +               dev_warn(dev, "cannot find OF match for device\n");
>> +               return 0;
>> +       }
>> +
>> +       if (!strcmp(match->compatible, "nvidia,gk20a"))
>> +               return 34;
>> +       else
>> +               return 0;
>> +}
> Instead of this function, you should probably use the data field of
> struct of_device_id. Define a local struct called, say,
> nouveau_platform_params containing (for now) a single iommu_addr_bit
> field and instanciate one for each entry of nouveau_platform_match.
> Then you can cast match->data and retrieve the field directly instead
> of using strcmp.
Actually I did that way when I was based on your tree since you already 
defined the probe data. I admit I jerry-built this patch while I rebased 
to Ben's tree. Will fix in the next version.

>
> I'd say this is then simple enough to do directly in
> nouveau_platform_probe_iommu() instead of having a function dedicated
> to it. It is also safer because when we add a new chip, we have to
> update nouveau_platform_match but might very well forget about your
> function, and will end up with bit 0 being set on all our GPU
> addresses and endless hours of fun figuring out how this happened. :)
>
> While I am at it: how about defining this as a u64 mask to set/unset
> on GPU addresses instead of just a bit? This is more flexible and
> again safer in case someone "omits" to specify the correct bit for a
> chip.
>


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau