Re: [PATCH 02/12] net: ll_temac: Extend support to non-device-tree platforms

Re: [PATCH 02/12] net: ll_temac: Extend support to non-device-tree platforms

[Andrew Lunn]

On Fri, Apr 26, 2019 at 09:32:21AM +0200, Esben Haabendal wrote:
> Support initialization with platdata, so the driver can be used on
> non-device-tree platforms.
> 
> For currently supported device-tree platforms, the driver should behave
> as before.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/net/ethernet/xilinx/ll_temac.h      |   3 +
>  drivers/net/ethernet/xilinx/ll_temac_main.c | 187 +++++++++++++++++++---------
>  drivers/net/ethernet/xilinx/ll_temac_mdio.c |  23 +++-
>  include/linux/xilinx_ll_temac.h             |  18 +++

Hi Esben

Please place platform data include files in include/linux/platform_data/

Thanks
	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 10/12] net: ll_temac: Replace bad usage of msleep() with usleep_range()

[Andrew Lunn]

On Fri, Apr 26, 2019 at 09:32:29AM +0200, Esben Haabendal wrote:
> Use usleep_range() to avoid problems with msleep() actually sleeping
> much longer than expected.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 06/12] net: ll_temac: Allow use on x86 platforms

[Andrew Lunn]

On Fri, Apr 26, 2019 at 09:32:25AM +0200, Esben Haabendal wrote:
> With little-endian and 64-bit support in place, the ll_temac driver can
> now be used on x86 and x86_64 platforms.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/net/ethernet/xilinx/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/Kconfig b/drivers/net/ethernet/xilinx/Kconfig
> index 6d68c8a..6f858f6 100644
> --- a/drivers/net/ethernet/xilinx/Kconfig
> +++ b/drivers/net/ethernet/xilinx/Kconfig
> @@ -5,7 +5,7 @@
>  config NET_VENDOR_XILINX
>  	bool "Xilinx devices"
>  	default y
> -	depends on PPC || PPC32 || MICROBLAZE || ARCH_ZYNQ || MIPS
> +	depends on PPC || PPC32 || MICROBLAZE || ARCH_ZYNQ || MIPS || X86

Hi Esben

While you are here, maybe also add COMPILE_TEST here and to
XILINX_LL_TEMAC?

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/12] net: ll_temac: Support indirect_mutex share within TEMAC IP

[Andrew Lunn]

On Fri, Apr 26, 2019 at 09:32:26AM +0200, Esben Haabendal wrote:
> @@ -1092,7 +1092,15 @@ static int temac_probe(struct platform_device *pdev)
>  	lp->dev = &pdev->dev;
>  	lp->options = XTE_OPTION_DEFAULTS;
>  	spin_lock_init(&lp->rx_lock);
> -	mutex_init(&lp->indirect_mutex);
> +
> +	/* Setup mutex for synchronization of indirect register access */
> +	if (pdata && pdata->indirect_mutex) {
> +		lp->indirect_mutex = pdata->indirect_mutex;
> +	} else {
> +		lp->indirect_mutex = devm_kmalloc(
> +			&pdev->dev, sizeof(*lp->indirect_mutex), GFP_KERNEL);
> +		mutex_init(lp->indirect_mutex);
> +	}

Hi Esben

I would make the mutex mandatory, not optional. I think there will be
less hard to debug errors that way. You want the developer to actually
think about this mutex, should it be shared, or individual. Forcing
them to provide it means they are more likely to read the
documentation, and more likely to over share it than under share
it. That is maybe not so good for performance, but safer.

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/12] net: ll_temac: Fix iommu/swiotlb leak

[Andrew Lunn]

On Fri, Apr 26, 2019 at 09:32:27AM +0200, Esben Haabendal wrote:
> Unmap the actual buffer length, not the amount of data received.

Hi Esben

The patch Subject does not seem to match the content?

Also, there can be performance advantages of just unmapping the
received length. The unmap operation does a cache invalidate, which
can be expensive. Consider the effort of unmapping a 64 byte ACK vs 9K
jumbo frame?

      Andrew
 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/net/ethernet/xilinx/ll_temac_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c
