Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <02ce6080-2dd2-047b-bd77-e8288d32dd46@amlogic.com>
Date:   Thu, 8 Jun 2023 11:26:24 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH V9 4/4] clk: meson: s4: add support for Amlogic S4 SoC
 peripheral clock controller
Content-Language: en-US
To:     Dmitry Rokosov <ddrokosov@sberdevices.ru>
Cc:     Jerome Brunet <jbrunet@baylibre.com>, linux-clk@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        kelvin.zhang@amlogic.com, qi.duan@amlogic.com
References: <20230517070215.28463-1-yu.tu@amlogic.com>
 <20230517070215.28463-5-yu.tu@amlogic.com>
 <1jwn0g39t2.fsf@starbuckisacylon.baylibre.com>
 <20230606153841.plf5uh6eyzpzsm2e@CAB-WSD-L081021>
From:   Yu Tu <yu.tu@amlogic.com>
In-Reply-To: <20230606153841.plf5uh6eyzpzsm2e@CAB-WSD-L081021>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N245TU1WZWNBU014VmV2TUlJN1poVEpROU9JMjdSa2RNTGdEVlJPNmluTmYw?=
 =?utf-8?B?elY4MUNvbFFQZ2NsT2ZDY1Q2eWZIUTJuYk40ZzdKOGlBWUZmTzBzRzZrQTRs?=
 =?utf-8?B?VlUvRmR3VnI4N1NaZWJ4Qi9YN0NyWmpoUnBLbW92SEYwWVJUb0J6VkJqNXJv?=
 =?utf-8?B?Y2FRL1hhS3NEdU52NFZPeEM3eTVYRkJUcFVlM3UrdUdBOVFxTEhYSHFPNUgy?=
 =?utf-8?B?ZG1HUkUvQmlieEpWdWdDMFgzMnNTc3lTZ0V5V0NLNU5ZQVFLZkZJeDdQU0Y5?=
 =?utf-8?B?d0hWd05aeGJBV05sRkYxVmU0V2VzRWRGdityYTJHZlRvMGQwR2ovdEIrV00y?=
 =?utf-8?B?dHo5S2ozS0F3SklTc2RhN2FURVlxMGsrQTR0ODdIbUp1clBSb29jWmVzZjVa?=
 =?utf-8?B?eFVMQTZDSzBURXBGTFBGRCtMSU0rTExYdHpteWdwb1NwYjUvTWFoTEZKT0Jl?=
 =?utf-8?B?VDRvdUNqSGlqeFJDcHlCb1gzWlZRVGFSM2xGWDlYd1QrOFZwRXVISE5kd3Bn?=
 =?utf-8?B?djNyZWZocmtySXlwTTZNcDNjOWtZSWtjUFU0a3UrckZOYUZJbDhMQUl0K2s1?=
 =?utf-8?B?OFlIRG8xbVE5MHROUTh6SWJBUlN2K2NLSm50dG5KYnVrZFJoLzRrNnhGTjZ2?=
 =?utf-8?B?MmIwdHBWZlRmdzFZUGNSRy9VYUZUdWFBV0xYRU9RRXlqT2tkM25XTUxxc1lF?=
 =?utf-8?B?Y2RGK2cralBXZEJmVXlIZlZzQjRnQStvZkY3ZlhqZ0VZQWl0cEgzVlRFbDdG?=
 =?utf-8?B?cGluUDlEUXoyN0ZRVVRLeEdmOG95dFdlZSttR09rVlpXWEh0QUxEcHBTWXIx?=
 =?utf-8?B?TDFiOWg0cGU2aFZ0Q05aWlB1QWNRZXFwL3ZVRjFCb3ZNVDNjK2ZpNWZqSUFE?=
 =?utf-8?B?M0IzWUViaDNqOUJ2OEFralB2UUE0S3FTVUQ4MnZpRWkrcnBmM2ZEU3BIcUZ1?=
 =?utf-8?B?M3ZnTlREaXFLZUNTOFk4R0prRjQ0M252aW9RU0Zub2dUMzZ4Q00ybDMvNUFm?=
 =?utf-8?B?WWt6T1llbTFNQzdJY2tDQnNqaUtBQUV2L2FnTGluU0pVa3VpNTk1cWpoKzhD?=
 =?utf-8?B?SjdxNlhVbVZqK1pJek1NTmxuQlh5UUh3RGFnT3BaTkMyWkNRYmVST21HK1U1?=
 =?utf-8?B?QWlkZWRDRDBpTE1pRXlaZ29Hakh1dkRYT0ZQMDhlblhsR09Fb010NzVxSldw?=
 =?utf-8?B?SmNtd05GQ2ZhcjJTYUlaeHVzbTI1TkFnL21nbU1UdWVxenhTNUlVbXdobVhr?=
 =?utf-8?B?U3NlMkNmU1NxZTJMdjhYQkh3U0U5UzJjM2tsOFhuV09MVEs3ajBaYVBOa00r?=
 =?utf-8?B?UVRjbDVRV3ZadDMxMzJmZHh2VEhTMENMT3lJZU1pMXFabHhSTlA4MkYvTWZP?=
 =?utf-8?B?enJxWHpTRE43VkFVUWFkaHNkRkRpWDNhVms4N3lwbmZBZFBHSlI5U2dWcUlZ?=
 =?utf-8?B?SE5HSUJiclllV1ZoZ0RZaE9ORWtUQy94SEdQaHRDc0dWRVpickN3U2d6cWVq?=
 =?utf-8?B?aXMxTXIzMnZGRFBPN2Q1OFVTNSt2SUtjdDcva3k2em5zOGV5R0lqV0ErSGdv?=
 =?utf-8?B?WW1ZS1VuY0lTQ2czS0hxYUYvTXdQdnUvcVRwck5lckJoYW1OWHlRaXltWUdR?=
 =?utf-8?B?b01ublpCSEE0bGdreFAwQ0ZSN24vV1ZvSytXcHM2VUFhYWdDTHpZVHpnc0xQ?=
 =?utf-8?B?cXU4a1hBa0lmMmlZODhHZ2NGbTRJOGRUZlQyOWxJVWtzeXJldURwdjFydEVX?=
 =?utf-8?B?dDA5RzdUN0x6UDREM1J5VWJ3b2JzY21WTHppNG9hcldpRTRTNlVLS3ZGZ1Js?=
 =?utf-8?B?RWFZNm9RUXpxSnVTMHJYcFZYRUhoQ2FjSXlCblFrTFY3a3NRaU41WGlOZ01Q?=
 =?utf-8?B?U1lGb1V2RVJzMG1OUDZYSjBFdlZaOTZtSEdZT3p0NWRNRmFDcTMrUnhKZ3Ru?=
 =?utf-8?B?UXh0RVpralRobVBpWXBwd1NYTU16LzVEVmwxRG5yaHM5V2FGbjJHOHMyem9L?=
 =?utf-8?B?NFFzRjVMemVCZFBHellhbXdwNk9HdUxRbWErYnNlVnVvRUVZa1dVMHZQc00y?=
 =?utf-8?B?MzlFMzRnRkdZek1ZcWlWL1lBc0FackZyK2JiRzFxMDA5cFM1dDV5VjBkTDlI?=
 =?utf-8?Q?AaM1oPzNI2+emwsIQYFqSULhQ?=
