[2/2] iommu/omap: Add support for configuring dsp iommus on DRA7xx
diff mbox

Message ID 1437522936-19758-3-git-send-email-s-anna@ti.com
State New
Headers show

Commit Message

Suman Anna July 21, 2015, 11:55 p.m. UTC
The DSP MMUs on DRA7xx SoC requires configuring an additional
MMU_CONFIG register present in the DSP_SYSTEM sub module. This
setting dictates whether the DSP Core's MDMA and EDMA traffic
is routed through the respective MMU or not. Add the support
to the OMAP iommu driver so that the traffic is not bypassed
when enabling the MMUs.

The MMU_CONFIG register has two different bits for enabling
each of these two MMUs present in the DSP processor sub-system
on DRA7xx. An id field is added to the OMAP iommu object to
identify and enable each IOMMU. The id information and the
DSP_SYSTEM.MMU_CONFIG register programming is achieved through
the processing of the optional "ti,syscon-mmuconfig" property.
A proper value is assigned to the id field only when this
property is present.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/iommu/omap-iommu.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/iommu/omap-iommu.h |  9 +++++++
 2 files changed, 67 insertions(+)

Comments

Tony Lindgren July 22, 2015, 5:26 a.m. UTC | #1
* Suman Anna <s-anna@ti.com> [150721 16:58]:
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -26,6 +26,8 @@
>  #include <linux/of_iommu.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
>  
>  #include <asm/cacheflush.h>
>  
> @@ -112,6 +114,18 @@ void omap_iommu_restore_ctx(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(omap_iommu_restore_ctx);
>  
> +static void dra7_cfg_dspsys_mmu(struct omap_iommu *obj, bool enable)
> +{
> +	u32 val, mask;
> +
> +	if (!obj->syscfg)
> +		return;
> +
> +	mask = (1 << (obj->id * DSP_SYS_MMU_CONFIG_EN_SHIFT));
> +	val = enable ? mask : 0;
> +	regmap_update_bits(obj->syscfg, DSP_SYS_MMU_CONFIG, mask, val);
> +}
> +
>  static void __iommu_set_twl(struct omap_iommu *obj, bool on)

I don't like using syscon for tinkering directly with SoC registers.

We should use some Linux generic framework for configuring these
bits to avoid nasty dependencies between various hardware modules
on the SoC.

What does DSP_SYS_MMU_CONFIG register do? It seems it's probably
a regulator or a gate clock? If so, it should be set up as a
regulator or a clock and then the omap-iommu driver can just
use regulator or clcok framework to request the resource.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Suman Anna July 22, 2015, 4:22 p.m. UTC | #2
Hi Tony,

On 07/22/2015 12:26 AM, Tony Lindgren wrote:
> * Suman Anna <s-anna@ti.com> [150721 16:58]:
>> --- a/drivers/iommu/omap-iommu.c
>> +++ b/drivers/iommu/omap-iommu.c
>> @@ -26,6 +26,8 @@
>>  #include <linux/of_iommu.h>
>>  #include <linux/of_irq.h>
>>  #include <linux/of_platform.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>>  
>>  #include <asm/cacheflush.h>
>>  
>> @@ -112,6 +114,18 @@ void omap_iommu_restore_ctx(struct device *dev)
>>  }
>>  EXPORT_SYMBOL_GPL(omap_iommu_restore_ctx);
>>  
>> +static void dra7_cfg_dspsys_mmu(struct omap_iommu *obj, bool enable)
>> +{
>> +	u32 val, mask;
>> +
>> +	if (!obj->syscfg)
>> +		return;
>> +
>> +	mask = (1 << (obj->id * DSP_SYS_MMU_CONFIG_EN_SHIFT));
>> +	val = enable ? mask : 0;
>> +	regmap_update_bits(obj->syscfg, DSP_SYS_MMU_CONFIG, mask, val);
>> +}
>> +
>>  static void __iommu_set_twl(struct omap_iommu *obj, bool on)
> 
> I don't like using syscon for tinkering directly with SoC registers.

This is not a SoC-level register, but a register within a sub-module of
the DSP processor sub-system. The DSP_SYSTEM sub-module in general is
described in Section 5.3.3 of the TRM [1], and it implements different
functionalities like the PRCM handshaking, wakeup logic and DSP
subsystem top-level configuration. It is a module present within the DSP
processor sub-system, so can only be accessed when the sub-system is
clocked and the appropriate reset is released.

> We should use some Linux generic framework for configuring these
> bits to avoid nasty dependencies between various hardware modules
> on the SoC.
> 
> What does DSP_SYS_MMU_CONFIG register do? It seems it's probably
> a regulator or a gate clock? If so, it should be set up as a
> regulator or a clock and then the omap-iommu driver can just
> use regulator or clcok framework to request the resource.

No, its neither. It is a control bit that dictates whether the
processor/EDMA addresses go through the respective MMU or not. The
register currently has 4 bits (bit 0 in each nibble), one each for
enabling each MMU and requesting an MMU abort on each. The MMU
integration and enablement notes are detailed in Section 5.3.6 of the
TRM [1], and the DSP_SYS_MMU_CONFIG register layout is in Table 5-28
(Page 1641).

