Hi Paul,
On 27 June 2012 20:27, Omar Ramirez Luna <omar.luna@xxxxxxxxxx> wrote:
> On 19 June 2012 12:48, Cousson, Benoit <b-cousson@xxxxxx> wrote:
>> On 6/19/2012 6:39 PM, Omar Ramirez Luna wrote:
>>>
>>> Hi Benoit,
>>>
>>> On 19 June 2012 07:36, Cousson, Benoit <b-cousson@xxxxxx> 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?
>
> 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@xxxxxxxxxxxxxxx/msg70443.html
Thanks,
Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