> index 309f149..56d8077 100644
> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c
> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
> @@ -821,7 +821,7 @@ static void ll_temac_recv(struct net_device *ndev)
>  		length = be32_to_cpu(cur_p->app4) & 0x3FFF;
>  
>  		dma_unmap_single(ndev->dev.parent, be32_to_cpu(cur_p->phys),
> -				 length, DMA_FROM_DEVICE);
> +				 XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
>  
>  		skb_put(skb, length);
>  		skb->protocol = eth_type_trans(skb, ndev);
> -- 
> 2.4.11
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/12] net: ll_temac: Fix iommu/swiotlb leak

[Robin Murphy]

On 26/04/2019 15:21, Andrew Lunn wrote:
> On Fri, Apr 26, 2019 at 09:32:27AM +0200, Esben Haabendal wrote:
>> Unmap the actual buffer length, not the amount of data received.
> 
> Hi Esben
> 
> The patch Subject does not seem to match the content?
> 
> Also, there can be performance advantages of just unmapping the
> received length. The unmap operation does a cache invalidate, which
> can be expensive. Consider the effort of unmapping a 64 byte ACK vs 9K
> jumbo frame?

If the size passed to dma_unmap_*() is not the same as was passed to the 
corresponding dma_map_*(), that is fundamentally incorrect use of the 
API and may lead to warnings, resource exhaustion, or possibly even 
corruption and crashes for some DMA API implementations.

If there's a case where you just need to look at a small part of the 
buffer right now, but can unmap the whole thing properly later. then 
dma_sync_single_*() does allow operating on partial buffers. Even 
better, if you're able to recycle buffers in your Rx pool you could 
potentially replace the unmap/map dance altogether with some careful use 
of sync_single.

Robin.

> 
>        Andrew
>   
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>>   drivers/net/ethernet/xilinx/ll_temac_main.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c
>> index 309f149..56d8077 100644
>> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c
>> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
>> @@ -821,7 +821,7 @@ static void ll_temac_recv(struct net_device *ndev)
>>   		length = be32_to_cpu(cur_p->app4) & 0x3FFF;
>>   
>>   		dma_unmap_single(ndev->dev.parent, be32_to_cpu(cur_p->phys),
>> -				 length, DMA_FROM_DEVICE);
>> +				 XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
>>   
>>   		skb_put(skb, length);
>>   		skb->protocol = eth_type_trans(skb, ndev);
>> -- 
>> 2.4.11
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/12] net: ll_temac: Fix iommu/swiotlb leak

[Andrew Lunn]

On Fri, Apr 26, 2019 at 03:43:20PM +0100, Robin Murphy wrote:
> On 26/04/2019 15:21, Andrew Lunn wrote:
> >On Fri, Apr 26, 2019 at 09:32:27AM +0200, Esben Haabendal wrote:
> >>Unmap the actual buffer length, not the amount of data received.
> >
> >Hi Esben
> >
> >The patch Subject does not seem to match the content?
> >
> >Also, there can be performance advantages of just unmapping the
> >received length. The unmap operation does a cache invalidate, which
> >can be expensive. Consider the effort of unmapping a 64 byte ACK vs 9K
> >jumbo frame?
> 
> If the size passed to dma_unmap_*() is not the same as was passed to the
> corresponding dma_map_*(), that is fundamentally incorrect use of the API
> and may lead to warnings, resource exhaustion, or possibly even corruption
> and crashes for some DMA API implementations.
> 
> If there's a case where you just need to look at a small part of the buffer
> right now, but can unmap the whole thing properly later. then
> dma_sync_single_*() does allow operating on partial buffers. Even better, if
> you're able to recycle buffers in your Rx pool you could potentially replace
> the unmap/map dance altogether with some careful use of sync_single.

Hi Robin

Thanks for the info.

I went back to the driver i was thinking of, and it is using
dma_sync_single_range_for_cpu() for just the received packet length.