regards
Suman

[1] http://www.ti.com/lit/ug/spruhz6/spruhz6.pdf


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren July 23, 2015, 7:24 a.m. UTC | #3
* Suman Anna <s-anna@ti.com> [150722 09:25]:
> On 07/22/2015 12:26 AM, Tony Lindgren wrote:
> > 
> > I don't like using syscon for tinkering directly with SoC registers.
> 
> This is not a SoC-level register, but a register within a sub-module of
> the DSP processor sub-system. The DSP_SYSTEM sub-module in general is
> described in Section 5.3.3 of the TRM [1], and it implements different
> functionalities like the PRCM handshaking, wakeup logic and DSP
> subsystem top-level configuration. It is a module present within the DSP
> processor sub-system, so can only be accessed when the sub-system is
> clocked and the appropriate reset is released.

OK so if it's specific to the DSP driver along the lines of sysc and
syss registers.

Typically we handle these registers by mapping them to the PM runtime
functions for the interconnect so we can reset and idle the hardware
modules even if there is no driver, see for example
omap54xx_mmu_dsp_hwmod.
 
> > We should use some Linux generic framework for configuring these
> > bits to avoid nasty dependencies between various hardware modules
> > on the SoC.
> > 
> > What does DSP_SYS_MMU_CONFIG register do? It seems it's probably
> > a regulator or a gate clock? If so, it should be set up as a
> > regulator or a clock and then the omap-iommu driver can just
> > use regulator or clcok framework to request the resource.
> 
> No, its neither. It is a control bit that dictates whether the
> processor/EDMA addresses go through the respective MMU or not. The
> register currently has 4 bits (bit 0 in each nibble), one each for
> enabling each MMU and requesting an MMU abort on each. The MMU
> integration and enablement notes are detailed in Section 5.3.6 of the
> TRM [1], and the DSP_SYS_MMU_CONFIG register layout is in Table 5-28
> (Page 1641).

OK yeah seems like it should be handled by the DSP driver during
probe after doing pm_runtime_get. Then the driver can configure
things like IOMMU and load firmware. Or am I missing something here?

Regards,

Tony

 
> [1] http://www.ti.com/lit/ug/spruhz6/spruhz6.pdf
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Suman Anna July 23, 2015, 4:22 p.m. UTC | #4
Hi Tony,

On 07/23/2015 02:24 AM, Tony Lindgren wrote:
> * Suman Anna <s-anna@ti.com> [150722 09:25]:
>> On 07/22/2015 12:26 AM, Tony Lindgren wrote:
>>>
>>> I don't like using syscon for tinkering directly with SoC registers.
>>
>> This is not a SoC-level register, but a register within a sub-module of
>> the DSP processor sub-system. The DSP_SYSTEM sub-module in general is
>> described in Section 5.3.3 of the TRM [1], and it implements different
>> functionalities like the PRCM handshaking, wakeup logic and DSP
>> subsystem top-level configuration. It is a module present within the DSP
>> processor sub-system, so can only be accessed when the sub-system is
>> clocked and the appropriate reset is released.
> 
> OK so if it's specific to the DSP driver along the lines of sysc and
> syss registers.

There will be those registers too within the MMU config register space,
even for DRA7xx MMUs. This is different, think of it like a register in
the Control module except that it is present within the DSP sub-system
instead of at the SoC level.

> 
> Typically we handle these registers by mapping them to the PM runtime
> functions for the interconnect so we can reset and idle the hardware
> modules even if there is no driver, see for example
> omap54xx_mmu_dsp_hwmod.

I haven't yet submitted the DRA7xx hwmods, but they will look almost
exactly like the OMAP5 ones. The reset and idle on these are in general
not effective at boot time since these are also controlled by a
hard-reset line, so that's left to the drivers to deal with it through
the omap_device_deassert_hardreset API.

>  
>>> We should use some Linux generic framework for configuring these
>>> bits to avoid nasty dependencies between various hardware modules
>>> on the SoC.
>>>
>>> What does DSP_SYS_MMU_CONFIG register do? It seems it's probably
>>> a regulator or a gate clock? If so, it should be set up as a
>>> regulator or a clock and then the omap-iommu driver can just
>>> use regulator or clcok framework to request the resource.
>>
>> No, its neither. It is a control bit that dictates whether the
>> processor/EDMA addresses go through the respective MMU or not. The
>> register currently has 4 bits (bit 0 in each nibble), one each for
>> enabling each MMU and requesting an MMU abort on each. The MMU
>> integration and enablement notes are detailed in Section 5.3.6 of the
>> TRM [1], and the DSP_SYS_MMU_CONFIG register layout is in Table 5-28
>> (Page 1641).
> 
> OK yeah seems like it should be handled by the DSP driver during
> probe after doing pm_runtime_get. Then the driver can configure
> things like IOMMU and load firmware. Or am I missing something here?

The DSP (remoteproc) driver uses the generic IOMMU API, and all the
IOMMU enabling sequence is transparent to the DSP driver. So, any
configuration/enabling sequence specific to the IOMMU has to be handled
within the respective IOMMU platform driver that gets integrated into
the IOMMU API.

regards
Suman

