> > > +obj-$(CONFIG_AMD_VERSAL_MGMT) += amd-vmgmt.o
> >
> > IMHO the naming vmgmt is hard to understand, any better idea?
>
> The "v" stand for Versal. We would change to amd-vpci for Versal based pcie
"v" + "pci" is quite a misleading term, maybe just versal-pci?
> devices.
>
>
[...]
> >
> > > +{
> > > + struct comms_device *ccdev;
> > > +
> > > + ccdev = devm_kzalloc(&vdev->pdev->dev, sizeof(*ccdev), GFP_KERNEL);
> > > + if (!ccdev)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + ccdev->vdev = vdev;
> > > +
> > > + ccdev->regmap = devm_regmap_init_mmio(&vdev->pdev->dev,
> > > + vdev->tbl + COMMS_PCI_BAR_OFF,
> > > + &comms_regmap_config);
> >
> > I'm not sure why a regmap is needed. All register accessing is within
> > the same module/file, and I assume a base+offset is enough to position
> > the register addr.
>
> I thought the regmap is preferred. We can use some common APIs like
> regmap_bulk_*. The base+offset works too, then we will implement our own
> bulk_* functions. Please let me know.
I didn't see any regmap_bulk_*. AFAICS regmap is not needed here.
[...]
> > > + /* create fgpa bridge, region for the base shell */
> > > + fdev->bridge = fpga_bridge_register(dev, "AMD Versal FPGA Bridge",
> > > + &vmgmt_br_ops, fdev);
> >
> > I didn't find the br_ops anywhere in this patchset. So how to gate the
> > FPGA region when it is being reprogrammed? What is the physical link
> > between the FPGA region and outside visitors?
>
> The FPGA region gate operation is done in the FW running in this PCIe card.
> The FW will "freeze" the gate before programing the PL. After downloading
> the new hardware. The FW will then "free" the gate.
So no OS operation is needed, then seems no need the fpga_bridge object.
>
> No physical link between FPGA region and outside visitors, the FW handles
> all requests.
>
> >
> > > + if (IS_ERR(fdev->bridge)) {
> > > + vmgmt_err(vdev, "Failed to register FPGA bridge, err %ld",
> > > + PTR_ERR(fdev->bridge));
> > > + ret = PTR_ERR(fdev->bridge);
> > > + goto unregister_fpga_mgr;
> > > + }
> > > +
> > > + region = (struct fpga_region_info) {
> > > + .compat_id = (struct fpga_compat_id *)&vdev->intf_uuid,
> > > + .get_bridges = vmgmt_get_bridges,
> > > + .mgr = fdev->mgr,
> > > + .priv = fdev,
> > > + };
> > > +
> > > + fdev->region = fpga_region_register_full(dev, ®ion);
> >
> > I assume the fpga region represents the user PF, is it? If you
> > reprogram the FPGA region, how does the user PF driver aware the HW is
> > changing?
>
> The HW changing request is always requested from the user PF driver. The
I don't understand. In your patch the FPGA reprograming is triggered by
an IOCTL, usually a userspace application calls it. But here says it is
triggered by the user PF *driver*, which IIUC is a kernel driver.
Anything I missed?
> user PF driver will make sure it is safe to change hardware. Then, the user
> PF driver notify the mgmt PF driver by a unique identify of the HW bitstream
> (PL Data).
>
> The mgmt PF driver, the amd-vpci driver, will check the unique identify and
> then find the same PL Data from its local storage which is previously
> installed, and start downloading it.
Is the flow included in this patchset? Please elaborate more.
[...]
> > > +/**
> > > + * VERSAL_MGMT_LOAD_XCLBIN_IOCTL - Download XCLBIN to the device
> > > + *
> > > + * This IOCTL is used to download XCLBIN down to the device.
> > > + * Return: 0 on success, -errno on failure.
> > > + */
> > > +#define VERSAL_MGMT_LOAD_XCLBIN_IOCTL _IOW(VERSAL_MGMT_MAGIC, \
> > > + VERSAL_MGMT_BASE + 0, void *)
> >
> > Many definitions are added in a batch but some are not used in this
> > patch. Please reorganize the patches for easer review, even for first
> > version.
> >
> > Thanks,
> > Yilun
>
> Hi Yilun,
>
> Thanks for taking your time, and yes for sure I will make each patch more
> self-contained.
>
> Here is my thoughts on upcoming patches structure:
> 1st patch, adding driver probe and FPGA framework; the actual ops
Just adding driver probe for 1st patch please.
Thanks,
Yilun
> of handling communication channel message and remote queue message
> will present as no-op with comments.
>
> 2nd patch, adding the communication channel services
> 3rd patch, adding the remote queue services
> 4th patch, adding the callers of using the remote queue services
>
> Thanks,
> David
>
> >
> > > +
> > > +#endif /* _UAPI_LINUX_VMGMT_H */
> > > --
> > > 2.34.1
> > >
> > >
