Hi > -----Original Message----- > From: Wu, Jingjing <jingjing...@intel.com> > Sent: Monday, September 7, 2020 16:35 > To: Li, Xiaoyun <xiaoyun...@intel.com> > Cc: dev@dpdk.org; Maslekar, Omkar <omkar.masle...@intel.com> > Subject: RE: [PATCH v3] raw/ntb: add Icelake support for Intel NTB > > > + > > +static int > > +intel_ntb_dev_init(const struct rte_rawdev *dev) { > > + struct ntb_hw *hw = dev->dev_private; > > + uint8_t bar; > > + int ret, i; > > + > > + if (hw == NULL) { > > + NTB_LOG(ERR, "Invalid device."); > > + return -EINVAL; > > + } > > + > > hw->hw_addr = (char *)hw->pci_dev->mem_resource[0].addr; > > > > + if (is_gen3_ntb(hw)) { > > + ret = intel_ntb3_check_ppd(hw); > > + } else if (is_gen4_ntb(hw)) { > > + /* PPD is in MMIO but not config space for NTB Gen4 */ > > + ret = intel_ntb4_check_ppd(hw); > > + if (ret) > > + return ret; > Above two lines are not necessary.
OK. > > + } else { > > + return -ENOTSUP; > > + } > > + > > + if (ret) > > + return ret; > > + > > hw->mw_cnt = XEON_MW_COUNT; > > hw->db_cnt = XEON_DB_COUNT; > > hw->spad_cnt = XEON_SPAD_COUNT; > > @@ -149,15 +219,28 @@ intel_ntb_mw_set_trans(const struct rte_rawdev > > *dev, int mw_idx, > > rte_write64(base, xlat_addr); > > rte_write64(limit, limit_addr); > > > > - /* Setup the external point so that remote can access. */ > > - xlat_off = XEON_EMBAR1_OFFSET + 8 * mw_idx; > > - xlat_addr = hw->hw_addr + xlat_off; > > - limit_off = XEON_EMBAR1XLMT_OFFSET + mw_idx * > > XEON_BAR_INTERVAL_OFFSET; > > - limit_addr = hw->hw_addr + limit_off; > > - base = rte_read64(xlat_addr); > > - base &= ~0xf; > > - limit = base + size; > > - rte_write64(limit, limit_addr); > > + if (is_gen3_ntb(hw)) { > > + /* Setup the external point so that remote can access. */ > > + xlat_off = XEON_EMBAR1_OFFSET + 8 * mw_idx; > > + xlat_addr = hw->hw_addr + xlat_off; > > + limit_off = XEON_EMBAR1XLMT_OFFSET + > > + mw_idx * XEON_BAR_INTERVAL_OFFSET; > > + limit_addr = hw->hw_addr + limit_off; > > + base = rte_read64(xlat_addr); > > + base &= ~0xf; > > + limit = base + size; > > + rte_write64(limit, limit_addr); > > + } else if (is_gen4_ntb(hw)) { > Can we use a variable in struct to indicate it's gen4 or gen3 after init > instead of > check it every time? What's the difference? It comes from the value in hw->pci_dev->id.device_id. Checking it in this way is trying to make it easier to extend it for gen2 ntb in the future. It's not either gen3 or gen4. I don't think it makes sense to have a bool value to indicate it's gen3 or gen4. > > > + /* Set translate base address index register */ > > + xlat_off = XEON_GEN4_IM1XBASEIDX_OFFSET + > > + mw_idx * XEON_GEN4_XBASEIDX_INTERVAL; > > + xlat_addr = hw->hw_addr + xlat_off; > > + rte_write16(rte_log2_u64(size), xlat_addr); > > + } else { > > + rte_write64(base, limit_addr); > > + rte_write64(0, xlat_addr); > > + return -ENOTSUP; > > + } > Is the else branch necessary? As if neither gen3 or gen4, the init would fail. > Would be better to print an ERR instead of just return NO support. I don't think so. Yes. It will fail in init. Returning err is to stop other following actions like in intel_ntb_vector_bind() since it should be stopped. And I'd like to keep them in one coding style. As to the print, I think that can be upper layer's job to check the value and print err. Choosing ENOTSUP is because that in init, if it's not supported hw, it will return -ENOTSUP err. > > > > return 0; > > } >