X-OriginatorOrg: amlogic.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ace3b27e-fc14-417f-ff23-08db67d0337f
X-MS-Exchange-CrossTenant-AuthSource: SG2PR03MB6730.apcprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jun 2023 03:26:51.9231
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0df2add9-25ca-4b3a-acb4-c99ddf0b1114
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: eL4hjh2IsUINPzQDGaDazyWw1rLsfm4Amq0YBJ3RHQMeIsl7nvbJcJTIT5D57RBpURcVKEuy6a/7zi5I7frBwA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: TYZPR03MB7576
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Dmitry,

On 2023/6/6 23:38, Dmitry Rokosov wrote:
> [ EXTERNAL EMAIL ]
> 
> Hello Yu,
> 
> On Tue, Jun 06, 2023 at 04:38:15PM +0200, Jerome Brunet wrote:
>>
>> On Wed 17 May 2023 at 15:02, Yu Tu <yu.tu@amlogic.com> wrote:
>>
>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>
>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>> ---
>>>   drivers/clk/meson/Kconfig          |   12 +
>>>   drivers/clk/meson/Makefile         |    1 +
>>>   drivers/clk/meson/s4-peripherals.c | 3830 ++++++++++++++++++++++++++++
>>>   drivers/clk/meson/s4-peripherals.h |  217 ++
>>>   4 files changed, 4060 insertions(+)
>>>   create mode 100644 drivers/clk/meson/s4-peripherals.c
>>>   create mode 100644 drivers/clk/meson/s4-peripherals.h
>>>
>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>> index a663c90a3f3b..a6eb9fa15c74 100644
>>> --- a/drivers/clk/meson/Kconfig
>>> +++ b/drivers/clk/meson/Kconfig
>>> @@ -128,4 +128,16 @@ config COMMON_CLK_S4_PLL
>>>        aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229.
>>>        Say Y if you want the board to work, because plls are the parent of most
>>>        peripherals.
>>> +
>>> +config COMMON_CLK_S4
>>> +   tristate "S4 SoC Peripherals clock controllers support"
>>> +   depends on ARM64
>>> +   default y
>>> +   select COMMON_CLK_MESON_REGMAP
>>> +   select COMMON_CLK_MESON_DUALDIV
>>> +   select COMMON_CLK_MESON_VID_PLL_DIV
>>> +   help
>>> +     Support for the Peripherals clock controller on Amlogic S805X2 and S905Y4
>>> +     devices, aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229.
>>> +     Say Y if you want peripherals to work.
>>>   endmenu
> 
> [...]
> 
>>> +static struct clk_regmap s4_rtc_32k_by_oscin = {
>>> +   .data = &(struct clk_regmap_gate_data){
>>> +           .offset = CLKCTRL_RTC_BY_OSCIN_CTRL0,
>>> +           .bit_idx = 30,
>>> +   },
>>> +   .hw.init = &(struct clk_init_data) {
>>> +           .name = "rtc_32k_by_oscin",
>>> +           .ops = &clk_regmap_gate_ops,
>>> +           .parent_hws = (const struct clk_hw *[]) {
>>> +                   &s4_rtc_32k_by_oscin_sel.hw
>>> +           },
>>> +           .num_parents = 1,
>>> +           .flags = CLK_SET_RATE_PARENT,
>>> +   },
>>> +};
>>> +
>>> +/*
>>> + * This RTC clock can be supplied by an external 32KHz crystal oscillator.
>>> + * If it is used, it should be documented in using fw_name and documented in the
>>> + * Bindings. Not currently in use on this board.
>>> + */
>>
>> This is confusing and not really helpful
>> What you describe here is simply the purpose of fw_name ... so it does
>> not warrant a specific comment
>>
>>> +static const struct clk_parent_data rtc_clk_sel_parent_data[] = {
>>> +   { .hw = &s4_rtc_32k_by_oscin.hw },
>>> +   { .hw = &s4_rtc_32k_by_oscin_div.hw },
>>> +   { .fw_name = "ext_32k",  }
>>> +};
>>> +
>>> +/*
>>> + * All clocks that can be inherited from a more accurate RTC clock are marked
>>> + * with the CLK_SET_RATE_NO_REPARENT flag. This is because in certain
>>> + * situations, we may need to freeze their parent. The parent setup of these
>>> + * clocks should be located on the device tree side.
>>> + */
>>
>> It looks like the consensus is that CLK_SET_RATE_NO_REPARENT is not
>> required. Please have at look at the discussion between Dmitry and
>> Martin for the a1 controller
>>
> 
> I hope below links will be helpful for you:
> 
> CLK_SET_RATE_NO_REPARENT IRC discussion:
> https://libera.irclog.whitequark.org/linux-amlogic/2023-05-18
> 
> Clock driver LKML discussion about CLK_SET_RATE_NO_REPARENT:
> https://lore.kernel.org/all/20230530120640.irugyrio3qa7czjy@CAB-WSD-L081021/
> https://lore.kernel.org/all/20230524092750.ldm362chnpkwkcj4@CAB-WSD-L081021/
> 
> PWM discussion about special RTC case:
> https://lore.kernel.org/all/20230522133739.7tc35zr2npsysopd@CAB-WSD-L081021/
> 
> And I apologize for any confusion I may have caused in our previous
> discussion. I want to clarify that I have updated the implementation
> of CLK_SET_RATE_NO_REPARENT after discussing it with Martin...
> 
> [...]

Thank you so much for your comments.

> 
> --
> Thank you,
> Dmitry