Re: [PATCH 3/6] ARM: OMAP4: hwmod data: add mmu hwmod for ipu and dsp

["Cousson, Benoit" <b-cousson@ti.com>]

On 6/19/2012 6:39 PM, Omar Ramirez Luna wrote:
> Hi Benoit,
>
> On 19 June 2012 07:36, Cousson, Benoit <b-cousson@ti.com> wrote:
>> On 6/16/2012 3:56 AM, Omar Ramirez Luna wrote:
> ...
>>> +static struct omap_hwmod omap44xx_ipu_mmu_hwmod = {
>>> +     .name           = "ipu_mmu",
>>> +     .class          = &omap44xx_mmu_hwmod_class,
>>> +     .clkdm_name     = "ducati_clkdm",
>>> +     .mpu_irqs       = omap44xx_ipu_mmu_irqs,
>>> +     .rst_lines      = omap44xx_ipu_mmu_resets,
>>> +     .rst_lines_cnt  = ARRAY_SIZE(omap44xx_ipu_mmu_resets),
>>> +     .main_clk       = "ipu_fck",
>>> +     .prcm = {
>>> +             .omap4 = {
>>> +                     .clkctrl_offs = OMAP4_CM_DUCATI_DUCATI_CLKCTRL_OFFSET,
>>> +                     .rstctrl_offs = OMAP4_RM_DUCATI_RSTCTRL_OFFSET,
>>> +                     .context_offs = OMAP4_RM_DUCATI_DUCATI_CONTEXT_OFFSET,
>>> +                     .modulemode   = MODULEMODE_HWCTRL,
>>> +             },
>>> +     },
>>> +     .dev_attr       = &ipu_mmu_dev_attr,
>>> +};
>>
>> In fact, the MMU_IPU hwmod is now almost the same one than the previous IPU one...
>> If we do that we should maybe just rename the IPU -> MMU_IPU and DSP -> MMU_DSP.
>>
>> But by doing that we will assume that the MMU does represent the subsystem, which is not necessarily super nice.
>>
>> I guess that a much better representation will be to keep the subsystem (IPU) to handle the PRCM part:
>>
>> +       .main_clk       = "ipu_fck",
>> +       .prcm = {
>> +               .clkctrl_offs = OMAP4_CM_DUCATI_DUCATI_CLKCTRL_OFFSET,
>> +               .rstctrl_offs = OMAP4_RM_DUCATI_RSTCTRL_OFFSET,
>> +               .context_offs = OMAP4_RM_DUCATI_DUCATI_CONTEXT_OFFSET,
>> +               .modulemode   = MODULEMODE_HWCTRL,
>>
>> And then the MMU_IPU will handle the configuration registers part and the reset + irq.
>>
>> But then, you will have to create a parent child dependency between your devices to ensure that the IPU subsystem device will be enabled before trying to access the MMU_IPU.
>>
>> This is what the DSS is about to do to handle the same kind of power dependency. The DSS device is the parent of all the DSS IPs (DISPC, HDMI...) and thus pm_runtime will ensure that the parent is enabled before trying to enable the children.
>>
>> In term of DT, just to illustrate the situation, it will be something like that:
>>
>> ipu {
>>         compatible = "simple-bus";
>>         ti,hwmods = "ipu";
>>
>>         ipu_mmu: mmu@4a066000 {
>>                 compatible = "omap-mmu";
>>                 ti,hwmods = "mmu_ipu";
>>                 reg...;
>>                 irqs...;
>>         }
>> }
>>
>> Is it something you can handle with your current framework?
>
> I agree it would be nice only IPU managed the prcm, however we can't
> do that right now because hwmod expects the IP block to be out of
> reset to continue the _enable sequence. On IPU both reset lines are
> asserted at that point and hence _are_any_hardreset_lines_asserted
> check will bail out, and ipu resets can't be lifted without a
> configured iommu otherwise it might execute random garbage.

That's a good point, but like Paul said, the hard reset was removed 
outside of the fmwk due to the lack of understanding about the real 
usage / need for it.

If we do have a better understanding, we might add some more support in 
the fmwk or at least improve it.

I'm now realizing that aborting the init if some reset lines are 
asserted is not what we want to do for the IPU SS hwmod that will 
contain the IPU (processor) reset control.
In fact the previous approach with a fake hwmod for the ipu_c0 processor 
would have been avoided that issue.

If we do not want to go back with that, we should clearly revise the 
_are_any_hardreset_lines_asserted approach and not prevent the init in 
such case since it will prevent all the subsystem to start properly.

> So, for IPU and DSP the mmu must be deasserted and configured before
> the processor reset line (which is more like a parking brake) is
> deasserted, and the latter should be made before _enable is called so
> it can fully execute the enable sequence.

Yep, so we have to change the way it is handled today.


Paul,

If we consider that the reset lines are stored in the subsystem hwmod 
(IPU, DSP or IVA), we cannot prevent the init phase because some line 
are asserted. Otherwise we will never allow the MMU or processor to be 
enabled later.
We might have to remove that check, maybe based on flag if we want to 
keep that functionality or do an explicit reset control like we use to do.

What do you think?

Regards,
Benoit