> 
>  
>> [1] http://www.ti.com/lit/ug/spruhz6/spruhz6.pdf

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren July 24, 2015, 4:30 a.m. UTC | #5
* Suman Anna <s-anna@ti.com> [150723 09:25]:
> Hi Tony,
> 
> On 07/23/2015 02:24 AM, Tony Lindgren wrote:
> > * Suman Anna <s-anna@ti.com> [150722 09:25]:
> >> On 07/22/2015 12:26 AM, Tony Lindgren wrote:
> >>>
> >>> I don't like using syscon for tinkering directly with SoC registers.
> >>
> >> This is not a SoC-level register, but a register within a sub-module of
> >> the DSP processor sub-system. The DSP_SYSTEM sub-module in general is
> >> described in Section 5.3.3 of the TRM [1], and it implements different
> >> functionalities like the PRCM handshaking, wakeup logic and DSP
> >> subsystem top-level configuration. It is a module present within the DSP
> >> processor sub-system, so can only be accessed when the sub-system is
> >> clocked and the appropriate reset is released.
> > 
> > OK so if it's specific to the DSP driver along the lines of sysc and
> > syss registers.
> 
> There will be those registers too within the MMU config register space,
> even for DRA7xx MMUs. This is different, think of it like a register in
> the Control module except that it is present within the DSP sub-system
> instead of at the SoC level.

And what is taking care of pm_runtime_get here to ensure the module
is powered and clocked?

I think you are missing a layer here and it's the Linux kernel side
device driver for DSP that initializes things.
 
> > Typically we handle these registers by mapping them to the PM runtime
> > functions for the interconnect so we can reset and idle the hardware
> > modules even if there is no driver, see for example
> > omap54xx_mmu_dsp_hwmod.
> 
> I haven't yet submitted the DRA7xx hwmods, but they will look almost
> exactly like the OMAP5 ones. The reset and idle on these are in general
> not effective at boot time since these are also controlled by a
> hard-reset line, so that's left to the drivers to deal with it through
> the omap_device_deassert_hardreset API.

If the MMU configuration is one time init, it may make sense to add
it to the hwmod reset function. However, if the Linux kernel side
driver needs to configure things depending on the DSP firmware, it
should be done in the kernel side device driver. 
 
> >>> We should use some Linux generic framework for configuring these
> >>> bits to avoid nasty dependencies between various hardware modules
> >>> on the SoC.
> >>>
> >>> What does DSP_SYS_MMU_CONFIG register do? It seems it's probably
> >>> a regulator or a gate clock? If so, it should be set up as a
> >>> regulator or a clock and then the omap-iommu driver can just
> >>> use regulator or clcok framework to request the resource.
> >>
> >> No, its neither. It is a control bit that dictates whether the
> >> processor/EDMA addresses go through the respective MMU or not. The
> >> register currently has 4 bits (bit 0 in each nibble), one each for
> >> enabling each MMU and requesting an MMU abort on each. The MMU
> >> integration and enablement notes are detailed in Section 5.3.6 of the
> >> TRM [1], and the DSP_SYS_MMU_CONFIG register layout is in Table 5-28
> >> (Page 1641).
> > 
> > OK yeah seems like it should be handled by the DSP driver during
> > probe after doing pm_runtime_get. Then the driver can configure
> > things like IOMMU and load firmware. Or am I missing something here?
> 
> The DSP (remoteproc) driver uses the generic IOMMU API, and all the
> IOMMU enabling sequence is transparent to the DSP driver. So, any
> configuration/enabling sequence specific to the IOMMU has to be handled
> within the respective IOMMU platform driver that gets integrated into
> the IOMMU API.

To me it seems you still need a DSP kernel driver to manage the
DSP hardware for PM runtime. I don't see how iommu and remoteproc
alone would be sufficient here. You end up sprinkling DSP driver
specific code to the iommu and remoteproc layers.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Suman Anna July 24, 2015, 4:24 p.m. UTC | #6
Hi Tony,

On 07/23/2015 11:30 PM, Tony Lindgren wrote:
> * Suman Anna <s-anna@ti.com> [150723 09:25]:
>> Hi Tony,
>>
>> On 07/23/2015 02:24 AM, Tony Lindgren wrote:
>>> * Suman Anna <s-anna@ti.com> [150722 09:25]:
>>>> On 07/22/2015 12:26 AM, Tony Lindgren wrote:
>>>>>
>>>>> I don't like using syscon for tinkering directly with SoC registers.
>>>>
>>>> This is not a SoC-level register, but a register within a sub-module of
>>>> the DSP processor sub-system. The DSP_SYSTEM sub-module in general is
>>>> described in Section 5.3.3 of the TRM [1], and it implements different
>>>> functionalities like the PRCM handshaking, wakeup logic and DSP
>>>> subsystem top-level configuration. It is a module present within the DSP
>>>> processor sub-system, so can only be accessed when the sub-system is
>>>> clocked and the appropriate reset is released.
>>>
>>> OK so if it's specific to the DSP driver along the lines of sysc and
>>> syss registers.
>>
>> There will be those registers too within the MMU config register space,
>> even for DRA7xx MMUs. This is different, think of it like a register in
>> the Control module except that it is present within the DSP sub-system
>> instead of at the SoC level.
> 
> And what is taking care of pm_runtime_get here to ensure the module
> is powered and clocked?

