Date:   Tue, 30 Nov 2021 13:34:56 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>
Cc:     lorenzo.pieralisi@arm.com, bhelgaas@google.com,
        svarbanov@mm-sol.com, bjorn.andersson@linaro.org, robh@kernel.org,
        linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v2] PCI: qcom: Use __be16 for catching cpu_to_be16()
 return instead of u16
Message-ID: <20211130080456.GE205712@thinkpad>
References: <20211130064215.207393-1-manivannan.sadhasivam@linaro.org>
 <YaXSoLpIfrTh81/+@rocinante>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <YaXSoLpIfrTh81/+@rocinante>
Precedence: bulk

Hey,

On Tue, Nov 30, 2021 at 08:28:32AM +0100, Krzysztof Wilczyński wrote:
> Hi Manivannan,
> 
> Thank you for sending the patch over!  Much appreciated!
> 
> A small nitpick, thus feel free to ignore it, of course: if I may, I would
> suggest the following subject:
> 
>   PCI: qcom: Use __be16 type to store return value from cpu_to_be16()
>

Will do
 
> Or something along the lines.
> 
> > cpu_to_be16() returns __be16 value but the driver uses u16 and that's
> > incorrect. Fix it by using __be16 as the datatype of bdf_be variable.
> 
> It would be "data type" in the above.
> 
> Not really a requirement to do so, but you could include the actual
> warning, as sometimes this is useful for reference later, as per:
> 
>   drivers/pci/controller/dwc/pcie-qcom.c:1346:30: warning: incorrect type in initializer (different base types)
>   drivers/pci/controller/dwc/pcie-qcom.c:1346:30:    expected unsigned short [usertype] bdf_be
>   drivers/pci/controller/dwc/pcie-qcom.c:1346:30:    got restricted __be16 [usertype]
> 

I usually do but as per Bjorn's comment I thought it is not recommended for PCI
subsystem (or maybe I misread his comments). Will add.

> > @@ -1343,7 +1343,7 @@ static int qcom_pcie_config_sid_sm8250(struct qcom_pcie *pcie)
> >  
> >  	/* Look for an available entry to hold the mapping */
> >  	for (i = 0; i < nr_map; i++) {
> > -		u16 bdf_be = cpu_to_be16(map[i].bdf);
> > +		__be16 bdf_be = cpu_to_be16(map[i].bdf);
> >  		u32 val;
> >  		u8 hash;
> 
> Thank you!
> 
> Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
> 
> Also, since I have your attention, it seems we have a number of unused
> macros in the qcom driver, as per:
> 
>   drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_BDF_TRANSLATE_CFG            0x24C
>   drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_SID_OFFSET                   0x234
>   drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_SLV_ADDR_SPACE_SIZE          0x16C
> 
> And also in the qcom-ep driver, as per:
> 

These defines are helpful for someone who wants to add some functionality or
even bug fix in the future. Since these controllers are not publically
documented, having these definitions helps a lot.

Thanks,
Mani

>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_BRIDGE_FLUSH_N           BIT(12)
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_MHI_A7                   BIT(7)
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_MHI_Q6                   BIT(6)
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SLV_ADDR_SPACE_SIZE              0x358
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_PLS_PME                  BIT(17)
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SLV_ADDR_MSB_CTRL                        0x2c0
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_DEBUG                    BIT(4)
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_L1SUB_TIMEOUT            BIT(9)
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_DBI_BASE_ADDR_HI                 0x354
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_ATU_BASE_ADDR                    0x634
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SLV_ADDR_SPACE_SIZE_HI           0x35c
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_LTR                      BIT(5)
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_DBI_BASE_ADDR                    0x350
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_SRIS_MODE                                0x644
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_MMIO_WRITE                       BIT(10)
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_PLS_ERR                  BIT(15)
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_AER_LEGACY                       BIT(14)
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_CFG_WRITE                        BIT(11)
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_ATU_BASE_ADDR_HI                 0x638
>   drivers/pci/controller/dwc/pcie-qcom-ep.c:#define PARF_INT_ALL_PME_LEGACY                       BIT(16)
> 
> Are these needed, or would it be fine to drop these?
> 
> 	Krzysztof