Sorry for the mixup.

      Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/12] net: ll_temac: Fix support for 64-bit platforms

[Jakub Kicinski]

On Fri, 26 Apr 2019 09:32:22 +0200, Esben Haabendal wrote:
> The use of buffer descriptor APP4 field (32-bit) for storing skb pointer
> obviously does not work on 64-bit platforms.
> As APP3 is also unused, we can use that to store the other half of 64-bit
> pointer values.
> 
> Contrary to what is hinted at in commit message of commit 15bfe05c8d63
> ("net: ethernet: xilinx: Mark XILINX_LL_TEMAC broken on 64-bit")
> there are no other pointers stored in cdmac_bd.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>

This is a bit strange, the driver stores the host's virtual address into
the HW descriptor?

> diff --git a/drivers/net/ethernet/xilinx/Kconfig b/drivers/net/ethernet/xilinx/Kconfig
> index da4ec57..6d68c8a 100644
> --- a/drivers/net/ethernet/xilinx/Kconfig
> +++ b/drivers/net/ethernet/xilinx/Kconfig
> @@ -34,7 +34,6 @@ config XILINX_AXI_EMAC
>  config XILINX_LL_TEMAC
>  	tristate "Xilinx LL TEMAC (LocalLink Tri-mode Ethernet MAC) driver"
>  	depends on (PPC || MICROBLAZE)
> -	depends on !64BIT || BROKEN
>  	select PHYLIB
>  	---help---
>  	  This driver supports the Xilinx 10/100/1000 LocalLink TEMAC
> diff --git a/drivers/net/ethernet/xilinx/ll_temac_main.c b/drivers/net/ethernet/xilinx/ll_temac_main.c
> index 4594fe3..a365bfd 100644
> --- a/drivers/net/ethernet/xilinx/ll_temac_main.c
> +++ b/drivers/net/ethernet/xilinx/ll_temac_main.c
> @@ -619,11 +619,39 @@ static void temac_adjust_link(struct net_device *ndev)
>  	mutex_unlock(&lp->indirect_mutex);
>  }
>  
> +#ifdef CONFIG_64BIT
> +
> +void ptr_to_txbd(void *p, struct cdmac_bd *bd)
> +{
> +	bd->app3 = (u32)(((u64)p) >> 32);
> +	bd->app4 = (u32)((u64)p & 0xFFFFFFFF);
> +}
> +
> +void *ptr_from_txbd(struct cdmac_bd *bd)
> +{
> +	return (void *)(((u64)(bd->app3) << 32) | bd->app4);
> +}
> +
> +#else
> +
> +void ptr_to_txbd(void *p, struct cmdac_bd *bd)
> +{
> +	bd->app4 = (u32)p;
> +}
> +
> +void *ptr_from_txbd(struct cdmac_bd *bd)
> +{
> +	return (void *)(bd->app4);
> +}
> +
> +#endif
> +
>  static void temac_start_xmit_done(struct net_device *ndev)
>  {
>  	struct temac_local *lp = netdev_priv(ndev);
>  	struct cdmac_bd *cur_p;
>  	unsigned int stat = 0;
> +	struct sk_buff *skb;
>  
>  	cur_p = &lp->tx_bd_v[lp->tx_bd_ci];
>  	stat = cur_p->app0;
> @@ -631,8 +659,9 @@ static void temac_start_xmit_done(struct net_device *ndev)
>  	while (stat & STS_CTRL_APP0_CMPLT) {
>  		dma_unmap_single(ndev->dev.parent, cur_p->phys, cur_p->len,
>  				 DMA_TO_DEVICE);
> -		if (cur_p->app4)
> -			dev_consume_skb_irq((struct sk_buff *)cur_p->app4);
> +		skb = (struct sk_buff *)ptr_from_txbd(cur_p);
> +		if (skb)
> +			dev_consume_skb_irq(skb);
>  		cur_p->app0 = 0;
>  		cur_p->app1 = 0;
>  		cur_p->app2 = 0;
> @@ -711,7 +740,7 @@ temac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	cur_p->len = skb_headlen(skb);
>  	cur_p->phys = dma_map_single(ndev->dev.parent, skb->data,
>  				     skb_headlen(skb), DMA_TO_DEVICE);
> -	cur_p->app4 = (unsigned long)skb;
> +	ptr_to_txbd((void *)skb, cur_p);
>  
>  	for (ii = 0; ii < num_frag; ii++) {
>  		lp->tx_bd_tail++;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/12] net: ll_temac: Fix support for 64-bit platforms

[Andrew Lunn]

On Fri, Apr 26, 2019 at 11:40:13AM -0700, Jakub Kicinski wrote:
> On Fri, 26 Apr 2019 09:32:22 +0200, Esben Haabendal wrote:
> > The use of buffer descriptor APP4 field (32-bit) for storing skb pointer
> > obviously does not work on 64-bit platforms.
> > As APP3 is also unused, we can use that to store the other half of 64-bit
> > pointer values.
> > 
> > Contrary to what is hinted at in commit message of commit 15bfe05c8d63
> > ("net: ethernet: xilinx: Mark XILINX_LL_TEMAC broken on 64-bit")
> > there are no other pointers stored in cdmac_bd.
> > 
> > Signed-off-by: Esben Haabendal <esben@geanix.com>
> 
> This is a bit strange, the driver stores the host's virtual address into
> the HW descriptor?

Hi Jukub

This is reasonably common. You need some sort of cookie which links
the hardware descriptor to the skbuf it points to. The hardware makes
no use of it, it is just a cookie.

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/12] net: ll_temac: Fix support for 64-bit platforms

[Jakub Kicinski]

On Fri, 26 Apr 2019 22:59:12 +0200, Andrew Lunn wrote:
> On Fri, Apr 26, 2019 at 11:40:13AM -0700, Jakub Kicinski wrote:
> > On Fri, 26 Apr 2019 09:32:22 +0200, Esben Haabendal wrote:  
> > > The use of buffer descriptor APP4 field (32-bit) for storing skb pointer
> > > obviously does not work on 64-bit platforms.
> > > As APP3 is also unused, we can use that to store the other half of 64-bit
> > > pointer values.
> > > 
> > > Contrary to what is hinted at in commit message of commit 15bfe05c8d63
> > > ("net: ethernet: xilinx: Mark XILINX_LL_TEMAC broken on 64-bit")
> > > there are no other pointers stored in cdmac_bd.
> > > 
> > > Signed-off-by: Esben Haabendal <esben@geanix.com>  
> > 
> > This is a bit strange, the driver stores the host's virtual address into
> > the HW descriptor?  
> 
> Hi Jukub

I need to start keeping track of all the ways my name gets spelled :)
I find it entertaining :)

