Re: [net-next v3 2/6] net: marvell: prestera: Add PCI interface support

[Vadym Kochan]

Hi Andy,

On Mon, Jul 27, 2020 at 11:04:56AM +0300, Andy Shevchenko wrote:
> On Mon, Jul 27, 2020 at 1:55 AM Vadym Kochan <vadym.kochan@xxxxxxxxxxx> wrote:
> > On Sun, Jul 26, 2020 at 01:32:19PM +0300, Andy Shevchenko wrote:
> > > On Sat, Jul 25, 2020 at 6:10 PM Vadym Kochan <vadym.kochan@xxxxxxxxxxx> wrote:
> 
> ...
> 
> For the non-commented I assume you are agree with. Correct?
> 
Yes

> ...
> 
> > > > +config PRESTERA_PCI
> > > > +       tristate "PCI interface driver for Marvell Prestera Switch ASICs family"
> > > > +       depends on PCI && HAS_IOMEM && PRESTERA
> > >
> > > > +       default m
> > >
> > > Even if I have CONFIG_PRESTERA=y, why as a user I must have this as a module?
> > > If it's a crucial feature, shouldn't it be rather
> > >   default CONFIG_PRESTERA
> > > ?
> >
> > The firmware image should be located on rootfs, and in case the rootfs
> > should be mounted later the pci driver can't pick this up when
> > statically compiled so I left it as 'm' by default.
> 
> We have for a long time to catch firmware blobs from initrd (initramfs).
> default m is very unusual.
> 
For example drivers/net/ethernet/mellanox/mlxsw/pci.c also uses 'm' as
default, but may be in that case the reason is that there are several
bus implementations - i2c, pci.

> ...
> 
> > > > +#define PRESTERA_FW_PATH \
> > > > +       "mrvl/prestera/mvsw_prestera_fw-v" \
> > > > +       __stringify(PRESTERA_SUPP_FW_MAJ_VER) \
> > > > +       "." __stringify(PRESTERA_SUPP_FW_MIN_VER) ".img"
> > >
> > > Wouldn't it be better to see this in the C code?
> >
> > I have no strong opinion on this, but looks like macro is enough for
> > this statically defined versioning.
> 
> The problem is that you have to bounce your editor to C code then to
> macro then to another macro...
> (in case you are looking for the code responsible for that)
> In many drivers I saw either it's one static line (without those
> __stringify(), etc) or done in C code dynamically near to
> request_firmware() call.
> 
> Maybe you may replace __stringify by explicit characters / strings and
> comment how the name was constructed?
> 
> #define FW_NAME "patch/to/it/fileX.Y.img"
> 
I used snprintf, and now it looks simpler.

> ...
> 
> > > > +static void prestera_pci_copy_to(u8 __iomem *dst, u8 *src, size_t len)
> > > > +{
> > > > +       u32 __iomem *dst32 = (u32 __iomem *)dst;
> > > > +       u32 *src32 = (u32 *)src;
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < (len / 4); dst32++, src32++, i++)
> > > > +               writel_relaxed(*src32, dst32);
> > > > +}
> > > > +
> > > > +static void prestera_pci_copy_from(u8 *dst, u8 __iomem *src, size_t len)
> > > > +{
> > > > +       u32 __iomem *src32 = (u32 __iomem *)src;
> > > > +       u32 *dst32 = (u32 *)dst;
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < (len / 4); dst32++, src32++, i++)
> > > > +               *dst32 = readl_relaxed(src32);
> > > > +}
> > >
> > > NIH of memcpy_fromio() / memcpy_toio() ?
> > >
> > I am not sure if there will be no issue with < 4 bytes transactions over
> > PCI bus. I need to check it.
> 
> I didn't get it. You always do 4 byte chunks, so, supply aligned
> length to memcpy and you will have the same.
> 
> ...
Yes, I converted code to use these helpers.

> 
> > > > +static int prestera_fw_rev_check(struct prestera_fw *fw)
> > > > +{
> > > > +       struct prestera_fw_rev *rev = &fw->dev.fw_rev;
> > > > +       u16 maj_supp = PRESTERA_SUPP_FW_MAJ_VER;
> > > > +       u16 min_supp = PRESTERA_SUPP_FW_MIN_VER;
> > > > +
> > >
> > > > +       if (rev->maj == maj_supp && rev->min >= min_supp)
> > > > +               return 0;
> > >
> > > Why not traditional pattern
> > >
> > > if (err) {
> > >  ...
> > > }
> >
> > At least for me it looks simpler when to check which version is
> > correct.
> 
> OK.
> 
> > > ...
> > > return 0;
> > >
> > > ?
> > >
> > > > +       dev_err(fw->dev.dev, "Driver supports FW version only '%u.%u.x'",
> > > > +               PRESTERA_SUPP_FW_MAJ_VER, PRESTERA_SUPP_FW_MIN_VER);
> > > > +
> > > > +       return -EINVAL;
> > > > +}
> 
> ...
> 
> > Thanks Andy for the comments, especially for pcim_ helpers.
> 
> You are welcome!
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Thanks!