[PATCH net 1/3] cpsw/netcp: cpts depends on posix_timers

[PATCH net 1/3] cpsw/netcp: cpts depends on posix_timers

[Arnd Bergmann]

With posix timers having become optional, we get a build error with
the cpts time sync option of the CPSW driver:

drivers/net/ethernet/ti/cpts.c: In function 'cpts_find_ts':
drivers/net/ethernet/ti/cpts.c:291:23: error: implicit declaration of function 'ptp_classify_raw';did you mean 'ptp_classifier_init'? [-Werror=implicit-function-declaration]

It really makes no sense to build this driver if we can't use PTP,
so it's better to go back to 'select PTP_1588_CLOCK' but instead
add a dependency on POSIX_TIMERS.

Fixes: baa73d9e478f ("posix-timers: Make them configurable")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/ti/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
index 296c8efd0038..366e29ff8605 100644
--- a/drivers/net/ethernet/ti/Kconfig
+++ b/drivers/net/ethernet/ti/Kconfig
@@ -76,7 +76,8 @@ config TI_CPSW
 config TI_CPTS
 	tristate "TI Common Platform Time Sync (CPTS) Support"
 	depends on TI_CPSW || TI_KEYSTONE_NETCP
-	imply PTP_1588_CLOCK
+	depends on POSIX_TIMERS
+	select PTP_1588_CLOCK
 	---help---
 	  This driver supports the Common Platform Time Sync unit of
 	  the CPSW Ethernet Switch and Keystone 2 1g/10g Switch Subsystem.
-- 
2.9.0

[PATCH net 2/3] cpsw/netcp: davinci_cpdma: sanitize inter-module API

[Arnd Bergmann]

The davinci_cpdma module is a helper library that is used by the
actual device drivers and does nothing by itself, so all its API
functions need to be exported.

Four new functions were added recently without an export, so now
we get a build error:

ERROR: "cpdma_chan_set_weight" [drivers/net/ethernet/ti/ti_cpsw.ko] undefined!
ERROR: "cpdma_chan_get_rate" [drivers/net/ethernet/ti/ti_cpsw.ko] undefined!
ERROR: "cpdma_chan_get_min_rate" [drivers/net/ethernet/ti/ti_cpsw.ko] undefined!
ERROR: "cpdma_chan_set_rate" [drivers/net/ethernet/ti/ti_cpsw.ko] undefined!

This exports those symbols. After taking a closer look, I found two
more global functions in this file that are not exported and not used
anywhere, so they can obviously be removed. There is also one function
that is only used internally in this module, and should be 'static'.

Fixes: 8f32b90981dc ("net: ethernet: ti: davinci_cpdma: add set rate for a channel")
Fixes: 0fc6432cc78d ("net: ethernet: ti: davinci_cpdma: add weight function for channels")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/ti/davinci_cpdma.c | 59 ++++++++++-----------------------
 drivers/net/ethernet/ti/davinci_cpdma.h |  4 ---
 2 files changed, 17 insertions(+), 46 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 36518fc5c7cc..b9d40f0cdf6c 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -628,6 +628,23 @@ int cpdma_ctlr_destroy(struct cpdma_ctlr *ctlr)
 }
 EXPORT_SYMBOL_GPL(cpdma_ctlr_destroy);
 
+static int cpdma_chan_int_ctrl(struct cpdma_chan *chan, bool enable)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&chan->lock, flags);
+	if (chan->state != CPDMA_STATE_ACTIVE) {
+		spin_unlock_irqrestore(&chan->lock, flags);
+		return -EINVAL;
+	}
+
+	dma_reg_write(chan->ctlr, enable ? chan->int_set : chan->int_clear,
+		      chan->mask);
+	spin_unlock_irqrestore(&chan->lock, flags);
+
+	return 0;
+}
+
 int cpdma_ctlr_int_ctrl(struct cpdma_ctlr *ctlr, bool enable)
 {
 	unsigned long flags;
@@ -1274,46 +1291,4 @@ int cpdma_chan_stop(struct cpdma_chan *chan)
 }
 EXPORT_SYMBOL_GPL(cpdma_chan_stop);
 