pm_runtime_get_sync is indeed getting invoked, its just the current
patch doesn't include the code block that invokes it. The function that
invokes omap2_iommu_enable does so after a call to the
pm_runtime_get_sync() call.

> 
> I think you are missing a layer here and it's the Linux kernel side
> device driver for DSP that initializes things.

We already have separate drivers for MMUs (omap-iommu) and the processor
management (omap-rproc). The former manages all the low-level
programming sequences for the MMUs, while the latter manages the
low-level reset etc, and is a client user of the respective IOMMU
device. Both integrate into the respective frameworks. The IOMMU API
invocations are handled in the remoteproc core, with the OMAP remoteproc
driver publishing itself as having an MMU with the remoteproc core. The
IOMMU API invoke the appropriate iommu_ops.

You can lookup the functions rproc_enable_iommu()/rproc_disable_iommu()
in the remoteproc core. The IOMMU enabling sequences happen within the
iommu_attach_device() API. The call flow is
iommu_attach_device()->omap_iommu_attach_dev()->omap_iommu_attach()->
iommu_enable()->
   omap_device_deassert_hardreset, pm_runtime_get_sync, omap2_iommu_enable.

>  
>>> Typically we handle these registers by mapping them to the PM runtime
>>> functions for the interconnect so we can reset and idle the hardware
>>> modules even if there is no driver, see for example
>>> omap54xx_mmu_dsp_hwmod.
>>
>> I haven't yet submitted the DRA7xx hwmods, but they will look almost
>> exactly like the OMAP5 ones. The reset and idle on these are in general
>> not effective at boot time since these are also controlled by a
>> hard-reset line, so that's left to the drivers to deal with it through
>> the omap_device_deassert_hardreset API.
> 
> If the MMU configuration is one time init, it may make sense to add
> it to the hwmod reset function. However, if the Linux kernel side
> driver needs to configure things depending on the DSP firmware, it
> should be done in the kernel side device driver.

The MMU configuration comes into picture whenever the remoteproc driver
is being booted and shut down, and also during suspend (no support for
this yet in mainline on OMAP rproc). Today, the hwmod
_enable/_idle/reset functions alone are not enough to power on the OMAP
remoteproc/iommus. We need sequencing calls to both
omap_device_assert/_deassert_hardreset (done through pdata-quirks) and
pm_runtime API to achieve this.


>  
>>>>> We should use some Linux generic framework for configuring these
>>>>> bits to avoid nasty dependencies between various hardware modules
>>>>> on the SoC.
>>>>>
>>>>> What does DSP_SYS_MMU_CONFIG register do? It seems it's probably
>>>>> a regulator or a gate clock? If so, it should be set up as a
>>>>> regulator or a clock and then the omap-iommu driver can just
>>>>> use regulator or clcok framework to request the resource.
>>>>
>>>> No, its neither. It is a control bit that dictates whether the
>>>> processor/EDMA addresses go through the respective MMU or not. The
>>>> register currently has 4 bits (bit 0 in each nibble), one each for
>>>> enabling each MMU and requesting an MMU abort on each. The MMU
>>>> integration and enablement notes are detailed in Section 5.3.6 of the
>>>> TRM [1], and the DSP_SYS_MMU_CONFIG register layout is in Table 5-28
>>>> (Page 1641).
>>>
>>> OK yeah seems like it should be handled by the DSP driver during
>>> probe after doing pm_runtime_get. Then the driver can configure
>>> things like IOMMU and load firmware. Or am I missing something here?
>>
>> The DSP (remoteproc) driver uses the generic IOMMU API, and all the
>> IOMMU enabling sequence is transparent to the DSP driver. So, any
>> configuration/enabling sequence specific to the IOMMU has to be handled
>> within the respective IOMMU platform driver that gets integrated into
>> the IOMMU API.
> 
> To me it seems you still need a DSP kernel driver to manage the
> DSP hardware for PM runtime. I don't see how iommu and remoteproc
> alone would be sufficient here. You end up sprinkling DSP driver
> specific code to the iommu and remoteproc layers.

I am not sure what you envision to be in this DSP kernel driver, because
all the needed functionality is already handled by existing drivers. The
OMAP remoteproc driver is the kernel driver to deal with the device
management for both IPUs and DSPs on OMAP4, OMAP5 and DRA7 (DT support
to be added).

