Re: [PATCH v4 10/18] soc: imx: add i.MX8M blk-ctrl driver

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

Le 05/10/2021 à 12:03, Lucas Stach a écrit :
> Hi Benjamin,
>
> Am Montag, dem 04.10.2021 um 16:27 +0200 schrieb Benjamin Gaignard:
>> Le 02/10/2021 à 03:07, Lucas Stach a écrit :
>>> Hi Benjamin,
>>>
>>> Am Dienstag, dem 14.09.2021 um 17:46 +0200 schrieb Benjamin Gaignard:
>>>> Le 10/09/2021 à 22:26, Lucas Stach a écrit :
>>>>> This adds a driver for the blk-ctrl blocks found in the i.MX8M* line of
>>>>> SoCs. The blk-ctrl is a top-level peripheral located in the various *MIX
>>>>> power domains and interacts with the GPC power controller to provide the
>>>>> peripherals in the power domain access to the NoC and ensures that those
>>>>> peripherals are properly reset when their respective power domain is
>>>>> brought back to life.
>>>>>
>>>>> Software needs to do different things to make the bus handshake happen
>>>>> after the GPC *MIX domain is powered up and before it is powered down.
>>>>> As the requirements are quite different between the various blk-ctrls
>>>>> there is a callback function provided to hook in the proper sequence.
>>>>>
>>>>> The peripheral domains are quite uniform, they handle the soft clock
>>>>> enables and resets in the blk-ctrl address space and sequencing with the
>>>>> upstream GPC power domains.
>>>> Hi Lucas,
>>>>
>>>> I have tried to use your patches for IMX8MQ but it seems that the hardware
>>>> have different architecture.
>>>> On IMX8MQ there is only one VPU domain for G1 and G2 and that doesn't match
>>>> with your implementation where it is needed to have "bus" and devices power domain.
>>>>    From what I experiment in current IMX8MQ implementation of blk-ctrl (inside VPU driver)
>>>> enabling the 3 clocks (bus, G1, G2) is needed to reset the VPUs.
>>>>
>>>> Do you think you can update your design to take care of these hardware variations ?
>>> The clocking/reset of the blk-ctrl and ADB in the i.MX8MQ VPU power
>>> domain is really a bit strange, as the ADB reset is tied to the VPU
>>> resets and the clk-ctrl seem to require all 3 VPU clocks, instead of
>>> only the bus clock as in newer designs. However I was able to make it
>>> work with the existing blk-ctrl driver design.
>>>
>>> My current WIP patches (only tested with the G1 core so far) on top of
>>> the v5 of the series I just sent out can be found here:
>>> https://git.pengutronix.de/cgit/lst/linux/log/?h=imx8mq-vpu-blk-ctrl
>>>
>>> Hope this helps.
>> Hi Lucas,
>>
>> I have been able to test your branch on my iMX8MQ.
>> I confirm that G1 is working fine, I able to decode H264 files.
>>
>> I wasn't able to make G2 works, I think it is coming from the reset sequence
>> done before each frame decoding in G2 driver.
>> I have change imx8mq_runtime_resume() and  imx8m_vpu_reset()
>> to call pm_runtime_put() and pm_runtime_get() to perform a reset like.
> I think you need to use the _sync variants of those functions to make
> sure the domain is going through the reset state. However that seems
> like a pretty heavy-weight thing to do if the decoder really requires a
> reset before each frame. Excuse my ignorance about the G2 block, but
> this sounds like a quite odd requirement. Is this to work around some
> hardware erratum?

When using _sync variants kernel stop when probing G2.
I don't have any documentation about the link between the control block and
G2. I have done lot of tests and resetting the hardware block between each frame
seems to be mandatory. That also the case in Hantro proprietary stack.

>
> If we really need to reset the G2 before each frame, I think it would
> be best to also expose a reset controller from the blk-ctrl driver, to
> allow the G2 driver to do a light-weight reset, instead of doing this
> runtime PM transition.

I do agree and I have already done a attempt in this way:
https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=440031&state=%2A&archive=both
but Philipp have fairly argue that clocks enabling is not the job of a reset driver.

Thanks for the time you spend on this topic.

Regards,
Benjamin

>
> Regards,
> Lucas
>
>> Without that G2 hangs when decoding the first frame.
>>
>> One G1 it seems that doing a reset before each frame decoding is not needed.
>>
>> On DT I had to assignee G1 and G2 on the both nodes to avoid a warning at probe time.
>> assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>,
>> 					  <&clk IMX8MQ_CLK_VPU_G2>,
>> 					  <&clk IMX8MQ_VPU_PLL_BYPASS>;
>> 			assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>,
>> 						 <&clk IMX8MQ_VPU_PLL_OUT>,
>> 						 <&clk IMX8MQ_VPU_PLL>;
>> 			assigned-clock-rates = <600000000>, <300000000>, <0>;
>>
>> I also set G2 clock at 300Mhz as specify in the TRM.
>> Even with all this G2 doesn't fire interrupts.
>>
>> Benjamin
>>
>>> Regards,
>>> Lucas
>>>
>