| Message ID | 20231228125805.661725-2-tudor.ambarus@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | GS101 Oriole: CMU_PERIC0 support and USI updates | expand |
Hi Tudor, On Thu, 28 Dec 2023 at 12:58, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0) > clock management unit. > > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > v2: > - fix comments as per Sam's suggestion and collect his R-b tag > - Rob's suggestion of renaming the clock-names to just "bus" and "ip" > was not implemented as I felt it affects readability in the driver > and consistency with other exynos clock drivers. I will happily update > the names in the -rc phase if someone else has a stronger opinion than > mine. > It would be good to get Krzysztof and Robs view on whether they agree with the above rationale or whether they would still like to see the names updated. Personally I like the consistency, grepability and the fact the current name encodes whether it is a gate, divider into the name. Seeing 'sss' or 'ip' as a clock name in the driver code doesn't tell you a lot without having to then cross reference with the dts. Is there some rationale and/or benefit behind having the shorter names? The only thing I could think of is trying to partially re-use this file on future SoCs like gs201 which might be clocked differently, but then these exynos clock drivers seem to be SoC specific anyway. Anyways apart from that: Reviewed-by: Peter Griffin <peter.griffin@linaro.org> kind regards, Peter
On Thu, Dec 28, 2023 at 12:57:54PM +0000, Tudor Ambarus wrote: > Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0) > clock management unit. > > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > v2: > - fix comments as per Sam's suggestion and collect his R-b tag > - Rob's suggestion of renaming the clock-names to just "bus" and "ip" > was not implemented as I felt it affects readability in the driver > and consistency with other exynos clock drivers. I will happily update > the names in the -rc phase if someone else has a stronger opinion than > mine. I'll defer to Krzysztof. Rob
On Mon, Jan 08, 2024 at 02:18:21PM +0000, Peter Griffin wrote: > Hi Tudor, > > On Thu, 28 Dec 2023 at 12:58, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > > > Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0) > > clock management unit. > > > > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > > --- > > v2: > > - fix comments as per Sam's suggestion and collect his R-b tag > > - Rob's suggestion of renaming the clock-names to just "bus" and "ip" > > was not implemented as I felt it affects readability in the driver > > and consistency with other exynos clock drivers. I will happily update > > the names in the -rc phase if someone else has a stronger opinion than > > mine. > > > > It would be good to get Krzysztof and Robs view on whether they agree > with the above rationale or whether they would still like to see the > names updated. > > Personally I like the consistency, grepability and the fact the > current name encodes whether it is a gate, divider into the name. > Seeing 'sss' or 'ip' as a clock name in the driver code doesn't tell > you a lot without having to then cross reference with the dts. > > Is there some rationale and/or benefit behind having the shorter > names? The only thing I could think of is trying to partially re-use > this file on future SoCs like gs201 which might be clocked > differently, but then these exynos clock drivers seem to be SoC > specific anyway. The point of -names is to identify one entry from another in the list. Having the name of the block is just redundant. I like consistency, but not when it's a pattern we don't want. Rob
On 09/01/2024 05:03, Rob Herring wrote: > On Thu, Dec 28, 2023 at 12:57:54PM +0000, Tudor Ambarus wrote: >> Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0) >> clock management unit. >> >> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> v2: >> - fix comments as per Sam's suggestion and collect his R-b tag >> - Rob's suggestion of renaming the clock-names to just "bus" and "ip" >> was not implemented as I felt it affects readability in the driver >> and consistency with other exynos clock drivers. I will happily update >> the names in the -rc phase if someone else has a stronger opinion than >> mine. > > I'll defer to Krzysztof. I miss the point why clock-names cannot be fixed now. This is the name of property, not the input clock name. Best regards, Krzysztof
On 09/01/2024 17:12, Tudor Ambarus wrote: > > > On 1/9/24 15:01, Krzysztof Kozlowski wrote: >> On 09/01/2024 12:58, Tudor Ambarus wrote: >>> >>> >>> On 1/9/24 11:09, Krzysztof Kozlowski wrote: >>>> On 09/01/2024 05:03, Rob Herring wrote: >>>>> On Thu, Dec 28, 2023 at 12:57:54PM +0000, Tudor Ambarus wrote: >>>>>> Add dt-schema documentation for the Connectivity Peripheral 0 (PERIC0) >>>>>> clock management unit. >>>>>> >>>>>> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> >>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >>>>>> --- >>>>>> v2: >>>>>> - fix comments as per Sam's suggestion and collect his R-b tag >>>>>> - Rob's suggestion of renaming the clock-names to just "bus" and "ip" >>>>>> was not implemented as I felt it affects readability in the driver >>>>>> and consistency with other exynos clock drivers. I will happily update >>>>>> the names in the -rc phase if someone else has a stronger opinion than >>>>>> mine. >>>>> >>>>> I'll defer to Krzysztof. >>>> >>>> I miss the point why clock-names cannot be fixed now. This is the name >>>> of property, not the input clock name. >>> >>> They can be fixed now. I've just aired the fixes at: >>> https://lore.kernel.org/linux-arm-kernel/20240109114908.3623645-1-tudor.ambarus@linaro.org/ >>> >>> Preparing v3 for this patch set to include the updated names here too. >> >> I think I was not that clear enough. I did not get your current patchset >> - so PERIC0 clock controller - cannot use new naming. >> > > Ok, I understand that the fixes from > https://lore.kernel.org/linux-arm-kernel/20240109114908.3623645-1-tudor.ambarus@linaro.org/ > > are NACK-ed and I shall use the full clock-names in this patch set as > well, thus "dout_cmu_peric0_bus", and "dout_cmu_peric0_ip". I don't mind > changing them back, will send a v4 using the full clock names. They are not rejected, it is just independent thing. At least looks like independent. > Out of curiosity, why can't we change the names? All gs101 patches are > for v6.8, thus they haven't made a release yet. We still have the -rc > phase where we can fix things. We can change. I would not bother that much with doing that, because I sent already them to arm-soc. That means I need to consider this as fixes and I just did not want to deal with it. The question is quite different for a new clock controller - peric0. What parts of driver readability is affected by using "bus" name? Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml index 3eebc03a309b..ba54c13c55bc 100644 --- a/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml +++ b/Documentation/devicetree/bindings/clock/google,gs101-clock.yaml @@ -30,14 +30,15 @@ properties: - google,gs101-cmu-top - google,gs101-cmu-apm - google,gs101-cmu-misc + - google,gs101-cmu-peric0 clocks: minItems: 1 - maxItems: 2 + maxItems: 3 clock-names: minItems: 1 - maxItems: 2 + maxItems: 3 "#clock-cells": const: 1 @@ -88,6 +89,26 @@ allOf: - const: dout_cmu_misc_bus - const: dout_cmu_misc_sss + - if: + properties: + compatible: + contains: + const: google,gs101-cmu-peric0 + + then: + properties: + clocks: + items: + - description: External reference clock (24.576 MHz) + - description: Connectivity Peripheral 0 bus clock (from CMU_TOP) + - description: Connectivity Peripheral 0 IP clock (from CMU_TOP) + + clock-names: + items: + - const: oscclk + - const: dout_cmu_peric0_bus + - const: dout_cmu_peric0_ip + additionalProperties: false examples: diff --git a/include/dt-bindings/clock/google,gs101.h b/include/dt-bindings/clock/google,gs101.h index 21adec22387c..64e6bdc6359c 100644 --- a/include/dt-bindings/clock/google,gs101.h +++ b/include/dt-bindings/clock/google,gs101.h @@ -389,4 +389,85 @@ #define CLK_GOUT_MISC_WDT_CLUSTER1_PCLK 73 #define CLK_GOUT_MISC_XIU_D_MISC_ACLK 74 +/* CMU_PERIC0 */ +#define CLK_MOUT_PERIC0_BUS_USER 1 +#define CLK_MOUT_PERIC0_I3C_USER 2 +#define CLK_MOUT_PERIC0_USI0_UART_USER 3 +#define CLK_MOUT_PERIC0_USI14_USI_USER 4 +#define CLK_MOUT_PERIC0_USI1_USI_USER 5 +#define CLK_MOUT_PERIC0_USI2_USI_USER 6 +#define CLK_MOUT_PERIC0_USI3_USI_USER 7 +#define CLK_MOUT_PERIC0_USI4_USI_USER 8 +#define CLK_MOUT_PERIC0_USI5_USI_USER 9 +#define CLK_MOUT_PERIC0_USI6_USI_USER 10 +#define CLK_MOUT_PERIC0_USI7_USI_USER 11 +#define CLK_MOUT_PERIC0_USI8_USI_USER 12 +#define CLK_DOUT_PERIC0_I3C 13 +#define CLK_DOUT_PERIC0_USI0_UART 14 +#define CLK_DOUT_PERIC0_USI14_USI 15 +#define CLK_DOUT_PERIC0_USI1_USI 16 +#define CLK_DOUT_PERIC0_USI2_USI 17 +#define CLK_DOUT_PERIC0_USI3_USI 18 +#define CLK_DOUT_PERIC0_USI4_USI 19 +#define CLK_DOUT_PERIC0_USI5_USI 20 +#define CLK_DOUT_PERIC0_USI6_USI 21 +#define CLK_DOUT_PERIC0_USI7_USI 22 +#define CLK_DOUT_PERIC0_USI8_USI 23 +#define CLK_GOUT_PERIC0_IP 24 +#define CLK_GOUT_PERIC0_PERIC0_CMU_PERIC0_PCLK 25 +#define CLK_GOUT_PERIC0_CLK_PERIC0_OSCCLK_CLK 26 +#define CLK_GOUT_PERIC0_D_TZPC_PERIC0_PCLK 27 +#define CLK_GOUT_PERIC0_GPC_PERIC0_PCLK 28 +#define CLK_GOUT_PERIC0_GPIO_PERIC0_PCLK 29 +#define CLK_GOUT_PERIC0_LHM_AXI_P_PERIC0_I_CLK 30 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_0 31 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_1 32 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_10 33 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_11 34 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_12 35 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_13 36 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_14 37 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_15 38 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_2 39 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_3 40 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_4 41 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_5 42 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_6 43 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_7 44 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_8 45 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_IPCLK_9 46 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_0 47 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_1 48 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_10 49 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_11 50 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_12 51 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_13 52 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_14 53 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_15 54 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_2 55 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_3 56 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_4 57 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_5 58 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_6 59 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_7 60 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_8 61 +#define CLK_GOUT_PERIC0_PERIC0_TOP0_PCLK_9 62 +#define CLK_GOUT_PERIC0_PERIC0_TOP1_IPCLK_0 63 +#define CLK_GOUT_PERIC0_PERIC0_TOP1_IPCLK_2 64 +#define CLK_GOUT_PERIC0_PERIC0_TOP1_PCLK_0 65 +#define CLK_GOUT_PERIC0_PERIC0_TOP1_PCLK_2 66 +#define CLK_GOUT_PERIC0_CLK_PERIC0_BUSP_CLK 67 +#define CLK_GOUT_PERIC0_CLK_PERIC0_I3C_CLK 68 +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI0_UART_CLK 69 +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI14_USI_CLK 70 +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI1_USI_CLK 71 +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI2_USI_CLK 72 +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI3_USI_CLK 73 +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI4_USI_CLK 74 +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI5_USI_CLK 75 +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI6_USI_CLK 76 +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI7_USI_CLK 77 +#define CLK_GOUT_PERIC0_CLK_PERIC0_USI8_USI_CLK 78 +#define CLK_GOUT_PERIC0_SYSREG_PERIC0_PCLK 79 + #endif /* _DT_BINDINGS_CLOCK_GOOGLE_GS101_H */