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

[Omar Ramirez Luna]

Hi Paul,

On 27 June 2012 20:27, Omar Ramirez Luna <omar.luna at linaro.org> wrote:
> On 19 June 2012 12:48, Cousson, Benoit <b-cousson at ti.com> wrote:
>> On 6/19/2012 6:39 PM, Omar Ramirez Luna wrote:
>>>
>>> Hi Benoit,
>>>
>>> On 19 June 2012 07:36, Cousson, Benoit <b-cousson at 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 at 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?
>
> I was waiting for some comments then I realized Paul wasn't actually
> in the list of recipients for this email.

Any chance you had time to check on this? While at it, could you please check:

http://www.mail-archive.com/linux-omap@vger.kernel.org/msg70443.html

Thanks,

Omar