Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions

2021-04-29 Thread Dmitry Osipenko
29.04.2021 08:51, Krishna Reddy пишет:
> Hi Dmitry,
> 
>> Thank you for the answer. Could you please give more information about:
>> 1) Are you on software or hardware team, or both?
> 
> I am in the software team and has contributed to initial Tegra SMMU driver in 
> the downstream along with earlier team member Hiroshi Doyu.
> 
>> 2) Is SMMU a third party IP or developed in-house?
> 
> Tegra SMMU is developed in-house. 
> 
>> 3) Do you have a direct access to HDL sources? Are you 100% sure that
>> hardware does what you say?
> 
> It was discussed with Hardware team before and again today as well.
> Enabling ASID for display engine while it continues to access the buffer 
> memory is a safe operation.
> As per HW team, The only side-effect that can happen is additional latency to 
> transaction as SMMU caches get warmed up.
> 
>> 4) What happens when CPU writes to ASID register? Does SMMU state machine
>> latch ASID status (making it visible) only at a single "safe" point?
> 
> MC makes a decision on routing transaction through either SMMU page tables or 
> bypassing based on the ASID register value.  It
> checks the ASID register only once per transaction in the pipeline.

Thank you very much for the clarification.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions

2021-04-28 Thread Dmitry Osipenko
28.04.2021 08:57, Mikko Perttunen пишет:
> On 4/28/21 8:51 AM, Dmitry Osipenko wrote:
>> 23.04.2021 19:32, Thierry Reding пишет:
>>> Note that there will be no new releases of the bootloader for earlier
>>> devices, so adding support for these new DT bindings will not be
>>> practical. The bootloaders on those devices do pass information about
>>> the active framebuffer via the kernel command-line, so we may want to
>>> add code to create reserved regions in the IOMMU based on that.
>>
>> Since this change requires a bootloader update anyways, why it's not
>> possible to fix the bootloader properly, making it to stop all the DMA
>> activity before jumping into kernel?
>>
> 
> That is not desirable, as then we couldn't have seamless
> bootloader-kernel boot splash transition.

The seamless transition should be more complicated since it should
require to read out the hardware state in order to convert it into DRM
state + display panel needs to stay ON. It's a bit questionable whether
this is really needed, so far this is not achievable in mainline.

Nevertheless, it will be good to have an early simple-framebuffer, which
I realized only after sending out the message.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions

2021-04-27 Thread Dmitry Osipenko
23.04.2021 19:32, Thierry Reding пишет:
> Note that an earlier proposal was to use the existing simple-framebuffer
> device tree bindings to transport this information. Unfortunately there
> are cases where this is not enough. On Tegra SoCs, for example, the
> bootloader will also set up a color space correction lookup table in the
> system memory that the display controller will access during boot,
> alongside the framebuffer. The simple-framebuffer DT bindings have no
> way of describing this (and I guess one could argue that this particular
> setup no longer is a "simple" framebuffer), so the above, more flexible
> proposal was implemented.

Will simple-framebuffer be able to use that reserved region
transparently? Or will it require a custom simple-framebuffer driver?

Could we make simple-framebuffer support a part of this series?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions

2021-04-27 Thread Dmitry Osipenko
23.04.2021 19:32, Thierry Reding пишет:
> Note that there will be no new releases of the bootloader for earlier
> devices, so adding support for these new DT bindings will not be
> practical. The bootloaders on those devices do pass information about
> the active framebuffer via the kernel command-line, so we may want to
> add code to create reserved regions in the IOMMU based on that.

Since this change requires a bootloader update anyways, why it's not
possible to fix the bootloader properly, making it to stop all the DMA
activity before jumping into kernel?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions

2021-04-27 Thread Dmitry Osipenko
27.04.2021 21:30, Krishna Reddy пишет:
>> Is it always safe to enable SMMU ASID in a middle of a DMA request made by a
>> memory client?
> 
> From MC point of view, It is safe to enable and has been this way from many 
> years in downstream code for display engine.
> It doesn't impact the transactions that have already bypassed SMMU before 
> enabling SMMU ASID. 
> Transactions that are yet to pass SMMU stage would go through SMMU once SMMU 
> ASID is enabled and visible.

Hello,

Thank you for the answer. Could you please give more information about:

1) Are you on software or hardware team, or both?

2) Is SMMU a third party IP or developed in-house?

3) Do you have a direct access to HDL sources? Are you 100% sure that
hardware does what you say?

4) What happens when CPU writes to ASID register? Does SMMU state
machine latch ASID status (making it visible) only at a single "safe" point?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook

2021-04-26 Thread Dmitry Osipenko
26.04.2021 10:14, Thierry Reding пишет:
> On Sat, Apr 24, 2021 at 11:27:10PM +0300, Dmitry Osipenko wrote:
>> 23.04.2021 18:23, Dmitry Osipenko пишет:
>>> 23.04.2021 18:01, Guillaume Tucker пишет:
>>>> On 02/04/2021 15:40, Dmitry Osipenko wrote:
>>>>> 01.04.2021 11:55, Nicolin Chen пишет:
>>>>>> On Mon, Mar 29, 2021 at 02:32:56AM +0300, Dmitry Osipenko wrote:
>>>>>>> The previous commit fixes problem where display client was attaching too
>>>>>>> early to IOMMU during kernel boot in a multi-platform kernel 
>>>>>>> configuration
>>>>>>> which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to
>>>>>>> defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore,
>>>>>>> revert it.
>>>>>>
>>>>>> Sorry for the late reply. I have been busy with downstream tasks.
>>>>>>
>>>>>> I will give them a try by the end of the week. Yet, probably it'd
>>>>>> be better to include Guillaume also as he has the Nyan platform.
>>>>>>
>>>>>
>>>>> Indeed, thanks. Although, I'm pretty sure that it's the same issue which
>>>>> I reproduced on Nexus 7.
>>>>>
>>>>> Guillaume, could you please give a test to these patches on Nyan Big?
>>>>> There should be no EMEM errors in the kernel log with this patches.
>>>>>
>>>>> https://patchwork.ozlabs.org/project/linux-tegra/list/?series=236215
>>>>
>>>> So sorry for the very late reply.  I have tried the patches but
>>>> hit some issues on linux-next, it's not reaching a login prompt
>>>> with next-20210422.  So I then tried with next-20210419 which
>>>> does boot but shows the IOMMU error:
>>>>
>>>> <6>[2.995341] tegra-dc 5420.dc: Adding to iommu group 1
>>>> <4>[3.001070] Failed to attached device 5420.dc to IOMMU_mapping  
>>>>
>>>>   https://lava.collabora.co.uk/scheduler/job/3570052#L1120
>>>>
>>>> The branch I'm using with the patches applied can be found here:
>>>>
>>>>   
>>>> https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210419-nyan-big-drm-read/
>>>>
>>>> Hope this helps, let me know if you need anything else to be
>>>> tested.
>>>
>>>
>>> Hello Guillaume,
>>>
>>> The current linux-next doesn't boot on all ARM (AFAIK), the older
>>> next-20210413 works. The above message should be unrelated to the boot
>>> problem. It should be okay to ignore that message as it should be
>>> harmless in yours case.
>>>
>>
>> Although, the 20210419 should be good.
>>
>> Thierry, do you know what those SOR and Nouveau issues are about?
> 
> There's a use-after-free (though it's really a use-before-init) issue in
> linux-next at the moment, but a fix has been suggested. The fix for this
> along with an additional leak plug is here:
> 
>   http://patchwork.ozlabs.org/project/linux-tegra/list/?series=240569

Nice, thank you. Maybe Guillaume could give it a test.

> I'm not aware of any Nouveau issues. What version and platform are those
> happening on? Are there any logs? I can't seem to find them in this
> thread.

This all is on Nyan Big using linux-next-20210419. There is a link to the full 
log above.

I see this Nouveau failure:

<6>[   19.323113] nouveau 5700.gpu: Adding to iommu group 2
<6>[   19.323615] nouveau 5700.gpu: NVIDIA GK20A (0ea000a1)
<6>[   19.323668] nouveau 5700.gpu: imem: using IOMMU
<3>[   19.323795] nouveau 5700.gpu: gr ctor failed: -2
<4>[   19.323983] nouveau: probe of 5700.gpu failed with error -2

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook

2021-04-24 Thread Dmitry Osipenko
24.04.2021 23:27, Dmitry Osipenko пишет:
> 23.04.2021 18:23, Dmitry Osipenko пишет:
>> 23.04.2021 18:01, Guillaume Tucker пишет:
>>> On 02/04/2021 15:40, Dmitry Osipenko wrote:
>>>> 01.04.2021 11:55, Nicolin Chen пишет:
>>>>> On Mon, Mar 29, 2021 at 02:32:56AM +0300, Dmitry Osipenko wrote:
>>>>>> The previous commit fixes problem where display client was attaching too
>>>>>> early to IOMMU during kernel boot in a multi-platform kernel 
>>>>>> configuration
>>>>>> which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to
>>>>>> defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore,
>>>>>> revert it.
>>>>>
>>>>> Sorry for the late reply. I have been busy with downstream tasks.
>>>>>
>>>>> I will give them a try by the end of the week. Yet, probably it'd
>>>>> be better to include Guillaume also as he has the Nyan platform.
>>>>>
>>>>
>>>> Indeed, thanks. Although, I'm pretty sure that it's the same issue which
>>>> I reproduced on Nexus 7.
>>>>
>>>> Guillaume, could you please give a test to these patches on Nyan Big?
>>>> There should be no EMEM errors in the kernel log with this patches.
>>>>
>>>> https://patchwork.ozlabs.org/project/linux-tegra/list/?series=236215
>>>
>>> So sorry for the very late reply.  I have tried the patches but
>>> hit some issues on linux-next, it's not reaching a login prompt
>>> with next-20210422.  So I then tried with next-20210419 which
>>> does boot but shows the IOMMU error:
>>>
>>> <6>[2.995341] tegra-dc 5420.dc: Adding to iommu group 1
>>> <4>[3.001070] Failed to attached device 5420.dc to IOMMU_mapping  
>>>
>>>   https://lava.collabora.co.uk/scheduler/job/3570052#L1120
>>>
>>> The branch I'm using with the patches applied can be found here:
>>>
>>>   
>>> https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210419-nyan-big-drm-read/
>>>
>>> Hope this helps, let me know if you need anything else to be
>>> tested.
>>
>>
>> Hello Guillaume,
>>
>> The current linux-next doesn't boot on all ARM (AFAIK), the older
>> next-20210413 works. The above message should be unrelated to the boot
>> problem. It should be okay to ignore that message as it should be
>> harmless in yours case.
>>
> 
> Although, the 20210419 should be good.
> 
> Thierry, do you know what those SOR and Nouveau issues are about?
> 

I don't see DRM driver being loaded at all and no errors related to it
in the log. This means that likely some of the DRM sub-drivers is stuck
in deferred probe.

Guillaume, could you please show contents of
/sys/kernel/debug/devices_deferred ?

These messages also don't look good:

tegra-xusb-padctl 7009f000.padctl: failed to setup XUSB ports: -22
tegra-xusb-padctl: probe of 7009f000.padctl failed with error -22

I don't have T124 and all these problems should be specific to the T124
platform. Somebody with a direct access to hardware should look into it.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook

2021-04-24 Thread Dmitry Osipenko
23.04.2021 18:23, Dmitry Osipenko пишет:
> 23.04.2021 18:01, Guillaume Tucker пишет:
>> On 02/04/2021 15:40, Dmitry Osipenko wrote:
>>> 01.04.2021 11:55, Nicolin Chen пишет:
>>>> On Mon, Mar 29, 2021 at 02:32:56AM +0300, Dmitry Osipenko wrote:
>>>>> The previous commit fixes problem where display client was attaching too
>>>>> early to IOMMU during kernel boot in a multi-platform kernel configuration
>>>>> which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to
>>>>> defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore,
>>>>> revert it.
>>>>
>>>> Sorry for the late reply. I have been busy with downstream tasks.
>>>>
>>>> I will give them a try by the end of the week. Yet, probably it'd
>>>> be better to include Guillaume also as he has the Nyan platform.
>>>>
>>>
>>> Indeed, thanks. Although, I'm pretty sure that it's the same issue which
>>> I reproduced on Nexus 7.
>>>
>>> Guillaume, could you please give a test to these patches on Nyan Big?
>>> There should be no EMEM errors in the kernel log with this patches.
>>>
>>> https://patchwork.ozlabs.org/project/linux-tegra/list/?series=236215
>>
>> So sorry for the very late reply.  I have tried the patches but
>> hit some issues on linux-next, it's not reaching a login prompt
>> with next-20210422.  So I then tried with next-20210419 which
>> does boot but shows the IOMMU error:
>>
>> <6>[2.995341] tegra-dc 5420.dc: Adding to iommu group 1
>> <4>[3.001070] Failed to attached device 5420.dc to IOMMU_mapping  
>>
>>   https://lava.collabora.co.uk/scheduler/job/3570052#L1120
>>
>> The branch I'm using with the patches applied can be found here:
>>
>>   
>> https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210419-nyan-big-drm-read/
>>
>> Hope this helps, let me know if you need anything else to be
>> tested.
> 
> 
> Hello Guillaume,
> 
> The current linux-next doesn't boot on all ARM (AFAIK), the older
> next-20210413 works. The above message should be unrelated to the boot
> problem. It should be okay to ignore that message as it should be
> harmless in yours case.
> 

Although, the 20210419 should be good.

Thierry, do you know what those SOR and Nouveau issues are about?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/5] iommu: Support identity mappings of reserved-memory regions

2021-04-24 Thread Dmitry Osipenko
23.04.2021 19:32, Thierry Reding пишет:
> Hi,
> 
> this is an updated proposal to solve the problem of passing memory
> regions that are actively being accessed during boot. The particular
> use-case that I need this for is when the bootloader has set up the
> display controller to scan out a boot splash screen. During boot the
> DMA/IOMMU glue code will attach devices to an IOMMU domain and by
> doing so enable IOMMU translations. Typically this will be before a
> device driver has had a chance to either disable the display
> controller or set up a new framebuffer and map it to the IOMMU.

Hello Thierry,

Is it always safe to enable SMMU ASID in a middle of a DMA request made
by a memory client?

The memory controller supports blocking DMA requests, which we are
already using for the memory hot-resetting. A block could be needed
before ASID is toggled. This needs to be clarified.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook

2021-04-23 Thread Dmitry Osipenko
23.04.2021 18:01, Guillaume Tucker пишет:
> On 02/04/2021 15:40, Dmitry Osipenko wrote:
>> 01.04.2021 11:55, Nicolin Chen пишет:
>>> On Mon, Mar 29, 2021 at 02:32:56AM +0300, Dmitry Osipenko wrote:
>>>> The previous commit fixes problem where display client was attaching too
>>>> early to IOMMU during kernel boot in a multi-platform kernel configuration
>>>> which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to
>>>> defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore,
>>>> revert it.
>>>
>>> Sorry for the late reply. I have been busy with downstream tasks.
>>>
>>> I will give them a try by the end of the week. Yet, probably it'd
>>> be better to include Guillaume also as he has the Nyan platform.
>>>
>>
>> Indeed, thanks. Although, I'm pretty sure that it's the same issue which
>> I reproduced on Nexus 7.
>>
>> Guillaume, could you please give a test to these patches on Nyan Big?
>> There should be no EMEM errors in the kernel log with this patches.
>>
>> https://patchwork.ozlabs.org/project/linux-tegra/list/?series=236215
> 
> So sorry for the very late reply.  I have tried the patches but
> hit some issues on linux-next, it's not reaching a login prompt
> with next-20210422.  So I then tried with next-20210419 which
> does boot but shows the IOMMU error:
> 
> <6>[2.995341] tegra-dc 5420.dc: Adding to iommu group 1
> <4>[3.001070] Failed to attached device 5420.dc to IOMMU_mapping  
> 
>   https://lava.collabora.co.uk/scheduler/job/3570052#L1120
> 
> The branch I'm using with the patches applied can be found here:
> 
>   
> https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210419-nyan-big-drm-read/
> 
> Hope this helps, let me know if you need anything else to be
> tested.


Hello Guillaume,

The current linux-next doesn't boot on all ARM (AFAIK), the older
next-20210413 works. The above message should be unrelated to the boot
problem. It should be okay to ignore that message as it should be
harmless in yours case.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display clients

2021-04-09 Thread Dmitry Osipenko
08.04.2021 17:07, Dmitry Osipenko пишет:
>> Whatever happened to the idea of creating identity mappings based on the
>> obscure tegra_fb_mem (or whatever it was called) command-line option? Is
>> that command-line not universally passed to the kernel from bootloaders
>> that initialize display?
> This is still a good idea! The command-line isn't universally passed
> just because it's up to a user to override the cmdline and then we get a
> hang (a very slow boot in reality) on T30 since display client takes out
> the whole memory bus with the constant SMMU faults. For example I don't
> have that cmdline option in my current setups.
> 

Thinking a bit more about the identity.. have you consulted with the
memory h/w people about whether it's always safe to enable ASID in a
middle of DMA request?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display clients

2021-04-08 Thread Dmitry Osipenko
08.04.2021 16:26, Thierry Reding пишет:
> On Thu, Apr 08, 2021 at 02:42:42AM -0700, Nicolin Chen wrote:
>> On Mon, Mar 29, 2021 at 02:32:55AM +0300, Dmitry Osipenko wrote:
>>> All consumer-grade Android and Chromebook devices show a splash screen
>>> on boot and then display is left enabled when kernel is booted. This
>>> behaviour is unacceptable in a case of implicit IOMMU domains to which
>>> devices are attached during kernel boot since devices, like display
>>> controller, may perform DMA at that time. We can work around this problem
>>> by deferring the enable of SMMU translation for a specific devices,
>>> like a display controller, until the first IOMMU mapping is created,
>>> which works good enough in practice because by that time h/w is already
>>> stopped.
>>>
>>> Signed-off-by: Dmitry Osipenko 
>>
>> For both patches:
>> Acked-by: Nicolin Chen 
>> Tested-by: Nicolin Chen 
>>
>> The WAR looks good to me. Perhaps Thierry would give some input.

Nicolin, thank you very much for the help!

>> Another topic:
>> I think this may help work around the mc-errors, which we have
>> been facing on Tegra210 also when we enable IOMMU_DOMAIN_DMA.
>> (attached a test patch rebasing on these two)
> 
> Ugh... that's exactly what I was afraid of. Now everybody is going to
> think that we can just work around this issue with driver-specific SMMU
> hacks...
> 
>> However, GPU would also report errors using DMA domain:
>>
>>  nouveau 5700.gpu: acr: firmware unavailable
>>  nouveau 5700.gpu: pmu: firmware unavailable
>>  nouveau 5700.gpu: gr: firmware unavailable
>>  tegra-mc 70019000.memory-controller: gpusrd: read @0xfffbe200: 
>> Security violation (TrustZone violation)
>>  nouveau 5700.gpu: DRM: failed to create kernel channel, -22
>>  tegra-mc 70019000.memory-controller: gpusrd: read @0xfffad000: 
>> Security violation (TrustZone violation)
>>  nouveau 5700.gpu: fifo: SCHED_ERROR 20 []
>>  nouveau 5700.gpu: fifo: SCHED_ERROR 20 []
>>
>> Looking at the address, seems that GPU allocated memory in 32-bit
>> physical address space behind SMMU, so a violation happened after
>> turning on DMA domain I guess... 
> 
> The problem with GPU is... extra complicated. You're getting these
> faults because you're enabling the IOMMU-backed DMA API, which then
> causes the Nouveau driver allocate buffers using the DMA API instead of
> explicitly allocating pages and then mapping them using the IOMMU API.
> However, there are additional patches needed to teach Nouveau about how
> to deal with SMMU and those haven't been merged yet. I've got prototypes
> of this, but before the whole framebuffer carveout passing work makes
> progress there's little sense in moving individual pieces forward.
> 
> One more not to try and cut corners. We know what the right solution is,
> even if it takes a lot of work. I'm willing to ack this patch, or some
> version of it, but only as a way of working around things we have no
> realistic chance of fixing properly anymore. I still think it would be
> best if we could derive identity mappings from command-line arguments on
> these platforms because I think most of them will actually set that, and
> then the solution becomes at least uniform at the SMMU level.
> 
> For Tegra210 I've already laid out a path to a solution that's going to
> be generic and extend to Tegra186 and later as well.

We still have issues in the DRM and other drivers that don't allow us to
flip ON the IOMMU_DOMAIN_DMA support.

My patch addresses the issue with the ARM_DMA_USE_IOMMU option, which
allocates the unmanaged domain for DMA purposes on ARM32, causing the
trouble in the multiplatform kernel configuration since it's not
possible to opt-out from ARM_DMA_USE_IOMMU in this case. Perhaps this
needs to be clarified in the commit message.

https://elixir.bootlin.com/linux/v5.12-rc6/source/arch/arm/mm/dma-mapping.c#L2078

https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/iommu/iommu.c#L1929
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display clients

2021-04-08 Thread Dmitry Osipenko
08.04.2021 15:40, Thierry Reding пишет:
> On Mon, Mar 29, 2021 at 02:32:55AM +0300, Dmitry Osipenko wrote:
>> All consumer-grade Android and Chromebook devices show a splash screen
>> on boot and then display is left enabled when kernel is booted. This
>> behaviour is unacceptable in a case of implicit IOMMU domains to which
>> devices are attached during kernel boot since devices, like display
>> controller, may perform DMA at that time. We can work around this problem
>> by deferring the enable of SMMU translation for a specific devices,
>> like a display controller, until the first IOMMU mapping is created,
>> which works good enough in practice because by that time h/w is already
>> stopped.
>>
>> Signed-off-by: Dmitry Osipenko 
>> ---
>>  drivers/iommu/tegra-smmu.c | 71 ++
>>  1 file changed, 71 insertions(+)
> 
> In general I do see why we would want to enable this. However, I think
> this is a bad idea because it's going to proliferate the bad practice of
> not describing things properly in device tree.
> 
> Whatever happened to the idea of creating identity mappings based on the
> obscure tegra_fb_mem (or whatever it was called) command-line option? Is
> that command-line not universally passed to the kernel from bootloaders
> that initialize display?

This is still a good idea! The command-line isn't universally passed
just because it's up to a user to override the cmdline and then we get a
hang (a very slow boot in reality) on T30 since display client takes out
the whole memory bus with the constant SMMU faults. For example I don't
have that cmdline option in my current setups.