-int cpdma_chan_int_ctrl(struct cpdma_chan *chan, bool enable)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&chan->lock, flags);
-	if (chan->state != CPDMA_STATE_ACTIVE) {
-		spin_unlock_irqrestore(&chan->lock, flags);
-		return -EINVAL;
-	}
-
-	dma_reg_write(chan->ctlr, enable ? chan->int_set : chan->int_clear,
-		      chan->mask);
-	spin_unlock_irqrestore(&chan->lock, flags);
-
-	return 0;
-}
-
-int cpdma_control_get(struct cpdma_ctlr *ctlr, int control)
-{
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&ctlr->lock, flags);
-	ret = _cpdma_control_get(ctlr, control);
-	spin_unlock_irqrestore(&ctlr->lock, flags);
-
-	return ret;
-}
-
-int cpdma_control_set(struct cpdma_ctlr *ctlr, int control, int value)
-{
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&ctlr->lock, flags);
-	ret = _cpdma_control_set(ctlr, control, value);
-	spin_unlock_irqrestore(&ctlr->lock, flags);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(cpdma_control_set);
-
 MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.h b/drivers/net/ethernet/ti/davinci_cpdma.h
index 4a167db2abab..36d0a09a3d44 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.h
+++ b/drivers/net/ethernet/ti/davinci_cpdma.h
@@ -87,7 +87,6 @@ int cpdma_chan_process(struct cpdma_chan *chan, int quota);
 
 int cpdma_ctlr_int_ctrl(struct cpdma_ctlr *ctlr, bool enable);
 void cpdma_ctlr_eoi(struct cpdma_ctlr *ctlr, u32 value);
-int cpdma_chan_int_ctrl(struct cpdma_chan *chan, bool enable);
 u32 cpdma_ctrl_rxchs_state(struct cpdma_ctlr *ctlr);
 u32 cpdma_ctrl_txchs_state(struct cpdma_ctlr *ctlr);
 bool cpdma_check_free_tx_desc(struct cpdma_chan *chan);
@@ -111,7 +110,4 @@ enum cpdma_control {
 	CPDMA_RX_BUFFER_OFFSET,		/* read-write */
 };
 
-int cpdma_control_get(struct cpdma_ctlr *ctlr, int control);
-int cpdma_control_set(struct cpdma_ctlr *ctlr, int control, int value);
-
 #endif
-- 
2.9.0

[PATCH net 3/3] cpsw/netcp: work around reverse cpts dependency

[Arnd Bergmann]

The dependency is reversed: cpsw and netcp call into cpts,
but cpts depends on the other two in Kconfig. This can lead
to cpts being a loadable module and its callers built-in:

