[EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings

[Krzysztof Kozlowski]

On 17/03/2023 15:55, Frank Li wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
>> Sent: Friday, March 17, 2023 4:09 AM
>> To: Frank Li <frank.li at nxp.com>; shawnguo at kernel.org
>> Cc: devicetree at vger.kernel.org; festevam at gmail.com; imx at lists.linux.dev;
>> kernel at pengutronix.de; krzysztof.kozlowski+dt at linaro.org; linux-arm-
>> kernel at lists.infradead.org; dl-linux-imx <linux-imx at nxp.com>; linux-
>> kernel at vger.kernel.org; robh+dt at kernel.org; s.hauer at pengutronix.de
>> Subject: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add
>> imx8qm cdns3 glue bindings
>>
>> Caution: EXT Email
>>
>> On 16/03/2023 22:27, Frank Li wrote:
>>> NXP imx8qm integrates 1 cdns3 IP. This is glue layer device bindings.
>>>
>>
>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>> prefix is already stating that these are bindings.
>>
>>> Signed-off-by: Frank Li <Frank.Li at nxp.com>
>>> ---
>>>  .../bindings/usb/fsl,imx8qm-cdns3.yaml        | 122 ++++++++++++++++++
>>>  1 file changed, 122 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/usb/fsl,imx8qm-
>> cdns3.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/fsl,imx8qm-
>> cdns3.yaml b/Documentation/devicetree/bindings/usb/fsl,imx8qm-
>> cdns3.yaml
>>> new file mode 100644
>>> index 000000000000..fc24df1e4483
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml
>>> @@ -0,0 +1,122 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +# Copyright (c) 2020 NXP
>>> +%YAML 1.2
>>> +---
>>> +$id:
>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevice
>> tree.org%2Fschemas%2Fusb%2Ffsl%2Cimx8qm-
>> cdns3.yaml%23&data=05%7C01%7CFrank.Li%40nxp.com%7Cac9af3d617dc4cf
>> 14baf08db26c74b07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
>> 38146409617970248%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
>> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C
>> &sdata=EczZhjyMUGnnp7ZGfSvTj4lmOUuGlOtWYIsxxXIlNXw%3D&reserved
>> =0
>>> +$schema:
>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevice
>> tree.org%2Fmeta-
>> schemas%2Fcore.yaml%23&data=05%7C01%7CFrank.Li%40nxp.com%7Cac9a
>> f3d617dc4cf14baf08db26c74b07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
>> 0%7C0%7C638146409617970248%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM
>> C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
>> %7C%7C%7C&sdata=uTNYuDm%2ByhZ56oQET2pX8sHpEqVvsUYtmOBCPXEK
>> v40%3D&reserved=0
>>> +
>>> +title: NXP iMX8QM Soc USB Controller
>>> +
>>> +maintainers:
>>> +  - Frank Li <Frank.Li at nxp.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: fsl,imx8qm-usb3
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: Address and length of the register set for iMX USB3
>> Platform Control
>>
>> Drop "Address and length of the"... or actually just maxItems: 1,
>> because the description is a bit obvious.
>>
>>> +
>>> +  "#address-cells":
>>> +    enum: [ 1, 2 ]
>>> +
>>> +  "#size-cells":
>>> +    enum: [ 1, 2 ]
>>> +
>>> +  ranges: true
>>> +
>>> +  clocks:
>>> +    description:
>>> +      A list of phandle and clock-specifier pairs for the clocks
>>> +      listed in clock-names.
>>
>> Drop description.
>>
>>> +    items:
>>> +      - description: Standby clock. Used during ultra low power states.
>>> +      - description: USB bus clock for usb3 controller.
>>> +      - description: AXI clock for AXI interface.
>>> +      - description: ipg clock for register access.
>>> +      - description: Core clock for usb3 controller.
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: usb3_lpm_clk
>>> +      - const: usb3_bus_clk
>>> +      - const: usb3_aclk
>>> +      - const: usb3_ipg_clk
>>> +      - const: usb3_core_pclk
>>> +
>>> +  assigned-clocks:
>>> +    items:
>>> +      - description: Phandle and clock specifier of IMX_SC_PM_CLK_PER.
>>> +      - description: Phandle and clock specifoer of IMX_SC_PM_CLK_MISC.
>>> +      - description: Phandle and clock specifoer of
>> IMX_SC_PM_CLK_MST_BUS.
>>> +
>>> +  assigned-clock-rates:
>>> +    items:
>>> +      - description: Must be 125 Mhz.
>>> +      - description: Must be 12 Mhz.
>>> +      - description: Must be 250 Mhz.
>>
>> I would argue that both properties above are not needed. If your
>> hardware requires fixed frequencies, clock provider can fix them, can't it?
> 
> Clock provider don't know fixed value and turn on only used by client.

So maybe fix the clock provider? Or this device driver? Requiring by
binding specific frequencies for every board is a bit redundant.

Best regards,
Krzysztof