> That idealistic objection aside, this seems a bit over-engineered for
> the hack that it is. See below.
> 
>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>> index 602aab98c079..af1e4b5adb27 100644
>> --- a/drivers/iommu/tegra-smmu.c
>> +++ b/drivers/iommu/tegra-smmu.c
>> @@ -60,6 +60,8 @@ struct tegra_smmu_as {
>>  dma_addr_t pd_dma;
>>  unsigned id;
>>  u32 attr;
>> +bool display_attached[2];
>> +bool attached_devices_need_sync;
>>  };
>>  
>>  static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom)
>> @@ -78,6 +80,10 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu, 
>> unsigned long offset)
>>  return readl(smmu->regs + offset);
>>  }
>>  
>> +/* all Tegra SoCs use the same group IDs for displays */
>> +#define SMMU_SWGROUP_DC 1
>> +#define SMMU_SWGROUP_DCB2
>> +
>>  #define SMMU_CONFIG 0x010
>>  #define  SMMU_CONFIG_ENABLE (1 << 0)
>>  
>> @@ -253,6 +259,20 @@ static inline void smmu_flush(struct tegra_smmu *smmu)
>>  smmu_readl(smmu, SMMU_PTB_ASID);
>>  }
>>  
>> +static int smmu_swgroup_to_display_id(unsigned int swgroup)
>> +{
>> +switch (swgroup) {
>> +case SMMU_SWGROUP_DC:
>> +return 0;
>> +
>> +case SMMU_SWGROUP_DCB:
>> +return 1;
>> +
>> +default:
>> +return -1;
>> +}
>> +}
>> +
> 
> Why do we need to have this two-level mapping? Do we even need to care
> about the specific swgroups IDs?

It's not clear to me what you're meaning here, the swgroup IDs are used
here for determining whether client belongs to a display controller.

> Can we not just simply check at attach
> time if the client that's being attached is a display client and then
> set atteched_devices_need_sync = true?

The reason I made atteched_devices_need_sync opt-out for display clients
instead of
opt-in is to make it clear and easy to override this option once we will
support the identity mappings.

- attached_devices_need_sync = true;
+ attached_devices_need_sync = no_identity_mapping_for_display;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook

2021-04-02 Thread Dmitry Osipenko
01.04.2021 11:55, Nicolin Chen пишет:
> On Mon, Mar 29, 2021 at 02:32:56AM +0300, Dmitry Osipenko wrote:
>> The previous commit fixes problem where display client was attaching too
>> early to IOMMU during kernel boot in a multi-platform kernel configuration
>> which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to
>> defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore,
>> revert it.
> 
> Sorry for the late reply. I have been busy with downstream tasks.
> 
> I will give them a try by the end of the week. Yet, probably it'd
> be better to include Guillaume also as he has the Nyan platform.
> 

Indeed, thanks. Although, I'm pretty sure that it's the same issue which
I reproduced on Nexus 7.

Guillaume, could you please give a test to these patches on Nyan Big?
There should be no EMEM errors in the kernel log with this patches.

https://patchwork.ozlabs.org/project/linux-tegra/list/?series=236215
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v1 1/2] iommu/tegra-smmu: Defer attachment of display clients

2021-03-28 Thread Dmitry Osipenko
All consumer-grade Android and Chromebook devices show a splash screen
on boot and then display is left enabled when kernel is booted. This
behaviour is unacceptable in a case of implicit IOMMU domains to which
devices are attached during kernel boot since devices, like display
controller, may perform DMA at that time. We can work around this problem
by deferring the enable of SMMU translation for a specific devices,
like a display controller, until the first IOMMU mapping is created,
which works good enough in practice because by that time h/w is already
stopped.

Signed-off-by: Dmitry Osipenko 
---
 drivers/iommu/tegra-smmu.c | 71 ++
 1 file changed, 71 insertions(+)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 602aab98c079..af1e4b5adb27 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -60,6 +60,8 @@ struct tegra_smmu_as {
dma_addr_t pd_dma;
unsigned id;
u32 attr;
+   bool display_attached[2];
+   bool attached_devices_need_sync;
 };
 
 static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom)
@@ -78,6 +80,10 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu, 
unsigned long offset)
return readl(smmu->regs + offset);
 }
 
+/* all Tegra SoCs use the same group IDs for displays */
+#define SMMU_SWGROUP_DC1
+#define SMMU_SWGROUP_DCB   2
+
 #define SMMU_CONFIG 0x010
 #define  SMMU_CONFIG_ENABLE (1 << 0)
 
@@ -253,6 +259,20 @@ static inline void smmu_flush(struct tegra_smmu *smmu)
smmu_readl(smmu, SMMU_PTB_ASID);
 }
 
+static int smmu_swgroup_to_display_id(unsigned int swgroup)
+{
+   switch (swgroup) {
+   case SMMU_SWGROUP_DC:
+   return 0;
+
+   case SMMU_SWGROUP_DCB:
+   return 1;
+
+   default:
+   return -1;
+   }
+}
+
 static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp)
 {
unsigned long id;
@@ -318,6 +338,9 @@ static struct iommu_domain 
*tegra_smmu_domain_alloc(unsigned type)
as->domain.geometry.aperture_end = 0x;
as->domain.geometry.force_aperture = true;
 
+   /* work around implicit attachment of devices with active DMA */
+   as->attached_devices_need_sync = true;
+
return >domain;
 }
 
@@ -410,6 +433,31 @@ static void tegra_smmu_disable(struct tegra_smmu *smmu, 
unsigned int swgroup,
}
 }
 
+static void tegra_smmu_attach_deferred_devices(struct iommu_domain *domain)
+{
+   struct tegra_smmu_as *as = to_smmu_as(domain);
+
+   if (!as->attached_devices_need_sync)
+   return;
+
+   if (as->display_attached[0] || as->display_attached[1]) {
+   struct tegra_smmu *smmu = as->smmu;
+   unsigned int i;
+
+   for (i = 0; i < smmu->soc->num_clients; i++) {
+   const struct tegra_mc_client *client = 
>soc->clients[i];
+   const int disp_id = 
smmu_swgroup_to_display_id(client->swgroup);
+
+   if (disp_id < 0 || !as->display_attached[disp_id])
+   continue;
+
+   tegra_smmu_enable(smmu, client->swgroup, as->id);
+   }
+   }
+
+   as->attached_devices_need_sync = false;
+}
+
 static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
 struct tegra_smmu_as *as)
 {
@@ -495,10 +543,26 @@ static int tegra_smmu_attach_dev(struct iommu_domain 
*domain,
return -ENOENT;
 
for (index = 0; index < fwspec->num_ids; index++) {
+   const unsigned int swgroup = fwspec->ids[index];
+   const int disp_id = smmu_swgroup_to_display_id(swgroup);
+
err = tegra_smmu_as_prepare(smmu, as);
if (err)
goto disable;
 
+   if (disp_id >= 0) {
+   as->display_attached[disp_id] = true;
+
+   /*
+* In most cases display is performing DMA before
+* driver is initialized by showing a splash screen
+* and in this case we should defer the h/w attachment
+* until the first mapping is created by display driver.
+*/
+   if (as->attached_devices_need_sync)
+   continue;
+   }
+
tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
}
 
@@ -527,6 +591,12 @@ static void tegra_smmu_detach_dev(struct iommu_domain 
*domain, struct device *de
return;
 
for (index = 0; index < fwspec->num_ids; index++) {
+   const unsigned int swgroup = fwspec->ids[index];
+   const int disp_id = smmu_swgroup_to_display_id(swgroup)

[PATCH v1 2/2] iommu/tegra-smmu: Revert workaround that was needed for Nyan Big Chromebook

2021-03-28 Thread Dmitry Osipenko
The previous commit fixes problem where display client was attaching too
early to IOMMU during kernel boot in a multi-platform kernel configuration
which enables CONFIG_ARM_DMA_USE_IOMMU=y. The workaround that helped to
defer the IOMMU attachment for Nyan Big Chromebook isn't needed anymore,
revert it.

Signed-off-by: Dmitry Osipenko 
---
 drivers/iommu/tegra-smmu.c | 71 +-
 1 file changed, 1 insertion(+), 70 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index af1e4b5adb27..572a4544ae88 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -869,69 +869,10 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct 
iommu_domain *domain,
return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
 }
 
-static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
-{
-   struct platform_device *pdev;
-   struct tegra_mc *mc;
-
-   pdev = of_find_device_by_node(np);
-   if (!pdev)
-   return NULL;
-
-   mc = platform_get_drvdata(pdev);
-   if (!mc)
-   return NULL;
-
-   return mc->smmu;
-}
-
-static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
-   struct of_phandle_args *args)
-{
-   const struct iommu_ops *ops = smmu->iommu.ops;
-   int err;
-
-   err = iommu_fwspec_init(dev, >of_node->fwnode, ops);
-   if (err < 0) {
-   dev_err(dev, "failed to initialize fwspec: %d\n", err);
-   return err;
-   }
-
-   err = ops->of_xlate(dev, args);
-   if (err < 0) {
-   dev_err(dev, "failed to parse SW group ID: %d\n", err);
-   iommu_fwspec_free(dev);
-   return err;
-   }
-
-   return 0;
-}
-
 static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 {
-   struct device_node *np = dev->of_node;
-   struct tegra_smmu *smmu = NULL;
-   struct of_phandle_args args;
-   unsigned int index = 0;
-   int err;
-
-   while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- ) == 0) {
-   smmu = tegra_smmu_find(args.np);
-   if (smmu) {
-   err = tegra_smmu_configure(smmu, dev, );
-
-   if (err < 0) {
-   of_node_put(args.np);
-   return ERR_PTR(err);
-   }
-   }
-
-   of_node_put(args.np);
-   index++;
-   }
+   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
 
-   smmu = dev_iommu_priv_get(dev);
if (!smmu)
return ERR_PTR(-ENODEV);
 
@@ -1158,16 +1099,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
if (!smmu)
return ERR_PTR(-ENOMEM);
 
-   /*
-* This is a bit of a hack. Ideally we'd want to simply return this
-* value. However the IOMMU registration process will attempt to add
-* all devices to the IOMMU when bus_set_iommu() is called. In order
-* not to rely on global variables to track the IOMMU instance, we
-* set it here so that it can be looked up from the .probe_device()
-* callback via the IOMMU device's .drvdata field.
-*/
-   mc->smmu = smmu;
-
size = BITS_TO_LONGS(soc->num_asids) * sizeof(long);
 
smmu->asids = devm_kzalloc(dev, size, GFP_KERNEL);
-- 
2.30.2

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-03-28 Thread Dmitry Osipenko
28.03.2021 18:25, Dmitry Osipenko пишет:
> 03.03.2021 12:47, Dmitry Osipenko пишет:
>> 03.03.2021 02:08, Nicolin Chen пишет:
>>> On Sat, Feb 27, 2021 at 12:59:17PM +0300, Dmitry Osipenko wrote:
>>>> 25.02.2021 09:27, Nicolin Chen пишет:
>>>> ...
>>>>>> The partially revert should be okay, but it's not clear to me what makes
>>>>>> difference for T124 since I don't see that problem on T30, which also
>>>>>> has active display at a boot time.
>>>>> Hmm..do you see ->attach_dev() is called from host1x_client_iommu_attach
>>>>> or from of_dma_configure_id/arch_setup_dma_ops?
>>>>>
>>>> I applied yours debug-patch, please see dmesg.txt attached to the email.
>>>> Seems probe-defer of the tegra-dc driver prevents the implicit
>>>> tegra_smmu_attach_dev, so it happens to work by accident.
>>>> [0.327826] tegra-dc 5420.dc: ---tegra_smmu_of_xlate: id 1
>>>> [0.328641] [] (tegra_smmu_of_xlate) from [] 
>>>> (of_iommu_xlate+0x51/0x70)
>>>> [0.328740] [] (of_iommu_xlate) from [] 
>>>> (of_iommu_configure+0x127/0x150)
>>>> [0.328896] [] (of_iommu_configure) from [] 
>>>> (of_dma_configure_id+0x1fb/0x2ec)
>>>> [0.329060] [] (of_dma_configure_id) from [] 
>>>> (really_probe+0x7b/0x2a0)
>>>> [0.331438] tegra-dc 5420.dc: tegra_smmu_probe_device, 822
>>>> [0.332234] [] (tegra_smmu_probe_device) from [] 
>>>> (__iommu_probe_device+0x35/0x1c4)
>>>> [0.332391] [] (__iommu_probe_device) from [] 
>>>> (iommu_probe_device+0x19/0xec)
>>>> [0.332545] [] (iommu_probe_device) from [] 
>>>> (of_iommu_configure+0xfb/0x150)
>>>> [0.332701] [] (of_iommu_configure) from [] 
>>>> (of_dma_configure_id+0x1fb/0x2ec)
>>>> [0.332804] [] (of_dma_configure_id) from [] 
>>>> (really_probe+0x7b/0x2a0)
>>>> [0.335202] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1572
>>>> [0.335292] tegra-dc 5420.dc: -tegra_smmu_device_group, 862
>>>> [0.335474] tegra-dc 5420.dc: -tegra_smmu_device_group, 
>>>> 909: 1: drm
>>>> [0.335566] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1574
>>>> [0.335718] tegra-dc 5420.dc: -iommu_group_add_device, 858
>>>> [0.335862] tegra-dc 5420.dc: Adding to iommu group 1
>>>> [0.335955] tegra-dc 5420.dc: -iommu_alloc_default_domain, 
>>>> 1543: type 3
>>>> [0.336101] iommu: --iommu_group_alloc_default_domain: platform, 
>>>> (null), drm
>>>> [0.336187] -tegra_smmu_domain_alloc, 284: type 3
>>>  [0.336968] [] (tegra_smmu_domain_alloc) from [] 
>>> (iommu_group_alloc_default_domain+0x4b/0xfa)
>>>> [0.337127] [] (iommu_group_alloc_default_domain) from 
>>>> [] (iommu_probe_device+0x69/0xec)
>>>> [0.337285] [] (iommu_probe_device) from [] 
>>>> (of_iommu_configure+0xfb/0x150)
>>>> [0.337441] [] (of_iommu_configure) from [] 
>>>> (of_dma_configure_id+0x1fb/0x2ec)
>>>> [0.337599] [] (of_dma_configure_id) from [] 
>>>> (really_probe+0x7b/0x2a0)
>>>> [0.339913] tegra-dc 5420.dc: -iommu_probe_device, 272
>>>> [0.348144] tegra-dc 5420.dc: failed to probe RGB output: -517
>>> Hmm..not sure where this EPROBE_DEFER comes from.
>> DC driver on Nexus 7 depends on LVDS bridge and display panel, which
>> cause the probe defer.
>>
>>> But you are right,
>>> as of_dma_configure_id() returns because of that so it didn't run to
>>> arch_setup_dma_ops() call, which allocates an UNMANAGED iommu domain
>>> and attaches DC to it on Tegra124.
>>>
>>> By the way, anyone can accept this change? It doesn't feel right to
>>> leave a regression in the newer release...
> 
> Guys, I have a good and bad news.
> 
> The good news is that I figured out why I didn't see this problem on
> Nexus 7 and the reason is that I had CONFIG_ARM_DMA_USE_IOMMU=n.
> 
> The other good news is that I have a simple workaround which fixes the
> implicit IOMMU problem by deferring the ASID enabling for display clients.
> 
> The bad news is that CONFIG_ARM_DMA_USE_IOMMU=y breaks GPU (DRM, host1x)
> drivers because they aren't properly prepared to this case and
> CONFIG_ARM_DMA_USE_IOMMU is enabled in multi-platform kernel config. I
> will try to fix up the drivers, but not sure how much time this may take.
> 

Oh, actually the old workaround with arm_iommu_detach_device() still
works, so we just need to bring it back. I'll prepare the patches.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-03-28 Thread Dmitry Osipenko
03.03.2021 12:47, Dmitry Osipenko пишет:
> 03.03.2021 02:08, Nicolin Chen пишет:
>> On Sat, Feb 27, 2021 at 12:59:17PM +0300, Dmitry Osipenko wrote:
>>> 25.02.2021 09:27, Nicolin Chen пишет:
>>> ...
>>>>> The partially revert should be okay, but it's not clear to me what makes
>>>>> difference for T124 since I don't see that problem on T30, which also
>>>>> has active display at a boot time.
>>>> Hmm..do you see ->attach_dev() is called from host1x_client_iommu_attach
>>>> or from of_dma_configure_id/arch_setup_dma_ops?
>>>>
>>> I applied yours debug-patch, please see dmesg.txt attached to the email.
>>> Seems probe-defer of the tegra-dc driver prevents the implicit
>>> tegra_smmu_attach_dev, so it happens to work by accident.
>>> [0.327826] tegra-dc 5420.dc: ---tegra_smmu_of_xlate: id 1
>>> [0.328641] [] (tegra_smmu_of_xlate) from [] 
>>> (of_iommu_xlate+0x51/0x70)
>>> [0.328740] [] (of_iommu_xlate) from [] 
>>> (of_iommu_configure+0x127/0x150)
>>> [0.328896] [] (of_iommu_configure) from [] 
>>> (of_dma_configure_id+0x1fb/0x2ec)
>>> [0.329060] [] (of_dma_configure_id) from [] 
>>> (really_probe+0x7b/0x2a0)
>>> [0.331438] tegra-dc 5420.dc: tegra_smmu_probe_device, 822
>>> [0.332234] [] (tegra_smmu_probe_device) from [] 
>>> (__iommu_probe_device+0x35/0x1c4)
>>> [0.332391] [] (__iommu_probe_device) from [] 
>>> (iommu_probe_device+0x19/0xec)
>>> [0.332545] [] (iommu_probe_device) from [] 
>>> (of_iommu_configure+0xfb/0x150)
>>> [0.332701] [] (of_iommu_configure) from [] 
>>> (of_dma_configure_id+0x1fb/0x2ec)
>>> [0.332804] [] (of_dma_configure_id) from [] 
>>> (really_probe+0x7b/0x2a0)
>>> [0.335202] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1572
>>> [0.335292] tegra-dc 5420.dc: -tegra_smmu_device_group, 862
>>> [0.335474] tegra-dc 5420.dc: -tegra_smmu_device_group, 909: 
>>> 1: drm
>>> [0.335566] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1574
>>> [0.335718] tegra-dc 5420.dc: -iommu_group_add_device, 858
>>> [0.335862] tegra-dc 5420.dc: Adding to iommu group 1
>>> [0.335955] tegra-dc 5420.dc: -iommu_alloc_default_domain, 
>>> 1543: type 3
>>> [0.336101] iommu: --iommu_group_alloc_default_domain: platform, 
>>> (null), drm
>>> [0.336187] -tegra_smmu_domain_alloc, 284: type 3
>>  [0.336968] [] (tegra_smmu_domain_alloc) from [] 
>> (iommu_group_alloc_default_domain+0x4b/0xfa)
>>> [0.337127] [] (iommu_group_alloc_default_domain) from 
>>> [] (iommu_probe_device+0x69/0xec)
>>> [0.337285] [] (iommu_probe_device) from [] 
>>> (of_iommu_configure+0xfb/0x150)
>>> [0.337441] [] (of_iommu_configure) from [] 
>>> (of_dma_configure_id+0x1fb/0x2ec)
>>> [0.337599] [] (of_dma_configure_id) from [] 
>>> (really_probe+0x7b/0x2a0)
>>> [0.339913] tegra-dc 5420.dc: -iommu_probe_device, 272
>>> [0.348144] tegra-dc 5420.dc: failed to probe RGB output: -517
>> Hmm..not sure where this EPROBE_DEFER comes from.
> DC driver on Nexus 7 depends on LVDS bridge and display panel, which
> cause the probe defer.
> 
>> But you are right,
>> as of_dma_configure_id() returns because of that so it didn't run to
>> arch_setup_dma_ops() call, which allocates an UNMANAGED iommu domain
>> and attaches DC to it on Tegra124.
>>
>> By the way, anyone can accept this change? It doesn't feel right to
>> leave a regression in the newer release...

Guys, I have a good and bad news.

The good news is that I figured out why I didn't see this problem on
Nexus 7 and the reason is that I had CONFIG_ARM_DMA_USE_IOMMU=n.

The other good news is that I have a simple workaround which fixes the
implicit IOMMU problem by deferring the ASID enabling for display clients.

The bad news is that CONFIG_ARM_DMA_USE_IOMMU=y breaks GPU (DRM, host1x)
drivers because they aren't properly prepared to this case and
CONFIG_ARM_DMA_USE_IOMMU is enabled in multi-platform kernel config. I
will try to fix up the drivers, but not sure how much time this may take.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/9] arm64: tegra: Prevent early SMMU faults

2021-03-26 Thread Dmitry Osipenko
26.03.2021 19:55, Dmitry Osipenko пишет:
> 26.03.2021 19:35, Thierry Reding пишет:
>> On Fri, Mar 26, 2021 at 06:29:28PM +0300, Dmitry Osipenko wrote:
>>> 25.03.2021 16:03, Thierry Reding пишет:
>>>> From: Thierry Reding 
>>>>
>>>> Hi,
>>>>
>>>> this is a set of patches that is the result of earlier discussions
>>>> regarding early identity mappings that are needed to avoid SMMU faults
>>>> during early boot.
>>>>
>>>> The goal here is to avoid early identity mappings altogether and instead
>>>> postpone the need for the identity mappings to when devices are attached
>>>> to the SMMU. This works by making the SMMU driver coordinate with the
>>>> memory controller driver on when to start enforcing SMMU translations.
>>>> This makes Tegra behave in a more standard way and pushes the code to
>>>> deal with the Tegra-specific programming into the NVIDIA SMMU
>>>> implementation.
>>>
>>> It is an interesting idea which inspired me to try to apply a somewhat 
>>> similar thing to Tegra SMMU driver by holding the SMMU ASID enable-bit 
>>> until display driver allows to toggle it. This means that we will need an 
>>> extra small tegra-specific SMMU API function, but it should be okay.
>>>
>>> I typed a patch and seems it's working good, I'll prepare a proper patch if 
>>> you like it.
>>
>> That would actually be working around the problem that this patch was
>> supposed to prepare for. The reason for this current patch series is to
>> make sure SMMU translation isn't enabled until a device has actually
>> been attached to the SMMU. Once it has been attached, the assumption is
>> that any identity mappings will have been created.
>>
>> One Tegra SMMU that shouldn't be a problem because translations aren't
>> enabled until device attach time. So in other words this patch set is to
>> get Tegra186 and later to parity with earlier chips from this point of
>> view.
>>
>> I think the problem that you're trying to work around is better solved
>> by establishing these identity mappings. I do have patches to implement
>> this for Tegra210 and earlier, though they may require additional work
>> if you have bootloaders that don't use standard DT bindings for passing
>> information about the framebuffer to the kernel.
> 
> I'm not sure what else reasonable could be done without upgrading to a
> very specific version of firmware, which definitely isn't a variant for
> older devices which have a wild variety of bootloaders, customized
> use-cases and etc.
> 
> We could add a kludge that I'm suggesting as a universal fallback
> solution, it should work well for all cases that I care about.
> 
> So we could have the variant with identity mappings, and if mapping
> isn't provided, then fall back to the kludge.
> 

