Hi,
It is preparation serie intended to clean up and optimize TI CPTS driver to
facilitate further integration with other TI's SoCs like Keystone 2.
It also include some non critical fixes:
net: ethernet: ti: exclude cpts from build when disabled
net: ethernet: ti: cpts: fix overflow check period
net: ethernet: ti: cpts: clean up event list if event pool is empty
Grygorii Strashko (7):
net: ethernet: ti: exclude cpts from build when disabled
net: ethernet: ti: cpsw: minimize direct access to struct cpts
net: ethernet: ti: cpts: rework initialization/deinitialization
net: ethernet: ti: cpts: move dt props parsing to cpts driver
net: ethernet: ti: cpts: calc mult and shift from refclk freq
net: ethernet: ti: cpts: fix overflow check period
net: ethernet: ti: cpts: switch to readl/writel_relaxed()
WingMan Kwok (2):
net: ethernet: ti: cpts: add return value to tx and rx timestamp
funcitons
net: ethernet: ti: cpts: clean up event list if event pool is empty
Documentation/devicetree/bindings/net/cpsw.txt | 4 +-
drivers/net/ethernet/ti/Makefile | 3 +-
drivers/net/ethernet/ti/cpsw.c | 83 ++++----
drivers/net/ethernet/ti/cpsw.h | 2 -
drivers/net/ethernet/ti/cpts.c | 256 ++++++++++++++++++-------
drivers/net/ethernet/ti/cpts.h | 93 +++++++--
6 files changed, 319 insertions(+), 122 deletions(-)
--
2.9.3
@@ -2311,18 +2311,6 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,}data->active_slave=prop;-if(of_property_read_u32(node,"cpts_clock_mult",&prop)){-dev_err(&pdev->dev,"Missing cpts_clock_mult property in the DT.\n");-return-EINVAL;-}-data->cpts_clock_mult=prop;--if(of_property_read_u32(node,"cpts_clock_shift",&prop)){-dev_err(&pdev->dev,"Missing cpts_clock_shift property in the DT.\n");-return-EINVAL;-}-data->cpts_clock_shift=prop;-data->slave_data=devm_kzalloc(&pdev->dev,data->slaves*sizeof(structcpsw_slave_data),GFP_KERNEL);
@@ -2742,9 +2730,7 @@ static int cpsw_probe(struct platform_device *pdev)gotoclean_dma_ret;}-cpsw->cpts=cpts_create(cpsw->dev,cpts_regs,-cpsw->data.cpts_clock_mult,-cpsw->data.cpts_clock_shift);+cpsw->cpts=cpts_create(cpsw->dev,cpts_regs,cpsw->dev->of_node);if(IS_ERR(cpsw->cpts)){ret=PTR_ERR(cpsw->cpts);gotoclean_ale_ret;
@@ -31,8 +31,6 @@ struct cpsw_platform_data {u32channels;/* number of cpdma channels (symmetric) */u32slaves;/* number of slave cpgmac ports */u32active_slave;/* time stamping, ethtool and SIOCGMIIPHY slave */-u32cpts_clock_mult;/* convert input clock ticks to nanoseconds */-u32cpts_clock_shift;/* convert input clock ticks to nanoseconds */u32ale_entries;/* ale table size */u32bd_ram_size;/*buffer descriptor ram size */u32mac_control;/* Mac control register */
Switch to readl/writel_relaxed() APIs, because The CPTS IP is reused
on Keystone 2 SoCs where LE/BE modes are supported.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
The CPTS drivers uses 8sec period for overflow checking with
assumption that CPTS rftclk will not exceed 500MHz. But that's not
true on some TI's platforms (Kesytone 2). As result, it is possible that
CPTS counter will overflow more than once between two readings.
Hence, fix it by selecting overflow check period dynamically as
max_sec_before_overflow/2, where
max_sec_before_overflow = max_counter_val / rftclk_freq.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 16 +++++++++++-----
drivers/net/ethernet/ti/cpts.h | 4 +---
2 files changed, 12 insertions(+), 8 deletions(-)
@@ -391,7 +391,7 @@ int cpts_register(struct cpts *cpts)}cpts->phc_index=ptp_clock_index(cpts->clock);-schedule_delayed_work(&cpts->overflow_work,CPTS_OVERFLOW_PERIOD);+schedule_delayed_work(&cpts->overflow_work,cpts->ov_check_period);return0;err_ptp:
@@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)u64ns;u64frac;-if(cpts->cc_mult||cpts->cc.shift)-return;-freq=clk_get_rate(cpts->refclk);/* Calc the maximum number of seconds which we can run before
@@ -442,11 +439,20 @@ static void cpts_calc_mult_shift(struct cpts *cpts)elseif(maxsec>600&&cpts->cc.mask>UINT_MAX)maxsec=600;+/* Calc overflow check period (maxsec / 2) */+cpts->ov_check_period=(HZ*maxsec)/2;+dev_info(cpts->dev,"cpts: overflow check period %lu\n",+cpts->ov_check_period);++if(cpts->cc_mult||cpts->cc.shift)+return;+clocks_calc_mult_shift(&mult,&shift,freq,NSEC_PER_SEC,maxsec);cpts->cc_mult=mult;cpts->cc.mult=mult;cpts->cc.shift=shift;+/* Check calculations and inform if not precise */frac=0;ns=cyclecounter_cyc2ns(&cpts->cc,freq,cpts->cc.mask,&frac);
The cyclecounter mult and shift values can be calculated based on the
CPTS rftclk frequency and timekeepnig framework provides required algos
and API's.
Hence, calc mult and shift basing on CPTS rftclk frequency if both
cpts_clock_shift and cpts_clock_mult properties are not provided in DT
(the basis of calculation algorithm is borrowed from
__clocksource_update_freq_scale()). After this change cpts_clock_shift
and cpts_clock_mult DT properties will become optional.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
Documentation/devicetree/bindings/net/cpsw.txt | 4 +-
drivers/net/ethernet/ti/cpts.c | 56 +++++++++++++++++++++++---
2 files changed, 52 insertions(+), 8 deletions(-)
@@ -20,8 +20,6 @@ Required properties: - slaves : Specifies number for slaves - active_slave : Specifies the slave to use for time stamping, ethtool and SIOCGMIIPHY-- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds-- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds Optional properties: - ti,hwmods : Must be "cpgmac0"
@@ -35,6 +33,8 @@ Optional properties: For example in dra72x-evm, pcf gpio has to be driven low so that cpsw slave 0 and phy data lines are connected via mux.+- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds+- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds Slave Properties:
@@ -418,18 +418,60 @@ void cpts_unregister(struct cpts *cpts)clk_disable(cpts->refclk);}+staticvoidcpts_calc_mult_shift(structcpts*cpts)+{+u64maxsec;+u32freq;+u32mult;+u32shift;+u64ns;+u64frac;++if(cpts->cc_mult||cpts->cc.shift)+return;++freq=clk_get_rate(cpts->refclk);++/* Calc the maximum number of seconds which we can run before+*wrappingaround.+*/+maxsec=cpts->cc.mask;+do_div(maxsec,freq);+if(!maxsec)+maxsec=1;+elseif(maxsec>600&&cpts->cc.mask>UINT_MAX)+maxsec=600;++clocks_calc_mult_shift(&mult,&shift,freq,NSEC_PER_SEC,maxsec);++cpts->cc_mult=mult;+cpts->cc.mult=mult;+cpts->cc.shift=shift;+/* Check calculations and inform if not precise */+frac=0;+ns=cyclecounter_cyc2ns(&cpts->cc,freq,cpts->cc.mask,&frac);++dev_info(cpts->dev,+"CPTS: ref_clk_freq:%u calc_mult:%u calc_shift:%u error:%lld nsec/sec\n",+freq,cpts->cc_mult,cpts->cc.shift,(ns-NSEC_PER_SEC));+}+staticintcpts_of_parse(structcpts*cpts,structdevice_node*node){intret=-EINVAL;u32prop;-if(of_property_read_u32(node,"cpts_clock_mult",&prop))-gotoof_error;-cpts->cc_mult=prop;+cpts->cc_mult=0;+if(!of_property_read_u32(node,"cpts_clock_mult",&prop))+cpts->cc_mult=prop;-if(of_property_read_u32(node,"cpts_clock_shift",&prop))-gotoof_error;-cpts->cc.shift=prop;+cpts->cc.shift=0;+if(!of_property_read_u32(node,"cpts_clock_shift",&prop))+cpts->cc.shift=prop;++if((cpts->cc_mult&&!cpts->cc.shift)||+(!cpts->cc_mult&&cpts->cc.shift))+gotoof_error;return0;
From: WingMan Kwok <redacted>
When a CPTS user does not exit gracefully by disabling cpts
timestamping and leaving a joined multicast group, the system
continues to receive and timestamps the ptp packets which eventually
occupy all the event list entries. When this happns, the added code
tries to remove some list entries which are expired.
Signed-off-by: WingMan Kwok <redacted>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
@@ -57,22 +57,48 @@ static int cpts_fifo_pop(struct cpts *cpts, u32 *high, u32 *low)return-1;}+staticintcpts_event_list_clean_up(structcpts*cpts)+{+structlist_head*this,*next;+structcpts_event*event;+intremoved=0;++list_for_each_safe(this,next,&cpts->events){+event=list_entry(this,structcpts_event,list);+if(event_expired(event)){+list_del_init(&event->list);+list_add(&event->list,&cpts->pool);+++removed;+}+}+returnremoved;+}+/**Returnszeroifmatchingeventtypewasfound.*/staticintcpts_fifo_read(structcpts*cpts,intmatch){inti,type=-1;+intremoved;u32hi,lo;structcpts_event*event;for(i=0;i<CPTS_FIFO_DEPTH;i++){if(cpts_fifo_pop(cpts,&hi,&lo))break;+if(list_empty(&cpts->pool)){-pr_err("cpts: event pool is empty\n");-return-1;+removed=cpts_event_list_clean_up(cpts);+if(!removed){+dev_err(cpts->dev,+"cpts: event pool is empty\n");+return-1;+}+dev_dbg(cpts->dev,+"cpts: event pool cleaned up %d\n",removed);}+event=list_first_entry(&cpts->pool,structcpts_event,list);event->tmo=jiffies+2;event->high=hi;
The current implementation CPTS initialization and deinitialization
(represented by cpts_register/unregister()) is pretty entangled and
has some issues, like:
- ptp clock registered before spinlock, which is protecting it, and
before timecounter and cyclecounter initialization;
- CPTS ref_clk requested using devm API while cpts_register() is
called from .ndo_open(), as result additional checks required;
- CPTS ref_clk is prepared, but never unprepared;
- CPTS is not disabled even when unregistered..
Hence, make things simpler and fix above issues by adding
cpts_create()/cpts_release() which should be called from
.probe()/.remove() respectively and move all static initialization
there. Clean up and update cpts_register/unregister() so PTP clock is
registered the last and unregistered first. In addition, this change
allows to clean up cpts.h for the case when CPTS is disabled.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpsw.c | 24 ++++----
drivers/net/ethernet/ti/cpts.c | 125 ++++++++++++++++++++++++++---------------
drivers/net/ethernet/ti/cpts.h | 26 +++++++--
3 files changed, 113 insertions(+), 62 deletions(-)
This will provide more flexibility in changing CPTS internals and also
required for further changes.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpsw.c | 28 +++++++++++++++-------------
drivers/net/ethernet/ti/cpts.h | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+), 13 deletions(-)
TI CPTS feature declared as optional, but cpts.c module is
included in build always.
Exclude cpts.c from build when CPTS is disabled in Kconfig and
optimize usage of CONFIG_TI_CPTS.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/Makefile | 3 ++-
drivers/net/ethernet/ti/cpsw.c | 21 ++++++++++++++++-----
drivers/net/ethernet/ti/cpts.c | 8 --------
drivers/net/ethernet/ti/cpts.h | 14 ++++++++++++--
4 files changed, 30 insertions(+), 16 deletions(-)
From: Richard Cochran <richardcochran@gmail.com> Date: 2016-09-14 14:00:33
On Wed, Sep 14, 2016 at 04:02:27PM +0300, Grygorii Strashko wrote:
From: WingMan Kwok <redacted>
Added return values in tx and rx timestamp funcitons facilitate the
possibililies of timestamping by CPSW modules other than CPTS, such as
packet accelerator on Keystone 2 devices.
From: Richard Cochran <richardcochran@gmail.com> Date: 2016-09-14 14:14:23
On Wed, Sep 14, 2016 at 04:02:28PM +0300, Grygorii Strashko wrote:
quoted hunk
From: WingMan Kwok <redacted>
When a CPTS user does not exit gracefully by disabling cpts
timestamping and leaving a joined multicast group, the system
continues to receive and timestamps the ptp packets which eventually
occupy all the event list entries. When this happns, the added code
tries to remove some list entries which are expired.
Signed-off-by: WingMan Kwok <redacted>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
5 words, that is quite a mouth full. How about this instead?
static int cpts_purge_events(struct cpts *cpts);
quoted hunk
+{+ struct list_head *this, *next;+ struct cpts_event *event;+ int removed = 0;++ list_for_each_safe(this, next, &cpts->events) {+ event = list_entry(this, struct cpts_event, list);+ if (event_expired(event)) {+ list_del_init(&event->list);+ list_add(&event->list, &cpts->pool);+ ++removed;+ }+ }+ return removed;+}+ /* * Returns zero if matching event type was found. */ static int cpts_fifo_read(struct cpts *cpts, int match) { int i, type = -1;+ int removed;
No need for another variable, just change the return code above to
return removed ? 0 : -1;
and then you have ...
quoted hunk
u32 hi, lo; struct cpts_event *event; for (i = 0; i < CPTS_FIFO_DEPTH; i++) { if (cpts_fifo_pop(cpts, &hi, &lo)) break;+ if (list_empty(&cpts->pool)) {- pr_err("cpts: event pool is empty\n");- return -1;+ removed = cpts_event_list_clean_up(cpts);+ if (!removed) {+ dev_err(cpts->dev,+ "cpts: event pool is empty\n");+ return -1;+ }
if (cpts_purge_events(cpts)) {
dev_err(cpts->dev, "cpts: event pool empty\n");
return -1;
}
Notice how I avoided the ugly line break?
From: Richard Cochran <richardcochran@gmail.com> Date: 2016-09-14 14:22:22
On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote:
quoted hunk
@@ -35,6 +33,8 @@ Optional properties: For example in dra72x-evm, pcf gpio has to be driven low so that cpsw slave 0 and phy data lines are connected via mux.+- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds+- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds
You should explain to the reader how these will be calculated when the
properties are missing.
Why so many new lines? This isn't good style. Please combine
variables of the same type into one line and sort the lists
alphabetically.
quoted hunk
+ if (cpts->cc_mult || cpts->cc.shift)+ return;++ freq = clk_get_rate(cpts->refclk);++ /* Calc the maximum number of seconds which we can run before+ * wrapping around.+ */+ maxsec = cpts->cc.mask;+ do_div(maxsec, freq);+ if (!maxsec)+ maxsec = 1;+ else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)+ maxsec = 600;++ clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);++ cpts->cc_mult = mult;+ cpts->cc.mult = mult;+ cpts->cc.shift = shift;+ /* Check calculations and inform if not precise */
Contrary to this comment, you are not making any kind of judgment
about whether the calculations are precise or not.
From: Richard Cochran <richardcochran@gmail.com> Date: 2016-09-14 14:25:17
On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
quoted hunk
@@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts) u64 ns; u64 frac;- if (cpts->cc_mult || cpts->cc.shift)- return;- freq = clk_get_rate(cpts->refclk); /* Calc the maximum number of seconds which we can run before
You have more (+) than (-). I wouldn't call that a simplification.
With this change It will not be required to add the same DT parsing code
in Keystone 2 netcp driver, so overall number of lines will be reduced.
As I mentioned in cover letter - this is preparation step and this series
indented help and simplify CPTS integration with few TI net drivers
(now cpsw, soon KS2 netcp), because similar CPTS IP is integrated
in two different TI's SoC families (and might be in more the future).
I've not posted KS2 patches here, because of two reasons:
- they are still under internal review
- I hate big patch series
--
regards,
-grygorii
On Wed, Sep 14, 2016 at 04:02:28PM +0300, Grygorii Strashko wrote:
quoted
From: WingMan Kwok <redacted>
When a CPTS user does not exit gracefully by disabling cpts
timestamping and leaving a joined multicast group, the system
continues to receive and timestamps the ptp packets which eventually
occupy all the event list entries. When this happns, the added code
tries to remove some list entries which are expired.
Signed-off-by: WingMan Kwok <redacted>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
5 words, that is quite a mouth full. How about this instead?
static int cpts_purge_events(struct cpts *cpts);
quoted
+{+ struct list_head *this, *next;+ struct cpts_event *event;+ int removed = 0;++ list_for_each_safe(this, next, &cpts->events) {+ event = list_entry(this, struct cpts_event, list);+ if (event_expired(event)) {+ list_del_init(&event->list);+ list_add(&event->list, &cpts->pool);+ ++removed;+ }+ }+ return removed;+}+ /* * Returns zero if matching event type was found. */ static int cpts_fifo_read(struct cpts *cpts, int match) { int i, type = -1;+ int removed;
No need for another variable, just change the return code above to
return removed ? 0 : -1;
and then you have ...
quoted
u32 hi, lo; struct cpts_event *event; for (i = 0; i < CPTS_FIFO_DEPTH; i++) { if (cpts_fifo_pop(cpts, &hi, &lo)) break;+ if (list_empty(&cpts->pool)) {- pr_err("cpts: event pool is empty\n");- return -1;+ removed = cpts_event_list_clean_up(cpts);+ if (!removed) {+ dev_err(cpts->dev,+ "cpts: event pool is empty\n");+ return -1;+ }
if (cpts_purge_events(cpts)) {
dev_err(cpts->dev, "cpts: event pool empty\n");
return -1;
}
Notice how I avoided the ugly line break?
On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote:
quoted
@@ -35,6 +33,8 @@ Optional properties: For example in dra72x-evm, pcf gpio has to be driven low so that cpsw slave 0 and phy data lines are connected via mux.+- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds+- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds
You should explain to the reader how these will be calculated when the
properties are missing.
Not sure how full should it be explained in bindings - I'll try.
Why so many new lines? This isn't good style. Please combine
variables of the same type into one line and sort the lists
alphabetically.
Its matter of preferences :), but sure - I'll update.
quoted
+ if (cpts->cc_mult || cpts->cc.shift)+ return;++ freq = clk_get_rate(cpts->refclk);++ /* Calc the maximum number of seconds which we can run before+ * wrapping around.+ */+ maxsec = cpts->cc.mask;+ do_div(maxsec, freq);+ if (!maxsec)+ maxsec = 1;+ else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)+ maxsec = 600;++ clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);++ cpts->cc_mult = mult;+ cpts->cc.mult = mult;+ cpts->cc.shift = shift;+ /* Check calculations and inform if not precise */
Contrary to this comment, you are not making any kind of judgment
about whether the calculations are precise or not.
On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
quoted
@@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts) u64 ns; u64 frac;- if (cpts->cc_mult || cpts->cc.shift)- return;- freq = clk_get_rate(cpts->refclk); /* Calc the maximum number of seconds which we can run before
This hunk has nothing to do with $subject.
Sry, but I did not get your comment here :(
I'd happy to update patch according to your request, but could you provide more info here, pls?
From: Richard Cochran <richardcochran@gmail.com> Date: 2016-09-14 20:08:53
On Wed, Sep 14, 2016 at 11:03:18PM +0300, Grygorii Strashko wrote:
On 09/14/2016 05:25 PM, Richard Cochran wrote:
quoted
On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
quoted
@@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts) u64 ns; u64 frac;- if (cpts->cc_mult || cpts->cc.shift)- return;- freq = clk_get_rate(cpts->refclk); /* Calc the maximum number of seconds which we can run before
This hunk has nothing to do with $subject.
Sry, but I did not get your comment here :(
I'd happy to update patch according to your request, but could you provide more info here, pls?
You added that code in patch #7. Then you moved it in patch #8. You
could have made the code correct in patch #7 to begin with.
Not hot path, but still silly. The caller should never pass NULL.
Ok. I can't say I'd like all this checks, but there are internal requirement
to allow CPTS to be disabled though DT on KS2 (even if built in).
I'd try to clarify and return back here.
But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?
--
regards,
-grygorii
On Wed, Sep 14, 2016 at 11:03:18PM +0300, Grygorii Strashko wrote:
quoted
On 09/14/2016 05:25 PM, Richard Cochran wrote:
quoted
On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
quoted
@@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts) u64 ns; u64 frac;- if (cpts->cc_mult || cpts->cc.shift)- return;- freq = clk_get_rate(cpts->refclk); /* Calc the maximum number of seconds which we can run before
This hunk has nothing to do with $subject.
Sry, but I did not get your comment here :(
I'd happy to update patch according to your request, but could you provide more info here, pls?
You added that code in patch #7. Then you moved it in patch #8. You
could have made the code correct in patch #7 to begin with.
Do you mean
- if (cpts->cc_mult || cpts->cc.shift)
- return;
??
if yes then those changes are correct as from patch#7 point of
view, as from patch#8 because they are separate standalone changes.
In patch patch#7 it reasonable to ball out earlier, while in patch#8
it required to move forward a bit as I need to know maxsec.
Sry, that I'm bothering you with stupid questions.
--
regards,
-grygorii
From: Richard Cochran <richardcochran@gmail.com> Date: 2016-09-14 20:26:27
On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote:
quoted hunk
+static void cpts_calc_mult_shift(struct cpts *cpts)+{+ u64 maxsec;+ u32 freq;+ u32 mult;+ u32 shift;+ u64 ns;+ u64 frac;++ if (cpts->cc_mult || cpts->cc.shift)+ return;++ freq = clk_get_rate(cpts->refclk);++ /* Calc the maximum number of seconds which we can run before+ * wrapping around.+ */+ maxsec = cpts->cc.mask;+ do_div(maxsec, freq);+ if (!maxsec)+ maxsec = 1;
This doesn't pass the smell test. If the max counter value is M, you
are figuring M*1/F which is the time in seconds corresponding to M.
We set M=2^32-1, and so 'freq' would have to be greater than 4 GHz in
order for 'maxsec' to be zero. Do you really expect such high
frequency input clocks?
In order to get good resolution on the frequency adjustment, we want
to keep 'mult' as large as possible. I don't see your code doing
this. We can rely on the watchdog reader (work queue) to prevent
overflows.
Thanks,
Richard
From: Richard Cochran <richardcochran@gmail.com> Date: 2016-09-14 20:32:23
On Wed, Sep 14, 2016 at 11:10:48PM +0300, Grygorii Strashko wrote:
On 09/14/2016 04:52 PM, Richard Cochran wrote:
quoted
On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
quoted
quoted
- if (!cpts->rx_enable)+ if (!cpts || !cpts->rx_enable) return;
Ok. I can't say I'd like all this checks, but there are internal requirement
to allow CPTS to be disabled though DT on KS2 (even if built in).
I'd try to clarify and return back here.
But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?
Look at that code snippet. We already test for @cpts->rx_enable. If
you want to disable cpts at run time, just avoid setting that field.
There is no need for any new tests or return codes.
Thanks,
Richard
On Wed, Sep 14, 2016 at 11:10:48PM +0300, Grygorii Strashko wrote:
quoted
On 09/14/2016 04:52 PM, Richard Cochran wrote:
quoted
On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
quoted
quoted
quoted
- if (!cpts->rx_enable)+ if (!cpts || !cpts->rx_enable) return;
quoted
Ok. I can't say I'd like all this checks, but there are internal requirement
to allow CPTS to be disabled though DT on KS2 (even if built in).
I'd try to clarify and return back here.
But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?
Look at that code snippet. We already test for @cpts->rx_enable. If
you want to disable cpts at run time, just avoid setting that field.
There is no need for any new tests or return codes.
The problem is that if cpts not initialized than pinter on
cpts (in consumer/parent driver - NETCP) will be NULL.
So, rx_enable will be unaccessible and cause crash :(
--
regards,
-grygorii
From: Richard Cochran <richardcochran@gmail.com> Date: 2016-09-14 20:43:14
On Wed, Sep 14, 2016 at 11:23:43PM +0300, Grygorii Strashko wrote:
if yes then those changes are correct as from patch#7 point of
view, as from patch#8 because they are separate standalone changes.
In patch patch#7 it reasonable to ball out earlier, while in patch#8
it required to move forward a bit as I need to know maxsec.
And what about the extra blank line? AFAICT, placing the test later
in patch #7 is correct logic and has the advantage of not distracting
reviews with pointless churn!
Thanks,
Richard
On Wed, Sep 14, 2016 at 11:23:43PM +0300, Grygorii Strashko wrote:
quoted
if yes then those changes are correct as from patch#7 point of
view, as from patch#8 because they are separate standalone changes.
In patch patch#7 it reasonable to ball out earlier, while in patch#8
it required to move forward a bit as I need to know maxsec.
And what about the extra blank line? AFAICT, placing the test later
in patch #7 is correct logic and has the advantage of not distracting
reviews with pointless churn!
On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote:
quoted
+static void cpts_calc_mult_shift(struct cpts *cpts)+{+ u64 maxsec;+ u32 freq;+ u32 mult;+ u32 shift;+ u64 ns;+ u64 frac;++ if (cpts->cc_mult || cpts->cc.shift)+ return;++ freq = clk_get_rate(cpts->refclk);++ /* Calc the maximum number of seconds which we can run before+ * wrapping around.+ */+ maxsec = cpts->cc.mask;+ do_div(maxsec, freq);+ if (!maxsec)+ maxsec = 1;
This doesn't pass the smell test. If the max counter value is M, you
are figuring M*1/F which is the time in seconds corresponding to M.
We set M=2^32-1, and so 'freq' would have to be greater than 4 GHz in
order for 'maxsec' to be zero. Do you really expect such high
frequency input clocks?
In order to get good resolution on the frequency adjustment, we want
to keep 'mult' as large as possible. I don't see your code doing
this. We can rely on the watchdog reader (work queue) to prevent
overflows.
As I understand (and tested), clocks_calc_mult_shift() will
return max possible mult which can be used without overflow.
if calculated results do not satisfy end user - the custom values can
be passed in DT.
--
regards,
-grygorii
On Wed, Sep 14, 2016 at 11:37:45PM +0300, Grygorii Strashko wrote:
quoted
The problem is that if cpts not initialized than pinter on
cpts (in consumer/parent driver - NETCP) will be NULL.
You made that problem with your "clean up" in this series.
Previously, cpts was always allocated.
not exactly - it was allocated in CPSW .probe() manually,
in without this re-work it has to be allocated in NTCP the
same way - manually. So, checks we are discussing here will be still
present, but they will need to be done in CPSW/NETCP drivers -
just one level up and duplicated in both these drivers.
quoted
So, rx_enable will be unaccessible and cause crash :(
So just make sure cpts is initialized.
OK. I'll update code this way.
--
regards,
-grygorii
From: Richard Cochran <richardcochran@gmail.com> Date: 2016-09-14 21:03:40
On Wed, Sep 14, 2016 at 11:47:46PM +0300, Grygorii Strashko wrote:
As I understand (and tested), clocks_calc_mult_shift() will
return max possible mult which can be used without overflow.
Yes, BUT the returned values depends on the @maxsec input. As the
kerneldec says,
* Larger ranges may reduce the conversion accuracy by chosing smaller
* mult and shift factors.
In addition to that, frequency tuning by calculating a percentage of
'mult', and if 'mult' is small, then the tuning resolution is poor.
So we don't want @maxsec as large as possible, but as small as
possible.
if calculated results do not satisfy end user - the custom values can
be passed in DT.
If we calculate automatically, then the result had better well be
optimal or nearly so. Otherwise, we should leave it as a manual input
via DTS, IMHO, so that someone is forced to check the values.
Thanks,
Richard
On Wed, Sep 14, 2016 at 11:47:46PM +0300, Grygorii Strashko wrote:
quoted
As I understand (and tested), clocks_calc_mult_shift() will
return max possible mult which can be used without overflow.
Yes, BUT the returned values depends on the @maxsec input. As the
kerneldec says,
* Larger ranges may reduce the conversion accuracy by chosing smaller
* mult and shift factors.
In addition to that, frequency tuning by calculating a percentage of
'mult', and if 'mult' is small, then the tuning resolution is poor.
So we don't want @maxsec as large as possible, but as small as
possible.
Ok. So, will it work if maxsec will be ~= (maxsec / 2) + 1?
+ 1 to cover potential delays of overflow check work execution.
[...]
--
regards,
-grygorii
From: Richard Cochran <richardcochran@gmail.com> Date: 2016-09-15 08:13:50
On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
The current implementation CPTS initialization and deinitialization
(represented by cpts_register/unregister()) is pretty entangled and
has some issues, like:
- ptp clock registered before spinlock, which is protecting it, and
before timecounter and cyclecounter initialization;
- CPTS ref_clk requested using devm API while cpts_register() is
called from .ndo_open(), as result additional checks required;
- CPTS ref_clk is prepared, but never unprepared;
- CPTS is not disabled even when unregistered..
This list of four items is a clear sign that this one patch should be
broken into a series of four.
Thanks,
Richard
In order to get good resolution on the frequency adjustment, we want
to keep 'mult' as large as possible. I don't see your code doing
this. We can rely on the watchdog reader (work queue) to prevent
overflows.
I took a closer look, and assuming cc.mask = 2^32 - 1, then using
clocks_calc_mult_shift() produces good results for a reasonable range
of input frequencies. Keeping 'maxsec' constant at 4 we have:
| Freq. MHz | mult | shift |
|-----------+------------+-------|
| 100 | 0xa0000000 | 28 |
| 250 | 0x80000000 | 29 |
| 500 | 0x80000000 | 30 |
| 1000 | 0x80000000 | 31 |
Can the input clock be higher than 1 GHz? If not, I suggest using
clocks_calc_mult_shift() with maxsec=4 and a setting the watchdog also
to 4*HZ.
Thanks,
Richard
From: Richard Cochran <richardcochran@gmail.com> Date: 2016-09-15 13:49:55
On Thu, Sep 15, 2016 at 01:58:15PM +0200, Richard Cochran wrote:
Can the input clock be higher than 1 GHz? If not, I suggest using
clocks_calc_mult_shift() with maxsec=4 and a setting the watchdog also
to 4*HZ.
On second thought, with the new 12% timer batching, using 4*HZ for 32
bits of 1 GHz is cutting it too close. So just keep it like you had
it, with maxsec=mask/freq and timeout=maxsec/2, to stay on the safe
side.
Thanks,
Richard