drivers/net/ethernet/ti/cpsw.o: In function `cpsw_remove':
cpsw.c:(.text.cpsw_remove+0xd0): undefined reference to `cpts_release'
drivers/net/ethernet/ti/cpsw.o: In function `cpsw_rx_handler':
cpsw.c:(.text.cpsw_rx_handler+0x2dc): undefined reference to `cpts_rx_timestamp'
drivers/net/ethernet/ti/cpsw.o: In function `cpsw_tx_handler':
cpsw.c:(.text.cpsw_tx_handler+0x7c): undefined reference to `cpts_tx_timestamp'
drivers/net/ethernet/ti/cpsw.o: In function `cpsw_ndo_stop':

As a workaround, I'm introducing another Kconfig symbol to
control the compilation of cpts, while making the actual
module controlled by a silent symbol that is =y when necessary.

Fixes: 6246168b4a38 ("net: ethernet: ti: netcp: add support of cpts")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/ti/Kconfig | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
index 366e29ff8605..c114efcd1575 100644
--- a/drivers/net/ethernet/ti/Kconfig
+++ b/drivers/net/ethernet/ti/Kconfig
@@ -73,8 +73,8 @@ config TI_CPSW
 	  To compile this driver as a module, choose M here: the module
 	  will be called cpsw.
 
-config TI_CPTS
-	tristate "TI Common Platform Time Sync (CPTS) Support"
+config TI_CPTS_ENABLE
+	bool "TI Common Platform Time Sync (CPTS) Support"
 	depends on TI_CPSW || TI_KEYSTONE_NETCP
 	depends on POSIX_TIMERS
 	select PTP_1588_CLOCK
@@ -84,6 +84,12 @@ config TI_CPTS
 	  The unit can time stamp PTP UDP/IPv4 and Layer 2 packets, and the
 	  driver offers a PTP Hardware Clock.
 
+config TI_CPTS
+	tristate
+	depends on TI_CPTS_ENABLE
+	default y if TI_CPSW=y || TI_KEYSTONE_NETCP=y
+	default m
+
 config TI_KEYSTONE_NETCP
 	tristate "TI Keystone NETCP Core Support"
 	select TI_CPSW_ALE
-- 
2.9.0

Re: [PATCH net 2/3] cpsw/netcp: davinci_cpdma: sanitize inter-module API

[Ivan Khoronzhuk]

On Fri, Dec 16, 2016 at 10:19:58AM +0100, Arnd Bergmann wrote:
> The davinci_cpdma module is a helper library that is used by the
> actual device drivers and does nothing by itself, so all its API
> functions need to be exported.
> 
> Four new functions were added recently without an export, so now
> we get a build error:
> 
> ERROR: "cpdma_chan_set_weight" [drivers/net/ethernet/ti/ti_cpsw.ko] undefined!
> ERROR: "cpdma_chan_get_rate" [drivers/net/ethernet/ti/ti_cpsw.ko] undefined!
> ERROR: "cpdma_chan_get_min_rate" [drivers/net/ethernet/ti/ti_cpsw.ko] undefined!
> ERROR: "cpdma_chan_set_rate" [drivers/net/ethernet/ti/ti_cpsw.ko] undefined!
/\
||
A patch correcting this has been applied.
(397c5ad153f0ea62023accb67b3d2b07e5039948)

> 
> This exports those symbols. After taking a closer look, I found two
> more global functions in this file that are not exported and not used
> anywhere, so they can obviously be removed. There is also one function
> that is only used internally in this module, and should be 'static'.
> 
> Fixes: 8f32b90981dc ("net: ethernet: ti: davinci_cpdma: add set rate for a channel")
> Fixes: 0fc6432cc78d ("net: ethernet: ti: davinci_cpdma: add weight function for channels")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/net/ethernet/ti/davinci_cpdma.c | 59 ++++++++++-----------------------
>  drivers/net/ethernet/ti/davinci_cpdma.h |  4 ---
>  2 files changed, 17 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> index 36518fc5c7cc..b9d40f0cdf6c 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> @@ -628,6 +628,23 @@ int cpdma_ctlr_destroy(struct cpdma_ctlr *ctlr)
>  }
>  EXPORT_SYMBOL_GPL(cpdma_ctlr_destroy);
>  
> +static int cpdma_chan_int_ctrl(struct cpdma_chan *chan, bool enable)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +	if (chan->state != CPDMA_STATE_ACTIVE) {
> +		spin_unlock_irqrestore(&chan->lock, flags);
> +		return -EINVAL;
> +	}
> +
> +	dma_reg_write(chan->ctlr, enable ? chan->int_set : chan->int_clear,
> +		      chan->mask);
> +	spin_unlock_irqrestore(&chan->lock, flags);
> +
> +	return 0;
> +}
> +
>  int cpdma_ctlr_int_ctrl(struct cpdma_ctlr *ctlr, bool enable)
>  {
>  	unsigned long flags;
> @@ -1274,46 +1291,4 @@ int cpdma_chan_stop(struct cpdma_chan *chan)
>  }
>  EXPORT_SYMBOL_GPL(cpdma_chan_stop);
>  
> -int cpdma_chan_int_ctrl(struct cpdma_chan *chan, bool enable)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&chan->lock, flags);
> -	if (chan->state != CPDMA_STATE_ACTIVE) {
> -		spin_unlock_irqrestore(&chan->lock, flags);
> -		return -EINVAL;
> -	}
> -
> -	dma_reg_write(chan->ctlr, enable ? chan->int_set : chan->int_clear,
> -		      chan->mask);
> -	spin_unlock_irqrestore(&chan->lock, flags);
> -
> -	return 0;
> -}
> -
> -int cpdma_control_get(struct cpdma_ctlr *ctlr, int control)
> -{
> -	unsigned long flags;
> -	int ret;
> -
> -	spin_lock_irqsave(&ctlr->lock, flags);
> -	ret = _cpdma_control_get(ctlr, control);
> -	spin_unlock_irqrestore(&ctlr->lock, flags);
> -
> -	return ret;
> -}
> -
> -int cpdma_control_set(struct cpdma_ctlr *ctlr, int control, int value)
> -{
> -	unsigned long flags;
> -	int ret;
> -
> -	spin_lock_irqsave(&ctlr->lock, flags);
> -	ret = _cpdma_control_set(ctlr, control, value);
> -	spin_unlock_irqrestore(&ctlr->lock, flags);
> -
> -	return ret;
> -}
> -EXPORT_SYMBOL_GPL(cpdma_control_set);
> -
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.h b/drivers/net/ethernet/ti/davinci_cpdma.h
> index 4a167db2abab..36d0a09a3d44 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.h
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.h
> @@ -87,7 +87,6 @@ int cpdma_chan_process(struct cpdma_chan *chan, int quota);
>  
>  int cpdma_ctlr_int_ctrl(struct cpdma_ctlr *ctlr, bool enable);
>  void cpdma_ctlr_eoi(struct cpdma_ctlr *ctlr, u32 value);
> -int cpdma_chan_int_ctrl(struct cpdma_chan *chan, bool enable);
>  u32 cpdma_ctrl_rxchs_state(struct cpdma_ctlr *ctlr);
>  u32 cpdma_ctrl_txchs_state(struct cpdma_ctlr *ctlr);
>  bool cpdma_check_free_tx_desc(struct cpdma_chan *chan);
> @@ -111,7 +110,4 @@ enum cpdma_control {
>  	CPDMA_RX_BUFFER_OFFSET,		/* read-write */
>  };
>  
> -int cpdma_control_get(struct cpdma_ctlr *ctlr, int control);
> -int cpdma_control_set(struct cpdma_ctlr *ctlr, int control, int value);
> -
>  #endif
> -- 
> 2.9.0
> 

Re: [PATCH net 1/3] cpsw/netcp: cpts depends on posix_timers

[Nicolas Pitre]

On Fri, 16 Dec 2016, Arnd Bergmann wrote:

> With posix timers having become optional, we get a build error with
> the cpts time sync option of the CPSW driver:
> 
> drivers/net/ethernet/ti/cpts.c: In function 'cpts_find_ts':
> drivers/net/ethernet/ti/cpts.c:291:23: error: implicit declaration of function 'ptp_classify_raw';did you mean 'ptp_classifier_init'? [-Werror=implicit-function-declaration]
> 
> It really makes no sense to build this driver if we can't use PTP,
> so it's better to go back to 'select PTP_1588_CLOCK' but instead
> add a dependency on POSIX_TIMERS.

Why not depend on PTP_1588_CLOCK directly instead?

> Fixes: baa73d9e478f ("posix-timers: Make them configurable")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/net/ethernet/ti/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
> index 296c8efd0038..366e29ff8605 100644
> --- a/drivers/net/ethernet/ti/Kconfig
> +++ b/drivers/net/ethernet/ti/Kconfig
> @@ -76,7 +76,8 @@ config TI_CPSW
>  config TI_CPTS
>  	tristate "TI Common Platform Time Sync (CPTS) Support"
>  	depends on TI_CPSW || TI_KEYSTONE_NETCP
> -	imply PTP_1588_CLOCK
> +	depends on POSIX_TIMERS
> +	select PTP_1588_CLOCK
>  	---help---
>  	  This driver supports the Common Platform Time Sync unit of
>  	  the CPSW Ethernet Switch and Keystone 2 1g/10g Switch Subsystem.
> -- 
> 2.9.0
> 
> 

Re: [PATCH net 3/3] cpsw/netcp: work around reverse cpts dependency

[Grygorii Strashko]

On 12/16/2016 03:19 AM, Arnd Bergmann wrote:
> The dependency is reversed: cpsw and netcp call into cpts,
> but cpts depends on the other two in Kconfig. This can lead
> to cpts being a loadable module and its callers built-in:
>
> drivers/net/ethernet/ti/cpsw.o: In function `cpsw_remove':
> cpsw.c:(.text.cpsw_remove+0xd0): undefined reference to `cpts_release'
> drivers/net/ethernet/ti/cpsw.o: In function `cpsw_rx_handler':
> cpsw.c:(.text.cpsw_rx_handler+0x2dc): undefined reference to `cpts_rx_timestamp'
> drivers/net/ethernet/ti/cpsw.o: In function `cpsw_tx_handler':
> cpsw.c:(.text.cpsw_tx_handler+0x7c): undefined reference to `cpts_tx_timestamp'
> drivers/net/ethernet/ti/cpsw.o: In function `cpsw_ndo_stop':
>
> As a workaround, I'm introducing another Kconfig symbol to
> control the compilation of cpts, while making the actual
> module controlled by a silent symbol that is =y when necessary.
>
> Fixes: 6246168b4a38 ("net: ethernet: ti: netcp: add support of cpts")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks for the fix.
  I've tried to test as much combination as possible,
