RE: [PATCH 3/3] ptp_qoriq: convert to use module parameters for initialization
Hi Richard, > -Original Message- > From: Richard Cochran [mailto:richardcoch...@gmail.com] > Sent: Wednesday, August 1, 2018 2:15 PM > To: Y.b. Lu > Cc: net...@vger.kernel.org; Madalin-cristian Bucur > ; Rob Herring ; Shawn Guo > ; David S . Miller ; > devicet...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org > Subject: Re: [PATCH 3/3] ptp_qoriq: convert to use module parameters for > initialization > > On Wed, Aug 01, 2018 at 04:36:40AM +, Y.b. Lu wrote: > > > Could I add a function to calculate a set of default register values > > to initialize ptp timer when dts method failed to get required > > properties in driver? > > Yes, it would be ideal if the driver can pick correct values automatically. > > However, the frequency on the FIPER outputs can't be configured > automatically, and we don't have an API for the user to choose this. [Y.b. Lu] Thanks a lot. Just let me send out the v2 patch for your reviewing. > > > I think this will be useful. The ptp timer on new platforms (you may > > see two dts patches in this patchset. Many platforms will be > > affected.) will work without these dts properties. If user want > > specific setting, they can set dts properties. > > Sure. > > Thanks, > Richard
Re: [PATCH 3/3] ptp_qoriq: convert to use module parameters for initialization
On Wed, Aug 01, 2018 at 04:36:40AM +, Y.b. Lu wrote: > Could I add a function to calculate a set of default register values > to initialize ptp timer when dts method failed to get required > properties in driver? Yes, it would be ideal if the driver can pick correct values automatically. However, the frequency on the FIPER outputs can't be configured automatically, and we don't have an API for the user to choose this. > I think this will be useful. The ptp timer on new platforms (you may > see two dts patches in this patchset. Many platforms will be > affected.) will work without these dts properties. If user want > specific setting, they can set dts properties. Sure. Thanks, Richard
RE: [PATCH 3/3] ptp_qoriq: convert to use module parameters for initialization
Hi David, > -Original Message- > From: David Miller [mailto:da...@davemloft.net] > Sent: Tuesday, July 31, 2018 12:26 AM > To: Y.b. Lu > Cc: net...@vger.kernel.org; Madalin-cristian Bucur > ; richardcoch...@gmail.com; robh...@kernel.org; > shawn...@kernel.org; devicet...@vger.kernel.org; > linuxppc-dev@lists.ozlabs.org; linux-arm-ker...@lists.infradead.org; > linux-ker...@vger.kernel.org > Subject: Re: [PATCH 3/3] ptp_qoriq: convert to use module parameters for > initialization > > From: Yangbo Lu > Date: Mon, 30 Jul 2018 18:01:54 +0800 > > > +static unsigned int cksel = DEFAULT_CKSEL; module_param(cksel, uint, > > +0644); MODULE_PARM_DESC(cksel, "Select reference clock"); > > + > > +static unsigned int clk_src; > > +module_param(clk_src, uint, 0644); > > +MODULE_PARM_DESC(clk_src, "Reference clock frequency (if clocks > > +property not provided in dts)"); > > + > > +static unsigned int tmr_prsc = 2; > > +module_param(tmr_prsc, uint, 0644); > > +MODULE_PARM_DESC(tmr_prsc, "Output clock division/prescale factor"); > > + > > +static unsigned int tmr_fiper1 = 10; module_param(tmr_fiper1, > > +uint, 0644); MODULE_PARM_DESC(tmr_fiper1, "Desired fixed interval > > +pulse period (ns)"); > > + > > +static unsigned int tmr_fiper2 = 10; module_param(tmr_fiper2, > > +uint, 0644); MODULE_PARM_DESC(tmr_fiper2, "Desired fixed interval > > +pulse period (ns)"); > > Sorry, there is no way I am every applying something like this. Module > parameters are to be avoided at all costs. > > And you don't need it here, you have DTS, please use it. > > You are required to support the existing DTS cases, in order to avoid breaking > things, anyways. [Y.b. Lu] I get your point. Will drop module_param method. Thanks a lot for your suggestion.
RE: [PATCH 3/3] ptp_qoriq: convert to use module parameters for initialization
Hi Richard, > -Original Message- > From: Richard Cochran [mailto:richardcoch...@gmail.com] > Sent: Monday, July 30, 2018 10:31 PM > To: Y.b. Lu > Cc: net...@vger.kernel.org; Madalin-cristian Bucur > ; Rob Herring ; Shawn Guo > ; David S . Miller ; > devicet...@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org > Subject: Re: [PATCH 3/3] ptp_qoriq: convert to use module parameters for > initialization > > On Mon, Jul 30, 2018 at 06:01:54PM +0800, Yangbo Lu wrote: > > The ptp_qoriq driver initialized the 1588 timer with the > > configurations provided by the properties of device tree node. For > > example, > > > > fsl,tclk-period = <5>; > > fsl,tmr-prsc= <2>; > > fsl,tmr-add = <0xaaab>; > > fsl,tmr-fiper1 = <5>; > > fsl,tmr-fiper2 = <0>; > > fsl,max-adj = <4>; > > > > These things actually were runtime configurations which were not > > proper to be put into dts. > > That is debatable. While I agree that the dts isn't ideal for these, still > it is the > lesser of two or more evils. [Y.b. Lu] Ok. You're right indeed :) > > > This patch is to convert > > to use module parameters for 1588 timer initialization, and to support > > initial register values calculation. > > It is hard for me to understand how using module parameters improves the > situation. [Y.b. Lu] Actually I'm not sure whether module_param will be accepted to replace dts. I thought the most possibility would be rejection before sending them out. Just want suggestion and confirmation whether there is better idea than dts from your comments. Since we should keep the dts, I will drop the module_param. Could I add a function to calculate a set of default register values to initialize ptp timer when dts method failed to get required properties in driver? I think this will be useful. The ptp timer on new platforms (you may see two dts patches in this patchset. Many platforms will be affected.) will work without these dts properties. If user want specific setting, they can set dts properties. > > > If the parameters are not provided, the driver will calculate register > > values with a set of default parameters. With this patch, those dts > > properties are no longer needed for new platform to support 1588 > > timer, and many QorIQ DPAA platforms (some P series and T series > > platforms of PowerPC, and some LS series platforms of ARM64) could use > > this driver for their fman ptp timer with default module parameters. > > However, this patch didn't remove the dts method. Because there were > > still many old platforms using the dts method. We need to clean up > > their dts files, verify module parameters on them, and convert them to > > the new method gradually in case of breaking any function. > > In addition, like it or not, because the dts is an ABI, you must continue > support > of the dts values as a legacy option. [Y.b. Lu] I get your point now. The dts should be kept :) > > Thanks, > Richard
Re: [PATCH 3/3] ptp_qoriq: convert to use module parameters for initialization
From: Yangbo Lu Date: Mon, 30 Jul 2018 18:01:54 +0800 > +static unsigned int cksel = DEFAULT_CKSEL; > +module_param(cksel, uint, 0644); > +MODULE_PARM_DESC(cksel, "Select reference clock"); > + > +static unsigned int clk_src; > +module_param(clk_src, uint, 0644); > +MODULE_PARM_DESC(clk_src, "Reference clock frequency (if clocks property not > provided in dts)"); > + > +static unsigned int tmr_prsc = 2; > +module_param(tmr_prsc, uint, 0644); > +MODULE_PARM_DESC(tmr_prsc, "Output clock division/prescale factor"); > + > +static unsigned int tmr_fiper1 = 10; > +module_param(tmr_fiper1, uint, 0644); > +MODULE_PARM_DESC(tmr_fiper1, "Desired fixed interval pulse period (ns)"); > + > +static unsigned int tmr_fiper2 = 10; > +module_param(tmr_fiper2, uint, 0644); > +MODULE_PARM_DESC(tmr_fiper2, "Desired fixed interval pulse period (ns)"); Sorry, there is no way I am every applying something like this. Module parameters are to be avoided at all costs. And you don't need it here, you have DTS, please use it. You are required to support the existing DTS cases, in order to avoid breaking things, anyways.
Re: [PATCH 3/3] ptp_qoriq: convert to use module parameters for initialization
On Mon, Jul 30, 2018 at 06:01:54PM +0800, Yangbo Lu wrote: > The ptp_qoriq driver initialized the 1588 timer with the > configurations provided by the properties of device tree > node. For example, > > fsl,tclk-period = <5>; > fsl,tmr-prsc= <2>; > fsl,tmr-add = <0xaaab>; > fsl,tmr-fiper1 = <5>; > fsl,tmr-fiper2 = <0>; > fsl,max-adj = <4>; > > These things actually were runtime configurations which > were not proper to be put into dts. That is debatable. While I agree that the dts isn't ideal for these, still it is the lesser of two or more evils. > This patch is to convert > to use module parameters for 1588 timer initialization, and > to support initial register values calculation. It is hard for me to understand how using module parameters improves the situation. > If the parameters are not provided, the driver will calculate > register values with a set of default parameters. With this > patch, those dts properties are no longer needed for new > platform to support 1588 timer, and many QorIQ DPAA platforms > (some P series and T series platforms of PowerPC, and some > LS series platforms of ARM64) could use this driver for their > fman ptp timer with default module parameters. However, this > patch didn't remove the dts method. Because there were still > many old platforms using the dts method. We need to clean up > their dts files, verify module parameters on them, and convert > them to the new method gradually in case of breaking any > function. In addition, like it or not, because the dts is an ABI, you must continue support of the dts values as a legacy option. Thanks, Richard
