Messages in this thread |  | | From | Andy Shevchenko <> | Date | Mon, 27 Jul 2020 11:04:56 +0300 | Subject | Re: [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
|  |