I tried a slightly different variant of the kludge by holding the ASID's
enable till the first mapping is created for the display clients and
IOMMU_DOMAIN_DMA now works properly (no EMEM errors on boot and etc) and
without a need to change the DC driver.

I also tried to remove the arm_iommu_detach_device() from the VDE driver
and we now have 3 implicit domains in use (DRM, HX, VDE[wasted]) + 1
explicit (VDE) on T30, which works okay for today. So technically we
could support the IOMMU_DOMAIN_DMA with a couple small changes right now
or at least revert the hacks that were needed for Nyan.

But in order to enable IOMMU_DOMAIN_DMA properly, we will need to do
something about the DMA mappings first in the DRM driver and I also
found that implicit IOMMU somehow doesn't work for host1x driver at all,
so this needs to be fixed too.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/9] arm64: tegra: Prevent early SMMU faults

2021-03-26 Thread Dmitry Osipenko
26.03.2021 19:35, Thierry Reding пишет:
> On Fri, Mar 26, 2021 at 06:29:28PM +0300, Dmitry Osipenko wrote:
>> 25.03.2021 16:03, Thierry Reding пишет:
>>> From: Thierry Reding 
>>>
>>> Hi,
>>>
>>> this is a set of patches that is the result of earlier discussions
>>> regarding early identity mappings that are needed to avoid SMMU faults
>>> during early boot.
>>>
>>> The goal here is to avoid early identity mappings altogether and instead
>>> postpone the need for the identity mappings to when devices are attached
>>> to the SMMU. This works by making the SMMU driver coordinate with the
>>> memory controller driver on when to start enforcing SMMU translations.
>>> This makes Tegra behave in a more standard way and pushes the code to
>>> deal with the Tegra-specific programming into the NVIDIA SMMU
>>> implementation.
>>
>> It is an interesting idea which inspired me to try to apply a somewhat 
>> similar thing to Tegra SMMU driver by holding the SMMU ASID enable-bit until 
>> display driver allows to toggle it. This means that we will need an extra 
>> small tegra-specific SMMU API function, but it should be okay.
>>
>> I typed a patch and seems it's working good, I'll prepare a proper patch if 
>> you like it.
> 
> That would actually be working around the problem that this patch was
> supposed to prepare for. The reason for this current patch series is to
> make sure SMMU translation isn't enabled until a device has actually
> been attached to the SMMU. Once it has been attached, the assumption is
> that any identity mappings will have been created.
> 
> One Tegra SMMU that shouldn't be a problem because translations aren't
> enabled until device attach time. So in other words this patch set is to
> get Tegra186 and later to parity with earlier chips from this point of
> view.
> 
> I think the problem that you're trying to work around is better solved
> by establishing these identity mappings. I do have patches to implement
> this for Tegra210 and earlier, though they may require additional work
> if you have bootloaders that don't use standard DT bindings for passing
> information about the framebuffer to the kernel.

I'm not sure what else reasonable could be done without upgrading to a
very specific version of firmware, which definitely isn't a variant for
older devices which have a wild variety of bootloaders, customized
use-cases and etc.

We could add a kludge that I'm suggesting as a universal fallback
solution, it should work well for all cases that I care about.

So we could have the variant with identity mappings, and if mapping
isn't provided, then fall back to the kludge.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/9] arm64: tegra: Prevent early SMMU faults

2021-03-26 Thread Dmitry Osipenko
25.03.2021 16:03, Thierry Reding пишет:
> From: Thierry Reding 
> 
> Hi,
> 
> this is a set of patches that is the result of earlier discussions
> regarding early identity mappings that are needed to avoid SMMU faults
> during early boot.
> 
> The goal here is to avoid early identity mappings altogether and instead
> postpone the need for the identity mappings to when devices are attached
> to the SMMU. This works by making the SMMU driver coordinate with the
> memory controller driver on when to start enforcing SMMU translations.
> This makes Tegra behave in a more standard way and pushes the code to
> deal with the Tegra-specific programming into the NVIDIA SMMU
> implementation.

It is an interesting idea which inspired me to try to apply a somewhat similar 
thing to Tegra SMMU driver by holding the SMMU ASID enable-bit until display 
driver allows to toggle it. This means that we will need an extra small 
tegra-specific SMMU API function, but it should be okay.

I typed a patch and seems it's working good, I'll prepare a proper patch if you 
like it.

What do you think about this:

diff --git a/drivers/gpu/drm/grate/dc.c b/drivers/gpu/drm/grate/dc.c
index 45a41586f153..8874cfba40a1 100644
--- a/drivers/gpu/drm/grate/dc.c
+++ b/drivers/gpu/drm/grate/dc.c
@@ -17,6 +17,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 #include 
@@ -2640,6 +2641,11 @@ static int tegra_dc_init(struct host1x_client *client)
return err;
}
 
