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.koc...@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.koc...@plvision.eu>
> > > 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!
