On Sun, Feb 7, 2021 at 1:36:15, Krzysztof Wilczyński <kw@xxxxxxxxx>
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