regards
Suman

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren July 28, 2015, 6:14 a.m. UTC | #7
* Suman Anna <s-anna@ti.com> [150724 09:27]:
> Hi Tony,
> 
> On 07/23/2015 11:30 PM, Tony Lindgren wrote:
> > * Suman Anna <s-anna@ti.com> [150723 09:25]:
> >> Hi Tony,
> >>
> >> On 07/23/2015 02:24 AM, Tony Lindgren wrote:
> >>> * Suman Anna <s-anna@ti.com> [150722 09:25]:
> >>>> On 07/22/2015 12:26 AM, Tony Lindgren wrote:
> >>>>>
> >>>>> I don't like using syscon for tinkering directly with SoC registers.
> >>>>
> >>>> This is not a SoC-level register, but a register within a sub-module of
> >>>> the DSP processor sub-system. The DSP_SYSTEM sub-module in general is
> >>>> described in Section 5.3.3 of the TRM [1], and it implements different
> >>>> functionalities like the PRCM handshaking, wakeup logic and DSP
> >>>> subsystem top-level configuration. It is a module present within the DSP
> >>>> processor sub-system, so can only be accessed when the sub-system is
> >>>> clocked and the appropriate reset is released.
> >>>
> >>> OK so if it's specific to the DSP driver along the lines of sysc and
> >>> syss registers.
> >>
> >> There will be those registers too within the MMU config register space,
> >> even for DRA7xx MMUs. This is different, think of it like a register in
> >> the Control module except that it is present within the DSP sub-system
> >> instead of at the SoC level.
> > 
> > And what is taking care of pm_runtime_get here to ensure the module
> > is powered and clocked?
> 
> pm_runtime_get_sync is indeed getting invoked, its just the current
> patch doesn't include the code block that invokes it. The function that
> invokes omap2_iommu_enable does so after a call to the
> pm_runtime_get_sync() call.

OK 

> > I think you are missing a layer here and it's the Linux kernel side
> > device driver for DSP that initializes things.
> 
> We already have separate drivers for MMUs (omap-iommu) and the processor
> management (omap-rproc). The former manages all the low-level
> programming sequences for the MMUs, while the latter manages the
> low-level reset etc, and is a client user of the respective IOMMU
> device. Both integrate into the respective frameworks. The IOMMU API
> invocations are handled in the remoteproc core, with the OMAP remoteproc
> driver publishing itself as having an MMU with the remoteproc core. The
> IOMMU API invoke the appropriate iommu_ops.
> 
> You can lookup the functions rproc_enable_iommu()/rproc_disable_iommu()
> in the remoteproc core. The IOMMU enabling sequences happen within the
> iommu_attach_device() API. The call flow is
> iommu_attach_device()->omap_iommu_attach_dev()->omap_iommu_attach()->
> iommu_enable()->
>    omap_device_deassert_hardreset, pm_runtime_get_sync, omap2_iommu_enable.

OK. The thing to check here is that you have a separate device driver
for each sysc/syss device registers. This is because each hardware module
can be independently clocked and idled. Otherwise things will break at
some point. And no "things are configured for autoidle" or "we're not
doing PM is not a good excuse here" :)

Can you please check that? If the remoteproc driver and iommu driver
are tinkering with registers in separate hardware modules, we have
a layering violation. My guess is that we have at least two hardware
modules involved here, one for the iommu and one for the DSP device.
 
> >>> Typically we handle these registers by mapping them to the PM runtime
> >>> functions for the interconnect so we can reset and idle the hardware
> >>> modules even if there is no driver, see for example
> >>> omap54xx_mmu_dsp_hwmod.
> >>
> >> I haven't yet submitted the DRA7xx hwmods, but they will look almost
> >> exactly like the OMAP5 ones. The reset and idle on these are in general
> >> not effective at boot time since these are also controlled by a
> >> hard-reset line, so that's left to the drivers to deal with it through
> >> the omap_device_deassert_hardreset API.
> > 
> > If the MMU configuration is one time init, it may make sense to add
> > it to the hwmod reset function. However, if the Linux kernel side
> > driver needs to configure things depending on the DSP firmware, it
> > should be done in the kernel side device driver.
> 
> The MMU configuration comes into picture whenever the remoteproc driver
> is being booted and shut down, and also during suspend (no support for
> this yet in mainline on OMAP rproc). Today, the hwmod
> _enable/_idle/reset functions alone are not enough to power on the OMAP
> remoteproc/iommus. We need sequencing calls to both
> omap_device_assert/_deassert_hardreset (done through pdata-quirks) and
> pm_runtime API to achieve this.

Right and that's why I'm worried that we have multiple hardware modules
involved :)  
 
> >>>>> We should use some Linux generic framework for configuring these
> >>>>> bits to avoid nasty dependencies between various hardware modules
> >>>>> on the SoC.
> >>>>>
> >>>>> What does DSP_SYS_MMU_CONFIG register do? It seems it's probably
> >>>>> a regulator or a gate clock? If so, it should be set up as a
> >>>>> regulator or a clock and then the omap-iommu driver can just
> >>>>> use regulator or clcok framework to request the resource.
> >>>>
> >>>> No, its neither. It is a control bit that dictates whether the
> >>>> processor/EDMA addresses go through the respective MMU or not. The
> >>>> register currently has 4 bits (bit 0 in each nibble), one each for
> >>>> enabling each MMU and requesting an MMU abort on each. The MMU
> >>>> integration and enablement notes are detailed in Section 5.3.6 of the
> >>>> TRM [1], and the DSP_SYS_MMU_CONFIG register layout is in Table 5-28
> >>>> (Page 1641).
> >>>
> >>> OK yeah seems like it should be handled by the DSP driver during
> >>> probe after doing pm_runtime_get. Then the driver can configure
> >>> things like IOMMU and load firmware. Or am I missing something here?
> >>
> >> The DSP (remoteproc) driver uses the generic IOMMU API, and all the
> >> IOMMU enabling sequence is transparent to the DSP driver. So, any
> >> configuration/enabling sequence specific to the IOMMU has to be handled
> >> within the respective IOMMU platform driver that gets integrated into
> >> the IOMMU API.
> > 
> > To me it seems you still need a DSP kernel driver to manage the
> > DSP hardware for PM runtime. I don't see how iommu and remoteproc
> > alone would be sufficient here. You end up sprinkling DSP driver
> > specific code to the iommu and remoteproc layers.
> 
> I am not sure what you envision to be in this DSP kernel driver, because
> all the needed functionality is already handled by existing drivers. The
> OMAP remoteproc driver is the kernel driver to deal with the device
> management for both IPUs and DSPs on OMAP4, OMAP5 and DRA7 (DT support
> to be added).