> This is reasonably common. You need some sort of cookie which links
> the hardware descriptor to the skbuf it points to. The hardware makes
> no use of it, it is just a cookie.

Right, but accesses to HW descriptor memory ring are significantly 
more expensive, especially on platforms which are not coherent with 
DMA operations (everything but x86?)

A preferable design is to have two descriptor rings - one for HW
descriptors and one for software context, no?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/12] net: ll_temac: Fix support for 64-bit platforms

[Andrew Lunn]

On Fri, Apr 26, 2019 at 02:08:56PM -0700, Jakub Kicinski wrote:
> On Fri, 26 Apr 2019 22:59:12 +0200, Andrew Lunn wrote:
> > On Fri, Apr 26, 2019 at 11:40:13AM -0700, Jakub Kicinski wrote:
> > > On Fri, 26 Apr 2019 09:32:22 +0200, Esben Haabendal wrote:  
> > > > The use of buffer descriptor APP4 field (32-bit) for storing skb pointer
> > > > obviously does not work on 64-bit platforms.
> > > > As APP3 is also unused, we can use that to store the other half of 64-bit
> > > > pointer values.
> > > > 
> > > > Contrary to what is hinted at in commit message of commit 15bfe05c8d63
> > > > ("net: ethernet: xilinx: Mark XILINX_LL_TEMAC broken on 64-bit")
> > > > there are no other pointers stored in cdmac_bd.
> > > > 
> > > > Signed-off-by: Esben Haabendal <esben@geanix.com>  
> > > 
> > > This is a bit strange, the driver stores the host's virtual address into
> > > the HW descriptor?  

