Re: [PATCH v1 5/5] riscv: dts: microchip: add a device tree for Discovery Kit

[<Valentina.FernandezAlanis@microchip.com>]

On 02/09/2025 07:22, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 01/09/2025 17:28, Valentina.FernandezAlanis@microchip.com wrote:
>> On 28/08/2025 18:46, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 25/08/2025 18:19, Valentina Fernandez wrote:
>>>> +++ b/arch/riscv/boot/dts/microchip/mpfs-disco-kit-fabric.dtsi
>>>> @@ -0,0 +1,58 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>>> +/* Copyright (c) 2020-2025 Microchip Technology Inc */
>>>> +
>>>> +/ {
>>>> +     core_pwm0: pwm@40000000 {
>>>> +             compatible = "microchip,corepwm-rtl-v4";
>>>> +             reg = <0x0 0x40000000 0x0 0xF0>;
>>>> +             microchip,sync-update-mask = /bits/ 32 <0>;
>>>> +             #pwm-cells = <3>;
>>>> +             clocks = <&ccc_sw CLK_CCC_PLL0_OUT3>;
>>>> +             status = "disabled";
>>>> +     };
>>>> +
>>>> +     i2c2: i2c@40000200 {
>>>> +             compatible = "microchip,corei2c-rtl-v7";
>>>> +             reg = <0x0 0x40000200 0x0 0x100>;
>>>> +             #address-cells = <1>;
>>>> +             #size-cells = <0>;
>>>> +             clocks = <&ccc_sw CLK_CCC_PLL0_OUT3>;
>>>> +             interrupt-parent = <&plic>;
>>>> +             interrupts = <122>;
>>>> +             clock-frequency = <100000>;
>>>> +             status = "disabled";
>>>> +     };
>>>> +
>>>> +     ihc: mailbox {
>>>> +             compatible = "microchip,sbi-ipc";
>>>> +             interrupt-parent = <&plic>;
>>>> +             interrupts = <180>, <179>, <178>, <177>;
>>>> +             interrupt-names = "hart-1", "hart-2", "hart-3", "hart-4";
>>>> +             #mbox-cells = <1>;
>>>> +             status = "disabled";
>>>> +     };
>>>> +
>>>> +     mailbox@50000000 {
>>>> +             compatible = "microchip,miv-ihc-rtl-v2";
>>>> +             microchip,ihc-chan-disabled-mask = /bits/ 16 <0>;
>>>
>>> Does not look like following DTS coding style - order of properties.
>>>
>>>> +             reg = <0x0 0x50000000 0x0 0x1c000>;
>>>> +             interrupt-parent = <&plic>;
>>>> +             interrupts = <180>, <179>, <178>, <177>;
>>>> +             interrupt-names = "hart-1", "hart-2", "hart-3", "hart-4";
>>>> +             #mbox-cells = <1>;
>>>> +             status = "disabled";
>>>> +     };
>>>> +
>>>> +     refclk_ccc: cccrefclk {
>>>
>>> Please use name for all fixed clocks which matches current format
>>> recommendation: 'clock-<freq>' (see also the pattern in the binding for
>>> any other options).
>>>
>>> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml
>> The fabric dtsi describes elements configured by the FPGA bitstream.
>> This node is named as such because the Clock Conditioner Circuit CCC's
>> reference clock source is set by the FPGA bitstream, while its frequency
>> is determined by an on-board oscillator.
>>
>> Hope this clarifies the rationale behind the node name.
> No, because there is no style naming clocks like this. Neither proper
> suffix, nor prefix. Use standard naming.
> 
> And all other comments you ignored?
I sent a v2 with the rest of the comments addressed. I didn't notice you 
were still not happy with the clock node name, please ignore the v2.

> 
> Best regards,
> Krzysztof