+   if (dc->soc->sync_smmu) {
+   struct iommu_domain *domain = iommu_get_domain_for_dev(dc->dev);
+   tegra_smmu_sync_domain(domain, dc->dev);
+   }
+
if (dc->soc->wgrps)
primary = tegra_dc_add_shared_planes(drm, dc);
else
@@ -2824,6 +2830,7 @@ static const struct tegra_dc_soc_info tegra20_dc_soc_info 
= {
.has_win_b_vfilter_mem_client = true,
.has_win_c_without_vert_filter = true,
.plane_tiled_memory_bandwidth_x2 = false,
+   .sync_smmu = false,
 };
 
 static const struct tegra_dc_soc_info tegra30_dc_soc_info = {
@@ -2846,6 +2853,7 @@ static const struct tegra_dc_soc_info tegra30_dc_soc_info 
= {
.has_win_b_vfilter_mem_client = true,
.has_win_c_without_vert_filter = false,
.plane_tiled_memory_bandwidth_x2 = true,
+   .sync_smmu = true,
 };
 
 static const struct tegra_dc_soc_info tegra114_dc_soc_info = {
@@ -2868,6 +2876,7 @@ static const struct tegra_dc_soc_info 
tegra114_dc_soc_info = {
.has_win_b_vfilter_mem_client = false,
.has_win_c_without_vert_filter = false,
.plane_tiled_memory_bandwidth_x2 = true,
+   .sync_smmu = true,
 };
 
 static const struct tegra_dc_soc_info tegra124_dc_soc_info = {
@@ -2890,6 +2899,7 @@ static const struct tegra_dc_soc_info 
tegra124_dc_soc_info = {
.has_win_b_vfilter_mem_client = false,
.has_win_c_without_vert_filter = false,
.plane_tiled_memory_bandwidth_x2 = false,
+   .sync_smmu = true,
 };
 
 static const struct tegra_dc_soc_info tegra210_dc_soc_info = {
@@ -2912,6 +2922,7 @@ static const struct tegra_dc_soc_info 
tegra210_dc_soc_info = {
.has_win_b_vfilter_mem_client = false,
.has_win_c_without_vert_filter = false,
.plane_tiled_memory_bandwidth_x2 = false,
+   .sync_smmu = true,
 };
 
 static const struct tegra_windowgroup_soc tegra186_dc_wgrps[] = {
@@ -2961,6 +2972,7 @@ static const struct tegra_dc_soc_info 
tegra186_dc_soc_info = {
.wgrps = tegra186_dc_wgrps,
.num_wgrps = ARRAY_SIZE(tegra186_dc_wgrps),
.plane_tiled_memory_bandwidth_x2 = false,
+   .sync_smmu = false,
 };
 
 static const struct tegra_windowgroup_soc tegra194_dc_wgrps[] = {
@@ -3010,6 +3022,7 @@ static const struct tegra_dc_soc_info 
tegra194_dc_soc_info = {
.wgrps = tegra194_dc_wgrps,
.num_wgrps = ARRAY_SIZE(tegra194_dc_wgrps),
.plane_tiled_memory_bandwidth_x2 = false,
+   .sync_smmu = false,
 };
 
 static const struct of_device_id tegra_dc_of_match[] = {
diff --git a/drivers/gpu/drm/grate/dc.h b/drivers/gpu/drm/grate/dc.h
index 316a56131cf1..e0057bf7be99 100644
--- a/drivers/gpu/drm/grate/dc.h
+++ b/drivers/gpu/drm/grate/dc.h
@@ -91,6 +91,7 @@ struct tegra_dc_soc_info {
bool has_win_b_vfilter_mem_client;
bool has_win_c_without_vert_filter;
bool plane_tiled_memory_bandwidth_x2;
+   bool sync_smmu;
 };
 
 struct tegra_dc {
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 602aab98c079..e750b1844a88 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -47,6 +47,9 @@ struct tegra_smmu {
struct dentry *debugfs;
 
struct iommu_device iommu;  /* IOMMU Core code handle */
+
+   bool display_synced[2];
+   bool display_enabled[2];
 };
 
 struct tegra_smmu_as {
@@ -78,6 +81,10 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu, 
unsigned long offset)
return readl(smmu->regs + offset);
 }
 

Re: [PATCH 1/9] memory: tegra: Move internal data structures into separate header

2021-03-26 Thread Dmitry Osipenko
25.03.2021 19:11, Dmitry Osipenko пишет:
> 25.03.2021 18:52, Thierry Reding пишет:
>> On Thu, Mar 25, 2021 at 06:12:51PM +0300, Dmitry Osipenko wrote:
>>> 25.03.2021 16:03, Thierry Reding пишет:
>>>> From: Thierry Reding 
>>>>
>>>> From Tegra20 through Tegra210, either the GART or SMMU drivers need
>>>> access to the internals of the memory controller driver because they are
>>>> tightly coupled (in fact, the GART and SMMU are part of the memory
>>>> controller). On later chips, a separate hardware block implements the
>>>> SMMU functionality, so this is no longer needed. However, we still want
>>>> to reuse some of the existing infrastructure on later chips, so split
>>>> the memory controller internals into a separate header file to avoid
>>>> conflicts with the implementation on newer chips.
>>>>
>>>> Signed-off-by: Thierry Reding 
>>>> ---
>>>>  drivers/iommu/tegra-gart.c  |  2 +-
>>>>  drivers/iommu/tegra-smmu.c  |  2 +-
>>>>  drivers/memory/tegra/mc.h   |  2 +-
>>>>  drivers/memory/tegra/tegra186.c | 12 ---
>>>>  include/soc/tegra/mc-internal.h | 62 +
>>>>  include/soc/tegra/mc.h  | 50 --
>>>>  6 files changed, 72 insertions(+), 58 deletions(-)
>>>>  create mode 100644 include/soc/tegra/mc-internal.h
>>>
>>> What about to make T186 to re-use the existing tegra_mc struct? Seems
>>> there is nothing special in that struct which doesn't fit for the newer
>>> SoCs. Please notice that both SMMU and GART are already optional and all
>>> the SoC differences are specified within the tegra_mc_soc. It looks to
>>> me that this could be a much nicer and cleaner variant.
>>
>> The problem is that much of the interesting bits in tegra_mc_soc are
>> basically incompatible between the two. For instance the tegra_mc_client
>> and tegra186_mc_client structures, while they have the same purpose,
>> have completely different content. I didn't see a way to unify that
>> without overly complicating things by making half of the fields
>> basically optional on one or the other SoC generation.
> 
> The additional fields aren't problem for T20, which doesn't need most of
> the fields. I'd try to go with the additional fields for now and see how
> it will look like, if it will be bothering too much, then we may
> consider to refactor the drivers more thoroughly (later on, in a
> separate series), with a better/nicer separation and taking into account
> a potential modularization support by the MC drivers.
> 
> Using a union for the exclusive fields also could work, although always
> need to be extra careful with the unions.
> 
>> Maybe one option would be to split tegra_mc into a tegra_mc_common and
>> then derive tegra_mc and tegra186_mc from that. That way we could share
>> the common bits while still letting the chip-specific differences be
>> handled separately.
> 
> But isn't tegra_mc already a superset of tegra186_mc? I think the
> tegra186_mc_client is the main difference here.
> 

Another thing we could do is to optimize the size of tegra_mc_client, but not 
sure whether it's worthwhile to care about extra ~3KB of data.

This slims down tegra_mc_client by two times:

 diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index edea9b2b406e..1d652bfc6b44 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -317,11 +317,11 @@ static int tegra_mc_setup_latency_allowance(struct 
tegra_mc *mc)
/* write latency allowance defaults */
for (i = 0; i < mc->soc->num_clients; i++) {
const struct tegra_mc_la *la = >soc->clients[i].la;
-   u32 value;
+   u32 value, la_mask = la->mask, la_def = la->def;
 
value = mc_readl(mc, la->reg);
-   value &= ~(la->mask << la->shift);
-   value |= (la->def & la->mask) << la->shift;
+   value &= ~(la_mask << la->shift);
+   value |= (la_def & la_mask) << la->shift;
mc_writel(mc, value, la->reg);
}
 
diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
index 46332fa82d10..ecf05484d656 100644
--- a/drivers/memory/tegra/tegra30.c
+++ b/drivers/memory/tegra/tegra30.c
@@ -1157,7 +1157,7 @@ static void tegra30_mc_tune_client_latency(struct 
tegra_mc *mc,
u32 arb_tolerance_compensation_nsec, arb_tolerance_compensation_div;
const struct tegra_mc_la *la = >la;
unsigned int fifo

Re: [PATCH 1/9] memory: tegra: Move internal data structures into separate header

2021-03-25 Thread Dmitry Osipenko
25.03.2021 18:52, Thierry Reding пишет:
> On Thu, Mar 25, 2021 at 06:12:51PM +0300, Dmitry Osipenko wrote:
>> 25.03.2021 16:03, Thierry Reding пишет:
>>> From: Thierry Reding 
>>>
>>> From Tegra20 through Tegra210, either the GART or SMMU drivers need
>>> access to the internals of the memory controller driver because they are
>>> tightly coupled (in fact, the GART and SMMU are part of the memory
>>> controller). On later chips, a separate hardware block implements the
>>> SMMU functionality, so this is no longer needed. However, we still want
>>> to reuse some of the existing infrastructure on later chips, so split
>>> the memory controller internals into a separate header file to avoid
>>> conflicts with the implementation on newer chips.
>>>
>>> Signed-off-by: Thierry Reding 
>>> ---
>>>  drivers/iommu/tegra-gart.c  |  2 +-
>>>  drivers/iommu/tegra-smmu.c  |  2 +-
>>>  drivers/memory/tegra/mc.h   |  2 +-
>>>  drivers/memory/tegra/tegra186.c | 12 ---
>>>  include/soc/tegra/mc-internal.h | 62 +
>>>  include/soc/tegra/mc.h  | 50 --
>>>  6 files changed, 72 insertions(+), 58 deletions(-)
>>>  create mode 100644 include/soc/tegra/mc-internal.h
>>
>> What about to make T186 to re-use the existing tegra_mc struct? Seems
>> there is nothing special in that struct which doesn't fit for the newer
>> SoCs. Please notice that both SMMU and GART are already optional and all
>> the SoC differences are specified within the tegra_mc_soc. It looks to
>> me that this could be a much nicer and cleaner variant.
> 
> The problem is that much of the interesting bits in tegra_mc_soc are
> basically incompatible between the two. For instance the tegra_mc_client
> and tegra186_mc_client structures, while they have the same purpose,
> have completely different content. I didn't see a way to unify that
> without overly complicating things by making half of the fields
> basically optional on one or the other SoC generation.

The additional fields aren't problem for T20, which doesn't need most of
the fields. I'd try to go with the additional fields for now and see how
it will look like, if it will be bothering too much, then we may
consider to refactor the drivers more thoroughly (later on, in a
separate series), with a better/nicer separation and taking into account
a potential modularization support by the MC drivers.

Using a union for the exclusive fields also could work, although always
need to be extra careful with the unions.

> Maybe one option would be to split tegra_mc into a tegra_mc_common and
> then derive tegra_mc and tegra186_mc from that. That way we could share
> the common bits while still letting the chip-specific differences be
> handled separately.

But isn't tegra_mc already a superset of tegra186_mc? I think the
tegra186_mc_client is the main difference here.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/9] memory: tegra: Move internal data structures into separate header

2021-03-25 Thread Dmitry Osipenko
25.03.2021 16:03, Thierry Reding пишет:
> From: Thierry Reding 
> 
> From Tegra20 through Tegra210, either the GART or SMMU drivers need
> access to the internals of the memory controller driver because they are
> tightly coupled (in fact, the GART and SMMU are part of the memory
> controller). On later chips, a separate hardware block implements the
> SMMU functionality, so this is no longer needed. However, we still want
> to reuse some of the existing infrastructure on later chips, so split
> the memory controller internals into a separate header file to avoid
> conflicts with the implementation on newer chips.
> 
> Signed-off-by: Thierry Reding 
> ---
>  drivers/iommu/tegra-gart.c  |  2 +-
>  drivers/iommu/tegra-smmu.c  |  2 +-
>  drivers/memory/tegra/mc.h   |  2 +-
>  drivers/memory/tegra/tegra186.c | 12 ---
>  include/soc/tegra/mc-internal.h | 62 +
>  include/soc/tegra/mc.h  | 50 --
>  6 files changed, 72 insertions(+), 58 deletions(-)
>  create mode 100644 include/soc/tegra/mc-internal.h

What about to make T186 to re-use the existing tegra_mc struct? Seems
there is nothing special in that struct which doesn't fit for the newer
SoCs. Please notice that both SMMU and GART are already optional and all
the SoC differences are specified within the tegra_mc_soc. It looks to
me that this could be a much nicer and cleaner variant.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs

2021-03-16 Thread Dmitry Osipenko
16.03.2021 14:16, Thierry Reding пишет:
>> +seq_puts(s, "}\n");
>> +seq_printf(s, "Total PDE count: %u\n", pde_count);
>> +seq_printf(s, "Total PTE count: %llu\n", pte_count);
> Some of the above looks like it wouldn't be very easily consumed by
> scripts. Is that something we want to do? Or is this targetted primarily
> at human consumption?

Output should be parsable using a simple regex. Could you please clarify
what exactly isn't easy?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs

2021-03-16 Thread Dmitry Osipenko
15.03.2021 23:36, Nicolin Chen пишет:
> +static int tegra_smmu_mappings_show(struct seq_file *s, void *data)
> +{
> + struct tegra_smmu_group_debug *group_debug = s->private;
> + const struct tegra_smmu_swgroup *group;
> + struct tegra_smmu_as *as;
> + struct tegra_smmu *smmu;
> + unsigned int pd_index;
> + unsigned int pt_index;
> + unsigned long flags;
> + u64 pte_count = 0;
> + u32 pde_count = 0;
> + u32 val, ptb_reg;
> + u32 *pd;
> +
> + if (!group_debug || !group_debug->priv || !group_debug->group)
> + return 0;

I'm also now curious how difficult would be to read out the actual h/w
state, i.e. check whether ASID is enabled and then dynamically map the
pointed pages instead of using pages allocated by driver. This will show
us the real h/w state. For example this may show mappings left after
bootloader or after reboot/kexec, which could be handy to see.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs

2021-03-16 Thread Dmitry Osipenko
15.03.2021 23:36, Nicolin Chen пишет:
> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int 
> pt_index)
> +{
> + return ((dma_addr_t)pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> +((dma_addr_t)pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> +}

Looking at this again, I'm now wondering whether will be better to
replace dma_addr_t with u32 everywhere since SMMU only supports 32bits
for IOVA.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4] iommu/tegra-smmu: Add pagetable mappings to debugfs

2021-03-15 Thread Dmitry Osipenko
15.03.2021 06:35, Nicolin Chen пишет:
> This patch dumps all active mapping entries from pagetable
> to a debugfs directory named "mappings".
> 
> Ataching an example:

Attaching

> 
> SWGROUP: hc
> ASID: 0
> reg: 0x250
> PTB_ASID: 0xe0080004
> as->pd_dma: 0x80004000
> {
> [1023] 0xf008000b (1)
> {
> PTE RANGE  | ATTR | PHYS   | IOVA 
>   | SIZE
> [#1023, #1023] | 0x5  | 0x000111a8d000 | 
> 0xf000 | 0x1000
> }
> }
> Total PDE count: 1
> Total PTE count: 1
> 
> Signed-off-by: Nicolin Chen 
> ---

Good to me, thanks.

Tested-by: Dmitry Osipenko 
Reviewed-by: Dmitry Osipenko 

> + for (pd_index = 0; pd_index < SMMU_NUM_PDE; pd_index++) {
> + struct page *pt_page;
> + u32 *addr;
> + unsigned int i;

Unimportant nit: I'd keep lines arranged by length for consistency with
the rest of the code.

...
> + group_debug = devm_kcalloc(dev, soc->num_swgroups, 
> sizeof(*group_debug), GFP_KERNEL);

Another nit: this is a long line, I'd split it into two lines to keep
coding style consistent and to improve readability for those who have a
side-by-side code viewing setup.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3] iommu/tegra-smmu: Add pagetable mappings to debugfs

2021-03-14 Thread Dmitry Osipenko
14.03.2021 11:06, Nicolin Chen пишет:
> + for (pd_index = 0; pd_index < SMMU_NUM_PDE; pd_index++) {
> + struct page *pt_page;
> + u32 *addr;
> + int i;

unsigned int

and then printf specifiers also should be %u
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3] iommu/tegra-smmu: Add pagetable mappings to debugfs

2021-03-14 Thread Dmitry Osipenko
14.03.2021 11:06, Nicolin Chen пишет:
> This patch dumps all active mapping entries from pagetable
> to a debugfs directory named "mappings".
> 
> Ataching an example:
> 
> SWGROUP: hc
> ASID: 0
> reg: 0x250
> PTB_ASID: 0xe0080004
> as->pd_dma: 0x80004000
> {
> [1023] 0xf008000b (1)
> {
> PTE RANGE  | ATTR | PHYS   | IOVA 
>   | SIZE
> [#1023, #1023] | 0x5  | 0x000111a8d000 | 
> 0xf000 | 0x1000
> }
> }
> Total PDE count: 1
> Total PTE count: 1
> 
> Signed-off-by: Nicolin Chen 
> ---
> Changelog
> v3:
>  * Fixed PHYS and IOVA print formats
>  * Changed variables to unsigned int type
>  * Changed the table outputs to be compact
> v2: https://lkml.org/lkml/2021/3/9/1382
>  * Expanded mutex range to the entire function
>  * Added as->lock to protect pagetable walkthrough
>  * Replaced devm_kzalloc with devm_kcalloc for group_debug
>  * Added "PTE RANGE" and "SIZE" columns to group contiguous mappings
>  * Dropped as->count check; added WARN_ON when as->count mismatches 
> pd[pd_index]
> v1: https://lkml.org/lkml/2020/9/26/70
> 
>  drivers/iommu/tegra-smmu.c | 175 +++--
>  1 file changed, 170 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 97eb62f667d2..269737d51ad4 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -19,6 +19,11 @@
>  #include 
>  #include 
>  
> +struct tegra_smmu_group_debug {
> + const struct tegra_smmu_swgroup *group;
> + void *priv;
> +};
> +
>  struct tegra_smmu_group {
>   struct list_head list;
>   struct tegra_smmu *smmu;
> @@ -47,6 +52,8 @@ struct tegra_smmu {
>   struct dentry *debugfs;
>  
>   struct iommu_device iommu;  /* IOMMU Core code handle */
> +
> + struct tegra_smmu_group_debug *group_debug;
>  };
>  
>  struct tegra_smmu_as {
> @@ -152,6 +159,9 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu, 
> unsigned long offset)
>  
>  #define SMMU_PDE_ATTR(SMMU_PDE_READABLE | SMMU_PDE_WRITABLE 
> | \
>SMMU_PDE_NONSECURE)
> +#define SMMU_PTE_ATTR(SMMU_PTE_READABLE | SMMU_PTE_WRITABLE 
> | \
> +  SMMU_PTE_NONSECURE)
> +#define SMMU_PTE_ATTR_SHIFT  (29)
>  
>  static unsigned int iova_pd_index(unsigned long iova)
>  {
> @@ -163,6 +173,12 @@ static unsigned int iova_pt_index(unsigned long iova)
>   return (iova >> SMMU_PTE_SHIFT) & (SMMU_NUM_PTE - 1);
>  }
>  
> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int 
> pt_index)
> +{
> + return ((dma_addr_t)pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> +((dma_addr_t)pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> +}
> +
>  static bool smmu_dma_addr_valid(struct tegra_smmu *smmu, dma_addr_t addr)
>  {
>   addr >>= 12;
> @@ -334,7 +350,7 @@ static void tegra_smmu_domain_free(struct iommu_domain 
> *domain)
>  }
>  
>  static const struct tegra_smmu_swgroup *
> -tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
> +tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup, int 
> *index)
>  {
>   const struct tegra_smmu_swgroup *group = NULL;
>   unsigned int i;
> @@ -342,6 +358,8 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned 
> int swgroup)
>   for (i = 0; i < smmu->soc->num_swgroups; i++) {
>   if (smmu->soc->swgroups[i].swgroup == swgroup) {
>   group = >soc->swgroups[i];
> + if (index)
> + *index = i;
>   break;
>   }
>   }
> @@ -350,19 +368,22 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu, 
> unsigned int swgroup)
>  }
>  
>  static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
> -   unsigned int asid)
> +   struct tegra_smmu_as *as)
>  {
>   const struct tegra_smmu_swgroup *group;
> + unsigned int asid = as->id;
>   unsigned int i;
>   u32 value;
>  
> - group = tegra_smmu_find_swgroup(smmu, swgroup);
> + group = tegra_smmu_find_swgroup(smmu, swgroup, );
>   if (group) {
>   value = smmu_readl(smmu, group->reg);
>   value &= ~SMMU_ASID_MASK;
>   value |= SMMU_ASID_VALUE(asid);
>   value |= SMMU_ASID_ENABLE;
>   smmu_writel(smmu, value, group->reg);
> + if (smmu->group_debug)
> + smmu->group_debug[i].priv = as;
>   } else {
>   pr_warn("%s group from swgroup %u not found\n", __func__,
>   swgroup);
> @@ -389,13 +410,15 @@ static void tegra_smmu_disable(struct tegra_smmu *smmu, 
> unsigned int swgroup,
>   unsigned int i;
>   u32 value;
>  
> - group = tegra_smmu_find_swgroup(smmu, swgroup);
> + group = 

[PATCH v1] iommu/tegra-smmu: Make tegra_smmu_probe_device() to handle all IOMMU phandles

2021-03-12 Thread Dmitry Osipenko
The tegra_smmu_probe_device() handles only the first IOMMU device-tree
phandle, skipping the rest. Devices like 3D module on Tegra30 have
multiple IOMMU phandles, one for each h/w block, and thus, only one
IOMMU phandle is added to fwspec for the 3D module, breaking GPU.
Previously this problem was masked by tegra_smmu_attach_dev() which
didn't use the fwspec, but parsed the DT by itself. The previous commit
to tegra-smmu driver partially reverted changes that caused problems for
T124 and now we have tegra_smmu_attach_dev() that uses the fwspec and
the old-buggy variant of tegra_smmu_probe_device() which skips secondary
IOMMUs.

Make tegra_smmu_probe_device() not to skip the secondary IOMMUs. This
fixes a partially attached IOMMU of the 3D module on Tegra30 and now GPU
works properly once again.

Fixes: 765a9d1d02b2 ("iommu/tegra-smmu: Fix mc errors on tegra124-nyan")
Signed-off-by: Dmitry Osipenko 
---
 drivers/iommu/tegra-smmu.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 97eb62f667d2..602aab98c079 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -849,12 +849,11 @@ static struct iommu_device 
*tegra_smmu_probe_device(struct device *dev)
smmu = tegra_smmu_find(args.np);
if (smmu) {
err = tegra_smmu_configure(smmu, dev, );
-   of_node_put(args.np);
 
-   if (err < 0)
+   if (err < 0) {
+   of_node_put(args.np);
return ERR_PTR(err);
-
-   break;
+   }
}
 
of_node_put(args.np);
-- 
2.29.2

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v2] iommu/tegra-smmu: Add pagetable mappings to debugfs

2021-03-11 Thread Dmitry Osipenko
10.03.2021 06:36, Nicolin Chen пишет:
...
> +static int tegra_smmu_mappings_show(struct seq_file *s, void *data)
> +{
> + struct tegra_smmu_group_debug *group_debug = s->private;
> + const struct tegra_smmu_swgroup *group;
> + struct tegra_smmu_as *as;
> + struct tegra_smmu *smmu;
> + int pd_index, pt_index;

> +DEFINE_SHOW_ATTRIBUTE(tegra_smmu_mappings);
> +
>  static void tegra_smmu_debugfs_init(struct tegra_smmu *smmu)
>  {
> + const struct tegra_smmu_soc *soc = smmu->soc;
> + struct tegra_smmu_group_debug *group_debug;
> + struct device *dev = smmu->dev;
> + struct dentry *d;
> + int i;

You should use unsigned types for everything that isn't signed.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-03-11 Thread Dmitry Osipenko
11.03.2021 01:17, Nicolin Chen пишет:
> On Wed, Mar 10, 2021 at 11:22:57PM +0300, Dmitry Osipenko wrote:
>> 10.03.2021 22:13, Dmitry Osipenko пишет:
>>> I found that this patch introduced a serious regression on Tegra30 using
>>> today's linux-next. Tegra30 has two 3d h/w blocks connected in SLI and
>>> only one of the blocks is now attached to IOMMU domain, meaning that GPU
>>> is unusable now. All 3d, 2d and display devices share the same "DRM"
>>> group on Tegra30.
>>>
>>> Nicolin, please let me know if have any suggestions. I may take a closer
>>> look a day later, for now I'll just revert this patch locally. Thanks in
>>> advance.
>>>
>>
>> Actually, this was easy to fix:
>>
>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>> index 97eb62f667d2..639d5ceab60b 100644
>> --- a/drivers/iommu/tegra-smmu.c
>> +++ b/drivers/iommu/tegra-smmu.c
>> @@ -853,8 +853,6 @@ static struct iommu_device
>> *tegra_smmu_probe_device(struct device *dev)
>>
>>  if (err < 0)
>>  return ERR_PTR(err);
>> -
>> -break;
> 
> Hmm..I don't understand why this "break" causes problems on Tegra30.
> The older versions that used _find()+configure() had it also, e.g.:
> https://elixir.bootlin.com/linux/v5.9.16/source/drivers/iommu/tegra-smmu.c#L760
> 
> Dmitry, do you have any idea?
> 

The older variant of tegra_smmu_attach_dev() didn't use fwspec [1], that
makes the difference. In other words, the older variant of
tegra_smmu_probe_device() was already buggy, but the bug was masked by
the tegra_smmu_attach_dev() that didn't use the fwspec.

[1]
https://elixir.bootlin.com/linux/v5.10.22/source/drivers/iommu/tegra-smmu.c#L476
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu/tegra-smmu: Add pagetable mappings to debugfs

2021-03-10 Thread Dmitry Osipenko
10.03.2021 06:36, Nicolin Chen пишет:
> This patch dumps all active mapping entries from pagetable
> to a debugfs directory named "mappings".
> 
> Ataching an example:
> 
> SWGROUP: hc
> ASID: 0
> reg: 0x250
> PTB_ASID: 0xe0080004
> as->pd_dma: 0x80004000
> {
> [1023] 0xf0080013 (1)
> {
> PTE RANGE   PHYS   IOVASIZEATTR
> #1023 - #1023   0x122e5e0000xf000  0x1000  0x5
> }
> }
> Total PDE count: 1
> Total PTE count: 1

Addresses are 32bit on ARM32, please fix the formatting. The phys_addr_t
and dma_addr_t have special printk specifiers [1].

[1]
https://www.kernel.org/doc/html/latest/core-api/printk-formats.html?highlight=printk#physical-address-types-phys-addr-t

as->pd_dma: 0xc026e81b026c
{
[0] 0xf0082848 (1024)
{
PTE RANGE   PHYS   IOVASIZE
ATTR
#0- #15 0xc0f9fc40bfde 0x0 0x1
   0x7
#16   - #47 0xc0f9fc40bfdc 0x1 0x2
   0x7
#48   - #1110xc0f9fc40bfd8 0x3 0x4
   0x7
#112  - #2390xc0f9fc40bfd0 0x7 0x8
   0x7
#240  - #4950xc0f9fc40bfc0 0xf 0x10
   0x7
#496  - #9990xc0f9fc40bf40 0x1f0x1f8000
   0x7
#1000 - #1000   0xc0f9fc40acc4 0x3e80000x1000
   0x7
#1001 - #1001   0xc0f9fc40bbb95000 0x3e90000x1000
   0x7
#1002 - #1002   0xc0f9fc40bbb97000 0x3ea0000x1000
   0x7
#1003 - #1003   0xc0f9fc40bbb91000 0x3eb0000x1000
   0x7
#1004 - #1005   0xc0f9fc40bb455000 0x3ec0000x2000
   0x7
#1006 - #1006   0xc0f9fc40bee9c000 0x3ee0000x1000
   0x7
#1007 - #1007   0xc0f9fc40bed2 0x3ef0000x1000
   0x7
#1008 - #1008   0xc0f9fc40bbde 0x3f0x1000
   0x7
#1009 - #1009   0xc0f9fc40be465000 0x3f10000x1000
   0x7
#1010 - #1010   0xc0f9fc40bb505000 0x3f20000x1000
   0x7
#1011 - #1011   0xc0f9fc40bdcb9000 0x3f30000x1000
   0x7
#1012 - #1012   0xc0f9fc40bcdf9000 0x3f40000x1000
   0x7
#1013 - #1013   0xc0f9fc40bb6ce000 0x3f50000x1000
   0x7
#1014 - #1014   0xc0f9fc40bcd74000 0x3f60000x1000
   0x7
#1015 - #1015   0xc0f9fc40bd0d 0x3f70000x1000
   0x7
#1016 - #1016   0xc0f9fc40bb6c5000 0x3f80000x1000
   0x7
#1017 - #1017   0xc0f9fc40bd121000 0x3f90000x1000
   0x7
#1018 - #1018   0xc0f9fc40bb6df000 0x3fa0000x1000
   0x7
#1019 - #1020   0xc0f9fc40bb6e9000 0x3fb0000x2000
   0x7
#1021 - #1021   0xc0f9fc40bb713000 0x3fd0000x1000
   0x7
#1022 - #1022   0xc0f9fc40bb738000 0x3fe0000x1000
   0x7
#1023 - #1023   0xc0f9fc40be444000 0x3ff0000x1000
   0x7
}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-03-10 Thread Dmitry Osipenko
10.03.2021 22:13, Dmitry Osipenko пишет:
> 19.02.2021 01:07, Nicolin Chen пишет:
>> Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
>> removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
>> of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
>> tegra_smmu_configure() that are typically done in the IOMMU core also.
>>
>> This approach works for both existing devices that have DT nodes and other
>> devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
>> upon testing. However, Page Fault errors are reported on tegra124-Nyan:
>>
>>   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>>   EMEM address decode error (SMMU translation error [--S])
>>   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>>   Page fault (SMMU translation error [--S])
>>
>> After debugging, I found that the mentioned commit changed some function
>> callback sequence of tegra-smmu's, resulting in enabling SMMU for display
>> client before display driver gets initialized. I couldn't reproduce exact
>> same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.
>>
>> Actually this Page Fault is a known issue, as on most of Tegra platforms,
>> display gets enabled by the bootloader for the splash screen feature, so
>> it keeps filling the framebuffer memory. A proper fix to this issue is to
>> 1:1 linear map the framebuffer memory to IOVA space so the SMMU will have
>> the same address as the physical address in its page table. Yet, Thierry
>> has been working on the solution above for a year, and it hasn't merged.
>>
>> Therefore, let's partially revert the mentioned commit to fix the errors.
>>
>> The reason why we do a partial revert here is that we can still set priv
>> in ->of_xlate() callback for PCI devices. Meanwhile, devices existing in
>> DT, like display, will go through tegra_smmu_configure() at the stage of
>> bus_set_iommu() when SMMU gets probed(), as what it did before we merged
>> the mentioned commit.
>>
>> Once we have the linear map solution for framebuffer memory, this change
>> can be cleaned away.
>>
>> [Big thank to Guillaume who reported and helped debugging/verification]
>>
>> Fixes: 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
>> Reported-by: Guillaume Tucker 
>> Signed-off-by: Nicolin Chen 
>> ---
>>
>> Guillaume, would you please give a "Tested-by" to this change? Thanks!
>>
>>  drivers/iommu/tegra-smmu.c | 72 +-
>>  1 file changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>> index 4a3f095a1c26..97eb62f667d2 100644
>> --- a/drivers/iommu/tegra-smmu.c
>> +++ b/drivers/iommu/tegra-smmu.c
>> @@ -798,10 +798,70 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct 
>> iommu_domain *domain,
>>  return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
>>  }
>>  
>> +static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
>> +{
>> +struct platform_device *pdev;
>> +struct tegra_mc *mc;
>> +
>> +pdev = of_find_device_by_node(np);
>> +if (!pdev)
>> +return NULL;
>> +
>> +mc = platform_get_drvdata(pdev);
>> +if (!mc)
>> +return NULL;
>> +
>> +return mc->smmu;
>> +}
>> +
>> +static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
>> +struct of_phandle_args *args)
>> +{
>> +const struct iommu_ops *ops = smmu->iommu.ops;
>> +int err;
>> +
>> +err = iommu_fwspec_init(dev, >of_node->fwnode, ops);
>> +if (err < 0) {
>> +dev_err(dev, "failed to initialize fwspec: %d\n", err);
>> +return err;
>> +}
>> +
>> +err = ops->of_xlate(dev, args);
>> +if (err < 0) {
>> +dev_err(dev, "failed to parse SW group ID: %d\n", err);
>> +iommu_fwspec_free(dev);
>> +return err;
>> +}
>> +
>> +return 0;
>> +}
>> +
>>  static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
>>  {
>> -struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>> +struct device_node *np = dev->of_node;
>> +struct tegra_smmu *smmu = NULL;
>> +struct of_phandle_args args;
>> +

Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-03-10 Thread Dmitry Osipenko
19.02.2021 01:07, Nicolin Chen пишет:
> Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
> removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
> of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
> tegra_smmu_configure() that are typically done in the IOMMU core also.
> 
> This approach works for both existing devices that have DT nodes and other
> devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
> upon testing. However, Page Fault errors are reported on tegra124-Nyan:
> 
>   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>EMEM address decode error (SMMU translation error [--S])
>   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>Page fault (SMMU translation error [--S])
> 
> After debugging, I found that the mentioned commit changed some function
> callback sequence of tegra-smmu's, resulting in enabling SMMU for display
> client before display driver gets initialized. I couldn't reproduce exact
> same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.
> 
> Actually this Page Fault is a known issue, as on most of Tegra platforms,
> display gets enabled by the bootloader for the splash screen feature, so
> it keeps filling the framebuffer memory. A proper fix to this issue is to
> 1:1 linear map the framebuffer memory to IOVA space so the SMMU will have
> the same address as the physical address in its page table. Yet, Thierry
> has been working on the solution above for a year, and it hasn't merged.
> 
> Therefore, let's partially revert the mentioned commit to fix the errors.
> 
> The reason why we do a partial revert here is that we can still set priv
> in ->of_xlate() callback for PCI devices. Meanwhile, devices existing in
> DT, like display, will go through tegra_smmu_configure() at the stage of
> bus_set_iommu() when SMMU gets probed(), as what it did before we merged
> the mentioned commit.
> 
> Once we have the linear map solution for framebuffer memory, this change
> can be cleaned away.
> 
> [Big thank to Guillaume who reported and helped debugging/verification]
> 
> Fixes: 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
> Reported-by: Guillaume Tucker 
> Signed-off-by: Nicolin Chen 
> ---
> 
> Guillaume, would you please give a "Tested-by" to this change? Thanks!
> 
>  drivers/iommu/tegra-smmu.c | 72 +-
>  1 file changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 4a3f095a1c26..97eb62f667d2 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -798,10 +798,70 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct 
> iommu_domain *domain,
>   return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
>  }
>  
> +static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
> +{
> + struct platform_device *pdev;
> + struct tegra_mc *mc;
> +
> + pdev = of_find_device_by_node(np);
> + if (!pdev)
> + return NULL;
> +
> + mc = platform_get_drvdata(pdev);
> + if (!mc)
> + return NULL;
> +
> + return mc->smmu;
> +}
> +
> +static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
> + struct of_phandle_args *args)
> +{
> + const struct iommu_ops *ops = smmu->iommu.ops;
> + int err;
> +
> + err = iommu_fwspec_init(dev, >of_node->fwnode, ops);
> + if (err < 0) {
> + dev_err(dev, "failed to initialize fwspec: %d\n", err);
> + return err;
> + }
> +
> + err = ops->of_xlate(dev, args);
> + if (err < 0) {
> + dev_err(dev, "failed to parse SW group ID: %d\n", err);
> + iommu_fwspec_free(dev);
> + return err;
> + }
> +
> + return 0;
> +}
> +
>  static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
>  {
> - struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> + struct device_node *np = dev->of_node;
> + struct tegra_smmu *smmu = NULL;
> + struct of_phandle_args args;
> + unsigned int index = 0;
> + int err;
> +
> + while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> +   ) == 0) {
> + smmu = tegra_smmu_find(args.np);
> + if (smmu) {
> + err = tegra_smmu_configure(smmu, dev, );
> + of_node_put(args.np);
>  
> + if (err < 0)
> + return ERR_PTR(err);
> +
> + break;
> + }
> +
> + of_node_put(args.np);
> + index++;
> + }
> +
> + smmu = dev_iommu_priv_get(dev);
>   if (!smmu)
>   return ERR_PTR(-ENODEV);
>  
> @@ -1028,6 +1088,16 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
>   if (!smmu)
>   

Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-03-03 Thread Dmitry Osipenko
03.03.2021 02:08, Nicolin Chen пишет:
> On Sat, Feb 27, 2021 at 12:59:17PM +0300, Dmitry Osipenko wrote:
>> 25.02.2021 09:27, Nicolin Chen пишет:
>> ...
>>>> The partially revert should be okay, but it's not clear to me what makes
>>>> difference for T124 since I don't see that problem on T30, which also
>>>> has active display at a boot time.
>>>
>>> Hmm..do you see ->attach_dev() is called from host1x_client_iommu_attach
>>> or from of_dma_configure_id/arch_setup_dma_ops?
>>>
>>
>> I applied yours debug-patch, please see dmesg.txt attached to the email.
>> Seems probe-defer of the tegra-dc driver prevents the implicit
>> tegra_smmu_attach_dev, so it happens to work by accident.
> 
>> [0.327826] tegra-dc 5420.dc: ---tegra_smmu_of_xlate: id 1
>> [0.328641] [] (tegra_smmu_of_xlate) from [] 
>> (of_iommu_xlate+0x51/0x70)
>> [0.328740] [] (of_iommu_xlate) from [] 
>> (of_iommu_configure+0x127/0x150)
>> [0.328896] [] (of_iommu_configure) from [] 
>> (of_dma_configure_id+0x1fb/0x2ec)
>> [0.329060] [] (of_dma_configure_id) from [] 
>> (really_probe+0x7b/0x2a0)
>> [0.331438] tegra-dc 5420.dc: tegra_smmu_probe_device, 822
>> [0.332234] [] (tegra_smmu_probe_device) from [] 
>> (__iommu_probe_device+0x35/0x1c4)
>> [0.332391] [] (__iommu_probe_device) from [] 
>> (iommu_probe_device+0x19/0xec)
>> [0.332545] [] (iommu_probe_device) from [] 
>> (of_iommu_configure+0xfb/0x150)
>> [0.332701] [] (of_iommu_configure) from [] 
>> (of_dma_configure_id+0x1fb/0x2ec)
>> [0.332804] [] (of_dma_configure_id) from [] 
>> (really_probe+0x7b/0x2a0)
>> [0.335202] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1572
>> [0.335292] tegra-dc 5420.dc: -tegra_smmu_device_group, 862
>> [0.335474] tegra-dc 5420.dc: -tegra_smmu_device_group, 909: 
>> 1: drm
>> [0.335566] tegra-dc 5420.dc: -iommu_group_get_for_dev, 1574
>> [0.335718] tegra-dc 5420.dc: -iommu_group_add_device, 858
>> [0.335862] tegra-dc 5420.dc: Adding to iommu group 1
>> [0.335955] tegra-dc 5420.dc: -iommu_alloc_default_domain, 
>> 1543: type 3
>> [0.336101] iommu: --iommu_group_alloc_default_domain: platform, 
>> (null), drm
>> [0.336187] -tegra_smmu_domain_alloc, 284: type 3
>  [0.336968] [] (tegra_smmu_domain_alloc) from [] 
> (iommu_group_alloc_default_domain+0x4b/0xfa)
>> [0.337127] [] (iommu_group_alloc_default_domain) from 
>> [] (iommu_probe_device+0x69/0xec)
>> [0.337285] [] (iommu_probe_device) from [] 
>> (of_iommu_configure+0xfb/0x150)
>> [0.337441] [] (of_iommu_configure) from [] 
>> (of_dma_configure_id+0x1fb/0x2ec)
>> [0.337599] [] (of_dma_configure_id) from [] 
>> (really_probe+0x7b/0x2a0)
>> [0.339913] tegra-dc 5420.dc: -iommu_probe_device, 272
>> [0.348144] tegra-dc 5420.dc: failed to probe RGB output: -517
> 
> Hmm..not sure where this EPROBE_DEFER comes from.

DC driver on Nexus 7 depends on LVDS bridge and display panel, which
cause the probe defer.

> But you are right,
> as of_dma_configure_id() returns because of that so it didn't run to
> arch_setup_dma_ops() call, which allocates an UNMANAGED iommu domain
> and attaches DC to it on Tegra124.
> 
> By the way, anyone can accept this change? It doesn't feel right to
> leave a regression in the newer release...
> 

I think Thierry should give ack.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-02-27 Thread Dmitry Osipenko
25.02.2021 09:27, Nicolin Chen пишет:
...
>> The partially revert should be okay, but it's not clear to me what makes
>> difference for T124 since I don't see that problem on T30, which also
>> has active display at a boot time.
> 
> Hmm..do you see ->attach_dev() is called from host1x_client_iommu_attach
> or from of_dma_configure_id/arch_setup_dma_ops?
> 

I applied yours debug-patch, please see dmesg.txt attached to the email.
Seems probe-defer of the tegra-dc driver prevents the implicit
tegra_smmu_attach_dev, so it happens to work by accident.
[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 5.11.0-next-20210226-3-g89069e0f4a28 
(dima@dimapc) (armv7a-hardfloat-linux-gnueabi-gcc (Gentoo 9.3.0-r2 p4) 9.3.0, 
GNU ld (Gentoo 2.26.1 p1.0) 2.26.1) #7012 SMP PREEMPT Sat Feb 27 11:49:27 MSK 
2021
[0.00] CPU: ARMv7 Processor [412fc099] revision 9 (ARMv7), cr=50c5387d
[0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing 
instruction cache
[0.00] OF: fdt: Machine model: ASUS Google Nexus 7 (Project Bach / 
ME370TG) E1565
[0.00] Memory policy: Data cache writealloc
[0.00] Reserved memory: created CMA memory pool at 0xa000, size 256 
MiB
[0.00] OF: reserved mem: initialized node linux,cma@8000, 
compatible id shared-dma-pool
[0.00] Zone ranges:
[0.00]   Normal   [mem 0x-0x9f7f]
[0.00]   HighMem  [mem 0x9f80-0xbfdf]
[0.00] Movable zone start for each node
[0.00] Early memory node ranges
[0.00]   node   0: [mem 0x8000-0xbfdf]
[0.00] Initmem setup node 0 [mem 0x8000-0xbfdf]
[0.00] On node 0 totalpages: 261632
[0.00]   Normal zone: 1134 pages used for memmap
[0.00]   Normal zone: 0 pages reserved
[0.00]   Normal zone: 129024 pages, LIFO batch:31
[0.00]   HighMem zone: 132608 pages, LIFO batch:31
[0.00] percpu: Embedded 21 pages/cpu s53324 r8192 d24500 u86016
[0.00] pcpu-alloc: s53324 r8192 d24500 u86016 alloc=21*4096
[0.00] pcpu-alloc: [0] 0 [0] 1 [0] 2 [0] 3
[0.00] Built 1 zonelists, mobility grouping on.  Total pages: 260498
[0.00] Kernel command line: tegraid=30.1.3.0.0 mem=1022M@2048M 
android.commchip=0 vmalloc=512M androidboot.serialno=015d3f18c9081210 
video=tegrafb no_console_suspend=1 console=none debug_uartport=hsport 
usbcore.old_scheme_first=1 lp0_vec=8192@0xbddf9000 
tegra_fbmem=8195200@0xabe01000 core_edp_mv=0 audio_codec=rt5640 
board_info=f41:a00:1:44:2 root=/dev/sda1 rw rootwait tegraboot=sdmmc gpt 
gpt_sector=61079551 androidboot.bootloader=4.23 
androidboot.baseband=1231_0.18.0_0409 init=/usr/lib/systemd/systemd 
root=/dev/nfs nfsroot=192.168.3.100:/nfs/root,v3,tcp rw 
ip=192.168.3.101::192.168.3.100:255.255.255.0::usb0 no_console_suspend gpt 
gpt_sector=61079551 console=tty1 no_console_suspend
[0.00] Dentry cache hash table entries: 65536 (order: 6, 262144 bytes, 
linear)
[0.00] Inode-cache hash table entries: 32768 (order: 5, 131072 bytes, 
linear)
[0.00] mem auto-init: stack:off, heap alloc:off, heap free:off
[0.00] Memory: 754680K/1046528K available (10240K kernel code, 1579K 
rwdata, 5404K rodata, 1024K init, 418K bss, 29704K reserved, 262144K 
cma-reserved, 268224K highmem)
[0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1
[0.00] ftrace: allocating 46557 entries in 91 pages
[0.00] [ cut here ]
[0.00] WARNING: CPU: 0 PID: 0 at arch/arm/kernel/insn.c:15 
__arm_gen_branch+0x7f/0x84
[0.00] Modules linked in:
[0.00] CPU: 0 PID: 0 Comm: swapper Not tainted 
5.11.0-next-20210226-3-g89069e0f4a28 #7012
[0.00] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[0.00] [] (unwind_backtrace) from [] 
(show_stack+0x11/0x14)
[0.00] [] (show_stack) from [] 
(dump_stack+0x8d/0x9a)
[0.00] [] (dump_stack) from [] (__warn+0xb7/0xc8)
[0.00] [] (__warn) from [] 
(warn_slowpath_fmt+0x45/0x78)
[0.00] [] (warn_slowpath_fmt) from [] 
(__arm_gen_branch+0x7f/0x84)
[0.00] [] (__arm_gen_branch) from [] 
(ftrace_make_nop+0xf/0x20)
[0.00] [] (ftrace_make_nop) from [] 
(ftrace_process_locs+0x211/0x320)
[0.00] [] (ftrace_process_locs) from [] 
(ftrace_init+0x6f/0xbe)
[0.00] [] (ftrace_init) from [] 
(start_kernel+0x42d/0x662)
[0.00] [] (start_kernel) from [<>] (0x0)
[0.00] random: get_random_bytes called from 
print_oops_end_marker+0x25/0x64 with crng_init=0
[0.00] ---[ end trace  ]---
[0.00] [ ftrace bug ]
[0.00] ftrace failed to modify
[0.00] [] crash_notes_memory_init+0x4/0x34
[0.00]  actual:   23:f0:9c:ec
[0.00] Initializing ftrace call sites
[0.00] ftrace record flags: 0

Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-02-22 Thread Dmitry Osipenko
23.02.2021 05:13, Nicolin Chen пишет:
> Hi Dmitry,
> 
> On Sat, Feb 20, 2021 at 08:16:22AM +0300, Dmitry Osipenko wrote:
>> 19.02.2021 01:07, Nicolin Chen пишет:
>>> Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
>>> removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
>>> of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
>>> tegra_smmu_configure() that are typically done in the IOMMU core also.
>>>
>>> This approach works for both existing devices that have DT nodes and other
>>> devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
>>> upon testing. However, Page Fault errors are reported on tegra124-Nyan:
>>>
>>>   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>>>  EMEM address decode error (SMMU translation error [--S])
>>>   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>>>  Page fault (SMMU translation error [--S])
>>>
>>> After debugging, I found that the mentioned commit changed some function
>>> callback sequence of tegra-smmu's, resulting in enabling SMMU for display
>>> client before display driver gets initialized. I couldn't reproduce exact
>>> same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.
>>
>> Hello Nicolin,
>>
>> Could you please explain in a more details what exactly makes the
>> difference for the callback sequence?
> 
> Here is a log with 5.11.0-rc6:
> https://lava.collabora.co.uk/scheduler/job/3187849
> [dump_stack was added in some tegra-smmu functions]
> 
> And here is a corresponding log with reverting the original commit:
> https://lava.collabora.co.uk/scheduler/job/3187851
> 
> Here is a log with 5.11.0-rc7-next-20210210:
> https://lava.collabora.co.uk/scheduler/job/3210245
> 
> And here is a corresponding log with reverting the original commit:
> https://lava.collabora.co.uk/scheduler/job/3210596
> 
> Both failing logs show that mc errors started right after client DC
> got enabled by ->attach_dev() callback that in the passing logs was
> not called until Host1x driver init. And note that two failing logs
> show that ->attach_dev() could be called from two different sources,
> of_dma_configure_id() or arch_setup_dma_ops().
> 
> The reason why ->attach_dev() gets called is probably related to the
> following reasons (sorry, can't be 100% sure as I don't have Tegra124
> or other 32bit Tegra board to test):
> 1) With the commit reverted, all clients are probed in "arch" stage,
>which is even prior to iommu core initialization -- including it
>setting default domain type. This probably messed up the type of
>allocating domains against the default domain type. Also internal
>group is somehow affected. So some condition check in iommu core
>failed and then it bypassed ->attach_dev callback in really_probe
>stage, until Host1x driver does attach_dev again.
> 
> 2) 32bit ARM has arch_setup_dma_ops() does an additional set of iommu
>domain allocation + attach_dev(), after of_dma_configure_id() did
>once. This isn't reproducible for me on Tegra210.
> 
> As debugging online isn't very efficient, and given that Thierry has
> been working on the linear mapping of framebuffer carveout, I choose
> to partially revert as a quick fix.

The partially revert should be okay, but it's not clear to me what makes
difference for T124 since I don't see that problem on T30, which also
has active display at a boot time.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-02-19 Thread Dmitry Osipenko
19.02.2021 01:07, Nicolin Chen пишет:
> Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
> removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
> of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
> tegra_smmu_configure() that are typically done in the IOMMU core also.
> 
> This approach works for both existing devices that have DT nodes and other
> devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
> upon testing. However, Page Fault errors are reported on tegra124-Nyan:
> 
>   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>EMEM address decode error (SMMU translation error [--S])
>   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
>Page fault (SMMU translation error [--S])
> 
> After debugging, I found that the mentioned commit changed some function
> callback sequence of tegra-smmu's, resulting in enabling SMMU for display
> client before display driver gets initialized. I couldn't reproduce exact
> same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.

Hello Nicolin,

Could you please explain in a more details what exactly makes the
difference for the callback sequence?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property

2021-01-12 Thread Dmitry Osipenko
05.11.2020 18:49, Thierry Reding пишет:
> On Thu, Sep 24, 2020 at 07:23:34PM +0300, Dmitry Osipenko wrote:
>> 24.09.2020 17:01, Thierry Reding пишет:
>>> On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
>>>> 04.09.2020 15:59, Thierry Reding пишет:
>>>>> From: Thierry Reding 
>>>>>
>>>>> Reserved memory regions can be marked as "active" if hardware is
>>>>> expected to access the regions during boot and before the operating
>>>>> system can take control. One example where this is useful is for the
>>>>> operating system to infer whether the region needs to be identity-
>>>>> mapped through an IOMMU.
>>>>>
>>>>> Signed-off-by: Thierry Reding 
>>>>> ---
>>>>>  .../bindings/reserved-memory/reserved-memory.txt   | 7 +++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git 
>>>>> a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt 
>>>>> b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>>> index 4dd20de6977f..163d2927e4fc 100644
>>>>> --- 
>>>>> a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>>> +++ 
>>>>> b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>>> @@ -63,6 +63,13 @@ reusable (optional) - empty property
>>>>>able to reclaim it back. Typically that means that the operating
>>>>>system can use that region to store volatile or cached data that
>>>>>can be otherwise regenerated or migrated elsewhere.
>>>>> +active (optional) - empty property
>>>>> +- If this property is set for a reserved memory region, it indicates
>>>>> +  that some piece of hardware may be actively accessing this region.
>>>>> +  Should the operating system want to enable IOMMU protection for a
>>>>> +  device, all active memory regions must have been identity-mapped
>>>>> +  in order to ensure that non-quiescent hardware during boot can
>>>>> +  continue to access the memory.
>>>>>  
>>>>>  Linux implementation note:
>>>>>  - If a "linux,cma-default" property is present, then Linux will use the
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> Could you please explain what devices need this quirk? I see that you're
>>>> targeting Tegra SMMU driver, which means that it should be some pre-T186
>>>> device.
>>>
>>> Primarily I'm looking at Tegra210 and later, because on earlier devices
>>> the bootloader doesn't consistently initialize display. I know that it
>>> does on some devices, but not all of them.
>>
>> AFAIK, all tablet devices starting with Tegra20 that have display panel
>> are initializing display at a boot time for showing splash screen. This
>> includes all T20/T30/T114 tablets that are already supported by upstream
>> kernel.
>>
>>> This same code should also
>>> work on Tegra186 and later (with an ARM SMMU) although the situation is
>>> slightly more complicated there because IOMMU translations will fault by
>>> default long before these identity mappings can be established.
>>>
>>>> Is this reservation needed for some device that has display
>>>> hardwired to a very specific IOMMU domain at the boot time?
>>>
>>> No, this is only used to convey information about the active framebuffer
>>> to the kernel. In practice the DMA/IOMMU code will use this information
>>> to establish a 1:1 mapping on whatever IOMMU domain that was picked for
>>> display.
>>>
>>>> If you're targeting devices that don't have IOMMU enabled by default at
>>>> the boot time, then this approach won't work for the existing devices
>>>> which won't ever get an updated bootloader.
>>>
>>> If the devices don't use an IOMMU, then there should be no problem. The
>>> extra reserved-memory nodes would still be necessary to ensure that the
>>> kernel doesn't reuse the framebuffer memory for the slab allocator, but
>>> if no IOMMU is used, then the display controller accessing the memory
>>> isn't going to cause problems other than perhaps scanning out data that
>>> is no longer a framebuffer.
>>>
>>> There should also be no problem for dev

Re: [PATCH RESEND 0/5] iommu/tegra-smmu: Some pending reviewed changes

2020-11-24 Thread Dmitry Osipenko
25.11.2020 00:21, Nicolin Chen пишет:
> Hi Joerg,
> 
> These five patches were acked by Thierry and acked-n-tested by
> Dmitry a while ago. Would it be possible for you to apply them?
> 
> Thanks!

Hi,

You probably should try to ping Will Deacon.

https://lkml.org/lkml/2020/11/17/243
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-05 Thread Dmitry Osipenko
05.10.2020 14:15, Thierry Reding пишет:
> On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote:
>> 05.10.2020 12:53, Thierry Reding пишет:
>>> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
>>>> 02.10.2020 17:22, Dmitry Osipenko пишет:
>>>>>>  static int tegra_smmu_of_xlate(struct device *dev,
>>>>>> struct of_phandle_args *args)
>>>>>>  {
>>>>>> +struct platform_device *iommu_pdev = 
>>>>>> of_find_device_by_node(args->np);
>>>>>> +struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>>>>>>  u32 id = args->args[0];
>>>>>>  
>>>>>> +of_node_put(args->np);
>>>>>> +
>>>>>> +if (!mc || !mc->smmu)
>>>>>> +return -EPROBE_DEFER;
>>>>> platform_get_drvdata(NULL) will crash.
>>>>>
>>>>
>>>> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
>>>
>>> How so? It's technically possible for the iommus property to reference a
>>> device tree node for which no platform device will ever be created, in
>>> which case of_find_device_by_node() will return NULL. That's very
>>> unlikely and perhaps worth just crashing on to make sure it gets fixed
>>> immediately.
>>
>> The tegra_smmu_ops are registered from the SMMU driver itself and MC
>> driver sets platform data before SMMU is initialized, hence device is
>> guaranteed to exist and mc can't be NULL.
> 
> Yes, but that assumes that args->np points to the memory controller's
> device tree node. It's obviously a mistake to do this, but I don't think
> anyone will prevent you from doing this:
> 
>   iommus = <&{/chosen} 0>;
> 
> In that case, since no platform device is created for the /chosen node,
> iommu_pdev will end up being NULL and platform_get_drvdata() will crash.

But then Tegra SMMU isn't associated with the device's IOMMU path, and
thus, tegra_smmu_of_xlate() won't be invoked for this device.

> That said, I'm fine with not adding a check for that. If anyone really
> does end up messing this up they deserve the crash.
> 
> I'm still a bit undecided about the mc->smmu check because I haven't
> convinced myself yet that it can't happen.

For now I can't see any realistic situation where mc->smmu could be NULL.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-05 Thread Dmitry Osipenko
05.10.2020 12:53, Thierry Reding пишет:
> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 17:22, Dmitry Osipenko пишет:
>>>>  static int tegra_smmu_of_xlate(struct device *dev,
>>>>   struct of_phandle_args *args)
>>>>  {
>>>> +  struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
>>>> +  struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>>>>u32 id = args->args[0];
>>>>  
>>>> +  of_node_put(args->np);
>>>> +
>>>> +  if (!mc || !mc->smmu)
>>>> +  return -EPROBE_DEFER;
>>> platform_get_drvdata(NULL) will crash.
>>>
>>
>> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
> 
> How so? It's technically possible for the iommus property to reference a
> device tree node for which no platform device will ever be created, in
> which case of_find_device_by_node() will return NULL. That's very
> unlikely and perhaps worth just crashing on to make sure it gets fixed
> immediately.

The tegra_smmu_ops are registered from the SMMU driver itself and MC
driver sets platform data before SMMU is initialized, hence device is
guaranteed to exist and mc can't be NULL.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-05 Thread Dmitry Osipenko
05.10.2020 12:16, Thierry Reding пишет:
...
>> I think you meant regmap in regards to protecting IO accesses, but this
>> is not what regmap is about if IO accesses are atomic by nature.
> 
> Then why is there regmap-mmio?

To protect programming sequences for example, actually that's the only
real use-case I saw in kernel drivers once. In our case there are no
sequences that require protection, at least I'm not aware about that.

>> Secondly, you're missing that tegra30-devfreq driver will also need to
>> perform the MC lookup [3] and then driver will no longer work for the
>> older DTs if phandle becomes mandatory because these DTs do not have the
>> MC phandle in the ACTMON node.
>>
>> [3]
>> https://github.com/grate-driver/linux/commit/441d19281f9b3428a4db1ecb3a02e1ec02a8ad7f
>>
>> So we will need the fall back for T30/124 as well.
> 
> No, we don't need the fallback because this is new functionality which
> can and should be gated on the existence of the new phandle. If there's
> no phandle then we have no choice but to use the old code to ensure old
> behaviour.

You just repeated what I was trying to say:)

Perhaps I spent a bit too much time touching that code to the point that
lost feeling that there is a need to clarify everything in details.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-05 Thread Dmitry Osipenko
05.10.2020 00:57, Nicolin Chen пишет:
> On Sat, Oct 03, 2020 at 05:06:42PM +0300, Dmitry Osipenko wrote:
>> 03.10.2020 09:59, Nicolin Chen пишет:
>>>  static int tegra_smmu_of_xlate(struct device *dev,
>>>struct of_phandle_args *args)
>>>  {
>>> +   struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
>>> +   struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>>> u32 id = args->args[0];
>>>  
>>> +   put_device(_pdev->dev);
>>> +
>>> +   if (!mc || !mc->smmu)
>>> +   return -EPROBE_DEFER;
>>
>> I'm not very excited by seeing code in the patches that can't be
>> explained by the patch authors and will appreciate if you could provide
>> a detailed explanation about why this NULL checking is needed because I
>> think it is unneeded, especially given that other IOMMU drivers don't
>> have such check.
> 
> This function could be called from of_iommu_configure(), which is
> a part of other driver's probe(). So I think it's safer to have a
> check. Yet, given mc driver is added to the "arch_initcall" stage,
> you are probably right that there's no really need at this moment
> because all clients should be called after mc/smmu are inited. So
> I'll resend a v6, if that makes you happy.

I wanted to get the explanation :) I'm very curious why it's actually
needed because I'm not 100% sure whether it's not needed at all.

I'd assume that the only possible problem could be if some device is
created in parallel with the MC probing and there is no locking that
could prevent this in the drivers core. It's not apparent to me whether
this situation could happen at all in practice.

The MC is created early and at that time everything is sequential, so
it's indeed should be safe to remove the check.

>> I'm asking this question second time now, please don't ignore review
>> comments next time.
> 
> I think I missed your reply or misunderstood it.
> 

Okay!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-05 Thread Dmitry Osipenko
05.10.2020 10:13, Thierry Reding пишет:
...
> Have you also seen that sun50i-iommu does look up the SMMU from a
> phandle using of_find_device_by_node()? So I think you've shown yourself
> that even "modern" drivers avoid global pointers and look up via
> phandle.

I have no problem with the lookup by phandle and I'm all for it. It's
now apparent to me that you completely missed my point, but that should
be my fault that I haven't conveyed it properly from the start. I just
wanted to avoid the incompatible DT changes which could break older DTs
+ I simply wanted to improve the older code without introducing new
features, that's it.

Anyways, after yours comments I started to look at how the interconnect
patches could be improved and found new things, like that OPPs now
support ICC and that EMC has a working EMC_STAT, I also discovered
syscon and simple-mfd. This means that we won't need the global pointers
at all neither for SMMU, nor for interconnect, nor for EMC drivers :)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/2] iommu/tegra-smmu: Unwrap tegra_smmu_group_get

2020-10-03 Thread Dmitry Osipenko
29.09.2020 20:41, Dmitry Osipenko пишет:
> 29.09.2020 09:13, Nicolin Chen пишет:
>> The tegra_smmu_group_get was added to group devices in different
>> SWGROUPs and it'd return a NULL group pointer upon a mismatch at
>> tegra_smmu_find_group(), so for most of clients/devices, it very
>> likely would mismatch and need a fallback generic_device_group().
>>
>> But now tegra_smmu_group_get handles devices in same SWGROUP too,
>> which means that it would allocate a group for every new SWGROUP
>> or would directly return an existing one upon matching a SWGROUP,
>> i.e. any device will go through this function.
>>
>> So possibility of having a NULL group pointer in device_group()
>> is upon failure of either devm_kzalloc() or iommu_group_alloc().
>> In either case, calling generic_device_group() no longer makes a
>> sense. Especially for devm_kzalloc() failing case, it'd cause a
>> problem if it fails at devm_kzalloc() yet succeeds at a fallback
>> generic_device_group(), because it does not create a group->list
>> for other devices to match.
>>
>> This patch simply unwraps the function to clean it up.
>>
>> Signed-off-by: Nicolin Chen 
>> ---
> 
> Reviewed-by: Dmitry Osipenko 
> 

Tested-by: Dmitry Osipenko 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/2] iommu/tegra-smmu: Expand mutex protection range

2020-10-03 Thread Dmitry Osipenko
29.09.2020 20:42, Dmitry Osipenko пишет:
> 29.09.2020 09:13, Nicolin Chen пишет:
>> This is used to protect potential race condition at use_count.
>> since probes of client drivers, calling attach_dev(), may run
>> concurrently.
>>
>> Signed-off-by: Nicolin Chen 
>> ---
> 
> It's always better not to mix success and error code paths in order to
> keep code readable, but not a big deal in the case of this particular
> patch since the changed code is quite simple. Please try to avoid doing
> this in the future patches.
> 
> Also, please note that in general it's better to use locked/unlocked
> versions for the functions like it's already done for
> tegra_smmu_map/unmap, this will remove the need to maintain lockings in
> the code. The code touched by this patch doesn't have complicated code
> paths, so it's good enough to me.
> 
> Reviewed-by: Dmitry Osipenko 
> 

Tested-by: Dmitry Osipenko 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-03 Thread Dmitry Osipenko
03.10.2020 09:59, Nicolin Chen пишет:
> The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
> to call in tegra_smmu_probe_device() where each client searches
> its DT node for smmu pointer and swgroup ID, so as to configure
> an fwspec. But this requires a valid smmu pointer even before mc
> and smmu drivers are probed. So in tegra_smmu_probe() we added a
> line of code to fill mc->smmu, marking "a bit of a hack".
> 
> This works for most of clients in the DTB, however, doesn't work
> for a client that doesn't exist in DTB, a PCI device for example.
> 
> Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
> it's called from bus_set_iommu(), iommu core will let everything
> carry on. Then when a client gets probed, of_iommu_configure() in
> iommu core will search DTB for swgroup ID and call ->of_xlate()
> to prepare an fwspec, similar to tegra_smmu_probe_device() and
> tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
> again, and this time we shall return smmu->iommu pointer properly.
> 
> So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
> along with DT polling code by letting the iommu core handle every
> thing, except a problem that we search iommus property in DTB not
> only for swgroup ID but also for mc node to get mc->smmu pointer
> to call dev_iommu_priv_set() and return the smmu->iommu pointer.
> So we'll need to find another way to get smmu pointer.
> 
> Referencing the implementation of sun50i-iommu driver, of_xlate()
> has client's dev pointer, mc node and swgroup ID. This means that
> we can call dev_iommu_priv_set() in of_xlate() instead, so we can
> simply get smmu pointer in ->probe_device().
> 
> This patch reworks tegra_smmu_probe_device() by:
> 1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
>ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
>tegra_smmu_probe/tegra_mc_probe().
> 2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
>pointer in tegra_smmu_probe_device() to replace DTB polling.
> 3) Removing tegra_smmu_configure() accordingly since iommu core
>takes care of it.
> 
> This also fixes a problem that previously we added all clients to
> iommu groups before iommu core initializes its default domain:
> ubuntu@jetson:~$ dmesg | grep iommu
> platform smmu_benchmark: Adding to iommu group 0
> platform 1003000.pcie: Adding to iommu group 1
> platform 5000.host1x: Adding to iommu group 2
> platform 5700.gpu: Adding to iommu group 3
> platform 7000c400.i2c: Adding to iommu group 4
> platform 7000c500.i2c: Adding to iommu group 4
> platform 7000c700.i2c: Adding to iommu group 4
> platform 7000d000.i2c: Adding to iommu group 4
> iommu: Default domain type: Translated
> 
> Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have
> warnings if switching to IOMMU_DOMAIN_DMA:
> iommu: Failed to allocate default IOMMU domain of type 0 for
>group (null) - Falling back to IOMMU_DOMAIN_DMA
> iommu: Failed to allocate default IOMMU domain of type 0 for
>group (null) - Falling back to IOMMU_DOMAIN_DMA
> 
> Now, bypassing the first probe_device() call from bus_set_iommu()
> fixes the sequence:
> ubuntu@jetson:~$ dmesg | grep iommu
> iommu: Default domain type: Translated 
> tegra-i2c 7000c400.i2c: Adding to iommu group 0
> tegra-i2c 7000c500.i2c: Adding to iommu group 0
> tegra-i2c 7000d000.i2c: Adding to iommu group 0
> tegra-pcie 1003000.pcie: Adding to iommu group 1
>     ...
> 
> Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED.
> 
> Signed-off-by: Nicolin Chen 
> ---

Everything looks good to me, apart from the very minor pending question
about the NULL-checking. Thanks!

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 3/3] iommu/tegra-smmu: Add PCI support

2020-10-03 Thread Dmitry Osipenko
03.10.2020 09:59, Nicolin Chen пишет:
> This patch simply adds support for PCI devices.
> 
> Signed-off-by: Nicolin Chen 
> Reviewed-by: Dmitry Osipenko 

Small nit: yours s-b tag always should be the last line of the commit
message because you're "signing up" words that were written by you.

Tested-by: Dmitry Osipenko 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-03 Thread Dmitry Osipenko
03.10.2020 09:59, Nicolin Chen пишет:
> In tegra_smmu_(de)attach_dev() functions, we poll DTB for each
> client's iommus property to get swgroup ID in order to prepare
> "as" and enable smmu. Actually tegra_smmu_configure() prepared
> an fwspec for each client, and added to the fwspec all swgroup
> IDs of client DT node in DTB.
> 
> So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as
> to replace the redundant DT polling code.
> 
> Signed-off-by: Nicolin Chen 
> ---

I'm still not highly impressed by seeing the !fwspec check in this
patch. But I'm not a maintainer of the SMMU driver, hence will leave it
up to Thierry and Joerg to decide whether this is good or needs to be
improved.

Otherwise this patch is good to me, thanks. I tested it on Nexus 7,
which is Tegra30.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-03 Thread Dmitry Osipenko
03.10.2020 09:59, Nicolin Chen пишет:
> ubuntu@jetson:~$ dmesg | grep iommu
> iommu: Default domain type: Translated 
> tegra-i2c 7000c400.i2c: Adding to iommu group 0
> tegra-i2c 7000c500.i2c: Adding to iommu group 0
> tegra-i2c 7000d000.i2c: Adding to iommu group 0
> tegra-pcie 1003000.pcie: Adding to iommu group 1

Could you please explain how you got I2C into IOMMU?

Are you testing vanilla upstream kerne? Upstream DT doesn't assign AHB
group to I2C controllers, nor to APB DMA controller.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-03 Thread Dmitry Osipenko
03.10.2020 09:59, Nicolin Chen пишет:
>  static int tegra_smmu_of_xlate(struct device *dev,
>  struct of_phandle_args *args)
>  {
> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>   u32 id = args->args[0];
>  
> + put_device(_pdev->dev);
> +
> + if (!mc || !mc->smmu)
> + return -EPROBE_DEFER;

I'm not very excited by seeing code in the patches that can't be
explained by the patch authors and will appreciate if you could provide
a detailed explanation about why this NULL checking is needed because I
think it is unneeded, especially given that other IOMMU drivers don't
have such check.

I'm asking this question second time now, please don't ignore review
comments next time.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-02 Thread Dmitry Osipenko
03.10.2020 02:53, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 11:12:18PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 22:45, Nicolin Chen пишет:
>>> On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
>>>> 02.10.2020 09:08, Nicolin Chen пишет:
>>>>>  static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>>>>>struct device *dev)
>>>>>  {
>>>>> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>>>>   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>>>>>   struct tegra_smmu_as *as = to_smmu_as(domain);
>>>>> - struct device_node *np = dev->of_node;
>>>>> - struct of_phandle_args args;
>>>>>   unsigned int index = 0;
>>>>>   int err = 0;
>>>>>  
>>>>> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
>>>>> -)) {
>>>>> - unsigned int swgroup = args.args[0];
>>>>> -
>>>>> - if (args.np != smmu->dev->of_node) {
>>>>> - of_node_put(args.np);
>>>>> - continue;
>>>>> - }
>>>>> -
>>>>> - of_node_put(args.np);
>>>>> + if (!fwspec)
>>>>> + return -ENOENT;
>>>>
>>>> Could the !fwspec ever be true here as well?
>>>
>>> There are multiple callers of this function. It's really not that
>>> straightforward to track every one of them. So I'd rather have it
>>> here as other iommu drivers do. We are human beings, so we could
>>> have missed something somewhere, especially callers are not from
>>> tegra-* drivers.
>>>
>>
>> I'm looking at the IOMMU core and it requires device to be in IOMMU
>> group before attach_dev() could be called.
>>
>> The group can't be assigned to device without the fwspec, see
>> tegra_smmu_device_group().
>>
>> Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
>> NULL in attach_dev(), some not checking anything, some check both and
>> only arm-smmu checks the fwspec.
> 
> As I said a couple of days ago, I don't like to assume that the
> callers won't change. And this time, it's from open code. So I
> don't want to assume that there won't be a change.
> 
> If you are confident that there is no need to add such a check,
> please send patches to remove those checks in those drivers to
> see if others would agree. I would be willing to remove it after
> that. Otherwise, I'd like to keep this.
> 
> Thanks for the review.
> 

I haven't tried to check every code path very thoroughly, expecting you
to do it since you're making this patch. Maybe there is a real reason
why majority of drivers do the checks and it would be good to know why.
Although, it's not critical in this particular case and indeed the
checks could be improved later on.

It looks to me that at least will be a bit better/cleaner to check the
dev_iommu_priv_get() for NULL instead of fwspec because the private
variable depends on the fwspec presence and there is a similar check in
probe_device, hence checks will be more consistent.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-02 Thread Dmitry Osipenko
02.10.2020 22:45, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>>  static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>>>  struct device *dev)
>>>  {
>>> +   struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>> struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>>> struct tegra_smmu_as *as = to_smmu_as(domain);
>>> -   struct device_node *np = dev->of_node;
>>> -   struct of_phandle_args args;
>>> unsigned int index = 0;
>>> int err = 0;
>>>  
>>> -   while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
>>> -  )) {
>>> -   unsigned int swgroup = args.args[0];
>>> -
>>> -   if (args.np != smmu->dev->of_node) {
>>> -   of_node_put(args.np);
>>> -   continue;
>>> -   }
>>> -
>>> -   of_node_put(args.np);
>>> +   if (!fwspec)
>>> +   return -ENOENT;
>>
>> Could the !fwspec ever be true here as well?
> 
> There are multiple callers of this function. It's really not that
> straightforward to track every one of them. So I'd rather have it
> here as other iommu drivers do. We are human beings, so we could
> have missed something somewhere, especially callers are not from
> tegra-* drivers.
> 

I'm looking at the IOMMU core and it requires device to be in IOMMU
group before attach_dev() could be called.

The group can't be assigned to device without the fwspec, see
tegra_smmu_device_group().

Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
NULL in attach_dev(), some not checking anything, some check both and
only arm-smmu checks the fwspec.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Dmitry Osipenko
02.10.2020 21:01, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 05:23:14PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>>  static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
>>>  {
>>> -   struct device_node *np = dev->of_node;
>>> -   struct tegra_smmu *smmu = NULL;
>>> -   struct of_phandle_args args;
>>> -   unsigned int index = 0;
>>> -   int err;
>>> -
>>> -   while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
>>> - ) == 0) {
>>> -   smmu = tegra_smmu_find(args.np);
>>> -   if (smmu) {
>>> -   err = tegra_smmu_configure(smmu, dev, );
>>> -   of_node_put(args.np);
>>> -
>>> -   if (err < 0)
>>> -   return ERR_PTR(err);
>>> -
>>> -   /*
>>> -* Only a single IOMMU master interface is currently
>>> -* supported by the Linux kernel, so abort after the
>>> -* first match.
>>> -*/
>>> -   dev_iommu_priv_set(dev, smmu);
>>> -
>>> -   break;
>>> -   }
>>> -
>>> -   of_node_put(args.np);
>>> -   index++;
>>> -   }
>>> +   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>>>  
>>> if (!smmu)
>>> return ERR_PTR(-ENODEV);
>>
>> The !smmu can't ever be true now, isn't it? Then please remove it.
> 
> How can you be so sure? Have you read my commit message? The whole
> point of removing the hack in tegra_smmu_probe() is to return the
> ERR_PTR(-ENODEV) here. The bus_set_iommu() will call this function
> when mc->smmu is not assigned it, as it's assigned after we return
> tegra_smmu_probe() while bus_set_iommu() is still in the middle of
> the tegra_smmu_probe().
> 

My bad, I probably missed that was looking at the probe_device(), looks
good then.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/3] iommu/tegra-smmu: Add PCI support

