RE: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver

[Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>]

On Sun, Feb 7, 2021 at 1:36:15, Krzysztof Wilczyński <kw@linux.com> 
wrote:

> Hi Gustavo,
> 
> Thank you for all the work here!
> 
> A few suggestions.
> 
> [...]
> > +static void dw_xdata_stop(struct dw_xdata *dw)
> > +{
> > +	u32 burst = readl(&(__dw_xdara_regs(dw)->burst_cnt));
> > +
> > +	if (burst & BIT(31)) {
> > +		burst &= ~(u32)BIT(31);
> > +		writel(burst, &(__dw_regs(dw)->burst_cnt));
> > +	}
> > +}
> 
> Would it be possible to add a define for this "BIT(31)", similarly to
> the "XPERF_CONTROL_ENABLE", for example:
> 
>   #define XPERF_CONTROL_ENABLE		BIT(5)
>   #define XPERF_CONTROL_DISABLE		BIT(31)
> 
> What do you think?

I will a define for them.

> 
> > +static void dw_xdata_start(struct dw_xdata *dw, bool write)
> > +{
> > +	u32 control, status;
> > +
> > +	/* Stop first if xfer in progress */
> > +	dw_xdata_stop(dw);
> > +
> > +	/* Clear status register */
> > +	writel(0x0, &(__dw_regs(dw)->status));
> > +
> > +	/* Burst count register set for continuous until stopped */
> > +	writel(0x80001001, &(__dw_regs(dw)->burst_cnt));
> 
> Would you mind adding a define (possibly with a comment, if it makes
> sense, of course) rather than open coding it here.

Ditto.

> 
> > +	/* Pattern register */
> > +	writel(0x0, &(__dw_regs(dw)->pattern));
> > +
> > +	/* Control register */
> > +	control = CONTROL_DOORBELL | CONTROL_PATTERN_INC | CONTROL_NO_ADDR_INC;
> > +	if (write) {
> > +		control |= CONTROL_IS_WRITE;
> > +		control |= CONTROL_LENGTH(dw->max_wr_len);
> > +	} else {
> > +		control |= CONTROL_LENGTH(dw->max_rd_len);
> > +	}
> > +	writel(control, &(__dw_regs(dw)->control));
> > +
> > +	usleep_range(100, 150);
> [...]
> 
> Why sleep here?
> 
> Do you just add some delay that changes were reflected, or is it
> a requirement of sorts?  What do you think about documenting why the
> sleep where? Would it make sense?

It requires to wait for some to allow the HW block to start the traffic 
generation.
I will some comments explaining that.

> 
> [...]
> > +static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
> > +{
> > +	u64 data[2], time[2], diff;
> > +
> > +	/* First measurement */
> > +	writel(0x0, &(__dw_regs(dw)->perf_control));
> > +	dw_xdata_perf_meas(dw, &data[0], write);
> > +	time[0] = jiffies;
> > +	writel((u32)XPERF_CONTROL_ENABLE, &(__dw_regs(dw)->perf_control));
> > +
> > +	/* Delay 100ms */
> > +	mdelay(100);
> [...]
> 
> The mdelay() is self-explanatory, so a comment that explains why to take
> two measurements that are 100 ms apart and how rate is calculated might
> be more useful (unless it would be an overkill here).
> 
> If this is an arbitrary delay without any special meaning, then probably
> no comment is needed here.
> 
> What do you think?
> 
> [...]
> > +	/* Calculations */
> 
> What sort of calculations precisely? :)

Speed calculation. I will add some explanation about it on the code.

> 
> [...]
> > +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> > +			       const struct pci_device_id *pid)
> > +{
> > +	const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
> > +	struct dw_xdata *dw;
> > +	u64 addr;
> > +	int err;
> > +
> > +	/* Enable PCI device */
> > +	err = pcim_enable_device(pdev);
> 
> This comment might not be needed.

I normally put these comments as a code bookmark.

> 
> [...]
> > +	/* Mapping PCI BAR regions */
> > +	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar), pci_name(pdev));
> 
> This comment might also not be needed.
> 
> [...]
> > +	/* Allocate memory */
> 
> And so this comment.
> 
> [...]
> > +	/* Data structure initialization */
> [...]
> > +	/* Saving data structure reference */
> [...]
> > +	/* Sysfs */
> [...]
> 
> And possibly few of these are also not needed.
> 
> Krzysztof

Thanks for your review. I will wait for a couple of days, before sending 
a new version of this patch series based on your feedback.

-Gustavo