Hi,
> Fix the following clang warning:
>
> drivers/pci/hotplug/shpchp_hpc.c:177:20: warning: unused function
> 'shpc_writeb' [-Wunused-function].
[...]
Nice catch! Thank you.
Reviewed-by: Krzysztof Wilczyński
By the way, next time capitalise the subject line.
Krzysztof
Hi,
> Fix the following clang warning:
>
> drivers/pci/controller/dwc/pcie-intel-gw.c:84:19: warning: unused
> function 'pcie_app_rd' [-Wunused-function].
[...]
Nice catch! Thank you.
Reviewed-by: Krzysztof Wilczyński
By the way, next time capitalise the subject line.
Krzysztof
Hi Greentime,
[...]
> + /* Wait for wait_idle */
> + ret = readl_poll_timeout(phy_cr_para_ack, val, val, 10, 5000);
> + if (ret)
> + dev_err(dev, "Wait for wait_ilde state failed!\n");
> +
> + /* Clear */
> + writel_relaxed(0, phy_cr_para_wr_en);
> +
> + /*
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
struct pci_epf_bar *epf_bar;
> struct pci_epc *epc;
Thank you!
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hi,
Thank you for sending the patch over!
[...]
> +static void pci_fixup_no_d3cold(struct pci_dev *pdev)
> +{
> + pci_info(pdev, "disable D3cold\n");
Not sure how useful this message would generally be? Unless this is
useful to someone who is doing some troubleshooting, etc.
> +
Hello,
[...]
Aside of the sysfs interface, would this new functionality also require
anything to be overridden at boot time via passing some command-line
arguments? Not sure how relevant such thing would be to device, but,
whatnot reset, though.
I am curious whether there would be a need for
Hi,
> + /*
> + * Regarding to the datasheet, the PCIE_VPH is suggested
> + * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the
> + * VREG_BYPASS should be cleared to zero.
> + */
[...]
A small nitpick here. What about the
Hi Kishon,
A few small nitpicks.
> Errata #i2037 in AM65x/DRA80xM Processors Silicon Revision 1.0
> (SPRZ452D–July 2018–Revised December 2019 [1]) mentions when an
> inbound PCIe TLP spans more than two internal AXI 128-byte bursts,
> the bus may corrupt the packet payload and the corrupt data
Hi Kishon,
[...]
> + if (!legacy_irq_domain) {
> + dev_err(dev, "Failed to add irq domain for legacy irqs\n");
> + return -EINVAL;
> + }
[...]
It would be "IRQ" and "IRQs" in the message above.
[...]
> - ret = ks_pcie_config_legacy_irq(ks_pcie);
> - if
Hi Kishon,
Thank you for sending the series over!
A few small nitpick, so feel free to ignore it.
[...]
> + ret = irq_domain_alloc_irqs_parent(domain, virq, 1, _fwspec);
> + if (ret < 0) {
> + pr_err("Failed to allocate parent irq %u: %d\n",
> +
u!
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hi,
[...]
> PCI: acpi_pcihp: Correct acpi_pci_check_ejectable() function name in
> the header
> PCI/AER: Correct function names in the header
> PCI/PME: Correct pcie_pme_init() function name in the header
> PCI/ATS: Correct pci_max_pasids() function name in header
[...]
All of this
Hi Alexander,
Thank you for sending the patch over!
> A single dw_pcie entity can't be a root complex and an endpoint at
> the same time.
Nice catch!
A small nitpick: this would be Root Complex and Endpoint, as it's
customary to capitalise these.
Also, if you could capitalise the subject line
just addressing report from Cocinelle).
Other than that,
Reviewed-by: Krzysztof Wilczyński
Thank you!
Krzysztof
+ pp->bridge->child_ops = _child_pcie_ops;
>
> ret = ks_pcie_config_legacy_irq(ks_pcie);
> if (ret)
[...]
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hi,
[...]
> > + /* Wait for wait_idle */
> > + ret = readl_poll_timeout(phy_cr_para_ack, val, val, 10, 5000);
> > + if (ret)
> > + dev_err(dev, "Wait for wait_ilde state failed!\n");
>
> It would be "wait_idle" rather than "wait_idle".
[...]
Apologies, meant to say "wait_ilde"
Hi,
[...]
> +static void fu740_phyregwrite(const uint8_t phy, const uint16_t addr,
> + const uint16_t wrdata, struct fu740_pcie *afp)
> +{
> + struct device *dev = afp->pci.dev;
> + void __iomem *phy_cr_para_addr;
> + void __iomem *phy_cr_para_wr_data;
> +
Hi,
Thank you for sending the patches over!
A few nitpicks.
> +title: SiFive fu740 PCIe host controller
> +
> +description:
> + SiFive fu740 PCIe host controller is based on the Synopsys DesignWare
> + PCI core. It shares common features with the PCIe DesignWare core and
> + inherits common
Hi Amey,
[...]
> Basically whole thing boils down to I'm not good at handling terminal
> email clients. I'll surely keep those points mentioned by Bjorn
> in my mind.
[...]
No worries. Thunderbird works fine with Google Mail and can send plain
text e-mails too, if you get tired of Mutt etc.
By
Hi Amey,
Thank you for sending the series over!
[...]
> > Reviews/Acks/Sign-off-by from others (aside from Tested/Reported-by)
> > really need to be explicit, IMO. This is a common issue for new
> > developers, but it really needs to be more formal. I wouldn't claim to
> > be able to speak for
Hi,
Thank you for sending the patches over!
> The Brcmstb PCIe RC uses a reset control "rescal" for certain chips. This
[...]
A small suggestion: it would be nicer to mention "Broadcom STB" rather
than "Brcmstb" in the sentence above.
[...]
> +err1:
> + reset_control_rearm(pcie->rescal);
;> drivers/built-in.a
>
> Select CONFIG_ECAM like the other drivers do.
[...]
Ouch. Thank you!
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hi,
[...]
> +/*
> + * pcie_port_service_get_irq - get irq of the service
> + * @dev: PCI Express port the service is associated with
> + * @service: For the service to find
> + *
> + * Get irq number associated with given service on a pci_dev
> + */
> +int pcie_port_service_get_irq(struct pci_dev
Hi,
> Currently, dw_pcie_msi_init() allocates and maps page for msi, then
> program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex
> may lose power during suspend-to-RAM, so when we resume, we want to
> redo the latter but not the former. If designware based driver (for
> example,
Hi Niklas,
[...]
> +static ssize_t index_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct zpci_dev *zdev = to_zpci(to_pci_dev(dev));
> + u32 index = ~0;
> +
> + if (zpci_unique_uid)
> + index = zdev->uid;
> +
> +
Hi,
[...]
> I would request NXP maintainers to take this patch, rewrite it as
> Bjorn requested and resend it as fast as possible, this is a very
> relevant fix.
[...]
Looking at the state of the pci-layerscape-ep.c file in Linus' tree,
this still hasn't been fixed, and it has been a while.
NXP
ank you!
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hi Moritz,
[...]
> + /* is PF driver loaded */
> + if (!pdev->driver) {
> + pci_info(pdev, "No driver bound to device. Cannot configure
> SRIOV\n");
> + ret = -ENOENT;
> + goto exit;
> + }
[...]
Thank you!
Hi,
[...]
> Property dma-can-stall depends on patchset
> https://lore.kernel.org/linux-iommu/20210108145217.2254447-1-jean-phili...@linaro.org/
[...]
If you plan to post another version of this patch to include the above
link into the commit message or reference to the commit itself, as
ndefined reference to
> `config_group_init_type_name'
[...]
Thank you for fixing this!
Reviewed-by: Krzysztof Wilczyński
Krzysztof
.]
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hi,
Thank you for sending the patch over. Few suggestions below.
There seem to be an extra space in the subject line.
> In this commit fixed coding style for braces and comments.
Where these coding style changes suggested by a tool? For example, was it
something like checkpatch.pl? If so,
Hi Antti,
A few nitpicks, so feel free to ignore these, of course.
If possible, capitalise the subject line. Also, perhaps "Add quirk to
prevent bus (...)" might read better.
> Some TI keystone C667X devices do no support bus/hot reset. Its PCIESS
[...]
It would be KeyStone in the above
Hi Pali,
> IRQ domain alloc function should return zero on success. Non-zero value
> indicates failure.
>
> Signed-off-by: Pali Rohár
> Fixes: fc54bae28818 ("PCI: iproc: Allow allocation of multiple MSIs")
[...]
Nice catch!
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hi Jiri,
> The drivers were removed in a1f714b44e34 (tty: Remove redundant synclink
> driver) and 3d608a591b2b (tty: Remove redundant synclinkmp driver).
>
> So remove also the PCI ID entries.
[...]
Thank you!
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hi Bjorn and Vidya,
[...]
> > +}
> > +
> > +struct pci_ecam_ops tegra194_pcie_ops = {
> > + .bus_shift = 20,
>
> I think e7708f5b10e2 ("PCI: Unify ECAM constants in native PCI Express
> drivers") means you don't need this .bus_shift.
[...]
Correct. If this platform implements ECAM as
Hi Jianjun,
[...]
> Thanks for your review,
Thank YOU for all the work here!
[...]
> > > Add suspend_noirq and resume_noirq callback functions to implement
> > > PM system suspend hooks for MediaTek Gen3 PCIe controller.
> >
> > So, "systems suspend" and "resume" hooks, correct?
>
> The
Hi Jianjun,
[...]
> +static struct irq_chip mtk_msi_irq_chip = {
> + .name = "MSI",
> + .irq_enable = mtk_pcie_irq_unmask,
> + .irq_disable = mtk_pcie_irq_mask,
> + .irq_ack = irq_chip_ack_parent,
> + .irq_mask = mtk_pcie_irq_mask,
> + .irq_unmask = mtk_pcie_irq_unmask,
>
Hi Jianjun,
[...]
> +/**
> + * mtk_intx_eoi
> + * @data: pointer to chip specific data
> + *
> + * As an emulated level IRQ, its interrupt status will remain
> + * until the corresponding de-assert message is received; hence that
> + * the status can only be cleared when the interrupt has been
Hi Jianjun,
> Add suspend_noirq and resume_noirq callback functions to implement
> PM system suspend hooks for MediaTek Gen3 PCIe controller.
So, "systems suspend" and "resume" hooks, correct?
> When system suspend, trigger the PCIe link to L2 state and pull down
It probably would be "the
Hi Jianjun,
Thank you for all the work here!
[...]
> + * struct mtk_pcie_port - PCIe port information
> + * @dev: pointer to PCIe device
> + * @base: IO mapped register base
> + * @reg_base: Physical register base
> + * @mac_reset: mac reset control
> + * @phy_reset: phy reset control
> + *
Hi Kishon,
[...]
>clocks:
> -maxItems: 1
> -description: clock-specifier to represent input to the PCIe
> +minItems: 1
> +maxItems: 2
> +description: clock-specifier to represent input to the PCIe for 1 item.
> + 2nd item if present represents reference clock to the
[+cc Lorenzo for visiblity]
Hi,
[...]
> Fix rcec_assoc_rciep() using the PCI_SLOT() macro and convert the value
> of "rciep->devfn" to a device number to ensure that the RCiEP devices
> associated with the RCEC are linked when the RCEC is enumerated.
>
> [ Krzysztof: Update commit message. ]
Hi Robert,
[...]
> Obiously this is meant here:
>
> if (!pci_is_managed(dev))
[...]
A question to improve my understanding for future reference. Was the
previous approach of checking for "enabled" flag from struct pci_devres
was not a good choice here?
Krzysztof
Hi Qiuxu,
[...]
> > Agree to simplify the commit message. How about the following subject and
> > commit message?
> >
> > Subject:
> > Use device number to check RCiEPBitmap of RCEC
> >
> > Commit message:
> > rcec_assoc_rciep() used the combination of device number and function
> > number
[+cc Bjorn as we talked about RCiEP briefly on IRC]
Hello Qiuxu,
[...]
> Sorry, just back from Chinese New Year holiday.
Welcome back! I hope you had a nice rest, and also Happy New Year!
[...]
> > Would this only affect error injection or would this be also a generic
> > problem
> > with
[...]
> > Refer to commit 075af61c19cd ("PCI: imx6: Limit DBI register length"),
> > i.MX6QP PCIe has the similar issue.
> > Define the length of the DBI registers and limit config space to its
> > length for i.MX6QP PCIe too.
>
> You could probably flip these two sentences around to make the
imx6_pcie_drvdata drvdata[] = {
> .variant = IMX6QP,
> .flags = IMX6_PCIE_FLAG_IMX6_PHY |
>IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
> + .dbi_length = 0x200,
> },
> [IMX7D] = {
> .variant = IMX7D,
Reviewed-by: Krzysztof Wilczyński
Krzysztof
> + dr = find_pci_dr(dev);
> + if (!dr || !dr->enabled)
> + return -EINVAL;
> +
> + return pci_alloc_irq_vectors(dev, min_vecs, max_vecs, flags);
> +}
> +EXPORT_SYMBOL(pcim_alloc_irq_vectors);
[...]
Looks good! Thank you for adding kernel-doc here! Much appreciated.
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hi Dejin,
> Use the new function pcim_alloc_irq_vectors() to allocate IRQ vectors,
> the pcim_alloc_irq_vectors() function, an explicit device-managed version
> of pci_alloc_irq_vectors(). If pcim_enable_device() has been called
> before, then pci_alloc_irq_vectors() is actually a device-managed
ci_driver's probe callback fail,
and I assume that this is precisely the case, then all of the above make
sense in the view of using pcim_alloc_irq_vectors().
Reviewed-by: Krzysztof Wilczyński
Krzysztof
.
Having said that, people might ask - how does it simplify the error
handling path?
You might have to back this with a line of two to explain how does the
change achieved that, so that when someone looks at the commit message
it would be clear what the benefits of the change were.
Reviewed-by:
Hi Dejin and Andy,
[...]
> > > Question: wouldn't you need to call pci_free_irq_vectors() somewhere,
> > > possibly to pcim_release() callback? Although, I am not sure where the
> > > right place would be.
> > >
> > > I am asking, as the documentation (see [4]) suggests that one would have
> >
een retired.
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hello Dejin,
[...]
> add pcim_alloc_irq_vectors(), a explicit device-managed version of
> pci_alloc_irq_vectors().
It would be "Add" at the start of the sentence. You could also drop
the "explicit" word or replace it with "an explicit", if you want to
keep it.
Generally, this commit message
Hi Dejin,
See my comments regarding a very similar patch:
https://lore.kernel.org/linux-pci/YCrnn+L42SR4N1PA@rocinante/
Krzysztof
Hi Dejin,
> Use the correct name of device-managed function to alloc irq vectors,
> the pcim_alloc_irq_vectors() function, a explicit device-managed version
> of pci_alloc_irq_vectors().
[...]
It would be "IRQ" in the sentence above.
Perhaps the "Use the new function pcim_alloc_irq_vectors() to
Hi Dejin,
Thank you for working on this series!
Do you have a link to the conversation that prompted addition of this
new function? If so, then it would be nice to include a reference to it
here (as a link to http://lore.kernel.org/) in the cover letter for
reference, if possible, of course.
Hi Dejin,
Thank you for all the work here!
The subject and the commit message could be improved to include a little
more details about why do you want to do it, and what problems does it
aims to solve.
> Introduce pcim_alloc_irq_vectors(), a explicit device-managed version of
>
Hi Gustavo,
> Detected by CoverityScan CID 16555 ("Dereference null return")
[...]
We can use the "Addresses-Coverity:" here, as it seems to become
a canonical thing for Coverity reports. It would also be nice to
briefly mention what the issues was, if possible.
Krzysztof
Hi Gustavo,
> Reverting the applied patch because it caused a regression on
> ARC700 platform (32 bits).
[...]
Do you have the previous commit handy to reference here?
Krzysztof
Hi Gustavo,
> In the previous implementation the driver assumes that only existed 2
> memory spaces that would be equal distributed amount the write/read
> channels.
What do you think about:
In the previous implementation the driver assumed that there existed
only two memory spaces that
Hi Gustavo,
> + /*
> + * Tries to find if exists a PCIe Vendor-Specific Extended Capability
> + * for the DMA, if exists one, then reconfigures with the new data
[...]
What about "if one exists" and "then reconfigures it". Missing period
at the end of the sentence.
Krzysztof
Hi Gustavo,
[...]
> + * Typically this function will be called by the pci driver, which passes
It would be "PCI" here.
> + * through argument the 'struct pci_dev *' already pointing for the device
> + * config space that is associated with the vendor and device ID which will
> + * know which ID
Hi Gustavo,
[...]
> +config DW_XDATA_PCIE
> + depends on PCI
> + tristate "Synopsys DesignWare xData PCIe driver"
> + help
> + This driver allows controlling Synopsys DesignWare PCIe traffic
> + generator IP also known as xData, present in Synopsys Designware
> +
Hi Bharat,
Thank you for sending the patches over!
> Add support for routing PCIe DMA traffic coherently when
> Cache Coherent Interconnect (CCI) is enabled in the system.
> The "dma-coherent" property is used to determine if CCI is enabled
> or not.
> Refer
Hi Qiuxu,
Nice catch! Thank you for sending the fix over!
[...]
> On a Sapphire Rapids server, it failed to inject correctable errors
> to the RCiEP device e8:02.0 which was associated with the RCEC device
> e8:00.4. See the following error log before applying the patch:
>
> aer-inject -s
gt; Signed-off-by: Yicong Yang
> Signed-off-by: Bjorn Helgaas
> Reviewed-by: Krzysztof Wilczyński
>
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 11cc79411e2d..d62c4ac4ae1b 100644
> --- a/drivers/pci/Makefile
> +++ b
Hi Gustavo,
[...]
> > That's true, there are a lot of drivers that don't verify that pointer.
> > What do you suggest?
> > 1) To remove the verification so that is aligned with the other drivers
> > 2) Leave it as is. Or even to add this verification to the other drivers?
> >
> > Either way, I
Hi Gustavo,
[...]
> > This "pcim_iomap_table(dev)[n]" pattern is extremely common. There
> > are over 100 calls of pcim_iomap_table(), and
> >
> > $ git grep "pcim_iomap_table(.*)\[.*\]" | wc -l
> >
> > says about 75 of them are of this form, where we dereference the
> > result before
Hi Gustavo,
[...]
> > The code in question would be (exceprt from the patch):
> >
> > [...]
> > +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *pid)
> > +{
> > + const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
Hi Gustavo,
[...]
> > When I do the following:
> >
> > # echo 1 > /sys/kernel/dw-xdata-pcie/write
> > # echo 1 > /sys/kernel/dw-xdata-pcie/stop
> > # cat /sys/kernel/dw-xdata-pcie/write
> >
> > Would output from cat above simply show "0 MB/s" then? I wonder how
> > someone using this new
[+cc Bjorn]
Hi Gustavo,
[...]
> Thanks for your review. I will wait for a couple of days, before sending
> a new version of this patch series based on your feedback.
Thank you!
There might be one more change, and improvement, to be done as per
Bjorn's feedback, see:
pci_ids.h
> @@ -2589,6 +2589,8 @@
>
> #define PCI_VENDOR_ID_REDHAT 0x1b36
>
> +#define PCI_VENDOR_ID_SILICOM_DENMARK0x1c2c
> +
> #define PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS 0x1c36
>
> #define PCI_VENDOR_ID_CIRCUITCO 0x1cc8
> --
> 2.29.2
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hi Martin,
Thank you for sending the patch over!
A few suggestions. Try to keep the patch style aligned with previous
submissions, see the following for to format the subject line, etc.,
a few examples:
$ git log --oneline include/linux/pci_ids.h | grep -E 'Add.*ID' | head -n 10
8b7beaf9f185
Hi Gustavo,
Thank you for all the work here!
A few suggestions.
[...]
> +static void dw_xdata_stop(struct dw_xdata *dw)
> +{
> + u32 burst = readl(&(__dw_xdara_regs(dw)->burst_cnt));
> +
> + if (burst & BIT(31)) {
> + burst &= ~(u32)BIT(31);
> + writel(burst,
Hi Gustavo,
[...]
> +The interaction with this driver is done through the module parameter and
> +can be changed in runtime. The driver outputs the requested command state
> +information to /var/log/kern.log or dmesg.
The driver does not seem to offer any parameters (aside of using sysfs
for
A suggestion. You could improve this even further, for example:
if (pci_aer_available() && pcie_aer_is_native(dev)) {
This is because the pcie_aer_is_native() function performs the
dev->aer_cap check internally, so we could rely on it, and avoid
checking the same thing twice.
What do
Hi Mauro,
Thank you for working on this!
> @@ -151,8 +152,10 @@ struct kirin_pcie {
> struct clk *phy_ref_clk;
> struct clk *pcie_aclk;
> struct clk *pcie_aux_clk;
> - int gpio_id_reset[4];
> + int n_gpio_resets;
> int
Hi Victor,
Thank you for working on this!
[...]
> i = pci_save_pcie_state(dev);
> if (i != 0)
> - return i;
> + goto Exit;
>
> i = pci_save_pcix_state(dev);
> if (i != 0)
> - return i;
> + goto Exit;
[...]
> +Exit:
> +
Hi Hai,
Thank you for taking care about this.
On 20-11-17 14:41:42, Wang Hai wrote:
I would have to ask you to capitalise the first letter in the subject
line as it has been done for other patches. Check Git history to see
what it normally would look like.
> Fix to return a negative error
when showing the
> speed to the user.
>
> This patch adds the decoding of this new speed, previously, reading the
> speed of a link operating at this speed showed "Unknown speed" instead
> of "64.0 GT/s".
Looks good! Thank you for taking care about this.
Reviewed-by: Krzysztof Wilczyński
Krzysztof
Hello Pali!
Sincere apologies for taking a long time to get back to you.
On 20-11-04 17:29:31, Pali Rohár wrote:
[...]
>
> Krzysztof, as Bjorn wrote, do you want to take this issue?
>
[...]
Yes. I already talked to Bjorn about this briefly, and thus I am more
than happy to take care about
Hi Maximilian,
On 20-11-02 15:15:20, Maximilian Luz wrote:
> While most PCI power-states can be queried from user-space via lspci,
> this has some limits. Specifically, lspci fails to provide an accurate
> value when the device is in D3cold as it has to resume the device before
> it can access
, and then drop said enum and the typedef pci_bus_flags_t.
This will keep PCI device-specific features as part of the struct
pci_dev and make the code that used to use flags simpler.
Related:
https://patchwork.kernel.org/patch/11772809
Suggested-by: Bjorn Helgaas
Signed-off-by: Krzysztof Wilczyński
86 matches
Mail list logo
