Re: [PATCH v2 net-next 01/10] net: eth: altera: tse_start_xmit ignores tx_buffer call response

Re: [PATCH v2 net-next 01/10] net: eth: altera: tse_start_xmit ignores tx_buffer call response

[Thor Thayer]

Hi Dalon,

On 12/13/18 11:52 AM, dwesterg@gmail.com wrote:
> From: Dalon Westergreen <dalon.westergreen@intel.com>
> 
> The return from tx_buffer call in tse_start_xmit is
> inapropriately ignored.  tse_buffer calls should return
> 0 for success or NETDEV_TX_BUSY.  tse_start_xmit should
> return not report a successful transmit when the tse_buffer
> call returns an error condition.
> 
> In addition to the above, the msgdma and sgdma do not return
> the same value on success or failure.  The sgdma_tx_buffer
> returned 0 on failure and a positive number of transmitted
> packets on success.  Given that it only ever sends 1 packet,
> this made no sense.  The msgdma implementation msgdma_tx_buffer
> returns 0 on success.
> 
>    -> Don't ignore the return from tse_buffer calls
>    -> Fix sgdma tse_buffer call to return 0 on success
>       and NETDEV_TX_BUSY on failure.
> 
> Signed-off-by: Dalon Westergreen <dalon.westergreen@intel.com>
> ---
>   drivers/net/ethernet/altera/altera_sgdma.c    | 14 ++++++++------
>   drivers/net/ethernet/altera/altera_tse_main.c |  4 +++-
>   2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/altera/altera_sgdma.c b/drivers/net/ethernet/altera/altera_sgdma.c
> index 88ef67a998b4..eb47b9b820bb 100644
> --- a/drivers/net/ethernet/altera/altera_sgdma.c
> +++ b/drivers/net/ethernet/altera/altera_sgdma.c
> @@ -15,6 +15,7 @@
>    */
>   
>   #include <linux/list.h>
> +#include <linux/netdevice.h>
>   #include "altera_utils.h"
>   #include "altera_tse.h"
>   #include "altera_sgdmahw.h"
> @@ -170,10 +171,11 @@ void sgdma_clear_txirq(struct altera_tse_private *priv)
>   		    SGDMA_CTRLREG_CLRINT);
>   }
>   
> -/* transmits buffer through SGDMA. Returns number of buffers
> - * transmitted, 0 if not possible.
> - *
> - * tx_lock is held by the caller
> +/* transmits buffer through SGDMA.
> + *   original behavior returned the number of transmitted packets (always 1) &
> + *   returned 0 on error.  This differs from the msgdma.  the calling function
> + *   will now actually look at the code, so from now, 0 is good and return
> + *   NETDEV_TX_BUSY when busy.
>    */
>   int sgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
>   {
> @@ -185,7 +187,7 @@ int sgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
>   
>   	/* wait 'til the tx sgdma is ready for the next transmit request */
>   	if (sgdma_txbusy(priv))
> -		return 0;
> +		return NETDEV_TX_BUSY;
>   
>   	sgdma_setup_descrip(cdesc,			/* current descriptor */
>   			    ndesc,			/* next descriptor */
> @@ -202,7 +204,7 @@ int sgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
>   	/* enqueue the request to the pending transmit queue */
>   	queue_tx(priv, buffer);
>   
> -	return 1;
> +	return 0;
>   }
>   
>   
> diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
> index baca8f704a45..dcb330129e23 100644
> --- a/drivers/net/ethernet/altera/altera_tse_main.c
> +++ b/drivers/net/ethernet/altera/altera_tse_main.c
> @@ -606,7 +606,9 @@ static int tse_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   	buffer->dma_addr = dma_addr;
>   	buffer->len = nopaged_len;
>   
> -	priv->dmaops->tx_buffer(priv, buffer);
> +	ret = priv->dmaops->tx_buffer(priv, buffer);
> +	if (ret)
> +		goto out;
>   
>   	skb_tx_timestamp(skb);
>   
> 
If the content hasn't changed, you can just include my previous Acked-by 
under your Signed-off-by in the subsequent versions. For now though,

Acked-by: Thor Thayer <thor.thayer@linux.intel.com>

Re: [PATCH v2 net-next 02/10] net: eth: altera: set rx and tx ring size before init_dma call

[Thor Thayer]

On 12/13/18 11:52 AM, dwesterg@gmail.com wrote:
> From: Dalon Westergreen <dalon.westergreen@intel.com>
> 
> It is more appropriate to set the rx and tx ring size before calling
> the init function for the dma.
> 
> Signed-off-by: Dalon Westergreen <dalon.westergreen@intel.com>
> ---
>   drivers/net/ethernet/altera/altera_tse_main.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
> index dcb330129e23..0c0e8f9bba9b 100644
> --- a/drivers/net/ethernet/altera/altera_tse_main.c
> +++ b/drivers/net/ethernet/altera/altera_tse_main.c
> @@ -1166,6 +1166,10 @@ static int tse_open(struct net_device *dev)
>   	int i;
>   	unsigned long int flags;
>   
> +	/* set tx and rx ring size */
> +	priv->rx_ring_size = dma_rx_num;
> +	priv->tx_ring_size = dma_tx_num;
> +
>   	/* Reset and configure TSE MAC and probe associated PHY */
>   	ret = priv->dmaops->init_dma(priv);
>   	if (ret != 0) {
> @@ -1208,8 +1212,6 @@ static int tse_open(struct net_device *dev)
>   	priv->dmaops->reset_dma(priv);
>   
>   	/* Create and initialize the TX/RX descriptors chains. */
> -	priv->rx_ring_size = dma_rx_num;
> -	priv->tx_ring_size = dma_tx_num;
>   	ret = alloc_init_skbufs(priv);
>   	if (ret) {
>   		netdev_err(dev, "DMA descriptors initialization failed\n");
> 
Acked-by: Thor Thayer <thor.thayer@linux.intel.com>

Re: [PATCH v2 net-next 03/10] net: eth: altera: fix altera_dmaops declaration

[Thor Thayer]

On 12/13/18 11:52 AM, dwesterg@gmail.com wrote:
> From: Dalon Westergreen <dalon.westergreen@intel.com>
> 
> The declaration of struct altera_dmaops does not have
> identifier names.  Add identifier names to confrom with
> required coding styles.
> 
> Signed-off-by: Dalon Westergreen <dalon.westergreen@intel.com>
> ---
>   drivers/net/ethernet/altera/altera_tse.h | 30 +++++++++++++-----------
>   1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/altera/altera_tse.h b/drivers/net/ethernet/altera/altera_tse.h
> index e2feee87180a..d5b97e02e6d6 100644
> --- a/drivers/net/ethernet/altera/altera_tse.h
> +++ b/drivers/net/ethernet/altera/altera_tse.h
> @@ -396,20 +396,22 @@ struct altera_tse_private;
>   struct altera_dmaops {
>   	int altera_dtype;
>   	int dmamask;
> -	void (*reset_dma)(struct altera_tse_private *);
> -	void (*enable_txirq)(struct altera_tse_private *);
> -	void (*enable_rxirq)(struct altera_tse_private *);
> -	void (*disable_txirq)(struct altera_tse_private *);
> -	void (*disable_rxirq)(struct altera_tse_private *);
> -	void (*clear_txirq)(struct altera_tse_private *);
> -	void (*clear_rxirq)(struct altera_tse_private *);
> -	int (*tx_buffer)(struct altera_tse_private *, struct tse_buffer *);
> -	u32 (*tx_completions)(struct altera_tse_private *);
> -	void (*add_rx_desc)(struct altera_tse_private *, struct tse_buffer *);
> -	u32 (*get_rx_status)(struct altera_tse_private *);
> -	int (*init_dma)(struct altera_tse_private *);
> -	void (*uninit_dma)(struct altera_tse_private *);
> -	void (*start_rxdma)(struct altera_tse_private *);
> +	void (*reset_dma)(struct altera_tse_private *priv);
> +	void (*enable_txirq)(struct altera_tse_private *priv);
> +	void (*enable_rxirq)(struct altera_tse_private *priv);
> +	void (*disable_txirq)(struct altera_tse_private *priv);
> +	void (*disable_rxirq)(struct altera_tse_private *priv);
> +	void (*clear_txirq)(struct altera_tse_private *priv);
> +	void (*clear_rxirq)(struct altera_tse_private *priv);
> +	int (*tx_buffer)(struct altera_tse_private *priv,
> +			 struct tse_buffer *buffer);
> +	u32 (*tx_completions)(struct altera_tse_private *priv);
> +	void (*add_rx_desc)(struct altera_tse_private *priv,
> +			    struct tse_buffer *buffer);
> +	u32 (*get_rx_status)(struct altera_tse_private *priv);
> +	int (*init_dma)(struct altera_tse_private *priv);
> +	void (*uninit_dma)(struct altera_tse_private *priv);
> +	void (*start_rxdma)(struct altera_tse_private *priv);
>   };
>   
>   /* This structure is private to each device.
> 
Acked-by: Thor Thayer <thor.thayer@linux.intel.com>

Re: [PATCH v2 net-next 04/10] net: eth: altera: add optional function to start tx dma

[Thor Thayer]

On 12/13/18 11:52 AM, dwesterg@gmail.com wrote:
> From: Dalon Westergreen <dalon.westergreen@intel.com>
> 
> Allow for optional start up of tx dma if the start_txdma
> function is defined in altera_dmaops.
> 
> Signed-off-by: Dalon Westergreen <dalon.westergreen@intel.com>
> ---
>   drivers/net/ethernet/altera/altera_tse.h      | 1 +
>   drivers/net/ethernet/altera/altera_tse_main.c | 5 +++++
>   2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/altera/altera_tse.h b/drivers/net/ethernet/altera/altera_tse.h
> index d5b97e02e6d6..7f246040135d 100644
> --- a/drivers/net/ethernet/altera/altera_tse.h
> +++ b/drivers/net/ethernet/altera/altera_tse.h
> @@ -412,6 +412,7 @@ struct altera_dmaops {
>   	int (*init_dma)(struct altera_tse_private *priv);
>   	void (*uninit_dma)(struct altera_tse_private *priv);
>   	void (*start_rxdma)(struct altera_tse_private *priv);
> +	void (*start_txdma)(struct altera_tse_private *priv);
>   };
>   
>   /* This structure is private to each device.
> diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
> index 0c0e8f9bba9b..f6b6a14b1ce9 100644
> --- a/drivers/net/ethernet/altera/altera_tse_main.c
> +++ b/drivers/net/ethernet/altera/altera_tse_main.c
> @@ -1256,6 +1256,9 @@ static int tse_open(struct net_device *dev)
>   
>   	priv->dmaops->start_rxdma(priv);
>   
> +	if (priv->dmaops->start_txdma)
> +		priv->dmaops->start_txdma(priv);
> +
>   	/* Start MAC Rx/Tx */
>   	spin_lock(&priv->mac_cfg_lock);
>   	tse_set_mac(priv, true);
> @@ -1658,6 +1661,7 @@ static const struct altera_dmaops altera_dtype_sgdma = {
>   	.init_dma = sgdma_initialize,
>   	.uninit_dma = sgdma_uninitialize,
>   	.start_rxdma = sgdma_start_rxdma,
> +	.start_txdma = NULL,
>   };
>   
>   static const struct altera_dmaops altera_dtype_msgdma = {
> @@ -1677,6 +1681,7 @@ static const struct altera_dmaops altera_dtype_msgdma = {
>   	.init_dma = msgdma_initialize,
>   	.uninit_dma = msgdma_uninitialize,
>   	.start_rxdma = msgdma_start_rxdma,
> +	.start_txdma = NULL,
>   };
>   
>   static const struct of_device_id altera_tse_ids[] = {
> 
Acked-by: Thor Thayer <thor.thayer@linux.intel.com>

Re: [PATCH v2 net-next 05/10] net: eth: altera: Move common functions to altera_utils

[Thor Thayer]

On 12/13/18 11:52 AM, dwesterg@gmail.com wrote:
> From: Dalon Westergreen <dalon.westergreen@intel.com>
> 
> Move request_and_map and other shared functions to altera_utils. This
> is the first step to moving common code out of tse specific code so
> that it can be shared with future altera ethernet ip.
> 
> Signed-off-by: Dalon Westergreen <dalon.westergreen@intel.com>
> ---
>   drivers/net/ethernet/altera/altera_tse.h      | 45 ------------------
>   .../net/ethernet/altera/altera_tse_ethtool.c  |  1 +
>   drivers/net/ethernet/altera/altera_tse_main.c | 32 +------------
>   drivers/net/ethernet/altera/altera_utils.c    | 30 ++++++++++++
>   drivers/net/ethernet/altera/altera_utils.h    | 46 +++++++++++++++++++
>   5 files changed, 78 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/net/ethernet/altera/altera_tse.h b/drivers/net/ethernet/altera/altera_tse.h
> index 7f246040135d..f435fb0eca90 100644
> --- a/drivers/net/ethernet/altera/altera_tse.h
> +++ b/drivers/net/ethernet/altera/altera_tse.h
> @@ -500,49 +500,4 @@ struct altera_tse_private {
>    */
>   void altera_tse_set_ethtool_ops(struct net_device *);
>   
> -static inline
> -u32 csrrd32(void __iomem *mac, size_t offs)
> -{
> -	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> -	return readl(paddr);
> -}
> -
> -static inline
> -u16 csrrd16(void __iomem *mac, size_t offs)
> -{
> -	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> -	return readw(paddr);
> -}
> -
> -static inline
> -u8 csrrd8(void __iomem *mac, size_t offs)
> -{
> -	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> -	return readb(paddr);
> -}
> -
> -static inline
> -void csrwr32(u32 val, void __iomem *mac, size_t offs)
> -{
> -	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> -
> -	writel(val, paddr);
> -}
> -
> -static inline
> -void csrwr16(u16 val, void __iomem *mac, size_t offs)
> -{
> -	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> -
> -	writew(val, paddr);
> -}
> -
> -static inline
> -void csrwr8(u8 val, void __iomem *mac, size_t offs)
> -{
> -	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> -
> -	writeb(val, paddr);
> -}
> -
>   #endif /* __ALTERA_TSE_H__ */
> diff --git a/drivers/net/ethernet/altera/altera_tse_ethtool.c b/drivers/net/ethernet/altera/altera_tse_ethtool.c
> index 7c367713c3e6..2998655ab316 100644
> --- a/drivers/net/ethernet/altera/altera_tse_ethtool.c
> +++ b/drivers/net/ethernet/altera/altera_tse_ethtool.c
> @@ -33,6 +33,7 @@
>   #include <linux/phy.h>
>   
>   #include "altera_tse.h"
> +#include "altera_utils.h"
>   
>   #define TSE_STATS_LEN	31
>   #define TSE_NUM_REGS	128
> diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
> index f6b6a14b1ce9..b25d03506470 100644
> --- a/drivers/net/ethernet/altera/altera_tse_main.c
> +++ b/drivers/net/ethernet/altera/altera_tse_main.c
> @@ -34,7 +34,6 @@
>   #include <linux/if_vlan.h>
>   #include <linux/init.h>
>   #include <linux/interrupt.h>
> -#include <linux/io.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/mii.h>
> @@ -44,7 +43,7 @@
>   #include <linux/of_net.h>
>   #include <linux/of_platform.h>
>   #include <linux/phy.h>
> -#include <linux/platform_device.h>
> +#include <linux/ptp_classify.h>
>   #include <linux/skbuff.h>
>   #include <asm/cacheflush.h>
>   
> @@ -1332,35 +1331,6 @@ static struct net_device_ops altera_tse_netdev_ops = {
>   	.ndo_validate_addr	= eth_validate_addr,
>   };
>   
> -static int request_and_map(struct platform_device *pdev, const char *name,
> -			   struct resource **res, void __iomem **ptr)
> -{
> -	struct resource *region;
> -	struct device *device = &pdev->dev;
> -
> -	*res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> -	if (*res == NULL) {
> -		dev_err(device, "resource %s not defined\n", name);
> -		return -ENODEV;
> -	}
> -
> -	region = devm_request_mem_region(device, (*res)->start,
> -					 resource_size(*res), dev_name(device));
> -	if (region == NULL) {
> -		dev_err(device, "unable to request %s\n", name);
> -		return -EBUSY;
> -	}
> -
> -	*ptr = devm_ioremap_nocache(device, region->start,
> -				    resource_size(region));
> -	if (*ptr == NULL) {
> -		dev_err(device, "ioremap_nocache of %s failed!", name);
> -		return -ENOMEM;
> -	}
> -
> -	return 0;
> -}
> -
>   /* Probe Altera TSE MAC device
>    */
>   static int altera_tse_probe(struct platform_device *pdev)
> diff --git a/drivers/net/ethernet/altera/altera_utils.c b/drivers/net/ethernet/altera/altera_utils.c
> index d7eeb1713ad2..bc33b7f0b0c5 100644
> --- a/drivers/net/ethernet/altera/altera_utils.c
> +++ b/drivers/net/ethernet/altera/altera_utils.c
> @@ -42,3 +42,33 @@ int tse_bit_is_clear(void __iomem *ioaddr, size_t offs, u32 bit_mask)
>   	u32 value = csrrd32(ioaddr, offs);
>   	return (value & bit_mask) ? 0 : 1;
>   }
> +
> +int request_and_map(struct platform_device *pdev, const char *name,
> +		    struct resource **res, void __iomem **ptr)
> +{
> +	struct resource *region;
> +	struct device *device = &pdev->dev;
> +
> +	*res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> +	if (!*res) {
> +		dev_err(device, "resource %s not defined\n", name);
> +		return -ENODEV;
> +	}
> +
> +	region = devm_request_mem_region(device, (*res)->start,
> +					 resource_size(*res), dev_name(device));
> +	if (!region) {
> +		dev_err(device, "unable to request %s\n", name);
> +		return -EBUSY;
> +	}
> +
> +	*ptr = devm_ioremap_nocache(device, region->start,
> +				    resource_size(region));
> +	if (!*ptr) {
> +		dev_err(device, "ioremap_nocache of %s failed!", name);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> diff --git a/drivers/net/ethernet/altera/altera_utils.h b/drivers/net/ethernet/altera/altera_utils.h
> index baf100ccf587..bb7eff792bb7 100644
> --- a/drivers/net/ethernet/altera/altera_utils.h
> +++ b/drivers/net/ethernet/altera/altera_utils.h
> @@ -14,7 +14,9 @@
>    * this program.  If not, see <http://www.gnu.org/licenses/>.
>    */
>   
> +#include <linux/io.h>
>   #include <linux/kernel.h>
> +#include <linux/platform_device.h>
>   
>   #ifndef __ALTERA_UTILS_H__
>   #define __ALTERA_UTILS_H__
> @@ -23,5 +25,49 @@ void tse_set_bit(void __iomem *ioaddr, size_t offs, u32 bit_mask);
>   void tse_clear_bit(void __iomem *ioaddr, size_t offs, u32 bit_mask);
>   int tse_bit_is_set(void __iomem *ioaddr, size_t offs, u32 bit_mask);
>   int tse_bit_is_clear(void __iomem *ioaddr, size_t offs, u32 bit_mask);
> +int request_and_map(struct platform_device *pdev, const char *name,
> +		    struct resource **res, void __iomem **ptr);
> +
> +static inline u32 csrrd32(void __iomem *mac, size_t offs)
> +{
> +	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> +
> +	return readl(paddr);
> +}
> +
> +static inline u16 csrrd16(void __iomem *mac, size_t offs)
> +{
> +	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> +
> +	return readw(paddr);
> +}
> +
> +static inline u8 csrrd8(void __iomem *mac, size_t offs)
> +{
> +	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> +
> +	return readb(paddr);
> +}
> +
> +static inline void csrwr32(u32 val, void __iomem *mac, size_t offs)
> +{
> +	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> +
> +	writel(val, paddr);
> +}
> +
> +static inline void csrwr16(u16 val, void __iomem *mac, size_t offs)
> +{
> +	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> +
> +	writew(val, paddr);
> +}
> +
> +static inline void csrwr8(u8 val, void __iomem *mac, size_t offs)
> +{
> +	void __iomem *paddr = (void __iomem *)((uintptr_t)mac + offs);
> +
> +	writeb(val, paddr);
> +}
>   
>   #endif /* __ALTERA_UTILS_H__*/
> 
Acked-by: Thor Thayer <thor.thayer@linux.intel.com>

Re: [PATCH v2 net-next 06/10] net: eth: altera: Add missing identifier names to function declarations

[Thor Thayer]

Hi Dalon,

On 12/13/18 11:52 AM, dwesterg@gmail.com wrote:
> From: Dalon Westergreen <dalon.westergreen@linux.intel.com>
> 
> The sgdma and msgdma header files included function declarations
> without identifier names for pointers.  Add appropriate identifier
> names.
> 
> Signed-off-by: Dalon Westergreen <dalon.westergreen@linux.intel.com>
> ---
>   drivers/net/ethernet/altera/altera_msgdma.h | 30 ++++++++++---------
>   drivers/net/ethernet/altera/altera_sgdma.h  | 32 +++++++++++----------
>   2 files changed, 33 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/ethernet/altera/altera_msgdma.h b/drivers/net/ethernet/altera/altera_msgdma.h
> index 42cf61c81057..e18be91a48a7 100644
> --- a/drivers/net/ethernet/altera/altera_msgdma.h
> +++ b/drivers/net/ethernet/altera/altera_msgdma.h
> @@ -17,19 +17,21 @@
>   #ifndef __ALTERA_MSGDMA_H__
>   #define __ALTERA_MSGDMA_H__
>   
> -void msgdma_reset(struct altera_tse_private *);
> -void msgdma_enable_txirq(struct altera_tse_private *);
> -void msgdma_enable_rxirq(struct altera_tse_private *);
> -void msgdma_disable_rxirq(struct altera_tse_private *);
> -void msgdma_disable_txirq(struct altera_tse_private *);
> -void msgdma_clear_rxirq(struct altera_tse_private *);
> -void msgdma_clear_txirq(struct altera_tse_private *);
> -u32 msgdma_tx_completions(struct altera_tse_private *);
> -void msgdma_add_rx_desc(struct altera_tse_private *, struct tse_buffer *);
> -int msgdma_tx_buffer(struct altera_tse_private *, struct tse_buffer *);
> -u32 msgdma_rx_status(struct altera_tse_private *);
> -int msgdma_initialize(struct altera_tse_private *);
> -void msgdma_uninitialize(struct altera_tse_private *);
> -void msgdma_start_rxdma(struct altera_tse_private *);
> +void msgdma_reset(struct altera_tse_private *priv);
> +void msgdma_enable_txirq(struct altera_tse_private *priv);
> +void msgdma_enable_rxirq(struct altera_tse_private *priv);
> +void msgdma_disable_rxirq(struct altera_tse_private *priv);
> +void msgdma_disable_txirq(struct altera_tse_private *priv);
> +void msgdma_clear_rxirq(struct altera_tse_private *priv);
> +void msgdma_clear_txirq(struct altera_tse_private *priv);
> +u32 msgdma_tx_completions(struct altera_tse_private *priv);
> +void msgdma_add_rx_desc(struct altera_tse_private *priv,
> +			struct tse_buffer *buffer);
> +int msgdma_tx_buffer(struct altera_tse_private *priv,
> +		     struct tse_buffer *buffer);
> +u32 msgdma_rx_status(struct altera_tse_private *priv);
> +int msgdma_initialize(struct altera_tse_private *priv);
> +void msgdma_uninitialize(struct altera_tse_private *priv);
> +void msgdma_start_rxdma(struct altera_tse_private *priv);
>   
>   #endif /*  __ALTERA_MSGDMA_H__ */
> diff --git a/drivers/net/ethernet/altera/altera_sgdma.h b/drivers/net/ethernet/altera/altera_sgdma.h
> index 584977e29ef9..769e720a2043 100644
> --- a/drivers/net/ethernet/altera/altera_sgdma.h
> +++ b/drivers/net/ethernet/altera/altera_sgdma.h
> @@ -17,20 +17,22 @@
>   #ifndef __ALTERA_SGDMA_H__
>   #define __ALTERA_SGDMA_H__
>   
> -void sgdma_reset(struct altera_tse_private *);
> -void sgdma_enable_txirq(struct altera_tse_private *);
> -void sgdma_enable_rxirq(struct altera_tse_private *);
> -void sgdma_disable_rxirq(struct altera_tse_private *);
> -void sgdma_disable_txirq(struct altera_tse_private *);
> -void sgdma_clear_rxirq(struct altera_tse_private *);
> -void sgdma_clear_txirq(struct altera_tse_private *);
> -int sgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *);
> -u32 sgdma_tx_completions(struct altera_tse_private *);
> -void sgdma_add_rx_desc(struct altera_tse_private *priv, struct tse_buffer *);
> -void sgdma_status(struct altera_tse_private *);
> -u32 sgdma_rx_status(struct altera_tse_private *);
> -int sgdma_initialize(struct altera_tse_private *);
> -void sgdma_uninitialize(struct altera_tse_private *);
> -void sgdma_start_rxdma(struct altera_tse_private *);
> +void sgdma_reset(struct altera_tse_private *priv);
> +void sgdma_enable_txirq(struct altera_tse_private *priv);
> +void sgdma_enable_rxirq(struct altera_tse_private *priv);
> +void sgdma_disable_rxirq(struct altera_tse_private *priv);
> +void sgdma_disable_txirq(struct altera_tse_private *priv);
> +void sgdma_clear_rxirq(struct altera_tse_private *priv);
> +void sgdma_clear_txirq(struct altera_tse_private *priv);
> +int sgdma_tx_buffer(struct altera_tse_private *priv,
> +		    struct tse_buffer *buffer);
> +u32 sgdma_tx_completions(struct altera_tse_private *priv);
> +void sgdma_add_rx_desc(struct altera_tse_private *priv,
> +		       struct tse_buffer *buffer);
> +void sgdma_status(struct altera_tse_private *priv);
> +u32 sgdma_rx_status(struct altera_tse_private *priv);
> +int sgdma_initialize(struct altera_tse_private *priv);
> +void sgdma_uninitialize(struct altera_tse_private *priv);
> +void sgdma_start_rxdma(struct altera_tse_private *priv);
>   
>   #endif /*  __ALTERA_SGDMA_H__ */
> 

FYI. Since this is a change in v2, there is normally a break below your 
Signed-off-by and then a description of what changed (in this case new 
patch). This isolates the changes to the patch even though you put it in 
the summary patch. So it would look like this

<snip>
 >
 > Signed-off-by: Dalon Westergreen <dalon.westergreen@linux.intel.com>
 > ---
 > v2 New patch cleanup that adds the pointer identifier names.
 > ---
 >   drivers/net/ethernet/altera/altera_msgdma.h | 30 ++++++++++---------
 >   drivers/net/ethernet/altera/altera_sgdma.h  | 32 +++++++++++----------
</snip>

Acked-by: Thor Thayer <thor.thayer@linux.intel.com>

Re: [PATCH v2 net-next 07/10] net: eth: altera: change tx functions to type netdev_tx_t

[Thor Thayer]

On 12/13/18 11:52 AM, dwesterg@gmail.com wrote:
> From: Dalon Westergreen <dalon.westergreen@linux.intel.com>
> 
> Modify all msgdma and sgdma tx_buffer functions to be of type
> netdev_tx_t, and also modify main tx function to be of
> netdev_tx_t type.
> 
> Signed-off-by: Dalon Westergreen <dalon.westergreen@linux.intel.com>
> ---
>   drivers/net/ethernet/altera/altera_msgdma.c   | 5 +++--
>   drivers/net/ethernet/altera/altera_msgdma.h   | 4 ++--
>   drivers/net/ethernet/altera/altera_sgdma.c    | 5 +++--
>   drivers/net/ethernet/altera/altera_sgdma.h    | 4 ++--
>   drivers/net/ethernet/altera/altera_tse.h      | 4 ++--
>   drivers/net/ethernet/altera/altera_tse_main.c | 2 +-
>   6 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/altera/altera_msgdma.c b/drivers/net/ethernet/altera/altera_msgdma.c
> index 0fb986ba3290..95f016731cc2 100644
> --- a/drivers/net/ethernet/altera/altera_msgdma.c
> +++ b/drivers/net/ethernet/altera/altera_msgdma.c
> @@ -117,7 +117,8 @@ void msgdma_clear_txirq(struct altera_tse_private *priv)
>   }
>   
>   /* return 0 to indicate transmit is pending */
> -int msgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
> +netdev_tx_t
> +msgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
>   {
>   	csrwr32(lower_32_bits(buffer->dma_addr), priv->tx_dma_desc,
>   		msgdma_descroffs(read_addr_lo));
> @@ -131,7 +132,7 @@ int msgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
>   		msgdma_descroffs(stride));
>   	csrwr32(MSGDMA_DESC_CTL_TX_SINGLE, priv->tx_dma_desc,
>   		msgdma_descroffs(control));
> -	return 0;
> +	return NETDEV_TX_OK;
>   }
>   
>   u32 msgdma_tx_completions(struct altera_tse_private *priv)
> diff --git a/drivers/net/ethernet/altera/altera_msgdma.h b/drivers/net/ethernet/altera/altera_msgdma.h
> index e18be91a48a7..e9008e3761b9 100644
> --- a/drivers/net/ethernet/altera/altera_msgdma.h
> +++ b/drivers/net/ethernet/altera/altera_msgdma.h
> @@ -27,8 +27,8 @@ void msgdma_clear_txirq(struct altera_tse_private *priv);
>   u32 msgdma_tx_completions(struct altera_tse_private *priv);
>   void msgdma_add_rx_desc(struct altera_tse_private *priv,
>   			struct tse_buffer *buffer);
> -int msgdma_tx_buffer(struct altera_tse_private *priv,
> -		     struct tse_buffer *buffer);
> +netdev_tx_t msgdma_tx_buffer(struct altera_tse_private *priv,
> +			     struct tse_buffer *buffer);
>   u32 msgdma_rx_status(struct altera_tse_private *priv);
>   int msgdma_initialize(struct altera_tse_private *priv);
>   void msgdma_uninitialize(struct altera_tse_private *priv);
> diff --git a/drivers/net/ethernet/altera/altera_sgdma.c b/drivers/net/ethernet/altera/altera_sgdma.c
> index eb47b9b820bb..93f66af338de 100644
> --- a/drivers/net/ethernet/altera/altera_sgdma.c
> +++ b/drivers/net/ethernet/altera/altera_sgdma.c
> @@ -177,7 +177,8 @@ void sgdma_clear_txirq(struct altera_tse_private *priv)
>    *   will now actually look at the code, so from now, 0 is good and return
>    *   NETDEV_TX_BUSY when busy.
>    */
> -int sgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
> +netdev_tx_t
> +sgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
>   {
>   	struct sgdma_descrip __iomem *descbase =
>   		(struct sgdma_descrip __iomem *)priv->tx_dma_desc;
> @@ -204,7 +205,7 @@ int sgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
>   	/* enqueue the request to the pending transmit queue */
>   	queue_tx(priv, buffer);
>   
> -	return 0;
> +	return NETDEV_TX_OK;
>   }
>   
>   
> diff --git a/drivers/net/ethernet/altera/altera_sgdma.h b/drivers/net/ethernet/altera/altera_sgdma.h
> index 769e720a2043..984c00d81596 100644
> --- a/drivers/net/ethernet/altera/altera_sgdma.h
> +++ b/drivers/net/ethernet/altera/altera_sgdma.h
> @@ -24,8 +24,8 @@ void sgdma_disable_rxirq(struct altera_tse_private *priv);
>   void sgdma_disable_txirq(struct altera_tse_private *priv);
>   void sgdma_clear_rxirq(struct altera_tse_private *priv);
>   void sgdma_clear_txirq(struct altera_tse_private *priv);
> -int sgdma_tx_buffer(struct altera_tse_private *priv,
> -		    struct tse_buffer *buffer);
> +netdev_tx_t sgdma_tx_buffer(struct altera_tse_private *priv,
> +			    struct tse_buffer *buffer);
>   u32 sgdma_tx_completions(struct altera_tse_private *priv);
>   void sgdma_add_rx_desc(struct altera_tse_private *priv,
>   		       struct tse_buffer *buffer);
> diff --git a/drivers/net/ethernet/altera/altera_tse.h b/drivers/net/ethernet/altera/altera_tse.h
> index f435fb0eca90..6e7f92ebf3cf 100644
> --- a/drivers/net/ethernet/altera/altera_tse.h
> +++ b/drivers/net/ethernet/altera/altera_tse.h
> @@ -403,8 +403,8 @@ struct altera_dmaops {
>   	void (*disable_rxirq)(struct altera_tse_private *priv);
>   	void (*clear_txirq)(struct altera_tse_private *priv);
>   	void (*clear_rxirq)(struct altera_tse_private *priv);
> -	int (*tx_buffer)(struct altera_tse_private *priv,
> -			 struct tse_buffer *buffer);
> +	netdev_tx_t (*tx_buffer)(struct altera_tse_private *priv,
> +				 struct tse_buffer *buffer);
>   	u32 (*tx_completions)(struct altera_tse_private *priv);
>   	void (*add_rx_desc)(struct altera_tse_private *priv,
>   			    struct tse_buffer *buffer);
> diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
> index b25d03506470..24915f72e965 100644
> --- a/drivers/net/ethernet/altera/altera_tse_main.c
> +++ b/drivers/net/ethernet/altera/altera_tse_main.c
> @@ -564,7 +564,7 @@ static irqreturn_t altera_isr(int irq, void *dev_id)
>    * physically contiguous fragment starting at
>    * skb->data, for length of skb_headlen(skb).
>    */
> -static int tse_start_xmit(struct sk_buff *skb, struct net_device *dev)
> +static netdev_tx_t tse_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   {
>   	struct altera_tse_private *priv = netdev_priv(dev);
>   	unsigned int txsize = priv->tx_ring_size;
> 
Acked-by: Thor Thayer <thor.thayer@linux.intel.com>

Re: [PATCH v2 net-next 08/10] net: eth: altera: add support for ptp and timestamping

[Richard Cochran]

On Thu, Dec 13, 2018 at 09:52:50AM -0800, dwesterg@gmail.com wrote:
> Changes from V1:
>  -> Remove debugfs for tod manipulation
>  -> Reorder variable declarations in functions to order
>     by length of declaration, largest to smallest
>  -> Rename altera_ptp to intel_fpga_tod
>  -> Rename functions in intel_fpga_tod so that they are not
>     generic
>  -> Use imply instead of select in Kconfig
>  -> Re-write adjust time function to remove while loop with
>     64 bit divide.

Overall this is looking better.  A few more comments follow...

> +static int tse_get_ts_info(struct net_device *dev,
> +			   struct ethtool_ts_info *info)
> +{
> +	struct altera_tse_private *priv = netdev_priv(dev);
> +
> +	if (priv->ptp_enable) {
> +		if (priv->ptp_priv.ptp_clock)
> +			info->phc_index =
> +				ptp_clock_index(priv->ptp_priv.ptp_clock);

else
info->phc_index = -1;

> +		info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
> +					SOF_TIMESTAMPING_RX_HARDWARE |
> +					SOF_TIMESTAMPING_RAW_HARDWARE;
> +
> +		info->tx_types = (1 << HWTSTAMP_TX_OFF) |
> +						 (1 << HWTSTAMP_TX_ON);

This fits on one line ^^^

> +		info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
> +						   (1 << HWTSTAMP_FILTER_ALL);

Funky indentation here.  One extra level is enough:

		info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
			(1 << HWTSTAMP_FILTER_ALL);

> +		return 0;
> +	} else {
> +		return ethtool_op_get_ts_info(dev, info);
> +	}
> +}
> +

...

> @@ -609,7 +613,14 @@ static netdev_tx_t tse_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	if (ret)
>  		goto out;
>  
> -	skb_tx_timestamp(skb);
> +	/* Provide a hardware time stamp if requested.
> +	 */
> +	if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> +		     priv->hwts_tx_en))
> +		/* declare that device is doing timestamping */

Please drop those two comments.  They are redundant because they only
restate what the code does.

> +		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +	else
> +		skb_tx_timestamp(skb);
>  
>  	priv->tx_prod++;
>  	dev->stats.tx_bytes += skb->len;

Thanks,
Richard

Re: [PATCH v2 net-next 08/10] net: eth: altera: add support for ptp and timestamping

[Westergreen, Dalon]

[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]

Richard,

On Tue, 2018-12-18 at 20:27 -0800, Richard Cochran wrote:
> On Thu, Dec 13, 2018 at 09:52:50AM -0800, dwesterg@gmail.com wrote:
> > Changes from V1:
> >  -> Remove debugfs for tod manipulation
> >  -> Reorder variable declarations in functions to order
> >     by length of declaration, largest to smallest
> >  -> Rename altera_ptp to intel_fpga_tod
> >  -> Rename functions in intel_fpga_tod so that they are not
> >     generic
> >  -> Use imply instead of select in Kconfig
> >  -> Re-write adjust time function to remove while loop with
> >     64 bit divide.
> 
> Overall this is looking better.  A few more comments follow...

Thanks, all of your comments will be included in a v3 patch.

--dalon

> 
> > +static int tse_get_ts_info(struct net_device *dev,
> > +			   struct ethtool_ts_info *info)
> > +{
> > +	struct altera_tse_private *priv = netdev_priv(dev);
> > +
> > +	if (priv->ptp_enable) {
> > +		if (priv->ptp_priv.ptp_clock)
> > +			info->phc_index =
> > +				ptp_clock_index(priv->ptp_priv.ptp_clock);
> 
> else
> info->phc_index = -1;
> 


> > +		info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
> > +					SOF_TIMESTAMPING_RX_HARDWARE |
> > +					SOF_TIMESTAMPING_RAW_HARDWARE;
> > +
> > +		info->tx_types = (1 << HWTSTAMP_TX_OFF) |
> > +						 (1 << HWTSTAMP_TX_ON);
> 
> This fits on one line ^^^
> 
> > +		info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
> > +						   (1 << HWTSTAMP_FILTER_ALL);
> 
> Funky indentation here.  One extra level is enough:
> 
> 		info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
> 			(1 << HWTSTAMP_FILTER_ALL);
> 
> > +		return 0;
> > +	} else {
> > +		return ethtool_op_get_ts_info(dev, info);
> > +	}
> > +}
> > +
> 
> ...
> 
> > @@ -609,7 +613,14 @@ static netdev_tx_t tse_start_xmit(struct sk_buff *skb,
> > struct net_device *dev)
> >  	if (ret)
> >  		goto out;
> >  
> > -	skb_tx_timestamp(skb);
> > +	/* Provide a hardware time stamp if requested.
> > +	 */
> > +	if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> > +		     priv->hwts_tx_en))
> > +		/* declare that device is doing timestamping */
> 
> Please drop those two comments.  They are redundant because they only
> restate what the code does.
> 
> > +		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> > +	else
> > +		skb_tx_timestamp(skb);
> >  
> >  	priv->tx_prod++;
> >  	dev->stats.tx_bytes += skb->len;
> 
> Thanks,
> Richard

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3282 bytes --]