OK maybe check the syss/sysc registers involved here for each hardware
module here and which driver tinkers with which registers? This will
make things a lot easier in the long run for sure.

Regrads,

Tony

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Suman Anna Sept. 3, 2015, 10:57 p.m. UTC | #8
Hi Tony,

Sorry for the long delay in getting back on this. I will repost the
series once 4.3-rc1 is out, but at the moment I do not think any changes
are required. Let me know if you still need any additional details.

>>>> On 07/23/2015 02:24 AM, Tony Lindgren wrote:
>>>>> * Suman Anna <s-anna@ti.com> [150722 09:25]:
>>>>>> On 07/22/2015 12:26 AM, Tony Lindgren wrote:
>>>>>>>
>>>>>>> I don't like using syscon for tinkering directly with SoC registers.
>>>>>>
>>>>>> This is not a SoC-level register, but a register within a sub-module of
>>>>>> the DSP processor sub-system. The DSP_SYSTEM sub-module in general is
>>>>>> described in Section 5.3.3 of the TRM [1], and it implements different
>>>>>> functionalities like the PRCM handshaking, wakeup logic and DSP
>>>>>> subsystem top-level configuration. It is a module present within the DSP
>>>>>> processor sub-system, so can only be accessed when the sub-system is
>>>>>> clocked and the appropriate reset is released.
>>>>>
>>>>> OK so if it's specific to the DSP driver along the lines of sysc and
>>>>> syss registers.
>>>>
>>>> There will be those registers too within the MMU config register space,
>>>> even for DRA7xx MMUs. This is different, think of it like a register in
>>>> the Control module except that it is present within the DSP sub-system
>>>> instead of at the SoC level.
>>>
>>> And what is taking care of pm_runtime_get here to ensure the module
>>> is powered and clocked?
>>
>> pm_runtime_get_sync is indeed getting invoked, its just the current
>> patch doesn't include the code block that invokes it. The function that
>> invokes omap2_iommu_enable does so after a call to the
>> pm_runtime_get_sync() call.
> 
> OK 
> 
>>> I think you are missing a layer here and it's the Linux kernel side
>>> device driver for DSP that initializes things.
>>
>> We already have separate drivers for MMUs (omap-iommu) and the processor
>> management (omap-rproc). The former manages all the low-level
>> programming sequences for the MMUs, while the latter manages the
>> low-level reset etc, and is a client user of the respective IOMMU
>> device. Both integrate into the respective frameworks. The IOMMU API
>> invocations are handled in the remoteproc core, with the OMAP remoteproc
>> driver publishing itself as having an MMU with the remoteproc core. The
>> IOMMU API invoke the appropriate iommu_ops.
>>
>> You can lookup the functions rproc_enable_iommu()/rproc_disable_iommu()
>> in the remoteproc core. The IOMMU enabling sequences happen within the
>> iommu_attach_device() API. The call flow is
>> iommu_attach_device()->omap_iommu_attach_dev()->omap_iommu_attach()->
>> iommu_enable()->
>>    omap_device_deassert_hardreset, pm_runtime_get_sync, omap2_iommu_enable.
> 
> OK. The thing to check here is that you have a separate device driver
> for each sysc/syss device registers. This is because each hardware module
> can be independently clocked and idled. Otherwise things will break at
> some point. And no "things are configured for autoidle" or "we're not
> doing PM is not a good excuse here" :)
> 
> Can you please check that? If the remoteproc driver and iommu driver
> are tinkering with registers in separate hardware modules, we have
> a layering violation. My guess is that we have at least two hardware
> modules involved here, one for the iommu and one for the DSP device.

Only the IOMMU has a SYSC/SYSS device register. It has both a soft-reset
as well as a hard-reset line. The IOMMU stays in reset, and its state
machine/programming is controlled through the remoteproc driver. The
clock source is also single clock going into the remote processor
subsystem, and split up internally to feed the memories and the
processors. For the PRCM to auto-gate the clocks, we also need the
processor to be executing a IDLE (DSPs) or WFI/WFE (Cortex M3/M4s). Just
the IOMMU will allow the OCP idle because of the associated sysc/syss
registers. Depending on the remote processor, there might be a separate
internal module (like the DSPs usually have a separate SYSC module), but
that programming is left to the software running on the DSP.

