lkml.org 
[lkml]   [2020]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [net-next v3 2/6] net: marvell: prestera: Add PCI interface support
On Mon, Jul 27, 2020 at 1:55 AM Vadym Kochan <vadym.kochan@plvision.eu> 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@plvision.eu> wrote:

...

For the non-commented I assume you are agree with. Correct?

...

> > > +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.

...

> > > +#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"

...

> > > +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.

...

> > > +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

\
 
 \ /
  Last update: 2020-07-27 10:06    [W:0.112 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site