2020-10-02 Thread Dmitry Osipenko
02.10.2020 09:08, Nicolin Chen пишет:
> This patch simply adds support for PCI devices.
> 
> Signed-off-by: Nicolin Chen 
> ---

Reviewed-by: Dmitry Osipenko 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/3] iommu/tegra-smmu: Add PCI support

2020-10-02 Thread Dmitry Osipenko
02.10.2020 20:45, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 05:35:24PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>> @@ -865,7 +866,11 @@ static struct iommu_group 
>>> *tegra_smmu_device_group(struct device *dev)
>>> group->smmu = smmu;
>>> group->soc = soc;
>>>  
>>> -   group->group = iommu_group_alloc();
>>> +   if (dev_is_pci(dev))
>>> +   group->group = pci_device_group(dev);
>>> +   else
>>> +   group->group = generic_device_group(dev);
>>> +
>>> if (IS_ERR(group->group)) {
>>> devm_kfree(smmu->dev, group);
>>> mutex_unlock(>lock);
>>> @@ -1069,22 +1074,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device 
>>> *dev,
>>> iommu_device_set_fwnode(>iommu, dev->fwnode);
>>>  
>>> err = iommu_device_register(>iommu);
>>> -   if (err) {
>>> -   iommu_device_sysfs_remove(>iommu);
>>> -   return ERR_PTR(err);
>>> -   }
>>> +   if (err)
>>> +   goto err_sysfs;
>>>  
>>> err = bus_set_iommu(_bus_type, _smmu_ops);
>>> -   if (err < 0) {
>>> -   iommu_device_unregister(>iommu);
>>> -   iommu_device_sysfs_remove(>iommu);
>>> -   return ERR_PTR(err);
>>> -   }
>>> +   if (err < 0)
>>> +   goto err_unregister;
>>> +
>>> +#ifdef CONFIG_PCI
>>> +   err = bus_set_iommu(_bus_type, _smmu_ops);
>>> +   if (err < 0)
>>> +   goto err_bus_set;
>>> +#endif
>>>  
>>> if (IS_ENABLED(CONFIG_DEBUG_FS))
>>> tegra_smmu_debugfs_init(smmu);
>>>  
>>> return smmu;
>>> +
>>> +err_bus_set: __maybe_unused;
>>
>> __maybe_unused?
> 
> In order to mute a build warning when CONFIG_PCI=n...
> 

okay
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Dmitry Osipenko
02.10.2020 19:37, Dmitry Osipenko пишет:
> 02.10.2020 19:00, Dmitry Osipenko пишет:
>> 02.10.2020 18:23, Dmitry Osipenko пишет:
>>> 02.10.2020 09:08, Nicolin Chen пишет:
>>>> Then when a client gets probed, of_iommu_configure() in
>>>> iommu core will search DTB for swgroup ID and call ->of_xlate()
>>>> to prepare an fwspec, similar to tegra_smmu_probe_device() and
>>>> tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
>>>> again, and this time we shall return smmu->iommu pointer properly.
>>>
>>> I don't quite see where IOMMU core calls of_xlate().
>>>
>>> Have tried to at least boot-test this patch?
>>>
>>
>> I don't see how it ever could work because of_xlate() is only invoked from:
>>
>> fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure()
>>
>> Looks like the tegra_smmu_configure() is still needed.
>>
>> I don't know how sun50i driver could work to be honest. Seems IOMMU is
>> broken on sun50i, but maybe I'm missing something.
>>
>> I added Maxime Ripard to this thread, who is the author of the
>> sun50i-iommu driver.
>>
> 
> Actually, I now see that the other IOMMU drivers (qcom, exynos, etc) do
> the same. So obviously I'm missing something and it should work..
> 

Okay, somehow I was oblivious to that of_dma_configure() invokes
of_dma_configure_id(). Should be good :)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Dmitry Osipenko
02.10.2020 19:00, Dmitry Osipenko пишет:
> 02.10.2020 18:23, Dmitry Osipenko пишет:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>> Then when a client gets probed, of_iommu_configure() in
>>> iommu core will search DTB for swgroup ID and call ->of_xlate()
>>> to prepare an fwspec, similar to tegra_smmu_probe_device() and
>>> tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
>>> again, and this time we shall return smmu->iommu pointer properly.
>>
>> I don't quite see where IOMMU core calls of_xlate().
>>
>> Have tried to at least boot-test this patch?
>>
> 
> I don't see how it ever could work because of_xlate() is only invoked from:
> 
> fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure()
> 
> Looks like the tegra_smmu_configure() is still needed.
> 
> I don't know how sun50i driver could work to be honest. Seems IOMMU is
> broken on sun50i, but maybe I'm missing something.
> 
> I added Maxime Ripard to this thread, who is the author of the
> sun50i-iommu driver.
> 

