On 1/18/2021 9:35 AM, Nalla Pradeep wrote:
Functions to setup device, basic IQ and OQ registers are added.


As the patch title can you please start with lowercase after module, and do not use the final '.', if you run './devtools/check-git-log.sh' script it will highlight these kind of issues, can you please be sure all are addressed in next version.

Also it would helpful to clarify all abbreviations used in the commit log, like IQ and OQ used above, it is helpul if you can write the long version for the fist time they are used like, Output Queue (OQ).

Signed-off-by: Nalla Pradeep <pna...@marvell.com>

<...>

+static void
+otx2_vf_setup_global_oq_reg(struct otx_ep_device *otx_ep, int q_no)
+{
+       volatile uint64_t reg_val = 0ull;
+
+       reg_val = otx2_read64(otx_ep->hw_addr + SDP_VF_R_OUT_CONTROL(q_no));
+
+#if defined(BUFPTR_ONLY_MODE)
+       reg_val &= ~(SDP_VF_R_OUT_CTL_IMODE);
+#else
+       reg_val |= (SDP_VF_R_OUT_CTL_IMODE);
+#endif

Where this macro, BUFPTR_ONLY_MODE, is defined, using macros like this can lead to the dead code, can you please either remove this or rename with RTE_ prefix and document it?

<...>

+int
+otx2_ep_vf_setup_device(struct otx_ep_device *otx_ep)
+{
+       uint64_t reg_val = 0ull;
+
+       /* If application doesn't provide its conf, use driver default conf */
+       if (otx_ep->conf == NULL) {
+               otx_ep->conf = otx2_ep_get_defconf(otx_ep);
+               if (otx_ep->conf == NULL) {
+                       otx2_err("SDP VF default config not found");
+                       return -ENOMEM;


Similar comment as previous, please prefer ENOMEM error type when there is a problem with memory.
There are multiple occurance of returning ENOMEM.

<...>

@@ -21,10 +22,12 @@ otx_ep_chip_specific_setup(struct otx_ep_device *otx_epvf)
        switch (dev_id) {
        case PCI_DEVID_OCTEONTX_EP_VF:
                otx_epvf->chip_id = dev_id;
+               ret = otx_ep_vf_setup_device(otx_epvf);
                break;
        case PCI_DEVID_OCTEONTX2_EP_NET_VF:
        case PCI_DEVID_CN98XX_EP_NET_VF:
                otx_epvf->chip_id = dev_id;
+               ret = otx2_ep_vf_setup_device(otx_epvf);
                break;
        default:
                otx_ep_err("Unsupported device\n");
@@ -46,6 +49,13 @@ otx_epdev_init(struct otx_ep_device *otx_epvf)
                goto setup_fail;
        }
+ if (otx_epvf->fn_list.setup_device_regs(otx_epvf)) {
+               otx_ep_err("Failed to configure device registers\n");
+               goto setup_fail;
+       }

So both otx & otx2 devices are supported via function pointers, out of curiosity how different the implementations are, since both are implementing same spec as far as I understand.

Reply via email to