From: "Yu, Xiangliang" <Xiangliang.Yu@amd.com>
To: Allen Hubbe <allenbh@gmail.com>
CC: "jdmason@kudzu.us" <jdmason@kudzu.us>,
        "dave.jiang@intel.com" <dave.jiang@intel.com>,
        "linux-ntb@googlegroups.com" <linux-ntb@googlegroups.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        SPG_Linux_Kernel <SPG_Linux_Kernel@amd.com>
Subject: RE: [PATCH 1/3] NTB: Add AMD PCI-Express NTB driver
Thread-Topic: [PATCH 1/3] NTB: Add AMD PCI-Express NTB driver
Thread-Index: AQHROHm1wOqCW2NzMUWtpNAMMJQ0JJ7PY8KAgADujWA=
Date: Fri, 18 Dec 2015 07:18:23 +0000
Message-ID: <BLUPR12MB04202381D9B14C68B3962A95EBE10@BLUPR12MB0420.namprd12.prod.outlook.com>
References: <1450340265-18912-1-git-send-email-Xiangliang.Yu@amd.com>
 <CAJ80satY97nwd5hJgN=F+a4fAwpqxh8nV5a7hHm6WK=2qqhOig@mail.gmail.com>
In-Reply-To: <CAJ80satY97nwd5hJgN=F+a4fAwpqxh8nV5a7hHm6WK=2qqhOig@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Dec 2015 07:18:23.1008
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR12MB0420
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id tBI7IXl7021878
Content-Length: 5599
Lines: 183

> From: Allen Hubbe [mailto:allenbh@gmail.com]
> Sent: Friday, December 18, 2015 12:46 AM
> To: Yu, Xiangliang
> Cc: jdmason@kudzu.us; dave.jiang@intel.com; linux-ntb@googlegroups.com;
> linux-kernel@vger.kernel.org; SPG_Linux_Kernel
> Subject: Re: [PATCH 1/3] NTB: Add AMD PCI-Express NTB driver
> 
> On Thu, Dec 17, 2015 at 3:17 AM, Xiangliang Yu <Xiangliang.Yu@amd.com>
> wrote:
> > AMD NTB support following main features:
> > (1) Three memory windows;
> > (2) Sixteen 32-bit scratch pad;
> > (3) Two 16-bit doorbell interrupt;
> > (4) Five event interrupts;
> > (5) One system can wake up opposite system of NTB;
> > (6) Flush previous request to the opposite system;
> > (7) There are reset and PME_TO mechanisms between two systems;
> >
> > Signed-off-by: Xiangliang Yu <Xiangliang.Yu@amd.com>
> 
> Is hardware available on which this can be tested?

No yet. Right now, verified the driver on emulator.

> 
> > +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
> 
> > +static u64 amd_ntb_db_read(struct ntb_dev *ntb) {
> > +       struct amd_ntb_dev *ndev = ntb_ndev(ntb);
> > +
> > +       return (u64)NTB_READ_REG(DBSTAT); }
> 
> DBSTAT hides the use of ndev, or ndev is unused.  The code should be more
> clear, here, and in other places where NTB_READ_REG and NTB_WRITE_REG
> are used with a macro argument.

Got it, I will add ndev into macro arguments.

