| Message ID | 20190416192730.15681-5-vidyas@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Tegra194 PCIe support | expand |
On Tue, Apr 16, 2019 at 20:27:18, Vidya Sagar <vidyas@nvidia.com> wrote: > Remove multiple write enable and disable sequences of dbi registers as > Tegra194 implements writes to BAR-0 register (offset: 0x10) controlled by > DBI write-lock enable bit thereby not allowing any further writes to BAR-0 > register in config space to take place. Hence disabling write permission > only towards the end. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > Changes since [v2]: > * None > > Changes since [v1]: > * None > > drivers/pci/controller/dwc/pcie-designware-host.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 2a5332e5ccfa..c0334c92c1a6 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -683,7 +683,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > val &= 0xffff00ff; > val |= 0x00000100; > dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val); > - dw_pcie_dbi_ro_wr_dis(pci); > > /* Setup bus numbers */ > val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS); > @@ -723,8 +722,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > > dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0); > > - /* Enable write permission for the DBI read-only register */ > - dw_pcie_dbi_ro_wr_en(pci); > /* Program correct class for RC */ > dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); > /* Better disable write permission right after the update */ > -- > 2.17.1 This setup sequence was written by Jingoo Han, let's check if he did this by some particular reason. Jingoo do you remember why you wrote the code like this? Regards, Gustavo
On Wed, 17 Apr 2019 09:56:33 +0000 Gustavo Pimentel wrote: > > On Tue, Apr 16, 2019 at 20:27:18, Vidya Sagar <vidyas@nvidia.com> wrote: > > > Remove multiple write enable and disable sequences of dbi registers as > > Tegra194 implements writes to BAR-0 register (offset: 0x10) controlled by > > DBI write-lock enable bit thereby not allowing any further writes to BAR-0 > > register in config space to take place. Hence disabling write permission > > only towards the end. > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > --- > > Changes since [v2]: > > * None > > > > Changes since [v1]: > > * None > > > > drivers/pci/controller/dwc/pcie-designware-host.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > > index 2a5332e5ccfa..c0334c92c1a6 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > @@ -683,7 +683,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > > val &= 0xffff00ff; > > val |= 0x00000100; > > dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val); > > - dw_pcie_dbi_ro_wr_dis(pci); > > > > /* Setup bus numbers */ > > val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS); > > @@ -723,8 +722,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > > > > dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0); > > > > - /* Enable write permission for the DBI read-only register */ > > - dw_pcie_dbi_ro_wr_en(pci); > > /* Program correct class for RC */ > > dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); > > /* Better disable write permission right after the update */ > > -- > > 2.17.1 > > This setup sequence was written by Jingoo Han, let's check if he did this > by some particular reason. > Jingoo do you remember why you wrote the code like this? FWICT, enabling RO writeable in the setup sequence is introduced in commit d91dfe5054d4 ("PCI: dwc: Enable write permission for Class Code, Interrupt Pin updates"). The Reason why not towards the end maybe only enable the RO writeable when necessary. thanks
On 4/22/2019 1:24 PM, Jisheng Zhang wrote: > On Wed, 17 Apr 2019 09:56:33 +0000 Gustavo Pimentel wrote: > >> >> On Tue, Apr 16, 2019 at 20:27:18, Vidya Sagar <vidyas@nvidia.com> wrote: >> >>> Remove multiple write enable and disable sequences of dbi registers as >>> Tegra194 implements writes to BAR-0 register (offset: 0x10) controlled by >>> DBI write-lock enable bit thereby not allowing any further writes to BAR-0 >>> register in config space to take place. Hence disabling write permission >>> only towards the end. >>> >>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >>> --- >>> Changes since [v2]: >>> * None >>> >>> Changes since [v1]: >>> * None >>> >>> drivers/pci/controller/dwc/pcie-designware-host.c | 3 --- >>> 1 file changed, 3 deletions(-) >>> >>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c >>> index 2a5332e5ccfa..c0334c92c1a6 100644 >>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >>> @@ -683,7 +683,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) >>> val &= 0xffff00ff; >>> val |= 0x00000100; >>> dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val); >>> - dw_pcie_dbi_ro_wr_dis(pci); >>> >>> /* Setup bus numbers */ >>> val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS); >>> @@ -723,8 +722,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) >>> >>> dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0); >>> >>> - /* Enable write permission for the DBI read-only register */ >>> - dw_pcie_dbi_ro_wr_en(pci); >>> /* Program correct class for RC */ >>> dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); >>> /* Better disable write permission right after the update */ >>> -- >>> 2.17.1 >> >> This setup sequence was written by Jingoo Han, let's check if he did this >> by some particular reason. >> Jingoo do you remember why you wrote the code like this? > > FWICT, enabling RO writeable in the setup sequence is introduced in > commit d91dfe5054d4 ("PCI: dwc: Enable write permission for Class Code, > Interrupt Pin updates"). The Reason why not towards the end maybe > only enable the RO writeable when necessary. In that case, my patch shouldn't cause any issue and if any it would avoid few writes to speed up the process. So, in my opinion, it is a good to go change. Please let me know if you have disagree with me on this. > > thanks >
On Mon, Apr 22, 2019 at 8:54:32, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > On Wed, 17 Apr 2019 09:56:33 +0000 Gustavo Pimentel wrote: > > > > > On Tue, Apr 16, 2019 at 20:27:18, Vidya Sagar <vidyas@nvidia.com> wrote: > > > > > Remove multiple write enable and disable sequences of dbi registers as > > > Tegra194 implements writes to BAR-0 register (offset: 0x10) controlled by > > > DBI write-lock enable bit thereby not allowing any further writes to BAR-0 > > > register in config space to take place. Hence disabling write permission > > > only towards the end. > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > --- > > > Changes since [v2]: > > > * None > > > > > > Changes since [v1]: > > > * None > > > > > > drivers/pci/controller/dwc/pcie-designware-host.c | 3 --- > > > 1 file changed, 3 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > > > index 2a5332e5ccfa..c0334c92c1a6 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > > @@ -683,7 +683,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > > > val &= 0xffff00ff; > > > val |= 0x00000100; > > > dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val); > > > - dw_pcie_dbi_ro_wr_dis(pci); > > > > > > /* Setup bus numbers */ > > > val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS); > > > @@ -723,8 +722,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > > > > > > dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0); > > > > > > - /* Enable write permission for the DBI read-only register */ > > > - dw_pcie_dbi_ro_wr_en(pci); > > > /* Program correct class for RC */ > > > dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); > > > /* Better disable write permission right after the update */ > > > -- > > > 2.17.1 > > > > This setup sequence was written by Jingoo Han, let's check if he did this > > by some particular reason. > > Jingoo do you remember why you wrote the code like this? > > FWICT, enabling RO writeable in the setup sequence is introduced in > commit d91dfe5054d4 ("PCI: dwc: Enable write permission for Class Code, > Interrupt Pin updates"). The Reason why not towards the end maybe > only enable the RO writeable when necessary. I also share that belief, in any case, I just want to confirm with the original developer if this was coded like this for some good reason, maybe to force some additional protection. Otherwise, I think the presented patch is harmless. Gustavo > > thanks
> -----Original Message----- > From: Jisheng Zhang [mailto:Jisheng.Zhang@synaptics.com] > Sent: 2019年4月22日 15:55 > To: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>; Z.q. Hou > <zhiqiang.hou@nxp.com> > Cc: Vidya Sagar <vidyas@nvidia.com>; bhelgaas@google.com; > robh+dt@kernel.org; mark.rutland@arm.com; thierry.reding@gmail.com; > jonathanh@nvidia.com; kishon@ti.com; catalin.marinas@arm.com; > will.deacon@arm.com; lorenzo.pieralisi@arm.com; jingoohan1@gmail.com; > devicetree@vger.kernel.org; mmaddireddy@nvidia.com; kthota@nvidia.com; > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > mperttunen@nvidia.com; linux-tegra@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; sagar.tv@gmail.com > Subject: [EXT] Re: [PATCH V3 04/16] PCI: dwc: Perform dbi regs write lock > towards the end > > WARNING: This email was created outside of NXP. DO NOT CLICK links or > attachments unless you recognize the sender and know the content is safe. > > > > On Wed, 17 Apr 2019 09:56:33 +0000 Gustavo Pimentel wrote: > > > > > On Tue, Apr 16, 2019 at 20:27:18, Vidya Sagar <vidyas@nvidia.com> wrote: > > > > > Remove multiple write enable and disable sequences of dbi registers > > > as > > > Tegra194 implements writes to BAR-0 register (offset: 0x10) > > > controlled by DBI write-lock enable bit thereby not allowing any > > > further writes to BAR-0 register in config space to take place. > > > Hence disabling write permission only towards the end. > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > --- > > > Changes since [v2]: > > > * None > > > > > > Changes since [v1]: > > > * None > > > > > > drivers/pci/controller/dwc/pcie-designware-host.c | 3 --- > > > 1 file changed, 3 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c > > > b/drivers/pci/controller/dwc/pcie-designware-host.c > > > index 2a5332e5ccfa..c0334c92c1a6 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > > > @@ -683,7 +683,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > > > val &= 0xffff00ff; > > > val |= 0x00000100; > > > dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val); > > > - dw_pcie_dbi_ro_wr_dis(pci); > > > > > > /* Setup bus numbers */ > > > val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS); @@ -723,8 > > > +722,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) > > > > > > dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0); > > > > > > - /* Enable write permission for the DBI read-only register */ > > > - dw_pcie_dbi_ro_wr_en(pci); > > > /* Program correct class for RC */ > > > dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, > PCI_CLASS_BRIDGE_PCI); > > > /* Better disable write permission right after the update */ > > > -- > > > 2.17.1 > > > > This setup sequence was written by Jingoo Han, let's check if he did > > this by some particular reason. > > Jingoo do you remember why you wrote the code like this? > > FWICT, enabling RO writeable in the setup sequence is introduced in commit > d91dfe5054d4 ("PCI: dwc: Enable write permission for Class Code, Interrupt > Pin updates"). The Reason why not towards the end maybe only enable the > RO writeable when necessary. > Yes, you get the point, I think it's not a good choice to unlock the write permission of the RO registers all through. Thanks, Zhiqiang
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 2a5332e5ccfa..c0334c92c1a6 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -683,7 +683,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) val &= 0xffff00ff; val |= 0x00000100; dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val); - dw_pcie_dbi_ro_wr_dis(pci); /* Setup bus numbers */ val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS); @@ -723,8 +722,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp) dw_pcie_wr_own_conf(pp, PCI_BASE_ADDRESS_0, 4, 0); - /* Enable write permission for the DBI read-only register */ - dw_pcie_dbi_ro_wr_en(pci); /* Program correct class for RC */ dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); /* Better disable write permission right after the update */
Remove multiple write enable and disable sequences of dbi registers as Tegra194 implements writes to BAR-0 register (offset: 0x10) controlled by DBI write-lock enable bit thereby not allowing any further writes to BAR-0 register in config space to take place. Hence disabling write permission only towards the end. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- Changes since [v2]: * None Changes since [v1]: * None drivers/pci/controller/dwc/pcie-designware-host.c | 3 --- 1 file changed, 3 deletions(-)