| Message ID | 1499322829-23018-1-git-send-email-Zhiqiang.Hou@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Zhiqiang, Às 7:33 AM de 7/6/2017, Zhiqiang Hou escreveu: > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > The read-only DBI registers can be written over the DBI when set > the "Write to RO Registers Using DBI" (DBI_RO_WR_EN) field of the > MISC_CONTROL_1_OFF register. I would suggest you to add a cover-letter next time to explain the global picture of the patch-set. I understand your need for this patch, but I don't agree on the approach. Sometimes the people in charge of the hardware design / configuration, forget to specify the device class and that can be problematic for some drivers, and so the typical workaround is to set it in the driver using a quirk for example. You can see some examples here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/quirks.c Thanks, Joao > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > --- > drivers/pci/dwc/pcie-designware.h | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h > index b4d2a89..bbdf35b 100644 > --- a/drivers/pci/dwc/pcie-designware.h > +++ b/drivers/pci/dwc/pcie-designware.h > @@ -76,6 +76,9 @@ > #define PCIE_ATU_FUNC(x) (((x) & 0x7) << 16) > #define PCIE_ATU_UPPER_TARGET 0x91C > > +#define PCIE_MISC_CONTROL_1_OFF 0x8BC > +#define PCIE_DBI_RO_WR_EN (0x1 << 0) > + > /* > * iATU Unroll-specific register definitions > * From 4.80 core version the address translation will be made by unroll > @@ -279,6 +282,28 @@ static inline u32 dw_pcie_readl_dbi2(struct dw_pcie *pci, u32 reg) > return __dw_pcie_read_dbi(pci, pci->dbi_base2, reg, 0x4); > } > > +static inline void dw_pcie_dbi_ro_wr_en(struct dw_pcie *pci) > +{ > + u32 reg; > + u32 val; > + > + reg = PCIE_MISC_CONTROL_1_OFF; > + val = dw_pcie_readl_dbi(pci, reg); > + val |= PCIE_DBI_RO_WR_EN; > + dw_pcie_writel_dbi(pci, reg, val); > +} > + > +static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci) > +{ > + u32 reg; > + u32 val; > + > + reg = PCIE_MISC_CONTROL_1_OFF; > + val = dw_pcie_readl_dbi(pci, reg); > + val &= ~PCIE_DBI_RO_WR_EN; > + dw_pcie_writel_dbi(pci, reg, val); > +} > + > #ifdef CONFIG_PCIE_DW_HOST > irqreturn_t dw_handle_msi_irq(struct pcie_port *pp); > void dw_pcie_msi_init(struct pcie_port *pp); >
Hi Joao, Thanks a lot for your comments! > -----Original Message----- > From: Joao Pinto [mailto:Joao.Pinto@synopsys.com] > Sent: 2017年7月6日 17:44 > To: Z.q. Hou <zhiqiang.hou@nxp.com>; linux-pci@vger.kernel.org; > Joao.Pinto@synopsys.com > Cc: bhelgaas@google.com; jingoohan1@gmail.com > Subject: Re: [PATCH 1/3] PCI: designware: add accessors for write permission > of DBI read-only registers > > > Hi Zhiqiang, > > Às 7:33 AM de 7/6/2017, Zhiqiang Hou escreveu: > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > The read-only DBI registers can be written over the DBI when set the > > "Write to RO Registers Using DBI" (DBI_RO_WR_EN) field of the > > MISC_CONTROL_1_OFF register. > > I would suggest you to add a cover-letter next time to explain the global > picture of the patch-set. Thanks, I will. > > I understand your need for this patch, but I don't agree on the approach. In the DWC common code, there is a function write a DBI read-only register 'Device class code', and the first 2 patches is to fix it. The 3rd patch is to refactor the Layerscape PCIe driver's host_init function and reuse the new added accessors. > Sometimes the people in charge of the hardware design / configuration, forget > to specify the device class and that can be problematic for some drivers, and > so the typical workaround is to set it in the driver using a quirk for example. > > You can see some examples here: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ > pci/quirks.c I don't know the PCI quirks, do you mean remove the pci Device Class fix code from the DWC common code and add it to quirks? Thanks, Zhiqiang
Hi Zhiqiang, Às 4:48 AM de 7/7/2017, Z.q. Hou escreveu: > Hi Joao, > > > > Thanks a lot for your comments! > > > >> -----Original Message----- > >> From: Joao Pinto [mailto:Joao.Pinto@synopsys.com] > >> Sent: 2017年7月6日 17:44 > >> To: Z.q. Hou <zhiqiang.hou@nxp.com>; linux-pci@vger.kernel.org; > >> Joao.Pinto@synopsys.com > >> Cc: bhelgaas@google.com; jingoohan1@gmail.com > >> Subject: Re: [PATCH 1/3] PCI: designware: add accessors for write permission > >> of DBI read-only registers > >> > >> > >> Hi Zhiqiang, > >> > >> Às 7:33 AM de 7/6/2017, Zhiqiang Hou escreveu: > >>> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > >>> > >>> The read-only DBI registers can be written over the DBI when set the > >>> "Write to RO Registers Using DBI" (DBI_RO_WR_EN) field of the > >>> MISC_CONTROL_1_OFF register. > >> > >> I would suggest you to add a cover-letter next time to explain the global > >> picture of the patch-set. > > > > Thanks, I will. > > > >> > >> I understand your need for this patch, but I don't agree on the approach. > > > > In the DWC common code, there is a function write a DBI read-only register 'Device class code', and the first 2 patches is to fix it. > > The 3rd patch is to refactor the Layerscape PCIe driver's host_init function and reuse the new added accessors. > > > >> Sometimes the people in charge of the hardware design / configuration, forget > >> to specify the device class and that can be problematic for some drivers, and > >> so the typical workaround is to set it in the driver using a quirk for example. > >> > >> You can see some examples here: > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=0DrBCvOc_J7LaMXBei1qCXxxfbLxWaVErKZ6Rkm6bUc&s=sOEmExQFqrCEmpAx9LjSeKRvkW1D-W82ckX5WGCgFWw&e= > >> pci/quirks.c > > > > I don't know the PCI quirks, do you mean remove the pci Device Class fix code from the DWC common code and add it to quirks? > In my opinion adding fixes to a common code is not a good approach. I would suggest the fix to go into the quirks file. @Bjorn: The quirks file is the best place for this type of fixes right? Thanks, Joao > > > Thanks, > > Zhiqiang >
Hi Bjorn, Due to all Freescale Layerscpe PCIe controllers have to fix the Class code, and this fixup existed in both Layerscape pcie driver and DWC common code, so I want to reuse the fixup in DWC common code. Can you give me any suggestion? > -----Original Message----- > From: Joao Pinto [mailto:Joao.Pinto@synopsys.com] > Sent: 2017年7月7日 16:53 > To: Z.q. Hou <zhiqiang.hou@nxp.com>; Joao Pinto > <Joao.Pinto@synopsys.com>; linux-pci@vger.kernel.org; > bhelgaas@google.com > Cc: jingoohan1@gmail.com > Subject: Re: [PATCH 1/3] PCI: designware: add accessors for write permission > of DBI read-only registers > > > Hi Zhiqiang, > > Às 4:48 AM de 7/7/2017, Z.q. Hou escreveu: > > Hi Joao, > > > > > > > > Thanks a lot for your comments! > > > > > > > >> -----Original Message----- > > > >> From: Joao Pinto [mailto:Joao.Pinto@synopsys.com] > > > >> Sent: 2017年7月6日 17:44 > > > >> To: Z.q. Hou <zhiqiang.hou@nxp.com>; linux-pci@vger.kernel.org; > > > >> Joao.Pinto@synopsys.com > > > >> Cc: bhelgaas@google.com; jingoohan1@gmail.com > > > >> Subject: Re: [PATCH 1/3] PCI: designware: add accessors for write > >> permission > > > >> of DBI read-only registers > > > >> > > > >> > > > >> Hi Zhiqiang, > > > >> > > > >> Às 7:33 AM de 7/6/2017, Zhiqiang Hou escreveu: > > > >>> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > >>> > > > >>> The read-only DBI registers can be written over the DBI when set the > > > >>> "Write to RO Registers Using DBI" (DBI_RO_WR_EN) field of the > > > >>> MISC_CONTROL_1_OFF register. > > > >> > > > >> I would suggest you to add a cover-letter next time to explain the > >> global > > > >> picture of the patch-set. > > > > > > > > Thanks, I will. > > > > > > > >> > > > >> I understand your need for this patch, but I don't agree on the approach. > > > > > > > > In the DWC common code, there is a function write a DBI read-only register > 'Device class code', and the first 2 patches is to fix it. > > > > The 3rd patch is to refactor the Layerscape PCIe driver's host_init function > and reuse the new added accessors. > > > > > > > >> Sometimes the people in charge of the hardware design / > >> configuration, forget > > > >> to specify the device class and that can be problematic for some > >> drivers, and > > > >> so the typical workaround is to set it in the driver using a quirk for example. > > > >> > > > >> You can see some examples here: > > > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_p > >> ub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_&d=DwIGaQ&c=D > >> > PL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io > _kx0&m > >> > =0DrBCvOc_J7LaMXBei1qCXxxfbLxWaVErKZ6Rkm6bUc&s=sOEmExQFqrCEmpA > x9LjSeK > >> RvkW1D-W82ckX5WGCgFWw&e= > > > >> pci/quirks.c > > > > > > > > I don't know the PCI quirks, do you mean remove the pci Device Class fix > code from the DWC common code and add it to quirks? > > > > In my opinion adding fixes to a common code is not a good approach. I would > suggest the fix to go into the quirks file. > > @Bjorn: The quirks file is the best place for this type of fixes right? Thanks, Zhiqiang
On Tuesday, July 11, 2017 12:11 AM, Z.q. Hou wrote: > > Hi Bjorn, > > Due to all Freescale Layerscpe PCIe controllers have to fix the Class code, > and this fixup existed in both Layerscape pcie driver and DWC common code, > so I want to reuse the fixup in DWC common code. > Can you give me any suggestion? Please don't add your comment on top of the email. > > > -----Original Message----- > > From: Joao Pinto [mailto:Joao.Pinto@synopsys.com] > > Sent: 2017年7月7日 16:53 > > To: Z.q. Hou <zhiqiang.hou@nxp.com>; Joao Pinto > > <Joao.Pinto@synopsys.com>; linux-pci@vger.kernel.org; > > bhelgaas@google.com > > Cc: jingoohan1@gmail.com > > Subject: Re: [PATCH 1/3] PCI: designware: add accessors for write > permission > > of DBI read-only registers > > > > > > Hi Zhiqiang, > > > > Às 4:48 AM de 7/7/2017, Z.q. Hou escreveu: > > > Hi Joao, > > > > > > > > > > > > Thanks a lot for your comments! > > > > > > > > > > > >> -----Original Message----- > > > > > >> From: Joao Pinto [mailto:Joao.Pinto@synopsys.com] > > > > > >> Sent: 2017年7月6日 17:44 > > > > > >> To: Z.q. Hou <zhiqiang.hou@nxp.com>; linux-pci@vger.kernel.org; > > > > > >> Joao.Pinto@synopsys.com > > > > > >> Cc: bhelgaas@google.com; jingoohan1@gmail.com > > > > > >> Subject: Re: [PATCH 1/3] PCI: designware: add accessors for write > > >> permission > > > > > >> of DBI read-only registers > > > > > >> > > > > > >> > > > > > >> Hi Zhiqiang, > > > > > >> > > > > > >> Às 7:33 AM de 7/6/2017, Zhiqiang Hou escreveu: > > > > > >>> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > > >>> > > > > > >>> The read-only DBI registers can be written over the DBI when set the > > > > > >>> "Write to RO Registers Using DBI" (DBI_RO_WR_EN) field of the > > > > > >>> MISC_CONTROL_1_OFF register. > > > > > >> > > > > > >> I would suggest you to add a cover-letter next time to explain the > > >> global > > > > > >> picture of the patch-set. > > > > > > > > > > > > Thanks, I will. > > > > > > > > > > > >> > > > > > >> I understand your need for this patch, but I don't agree on the > approach. > > > > > > > > > > > > In the DWC common code, there is a function write a DBI read-only > register > > 'Device class code', and the first 2 patches is to fix it. > > > > > > The 3rd patch is to refactor the Layerscape PCIe driver's host_init > function > > and reuse the new added accessors. > > > > > > > > > > > >> Sometimes the people in charge of the hardware design / > > >> configuration, forget > > > > > >> to specify the device class and that can be problematic for some > > >> drivers, and > > > > > >> so the typical workaround is to set it in the driver using a quirk > for example. > > > > > >> > > > > > >> You can see some examples here: > > > > > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_p > > >> ub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_&d=DwIGaQ&c=D > > >> > > PL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io > > _kx0&m > > >> > > =0DrBCvOc_J7LaMXBei1qCXxxfbLxWaVErKZ6Rkm6bUc&s=sOEmExQFqrCEmpA > > x9LjSeK > > >> RvkW1D-W82ckX5WGCgFWw&e= > > > > > >> pci/quirks.c > > > > > > > > > > > > I don't know the PCI quirks, do you mean remove the pci Device Class > fix > > code from the DWC common code and add it to quirks? > > > > > > > In my opinion adding fixes to a common code is not a good approach. I > would > > suggest the fix to go into the quirks file. Your answer should be added to here. Then, it will help other people follow this email thread. Best regards, Jingoo Han > > > > @Bjorn: The quirks file is the best place for this type of fixes right? > > Thanks, > Zhiqiang >
Hi Jingoo Han, Thanks for your comments! > -----Original Message----- > From: Jingoo Han [mailto:jingoohan1@gmail.com] > Sent: 2017年7月18日 3:41 > To: Z.q. Hou <zhiqiang.hou@nxp.com>; 'Joao Pinto' > <Joao.Pinto@synopsys.com>; linux-pci@vger.kernel.org; > bhelgaas@google.com > Subject: Re: [PATCH 1/3] PCI: designware: add accessors for write permission > of DBI read-only registers > > On Tuesday, July 11, 2017 12:11 AM, Z.q. Hou wrote: > > > > Hi Bjorn, > > > > Due to all Freescale Layerscpe PCIe controllers have to fix the Class > > code, and this fixup existed in both Layerscape pcie driver and DWC > > common code, so I want to reuse the fixup in DWC common code. > > Can you give me any suggestion? > > Please don't add your comment on top of the email. Ok, got it. > > > > > > > -----Original Message----- > > > From: Joao Pinto [mailto:Joao.Pinto@synopsys.com] > > > Sent: 2017年7月7日 16:53 > > > To: Z.q. Hou <zhiqiang.hou@nxp.com>; Joao Pinto > > > <Joao.Pinto@synopsys.com>; linux-pci@vger.kernel.org; > > > bhelgaas@google.com > > > Cc: jingoohan1@gmail.com > > > Subject: Re: [PATCH 1/3] PCI: designware: add accessors for write > > permission > > > of DBI read-only registers > > > > > > > > > Hi Zhiqiang, > > > > > > Às 4:48 AM de 7/7/2017, Z.q. Hou escreveu: > > > > Hi Joao, > > > > > > > > > > > > > > > > Thanks a lot for your comments! > > > > > > > > > > > > > > > >> -----Original Message----- > > > > > > > >> From: Joao Pinto [mailto:Joao.Pinto@synopsys.com] > > > > > > > >> Sent: 2017年7月6日 17:44 > > > > > > > >> To: Z.q. Hou <zhiqiang.hou@nxp.com>; linux-pci@vger.kernel.org; > > > > > > > >> Joao.Pinto@synopsys.com > > > > > > > >> Cc: bhelgaas@google.com; jingoohan1@gmail.com > > > > > > > >> Subject: Re: [PATCH 1/3] PCI: designware: add accessors for write > > > >> permission > > > > > > > >> of DBI read-only registers > > > > > > > >> > > > > > > > >> > > > > > > > >> Hi Zhiqiang, > > > > > > > >> > > > > > > > >> Às 7:33 AM de 7/6/2017, Zhiqiang Hou escreveu: > > > > > > > >>> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > > > > >>> > > > > > > > >>> The read-only DBI registers can be written over the DBI when set > > > >>> the > > > > > > > >>> "Write to RO Registers Using DBI" (DBI_RO_WR_EN) field of the > > > > > > > >>> MISC_CONTROL_1_OFF register. > > > > > > > >> > > > > > > > >> I would suggest you to add a cover-letter next time to explain > > > >> the global > > > > > > > >> picture of the patch-set. > > > > > > > > > > > > > > > > Thanks, I will. > > > > > > > > > > > > > > > >> > > > > > > > >> I understand your need for this patch, but I don't agree on the > > approach. > > > > > > > > > > > > > > > > In the DWC common code, there is a function write a DBI read-only > > register > > > 'Device class code', and the first 2 patches is to fix it. > > > > > > > > The 3rd patch is to refactor the Layerscape PCIe driver's > > > > host_init > > function > > > and reuse the new added accessors. > > > > > > > > > > > > > > > >> Sometimes the people in charge of the hardware design / > > > >> configuration, forget > > > > > > > >> to specify the device class and that can be problematic for some > > > >> drivers, and > > > > > > > >> so the typical workaround is to set it in the driver using a > > > >> quirk > > for example. > > > > > > > >> > > > > > > > >> You can see some examples here: > > > > > > > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.o > > > >> rg_p > > > >> ub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_&d=DwIGaQ > > > >> &c=D > > > >> > > > > PL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io > > > _kx0&m > > > >> > > > > =0DrBCvOc_J7LaMXBei1qCXxxfbLxWaVErKZ6Rkm6bUc&s=sOEmExQFqrCEmpA > > > x9LjSeK > > > >> RvkW1D-W82ckX5WGCgFWw&e= > > > > > > > >> pci/quirks.c > > > > > > > > > > > > > > > > I don't know the PCI quirks, do you mean remove the pci Device > > > > Class > > fix > > > code from the DWC common code and add it to quirks? > > > > > > > > > > In my opinion adding fixes to a common code is not a good approach. > > > I > > would > > > suggest the fix to go into the quirks file. > > Your answer should be added to here. > Then, it will help other people follow this email thread. > Yes, and do you know why write to the DBI read-only register(class code) without enable the write permission of DBI read-only registers? Can you give some suggestions? Thanks, Zhiqiang
On Thu, Jul 06, 2017 at 10:44:04AM +0100, Joao Pinto wrote: > > Hi Zhiqiang, > > Às 7:33 AM de 7/6/2017, Zhiqiang Hou escreveu: > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > The read-only DBI registers can be written over the DBI when set > > the "Write to RO Registers Using DBI" (DBI_RO_WR_EN) field of the > > MISC_CONTROL_1_OFF register. > > I would suggest you to add a cover-letter next time to explain the global > picture of the patch-set. > > I understand your need for this patch, but I don't agree on the approach. > Sometimes the people in charge of the hardware design / configuration, forget to > specify the device class and that can be problematic for some drivers, and so > the typical workaround is to set it in the driver using a quirk for example. > > You can see some examples here: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/quirks.c I assume you're suggesting something similar to quirk_tw686x_class() and fixup_ti816x_class(), quirk_amd_nl_class(), quirk_eisa_bridge(), fixup_rev1_53c810(), etc. The current dw_pcie_setup_rc() contains this: /* program correct class for RC */ dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); That suggests that DesignWare-based devices have the wrong class code, and we're trying to fix it. Patch 2/3 of this series suggests that the existing fix doesn't actually work because the register is read-only. If it's necessary and possible to update the class code register in dw_pcie_setup_rc(), I think that's a reasonable spot to do it. The quirks in drivers/pci/quirks.c are necessary because per spec, the PCI Class Code is read-only, so in general we can't update it. In the DesignWare case, the driver has additional device-specific knowledge that allows it to update Class Code value, and I think it makes sense to do it there. > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > --- > > drivers/pci/dwc/pcie-designware.h | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h > > index b4d2a89..bbdf35b 100644 > > --- a/drivers/pci/dwc/pcie-designware.h > > +++ b/drivers/pci/dwc/pcie-designware.h > > @@ -76,6 +76,9 @@ > > #define PCIE_ATU_FUNC(x) (((x) & 0x7) << 16) > > #define PCIE_ATU_UPPER_TARGET 0x91C > > > > +#define PCIE_MISC_CONTROL_1_OFF 0x8BC > > +#define PCIE_DBI_RO_WR_EN (0x1 << 0) > > + > > /* > > * iATU Unroll-specific register definitions > > * From 4.80 core version the address translation will be made by unroll > > @@ -279,6 +282,28 @@ static inline u32 dw_pcie_readl_dbi2(struct dw_pcie *pci, u32 reg) > > return __dw_pcie_read_dbi(pci, pci->dbi_base2, reg, 0x4); > > } > > > > +static inline void dw_pcie_dbi_ro_wr_en(struct dw_pcie *pci) > > +{ > > + u32 reg; > > + u32 val; > > + > > + reg = PCIE_MISC_CONTROL_1_OFF; > > + val = dw_pcie_readl_dbi(pci, reg); > > + val |= PCIE_DBI_RO_WR_EN; > > + dw_pcie_writel_dbi(pci, reg, val); > > +} > > + > > +static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci) > > +{ > > + u32 reg; > > + u32 val; > > + > > + reg = PCIE_MISC_CONTROL_1_OFF; > > + val = dw_pcie_readl_dbi(pci, reg); > > + val &= ~PCIE_DBI_RO_WR_EN; > > + dw_pcie_writel_dbi(pci, reg, val); > > +} > > + > > #ifdef CONFIG_PCIE_DW_HOST > > irqreturn_t dw_handle_msi_irq(struct pcie_port *pp); > > void dw_pcie_msi_init(struct pcie_port *pp); > > >
Hi Bjorn, Thanks a lot for your comments! > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: 2017年8月3日 5:25 > To: Joao Pinto <Joao.Pinto@synopsys.com> > Cc: Z.q. Hou <zhiqiang.hou@nxp.com>; linux-pci@vger.kernel.org; > bhelgaas@google.com; jingoohan1@gmail.com > Subject: Re: [PATCH 1/3] PCI: designware: add accessors for write permission > of DBI read-only registers > > On Thu, Jul 06, 2017 at 10:44:04AM +0100, Joao Pinto wrote: > > > > Hi Zhiqiang, > > > > Às 7:33 AM de 7/6/2017, Zhiqiang Hou escreveu: > > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > > > The read-only DBI registers can be written over the DBI when set the > > > "Write to RO Registers Using DBI" (DBI_RO_WR_EN) field of the > > > MISC_CONTROL_1_OFF register. > > > > I would suggest you to add a cover-letter next time to explain the > > global picture of the patch-set. > > > > I understand your need for this patch, but I don't agree on the approach. > > Sometimes the people in charge of the hardware design / configuration, > > forget to specify the device class and that can be problematic for > > some drivers, and so the typical workaround is to set it in the driver using a > quirk for example. > > > > You can see some examples here: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre > > e/drivers/pci/quirks.c > > I assume you're suggesting something similar to quirk_tw686x_class() and > fixup_ti816x_class(), quirk_amd_nl_class(), quirk_eisa_bridge(), > fixup_rev1_53c810(), etc. > > The current dw_pcie_setup_rc() contains this: > > /* program correct class for RC */ > dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); > > That suggests that DesignWare-based devices have the wrong class code, and > we're trying to fix it. > > Patch 2/3 of this series suggests that the existing fix doesn't actually work > because the register is read-only. > > If it's necessary and possible to update the class code register in > dw_pcie_setup_rc(), I think that's a reasonable spot to do it. > > The quirks in drivers/pci/quirks.c are necessary because per spec, the PCI Class > Code is read-only, so in general we can't update it. > > In the DesignWare case, the driver has additional device-specific knowledge > that allows it to update Class Code value, and I think it makes sense to do it > there. Yes, will keep the Class Code fixup function in the DesignWare driver. > > > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > --- > > > drivers/pci/dwc/pcie-designware.h | 25 +++++++++++++++++++++++++ > > > 1 file changed, 25 insertions(+) > > > > > > diff --git a/drivers/pci/dwc/pcie-designware.h > > > b/drivers/pci/dwc/pcie-designware.h > > > index b4d2a89..bbdf35b 100644 > > > --- a/drivers/pci/dwc/pcie-designware.h > > > +++ b/drivers/pci/dwc/pcie-designware.h > > > @@ -76,6 +76,9 @@ > > > #define PCIE_ATU_FUNC(x) (((x) & 0x7) << 16) > > > #define PCIE_ATU_UPPER_TARGET 0x91C > > > > > > +#define PCIE_MISC_CONTROL_1_OFF 0x8BC > > > +#define PCIE_DBI_RO_WR_EN (0x1 << 0) > > > + > > > /* > > > * iATU Unroll-specific register definitions > > > * From 4.80 core version the address translation will be made by > > > unroll @@ -279,6 +282,28 @@ static inline u32 dw_pcie_readl_dbi2(struct > dw_pcie *pci, u32 reg) > > > return __dw_pcie_read_dbi(pci, pci->dbi_base2, reg, 0x4); } > > > > > > +static inline void dw_pcie_dbi_ro_wr_en(struct dw_pcie *pci) { > > > + u32 reg; > > > + u32 val; > > > + > > > + reg = PCIE_MISC_CONTROL_1_OFF; > > > + val = dw_pcie_readl_dbi(pci, reg); > > > + val |= PCIE_DBI_RO_WR_EN; > > > + dw_pcie_writel_dbi(pci, reg, val); } > > > + > > > +static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci) { > > > + u32 reg; > > > + u32 val; > > > + > > > + reg = PCIE_MISC_CONTROL_1_OFF; > > > + val = dw_pcie_readl_dbi(pci, reg); > > > + val &= ~PCIE_DBI_RO_WR_EN; > > > + dw_pcie_writel_dbi(pci, reg, val); } > > > + > > > #ifdef CONFIG_PCIE_DW_HOST > > > irqreturn_t dw_handle_msi_irq(struct pcie_port *pp); void > > > dw_pcie_msi_init(struct pcie_port *pp); > > > > > Thanks, Zhiqiang
diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h index b4d2a89..bbdf35b 100644 --- a/drivers/pci/dwc/pcie-designware.h +++ b/drivers/pci/dwc/pcie-designware.h @@ -76,6 +76,9 @@ #define PCIE_ATU_FUNC(x) (((x) & 0x7) << 16) #define PCIE_ATU_UPPER_TARGET 0x91C +#define PCIE_MISC_CONTROL_1_OFF 0x8BC +#define PCIE_DBI_RO_WR_EN (0x1 << 0) + /* * iATU Unroll-specific register definitions * From 4.80 core version the address translation will be made by unroll @@ -279,6 +282,28 @@ static inline u32 dw_pcie_readl_dbi2(struct dw_pcie *pci, u32 reg) return __dw_pcie_read_dbi(pci, pci->dbi_base2, reg, 0x4); } +static inline void dw_pcie_dbi_ro_wr_en(struct dw_pcie *pci) +{ + u32 reg; + u32 val; + + reg = PCIE_MISC_CONTROL_1_OFF; + val = dw_pcie_readl_dbi(pci, reg); + val |= PCIE_DBI_RO_WR_EN; + dw_pcie_writel_dbi(pci, reg, val); +} + +static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci) +{ + u32 reg; + u32 val; + + reg = PCIE_MISC_CONTROL_1_OFF; + val = dw_pcie_readl_dbi(pci, reg); + val &= ~PCIE_DBI_RO_WR_EN; + dw_pcie_writel_dbi(pci, reg, val); +} + #ifdef CONFIG_PCIE_DW_HOST irqreturn_t dw_handle_msi_irq(struct pcie_port *pp); void dw_pcie_msi_init(struct pcie_port *pp);