> 
> > +static void amd_ack_SMU(struct amd_ntb_dev *ndev, u32 bit) {
> > +       int reg;
> > +
> > +       reg = NTB_READ_REG(SMUACK);
> > +       reg |= bit;
> > +       NTB_WRITE_REG(reg, SMUACK);
> > +
> > +       ndev->peer_sta |= bit;
> > +}
> > +
> > +/*
> > + * flush the requests to peer side
> > + */
> > +static int amd_flush_peer_requests(struct amd_ntb_dev *ndev) {
> > +       u32 reg;
> > +
> > +       if (!amd_link_is_up(ndev)) {
> > +               dev_err(ndev_dev(ndev), "link is down.\n");
> > +               return -EINVAL;
> > +       }
> > +
> 
> Add reinit_completion, or this may already be "complete" from a previous
> flush.

Ok.

> 
> > +       reg = NTB_READ_REG(FLUSHTRIG);
> > +       reg |= 0x1;
> > +       NTB_WRITE_REG(reg, FLUSHTRIG);
> > +
> > +       wait_for_completion(&ndev->flush_cmpl);
> 
> Because of wait_for_completion, that this can only be called in a thread
> context.  This is unlike other functions of ntb.h, so there should at least be a
> note in the api documentation.

Ok.
> 
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * wake up the peer side
> > + */
> > +static int amd_wakeup_peer_side(struct amd_ntb_dev *ndev) {
> > +       u32 reg;
> > +
> > +       if (!amd_link_is_up(ndev)) {
> > +               dev_warn(ndev_dev(ndev), "link is down.\n");
> > +               return -EINVAL;
> > +       }
> > +
> 
> See previous comment.
> 
> > +       NTB_READ_REG(PMSGTRIG);
> > +       reg |= 0x1;
> > +       NTB_WRITE_REG(reg, PMSGTRIG);
> > +
> > +       wait_for_completion(&ndev->wakeup_cmpl);
> > +
> > +       return 0;
> > +}
> 
> 
> > +static void amd_handle_event(struct amd_ntb_dev *ndev, int vec) {
> > +       u32 status;
> > +
> > +       status = NTB_READ_REG(INTSTAT);
> > +       if (!(status & AMD_EVENT_INTMASK))
> > +               return;
> > +
> > +       dev_dbg(ndev_dev(ndev), "status = 0x%x and vec = %d\n",
> > + status, vec);
> > +
> > +       status &= AMD_EVENT_INTMASK;
> > +       switch (status) {
> > +       case AMD_PEER_FLUSH_EVENT:
> > +               complete(&ndev->flush_cmpl);
> > +               break;
> > +       case AMD_PEER_RESET_EVENT:
> > +               amd_ack_SMU(ndev, AMD_PEER_RESET_EVENT);
> > +
> > +               /* link down first */
> > +               ntb_link_event(&ndev->ntb);
> > +               /* polling peer status */
> > +               schedule_delayed_work(&ndev->hb_timer,
> > + AMD_LINK_HB_TIMEOUT);
> > +
> > +               break;
> > +       case AMD_PEER_D3_EVENT:
> > +       case AMD_PEER_PMETO_EVENT:
> > +               amd_ack_SMU(ndev, status);
> > +
> > +               /* link down */
> > +               ntb_link_event(&ndev->ntb);
> > +
> > +               break;
> > +       case AMD_PEER_D0_EVENT:
> > +               status = NTB_READ_PEER_REG(PMESTAT);
> > +               /* check if this is WAKEUP event */
> > +               if (status & 0x1)
> > +                       complete(&ndev->wakeup_cmpl);
> > +
> > +               amd_ack_SMU(ndev, AMD_PEER_D0_EVENT);
> > +
> > +               if (amd_link_is_up(ndev))
> > +                       ntb_link_event(&ndev->ntb);
> > +               else
> > +                       schedule_delayed_work(&ndev->hb_timer,
> > +                                               AMD_LINK_HB_TIMEOUT);
> > +               break;
> > +       default:
> > +               pr_err("Unsupported interrupt.\n");
> > +               break;
> > +       }
> > +}
> > +
> > +static irqreturn_t ndev_interrupt(struct amd_ntb_dev *ndev, int vec)
> > +{
> > +       dev_dbg(ndev_dev(ndev), "vec %d\n", vec);
> > +
> > +       if (vec > 20) {
> 
> This duplicates the "default" case of amd_handle_event.
Ok, I'll refine the code.

> 
> > +               dev_err(ndev_dev(ndev), "Invalid interrupt.\n");
> > +               return IRQ_HANDLED;
> > +       }
> > +
> > +       if (vec > 16 || (ndev->msix_vec_count == 1))
> > +               amd_handle_event(ndev, vec);
> > +
> > +       if (vec < 16)
> > +               ntb_db_event(&ndev->ntb, vec);
> > +
> > +       return IRQ_HANDLED;
> > +}
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?