RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Quoting Anson Huang (2018-10-15 21:37:31) > Hi, Stephen > > Anson Huang > Best Regards! > > > > -Original Message- > > From: Stephen Boyd > > Sent: Tuesday, October 16, 2018 12:46 AM > > To: ker...@pengutronix.de; linux-arm-ker...@lists.infradead.org; > > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > > mturque...@baylibre.com; s.ha...@pengutronix.de; shawn...@kernel.org; > > Anson Huang ; Fabio Estevam > > ; Jerome Forissier ; > > Peng Fan ; Rob Herring > > Cc: dl-linux-imx > > Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > > > Quoting Anson Huang (2018-10-15 02:33:35) > > > > > > > > > > > > > > > > > > > Why can't we add clks to the op-tee node in DT's /firmware > > > > > > container? > > > > > > Then any clks in there can be turned on forever and left enabled > > > > > > by the linux driver? > > > > > > > > > > I did NOT run op-tee with Linux-next kernel before, can you advise > > > > > more? > > > > > > > > Neither have I, so I can't advise more. > > > > > > > > > And I think if op-tee has such requirement, can we have another > > > > > patch to cover it? > > > > > > > > Yes. > > > > > > > > > > > > > I believe all other i.MX platforms also have same requirements if > > > > > considering op-tee support, so I think it should be another topic, > > > > > what do you > > > > think? > > > > > > > > > > > > > I'm going to drop these patches from my review queue. Please resend > > > > them and please include the op-tee patches too. > > > > > > > > > I do NOT know how to include the op-tee patch to meet special > > > requirement, should the op-tee related patch be added later when someone > > actually add the op-tee support for i.MX7? > > > It should NOT block this patch set, do you think we can add this patch set > > first? > > > > > > > Please resend the two patches. In the commit text for the second patch, > > describe the plan to remove CLK_IS_CRITICAL from these clks by adding an > > OP-TEE device/driver to the kernel to keep these clks enabled. My > > understanding is that there isn't an OP-TEE driver right now, but these > > clks are > > used by the firmware and can't be turned off in Linux. If in the future we > > want > > to be able to turn them on and off, we'll need to add them to an OP-TEE > > device > > node and have that driver manage the clks. > > > > How that will work when a system doesn't enable the OP-TEE driver I'm not > > sure. We may need to develop some system whereby clks like this are handed > > from the clk controller to the consumer driver when it's enabled for further > > power managment, but if they're never handed off, they're kept on forever > > like > > is done here. Anyway, please resend with a note about why these are marked > > CLK_IS_CRITICAL. > > I think there is some misunderstanding here, this patch sets those critical > clocks > with CLK_IS_CRITICAL flag to let clock driver keep them always ON, as they are > critical clocks and system (Linux kernel, NOT include op-tee) can NOT run > normally > if anyone of them is OFF. Ok it sounds like OP-TEE usage completely orthogonal here? I'll go apply these refactorings then.
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Hi, Stephen Anson Huang Best Regards! > -Original Message- > From: Stephen Boyd > Sent: Tuesday, October 16, 2018 12:46 AM > To: ker...@pengutronix.de; linux-arm-ker...@lists.infradead.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > mturque...@baylibre.com; s.ha...@pengutronix.de; shawn...@kernel.org; > Anson Huang ; Fabio Estevam > ; Jerome Forissier ; > Peng Fan ; Rob Herring > Cc: dl-linux-imx > Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > Quoting Anson Huang (2018-10-15 02:33:35) > > > > > > > > > > > > > > > > Why can't we add clks to the op-tee node in DT's /firmware container? > > > > > Then any clks in there can be turned on forever and left enabled > > > > > by the linux driver? > > > > > > > > I did NOT run op-tee with Linux-next kernel before, can you advise more? > > > > > > Neither have I, so I can't advise more. > > > > > > > And I think if op-tee has such requirement, can we have another > > > > patch to cover it? > > > > > > Yes. > > > > > > > > > > I believe all other i.MX platforms also have same requirements if > > > > considering op-tee support, so I think it should be another topic, > > > > what do you > > > think? > > > > > > > > > > I'm going to drop these patches from my review queue. Please resend > > > them and please include the op-tee patches too. > > > > > > I do NOT know how to include the op-tee patch to meet special > > requirement, should the op-tee related patch be added later when someone > actually add the op-tee support for i.MX7? > > It should NOT block this patch set, do you think we can add this patch set > first? > > > > Please resend the two patches. In the commit text for the second patch, > describe the plan to remove CLK_IS_CRITICAL from these clks by adding an > OP-TEE device/driver to the kernel to keep these clks enabled. My > understanding is that there isn't an OP-TEE driver right now, but these clks > are > used by the firmware and can't be turned off in Linux. If in the future we > want > to be able to turn them on and off, we'll need to add them to an OP-TEE device > node and have that driver manage the clks. > > How that will work when a system doesn't enable the OP-TEE driver I'm not > sure. We may need to develop some system whereby clks like this are handed > from the clk controller to the consumer driver when it's enabled for further > power managment, but if they're never handed off, they're kept on forever like > is done here. Anyway, please resend with a note about why these are marked > CLK_IS_CRITICAL. I think there is some misunderstanding here, this patch sets those critical clocks with CLK_IS_CRITICAL flag to let clock driver keep them always ON, as they are critical clocks and system (Linux kernel, NOT include op-tee) can NOT run normally if anyone of them is OFF. The question about op-tee is, there might be some special clocks needed by op-tee, for example, currently clk A, B and C are kept always ON for Linux kernel to run normally, but when op-tee is executing, it might need clk D to be ON, so I think the clk D is needed ONLY for op-tee, op-tee should have a driver or firmware to runtime ON/OFF clk D as needed. Since I do NOT know what clocks op-tee needs, so I will leave them to be added by op-tee owner, whoever enables op-tee, he should consider where to manage its special clocks, either in clk driver or op-tee driver/firmware. So do we still need to mention the op-tee related info in commit text? Actually, this patch is just to replace those always-ON clocks in clks_init_on array with CLK_IS_CRITICAL flag. Then no need to explicitly enable clocks in clks_init_on array and just rely on common clk framework which will enable all clocks with CLK_IS_CRITICAL flag set, it will align with all other i.MX SoCs clk driver. Anson.
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Quoting Anson Huang (2018-10-15 02:33:35) > > > > > > > > > > > > > Why can't we add clks to the op-tee node in DT's /firmware container? > > > > Then any clks in there can be turned on forever and left enabled by > > > > the linux driver? > > > > > > I did NOT run op-tee with Linux-next kernel before, can you advise more? > > > > Neither have I, so I can't advise more. > > > > > And I think if op-tee has such requirement, can we have another patch > > > to cover it? > > > > Yes. > > > > > > > I believe all other i.MX platforms also have same requirements if > > > considering op-tee support, so I think it should be another topic, what > > > do you > > think? > > > > > > > I'm going to drop these patches from my review queue. Please resend them > > and please include the op-tee patches too. > > > I do NOT know how to include the op-tee patch to meet special requirement, > should > the op-tee related patch be added later when someone actually add the op-tee > support for i.MX7? > It should NOT block this patch set, do you think we can add this patch set > first? > Please resend the two patches. In the commit text for the second patch, describe the plan to remove CLK_IS_CRITICAL from these clks by adding an OP-TEE device/driver to the kernel to keep these clks enabled. My understanding is that there isn't an OP-TEE driver right now, but these clks are used by the firmware and can't be turned off in Linux. If in the future we want to be able to turn them on and off, we'll need to add them to an OP-TEE device node and have that driver manage the clks. How that will work when a system doesn't enable the OP-TEE driver I'm not sure. We may need to develop some system whereby clks like this are handed from the clk controller to the consumer driver when it's enabled for further power managment, but if they're never handed off, they're kept on forever like is done here. Anyway, please resend with a note about why these are marked CLK_IS_CRITICAL.
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Hi, Stephen Anson Huang Best Regards! > -Original Message- > From: Stephen Boyd > Sent: Saturday, October 13, 2018 3:48 AM > To: ker...@pengutronix.de; linux-arm-ker...@lists.infradead.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > mturque...@baylibre.com; s.ha...@pengutronix.de; shawn...@kernel.org; > Anson Huang ; Fabio Estevam > ; Jerome Forissier ; > Peng Fan ; Rob Herring > Cc: dl-linux-imx > Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > Quoting Anson Huang (2018-10-08 01:34:59) > > > Quoting Anson Huang (2018-09-03 00:20:53) > > > > > > On 08/31/2018 03:29 AM, Stephen Boyd wrote: > > > > > > > Quoting Peng Fan (2018-08-12 18:15:41) > > > > > > >> Hi Anson, > > > > > > >> > > > > > > >>>>> -Original Message- > > > > > > >>>>> From: Anson Huang > > > > > > >>>>> Sent: 2018年8月8日 12:39 > > > > > > >>>>> To: shawn...@kernel.org; s.ha...@pengutronix.de; > > > > > > >>>>> ker...@pengutronix.de; Fabio Estevam > > > > > > >>>>> ; mturque...@baylibre.com; > > > > > > >>>>> sb...@kernel.org; linux-arm-ker...@lists.infradead.org; > > > > > > >>>>> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > > > > > > >>>>> Cc: dl-linux-imx > > > > > > >>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove > > > > > > >>>>> clks_init_on array > > > > > > >>>>> > > > > > > >>>>> Clock framework will enable those clocks registered with > > > > > > >>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on > > > > > > >>>>> array during clock > > > > > > >>>> initialization now. > > > > > > >>>> > > > > > > >>>> Will it be more flexible to parse dts saying "critical-clocks = > " > > > > > > >>>> or "init-on-arrary=" > > > > > > >>>> and enable those clocks? > > > > > > >>> > > > > > > >>> Parsing the clocks arrays from dtb is another way of > > > > > > >>> enabling critical clocks, but for current i.MX6/7 > > > > > > >>> platforms, we implement it in same way as most of other > > > > > > >>> SoCs, currently I did NOT see any necessity of putting > > > > > > >>> them in dtb, just adding flag during clock registering is > > > > > > >>> more simple, if there is any special requirement for > > > > > > >>> different clocks set to be enabled, then we can add > > > > > > >>> support to enable > > > > > the method of parsing critical-clocks from dtb. Just my two cents. > > > > > > >> > > > > > > >> Thinking about OP-TEE want to use one device, but it's > > > > > > >> clocks are registered by Linux, because there is no module > > > > > > >> in Linux side use it, it will shutdown the clock, which > > > > > > >> cause OP-TEE could not access the > > > > > device. > > > > > > >> > > > > > > >> Then people have to modify clk code to add CLK_IS_CRITICAL > > > > > > >> flag to make sure the clocks are not shutdown by Linux. > > > > > > >> > > > > > > >> However adding a new property in clk node and let driver > > > > > > >> code parse the dts, there is no need to modify clk driver > > > > > > >> code when OP-TEE needs > > > > > another device clock. > > > > > > >> > > > > > > > > > > > > > > If OP-TEE needs linux to keep things on then why can't the > > > > > > > OP-TEE driver in Linux probe, acquire clocks, and keep the > > > > > > > clks enabled > > > forever? > > > > > > > > > > > > Sounds reasonable, but how could this be done without > > > > > > introducing platform-specific stuff in the OP-TEE driver? > > > > > > > > > > > > > > > > Why is that a goal? > > > > > > > > I do NOT think we should consider such case in this patch series, > > > > whatever OP-TEE needs for its own feature, it should do necessary > > > > operations > > > either in its driver or somewhere else by adding new patch. > > > > > > > > > > Why can't we add clks to the op-tee node in DT's /firmware container? > > > Then any clks in there can be turned on forever and left enabled by > > > the linux driver? > > > > I did NOT run op-tee with Linux-next kernel before, can you advise more? > > Neither have I, so I can't advise more. > > > And I think if op-tee has such requirement, can we have another patch > > to cover it? > > Yes. > > > > I believe all other i.MX platforms also have same requirements if > > considering op-tee support, so I think it should be another topic, what do > > you > think? > > > > I'm going to drop these patches from my review queue. Please resend them > and please include the op-tee patches too. I do NOT know how to include the op-tee patch to meet special requirement, should the op-tee related patch be added later when someone actually add the op-tee support for i.MX7? It should NOT block this patch set, do you think we can add this patch set first? Anson.
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Quoting Anson Huang (2018-10-08 01:34:59) > > Quoting Anson Huang (2018-09-03 00:20:53) > > > > > On 08/31/2018 03:29 AM, Stephen Boyd wrote: > > > > > > Quoting Peng Fan (2018-08-12 18:15:41) > > > > > >> Hi Anson, > > > > > >> > > > > > > -Original Message- > > > > > > From: Anson Huang > > > > > > Sent: 2018年8月8日 12:39 > > > > > > To: shawn...@kernel.org; s.ha...@pengutronix.de; > > > > > > ker...@pengutronix.de; Fabio Estevam > > > > > > ; mturque...@baylibre.com; > > > > > > sb...@kernel.org; linux-arm-ker...@lists.infradead.org; > > > > > > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > > > > > > Cc: dl-linux-imx > > > > > > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on > > > > > > array > > > > > > > > > > > > Clock framework will enable those clocks registered with > > > > > > CLK_IS_CRITICAL flag, so no need to have clks_init_on array > > > > > > during clock > > > > > initialization now. > > > > > > > > > > Will it be more flexible to parse dts saying "critical-clocks = > > > > > " > > > > > or "init-on-arrary=" > > > > > and enable those clocks? > > > > > >>> > > > > > >>> Parsing the clocks arrays from dtb is another way of enabling > > > > > >>> critical clocks, but for current i.MX6/7 platforms, we > > > > > >>> implement it in same way as most of other SoCs, currently I > > > > > >>> did NOT see any necessity of putting them in dtb, just adding > > > > > >>> flag during clock registering is more simple, if there is any > > > > > >>> special requirement for different clocks set to be enabled, > > > > > >>> then we can add support to enable > > > > the method of parsing critical-clocks from dtb. Just my two cents. > > > > > >> > > > > > >> Thinking about OP-TEE want to use one device, but it's clocks > > > > > >> are registered by Linux, because there is no module in Linux > > > > > >> side use it, it will shutdown the clock, which cause OP-TEE > > > > > >> could not access the > > > > device. > > > > > >> > > > > > >> Then people have to modify clk code to add CLK_IS_CRITICAL flag > > > > > >> to make sure the clocks are not shutdown by Linux. > > > > > >> > > > > > >> However adding a new property in clk node and let driver code > > > > > >> parse the dts, there is no need to modify clk driver code when > > > > > >> OP-TEE needs > > > > another device clock. > > > > > >> > > > > > > > > > > > > If OP-TEE needs linux to keep things on then why can't the > > > > > > OP-TEE driver in Linux probe, acquire clocks, and keep the clks > > > > > > enabled > > forever? > > > > > > > > > > Sounds reasonable, but how could this be done without introducing > > > > > platform-specific stuff in the OP-TEE driver? > > > > > > > > > > > > > Why is that a goal? > > > > > > I do NOT think we should consider such case in this patch series, > > > whatever OP-TEE needs for its own feature, it should do necessary > > > operations > > either in its driver or somewhere else by adding new patch. > > > > > > > Why can't we add clks to the op-tee node in DT's /firmware container? > > Then any clks in there can be turned on forever and left enabled by the > > linux > > driver? > > I did NOT run op-tee with Linux-next kernel before, can you advise more? Neither have I, so I can't advise more. > And I think if op-tee has such requirement, > can we have another patch to cover it? Yes. > I believe all other i.MX platforms also have same > requirements if considering op-tee support, so I think it should be another > topic, what do you think? > I'm going to drop these patches from my review queue. Please resend them and please include the op-tee patches too.
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Anson Huang Best Regards! > -Original Message- > From: Stephen Boyd > Sent: Monday, October 8, 2018 3:41 PM > To: ker...@pengutronix.de; linux-arm-ker...@lists.infradead.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > mturque...@baylibre.com; s.ha...@pengutronix.de; shawn...@kernel.org; > Anson Huang ; Fabio Estevam > ; Jerome Forissier ; > Peng Fan ; Rob Herring > Cc: dl-linux-imx > Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > Quoting Anson Huang (2018-09-03 00:20:53) > > > > On 08/31/2018 03:29 AM, Stephen Boyd wrote: > > > > > Quoting Peng Fan (2018-08-12 18:15:41) > > > > >> Hi Anson, > > > > >> > > > > >>>>> -Original Message- > > > > >>>>> From: Anson Huang > > > > >>>>> Sent: 2018年8月8日 12:39 > > > > >>>>> To: shawn...@kernel.org; s.ha...@pengutronix.de; > > > > >>>>> ker...@pengutronix.de; Fabio Estevam > > > > >>>>> ; mturque...@baylibre.com; > > > > >>>>> sb...@kernel.org; linux-arm-ker...@lists.infradead.org; > > > > >>>>> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > > > > >>>>> Cc: dl-linux-imx > > > > >>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on > > > > >>>>> array > > > > >>>>> > > > > >>>>> Clock framework will enable those clocks registered with > > > > >>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on array > > > > >>>>> during clock > > > > >>>> initialization now. > > > > >>>> > > > > >>>> Will it be more flexible to parse dts saying "critical-clocks = > > > > >>>> " > > > > >>>> or "init-on-arrary=" > > > > >>>> and enable those clocks? > > > > >>> > > > > >>> Parsing the clocks arrays from dtb is another way of enabling > > > > >>> critical clocks, but for current i.MX6/7 platforms, we > > > > >>> implement it in same way as most of other SoCs, currently I > > > > >>> did NOT see any necessity of putting them in dtb, just adding > > > > >>> flag during clock registering is more simple, if there is any > > > > >>> special requirement for different clocks set to be enabled, > > > > >>> then we can add support to enable > > > the method of parsing critical-clocks from dtb. Just my two cents. > > > > >> > > > > >> Thinking about OP-TEE want to use one device, but it's clocks > > > > >> are registered by Linux, because there is no module in Linux > > > > >> side use it, it will shutdown the clock, which cause OP-TEE > > > > >> could not access the > > > device. > > > > >> > > > > >> Then people have to modify clk code to add CLK_IS_CRITICAL flag > > > > >> to make sure the clocks are not shutdown by Linux. > > > > >> > > > > >> However adding a new property in clk node and let driver code > > > > >> parse the dts, there is no need to modify clk driver code when > > > > >> OP-TEE needs > > > another device clock. > > > > >> > > > > > > > > > > If OP-TEE needs linux to keep things on then why can't the > > > > > OP-TEE driver in Linux probe, acquire clocks, and keep the clks > > > > > enabled > forever? > > > > > > > > Sounds reasonable, but how could this be done without introducing > > > > platform-specific stuff in the OP-TEE driver? > > > > > > > > > > Why is that a goal? > > > > I do NOT think we should consider such case in this patch series, > > whatever OP-TEE needs for its own feature, it should do necessary operations > either in its driver or somewhere else by adding new patch. > > > > Why can't we add clks to the op-tee node in DT's /firmware container? > Then any clks in there can be turned on forever and left enabled by the linux > driver? I did NOT run op-tee with Linux-next kernel before, can you advise more? And I think if op-tee has such requirement, can we have another patch to cover it? I believe all other i.MX platforms also have same requirements if considering op-tee support, so I think it should be another topic, what do you think? Anson.
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Quoting Anson Huang (2018-09-03 00:20:53) > > > On 08/31/2018 03:29 AM, Stephen Boyd wrote: > > > > Quoting Peng Fan (2018-08-12 18:15:41) > > > >> Hi Anson, > > > >> > > > > -Original Message- > > > > From: Anson Huang > > > > Sent: 2018年8月8日 12:39 > > > > To: shawn...@kernel.org; s.ha...@pengutronix.de; > > > > ker...@pengutronix.de; Fabio Estevam ; > > > > mturque...@baylibre.com; sb...@kernel.org; > > > > linux-arm-ker...@lists.infradead.org; > > > > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > > > > Cc: dl-linux-imx > > > > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > > > > > > > Clock framework will enable those clocks registered with > > > > CLK_IS_CRITICAL flag, so no need to have clks_init_on array > > > > during clock > > > initialization now. > > > > > > Will it be more flexible to parse dts saying "critical-clocks = > > > " > > > or "init-on-arrary=" > > > and enable those clocks? > > > >>> > > > >>> Parsing the clocks arrays from dtb is another way of enabling > > > >>> critical clocks, but for current i.MX6/7 platforms, we implement > > > >>> it in same way as most of other SoCs, currently I did NOT see any > > > >>> necessity of putting them in dtb, just adding flag during clock > > > >>> registering is more simple, if there is any special requirement > > > >>> for different clocks set to be enabled, then we can add support to > > > >>> enable > > the method of parsing critical-clocks from dtb. Just my two cents. > > > >> > > > >> Thinking about OP-TEE want to use one device, but it's clocks are > > > >> registered by Linux, because there is no module in Linux side use > > > >> it, it will shutdown the clock, which cause OP-TEE could not access the > > device. > > > >> > > > >> Then people have to modify clk code to add CLK_IS_CRITICAL flag to > > > >> make sure the clocks are not shutdown by Linux. > > > >> > > > >> However adding a new property in clk node and let driver code parse > > > >> the dts, there is no need to modify clk driver code when OP-TEE needs > > another device clock. > > > >> > > > > > > > > If OP-TEE needs linux to keep things on then why can't the OP-TEE > > > > driver in Linux probe, acquire clocks, and keep the clks enabled > > > > forever? > > > > > > Sounds reasonable, but how could this be done without introducing > > > platform-specific stuff in the OP-TEE driver? > > > > > > > Why is that a goal? > > I do NOT think we should consider such case in this patch series, whatever > OP-TEE needs for its own > feature, it should do necessary operations either in its driver or somewhere > else by adding new patch. > Why can't we add clks to the op-tee node in DT's /firmware container? Then any clks in there can be turned on forever and left enabled by the linux driver?
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Gentle Ping... Anson Huang Best Regards! > -Original Message- > From: Anson Huang > Sent: Monday, September 3, 2018 3:21 PM > To: Stephen Boyd ; ker...@pengutronix.de; > linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org; > linux-kernel@vger.kernel.org; mturque...@baylibre.com; > s.ha...@pengutronix.de; shawn...@kernel.org; Fabio Estevam > ; Jerome Forissier ; > Peng Fan ; Rob Herring > Cc: dl-linux-imx > Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > > > Anson Huang > Best Regards! > > > > -Original Message- > > From: Stephen Boyd > > Sent: Saturday, September 1, 2018 1:58 AM > > To: ker...@pengutronix.de; linux-arm-ker...@lists.infradead.org; > > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > > mturque...@baylibre.com; s.ha...@pengutronix.de; > shawn...@kernel.org; > > Anson Huang ; Fabio Estevam > > ; Jerome Forissier > > ; Peng Fan ; Rob > > Herring > > Cc: dl-linux-imx > > Subject: Re: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > > > Quoting Jerome Forissier (2018-08-31 01:01:44) > > > > > > > > > On 08/31/2018 03:29 AM, Stephen Boyd wrote: > > > > Quoting Peng Fan (2018-08-12 18:15:41) > > > >> Hi Anson, > > > >> > > > >>>>> -Original Message- > > > >>>>> From: Anson Huang > > > >>>>> Sent: 2018年8月8日 12:39 > > > >>>>> To: shawn...@kernel.org; s.ha...@pengutronix.de; > > > >>>>> ker...@pengutronix.de; Fabio Estevam ; > > > >>>>> mturque...@baylibre.com; sb...@kernel.org; > > > >>>>> linux-arm-ker...@lists.infradead.org; > > > >>>>> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > > > >>>>> Cc: dl-linux-imx > > > >>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on > > > >>>>> array > > > >>>>> > > > >>>>> Clock framework will enable those clocks registered with > > > >>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on array > > > >>>>> during clock > > > >>>> initialization now. > > > >>>> > > > >>>> Will it be more flexible to parse dts saying "critical-clocks = > > > >>>> " > > > >>>> or "init-on-arrary=" > > > >>>> and enable those clocks? > > > >>> > > > >>> Parsing the clocks arrays from dtb is another way of enabling > > > >>> critical clocks, but for current i.MX6/7 platforms, we implement > > > >>> it in same way as most of other SoCs, currently I did NOT see > > > >>> any necessity of putting them in dtb, just adding flag during > > > >>> clock registering is more simple, if there is any special > > > >>> requirement for different clocks set to be enabled, then we can > > > >>> add support to enable > > the method of parsing critical-clocks from dtb. Just my two cents. > > > >> > > > >> Thinking about OP-TEE want to use one device, but it's clocks are > > > >> registered by Linux, because there is no module in Linux side use > > > >> it, it will shutdown the clock, which cause OP-TEE could not > > > >> access the > > device. > > > >> > > > >> Then people have to modify clk code to add CLK_IS_CRITICAL flag > > > >> to make sure the clocks are not shutdown by Linux. > > > >> > > > >> However adding a new property in clk node and let driver code > > > >> parse the dts, there is no need to modify clk driver code when > > > >> OP-TEE needs > > another device clock. > > > >> > > > > > > > > If OP-TEE needs linux to keep things on then why can't the OP-TEE > > > > driver in Linux probe, acquire clocks, and keep the clks enabled > > > > forever? > > > > > > Sounds reasonable, but how could this be done without introducing > > > platform-specific stuff in the OP-TEE driver? > > > > > > > Why is that a goal? > > I do NOT think we should consider such case in this patch series, whatever > OP-TEE needs for its own feature, it should do necessary operations either in > its > driver or somewhere else by adding new patch. > > Anson.
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Anson Huang Best Regards! > -Original Message- > From: Stephen Boyd > Sent: Saturday, September 1, 2018 1:58 AM > To: ker...@pengutronix.de; linux-arm-ker...@lists.infradead.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > mturque...@baylibre.com; s.ha...@pengutronix.de; shawn...@kernel.org; > Anson Huang ; Fabio Estevam > ; Jerome Forissier ; > Peng Fan ; Rob Herring > Cc: dl-linux-imx > Subject: Re: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > Quoting Jerome Forissier (2018-08-31 01:01:44) > > > > > > On 08/31/2018 03:29 AM, Stephen Boyd wrote: > > > Quoting Peng Fan (2018-08-12 18:15:41) > > >> Hi Anson, > > >> > > >>>>> -Original Message- > > >>>>> From: Anson Huang > > >>>>> Sent: 2018年8月8日 12:39 > > >>>>> To: shawn...@kernel.org; s.ha...@pengutronix.de; > > >>>>> ker...@pengutronix.de; Fabio Estevam ; > > >>>>> mturque...@baylibre.com; sb...@kernel.org; > > >>>>> linux-arm-ker...@lists.infradead.org; > > >>>>> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > > >>>>> Cc: dl-linux-imx > > >>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > >>>>> > > >>>>> Clock framework will enable those clocks registered with > > >>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on array > > >>>>> during clock > > >>>> initialization now. > > >>>> > > >>>> Will it be more flexible to parse dts saying "critical-clocks = " > > >>>> or "init-on-arrary=" > > >>>> and enable those clocks? > > >>> > > >>> Parsing the clocks arrays from dtb is another way of enabling > > >>> critical clocks, but for current i.MX6/7 platforms, we implement > > >>> it in same way as most of other SoCs, currently I did NOT see any > > >>> necessity of putting them in dtb, just adding flag during clock > > >>> registering is more simple, if there is any special requirement > > >>> for different clocks set to be enabled, then we can add support to > > >>> enable > the method of parsing critical-clocks from dtb. Just my two cents. > > >> > > >> Thinking about OP-TEE want to use one device, but it's clocks are > > >> registered by Linux, because there is no module in Linux side use > > >> it, it will shutdown the clock, which cause OP-TEE could not access the > device. > > >> > > >> Then people have to modify clk code to add CLK_IS_CRITICAL flag to > > >> make sure the clocks are not shutdown by Linux. > > >> > > >> However adding a new property in clk node and let driver code parse > > >> the dts, there is no need to modify clk driver code when OP-TEE needs > another device clock. > > >> > > > > > > If OP-TEE needs linux to keep things on then why can't the OP-TEE > > > driver in Linux probe, acquire clocks, and keep the clks enabled forever? > > > > Sounds reasonable, but how could this be done without introducing > > platform-specific stuff in the OP-TEE driver? > > > > Why is that a goal? I do NOT think we should consider such case in this patch series, whatever OP-TEE needs for its own feature, it should do necessary operations either in its driver or somewhere else by adding new patch. Anson.
Re: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Quoting Jerome Forissier (2018-08-31 01:01:44) > > > On 08/31/2018 03:29 AM, Stephen Boyd wrote: > > Quoting Peng Fan (2018-08-12 18:15:41) > >> Hi Anson, > >> > > -Original Message- > > From: Anson Huang > > Sent: 2018年8月8日 12:39 > > To: shawn...@kernel.org; s.ha...@pengutronix.de; > > ker...@pengutronix.de; Fabio Estevam ; > > mturque...@baylibre.com; sb...@kernel.org; > > linux-arm-ker...@lists.infradead.org; > > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > > Cc: dl-linux-imx > > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > > > Clock framework will enable those clocks registered with > > CLK_IS_CRITICAL flag, so no need to have clks_init_on array during > > clock > initialization now. > > Will it be more flexible to parse dts saying "critical-clocks = " > or "init-on-arrary=" > and enable those clocks? > >>> > >>> Parsing the clocks arrays from dtb is another way of enabling critical > >>> clocks, but > >>> for current i.MX6/7 platforms, we implement it in same way as most of > >>> other > >>> SoCs, currently I did NOT see any necessity of putting them in dtb, just > >>> adding > >>> flag during clock registering is more simple, if there is any special > >>> requirement > >>> for different clocks set to be enabled, then we can add support to enable > >>> the > >>> method of parsing critical-clocks from dtb. Just my two cents. > >> > >> Thinking about OP-TEE want to use one device, but it's clocks are > >> registered > >> by Linux, because there is no module in Linux side use it, it will > >> shutdown the clock, > >> which cause OP-TEE could not access the device. > >> > >> Then people have to modify clk code to add CLK_IS_CRITICAL flag to make > >> sure > >> the clocks are not shutdown by Linux. > >> > >> However adding a new property in clk node and let driver code parse the > >> dts, > >> there is no need to modify clk driver code when OP-TEE needs another > >> device clock. > >> > > > > If OP-TEE needs linux to keep things on then why can't the OP-TEE driver > > in Linux probe, acquire clocks, and keep the clks enabled forever? > > Sounds reasonable, but how could this be done without introducing > platform-specific stuff in the OP-TEE driver? > Why is that a goal?
Re: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
On 08/31/2018 03:29 AM, Stephen Boyd wrote: > Quoting Peng Fan (2018-08-12 18:15:41) >> Hi Anson, >> > -Original Message- > From: Anson Huang > Sent: 2018年8月8日 12:39 > To: shawn...@kernel.org; s.ha...@pengutronix.de; > ker...@pengutronix.de; Fabio Estevam ; > mturque...@baylibre.com; sb...@kernel.org; > linux-arm-ker...@lists.infradead.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: dl-linux-imx > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > Clock framework will enable those clocks registered with > CLK_IS_CRITICAL flag, so no need to have clks_init_on array during > clock initialization now. Will it be more flexible to parse dts saying "critical-clocks = " or "init-on-arrary=" and enable those clocks? >>> >>> Parsing the clocks arrays from dtb is another way of enabling critical >>> clocks, but >>> for current i.MX6/7 platforms, we implement it in same way as most of other >>> SoCs, currently I did NOT see any necessity of putting them in dtb, just >>> adding >>> flag during clock registering is more simple, if there is any special >>> requirement >>> for different clocks set to be enabled, then we can add support to enable >>> the >>> method of parsing critical-clocks from dtb. Just my two cents. >> >> Thinking about OP-TEE want to use one device, but it's clocks are registered >> by Linux, because there is no module in Linux side use it, it will shutdown >> the clock, >> which cause OP-TEE could not access the device. >> >> Then people have to modify clk code to add CLK_IS_CRITICAL flag to make sure >> the clocks are not shutdown by Linux. >> >> However adding a new property in clk node and let driver code parse the dts, >> there is no need to modify clk driver code when OP-TEE needs another device >> clock. >> > > If OP-TEE needs linux to keep things on then why can't the OP-TEE driver > in Linux probe, acquire clocks, and keep the clks enabled forever? Sounds reasonable, but how could this be done without introducing platform-specific stuff in the OP-TEE driver? > > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Hi, Stephen Anson Huang Best Regards! > -Original Message- > From: Stephen Boyd > Sent: Friday, August 31, 2018 9:29 AM > To: ker...@pengutronix.de; linux-arm-ker...@lists.infradead.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > mturque...@baylibre.com; s.ha...@pengutronix.de; shawn...@kernel.org; > Anson Huang ; Fabio Estevam > ; Peng Fan ; Rob Herring > > Cc: dl-linux-imx > Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > Quoting Peng Fan (2018-08-12 18:15:41) > > Hi Anson, > > > > > > > -Original Message- > > > > > From: Anson Huang > > > > > Sent: 2018年8月8日 12:39 > > > > > To: shawn...@kernel.org; s.ha...@pengutronix.de; > > > > > ker...@pengutronix.de; Fabio Estevam ; > > > > > mturque...@baylibre.com; sb...@kernel.org; > > > > > linux-arm-ker...@lists.infradead.org; > > > > > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > > > > > Cc: dl-linux-imx > > > > > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > > > > > > > > > Clock framework will enable those clocks registered with > > > > > CLK_IS_CRITICAL flag, so no need to have clks_init_on array > > > > > during clock > > > > initialization now. > > > > > > > > Will it be more flexible to parse dts saying "critical-clocks = " > > > > or "init-on-arrary=" > > > > and enable those clocks? > > > > > > Parsing the clocks arrays from dtb is another way of enabling > > > critical clocks, but for current i.MX6/7 platforms, we implement it > > > in same way as most of other SoCs, currently I did NOT see any > > > necessity of putting them in dtb, just adding flag during clock > > > registering is more simple, if there is any special requirement for > > > different clocks set to be enabled, then we can add support to enable the > method of parsing critical-clocks from dtb. Just my two cents. > > > > Thinking about OP-TEE want to use one device, but it's clocks are > > registered by Linux, because there is no module in Linux side use it, > > it will shutdown the clock, which cause OP-TEE could not access the device. > > > > Then people have to modify clk code to add CLK_IS_CRITICAL flag to > > make sure the clocks are not shutdown by Linux. > > > > However adding a new property in clk node and let driver code parse > > the dts, there is no need to modify clk driver code when OP-TEE needs > another device clock. > > > > If OP-TEE needs linux to keep things on then why can't the OP-TEE driver in > Linux probe, acquire clocks, and keep the clks enabled forever? Yes, I agree with you, OP-TEE should maintain its clocks when OP-TEE is enabled. This is ONLY a concern raised by Peng, all platforms' current clock driver does NOT consider this case, so I think this patch should be fine for now? Thanks. Anson.
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Quoting Peng Fan (2018-08-12 18:15:41) > Hi Anson, > > > > > -Original Message- > > > > From: Anson Huang > > > > Sent: 2018年8月8日 12:39 > > > > To: shawn...@kernel.org; s.ha...@pengutronix.de; > > > > ker...@pengutronix.de; Fabio Estevam ; > > > > mturque...@baylibre.com; sb...@kernel.org; > > > > linux-arm-ker...@lists.infradead.org; > > > > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > > > > Cc: dl-linux-imx > > > > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array > > > > > > > > Clock framework will enable those clocks registered with > > > > CLK_IS_CRITICAL flag, so no need to have clks_init_on array during > > > > clock > > > initialization now. > > > > > > Will it be more flexible to parse dts saying "critical-clocks = " > > > or "init-on-arrary=" > > > and enable those clocks? > > > > Parsing the clocks arrays from dtb is another way of enabling critical > > clocks, but > > for current i.MX6/7 platforms, we implement it in same way as most of other > > SoCs, currently I did NOT see any necessity of putting them in dtb, just > > adding > > flag during clock registering is more simple, if there is any special > > requirement > > for different clocks set to be enabled, then we can add support to enable > > the > > method of parsing critical-clocks from dtb. Just my two cents. > > Thinking about OP-TEE want to use one device, but it's clocks are registered > by Linux, because there is no module in Linux side use it, it will shutdown > the clock, > which cause OP-TEE could not access the device. > > Then people have to modify clk code to add CLK_IS_CRITICAL flag to make sure > the clocks are not shutdown by Linux. > > However adding a new property in clk node and let driver code parse the dts, > there is no need to modify clk driver code when OP-TEE needs another device > clock. > If OP-TEE needs linux to keep things on then why can't the OP-TEE driver in Linux probe, acquire clocks, and keep the clks enabled forever?
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Hi, Peng
Anson Huang
Best Regards!
> -Original Message-
> From: Peng Fan
> Sent: Monday, August 13, 2018 9:16 AM
> To: Anson Huang ; shawn...@kernel.org;
> s.ha...@pengutronix.de; ker...@pengutronix.de; Fabio Estevam
> ; mturque...@baylibre.com; sb...@kernel.org;
> linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org;
> linux-kernel@vger.kernel.org; Rob Herring
> Cc: dl-linux-imx
> Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
>
> Hi Anson,
>
> > > > -Original Message-
> > > > From: Anson Huang
> > > > Sent: 2018年8月8日 12:39
> > > > To: shawn...@kernel.org; s.ha...@pengutronix.de;
> > > > ker...@pengutronix.de; Fabio Estevam ;
> > > > mturque...@baylibre.com; sb...@kernel.org;
> > > > linux-arm-ker...@lists.infradead.org;
> > > > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Cc: dl-linux-imx
> > > > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> > > >
> > > > Clock framework will enable those clocks registered with
> > > > CLK_IS_CRITICAL flag, so no need to have clks_init_on array during
> > > > clock
> > > initialization now.
> > >
> > > Will it be more flexible to parse dts saying "critical-clocks = "
> > > or "init-on-arrary="
> > > and enable those clocks?
> >
> > Parsing the clocks arrays from dtb is another way of enabling critical
> > clocks, but for current i.MX6/7 platforms, we implement it in same way
> > as most of other SoCs, currently I did NOT see any necessity of
> > putting them in dtb, just adding flag during clock registering is more
> > simple, if there is any special requirement for different clocks set
> > to be enabled, then we can add support to enable the method of parsing
> critical-clocks from dtb. Just my two cents.
>
> Thinking about OP-TEE want to use one device, but it's clocks are registered
> by
> Linux, because there is no module in Linux side use it, it will shutdown the
> clock,
> which cause OP-TEE could not access the device.
>
> Then people have to modify clk code to add CLK_IS_CRITICAL flag to make sure
> the clocks are not shutdown by Linux.
>
> However adding a new property in clk node and let driver code parse the dts,
> there is no need to modify clk driver code when OP-TEE needs another device
> clock.
For supporting OP-TEE or other features which has special requirement about
clock
settings, I think we can have another patch to add parsing critical clocks from
dtb in
common place of i.MX clock drivers.
Anson.
>
> Regards,
> Peng.
>
> >
> > Anson.
> >
> > >
> > > Regards,
> > > Peng.
> > >
> > > >
> > > > Signed-off-by: Anson Huang
> > > > ---
> > > > drivers/clk/imx/clk-imx7d.c | 27 ---
> > > > drivers/clk/imx/clk.h | 7 +++
> > > > 2 files changed, 15 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imx/clk-imx7d.c
> > > > b/drivers/clk/imx/clk-imx7d.c index c4518d7..076460b 100644
> > > > --- a/drivers/clk/imx/clk-imx7d.c
> > > > +++ b/drivers/clk/imx/clk-imx7d.c
> > > > @@ -379,13 +379,6 @@ static const char *pll_enet_bypass_sel[] = {
> > > > "pll_enet_main", "pll_enet_main_src static const char
> > > > *pll_audio_bypass_sel[] = { "pll_audio_main",
> > > > "pll_audio_main_src", }; static const char *pll_video_bypass_sel[]
> > > > = { "pll_video_main", "pll_video_main_src", };
> > > >
> > > > -static int const clks_init_on[] __initconst = {
> > > > - IMX7D_ARM_A7_ROOT_CLK, IMX7D_MAIN_AXI_ROOT_CLK,
> > > > - IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_IPG_ROOT_CLK,
> > > > - IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
> > > > - IMX7D_DRAM_PHYM_ALT_ROOT_CLK,
> IMX7D_DRAM_ALT_ROOT_CLK,
> > > > -};
> > > > -
> > > > static struct clk_onecell_data clk_data;
> > > >
> > > > static struct clk ** const uart_clks[] __initconst = { @@ -403,7
> > > > +396,6 @@ static void __init imx7d_clocks_init(struct device_node
> > > *ccm_node) {
> > > > struct device_node *np;
> > > > void __iomem *base;
> > > > - int i;
> > > >
> > > > clks[IMX7D_CLK_DUMMY] = imx_clk_fixed("
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Hi Anson,
> > > -Original Message-
> > > From: Anson Huang
> > > Sent: 2018年8月8日 12:39
> > > To: shawn...@kernel.org; s.ha...@pengutronix.de;
> > > ker...@pengutronix.de; Fabio Estevam ;
> > > mturque...@baylibre.com; sb...@kernel.org;
> > > linux-arm-ker...@lists.infradead.org;
> > > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Cc: dl-linux-imx
> > > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> > >
> > > Clock framework will enable those clocks registered with
> > > CLK_IS_CRITICAL flag, so no need to have clks_init_on array during
> > > clock
> > initialization now.
> >
> > Will it be more flexible to parse dts saying "critical-clocks = "
> > or "init-on-arrary="
> > and enable those clocks?
>
> Parsing the clocks arrays from dtb is another way of enabling critical
> clocks, but
> for current i.MX6/7 platforms, we implement it in same way as most of other
> SoCs, currently I did NOT see any necessity of putting them in dtb, just
> adding
> flag during clock registering is more simple, if there is any special
> requirement
> for different clocks set to be enabled, then we can add support to enable the
> method of parsing critical-clocks from dtb. Just my two cents.
Thinking about OP-TEE want to use one device, but it's clocks are registered
by Linux, because there is no module in Linux side use it, it will shutdown the
clock,
which cause OP-TEE could not access the device.
Then people have to modify clk code to add CLK_IS_CRITICAL flag to make sure
the clocks are not shutdown by Linux.
However adding a new property in clk node and let driver code parse the dts,
there is no need to modify clk driver code when OP-TEE needs another device
clock.
Regards,
Peng.
>
> Anson.
>
> >
> > Regards,
> > Peng.
> >
> > >
> > > Signed-off-by: Anson Huang
> > > ---
> > > drivers/clk/imx/clk-imx7d.c | 27 ---
> > > drivers/clk/imx/clk.h | 7 +++
> > > 2 files changed, 15 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/clk/imx/clk-imx7d.c
> > > b/drivers/clk/imx/clk-imx7d.c index c4518d7..076460b 100644
> > > --- a/drivers/clk/imx/clk-imx7d.c
> > > +++ b/drivers/clk/imx/clk-imx7d.c
> > > @@ -379,13 +379,6 @@ static const char *pll_enet_bypass_sel[] = {
> > > "pll_enet_main", "pll_enet_main_src static const char
> > > *pll_audio_bypass_sel[] = { "pll_audio_main", "pll_audio_main_src",
> > > }; static const char *pll_video_bypass_sel[] = { "pll_video_main",
> > > "pll_video_main_src", };
> > >
> > > -static int const clks_init_on[] __initconst = {
> > > - IMX7D_ARM_A7_ROOT_CLK, IMX7D_MAIN_AXI_ROOT_CLK,
> > > - IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_IPG_ROOT_CLK,
> > > - IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
> > > - IMX7D_DRAM_PHYM_ALT_ROOT_CLK, IMX7D_DRAM_ALT_ROOT_CLK,
> > > -};
> > > -
> > > static struct clk_onecell_data clk_data;
> > >
> > > static struct clk ** const uart_clks[] __initconst = { @@ -403,7
> > > +396,6 @@ static void __init imx7d_clocks_init(struct device_node
> > *ccm_node) {
> > > struct device_node *np;
> > > void __iomem *base;
> > > - int i;
> > >
> > > clks[IMX7D_CLK_DUMMY] = imx_clk_fixed("dummy", 0);
> > > clks[IMX7D_OSC_24M_CLK] = of_clk_get_by_name(ccm_node, "osc");
> > @@
> > > -466,7 +458,7 @@ static void __init imx7d_clocks_init(struct
> > > device_node
> > > *ccm_node)
> > > clks[IMX7D_PLL_SYS_MAIN_120M] =
> > > imx_clk_fixed_factor("pll_sys_main_120m", "pll_sys_main_clk", 1, 4);
> > > clks[IMX7D_PLL_DRAM_MAIN_533M] =
> > > imx_clk_fixed_factor("pll_dram_533m", "pll_dram_main_clk", 1, 2);
> > >
> > > - clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> > > imx_clk_gate_dis("pll_sys_main_480m_clk", "pll_sys_main_480m", base
> > > + 0xb0, 4);
> > > + clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> > > +imx_clk_gate_dis_flags("pll_sys_main_480m_clk",
> > > +"pll_sys_main_480m", base + 0xb0, 4, CLK_IS_CRITICAL);
> > > clks[IMX7D_PLL_SYS_MAIN_240M_CLK] =
> > > imx_clk_gate_dis("pll_sys_main_240m_clk", "pll_sys_main_240m", base
> > > + 0xb0, 5);
> > > clks[IMX7D_PLL_SYS_MAIN_120M_CLK] =
> > > imx_clk_gate_dis("pll_sys_main_120m_clk", "pll_sys_main_120m", base
> > > + 0xb0, 6);
> > > clks[IMX7D_PLL_DRAM_MAIN_533M_CLK] =
> > > imx_clk_gate("pll_dram_533m_clk", "pll_dram_533m", base + 0x70, 12);
> > > @@
> > > -719,7 +711,7 @@ static void __init imx7d_clocks_init(struct
> > > device_node
> > > *ccm_node)
> > > clks[IMX7D_ENET_AXI_ROOT_DIV] =
> > > imx_clk_divider2("enet_axi_post_div", "enet_axi_pre_div", base +
> > > 0x8900, 0,
> > 6);
> > > clks[IMX7D_NAND_USDHC_BUS_ROOT_CLK] =
> > > imx_clk_divider2("nand_usdhc_root_clk", "nand_usdhc_pre_div", base +
> > > 0x8980, 0, 6);
> > > clks[IMX7D_AHB_CHANNEL_ROOT_DIV] =
> > > imx_clk_divider2("ahb_root_clk", "ahb_pre_div", base + 0x9000, 0, 6);
> > > - clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider2("ipg_root_clk",
> > > "ahb_root_clk", base + 0x9080, 0, 2);
> > > + clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider_flags("ipg_root_clk",
> > > +"ahb
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
Anson Huang
Best Regards!
> -Original Message-
> From: Peng Fan
> Sent: Wednesday, August 8, 2018 4:49 PM
> To: Anson Huang ; shawn...@kernel.org;
> s.ha...@pengutronix.de; ker...@pengutronix.de; Fabio Estevam
> ; mturque...@baylibre.com; sb...@kernel.org;
> linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org;
> linux-kernel@vger.kernel.org; Rob Herring
> Cc: dl-linux-imx
> Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
>
>
>
> > -Original Message-
> > From: Anson Huang
> > Sent: 2018年8月8日 12:39
> > To: shawn...@kernel.org; s.ha...@pengutronix.de;
> > ker...@pengutronix.de; Fabio Estevam ;
> > mturque...@baylibre.com; sb...@kernel.org;
> > linux-arm-ker...@lists.infradead.org;
> > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
> > Cc: dl-linux-imx
> > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> >
> > Clock framework will enable those clocks registered with
> > CLK_IS_CRITICAL flag, so no need to have clks_init_on array during clock
> initialization now.
>
> Will it be more flexible to parse dts saying "critical-clocks = " or
> "init-on-arrary="
> and enable those clocks?
Parsing the clocks arrays from dtb is another way of enabling critical clocks,
but
for current i.MX6/7 platforms, we implement it in same way as most of other
SoCs,
currently I did NOT see any necessity of putting them in dtb,
just adding flag during clock registering is more simple, if there is any
special requirement
for different clocks set to be enabled, then we can add support to enable the
method of
parsing critical-clocks from dtb. Just my two cents.
Anson.
>
> Regards,
> Peng.
>
> >
> > Signed-off-by: Anson Huang
> > ---
> > drivers/clk/imx/clk-imx7d.c | 27 ---
> > drivers/clk/imx/clk.h | 7 +++
> > 2 files changed, 15 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
> > index c4518d7..076460b 100644
> > --- a/drivers/clk/imx/clk-imx7d.c
> > +++ b/drivers/clk/imx/clk-imx7d.c
> > @@ -379,13 +379,6 @@ static const char *pll_enet_bypass_sel[] = {
> > "pll_enet_main", "pll_enet_main_src static const char
> > *pll_audio_bypass_sel[] = { "pll_audio_main", "pll_audio_main_src", };
> > static const char *pll_video_bypass_sel[] = { "pll_video_main",
> > "pll_video_main_src", };
> >
> > -static int const clks_init_on[] __initconst = {
> > - IMX7D_ARM_A7_ROOT_CLK, IMX7D_MAIN_AXI_ROOT_CLK,
> > - IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_IPG_ROOT_CLK,
> > - IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
> > - IMX7D_DRAM_PHYM_ALT_ROOT_CLK, IMX7D_DRAM_ALT_ROOT_CLK,
> > -};
> > -
> > static struct clk_onecell_data clk_data;
> >
> > static struct clk ** const uart_clks[] __initconst = { @@ -403,7
> > +396,6 @@ static void __init imx7d_clocks_init(struct device_node
> *ccm_node) {
> > struct device_node *np;
> > void __iomem *base;
> > - int i;
> >
> > clks[IMX7D_CLK_DUMMY] = imx_clk_fixed("dummy", 0);
> > clks[IMX7D_OSC_24M_CLK] = of_clk_get_by_name(ccm_node, "osc");
> @@
> > -466,7 +458,7 @@ static void __init imx7d_clocks_init(struct
> > device_node
> > *ccm_node)
> > clks[IMX7D_PLL_SYS_MAIN_120M] =
> > imx_clk_fixed_factor("pll_sys_main_120m", "pll_sys_main_clk", 1, 4);
> > clks[IMX7D_PLL_DRAM_MAIN_533M] =
> > imx_clk_fixed_factor("pll_dram_533m", "pll_dram_main_clk", 1, 2);
> >
> > - clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> > imx_clk_gate_dis("pll_sys_main_480m_clk", "pll_sys_main_480m", base +
> > 0xb0, 4);
> > + clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> > +imx_clk_gate_dis_flags("pll_sys_main_480m_clk", "pll_sys_main_480m",
> > +base + 0xb0, 4, CLK_IS_CRITICAL);
> > clks[IMX7D_PLL_SYS_MAIN_240M_CLK] =
> > imx_clk_gate_dis("pll_sys_main_240m_clk", "pll_sys_main_240m", base +
> > 0xb0, 5);
> > clks[IMX7D_PLL_SYS_MAIN_120M_CLK] =
> > imx_clk_gate_dis("pll_sys_main_120m_clk", "pll_sys_main_120m", base +
> > 0xb0, 6);
> > clks[IMX7D_PLL_DRAM_MAIN_533M_CLK] =
> > imx_clk_gate("pll_dram_533m_clk", "pll_dram_533m", base + 0x70, 12);
> > @@
> > -719,7 +711,7 @@ static void __init imx7d_clocks_init(struct
> > device_node
> > *ccm_node)
> > clks[IMX7D_ENET_AXI_ROOT_DIV] =
>
RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> -Original Message-
> From: Anson Huang
> Sent: 2018年8月8日 12:39
> To: shawn...@kernel.org; s.ha...@pengutronix.de; ker...@pengutronix.de;
> Fabio Estevam ; mturque...@baylibre.com;
> sb...@kernel.org; linux-arm-ker...@lists.infradead.org;
> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: dl-linux-imx
> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
>
> Clock framework will enable those clocks registered with CLK_IS_CRITICAL flag,
> so no need to have clks_init_on array during clock initialization now.
Will it be more flexible to parse dts saying "critical-clocks = " or
"init-on-arrary="
and enable those clocks?
Regards,
Peng.
>
> Signed-off-by: Anson Huang
> ---
> drivers/clk/imx/clk-imx7d.c | 27 ---
> drivers/clk/imx/clk.h | 7 +++
> 2 files changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c index
> c4518d7..076460b 100644
> --- a/drivers/clk/imx/clk-imx7d.c
> +++ b/drivers/clk/imx/clk-imx7d.c
> @@ -379,13 +379,6 @@ static const char *pll_enet_bypass_sel[] =
> { "pll_enet_main", "pll_enet_main_src static const char
> *pll_audio_bypass_sel[] = { "pll_audio_main", "pll_audio_main_src", }; static
> const char *pll_video_bypass_sel[] = { "pll_video_main",
> "pll_video_main_src", };
>
> -static int const clks_init_on[] __initconst = {
> - IMX7D_ARM_A7_ROOT_CLK, IMX7D_MAIN_AXI_ROOT_CLK,
> - IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_IPG_ROOT_CLK,
> - IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
> - IMX7D_DRAM_PHYM_ALT_ROOT_CLK, IMX7D_DRAM_ALT_ROOT_CLK,
> -};
> -
> static struct clk_onecell_data clk_data;
>
> static struct clk ** const uart_clks[] __initconst = { @@ -403,7 +396,6 @@
> static void __init imx7d_clocks_init(struct device_node *ccm_node) {
> struct device_node *np;
> void __iomem *base;
> - int i;
>
> clks[IMX7D_CLK_DUMMY] = imx_clk_fixed("dummy", 0);
> clks[IMX7D_OSC_24M_CLK] = of_clk_get_by_name(ccm_node, "osc"); @@
> -466,7 +458,7 @@ static void __init imx7d_clocks_init(struct device_node
> *ccm_node)
> clks[IMX7D_PLL_SYS_MAIN_120M] =
> imx_clk_fixed_factor("pll_sys_main_120m", "pll_sys_main_clk", 1, 4);
> clks[IMX7D_PLL_DRAM_MAIN_533M] =
> imx_clk_fixed_factor("pll_dram_533m", "pll_dram_main_clk", 1, 2);
>
> - clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> imx_clk_gate_dis("pll_sys_main_480m_clk", "pll_sys_main_480m", base + 0xb0,
> 4);
> + clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> +imx_clk_gate_dis_flags("pll_sys_main_480m_clk", "pll_sys_main_480m",
> +base + 0xb0, 4, CLK_IS_CRITICAL);
> clks[IMX7D_PLL_SYS_MAIN_240M_CLK] =
> imx_clk_gate_dis("pll_sys_main_240m_clk", "pll_sys_main_240m", base + 0xb0,
> 5);
> clks[IMX7D_PLL_SYS_MAIN_120M_CLK] =
> imx_clk_gate_dis("pll_sys_main_120m_clk", "pll_sys_main_120m", base + 0xb0,
> 6);
> clks[IMX7D_PLL_DRAM_MAIN_533M_CLK] =
> imx_clk_gate("pll_dram_533m_clk", "pll_dram_533m", base + 0x70, 12); @@
> -719,7 +711,7 @@ static void __init imx7d_clocks_init(struct device_node
> *ccm_node)
> clks[IMX7D_ENET_AXI_ROOT_DIV] =
> imx_clk_divider2("enet_axi_post_div", "enet_axi_pre_div", base + 0x8900, 0,
> 6);
> clks[IMX7D_NAND_USDHC_BUS_ROOT_CLK] =
> imx_clk_divider2("nand_usdhc_root_clk", "nand_usdhc_pre_div", base +
> 0x8980, 0, 6);
> clks[IMX7D_AHB_CHANNEL_ROOT_DIV] =
> imx_clk_divider2("ahb_root_clk", "ahb_pre_div", base + 0x9000, 0, 6);
> - clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider2("ipg_root_clk",
> "ahb_root_clk", base + 0x9080, 0, 2);
> + clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider_flags("ipg_root_clk",
> +"ahb_root_clk", base + 0x9080, 0, 2, CLK_IS_CRITICAL |
> +CLK_OPS_PARENT_ENABLE | CLK_SET_RATE_PARENT);
> clks[IMX7D_DRAM_ROOT_DIV] = imx_clk_divider2("dram_post_div",
> "dram_cg", base + 0x9880, 0, 3);
> clks[IMX7D_DRAM_PHYM_ALT_ROOT_DIV] =
> imx_clk_divider2("dram_phym_alt_post_div", "dram_phym_alt_pre_div", base
> + 0xa000, 0, 3);
> clks[IMX7D_DRAM_ALT_ROOT_DIV] =
> imx_clk_divider2("dram_alt_post_div", "dram_alt_pre_div", base + 0xa080, 0,
> 3); @@ -783,17 +775,17 @@ static void __init imx7d_clocks_init(struct
> device_node *ccm_node)
> clks[IMX7D_CLKO1_ROOT_DIV] = imx_clk_divider2("clko1_post_div",
> "clko1_pre_div", base + 0xbd80, 0, 6);
> clks[IMX7D_CLKO2_ROOT_DIV] = imx_clk_divider2("clko2_post_div",
> "clko2_pre_div", base + 0xbe00, 0, 6);
>
> - clks[IMX7D_ARM_A7_ROOT_CLK] = imx_clk_gate4("arm_a7_root_clk",
> "arm_a7_div", base + 0x4000, 0);
> + clks[IMX7D_ARM_A7_ROOT_CLK] =
> imx_clk_gate2_flags("arm_a7_root_clk",
> +"arm_a7_div", base + 0x4000, 0, CLK_IS_CRITICAL |
> +CLK_OPS_PARENT_ENABLE);
> clks[IMX7D_ARM_M4_ROOT_CLK] = imx_clk_gate4("arm_m4_root_clk",
> "arm_m4_div", base + 0x4010, 0);
> - clks[IMX7D_MAIN_AXI_ROOT_CLK] = imx_clk_gate4("main_axi_root_clk",
> "axi_post_div", base + 0x4040, 0);
> + clks[IMX7D_MAIN_AXI_ROOT_CLK] =
> +imx_clk_g