but seems not of them:(

Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>

> ---
>  drivers/net/ethernet/ti/Kconfig | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
> index 366e29ff8605..c114efcd1575 100644
> --- a/drivers/net/ethernet/ti/Kconfig
> +++ b/drivers/net/ethernet/ti/Kconfig
> @@ -73,8 +73,8 @@ config TI_CPSW
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called cpsw.
>
> -config TI_CPTS
> -	tristate "TI Common Platform Time Sync (CPTS) Support"
> +config TI_CPTS_ENABLE
> +	bool "TI Common Platform Time Sync (CPTS) Support"
>  	depends on TI_CPSW || TI_KEYSTONE_NETCP
>  	depends on POSIX_TIMERS
>  	select PTP_1588_CLOCK
> @@ -84,6 +84,12 @@ config TI_CPTS
>  	  The unit can time stamp PTP UDP/IPv4 and Layer 2 packets, and the
>  	  driver offers a PTP Hardware Clock.
>
> +config TI_CPTS
> +	tristate
> +	depends on TI_CPTS_ENABLE
> +	default y if TI_CPSW=y || TI_KEYSTONE_NETCP=y
> +	default m
> +
>  config TI_KEYSTONE_NETCP
>  	tristate "TI Keystone NETCP Core Support"
>  	select TI_CPSW_ALE
>

-- 
regards,
-grygorii

Re: [PATCH net 1/3] cpsw/netcp: cpts depends on posix_timers

[Arnd Bergmann]

On Fri, Dec 16, 2016 at 7:18 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Fri, 16 Dec 2016, Arnd Bergmann wrote:
>
>> With posix timers having become optional, we get a build error with
>> the cpts time sync option of the CPSW driver:
>>
>> drivers/net/ethernet/ti/cpts.c: In function 'cpts_find_ts':
>> drivers/net/ethernet/ti/cpts.c:291:23: error: implicit declaration of function 'ptp_classify_raw';did you mean 'ptp_classifier_init'? [-Werror=implicit-function-declaration]
>>
>> It really makes no sense to build this driver if we can't use PTP,
>> so it's better to go back to 'select PTP_1588_CLOCK' but instead
>> add a dependency on POSIX_TIMERS.
>
> Why not depend on PTP_1588_CLOCK directly instead?

Sorry for dropping the ball on this. I just noticed my two patches in my patch
are still required, so I'm resending them now.

I think using 'select' here is better for consistency as we use 'imply'
elsewhere, and mixing the two might cause circular dependencies.

If you still have concerns about this, please follow up on the resend.

      Arnd