Actually, I now see that the other IOMMU drivers (qcom, exynos, etc) do
the same. So obviously I'm missing something and it should work..
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Dmitry Osipenko
02.10.2020 18:23, Dmitry Osipenko пишет:
> 02.10.2020 09:08, Nicolin Chen пишет:
>> Then when a client gets probed, of_iommu_configure() in
>> iommu core will search DTB for swgroup ID and call ->of_xlate()
>> to prepare an fwspec, similar to tegra_smmu_probe_device() and
>> tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
>> again, and this time we shall return smmu->iommu pointer properly.
> 
> I don't quite see where IOMMU core calls of_xlate().
> 
> Have tried to at least boot-test this patch?
> 

I don't see how it ever could work because of_xlate() is only invoked from:

fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure()

Looks like the tegra_smmu_configure() is still needed.

I don't know how sun50i driver could work to be honest. Seems IOMMU is
broken on sun50i, but maybe I'm missing something.

I added Maxime Ripard to this thread, who is the author of the
sun50i-iommu driver.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Dmitry Osipenko
02.10.2020 09:08, Nicolin Chen пишет:
> Then when a client gets probed, of_iommu_configure() in
> iommu core will search DTB for swgroup ID and call ->of_xlate()
> to prepare an fwspec, similar to tegra_smmu_probe_device() and
> tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
> again, and this time we shall return smmu->iommu pointer properly.

I don't quite see where IOMMU core calls of_xlate().

Have tried to at least boot-test this patch?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Dmitry Osipenko
02.10.2020 09:08, Nicolin Chen пишет:
>  static int tegra_smmu_of_xlate(struct device *dev,
>  struct of_phandle_args *args)
>  {
> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>   u32 id = args->args[0];
>  
> + of_node_put(args->np);

of_find_device_by_node() takes device reference and not the np
reference. This is a bug, please remove of_node_put().
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Dmitry Osipenko
02.10.2020 17:22, Dmitry Osipenko пишет:
> 02.10.2020 09:08, Nicolin Chen пишет:
>> -static void tegra_smmu_release_device(struct device *dev)
>> -{
>> -dev_iommu_priv_set(dev, NULL);
>> -}
>> +static void tegra_smmu_release_device(struct device *dev) {}
> 
> Please keep the braces as-is.
> 

I noticed that you borrowed this style from the sun50i-iommu driver, but
this is a bit unusual coding style for the c files. At least to me it's
unusual to see header-style function stub in a middle of c file. But
maybe it's just me.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-02 Thread Dmitry Osipenko
02.10.2020 17:22, Dmitry Osipenko пишет:
> 02.10.2020 09:08, Nicolin Chen пишет:
>>  static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>>   struct device *dev)
>>  {
>> +struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>  struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>>  struct tegra_smmu_as *as = to_smmu_as(domain);
>> -struct device_node *np = dev->of_node;
>> -struct of_phandle_args args;
>>  unsigned int index = 0;
>>  int err = 0;
> 
> Looks like there is no need to initialize 'index' and 'err' variables
> anymore.
> 

Same for tegra_smmu_detach_dev().
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Dmitry Osipenko
02.10.2020 17:22, Dmitry Osipenko пишет:
>>  static int tegra_smmu_of_xlate(struct device *dev,
>> struct of_phandle_args *args)
>>  {
>> +struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
>> +struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>>  u32 id = args->args[0];
>>  
>> +of_node_put(args->np);
>> +
>> +if (!mc || !mc->smmu)
>> +return -EPROBE_DEFER;
> platform_get_drvdata(NULL) will crash.
> 

Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-02 Thread Dmitry Osipenko
02.10.2020 09:08, Nicolin Chen пишет:
>  static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>struct device *dev)
>  {
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>   struct tegra_smmu_as *as = to_smmu_as(domain);
> - struct device_node *np = dev->of_node;
> - struct of_phandle_args args;
>   unsigned int index = 0;
>   int err = 0;
>  
> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> -)) {
> - unsigned int swgroup = args.args[0];
> -
> - if (args.np != smmu->dev->of_node) {
> - of_node_put(args.np);
> - continue;
> - }
> -
> - of_node_put(args.np);
> + if (!fwspec)
> + return -ENOENT;

Could the !fwspec ever be true here as well?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/3] iommu/tegra-smmu: Add PCI support

