On 13/07/2020 at 17:45, Claudiu Beznea - M18063 wrote:
> Hi Nicolas,
>
>
> On 13.07.2020 13:05, nicolas.fe...@microchip.com wrote:
>> From: Nicolas Ferre <nicolas.fe...@microchip.com>
>>
>> Adapt the Wake-on-Lan feature to the Cadence GEM Ethernet controller.
>> This controller has different register layout and cannot be handled by
>> previous code.
>> We disable completely interrupts on all the queues but the queue 0.
>> Handling of WoL interrupt is done in another interrupt handler
>> positioned depending on the controller version used, just between
>> suspend() and resume() calls.
>> It allows to lower pressure on the generic interrupt hot path by
>> removing the need to handle 2 tests for each IRQ: the first figuring out
>> the controller revision, the second for actually knowing if the WoL bit
>> is set.
>>
>> Queue management in suspend()/resume() functions inspired from RFC patch
>> by Harini Katakam <hari...@xilinx.com>, thanks!
>>
>> Cc: Claudiu Beznea <claudiu.bez...@microchip.com>
>> Cc: Harini Katakam <harini.kata...@xilinx.com>
>> Signed-off-by: Nicolas Ferre <nicolas.fe...@microchip.com>
>> ---
>> Changes in v6:
>> - Values of registers usrio and scrt2 properly read in any case (WoL or not)
>> during macb_suspend() to be restored during macb_resume()
>>
>> Changes in v3:
>> - In macb_resume(), move to a more in-depth re-configuration of the
>> controller
>> even on the non-WoL path in order to accept deeper sleep states.
>> - this ends up having a phylink_stop()/phylink_start() for each of the
>> WoL/!WoL paths
>> - In macb_resume(), keep setting the MPE bit in NCR register which is needed
>> in
>> case of deep power saving mode used
>> - Tests done in "standby" as well as our deeper Power Management mode:
>> Backup Self-Refresh (BSR)
>>
>> Changes in v2:
>> - Addition of pm_wakeup_event() in WoL IRQ
>> - In macb_resume(), removal of setting the MPE bit in NCR register which is
>> not
>> needed in any case: IP is reset on the usual path and kept alive if WoL
>> is used
>> - In macb_resume(), complete reset of the controller is kept only for non-WoL
>> case. For the WoL case, we only replace the usual IRQ handler.
>>
>> drivers/net/ethernet/cadence/macb.h | 3 +
>> drivers/net/ethernet/cadence/macb_main.c | 151 +++++++++++++++++++----
>> 2 files changed, 127 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.h
>> b/drivers/net/ethernet/cadence/macb.h
>> index ab827fb4b6b9..4f1b41569260 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -90,6 +90,7 @@
>> #define GEM_SA3T 0x009C /* Specific3 Top */
>> #define GEM_SA4B 0x00A0 /* Specific4 Bottom */
>> #define GEM_SA4T 0x00A4 /* Specific4 Top */
>> +#define GEM_WOL 0x00b8 /* Wake on LAN */
>> #define GEM_EFTSH 0x00e8 /* PTP Event Frame Transmitted Seconds
>> Register 47:32 */
>> #define GEM_EFRSH 0x00ec /* PTP Event Frame Received Seconds
>> Register 47:32 */
>> #define GEM_PEFTSH 0x00f0 /* PTP Peer Event Frame Transmitted
>> Seconds Register 47:32 */
>> @@ -396,6 +397,8 @@
>> #define MACB_PDRSFT_SIZE 1
>> #define MACB_SRI_OFFSET 26 /* TSU Seconds Register Increment */
>> #define MACB_SRI_SIZE 1
>> +#define GEM_WOL_OFFSET 28 /* Enable wake-on-lan interrupt */
>> +#define GEM_WOL_SIZE 1
>>
>> /* Timer increment fields */
>> #define MACB_TI_CNS_OFFSET 0
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index e5e37aa81b02..122c54e40f91 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -1517,6 +1517,35 @@ static void macb_tx_restart(struct macb_queue *queue)
>> macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
>> }
>>
>> +static irqreturn_t gem_wol_interrupt(int irq, void *dev_id)
>> +{
>> + struct macb_queue *queue = dev_id;
>> + struct macb *bp = queue->bp;
>> + u32 status;
>> +
>> + status = queue_readl(queue, ISR);
>> +
>> + if (unlikely(!status))
>> + return IRQ_NONE;
>> +
>> + spin_lock(&bp->lock);
>> +
>> + if (status & GEM_BIT(WOL)) {
>> + queue_writel(queue, IDR, GEM_BIT(WOL));
>> + gem_writel(bp, WOL, 0);
>> + netdev_vdbg(bp->dev, "GEM WoL: queue = %u, isr = 0x%08lx\n",
>> + (unsigned int)(queue - bp->queues),
>> + (unsigned long)status);
>> + if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>> + queue_writel(queue, ISR, GEM_BIT(WOL));
>> + pm_wakeup_event(&bp->pdev->dev, 0);
>> + }
>> +
>> + spin_unlock(&bp->lock);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> static irqreturn_t macb_interrupt(int irq, void *dev_id)
>> {
>> struct macb_queue *queue = dev_id;
>> @@ -3316,6 +3345,8 @@ static const struct ethtool_ops macb_ethtool_ops = {
>> static const struct ethtool_ops gem_ethtool_ops = {
>> .get_regs_len = macb_get_regs_len,
>> .get_regs = macb_get_regs,
>> + .get_wol = macb_get_wol,
>> + .set_wol = macb_set_wol,
>> .get_link = ethtool_op_get_link,
>> .get_ts_info = macb_get_ts_info,
>> .get_ethtool_stats = gem_get_ethtool_stats,
>> @@ -4567,33 +4598,67 @@ static int __maybe_unused macb_suspend(struct device
>> *dev)
>> struct macb_queue *queue = bp->queues;
>> unsigned long flags;
>> unsigned int q;
>> + int err;
>>
>> if (!netif_running(netdev))
>> return 0;
>>
>> if (bp->wol & MACB_WOL_ENABLED) {
>> - macb_writel(bp, IER, MACB_BIT(WOL));
>> - macb_writel(bp, WOL, MACB_BIT(MAG));
>> - enable_irq_wake(bp->queues[0].irq);
>> - netif_device_detach(netdev);
>> - } else {
>> - netif_device_detach(netdev);
>> + spin_lock_irqsave(&bp->lock, flags);
>> + /* Flush all status bits */
>> + macb_writel(bp, TSR, -1);
>> + macb_writel(bp, RSR, -1);
>> for (q = 0, queue = bp->queues; q < bp->num_queues;
>> - ++q, ++queue)
>> - napi_disable(&queue->napi);
>> + ++q, ++queue) {
>> + /* Disable all interrupts */
>> + queue_writel(queue, IDR, -1);
>> + queue_readl(queue, ISR);
>> + if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>> + queue_writel(queue, ISR, -1);
>> + }
>> + /* Change interrupt handler and
>> + * Enable WoL IRQ on queue 0
>> + */
>> + if (macb_is_gem(bp)) {
>> + devm_free_irq(dev, bp->queues[0].irq, bp->queues);
>> + err = devm_request_irq(dev, bp->queues[0].irq,
>> gem_wol_interrupt,
>> + IRQF_SHARED, netdev->name,
>> bp->queues);
>> + if (err) {
>> + dev_err(dev,
>> + "Unable to request IRQ %d (error %d)\n",
>> + bp->queues[0].irq, err);
>> + return err;
>
> Restoring from this state will complicate the code here even further.
> At least release the spinlock before exiting.
Oh yes, good catch Claudiu. It'll be fixed in v7.
I give this series a couple of days more and plan to re-send by the end
of the week.
>> + }
>> + queue_writel(bp->queues, IER, GEM_BIT(WOL));
>> + gem_writel(bp, WOL, MACB_BIT(MAG));
>> + } else {
>> + queue_writel(bp->queues, IER, MACB_BIT(WOL));
>> + macb_writel(bp, WOL, MACB_BIT(MAG));
>> + }
>> + spin_unlock_irqrestore(&bp->lock, flags);
>> +
>> + enable_irq_wake(bp->queues[0].irq);
>> + }
>> +
[..]
>> + /* Replace interrupt handler on queue 0 */
>> + devm_free_irq(dev, bp->queues[0].irq, bp->queues);
>> + err = devm_request_irq(dev, bp->queues[0].irq, macb_interrupt,
>> + IRQF_SHARED, netdev->name, bp->queues);
>> + if (err) {
>> + dev_err(dev,
>> + "Unable to request IRQ %d (error %d)\n",
>> + bp->queues[0].irq, err);
>> + return err;
>
> Same here.
Ok.
>> + }
>> + spin_unlock_irqrestore(&bp->lock, flags);
[..]
Thanks for your review. Best regards,
--
Nicolas Ferre
