From: "Pali Rohár" <pali@kernel.org> To: "Krzysztof Wilczyński" <kw@linux.com> Cc: "Bjorn Helgaas" <bhelgaas@google.com>, "Kalle Valo" <kvalo@codeaurora.org>, "Toke Høiland-Jørgensen" <toke@redhat.com>, "Marek Behún" <kabel@kernel.org>, vtolkm@gmail.com, "Rob Herring" <robh@kernel.org>, "Ilias Apalodimas" <ilias.apalodimas@linaro.org>, "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>, "Jason Cooper" <jason@lakedaemon.net>, linux-pci@vger.kernel.org, ath10k@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] PCI: Disallow retraining link for Atheros QCA98xx chips on non-Gen1 PCIe bridges Date: Sat, 27 Mar 2021 14:29:59 +0100 [thread overview] Message-ID: <20210327132959.fwkphna7gg57aove@pali> (raw) In-Reply-To: <YF540gjh156QIirA@rocinante> Hello! On Saturday 27 March 2021 01:14:10 Krzysztof Wilczyński wrote: > Hi Pali, > > Thank you for sending the patch over! > > [...] > > +static int pcie_change_tls_to_gen1(struct pci_dev *parent) > > Just a nitpick, so feel free to ignore it. I would just call the > variable "dev" as we pass a pointer to a particular device, but it does > not matter as much, so I am leaving this to you. I called it 'parent' because it is called 'parent' also in caller function. Link consists of two devices, so 'dev' could be ambiguous. > [...] > > + if (ret == 0) { > > You prefer this style over "if (!ret)"? Just asking in the view of the > style that seem to be preferred in the code base at the moment. I can change this to 'if (!ret)' if needed, no problem. I use 'if (!val)' mostly for boolean and pointer variables. If variable can contain more integer values then I lot of times I use '=='. > > + /* Verify that new value was really set */ > > + pcie_capability_read_word(parent, PCI_EXP_LNKCTL2, ®16); > > + if ((reg16 & PCI_EXP_LNKCTL2_TLS) != PCI_EXP_LNKCTL2_TLS_2_5GT) > > + ret = -EINVAL; > > I am wondering about this verification - did you have a case where the > device would not properly set its capability, or accept the write and do > nothing? I do not know any real device which is doing this thing. But this issue can happen with kernel's PCIe emulated root bridge: drivers/pci/pci-bridge-emul.c Drivers which are using this emulated root bridge (and both pci-mvebu.c and pci-aardvark.c are using it!) do not have to implement callback for every read and every write operation of every register. Note that both pci-mvebu.c and pci-aardvark.c currently does not implement access to HW register PCI_EXP_LNKCTL2. So currently it is not possible to set PCI_EXP_LNKCTL2_TLS_2_5GT. And above code correctly fails and disallow ASPM code to retrain link. I have some WIP patches which implement LNKCAP2, LNKCTL2 and LNKSTA2 read/write callbacks on emulated bridge for pci-mvebu.c and pci-aardvark.c. And I have tested that with those WIP patches ASPM code can correctly switch link to 2.5GT/s and enable ASPM. > > + if (ret != 0) > > I think "if (ret)" would be fine to use here, unless you prefer being > more explicit. See my question about style above. > > > static bool pcie_retrain_link(struct pcie_link_state *link) > > { > > struct pci_dev *parent = link->pdev; > > unsigned long end_jiffies; > > u16 reg16; > > + u32 reg32; > > + > > + /* Check if link is capable of higher speed than 2.5 GT/s and needs quirk */ > > + pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, ®32); > > + if ((reg32 & PCI_EXP_LNKCAP_SLS) > PCI_EXP_LNKCAP_SLS_2_5GB) { > > I wonder if moving this check to pcie_change_tls_to_gen1() would make > more sense? It would then make this function a little cleaner. What do > you think? Ok, no problem. But then function needs to be renamed. Any idea how should be this function called? > [...] > > +static void quirk_no_bus_reset_and_no_retrain_link(struct pci_dev *dev) > > +{ > > + dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET | PCI_DEV_FLAGS_NO_RETRAIN_LINK_WHEN_NOT_GEN1; > > +} > [...] > > I know that the style has been changed to allow 100 characters width and > that checkpatch.pl now also does not warn about line length, as per > commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column > warning"), but I think Bjorn still prefers 80 characters, thus this line > above might have to be aligned. Ok! If needed I can reformat patch to 80 characters per line.
WARNING: multiple messages have this Message-ID (diff)
From: "Pali Rohár" <pali@kernel.org> To: "Krzysztof Wilczyński" <kw@linux.com> Cc: "Bjorn Helgaas" <bhelgaas@google.com>, "Kalle Valo" <kvalo@codeaurora.org>, "Toke Høiland-Jørgensen" <toke@redhat.com>, "Marek Behún" <kabel@kernel.org>, vtolkm@gmail.com, "Rob Herring" <robh@kernel.org>, "Ilias Apalodimas" <ilias.apalodimas@linaro.org>, "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>, "Jason Cooper" <jason@lakedaemon.net>, linux-pci@vger.kernel.org, ath10k@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] PCI: Disallow retraining link for Atheros QCA98xx chips on non-Gen1 PCIe bridges Date: Sat, 27 Mar 2021 14:29:59 +0100 [thread overview] Message-ID: <20210327132959.fwkphna7gg57aove@pali> (raw) In-Reply-To: <YF540gjh156QIirA@rocinante> Hello! On Saturday 27 March 2021 01:14:10 Krzysztof Wilczyński wrote: > Hi Pali, > > Thank you for sending the patch over! > > [...] > > +static int pcie_change_tls_to_gen1(struct pci_dev *parent) > > Just a nitpick, so feel free to ignore it. I would just call the > variable "dev" as we pass a pointer to a particular device, but it does > not matter as much, so I am leaving this to you. I called it 'parent' because it is called 'parent' also in caller function. Link consists of two devices, so 'dev' could be ambiguous. > [...] > > + if (ret == 0) { > > You prefer this style over "if (!ret)"? Just asking in the view of the > style that seem to be preferred in the code base at the moment. I can change this to 'if (!ret)' if needed, no problem. I use 'if (!val)' mostly for boolean and pointer variables. If variable can contain more integer values then I lot of times I use '=='. > > + /* Verify that new value was really set */ > > + pcie_capability_read_word(parent, PCI_EXP_LNKCTL2, ®16); > > + if ((reg16 & PCI_EXP_LNKCTL2_TLS) != PCI_EXP_LNKCTL2_TLS_2_5GT) > > + ret = -EINVAL; > > I am wondering about this verification - did you have a case where the > device would not properly set its capability, or accept the write and do > nothing? I do not know any real device which is doing this thing. But this issue can happen with kernel's PCIe emulated root bridge: drivers/pci/pci-bridge-emul.c Drivers which are using this emulated root bridge (and both pci-mvebu.c and pci-aardvark.c are using it!) do not have to implement callback for every read and every write operation of every register. Note that both pci-mvebu.c and pci-aardvark.c currently does not implement access to HW register PCI_EXP_LNKCTL2. So currently it is not possible to set PCI_EXP_LNKCTL2_TLS_2_5GT. And above code correctly fails and disallow ASPM code to retrain link. I have some WIP patches which implement LNKCAP2, LNKCTL2 and LNKSTA2 read/write callbacks on emulated bridge for pci-mvebu.c and pci-aardvark.c. And I have tested that with those WIP patches ASPM code can correctly switch link to 2.5GT/s and enable ASPM. > > + if (ret != 0) > > I think "if (ret)" would be fine to use here, unless you prefer being > more explicit. See my question about style above. > > > static bool pcie_retrain_link(struct pcie_link_state *link) > > { > > struct pci_dev *parent = link->pdev; > > unsigned long end_jiffies; > > u16 reg16; > > + u32 reg32; > > + > > + /* Check if link is capable of higher speed than 2.5 GT/s and needs quirk */ > > + pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, ®32); > > + if ((reg32 & PCI_EXP_LNKCAP_SLS) > PCI_EXP_LNKCAP_SLS_2_5GB) { > > I wonder if moving this check to pcie_change_tls_to_gen1() would make > more sense? It would then make this function a little cleaner. What do > you think? Ok, no problem. But then function needs to be renamed. Any idea how should be this function called? > [...] > > +static void quirk_no_bus_reset_and_no_retrain_link(struct pci_dev *dev) > > +{ > > + dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET | PCI_DEV_FLAGS_NO_RETRAIN_LINK_WHEN_NOT_GEN1; > > +} > [...] > > I know that the style has been changed to allow 100 characters width and > that checkpatch.pl now also does not warn about line length, as per > commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column > warning"), but I think Bjorn still prefers 80 characters, thus this line > above might have to be aligned. Ok! If needed I can reformat patch to 80 characters per line. _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
next prev parent reply other threads:[~2021-03-27 13:36 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-26 12:43 [PATCH] PCI: Disallow retraining link for Atheros QCA98xx chips on non-Gen1 PCIe bridges Pali Rohár 2021-03-26 12:43 ` Pali Rohár 2021-03-26 18:13 ` Toke Høiland-Jørgensen 2021-03-26 18:13 ` Toke Høiland-Jørgensen 2021-03-27 0:14 ` Krzysztof Wilczyński 2021-03-27 0:14 ` Krzysztof Wilczyński 2021-03-27 13:29 ` Pali Rohár [this message] 2021-03-27 13:29 ` Pali Rohár 2021-03-27 14:42 ` Marek Behún 2021-03-27 14:42 ` Marek Behún 2021-03-27 17:33 ` Pali Rohár 2021-03-27 17:33 ` Pali Rohár 2021-04-27 10:55 ` [PATCH v2] PCI: Disallow retraining link for Atheros " Pali Rohár 2021-04-27 10:55 ` Pali Rohár 2021-04-30 11:41 ` Pali Rohár 2021-04-30 11:41 ` Pali Rohár 2021-05-05 16:33 ` [PATCH v3] " Pali Rohár 2021-05-05 16:33 ` Pali Rohár 2021-05-11 20:39 ` Pali Rohár 2021-05-11 20:39 ` Pali Rohár 2021-05-28 0:08 ` Pali Rohár 2021-05-28 0:08 ` Pali Rohár 2021-06-01 11:28 ` Krzysztof Wilczyński 2021-06-01 11:28 ` Krzysztof Wilczyński 2021-06-01 20:05 ` Bjorn Helgaas 2021-06-01 20:05 ` Bjorn Helgaas 2021-06-01 21:18 ` Pali Rohár 2021-06-01 21:18 ` Pali Rohár 2021-06-02 0:00 ` Bjorn Helgaas 2021-06-02 0:00 ` Bjorn Helgaas 2021-06-02 12:08 ` Pali Rohár 2021-06-02 12:08 ` Pali Rohár 2021-06-02 15:55 ` Bjorn Helgaas 2021-06-02 15:55 ` Bjorn Helgaas 2021-06-02 19:03 ` Pali Rohár 2021-06-02 19:03 ` Pali Rohár 2021-06-16 21:38 ` Bjorn Helgaas 2021-06-16 21:38 ` Bjorn Helgaas 2021-06-21 14:28 ` Pali Rohár 2021-06-21 14:28 ` Pali Rohár 2021-06-25 20:19 ` Bjorn Helgaas 2021-06-25 20:19 ` Bjorn Helgaas 2021-06-26 14:38 ` Pali Rohár 2021-06-26 14:38 ` Pali Rohár 2021-06-21 14:39 ` Pali Rohár 2021-06-21 14:39 ` Pali Rohár 2021-10-05 19:43 ` Jannis 2021-10-05 19:43 ` Jannis
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210327132959.fwkphna7gg57aove@pali \ --to=pali@kernel.org \ --cc=ath10k@lists.infradead.org \ --cc=bhelgaas@google.com \ --cc=ilias.apalodimas@linaro.org \ --cc=jason@lakedaemon.net \ --cc=kabel@kernel.org \ --cc=kvalo@codeaurora.org \ --cc=kw@linux.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=robh@kernel.org \ --cc=thomas.petazzoni@bootlin.com \ --cc=toke@redhat.com \ --cc=vtolkm@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.