[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20211130080456.GE205712@thinkpad>
Date: Tue, 30 Nov 2021 13:34:56 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Krzysztof Wilczyński <kw@...ux.com>
Cc: lorenzo.pieralisi@....com, bhelgaas@...gle.com,
svarbanov@...sol.com, bjorn.andersson@...aro.org, robh@...nel.org,
linux-pci@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2] PCI: qcom: Use __be16 for catching cpu_to_be16()
return instead of u16
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@...ux.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
Powered by blists - more mailing lists