>  
>>>>> Typically we handle these registers by mapping them to the PM runtime
>>>>> functions for the interconnect so we can reset and idle the hardware
>>>>> modules even if there is no driver, see for example
>>>>> omap54xx_mmu_dsp_hwmod.
>>>>
>>>> I haven't yet submitted the DRA7xx hwmods, but they will look almost
>>>> exactly like the OMAP5 ones. The reset and idle on these are in general
>>>> not effective at boot time since these are also controlled by a
>>>> hard-reset line, so that's left to the drivers to deal with it through
>>>> the omap_device_deassert_hardreset API.
>>>
>>> If the MMU configuration is one time init, it may make sense to add
>>> it to the hwmod reset function. However, if the Linux kernel side
>>> driver needs to configure things depending on the DSP firmware, it
>>> should be done in the kernel side device driver.
>>
>> The MMU configuration comes into picture whenever the remoteproc driver
>> is being booted and shut down, and also during suspend (no support for
>> this yet in mainline on OMAP rproc). Today, the hwmod
>> _enable/_idle/reset functions alone are not enough to power on the OMAP
>> remoteproc/iommus. We need sequencing calls to both
>> omap_device_assert/_deassert_hardreset (done through pdata-quirks) and
>> pm_runtime API to achieve this.
> 
> Right and that's why I'm worried that we have multiple hardware modules
> involved :)  
>  
>>>>>>> We should use some Linux generic framework for configuring these
>>>>>>> bits to avoid nasty dependencies between various hardware modules
>>>>>>> on the SoC.
>>>>>>>
>>>>>>> What does DSP_SYS_MMU_CONFIG register do? It seems it's probably
>>>>>>> a regulator or a gate clock? If so, it should be set up as a
>>>>>>> regulator or a clock and then the omap-iommu driver can just
>>>>>>> use regulator or clcok framework to request the resource.
>>>>>>
>>>>>> No, its neither. It is a control bit that dictates whether the
>>>>>> processor/EDMA addresses go through the respective MMU or not. The
>>>>>> register currently has 4 bits (bit 0 in each nibble), one each for
>>>>>> enabling each MMU and requesting an MMU abort on each. The MMU
>>>>>> integration and enablement notes are detailed in Section 5.3.6 of the
>>>>>> TRM [1], and the DSP_SYS_MMU_CONFIG register layout is in Table 5-28
>>>>>> (Page 1641).
>>>>>
>>>>> OK yeah seems like it should be handled by the DSP driver during
>>>>> probe after doing pm_runtime_get. Then the driver can configure
>>>>> things like IOMMU and load firmware. Or am I missing something here?
>>>>
>>>> The DSP (remoteproc) driver uses the generic IOMMU API, and all the
>>>> IOMMU enabling sequence is transparent to the DSP driver. So, any
>>>> configuration/enabling sequence specific to the IOMMU has to be handled
>>>> within the respective IOMMU platform driver that gets integrated into
>>>> the IOMMU API.
>>>
>>> To me it seems you still need a DSP kernel driver to manage the
>>> DSP hardware for PM runtime. I don't see how iommu and remoteproc
>>> alone would be sufficient here. You end up sprinkling DSP driver
>>> specific code to the iommu and remoteproc layers.
>>
>> I am not sure what you envision to be in this DSP kernel driver, because
>> all the needed functionality is already handled by existing drivers. The
>> OMAP remoteproc driver is the kernel driver to deal with the device
>> management for both IPUs and DSPs on OMAP4, OMAP5 and DRA7 (DT support
>> to be added).
> 
> OK maybe check the syss/sysc registers involved here for each hardware
> module here and which driver tinkers with which registers? This will
> make things a lot easier in the long run for sure.

The OMAP remoteproc driver typically deals with only the reset registers
and another register in the Control module for programming the boot
address for the DSPs. You can look up the OMAP4 hwmod data for
reference. There is currently a .prcm.omap4.modulemode set for these,
and I have a patch to remove those. I haven't yet posted it yet [1],
because OMAP remoteprocs are still not bootable on mainline.

regards
Suman

[1]
http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=0dd2a6a2d8869ee6cec92f75d70663259a41c11f
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren Sept. 23, 2015, 5:53 p.m. UTC | #9
* Suman Anna <s-anna@ti.com> [150903 16:01]:
> >>>> On 07/23/2015 02:24 AM, Tony Lindgren wrote:
> > OK maybe check the syss/sysc registers involved here for each hardware
> > module here and which driver tinkers with which registers? This will
> > make things a lot easier in the long run for sure.
> 
> The OMAP remoteproc driver typically deals with only the reset registers
> and another register in the Control module for programming the boot
> address for the DSPs. You can look up the OMAP4 hwmod data for
> reference. There is currently a .prcm.omap4.modulemode set for these,
> and I have a patch to remove those. I haven't yet posted it yet [1],
> because OMAP remoteprocs are still not bootable on mainline.

Thanks for checking that. Like we chatted, let's make sure we don't
end up with iommu and remoteproc "consumer device" specific data
in the iommu and remoteproc code.

Regards,

Tony