Lets try that again

Hi Jakub

> > Hi Jukub
> 
> I need to start keeping track of all the ways my name gets spelled :)
> I find it entertaining :)

Sorry. 

And i prefer entertaining over offended :-)

> > This is reasonably common. You need some sort of cookie which links
> > the hardware descriptor to the skbuf it points to. The hardware makes
> > no use of it, it is just a cookie.
> 
> Right, but accesses to HW descriptor memory ring are significantly 
> more expensive, especially on platforms which are not coherent with 
> DMA operations (everything but x86?)
>
> A preferable design is to have two descriptor rings - one for HW
> descriptors and one for software context, no?

Modern drivers do that. But this driver seems to be quite old.  And if
you look at what it is used on, PPC & MICROBLAZE, they are old
architectures, i don't think hardware access are that as expensive as
for modern architectures.

	  Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/12] net: ll_temac: Fix support for 64-bit platforms

[Jakub Kicinski]

On Sat, 27 Apr 2019 00:02:26 +0200, Andrew Lunn wrote:
> On Fri, Apr 26, 2019 at 02:08:56PM -0700, Jakub Kicinski wrote:
> > On Fri, 26 Apr 2019 22:59:12 +0200, Andrew Lunn wrote:  
> > > On Fri, Apr 26, 2019 at 11:40:13AM -0700, Jakub Kicinski wrote:  
> > > > On Fri, 26 Apr 2019 09:32:22 +0200, Esben Haabendal wrote:    
> > > > > The use of buffer descriptor APP4 field (32-bit) for storing skb pointer
> > > > > obviously does not work on 64-bit platforms.
> > > > > As APP3 is also unused, we can use that to store the other half of 64-bit
> > > > > pointer values.
> > > > > 
> > > > > Contrary to what is hinted at in commit message of commit 15bfe05c8d63
> > > > > ("net: ethernet: xilinx: Mark XILINX_LL_TEMAC broken on 64-bit")
> > > > > there are no other pointers stored in cdmac_bd.
> > > > > 
> > > > > Signed-off-by: Esben Haabendal <esben@geanix.com>    
> > > > 
> > > > This is a bit strange, the driver stores the host's virtual address into
> > > > the HW descriptor?    
> 
> Lets try that again
> 
> Hi Jakub

:)

> > > Hi Jukub  
> > 
> > I need to start keeping track of all the ways my name gets spelled :)
> > I find it entertaining :)  
> 
> Sorry. 
> 
> And i prefer entertaining over offended :-)

Certainly no offence taken! :)

> > > This is reasonably common. You need some sort of cookie which links
> > > the hardware descriptor to the skbuf it points to. The hardware makes
> > > no use of it, it is just a cookie.  
> > 
> > Right, but accesses to HW descriptor memory ring are significantly 
> > more expensive, especially on platforms which are not coherent with 
> > DMA operations (everything but x86?)
> >
> > A preferable design is to have two descriptor rings - one for HW
> > descriptors and one for software context, no?  
> 
> Modern drivers do that. But this driver seems to be quite old.  And if
> you look at what it is used on, PPC & MICROBLAZE, they are old
> architectures, i don't think hardware access are that as expensive as
> for modern architectures.

True, my comment was certainly more of a suggestion than a blocker.

Looking closer at the series it kind of looks like a soft IP.
Esben, is there anything architecture specific here?  Should we perhaps
drop the dependency on the architectures in patch 6 completely?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/12] net: ll_temac: Fix support for 64-bit platforms

[Andrew Lunn]

