Re: [dpdk-dev] [PATCH v6 2/6] raw/ntb: add intel ntb support

["Wu, Jingjing" <jingjing.wu@intel.com>]

One general comment:

Think about to use rte_read32() and rte_write32() when reading and writing registers. Or you can define a Macro or inline function for NTB driver to touch registers. Then you can omit lots of "(char *)hw->pci_dev->mem_resource[0].addr " and "*((volatile uint32_t *)" like things. It will make the code much easier to read.


> +static void *
> +intel_ntb_get_peer_mw_addr(struct rte_rawdev *dev, int mw_idx)
> +{
> +	struct ntb_hw *hw = dev->dev_private;
> +	uint8_t bar;
> +
> +	if (hw == NULL) {
> +		NTB_LOG(ERR, "Invalid device.");
> +		return 0;
> +	}
> +
> +	if (mw_idx < 0 || mw_idx > hw->mw_cnt) {
mw_idx >= hw->mw_cnt?

[...]


> +static int
> +intel_ntb_mw_set_trans(struct rte_rawdev *dev, int mw_idx,
> +		       uint64_t addr, uint64_t size)
> +{
> +	struct ntb_hw *hw = dev->dev_private;
> +	void *xlat_addr, *limit_addr;
> +	uint64_t xlat_off, limit_off;
> +	uint64_t base, limit;
> +	uint8_t bar;
> +
> +	if (hw == NULL) {
> +		NTB_LOG(ERR, "Invalid device.");
> +		return -EINVAL;
> +	}
> +
> +	if (mw_idx < 0 || mw_idx > hw->mw_cnt) {
Same as above.

[...]

> +static uint32_t
> +intel_ntb_spad_read(struct rte_rawdev *dev, int spad, bool peer)
> +{
> +	struct ntb_hw *hw = dev->dev_private;
> +	uint32_t spad_v, reg_off;
> +	void *reg_addr;
> +
> +	if (spad < 0 || spad >= hw->spad_cnt) {
> +		NTB_LOG(ERR, "Invalid spad reg index.");
> +		return 0;
> +	}
> +
> +	/* When peer is true, read peer spad reg */
> +	if (peer)
> +		reg_off = XEON_B2B_SPAD_OFFSET;
> +	else
> +		reg_off = XEON_IM_SPAD_OFFSET;

How about one line if check is simple?
reg_off = peer ? XEON_B2B_SPAD_OFFSET : XEON_IM_SPAD_OFFSET;

Thanks
Jingjing