Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <c162ab8d-127d-d998-b92b-2d61511729ff@microchip.com>
References: <1530286269-32235-1-git-send-email-harini.katakam@xilinx.com>
 <1530286269-32235-2-git-send-email-harini.katakam@xilinx.com> <c162ab8d-127d-d998-b92b-2d61511729ff@microchip.com>
From:   Harini Katakam <harinik@xilinx.com>
Date:   Wed, 4 Jul 2018 14:15:24 +0530
Message-ID: <CAFcVECL9McURN1xVhCxOFU6iTzBQW7Lsayt2AWhFddkGb3+mdA@mail.gmail.com>
Subject: Re: [RFC PATCH 2/2] net: macb: Allocate valid memory for TX and RX BD prefetch
To:     Claudiu Beznea <Claudiu.Beznea@microchip.com>
Cc:     Harini Katakam <harini.katakam@xilinx.com>,
        Nicolas Ferre <nicolas.ferre@microchip.com>,
        David Miller <davem@davemloft.net>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Michal Simek <michal.simek@xilinx.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Claudiu,

On Wed, Jul 4, 2018 at 1:52 PM, Claudiu Beznea
<Claudiu.Beznea@microchip.com> wrote:
> Hi Harini,
>
> Few comments below.
>
> Thank you,
> Claudiu Beznea
>
> On 29.06.2018 18:31, Harini Katakam wrote:
>> GEM version in ZynqMP and most versions greater than r1p07 supports
>> TX and RX BD prefetch. The number of BDs that can be prefetched is a
>> HW configurable parameter. For ZynqMP, this parameter is 4.
>>
>> When GEM DMA is accessing the last BD in the ring, even before the
>> BD is processed and the WRAP bit is noticed, it will have prefetched
>> BDs outside the BD ring. These will not be processed but it is
>> necessary to have accessible memory after the last BD. Especially
>> in cases where SMMU is used, memory locations immediately after the
>> last BD may not have translation tables triggering HRESP errors. Hence
>> always allocate extra BDs to accommodate for prefetch.
>> The value of tx/rx bd prefetch for any given SoC version is:
>> 2 ^ (corresponding field in design config 10 register).
>> (value of this field >= 1)
>>
>> Added a capability flag so that older IP versions that do not have
>> DCFG10 or this prefetch capability are not affected.
>>
>> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
>> ---
>>  drivers/net/ethernet/cadence/macb.h      | 11 +++++++++++
>>  drivers/net/ethernet/cadence/macb_main.c | 31 +++++++++++++++++++++++++------
>>  2 files changed, 36 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 8665982..b267a7b 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -166,6 +166,7 @@
>>  #define GEM_DCFG6            0x0294 /* Design Config 6 */
>>  #define GEM_DCFG7            0x0298 /* Design Config 7 */
>>  #define GEM_DCFG8            0x029C /* Design Config 8 */
>> +#define GEM_DCFG10           0x02A4 /* Design Config 10 */
>>
>>  #define GEM_TXBDCTRL 0x04cc /* TX Buffer Descriptor control register */
>>  #define GEM_RXBDCTRL 0x04d0 /* RX Buffer Descriptor control register */
>> @@ -490,6 +491,12 @@
>>  #define GEM_SCR2CMP_OFFSET                   0
>>  #define GEM_SCR2CMP_SIZE                     8
>>
>> +/* Bitfields in DCFG10 */
>> +#define GEM_TXBD_RDBUFF_OFFSET                       12
>> +#define GEM_TXBD_RDBUFF_SIZE                 4
>> +#define GEM_RXBD_RDBUFF_OFFSET                       8
>> +#define GEM_RXBD_RDBUFF_SIZE                 4
>> +
>>  /* Bitfields in TISUBN */
>>  #define GEM_SUBNSINCR_OFFSET                 0
>>  #define GEM_SUBNSINCR_SIZE                   16
>> @@ -635,6 +642,7 @@
>>  #define MACB_CAPS_USRIO_DISABLED             0x00000010
>>  #define MACB_CAPS_JUMBO                              0x00000020
>>  #define MACB_CAPS_GEM_HAS_PTP                        0x00000040
>> +#define MACB_CAPS_BD_PREFETCH                        0x00000080
>
> Rename it to MACB_CAPS_BD_RD_PREFETCH, since it is about read prefetch.
>
>>  #define MACB_CAPS_FIFO_MODE                  0x10000000
>>  #define MACB_CAPS_GIGABIT_MODE_AVAILABLE     0x20000000
>>  #define MACB_CAPS_SG_DISABLED                        0x40000000
>> @@ -1203,6 +1211,9 @@ struct macb {
>>       unsigned int max_tuples;
>>
>>       struct tasklet_struct   hresp_err_tasklet;
>> +
>> +     int     rx_bd_prefetch;
>> +     int     tx_bd_prefetch;
>
> Since it is about read prefetch I would say to rename these fields properly
> to describe this:
>         int     rx_bd_rd_prefetch;
>         int     tx_bd_rd_prefetch;
>
> or something similar as you did with macros.
>
>>  };
>>
>>  #ifdef CONFIG_MACB_USE_HWSTAMP
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index e56ffa9..a7612f6 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -1811,6 +1811,7 @@ static void macb_free_consistent(struct macb *bp)
>>  {
>>       struct macb_queue *queue;
>>       unsigned int q;
>> +     int size;
>>
>>       bp->macbgem_ops.mog_free_rx_buffers(bp);
>>
>> @@ -1818,12 +1819,16 @@ static void macb_free_consistent(struct macb *bp)
>>               kfree(queue->tx_skb);
>>               queue->tx_skb = NULL;
>>               if (queue->tx_ring) {
>> -                     dma_free_coherent(&bp->pdev->dev, TX_RING_BYTES(bp),
>> +                     size = TX_RING_BYTES(bp) +
>> +                            (macb_dma_desc_get_size(bp) * bp->tx_bd_prefetch);
>> +                     dma_free_coherent(&bp->pdev->dev, size,
>>                                         queue->tx_ring, queue->tx_ring_dma);
>>                       queue->tx_ring = NULL;
>>               }
>>               if (queue->rx_ring) {
>> -                     dma_free_coherent(&bp->pdev->dev, RX_RING_BYTES(bp),
>> +                     size = RX_RING_BYTES(bp) +
>> +                            (macb_dma_desc_get_size(bp) * bp->rx_bd_prefetch);
>> +                     dma_free_coherent(&bp->pdev->dev, size,
>>                                         queue->rx_ring, queue->rx_ring_dma);
>>                       queue->rx_ring = NULL;
>>               }
>> @@ -1873,7 +1878,8 @@ static int macb_alloc_consistent(struct macb *bp)
>>       int size;
>>
>>       for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>> -             size = TX_RING_BYTES(bp);
>> +             size = TX_RING_BYTES(bp) +
>> +                    (macb_dma_desc_get_size(bp) * bp->tx_bd_prefetch);
>>               queue->tx_ring = dma_alloc_coherent(&bp->pdev->dev, size,
>>                                                   &queue->tx_ring_dma,
>>                                                   GFP_KERNEL);
>> @@ -1889,7 +1895,8 @@ static int macb_alloc_consistent(struct macb *bp)
>>               if (!queue->tx_skb)
>>                       goto out_err;
>>
>> -             size = RX_RING_BYTES(bp);
>> +             size = RX_RING_BYTES(bp) +
>> +                    (macb_dma_desc_get_size(bp) * bp->rx_bd_prefetch);
>>               queue->rx_ring = dma_alloc_coherent(&bp->pdev->dev, size,
>>                                                &queue->rx_ring_dma, GFP_KERNEL);
>>               if (!queue->rx_ring)
>> @@ -3794,7 +3801,7 @@ static const struct macb_config np4_config = {
>>  static const struct macb_config zynqmp_config = {
>>       .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
>>                       MACB_CAPS_JUMBO |
>> -                     MACB_CAPS_GEM_HAS_PTP,
>> +                     MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_PREFETCH,
>>       .dma_burst_length = 16,
>>       .clk_init = macb_clk_init,
>>       .init = macb_init,
>> @@ -3855,7 +3862,7 @@ static int macb_probe(struct platform_device *pdev)
>>       void __iomem *mem;
>>       const char *mac;
>>       struct macb *bp;
>> -     int err;
>> +     int err, buff;
>
> I would use "val" instead of "buff"
>
>>
>>       regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       mem = devm_ioremap_resource(&pdev->dev, regs);
>> @@ -3944,6 +3951,18 @@ static int macb_probe(struct platform_device *pdev)
>>       else
>>               dev->max_mtu = ETH_DATA_LEN;
>>
>> +     bp->rx_bd_prefetch = 0;
>> +     bp->tx_bd_prefetch = 0;
>
> No need for zero init since alloc_etherdev_mq() will allocate with
> __GFP_ZERO flag (actually, kvzalloc() will be called)
>
>> +     if (bp->caps & MACB_CAPS_BD_PREFETCH) {
>> +             buff = GEM_BFEXT(RXBD_RDBUFF, gem_readl(bp, DCFG10));
>> +             if (buff)
>> +                     bp->rx_bd_prefetch = 2 << (buff - 1);
>> +
>> +             buff = GEM_BFEXT(TXBD_RDBUFF, gem_readl(bp, DCFG10));
>> +             if (buff)
>> +                     bp->tx_bd_prefetch = 2 << (buff - 1);
>> +     }
>> +
>>       mac = of_get_mac_address(np);
>>       if (mac) {
>>               ether_addr_copy(bp->dev->dev_addr, mac);
>>
>
> With these you can add:
> Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com>

Thanks for the review. I'll send a v2 with these changes.

Regards,
Harini