From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030706AbcDMLyH (ORCPT ); Wed, 13 Apr 2016 07:54:07 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:39291 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030522AbcDMLyC (ORCPT ); Wed, 13 Apr 2016 07:54:02 -0400 X-AuditID: cbfee690-f79e56d0000012c4-5e-570e335600f7 Message-id: <570E3366.2030809@samsung.com> Date: Wed, 13 Apr 2016 17:24:14 +0530 From: Alim Akhtar User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-version: 1.0 To: Tomasz Figa , Sylwester Nawrocki Cc: linux-arm-kernel , Thomas Abraham , =?UTF-8?B?S3J6eXN6dG9mIEtvesWCb3dza2k=?= , linux-kernel , "linux-samsung-soc@vger.kernel.org" , linux-clk@vger.kernel.org, Stephen Boyd , Michael Turquette Subject: Re: [PATCH] clk: samsung: exynos7: Enable clocks for CMU_CCORE and CMU_FSYS0 blocks References: <1460459243-27120-1-git-send-email-alim.akhtar@samsung.com> <570E2123.1000107@samsung.com> In-reply-to: Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrCIsWRmVeSWpSXmKPExsWyRsSkXTfMmC/coOuWpMXrF4YWmx5fY7X4 2HOP1eLyrjlsFjPO72OyuHjK1eLwm3ZWix9nulksOpYxWqza9YfRgcvj/Y1Wdo/Lfb1MHjtn 3WX32Lyk3qNvyypGj8+b5ALYorhsUlJzMstSi/TtErgyGro+shWsFq34NG86ewNjs2AXIyeH hICJxKz131khbDGJC/fWs3UxcnEICaxglFh8YiU7TNHr64dZQGwhgVmMEv/uVkEUPWCU2D31 LViCV0BLYvaqd4wgNouAqsThHRfAmtkEtCXuTt/C1MXIwSEqECHx+IIQRLmgxI/J98BaRQTC JP5efs8KMpNZYBqzxOX/28HmCAvESezddIwZYtk3RombO3uYQBKcAsESd7+0gJ3NLGAm8ahl HTOELS+xec1bsAYJgY/sEhe6D0JdJCDxbfIhFpArJARkJTYdYIb4TFLi4IobLBMYxWYhOWoW krGzkIxdwMi8ilE0tSC5oDgpvchErzgxt7g0L10vOT93EyMwMk//ezZhB+O9A9aHGAU4GJV4 eC+s4Q0XYk0sK67MPcRoCnTFRGYp0eR8YPznlcQbGpsZWZiamBobmVuaKYnzvpb6GSwkkJ5Y kpqdmlqQWhRfVJqTWnyIkYmDU6qB0TEh74GUi1xEvkR3qI8Tv+nzs1FPDjbM2B7ErhbyKniV 0r71svGX11imTyjPiDXxjl1wLtLQxelp+onl92oa1aw8c7QtExsTNzcuN17tz/Vn17E3iVeM szb9//GhZEIsq89Md/Wks69fl1atZt9QdW7hL7Mlt5YH1r25/nfv3jPb9nxfnZLuoMRSnJFo qMVcVJwIAJZh/hnHAgAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFmpkleLIzCtJLcpLzFFi42I5/e+xoG6YMV+4waXDIhavXxhabHp8jdXi Y889VovLu+awWcw4v4/J4uIpV4vDb9pZLX6c6Wax6FjGaLFq1x9GBy6P9zda2T0u9/Uyeeyc dZfdY/OSeo++LasYPT5vkgtgi2pgtMlITUxJLVJIzUvOT8nMS7dV8g6Od443NTMw1DW0tDBX UshLzE21VXLxCdB1y8wBuktJoSwxpxQoFJBYXKykb4dpQmiIm64FTGOErm9IEFyPkQEaSFjH mNHQ9ZGtYLVoxad509kbGJsFuxg5OSQETCReXz/MAmGLSVy4t54NxBYSmMUo8e9uVRcjF5D9 gFFi99S3YEW8AloSs1e9YwSxWQRUJQ7vuMAOYrMJaEvcnb6FqYuRg0NUIELi8QUhiHJBiR+T 74G1igiESfy9/J4VZCazwDRmicv/t4PNERaIk9i76RgzxLJvjBI3d/YwgSQ4BYIl7n5pYQWx mQXMJB61rGOGsOUlNq95yzyBEehMhCWzkJTNQlK2gJF5FaNEakFyQXFSeq5RXmq5XnFibnFp Xrpecn7uJkZw9D+T3sF4eJf7IUYBDkYlHl6N9bzhQqyJZcWVuYcYJTiYlUR4r+vzhQvxpiRW VqUW5ccXleakFh9iNAWGwkRmKdHkfGBiyiuJNzQ2MTOyNDKzMDIxN1cS5338f12YkEB6Yklq dmpqQWoRTB8TB6dUA2NsmYHrc/8H1+NWOWW48R9t27b2WJJSrqn0zcU/gm8eiSpTNskIDeZQ a9yuZb3oWcttO/nGr3M0VXe+7Nn8fOFkLg4O3UQpFnWRtbbXOt9P+X9omcAvwa19mXPffP9V PjH88QEGvQnxztebTyyadXDPftZP0vbJvNdOn36+9nRLsfSfYte6TWJKLMUZiYZazEXFiQDX lPhMFAMAAA== DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi On 04/13/2016 05:01 PM, Tomasz Figa wrote: > 2016-04-13 13:36 GMT+03:00 Sylwester Nawrocki : >> On 04/13/2016 08:26 AM, Tomasz Figa wrote: >>>>> @@ -205,7 +206,11 @@ static struct samsung_cmu_info topc_cmu_info __initdata = { >>>>> >>>>> static void __init exynos7_clk_topc_init(struct device_node *np) >>>>> { >>>>> + struct clk *clk; >>>>> + >>>>> samsung_cmu_register_one(np, &topc_cmu_info); >>>>> + clk = __clk_lookup("aclk_ccore_133"); >>>>> + clk_prepare_enable(clk); >>> >>> Shouldn't this be rather done before calling >>> samsung_cmu_register_one()? I don't remember exactly, but wouldn't >>> clock registration trigger reading back current (mux, div) values from >>> registers? >>> >>> Also, do we have any guarantees on order of initialization of >>> particular CMUs? I believe this will happen in order of DT nodes and >>> so would be not any kind of guarantee at all. >> >> If these clocks need to be kept enabled perhaps it's better to just set >> CLK_IS_CRITICAL flag for them? Patches adding this flag are already in >> clk-next branch in the clk git tree. This way the clocks would get >> enabled within the clk_register() call. >> > > Oh, I didn't know about this. Sounds like a good idea. :) > I was not aware of this flag, Thanks for pointing this out. This flag indeed solves my problem. These clocks should not be gated since the CMU block using this clock for SFR access. Please ignore the $SUBJECT patch, I will send a new patch which uses CLK_IS_CRITICAL flag instead. >> The CMU registration order is enforced by listing input clocks to each >> CMU in DT. > > Ah, right, I recall now, I even reviewed relevant patch. Thanks for > refreshing my memory. > >> However, I wouldn't be concerned much about it in context >> of this patch. We are enabling here clocks which belong to same CMU. >> samsung_cmu_register_one() needs to be called first for subsequent >> __clk_lookup() calls to work. > > I thought a clock from another CMU has to be enabled for the CPU to be > able to access the CMU being registered. > >> >> Perhaps related bits need to be set manually in CMU registers before >> registering a clock provider for the CMU, to fulfil the requirements. >> >> Anyway, summary of the $subject patch seems not precise enough: >> >> "This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is >> required before accessing registers of these blocks." >> >> We need to enable selected clocks (i.e. access the CMU's registers) >> before accessing this CMU's registers? > > Yeah, maybe the explanation could be a bit more precise. > > Best regards, > Tomasz > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: alim.akhtar@samsung.com (Alim Akhtar) Date: Wed, 13 Apr 2016 17:24:14 +0530 Subject: [PATCH] clk: samsung: exynos7: Enable clocks for CMU_CCORE and CMU_FSYS0 blocks In-Reply-To: References: <1460459243-27120-1-git-send-email-alim.akhtar@samsung.com> <570E2123.1000107@samsung.com> Message-ID: <570E3366.2030809@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi On 04/13/2016 05:01 PM, Tomasz Figa wrote: > 2016-04-13 13:36 GMT+03:00 Sylwester Nawrocki : >> On 04/13/2016 08:26 AM, Tomasz Figa wrote: >>>>> @@ -205,7 +206,11 @@ static struct samsung_cmu_info topc_cmu_info __initdata = { >>>>> >>>>> static void __init exynos7_clk_topc_init(struct device_node *np) >>>>> { >>>>> + struct clk *clk; >>>>> + >>>>> samsung_cmu_register_one(np, &topc_cmu_info); >>>>> + clk = __clk_lookup("aclk_ccore_133"); >>>>> + clk_prepare_enable(clk); >>> >>> Shouldn't this be rather done before calling >>> samsung_cmu_register_one()? I don't remember exactly, but wouldn't >>> clock registration trigger reading back current (mux, div) values from >>> registers? >>> >>> Also, do we have any guarantees on order of initialization of >>> particular CMUs? I believe this will happen in order of DT nodes and >>> so would be not any kind of guarantee at all. >> >> If these clocks need to be kept enabled perhaps it's better to just set >> CLK_IS_CRITICAL flag for them? Patches adding this flag are already in >> clk-next branch in the clk git tree. This way the clocks would get >> enabled within the clk_register() call. >> > > Oh, I didn't know about this. Sounds like a good idea. :) > I was not aware of this flag, Thanks for pointing this out. This flag indeed solves my problem. These clocks should not be gated since the CMU block using this clock for SFR access. Please ignore the $SUBJECT patch, I will send a new patch which uses CLK_IS_CRITICAL flag instead. >> The CMU registration order is enforced by listing input clocks to each >> CMU in DT. > > Ah, right, I recall now, I even reviewed relevant patch. Thanks for > refreshing my memory. > >> However, I wouldn't be concerned much about it in context >> of this patch. We are enabling here clocks which belong to same CMU. >> samsung_cmu_register_one() needs to be called first for subsequent >> __clk_lookup() calls to work. > > I thought a clock from another CMU has to be enabled for the CPU to be > able to access the CMU being registered. > >> >> Perhaps related bits need to be set manually in CMU registers before >> registering a clock provider for the CMU, to fulfil the requirements. >> >> Anyway, summary of the $subject patch seems not precise enough: >> >> "This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is >> required before accessing registers of these blocks." >> >> We need to enable selected clocks (i.e. access the CMU's registers) >> before accessing this CMU's registers? > > Yeah, maybe the explanation could be a bit more precise. > > Best regards, > Tomasz > >