Re: [PATCH 08/17] clk: imx: convert to devm_platform_ioremap_resource

[Leonard Crestez <leonard.crestez@nxp.com>]

On 11.12.2019 19:51, Paul Walmsley wrote:
> On Tue, 10 Dec 2019, Thierry Reding wrote:
>> On Mon, Dec 09, 2019 at 08:44:39PM +0000, Leonard Crestez wrote:
>>
>>> This breaks imx8qxp-mek boot by causing most peripherals (like uart) to
>>> fail to probe.
>>>
>>> The old and new paths are not equivalent: devm_platform_ioremap_resource
>>> calls devm_ioremap_resource which differs from devm_ioremap by also
>>> calling devm_request_mem_region.
>>>
>>> This prevents other mappings in the area and imx8qxp-lpcg nodes map
>>> whole hardware "subsystems" and overlap most peripherals. For example:
>>>
>>>                    adma_lpcg: clock-controller@59000000 {
>>>                            compatible = "fsl,imx8qxp-lpcg-adma";
>>>                            reg = <0x59000000 0x2000000>;
>>>                            #clock-cells = <1>;
>>> 		};
>>>
>>>                   adma_lpuart0: serial@5a060000 {
>>> 			reg = <0x5a060000 0x1000>;
>>> 			...
>>> 		};
>>
>> The whole point of doing a request_mem_region() is to avoid having
>> multiple drivers trample on each others' mappings. What you do above
>> doesn't look right. Why does that clock controller need access to 32
>> MiB of I/O memory space?
> 
> Thierry's right; your DT data looks broken.  Unfortunately the IMX8M TRM
> requires some sort of signup to download, so I can't easily read it, but
> based on files like this:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fclk%2Fimx%2Fclk-imx8qxp-lpcg.h&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7Cf0e68024888e4aed777e08d77e62c5df%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637116835018036168&amp;sdata=z70QUYwknXfvc4zhyOrT7dcLPq5eryv4U7HoGMzI4ZE%3D&amp;reserved=0
> 
> it looks like what should be in the DT instead is either a large set of
> smaller register ranges, or a set of separate DT nodes/drivers: one per
> peripheral LPCG instance.  That should avoid the overlapping memory range
> issue.

Yes, current imx8 clock DT bindings are definitely odd. I only objected 
to the cleanup because it breaks boot.

This series converts imx8 LPCG to use separate DT nodes for each 
peripheral LPCG instance and makes other improvements, it's just been 
stuck in review:

https://patchwork.kernel.org/cover/11248249/

Having a large number of clock-providing DT nodes is also odd but it 
closely aligns with SOC hardware architecture. The clock gating bits 
were deliberately split into many 64k areas so that access can be 
controlled for each peripheral using very coarse-grained access control 
methods.

For example hypervisors can control guest access on page boundaries but 
not to individual bits in a byte.

This design makes it easy to securely grant control for a peripheral and 
all of its clock bits to an M4 core or virtualization guest without 
additional layers of software indirection.

--
Regards,
Leonard