On Mon, Apr 29, 2019 at 10:34:13AM +0200, Esben Haabendal wrote:
> The use of buffer descriptor APP4 field (32-bit) for storing skb pointer
> obviously does not work on 64-bit platforms.
> As APP3 is also unused, we can use that to store the other half of 64-bit
> pointer values.
> 
> Contrary to what is hinted at in commit message of commit 15bfe05c8d63
> ("net: ethernet: xilinx: Mark XILINX_LL_TEMAC broken on 64-bit")
> there are no other pointers stored in cdmac_bd.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 06/12] net: ll_temac: Allow use on x86 platforms

[Andrew Lunn]

On Mon, Apr 29, 2019 at 10:34:16AM +0200, Esben Haabendal wrote:
> With little-endian and 64-bit support in place, the ll_temac driver can
> now be used on x86 and x86_64 platforms.
> 
> And while at it, enable COMPILE_TEST also.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/12] net: ll_temac: Support indirect_mutex share within TEMAC IP

[Andrew Lunn]

> For OF devices, the xlnx,compound parent of the temac node should be
> used to find siblings, and setup a shared indirect_mutex between them.
> I will leave this work to somebody else, as I don't have hardware to
> test that.  No regression is introduced by that, as before this commit
> using two Ethernet interfaces in same TEMAC block is simply broken.

Is that true?

> @@ -1092,7 +1092,16 @@ static int temac_probe(struct platform_device *pdev)
>  	lp->dev = &pdev->dev;
>  	lp->options = XTE_OPTION_DEFAULTS;
>  	spin_lock_init(&lp->rx_lock);
> -	mutex_init(&lp->indirect_mutex);
> +
> +	/* Setup mutex for synchronization of indirect register access */
> +	if (pdata) {
> +		if (!pdata->indirect_mutex) {
> +			dev_err(&pdev->dev,
> +				"indirect_mutex missing in platform_data\n");
> +			return -EINVAL;
> +		}
> +		lp->indirect_mutex = pdata->indirect_mutex;
> +	}

In the OF case, isn't lp->indirect_mutex now a NULL pointer, where as
before it was a valid mutex?

Or did i miss something somewhere?

   Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 07/12] net: ll_temac: Support indirect_mutex share within TEMAC IP

[Andrew Lunn]

On Tue, Apr 30, 2019 at 09:17:54AM +0200, Esben Haabendal wrote:
> Indirect register access goes through a DCR bus bridge, which
> allows only one outstanding transaction.  And to make matters
> worse, each TEMAC IP block contains two Ethernet interfaces, and
> although they seem to have separate registers for indirect access,
> they actually share the registers.  Or to be more specific, MSW, LSW
> and CTL registers are physically shared between Ethernet interfaces
> in same TEMAC IP, with RDY register being (almost) specificic to
> the Ethernet interface.  The 0x10000 bit in RDY reflects combined
> bus ready state though.
> 
> So we need to take care to synchronize not only within a single
> device, but also between devices in same TEMAC IP.
> 
> This commit allows to do that with legacy platform devices.
> 
> For OF devices, the xlnx,compound parent of the temac node should be
> used to find siblings, and setup a shared indirect_mutex between them.
> I will leave this work to somebody else, as I don't have hardware to
> test that.  No regression is introduced by that, as before this commit
> using two Ethernet interfaces in same TEMAC block is simply broken.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 08/12] net: ll_temac: Fix iommu/swiotlb leak

[Andrew Lunn]

On Tue, Apr 30, 2019 at 09:17:55AM +0200, Esben Haabendal wrote:
> Unmap the actual buffer length, not the amount of data received, avoiding
> resource exhaustion of swiotlb (seen on x86_64 platform).
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 00/12] net: ll_temac: x86_64 support

[David Miller]

From: Esben Haabendal <esben@geanix.com>
Date: Tue, 30 Apr 2019 09:17:47 +0200

> This patch series adds support for use of ll_temac driver with
> platform_data configuration and fixes endianess and 64-bit problems so
> that it can be used on x86_64 platform.
> 
> A few bugfixes are also included.

Series applied to net-next.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel