| 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 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. Thanks, ta
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. Best regards, Krzysztof
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. 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. Thanks for the guidance. ta
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
On 1/9/24 18:38, Krzysztof Kozlowski wrote: > 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. The datasheet is not so verbose, but as I understand, CMU_MISC and CMU_PERIC0 are clock domains of the same clock controller, thus I think they should be treated the same. We should either get rid of the name of the block in the clock names or keep it, but be consistent across all the clock domains. > >> 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? > As Peter pointed out, if keeping the shorter names, one would have to cross reference with the device tree in order to determine which clock is used, its type, whether it's a gate or a divider. Whereas if we keep the full name, one can see what's the clock about with a glance. The full name coincides with the clock names that are defined in the clock driver, thus one can grep for the full name from the device tree and hit the clock definition from the clock driver. The cons of keeping the full name is that keeping the name of the block in the DT's clock name is just redundant. Rob was clear and said that including the block name in the -names is a pattern we don't want. In what concerns my personal preference, I like the full name. At the same time, I see Rob's point, and if that turns out to be a rule, let's respect it. So I'm fine with both, but let's be consistent across the driver and have the same clock name scheme for all the clock domains, otherwise it will just look weird. Thanks, ta
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 */