2020-10-02 Thread Dmitry Osipenko
02.10.2020 09:08, Nicolin Chen пишет:
> @@ -865,7 +866,11 @@ static struct iommu_group 
> *tegra_smmu_device_group(struct device *dev)
>   group->smmu = smmu;
>   group->soc = soc;
>  
> - group->group = iommu_group_alloc();
> + if (dev_is_pci(dev))
> + group->group = pci_device_group(dev);
> + else
> + group->group = generic_device_group(dev);
> +
>   if (IS_ERR(group->group)) {
>   devm_kfree(smmu->dev, group);
>   mutex_unlock(>lock);
> @@ -1069,22 +1074,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device 
> *dev,
>   iommu_device_set_fwnode(>iommu, dev->fwnode);
>  
>   err = iommu_device_register(>iommu);
> - if (err) {
> - iommu_device_sysfs_remove(>iommu);
> - return ERR_PTR(err);
> - }
> + if (err)
> + goto err_sysfs;
>  
>   err = bus_set_iommu(_bus_type, _smmu_ops);
> - if (err < 0) {
> - iommu_device_unregister(>iommu);
> - iommu_device_sysfs_remove(>iommu);
> - return ERR_PTR(err);
> - }
> + if (err < 0)
> + goto err_unregister;
> +
> +#ifdef CONFIG_PCI
> + err = bus_set_iommu(_bus_type, _smmu_ops);
> + if (err < 0)
> + goto err_bus_set;
> +#endif
>  
>   if (IS_ENABLED(CONFIG_DEBUG_FS))
>   tegra_smmu_debugfs_init(smmu);
>  
>   return smmu;
> +
> +err_bus_set: __maybe_unused;

__maybe_unused?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-02 Thread Dmitry Osipenko
02.10.2020 09:08, Nicolin Chen пишет:
>  static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device 
> *dev)
>  {
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>   struct tegra_smmu_as *as = to_smmu_as(domain);
> - struct device_node *np = dev->of_node;
>   struct tegra_smmu *smmu = as->smmu;
> - struct of_phandle_args args;
>   unsigned int index = 0;
>  
> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> -)) {
> - unsigned int swgroup = args.args[0];
> -
> - if (args.np != smmu->dev->of_node) {
> - of_node_put(args.np);
> - continue;
> - }
> -
> - of_node_put(args.np);
> + if (!fwspec)
> + return;

When !fwspec could be true?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Dmitry Osipenko
02.10.2020 09:08, Nicolin Chen пишет:
>  static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
>  {
> - struct device_node *np = dev->of_node;
> - struct tegra_smmu *smmu = NULL;
> - struct of_phandle_args args;
> - unsigned int index = 0;
> - int err;
> -
> - while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> -   ) == 0) {
> - smmu = tegra_smmu_find(args.np);
> - if (smmu) {
> - err = tegra_smmu_configure(smmu, dev, );
> - of_node_put(args.np);
> -
> - if (err < 0)
> - return ERR_PTR(err);
> -
> - /*
> -  * Only a single IOMMU master interface is currently
> -  * supported by the Linux kernel, so abort after the
> -  * first match.
> -  */
> - dev_iommu_priv_set(dev, smmu);
> -
> - break;
> - }
> -
> - of_node_put(args.np);
> - index++;
> - }
> + struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>  
>   if (!smmu)
>   return ERR_PTR(-ENODEV);

The !smmu can't ever be true now, isn't it? Then please remove it.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Dmitry Osipenko
02.10.2020 09:08, Nicolin Chen пишет:
>  static int tegra_smmu_of_xlate(struct device *dev,
>  struct of_phandle_args *args)
>  {
> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>   u32 id = args->args[0];
>  
> + of_node_put(args->np);
> +
> + if (!mc || !mc->smmu)
> + return -EPROBE_DEFER;

platform_get_drvdata(NULL) will crash.

> + dev_iommu_priv_set(dev, mc->smmu);

I think put_device(mc->dev) is missed here, doesn't it?

Why sun50i-iommu driver doesn't have this error-checking? Is it really
needed at all?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-02 Thread Dmitry Osipenko
02.10.2020 09:08, Nicolin Chen пишет:
>  static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>struct device *dev)
>  {
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>   struct tegra_smmu_as *as = to_smmu_as(domain);
> - struct device_node *np = dev->of_node;
> - struct of_phandle_args args;
>   unsigned int index = 0;
>   int err = 0;

Looks like there is no need to initialize 'index' and 'err' variables
anymore.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Dmitry Osipenko
02.10.2020 09:08, Nicolin Chen пишет:
> -static void tegra_smmu_release_device(struct device *dev)
> -{
> - dev_iommu_priv_set(dev, NULL);
> -}
> +static void tegra_smmu_release_device(struct device *dev) {}

Please keep the braces as-is.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-01 Thread Dmitry Osipenko
02.10.2020 04:07, Nicolin Chen пишет:
> On Thu, Oct 01, 2020 at 11:33:38PM +0300, Dmitry Osipenko wrote:
>>>>> If we can't come to an agreement on globalizing mc pointer, would
>>>>> it be possible to pass tegra_mc_driver through tegra_smmu_probe()
>>>>> so we can continue to use driver_find_device_by_fwnode() as v1?
>>>>>
>>>>> v1: https://lkml.org/lkml/2020/9/26/68
>>>>
>>>> tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean
>>>> tegra_smmu_probe_device()? I don't think we can do that because it isn't
>>>
>>> I was saying to have a global parent_driver pointer: similar to
>>> my v1, yet rather than "extern" the tegra_mc_driver, we pass it
>>> through egra_smmu_probe() and store it in a static global value
>>> so as to call tegra_smmu_get_by_fwnode() in ->probe_device().
>>>
>>> Though I agree that creating a global device pointer (mc) might
>>> be controversial, yet having a global parent_driver pointer may
>>> not be against the rule, considering that it is common in iommu
>>> drivers to call driver_find_device_by_fwnode in probe_device().
>>
>> You don't need the global pointer if you have SMMU OF node.
>>
>> You could also get driver pointer from mc->dev->driver.
>>
>> But I don't think you need to do this at all. The probe_device() could
>> be invoked only for the tegra_smmu_ops and then seems you could use
>> dev_iommu_priv_set() in tegra_smmu_of_xlate(), like sun50i-iommu driver
>> does.
> 
> Getting iommu device pointer using driver_find_device_by_fwnode()
> is a common practice in ->probe_device() of other iommu drivers.

Please give me a full list of the IOMMU drivers which use this method.

> But this requires a device_driver pointer that tegra-smmu doesn't
> have. So passing tegra_mc_driver through tegra_smmu_probe() will
> address it.
> 

If you're borrowing code and ideas from other drivers, then at least
please borrow them from a modern good-looking drivers. And I already
pointed out that following cargo cult is not always a good idea.

ARM-SMMU isn't a modern driver and it has legacy code. You shouldn't
copy it blindly. The sun50i-iommu driver was added half year ago, you
may use it as a reference.

Always consult the IOMMU core code. If you're too unsure about
something, then maybe better to start a new thread and ask Joerg about
the best modern practices that IOMMU drivers should use.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-01 Thread Dmitry Osipenko
01.10.2020 14:04, Nicolin Chen пишет:
> On Thu, Oct 01, 2020 at 12:23:16PM +0200, Thierry Reding wrote:
>  > > >> It looks to me like the only reason why you need this new global 
> API is
>> because PCI devices may not have a device tree node with a phandle to
>> the IOMMU. However, SMMU support for PCI will only be enabled if the
>> root complex has an iommus property, right? In that case, can't we
>> simply do something like this:
>>
>>  if (dev_is_pci(dev))
>>  np = find_host_bridge(dev)->of_node;
>>  else
>>  np = dev->of_node;
> 
>>> I personally am not a fan of adding a path for PCI device either,
>>> since PCI/IOMMU cores could have taken care of it while the same
>>> path can't be used for other buses.
>>
>> There's already plenty of other drivers that do something similar to
>> this. Take a look at the arm-smmu driver, for example, which seems to be
>> doing exactly the same thing to finding the right device tree node to
>> look at (see dev_get_dev_node() in drivers/iommu/arm-smmu/arm-smmu.c).
> 
> Hmm..okay..that is quite convincing then...

Not very convincing to me. I don't see a "plenty of other drivers",
there is only one arm-smmu driver.

The dev_get_dev_node() is under CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS (!).
Guys, doesn't it look strange to you? :)

The arm-smmu driver does a similar thing for the modern bindings to what
Nicolin's v3 is doing.

>>> If we can't come to an agreement on globalizing mc pointer, would
>>> it be possible to pass tegra_mc_driver through tegra_smmu_probe()
>>> so we can continue to use driver_find_device_by_fwnode() as v1?
>>>
>>> v1: https://lkml.org/lkml/2020/9/26/68
>>
>> tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean
>> tegra_smmu_probe_device()? I don't think we can do that because it isn't
> 
> I was saying to have a global parent_driver pointer: similar to
> my v1, yet rather than "extern" the tegra_mc_driver, we pass it
> through egra_smmu_probe() and store it in a static global value
> so as to call tegra_smmu_get_by_fwnode() in ->probe_device().
> 
> Though I agree that creating a global device pointer (mc) might
> be controversial, yet having a global parent_driver pointer may
> not be against the rule, considering that it is common in iommu
> drivers to call driver_find_device_by_fwnode in probe_device().

You don't need the global pointer if you have SMMU OF node.

You could also get driver pointer from mc->dev->driver.

But I don't think you need to do this at all. The probe_device() could
be invoked only for the tegra_smmu_ops and then seems you could use
dev_iommu_priv_set() in tegra_smmu_of_xlate(), like sun50i-iommu driver
does.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-01 Thread Dmitry Osipenko
...
>> There are dozens variants of the panels and system could easily have
>> more than one panel, hence a direct lookup by phandle is a natural
>> choice for the panels.
> 
> Not really, there's typically only just one panel. But that's just one
> example. EMC would be another. There's only a single EMC on Tegra and
> yet for something like interconnects we still reference it by phandle.

Interconnect is a generic API.

> PMC is another case and so is CAR, GPIO (at least on early Tegra SoCs)
> and pinmux, etc.
> 
> The example of GPIO shows very well how this is important. If we had
> made the assumption from the beginning that there was only ever going to
> be a single GPIO controller, then we would've had a big problem when the
> first SoC shipped that had multiple GPIO controllers.

This is true, but only if all these words are applied to the generic APIs.

>> While all Tegra SoCs have a single fixed MC in the system, and thus,
>> there is no real need to use phandle because we can't mix up MC with
>> anything else.
> 
> The same is true for the SMMU, and yet the iommus property references
> the SMMU by phandle. There are a *lot* of cases where you could imply
> dependencies because you have intimate knowledge about the hardware
> within drivers. But the point is to avoid this wherever possible so
> that the DTB is as self-describing as possible.
> 
 older DTs if DT change will be needed. Please give a detailed explanation.
>>>
>>> New functionality doesn't have to work with older DTs.
>>
>> This is fine in general, but I'm afraid that in this particular case we
>> will need to have a fall back anyways because otherwise it should break
>> the old functionality.
> 
> It looks like tegra20-devfreq is the only one that currently does this
> lookup via compatible string. And looking at the driver, what it does is
> pretty horrible, to be honest. It gets a reference to the memory
> controller and then simply accesses registers within the memory
> controller without any type of protection against concurrent accesses or
> reference counting to make sure the registers it accesses are still
> valid. At the very least this should've been a regmap.

Regmap is about abstracting accesses to devices that may sit on
different types of buses, like I2C or SPI for example. Or devices that
have a non-trivial registers mapping, or have slow IO and need caching.

I think you meant regmap in regards to protecting IO accesses, but this
is not what regmap is about if IO accesses are atomic by nature.

The tegra20-devfreq functionality is very separated from the rest of the
memory controller, hence there are no conflicts in regards to hardware
accesses, so there is nothing to protect.

Also, Regmap API itself doesn't manage refcounting of the mappings.

> And not
> coincidentally, regmaps are usually passed around by referencing their
> provider via phandle.

Any real-world examples? I think you're mixing up regmap with something
else.

The devfreq driver works just like the SMMU and GART. The devfreq device
is supposed to be created only once both MC and EMC drivers are loaded
and we know that they can't go away [1].

[1]
https://patchwork.ozlabs.org/project/linux-tegra/patch/20200814000621.8415-32-dig...@gmail.com/

Hence the tegra20-devfreq driver is horrible as much as the SMMU and
GART drivers. Perhaps not much could be done about it unless MC driver
is converted to MFD. But MFD won't work for tegra20-devfreq driver
anyways because it depends on presence of both MC and EMC drivers
simultaneously :)

Besides you didn't want the MFD couple years ago [2].

[2]
https://patchwork.ozlabs.org/project/linux-tegra/patch/675f74f82378b5f7d8f61d35e929614a0e156141.1523301400.git.dig...@gmail.com/#1902020

> That's exactly the kind of hack that I want to prevent from happening.
> If you can just grab a pointer to the memory controller with a global
> function pointer it makes people think that it's okay to use this kind
> of shortcut. But it isn't.
> Given the above, the lookup-by-compatible fallback should stay limited
> to tegra20-devfreq. Everything else should move to something saner. So
> this new helper should look up by phandle and not have a fallback, but
> instead the tegra20-devfreq should fall back if the new helper doesn't
> return anything useful (probably something like -ENOENT, meaning that
> there's no phandle and that we're using an old device tree). Bonus
> points for updating the DT bindings for tegra20-devfreq to also allow
> the memory controller to be specified by phandle and use a regmap for
> the shared registers.
The tegra20-devfreq driver doesn't share registers with other drivers.
MC statistics collection is a part of the MC, but it has no connection
to the other functions of the MC, at least from SW perspective.

Apparently you're missing that it's still not a problem to change the
T20 DT because all the MC-related drivers are still inactive in the
upstream kernel and awaiting the interconnect support 

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
01.10.2020 05:48, Nicolin Chen пишет:
> On Thu, Oct 01, 2020 at 05:06:19AM +0300, Dmitry Osipenko wrote:
>> 01.10.2020 04:26, Nicolin Chen пишет:
>>> On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote:
>>>> 01.10.2020 00:32, Nicolin Chen пишет:
>>>>> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
>>>>>> ...
>>>>>>>> It looks to me like the only reason why you need this new global API is
>>>>>>>> because PCI devices may not have a device tree node with a phandle to
>>>>>>>> the IOMMU. However, SMMU support for PCI will only be enabled if the
>>>>>>>> root complex has an iommus property, right? In that case, can't we
>>>>>>>> simply do something like this:
>>>>>>>>
>>>>>>>>if (dev_is_pci(dev))
>>>>>>>>np = find_host_bridge(dev)->of_node;
>>>>>>>>else
>>>>>>>>np = dev->of_node;
>>>>>>>>
>>>>>>>> ? I'm not sure exactly what find_host_bridge() is called, but I'm 
>>>>>>>> pretty
>>>>>>>> sure that exists.
>>>>>>>>
>>>>>>>> Once we have that we can still iterate over the iommus property and do
>>>>>>>> not need to rely on this global variable.
>>>>>>>
>>>>>>> I agree that it'd work. But I was hoping to simplify the code
>>>>>>> here if it's possible. Looks like we have an argument on this
>>>>>>> so I will choose to go with your suggestion above for now.
>>>>>>
>>>>>> This patch removed more lines than were added. If this will be opposite
>>>>>> for the Thierry's suggestion, then it's probably not a great suggestion.
>>>>>
>>>>> Sorry, I don't quite understand this comments. Would you please
>>>>> elaborate what's this "it" being "not a great suggestion"?
>>>>>
>>>>
>>>> I meant that you should try to implement Thierry's solution, but if the
>>>> end result will be worse than the current patch, then you shouldn't make
>>>> a v4, but get back to this discussion in order to choose the best option
>>>> and make everyone agree on it.
>>>
>>> I see. Thanks for the reply. And here is a sample implementation:
>>
>> That's what I supposed to happen :) The new variant adds code and
>> complexity, while old did the opposite. Hence the old variant is clearly
>> more attractive, IMO.
> 
> I personally am not a fan of adding a path for PCI device either,
> since PCI/IOMMU cores could have taken care of it while the same
> path can't be used for other buses.
> 
> If we can't come to an agreement on globalizing mc pointer, would
> it be possible to pass tegra_mc_driver through tegra_smmu_probe()
> so we can continue to use driver_find_device_by_fwnode() as v1?
> 
> v1: https://lkml.org/lkml/2020/9/26/68
> 

I think we already agreed that it will be good to have a common helper.
So far Thierry only objected that the implementation of the helper could
be improved.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
30.09.2020 19:47, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 07:25:41PM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 19:06, Thierry Reding пишет:
>>> On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
>>>>  I'...
>>>>>> +struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
>>>>>> +struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>>>>
>>>>> It looks to me like the only reason why you need this new global API is
>>>>> because PCI devices may not have a device tree node with a phandle to
>>>>> the IOMMU. However, SMMU support for PCI will only be enabled if the
>>>>> root complex has an iommus property, right? In that case, can't we
>>>>> simply do something like this:
>>>>>
>>>>>   if (dev_is_pci(dev))
>>>>>   np = find_host_bridge(dev)->of_node;
>>>>>   else
>>>>>   np = dev->of_node;
>>>>>
>>>>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
>>>>> sure that exists.
>>>>>
>>>>> Once we have that we can still iterate over the iommus property and do
>>>>> not need to rely on this global variable.
>>>>
>>>> This sounds more complicated than the current variant.
>>>>
>>>> Secondly, I'm already about to use the new tegra_get_memory_controller()
>>>> API for all the T20/30/124/210 EMC and devfreq drivers.
>>>
>>> Why do we need it there? They seem to work fine without it right now.
>>
>> All the Tegra30/124/210 EMC drivers are already duplicating that MC
>> lookup code and only the recent T210 driver does it properly.
>>
>>> If it is required for new functionality, we can always make the dependent
>>> on a DT reference via phandle without breaking any existing code.
>>
>> That's correct, it will be also needed for the new functionality as
>> well, hence even more drivers will need to perform the MC lookup.
> 
> I don't have any issues with adding a helper if we need it from several
> different locations. But the helper should be working off of a given
> device and look up the device via the device tree node referenced by
> phandle. We already have those phandles in place for the EMC devices,
> and any other device that needs to interoperate with the MC should also
> get such a reference.
> 
>> I don't quite understand why you're asking for the phandle reference,
>> it's absolutely not needed for the MC lookup and won't work for the
> 
> We need that phandle in order to establish a link between the devices.
> Yes, you can probably do it without the phandle and just match by
> compatible string. But we don't do that for other types of devices
> either, right? For a display driver we reference the attached panel via
> phandle, but we could also just look it up via name or absolute path or
> some other heuristic. But a phandle is just a much more explicit way of
> linking the devices, so why not use it?

There are dozens variants of the panels and system could easily have
more than one panel, hence a direct lookup by phandle is a natural
choice for the panels.

While all Tegra SoCs have a single fixed MC in the system, and thus,
there is no real need to use phandle because we can't mix up MC with
anything else.

>> older DTs if DT change will be needed. Please give a detailed explanation.
> 
> New functionality doesn't have to work with older DTs.

This is fine in general, but I'm afraid that in this particular case we
will need to have a fall back anyways because otherwise it should break
the old functionality.

So I don't think that using phandle for the MC device finding is really
warrant.

Phandle is kinda more applicable for the cases where only the DT node
lookup is needed (not the lookup of the MC device driver), but even then
it is also not mandatory.

I hope you agree.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
01.10.2020 04:26, Nicolin Chen пишет:
> On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote:
>> 01.10.2020 00:32, Nicolin Chen пишет:
>>> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
>>>> ...
>>>>>> It looks to me like the only reason why you need this new global API is
>>>>>> because PCI devices may not have a device tree node with a phandle to
>>>>>> the IOMMU. However, SMMU support for PCI will only be enabled if the
>>>>>> root complex has an iommus property, right? In that case, can't we
>>>>>> simply do something like this:
>>>>>>
>>>>>>  if (dev_is_pci(dev))
>>>>>>  np = find_host_bridge(dev)->of_node;
>>>>>>  else
>>>>>>  np = dev->of_node;
>>>>>>
>>>>>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
>>>>>> sure that exists.
>>>>>>
>>>>>> Once we have that we can still iterate over the iommus property and do
>>>>>> not need to rely on this global variable.
>>>>>
>>>>> I agree that it'd work. But I was hoping to simplify the code
>>>>> here if it's possible. Looks like we have an argument on this
>>>>> so I will choose to go with your suggestion above for now.
>>>>
>>>> This patch removed more lines than were added. If this will be opposite
>>>> for the Thierry's suggestion, then it's probably not a great suggestion.
>>>
>>> Sorry, I don't quite understand this comments. Would you please
>>> elaborate what's this "it" being "not a great suggestion"?
>>>
>>
>> I meant that you should try to implement Thierry's solution, but if the
>> end result will be worse than the current patch, then you shouldn't make
>> a v4, but get back to this discussion in order to choose the best option
>> and make everyone agree on it.
> 
> I see. Thanks for the reply. And here is a sample implementation:

That's what I supposed to happen :) The new variant adds code and
complexity, while old did the opposite. Hence the old variant is clearly
more attractive, IMO.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
01.10.2020 00:32, Nicolin Chen пишет:
> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
>> ...
>>>> It looks to me like the only reason why you need this new global API is
>>>> because PCI devices may not have a device tree node with a phandle to
>>>> the IOMMU. However, SMMU support for PCI will only be enabled if the
>>>> root complex has an iommus property, right? In that case, can't we
>>>> simply do something like this:
>>>>
>>>>if (dev_is_pci(dev))
>>>>np = find_host_bridge(dev)->of_node;
>>>>else
>>>>np = dev->of_node;
>>>>
>>>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
>>>> sure that exists.
>>>>
>>>> Once we have that we can still iterate over the iommus property and do
>>>> not need to rely on this global variable.
>>>
>>> I agree that it'd work. But I was hoping to simplify the code
>>> here if it's possible. Looks like we have an argument on this
>>> so I will choose to go with your suggestion above for now.
>>
>> This patch removed more lines than were added. If this will be opposite
>> for the Thierry's suggestion, then it's probably not a great suggestion.
> 
> Sorry, I don't quite understand this comments. Would you please
> elaborate what's this "it" being "not a great suggestion"?
> 

I meant that you should try to implement Thierry's solution, but if the
end result will be worse than the current patch, then you shouldn't make
a v4, but get back to this discussion in order to choose the best option
and make everyone agree on it.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
...
>> It looks to me like the only reason why you need this new global API is
>> because PCI devices may not have a device tree node with a phandle to
>> the IOMMU. However, SMMU support for PCI will only be enabled if the
>> root complex has an iommus property, right? In that case, can't we
>> simply do something like this:
>>
>>  if (dev_is_pci(dev))
>>  np = find_host_bridge(dev)->of_node;
>>  else
>>  np = dev->of_node;
>>
>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
>> sure that exists.
>>
>> Once we have that we can still iterate over the iommus property and do
>> not need to rely on this global variable.
> 
> I agree that it'd work. But I was hoping to simplify the code
> here if it's possible. Looks like we have an argument on this
> so I will choose to go with your suggestion above for now.

This patch removed more lines than were added. If this will be opposite
for the Thierry's suggestion, then it's probably not a great suggestion.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

2020-09-30 Thread Dmitry Osipenko
...
>> I don't think that the MC driver will stay built-in forever, although
>> its modularization is complicated right now. Hence something shall keep
>> the reference to the MC device resources while they are in use and this
>> patch takes care of doing that.
> 
> It looks to me like all the other places where we get a reference to the
> MC also keep a reference to the device. That's obviously not going to be
> enough once the code is turned into a module. At that point we need to
> make sure to also grab a reference to the module. But that's orthogonal
> to this discussion.
> 
>> Secondly, the Nicolin's patch doesn't pretend on anything, but rather
> 
> Yes, the patch does pretend to "look up" the memory controller device,
> but in reality it will always return a singleton object, which can just
> as easily be achieved by using a global variable.
> 
>> brings the already existing duplicated code to a single common place.
> 
> Where exactly is that duplicated code? The only places I see where we
> get a reference to the memory controller are from the EMC drivers and
> they properly look up the MC via the nvidia,memory-controller device
> tree property.

Yours observation is correct, all the drivers *do the lookup*. My point
is that the nvidia,memory-controller usage isn't strictly necessary.

The tegra20-devfreq driver doesn't use the phandle lookup because
Tegra20 DTs don't have it, instead it uses the compatible lookup. Hence
this patch doesn't really change the already existing behaviour for the
drivers. The phandle usage is optional.

This patch adds a common API that is usable by *all* the already
existing drivers, starting from the Tegra20 drivers.

> But that's not what this new helper does. This code will use the OF
> lookup table to find any match and then returns that, completely
> ignoring any links established by the device tree.

As I already said in the other reply, it should be fine to add lookup by
the phandle and then fall back to the compatible matching. On the other
hand, this is not strictly necessary because we always have only a
single MC device at a time.

Please note that I don't have any objections to improving this patch. In
the end either way will work, so we just need to choose the best option.
I was merely explaining the rationale behind this patch and not trying
to defend it.

Yours suggestion about using static mc variable is also good to me since
currently MC driver is built-in and at least that won't be a
globally-visible kernel variable, which doesn't feel great to have.

I think we can follow approach of the static mc variable for the starter
and improve it once there will be a real need. This should be the
simplest option right now.

But again, we may slightly future-proof the API by adding the
resource-managed variant. Either way will be good, IMO :) Currently I
don't have a strong preference.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
...
>> Secondly, I'm already about to use the new tegra_get_memory_controller()
>> API for all the T20/30/124/210 EMC and devfreq drivers.
> 
> Also, this really proves the point I was trying to make about how this
> is going to proliferate...

Sorry, I'm probably totally missing yours point.. "what" exactly will
proliferate?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
30.09.2020 19:06, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
>>  I'...
>>>> +  struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
>>>> +  struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>>
>>> It looks to me like the only reason why you need this new global API is
>>> because PCI devices may not have a device tree node with a phandle to
>>> the IOMMU. However, SMMU support for PCI will only be enabled if the
>>> root complex has an iommus property, right? In that case, can't we
>>> simply do something like this:
>>>
>>> if (dev_is_pci(dev))
>>> np = find_host_bridge(dev)->of_node;
>>> else
>>> np = dev->of_node;
>>>
>>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
>>> sure that exists.
>>>
>>> Once we have that we can still iterate over the iommus property and do
>>> not need to rely on this global variable.
>>
>> This sounds more complicated than the current variant.
>>
>> Secondly, I'm already about to use the new tegra_get_memory_controller()
>> API for all the T20/30/124/210 EMC and devfreq drivers.
> 
> Why do we need it there? They seem to work fine without it right now.

All the Tegra30/124/210 EMC drivers are already duplicating that MC
lookup code and only the recent T210 driver does it properly.

> If it is required for new functionality, we can always make the dependent
> on a DT reference via phandle without breaking any existing code.

That's correct, it will be also needed for the new functionality as
well, hence even more drivers will need to perform the MC lookup.

I don't quite understand why you're asking for the phandle reference,
it's absolutely not needed for the MC lookup and won't work for the
older DTs if DT change will be needed. Please give a detailed explanation.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

2020-09-30 Thread Dmitry Osipenko
30.09.2020 19:15, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 19:03, Thierry Reding пишет:
>>> On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
>>>> 30.09.2020 18:23, Thierry Reding пишет:
>>>>> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>>>>>> From: Dmitry Osipenko 
>>>>>>
>>>>>> Multiple Tegra drivers need to retrieve Memory Controller and hence there
>>>>>> is quite some duplication of the retrieval code among the drivers. Let's
>>>>>> add a new common helper for the retrieval of the MC.
>>>>>>
>>>>>> Signed-off-by: Dmitry Osipenko 
>>>>>> Signed-off-by: Nicolin Chen 
>>>>>> ---
>>>>>>
>>>>>> Changelog
>>>>>> v2->v3:
>>>>>>  * Replaced with Dimtry's devm_tegra_get_memory_controller()
>>>>>> v1->v2:
>>>>>>  * N/A
>>>>>>
>>>>>>  drivers/memory/tegra/mc.c | 39 +++
>>>>>>  include/soc/tegra/mc.h| 17 +
>>>>>>  2 files changed, 56 insertions(+)
>>>>>
>>>>> Let's not add this helper, please. If a device needs a reference to the
>>>>> memory controller, it should have a phandle to the memory controller in
>>>>> device tree so that it can be looked up explicitly.
>>>>>
>>>>> Adding this helper is officially sanctioning that it's okay not to have
>>>>> that reference and that's a bad idea.
>>>>
>>>> And please explain why it's a bad idea, I don't see anything bad here at
>>>> all.
>>>
>>> Well, you said yourself in a recent comment that we should avoid global
>>> variables. devm_tegra_get_memory_controller() is nothing but a glorified
>>> global variable.
>>
>> This is not a variable, but a common helper function which will remove
>> the duplicated code and will help to avoid common mistakes like a missed
>> put_device().
> 
> Yeah, you're right: this is actually much worse than a global variable.
> It's a helper function that needs 50+ lines in order to effectively
> access a global variable.
> 
> You could write this much simpler by doing something like this:
> 
>   static struct tegra_mc *global_mc;
> 
>   int tegra_mc_probe(...)
>   {
>   ...
> 
>   global_mc = mc;
> 
>   ...
>   }
> 
>   struct tegra_mc *tegra_get_memory_controller(void)
>   {
>   return global_mc;
>   }
> 
> The result is *exactly* the same, except that this is actually more
> honest. Nicolin's patch *pretends* that it isn't using a global variable
> by wrapping a lot of complicated code around it.
> 
> But that doesn't change the fact that this accesses a singleton object
> without actually being able to tie it to the device in the first place.

I don't think that the MC driver will stay built-in forever, although
its modularization is complicated right now. Hence something shall keep
the reference to the MC device resources while they are in use and this
patch takes care of doing that.

Secondly, the Nicolin's patch doesn't pretend on anything, but rather
brings the already existing duplicated code to a single common place.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

2020-09-30 Thread Dmitry Osipenko
30.09.2020 19:03, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 18:23, Thierry Reding пишет:
>>> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>>>> From: Dmitry Osipenko 
>>>>
>>>> Multiple Tegra drivers need to retrieve Memory Controller and hence there
>>>> is quite some duplication of the retrieval code among the drivers. Let's
>>>> add a new common helper for the retrieval of the MC.
>>>>
>>>> Signed-off-by: Dmitry Osipenko 
>>>> Signed-off-by: Nicolin Chen 
>>>> ---
>>>>
>>>> Changelog
>>>> v2->v3:
>>>>  * Replaced with Dimtry's devm_tegra_get_memory_controller()
>>>> v1->v2:
>>>>  * N/A
>>>>
>>>>  drivers/memory/tegra/mc.c | 39 +++
>>>>  include/soc/tegra/mc.h| 17 +
>>>>  2 files changed, 56 insertions(+)
>>>
>>> Let's not add this helper, please. If a device needs a reference to the
>>> memory controller, it should have a phandle to the memory controller in
>>> device tree so that it can be looked up explicitly.
>>>
>>> Adding this helper is officially sanctioning that it's okay not to have
>>> that reference and that's a bad idea.
>>
>> And please explain why it's a bad idea, I don't see anything bad here at
>> all.
> 
> Well, you said yourself in a recent comment that we should avoid global
> variables. devm_tegra_get_memory_controller() is nothing but a glorified
> global variable.

This is not a variable, but a common helper function which will remove
the duplicated code and will help to avoid common mistakes like a missed
put_device().
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

2020-09-30 Thread Dmitry Osipenko
30.09.2020 18:23, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>> From: Dmitry Osipenko 
>>
>> Multiple Tegra drivers need to retrieve Memory Controller and hence there
>> is quite some duplication of the retrieval code among the drivers. Let's
>> add a new common helper for the retrieval of the MC.
>>
>> Signed-off-by: Dmitry Osipenko 
>> Signed-off-by: Nicolin Chen 
>> ---
>>
>> Changelog
>> v2->v3:
>>  * Replaced with Dimtry's devm_tegra_get_memory_controller()
>> v1->v2:
>>  * N/A
>>
>>  drivers/memory/tegra/mc.c | 39 +++
>>  include/soc/tegra/mc.h| 17 +
>>  2 files changed, 56 insertions(+)
> 
> Let's not add this helper, please. If a device needs a reference to the
> memory controller, it should have a phandle to the memory controller in
> device tree so that it can be looked up explicitly.
> 
> Adding this helper is officially sanctioning that it's okay not to have
> that reference and that's a bad idea.

And please explain why it's a bad idea, I don't see anything bad here at
all.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
 I'...
>> +struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
>> +struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> 
> It looks to me like the only reason why you need this new global API is
> because PCI devices may not have a device tree node with a phandle to
> the IOMMU. However, SMMU support for PCI will only be enabled if the
> root complex has an iommus property, right? In that case, can't we
> simply do something like this:
> 
>   if (dev_is_pci(dev))
>   np = find_host_bridge(dev)->of_node;
>   else
>   np = dev->of_node;
> 
> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> sure that exists.
> 
> Once we have that we can still iterate over the iommus property and do
> not need to rely on this global variable.

This sounds more complicated than the current variant.

Secondly, I'm already about to use the new tegra_get_memory_controller()
API for all the T20/30/124/210 EMC and devfreq drivers.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

2020-09-30 Thread Dmitry Osipenko
30.09.2020 18:23, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>> From: Dmitry Osipenko 
>>
>> Multiple Tegra drivers need to retrieve Memory Controller and hence there
>> is quite some duplication of the retrieval code among the drivers. Let's
>> add a new common helper for the retrieval of the MC.
>>
>> Signed-off-by: Dmitry Osipenko 
>> Signed-off-by: Nicolin Chen 
>> ---
>>
>> Changelog
>> v2->v3:
>>  * Replaced with Dimtry's devm_tegra_get_memory_controller()
>> v1->v2:
>>  * N/A
>>
>>  drivers/memory/tegra/mc.c | 39 +++
>>  include/soc/tegra/mc.h| 17 +
>>  2 files changed, 56 insertions(+)
> 
> Let's not add this helper, please. If a device needs a reference to the
> memory controller, it should have a phandle to the memory controller in
> device tree so that it can be looked up explicitly.
> 
> Adding this helper is officially sanctioning that it's okay not to have
> that reference and that's a bad idea.

Maybe that's because the reference isn't really needed for the lookup? :)

Secondly, we could use the reference and then fall back to the
of-matching for devices that don't have the reference, like all Tegra20
devices + Tegra30/124 ACTMON devices.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

2020-09-30 Thread Dmitry Osipenko
30.09.2020 17:45, Krzysztof Kozlowski пишет:
> On Wed, 30 Sep 2020 at 16:41, Dmitry Osipenko  wrote:
>>
>> ...
>>> +struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev)
>>> +{
>>> + struct platform_device *pdev;
>>> + struct device_node *np;
>>> + struct tegra_mc *mc;
>>> + int err;
>>> +
>>> + np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, NULL);
>>> + if (!np)
>>> + return ERR_PTR(-ENOENT);
>>> +
>>> + pdev = of_find_device_by_node(np);
>>> + of_node_put(np);
>>> + if (!pdev)
>>> + return ERR_PTR(-ENODEV);
>>> +
>>> + mc = platform_get_drvdata(pdev);
>>> + if (!mc) {
>>> + put_device(mc->dev);
>>
>> This should be put_device(>dev). Please always be careful while
>> copying someones else code :)
> 
> Good catch. I guess devm_add_action_or_reset() would also work... or
> running Smatch on new code. Smatch should point it out.

The devm_add_action_or_reset() shouldn't help much in this particular
case because it's too early for the devm_add_action here.

Having an explicit put_device() in all error code paths also helps with
improving readability of the code a tad, IMO.

Smatch indeed should catch this bug, but Smatch usually isn't a part of
the developers workflow. More often Smatch is a part of maintainers
workflow, hence such problems usually are getting caught before patches
are applied.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
...
>  static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>struct device *dev)
>  {
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>   struct tegra_smmu_as *as = to_smmu_as(domain);
> - struct device_node *np = dev->of_node;
> - struct of_phandle_args args;
>   unsigned int index = 0;
>   int err = 0;
>  
> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> -)) {
> - unsigned int swgroup = args.args[0];
> -
> - if (args.np != smmu->dev->of_node) {
> - of_node_put(args.np);
> - continue;
> - }
> -
> - of_node_put(args.np);
> + if (!fwspec || fwspec->ops != _smmu_ops)
> + return -ENOENT;

In previous reply you said that these fwspec checks are borrowed from
the arm-smmu driver, but I'm now looking at what other drivers do and I
don't see them having this check.

Hence I'm objecting the need to have this check here. You either should
give a rational to having the check or it should be removed.

Please never blindly copy foreign code, you should always double-check it.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 3/3] iommu/tegra-smmu: Add PCI support

2020-09-30 Thread Dmitry Osipenko
...
> +#ifdef CONFIG_PCI
> + if (!iommu_present(_bus_type)) {


In the previous reply you said that you're borrowing this check from the
arm-smmu driver, but arm-smmu also has a similar check for
platform_bus_type, while Tegra SMMU driver doesn't have it. Hence I'm
objecting the necessity of having this check.

Please give a rationale for having this check in the code. And please
note that cargo cult isn't a rationale to me.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
...
> + struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>  
> - of_node_put(args.np);
> - index++;
> - }
> + /* An invalid mc pointer means mc and smmu drivers are not ready */
> + if (IS_ERR(mc))
> + return ERR_PTR(-EPROBE_DEFER);
>  
> - if (!smmu)
> + /*
> +  * IOMMU core allows -ENODEV return to carry on. So bypass any call
> +  * from bus_set_iommu() during tegra_smmu_probe(), as a device will
> +  * call in again via of_iommu_configure when fwspec is prepared.
> +  */
> + if (!mc->smmu || !fwspec || fwspec->ops != _smmu_ops)
>   return ERR_PTR(-ENODEV);
>  
> - return >iommu;
> + dev_iommu_priv_set(dev, mc->smmu);
> +
> + return >smmu->iommu;
>  }

Is it really okay to use devm_tegra_get_memory_controller() here?

I assume it should be more preferred to do it only for devices that have
fwspec->ops == _smmu_ops.

Secondly, it also looks to me that a non-devm variant should be more
appropriate here because tegra_smmu_probe_device() isn't invoked by the
devices themselves.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

2020-09-30 Thread Dmitry Osipenko
...
> +struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev)
> +{
> + struct platform_device *pdev;
> + struct device_node *np;
> + struct tegra_mc *mc;
> + int err;
> +
> + np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, NULL);
> + if (!np)
> + return ERR_PTR(-ENOENT);
> +
> + pdev = of_find_device_by_node(np);
> + of_node_put(np);
> + if (!pdev)
> + return ERR_PTR(-ENODEV);
> +
> + mc = platform_get_drvdata(pdev);
> + if (!mc) {
> + put_device(mc->dev);

This should be put_device(>dev). Please always be careful while
copying someones else code :)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v2 1/3] memory: tegra: Add helper function tegra_get_memory_controller

2020-09-30 Thread Dmitry Osipenko
30.09.2020 09:38, Nicolin Chen пишет:
> On Wed, Sep 30, 2020 at 09:32:20AM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 08:44, Nicolin Chen пишет:
>>> On Wed, Sep 30, 2020 at 08:12:10AM +0300, Dmitry Osipenko wrote:
>>>> 30.09.2020 03:30, Nicolin Chen пишет:
>>>> ...
>>>>>  int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long 
>>>>> rate);
>>>>>  unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
>>>>> +struct tegra_mc *tegra_get_memory_controller(void);
>>>>>  
>>>>>  #endif /* __SOC_TEGRA_MC_H__ */
>>>>>
>>>>
>>>> This will conflict with the tegra20-devfreq driver, you should change it
>>>> as well.
>>>
>>> Will remove that in v3.
>>>
>>> Thanks
>>>
>>
>> Please also consider to add a resource-managed variant, similar to what
>> I did here:
>>
>> https://github.com/grate-driver/linux/commit/2105e7664063772d72fefe9696bdab0b688b9de2
>>
>> I was going to use it in the next iteration of the memory interconnect
>> series.
>>
>> But now it also will be good if you could add the devm variant to yours
>> SMMU patchset since you're already about to touch the tegra20-devfreq
>> driver. I'll then rebase my patches on top of yours patch.
> 
> Just saw this reply. Yea, I think this'd be better. Thanks
> 

Please don't forget to add a stub for !MC as well since devfreq drivers
use COMPILE_TEST and don't directly depend on the MC driver.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
30.09.2020 09:13, Nicolin Chen пишет:
> On Wed, Sep 30, 2020 at 09:10:38AM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 08:49, Nicolin Chen пишет:
>>> On Wed, Sep 30, 2020 at 08:11:52AM +0300, Dmitry Osipenko wrote:
>>>> 30.09.2020 03:30, Nicolin Chen пишет:
>>>>> + /* An invalid mc pointer means mc and smmu drivers are not ready */
>>>>> + if (IS_ERR_OR_NULL(mc))
>>>>
>>>> tegra_get_memory_controller() doesn't return NULL.
>>>
>>> Well, I don't want to assume that it'd do that forever, and the
>>> NULL check of IS_ERR_OR_NULL is marked "unlikely" so it doesn't
>>> hurt to have.
>>>
>>
>> I don't see any reasons why it won't do that forever.
>>
>> Secondly, public function can't be changed randomly without updating all
>> the callers.
>>
>> Hence there is no need to handle cases that can't ever happen and it
>> hurts readability of the code + original error code is missed.
> 
> I don't quite understand why an extra "_OR_NULL" would hurt
> readabilitybut I'd take a step back and use IS_ERR().
> 

The tegra_get_memory_controller() doesn't return NULL, hence the
NULL-check is misleading.

If I was reading that code for the first time and notice such a thing,
then instantly I'd have a much lower credibility to the whole code.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] memory: tegra: Add helper function tegra_get_memory_controller

2020-09-30 Thread Dmitry Osipenko
30.09.2020 08:44, Nicolin Chen пишет:
> On Wed, Sep 30, 2020 at 08:12:10AM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 03:30, Nicolin Chen пишет:
>> ...
>>>  int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long 
>>> rate);
>>>  unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
>>> +struct tegra_mc *tegra_get_memory_controller(void);
>>>  
>>>  #endif /* __SOC_TEGRA_MC_H__ */
>>>
>>
>> This will conflict with the tegra20-devfreq driver, you should change it
>> as well.
> 
> Will remove that in v3.
> 
> Thanks
> 

Please also consider to add a resource-managed variant, similar to what
I did here:

https://github.com/grate-driver/linux/commit/2105e7664063772d72fefe9696bdab0b688b9de2

I was going to use it in the next iteration of the memory interconnect
series.

But now it also will be good if you could add the devm variant to yours
SMMU patchset since you're already about to touch the tegra20-devfreq
driver. I'll then rebase my patches on top of yours patch.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Dmitry Osipenko
30.09.2020 08:49, Nicolin Chen пишет:
> On Wed, Sep 30, 2020 at 08:11:52AM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 03:30, Nicolin Chen пишет:
>>> +   /* An invalid mc pointer means mc and smmu drivers are not ready */
>>> +   if (IS_ERR_OR_NULL(mc))
>>
>> tegra_get_memory_controller() doesn't return NULL.
> 
> Well, I don't want to assume that it'd do that forever, and the
> NULL check of IS_ERR_OR_NULL is marked "unlikely" so it doesn't
> hurt to have.
> 

I don't see any reasons why it won't do that forever.

Secondly, public function can't be changed randomly without updating all
the callers.

Hence there is no need to handle cases that can't ever happen and it
hurts readability of the code + original error code is missed.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support

2020-09-30 Thread Dmitry Osipenko
30.09.2020 08:34, Nicolin Chen пишет:
> On Wed, Sep 30, 2020 at 08:10:35AM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 03:30, Nicolin Chen пишет:
>>>  void tegra_smmu_remove(struct tegra_smmu *smmu)
>>>  {
>>> +   bus_set_iommu(_bus_type, NULL);
>>
>> Why only platform_bus? Is this really needed at all?
> 
> I see qcom_iommu.c file set to NULL in remove(), Probably should
> have added pci_bus_type too though.
> 
> Or are you sure that there's no need at all?
> 

It wasn't here before this patch and platform_bus is unrelated to the
topic of this patch. But it probably should be there.

On the other hand, the tegra_smmu_remove() is unused and maybe it could
be better to get rid of this unused function at all.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  1   2   3   4   >