> [1]
> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=0dd2a6a2d8869ee6cec92f75d70663259a41c11f
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index fe742c01a4f2..0849a5927732 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -26,6 +26,8 @@ 
 #include <linux/of_iommu.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
 
 #include <asm/cacheflush.h>
 
@@ -112,6 +114,18 @@  void omap_iommu_restore_ctx(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(omap_iommu_restore_ctx);
 
+static void dra7_cfg_dspsys_mmu(struct omap_iommu *obj, bool enable)
+{
+	u32 val, mask;
+
+	if (!obj->syscfg)
+		return;
+
+	mask = (1 << (obj->id * DSP_SYS_MMU_CONFIG_EN_SHIFT));
+	val = enable ? mask : 0;
+	regmap_update_bits(obj->syscfg, DSP_SYS_MMU_CONFIG, mask, val);
+}
+
 static void __iommu_set_twl(struct omap_iommu *obj, bool on)
 {
 	u32 l = iommu_read_reg(obj, MMU_CNTL);
@@ -147,6 +161,8 @@  static int omap2_iommu_enable(struct omap_iommu *obj)
 
 	iommu_write_reg(obj, pa, MMU_TTB);
 
+	dra7_cfg_dspsys_mmu(obj, true);
+
 	if (obj->has_bus_err_back)
 		iommu_write_reg(obj, MMU_GP_REG_BUS_ERR_BACK_EN, MMU_GP_REG);
 
@@ -161,6 +177,7 @@  static void omap2_iommu_disable(struct omap_iommu *obj)
 
 	l &= ~MMU_CNTL_MASK;
 	iommu_write_reg(obj, l, MMU_CNTL);
+	dra7_cfg_dspsys_mmu(obj, false);
 
 	dev_dbg(obj->dev, "%s is shutting down\n", obj->name);
 }
@@ -864,6 +881,42 @@  static void omap_iommu_detach(struct omap_iommu *obj)
 	dev_dbg(obj->dev, "%s: %s\n", __func__, obj->name);
 }
 
+static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
+					      struct omap_iommu *obj)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	if (!of_device_is_compatible(np, "ti,dra7-dsp-iommu"))
+		return 0;
+
+	if (!of_property_read_bool(np, "ti,syscon-mmuconfig")) {
+		dev_err(&pdev->dev, "ti,syscon-mmuconfig property is missing\n");
+		return -EINVAL;
+	}
+
+	obj->syscfg =
+		syscon_regmap_lookup_by_phandle(np, "ti,syscon-mmuconfig");
+	if (IS_ERR(obj->syscfg)) {
+		/* can fail with -EPROBE_DEFER */
+		ret = PTR_ERR(obj->syscfg);
+		return ret;
+	}
+
+	if (of_property_read_u32_index(np, "ti,syscon-mmuconfig", 1,
+				       &obj->id)) {
+		dev_err(&pdev->dev, "couldn't get the IOMMU instance id within subsystem\n");
+		return -EINVAL;
+	}
+
+	if (obj->id != 0 && obj->id != 1) {
+		dev_err(&pdev->dev, "invalid IOMMU instance id\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  *	OMAP Device MMU(IOMMU) detection
  */
@@ -907,6 +960,10 @@  static int omap_iommu_probe(struct platform_device *pdev)
 	if (IS_ERR(obj->regbase))
 		return PTR_ERR(obj->regbase);
 
+	err = omap_iommu_dra7_get_dsp_system_cfg(pdev, obj);
+	if (err)
+		return err;
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return -ENODEV;
@@ -943,6 +1000,7 @@  static const struct of_device_id omap_iommu_of_match[] = {
 	{ .compatible = "ti,omap2-iommu" },
 	{ .compatible = "ti,omap4-iommu" },
 	{ .compatible = "ti,dra7-iommu"	},
+	{ .compatible = "ti,dra7-dsp-iommu" },
 	{},
 };
 
diff --git a/drivers/iommu/omap-iommu.h b/drivers/iommu/omap-iommu.h
index a656df2f9e03..59628e5017b4 100644
--- a/drivers/iommu/omap-iommu.h
+++ b/drivers/iommu/omap-iommu.h
@@ -30,6 +30,7 @@  struct iotlb_entry {
 struct omap_iommu {
 	const char	*name;
 	void __iomem	*regbase;
+	struct regmap	*syscfg;
 	struct device	*dev;
 	struct iommu_domain *domain;
 	struct dentry	*debug_dir;
@@ -48,6 +49,7 @@  struct omap_iommu {
 	void *ctx; /* iommu context: registres saved area */
 
 	int has_bus_err_back;
+	u32 id;
 };
 
 struct cr_regs {
@@ -159,6 +161,13 @@  static inline struct omap_iommu *dev_to_omap_iommu(struct device *dev)
 	 ((pgsz) == MMU_CAM_PGSZ_4K)  ? 0xfffff000 : 0)
 
 /*
+ * DSP_SYSTEM registers and bit definitions (applicable only for DRA7xx DSP)
+ */
+#define DSP_SYS_REVISION		0x00
+#define DSP_SYS_MMU_CONFIG		0x18
+#define DSP_SYS_MMU_CONFIG_EN_SHIFT	4
+
+/*
  * utilities for super page(16MB, 1MB, 64KB and 4KB)
  */