Re: [PATCH v9 2/9] Input: goodix - reset device at init
On Tue, Oct 13, 2015 at 08:54:12AM +, Tirdea, Irina wrote: > > > I did not use devm_gpiod_get_optional() in order to ignore more errors > > > than -ENOENT. This is needed because the ACPI gpio core will fall back > > > to indexed gpios if named gpios are not found. In the common case of > > > having 2 indexed gpio pins declared in the ACPI table, the first > > > devm_gpiod_get() will successfully get indexed gpio pin 0 and the > > > second devm_gpiod_get() will try to get the same gpio pin 0 and return > > > -EBUSY. Considering this, I thought it is better to just ignore all > > > errors in > > > order not to break any platforms currently using this driver. > > > > This seems like issue with ACPI gpio lookup implementation. If I am > > requesting named gpio and it is not present then I definitely do not > > need to be returned some random gpio. Doing so breaks all other drivers > > that use several names to retrieve GPIOs. We basically can't trust GPIO > > API on ACPI systems. > > > > I'm not sure there is a way to avoid fall back to indexed gpios when > requesting > named gpios. > Adding Mika to this thread as he might help answer this. Before ACPI 5.1 _DSD device properties were introduced all we had was an array of GPIOs returned by _CRS ACPI method. Ordering of those GPIOs could change from one vendor to another :-( We can (and do) use acpi_dev_add_driver_gpios() to pass correct mappings where _DSD is not present based on the device ACPI ID for instance. Not all drivers do that, though. I would like to get rid of the fallback completely at some point. We have had already problems with the API because then some ACPI only drivers did this: reset_gpio = gpiod_get_index(dev, NULL, 0); power_gpio = gpiod_get_index(dev, NULL, 1); which might not do what is expected on DT systems. That's why acpi_dev_add_driver_gpios() was added in the first place IIRC. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 2/9] Input: goodix - reset device at init
On Tue, Oct 13, 2015 at 11:23:03PM -0700, Dmitry Torokhov wrote: > I understand why one might use acpi_dev_add_driver_gpios() to augment > data in ACPI, however here we have completely different issue: driver > that expects named gpios gets returned gpio that has nothing to do with > what it requested, because gpiolib acpi code always falls back to > unnamed gpio if it does not find named gpio. That can be acceptable if > driver uses the same con_id for all requests to gpiolib, but is not > working when driver supplies different con_ids. Right, the ACPI fallback ignores con_id completely and uses only the index. AFAIK there is only one driver using ACPI _CRS index method: sdhci-[acpi|pci].c. If we can convert that to use acpi_dev_add_driver_gpios() to feed names for card detection GPIOs, I think we can remove the fallback alltogether in favor of named GPIOs for ACPI. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 2/9] Input: goodix - reset device at init
On Wed, Oct 14, 2015 at 02:18:20PM +0300, mika.westerb...@linux.intel.com wrote: > On Tue, Oct 13, 2015 at 11:23:03PM -0700, Dmitry Torokhov wrote: > > I understand why one might use acpi_dev_add_driver_gpios() to augment > > data in ACPI, however here we have completely different issue: driver > > that expects named gpios gets returned gpio that has nothing to do with > > what it requested, because gpiolib acpi code always falls back to > > unnamed gpio if it does not find named gpio. That can be acceptable if > > driver uses the same con_id for all requests to gpiolib, but is not > > working when driver supplies different con_ids. > > Right, the ACPI fallback ignores con_id completely and uses only the > index. > > AFAIK there is only one driver using ACPI _CRS index method: > sdhci-[acpi|pci].c. If we can convert that to use acpi_dev_add_driver_gpios() > to feed names for card detection GPIOs, I think we can remove the > fallback alltogether in favor of named GPIOs for ACPI. Nah, there seems to be several drivers relying on this already :-/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 2/9] Input: goodix - reset device at init
On Mon, Oct 19, 2015 at 02:32:24PM +, Tirdea, Irina wrote:
>
>
> > -Original Message-
> > From: linux-input-ow...@vger.kernel.org
> > [mailto:linux-input-ow...@vger.kernel.org] On Behalf Of
> > mika.westerb...@linux.intel.com
> > Sent: 14 October, 2015 16:44
> > To: Dmitry Torokhov
> > Cc: Tirdea, Irina; Bastien Nocera; Aleksei Mamlin; Karsten Merker;
> > linux-in...@vger.kernel.org; Mark Rutland; Purdila, Octavian; linux-
> > ker...@vger.kernel.org; devicet...@vger.kernel.org
> > Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init
> >
> > On Wed, Oct 14, 2015 at 02:18:20PM +0300, mika.westerb...@linux.intel.com
> > wrote:
> > > On Tue, Oct 13, 2015 at 11:23:03PM -0700, Dmitry Torokhov wrote:
> > > > I understand why one might use acpi_dev_add_driver_gpios() to augment
> > > > data in ACPI, however here we have completely different issue: driver
> > > > that expects named gpios gets returned gpio that has nothing to do with
> > > > what it requested, because gpiolib acpi code always falls back to
> > > > unnamed gpio if it does not find named gpio. That can be acceptable if
> > > > driver uses the same con_id for all requests to gpiolib, but is not
> > > > working when driver supplies different con_ids.
> > >
> > > Right, the ACPI fallback ignores con_id completely and uses only the
> > > index.
> > >
> > > AFAIK there is only one driver using ACPI _CRS index method:
> > > sdhci-[acpi|pci].c. If we can convert that to use
> > > acpi_dev_add_driver_gpios()
> > > to feed names for card detection GPIOs, I think we can remove the
> > > fallback alltogether in favor of named GPIOs for ACPI.
> >
> > Nah, there seems to be several drivers relying on this already :-/
>
> Would it be possible to add an optional parameter to the GPIO API
> to specify whether we want to fall back to indexed GPIOs for ACPI?
I don't think it's a good idea to add ACPI specifics to generic APIs.
I went through ACPI enabled drivers calling GPIO APIs and majority of
them are doing this:
static int stk8312_gpio_probe(struct i2c_client *client)
{
struct device *dev;
struct gpio_desc *gpio;
int ret;
if (!client)
return -EINVAL;
dev = &client->dev;
/* data ready gpio interrupt pin */
gpio = devm_gpiod_get_index(dev, STK8312_GPIO, 0, GPIOD_IN);
if (IS_ERR(gpio)) {
dev_err(dev, "acpi gpio get index failed\n");
return PTR_ERR(gpio);
}
ret = gpiod_to_irq(gpio);
dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
return ret;
}
We can drop all this because I2C core already handles GpioInt -> interrupt
number translation.
Few drivers are doing something more complex but I think we can still convert
them to use acpi_dev_add_driver_gpios() and eventually get rid of the whole
_CRS index lookup.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Qemu Guest kernel 4.20-rc1 PCIe hotplug issue
+Lukas
On Tue, Nov 13, 2018 at 11:45:42AM +, Shameerali Kolothum Thodi wrote:
> Hi Mika,
Hi,
> Since the commit commit 720d6a671a6e("PCI: pciehp: Do not handle events
> if interrupts are masked"), the hotplug support on Qemu Guest(4.120-rc1)
> with a vfio passthrough device seems to be broken. This is on an ARM64
> platform.
>
> I am booting a Guest with below command line options with the intention of
> hot add a ixgbevf dev later,
>
> ./qemu-system-aarch64 -machine virt,kernel_irqchip=on,gic-version=3 -cpu host
> \
> -kernel Image_4.20-rc1 \
> -initrd rootfs-iperf.cpio \
> -device ioh3420,id=rp1 \
> -net none \
> -m 4096 \
> -nographic -D -d -enable-kvm \
> -append "console=ttyAMA0 root=/dev/vda -m 4096 rw pciehp.pciehp_debug=1
> pcie_ports=native searlycon=pl011,0x900"
>
> But receives this on boot,
>
> [1.327852] pciehp :00:01.0:pcie004: Timeout
> on hotplug command 0x03f1 (issued 1016 msec ago)
> [1.335842] pciehp :00:01.0:pcie004: Timeout on hotplug command
> 0x03f1 (issued 1024 msec ago)
> [3.847843] pciehp :00:01.0:pcie004: Failed to check link status
> [3.855843] pciehp :00:01.0:pcie004: Timeout on hotplug command
> 0x02f1 (issued 2520 msec ago)
> [4.879846] pciehp :00:01.0:pcie004: Timeout on hotplug command
> 0x06f1 (issued 1024 msec ago)
> [5.911840] pciehp :00:01.0:pcie004: Timeout on hotplug command
> 0x06f1 (issued 2056 msec ago)
> [6.927844] pciehp :00:01.0:pcie004: Timeout on hotplug command
> 0x07f1 (issued 1016 msec ago)
> [7.951843] pciehp :00:01.0:pcie004: Timeout on hotplug command
> 0x0771 (issued 1024 msec ago)
>
> Trying to hot add using "device_addvfio-pci,host=:01:10.1,id=net0,bus=rp1"
> doesn't work either. And if I boot the guest with an assigned device
> (-device vfio-pci,host=:01:10.1,id=net0,bus=rp1), I can see the dev
> listed in
> the Guest but then hot remove doesn't work.
>
> This all works on 4.19 and bisect points to the above mentioned commit, where
> an
> additional check is added in pciehp_isr(),
>
> - * Interrupts only occur in D3hot or shallower (PCIe r4.0, sec 6.7.3.4).
> + * Interrupts only occur in D3hot or shallower and only if enabled
> + * in the Slot Control register (PCIe r4.0, sec 6.7.3.4).
>*/
> - if (pdev->current_state == PCI_D3cold)
> + if (pdev->current_state == PCI_D3cold ||
> + (!(ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE) && !pciehp_poll_mode))
> return IRQ_NONE;
>
> I think this doesn't work for the first time, where the cmd with
> PCI_EXP_SLTCTL_HPIE bit set
> is written,
> pciehp_probe()
> pcie_init_notification()
> pcie_enable_notification()
>pcie_do_write_cmd()
>
> to begin with, ctrl->slot_ctrl = 0 in pciehp_isr() as this is only set once
> the write
> is returned.
>
> Or else I am missing something here. Please take a look and let me know.
Thanks for the detailed report and analysis. I think you are right and
the ctrl->slot_ctrl is only set after the actual value has been
programmed to the hardware, so if there was interrupt "pending" it will
trigger immediately (just to find ctrl->slot_ctrl == 0).
I wonder if the following change helps here?
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 7dd443aea5a5..cd9eae650aa5 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -156,9 +156,9 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16
cmd,
slot_ctrl |= (cmd & mask);
ctrl->cmd_busy = 1;
smp_mb();
+ ctrl->slot_ctrl = slot_ctrl;
pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);
ctrl->cmd_started = jiffies;
- ctrl->slot_ctrl = slot_ctrl;
/*
* Controllers with the Intel CF118 and similar errata advertise
Re: Qemu Guest kernel 4.20-rc1 PCIe hotplug issue
On Tue, Nov 13, 2018 at 12:36:20PM +, Shameerali Kolothum Thodi wrote: > > @@ -156,9 +156,9 @@ static void pcie_do_write_cmd(struct controller *ctrl, > > u16 cmd, > > slot_ctrl |= (cmd & mask); > > ctrl->cmd_busy = 1; > > smp_mb(); > > + ctrl->slot_ctrl = slot_ctrl; > > Actually I tried this one, but it doesn't help in this case as the initial > pcie_capability_read_word() returns the slot_ctrl without PCI_EXP_SLTCTL_HPIE > bit set. It looks to me pcie_enable_notification() function enables this, > > if (!pciehp_poll_mode) > cmd |= PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE; > > I don't know this is as per the spec or not as the initial cap read doesn't > seems to > have the PCI_EXP_SLTCTL_HPIE bit set. If I read the code right cmd value should end up in ctrl->slot_ctrl properly from pcie_enable_notification(). However, I think we are missing check for PCI_EXP_SLTCTL_CCIE in pciehp_isr(). Here's an updated patch, can you try and see if it makes any difference? diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 7dd443aea5a5..da2cbe892444 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -156,9 +156,9 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd, slot_ctrl |= (cmd & mask); ctrl->cmd_busy = 1; smp_mb(); + ctrl->slot_ctrl = slot_ctrl; pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl); ctrl->cmd_started = jiffies; - ctrl->slot_ctrl = slot_ctrl; /* * Controllers with the Intel CF118 and similar errata advertise @@ -522,7 +522,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id) * in the Slot Control register (PCIe r4.0, sec 6.7.3.4). */ if (pdev->current_state == PCI_D3cold || - (!(ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE) && !pciehp_poll_mode)) + (!(ctrl->slot_ctrl & (PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE)) && !pciehp_poll_mode)) return IRQ_NONE; /*
Re: Qemu Guest kernel 4.20-rc1 PCIe hotplug issue
On Tue, Nov 13, 2018 at 01:28:04PM +, Shameerali Kolothum Thodi wrote: > > > -Original Message- > > From: mika.westerb...@linux.intel.com > > [mailto:mika.westerb...@linux.intel.com] > > Sent: 13 November 2018 12:59 > > To: Shameerali Kolothum Thodi > > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Wangzhou (B) > > ; Linuxarm ; Lukas > > Wunner > > Subject: Re: Qemu Guest kernel 4.20-rc1 PCIe hotplug issue > > > > On Tue, Nov 13, 2018 at 12:36:20PM +, Shameerali Kolothum Thodi wrote: > > > > @@ -156,9 +156,9 @@ static void pcie_do_write_cmd(struct controller > > > > *ctrl, > > > > u16 cmd, > > > > slot_ctrl |= (cmd & mask); > > > > ctrl->cmd_busy = 1; > > > > smp_mb(); > > > > + ctrl->slot_ctrl = slot_ctrl; > > > > > > Actually I tried this one, but it doesn't help in this case as the > > > initial > > > pcie_capability_read_word() returns the slot_ctrl without > > > PCI_EXP_SLTCTL_HPIE bit set. It looks to me > > > pcie_enable_notification() function enables this, > > > > > > if (!pciehp_poll_mode) > > > cmd |= PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE; > > > > > > I don't know this is as per the spec or not as the initial cap read > > > doesn't seems to have the PCI_EXP_SLTCTL_HPIE bit set. > > > > If I read the code right cmd value should end up in ctrl->slot_ctrl > > properly from > > pcie_enable_notification(). > > Right. As I mentioned in my previous mail, I missed the fact that you are > updating > the ctrl->slot_ctrl with cmd value while in my test I did my update with the > value > returned by pcie_capability_read_word(). OK, I see. > > However, I think we are missing check for PCI_EXP_SLTCTL_CCIE in > > pciehp_isr(). > > Ok. > > > Here's an updated patch, can you try and see if it makes any difference? > > I just tried this and it works. Thanks. Can you still check that the previous one (without _CCIE check) works? > See few comments below. > > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c > > b/drivers/pci/hotplug/pciehp_hpc.c > > index 7dd443aea5a5..da2cbe892444 100644 > > --- a/drivers/pci/hotplug/pciehp_hpc.c > > +++ b/drivers/pci/hotplug/pciehp_hpc.c > > @@ -156,9 +156,9 @@ static void pcie_do_write_cmd(struct controller *ctrl, > > u16 cmd, > > slot_ctrl |= (cmd & mask); > > ctrl->cmd_busy = 1; > > smp_mb(); > > + ctrl->slot_ctrl = slot_ctrl; > > Does it make more sense if we can move this before smp_mb()?. Also I am not > sure updating the ctrl->slot_ctrl before actually the hardware is programmed > with that value will result in any other race conditions? TBH, I am not that > familiar > with this code and I leave that to you :) Both are good questions :) For the moving ctrl->slot_ctrl before pcie_capability_write_word(), I think we should be fine and this is actually more correct because if we are unmasking interrupts they may trigger immediately making pciehp_isr() find wrong values in ctrl->slot_ctrl (as can be seen in the issue you reported). The smb_mb() thing is not that clear (at least to me) because it is used in two places in the driver and both seem to be making write to ctrl->cmd_busy visible to other CPUs but I don't see where we deal with the read part. I may be missing something, though.
Re: Qemu Guest kernel 4.20-rc1 PCIe hotplug issue
On Tue, Nov 13, 2018 at 03:57:47PM +, Shameerali Kolothum Thodi wrote:
> > The smb_mb() thing is not that clear (at least to me) because it is used
> > in two places in the driver and both seem to be making write to
> > ctrl->cmd_busy visible to other CPUs but I don't see where we deal with
> > the read part.
> >
> > I may be missing something, though.
>
> I think the read part is in wait_event_timeout() which evaluates the
> condition.
> The wake_up is called from the pciehp_isr(). Since the flag is being updated
> in both process level and interrupt handler context, smp_mb() is used. I think
> the same now applies to ctrl->slot_ctrl now as this being used in process
> context and interrupt context as well.
Right, but that would require to use another read/general barrier in the
pciehp_isr() before we read the variable in case interrupt happens
immediately on another CPU (at least that's my understanding). Since I'm
not too comfortable with all these barriers to be honest I would prefer
reading the slot control register directly in pciehp_isr() :-)
I wonder if the below works in your case? I think it is still easier to
understand than adding another barrier there.
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 7dd443aea5a5..575da1005836 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -518,11 +518,9 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
u16 status, events;
/*
-* Interrupts only occur in D3hot or shallower and only if enabled
-* in the Slot Control register (PCIe r4.0, sec 6.7.3.4).
+* Interrupts only occur in D3hot or shallower (PCIe r4.0, sec 6.7.3.4).
*/
- if (pdev->current_state == PCI_D3cold ||
- (!(ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE) && !pciehp_poll_mode))
+ if (pdev->current_state == PCI_D3cold)
return IRQ_NONE;
/*
@@ -548,6 +546,22 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
return IRQ_NONE;
}
+ if (!pciehp_poll_mode) {
+ u16 ctrl;
+
+ /*
+* Check that the hotplug interrupt was enabled. It may
+* be that the interrupt was meant for PME instead as
+* they share the MSI vector.
+*/
+ pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
+ if (ctrl == (u16) ~0 || !(ctrl & PCI_EXP_SLTCTL_HPIE)) {
+ if (parent)
+ pm_runtime_put(parent);
+ return IRQ_NONE;
+ }
+ }
+
/*
* Slot Status contains plain status bits as well as event
* notification bits; right now we only want the event bits.
Re: Qemu Guest kernel 4.20-rc1 PCIe hotplug issue
On Wed, Nov 14, 2018 at 02:30:14PM +0100, Lukas Wunner wrote: > On Wed, Nov 14, 2018 at 11:52:25AM +0200, mika.westerb...@linux.intel.com > wrote: > > On Tue, Nov 13, 2018 at 03:57:47PM +, Shameerali Kolothum Thodi wrote: > > > > The smb_mb() thing is not that clear (at least to me) because it is used > > > > in two places in the driver and both seem to be making write to > > > > ctrl->cmd_busy visible to other CPUs but I don't see where we deal with > > > > the read part. > > > > > > > > I may be missing something, though. > > > > > > I think the read part is in wait_event_timeout() which evaluates the > > > condition. The wake_up is called from the pciehp_isr(). Since the flag > > > is being updated in both process level and interrupt handler context, > > > smp_mb() is used. I think the same now applies to ctrl->slot_ctrl now > > > as this being used in process context and interrupt context as well. > > > > Right, but that would require to use another read/general barrier in the > > pciehp_isr() before we read the variable in case interrupt happens > > immediately on another CPU (at least that's my understanding). > > In pcie_do_write_cmd(), please just move the > > ctrl->slot_ctrl = slot_ctrl; > > above the call to pcie_capability_write_word(). > > AFAICS an explicit memory barrier isn't needed here because of the call to > pcie_capability_write_word(), which "will [ordinarily] be guaranteed to be > fully ordered and uncombined" (Documentation/memory-barriers.txt, section > "KERNEL I/O BARRIER EFFECTS"). > > The memory barrier in pciehp_isr() is also bogus because the following > wake_up() implies a memory barrier if a task was woken. (And if none > was woken, who cares.) > > > > Since I'm > > not too comfortable with all these barriers to be honest I would prefer > > reading the slot control register directly in pciehp_isr() :-) > > That is an approach I'd strongly object to: While pciehp itself only > signals very few interrupts (making an additional mmio read appear to > be negligible), it may share its interrupt with other devices. On my > MacBookPro9,1, a hotplug port of the Thunderbolt controller shares > its interrupt line with the Wifi card and SD card reader, and those > may signal a huge number of interrupts. On such a machine an additional > mmio read per interrupt becomes a problem. OK. I just sent a patch moving ctrl->slot_ctrl assignment to happen before pcie_capability_write_word().
Re: [PATCH] i2c: designware: use enable on resume instead initialization
On Wed, Jun 24, 2015 at 12:56:19PM +, De Marchi, Lucas wrote: > Yeah, but it would be bad to ignore the problem as well. The way it is now > kills any possibility of using DW controller for reading sensors like > gyroscope, accelerometer, barometer that have higher sampling rate etc. I'll > try to come up with a new patch but since I can't reproduce the problem here > it'd be good to know if there's any means for me to test. What do you think > that could be done? Maybe putting the controller to sleep only in case of > errors? Instead of disabling the adapter after each transfer, I wonder if it is enough if we just mask all interrupts? That should also prevent the interrupt loop Christian is seeing on his hardware. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: How to pass I2C platform_data under ACPI
On Thu, Apr 03, 2014 at 08:10:40AM +, Pallala, Ramakrishna wrote:
> Hi All,
>
> I am trying to enable a i2c client driver under ACPI. The device is being
> enumerated behind adapter device and I am getting IRQ resource as well.
>
> The problem I have now is, how do I pass the platform data to driver?
>
> struct i2c_board_info {
> chartype[I2C_NAME_SIZE];
> unsigned short flags;
> unsigned short addr;
> void*platform_data; ===> how can I initialize
> this filed.
> struct dev_archdata *archdata;
> struct device_node *of_node;
> struct acpi_dev_node acpi_node;
> int irq;
> };
>
> In non ACPI environment I used to initialize the platform_data under
> board or platforms files. Under ACPI how do I do that?
If you can't extract that information from ACPI namespace, then one option
is to pass platform data along with the device ACPI ID:
static const struct acpi_device_id my_acpi_match[] = {
{ "MYID0001", (kernel_ulong_t)&my_platform_data }
...
{ },
};
static struct i2c_driver i2c_hid_driver = {
.driver = {
...
.acpi_match_table = ACPI_PTR(my_acpi_match),
},
...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: How to pass I2C platform_data under ACPI
On Thu, Apr 03, 2014 at 11:25:34AM +, Pallala, Ramakrishna wrote:
> >> In non ACPI environment I used to initialize the platform_data under
> >> board or platforms files. Under ACPI how do I do that?
> >
> >If you can't extract that information from ACPI namespace, then one option
> >is to pass platform data along with the device ACPI ID:
> >
> >static const struct acpi_device_id my_acpi_match[] = {
> > { "MYID0001", (kernel_ulong_t)&my_platform_data }
> > ...
> > { },
> >};
>
> Thanks for the Quick reply.
>
> So If I want to use different platform_data for different boards can I
> do something like below?
Exactly.
> And initialize the platform data in either driver or in separate module
> which gets compiled along with driver?
Typically it has been done in the same driver but I don't see any problems
having a separate module as well.
> static const struct acpi_device_id my_acpi_match[] = {
> { "MYID0001", (kernel_ulong_t)&my_platform_data1 }
> { "MYID0002", (kernel_ulong_t)&my_platform_data2 }
> ...
> { },
>
> Thanks,
> Ram
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Linaro-acpi] How to pass I2C platform_data under ACPI
On Thu, Apr 03, 2014 at 03:34:30PM +0200, Arnd Bergmann wrote:
> On Thursday 03 April 2014 14:38:33 mika.westerb...@linux.intel.com wrote:
> > On Thu, Apr 03, 2014 at 11:25:34AM +, Pallala, Ramakrishna wrote:
> > > >> In non ACPI environment I used to initialize the platform_data under
> > > >> board or platforms files. Under ACPI how do I do that?
> > > >
> > > >If you can't extract that information from ACPI namespace, then one
> > > >option is to pass platform data along with the device ACPI ID:
> > > >
> > > >static const struct acpi_device_id my_acpi_match[] = {
> > > > { "MYID0001", (kernel_ulong_t)&my_platform_data }
> > > > ...
> > > > { },
> > > >};
> > >
> > > Thanks for the Quick reply.
> > >
> > > So If I want to use different platform_data for different boards can I
> > > do something like below?
> >
> > Exactly.
> >
> > > And initialize the platform data in either driver or in separate module
> > > which gets compiled along with driver?
> >
> > Typically it has been done in the same driver but I don't see any problems
> > having a separate module as well.
> >
> > > static const struct acpi_device_id my_acpi_match[] = {
> > > { "MYID0001", (kernel_ulong_t)&my_platform_data1 }
> > > { "MYID0002", (kernel_ulong_t)&my_platform_data2 }
> > > ...
> > > { },
>
> We definitely don't want per-board match entries, that does not scale.
> The driver should be reasonably generic and get all the necessary data
> out of well-defined tables. You can have different IDs when there
> are only a few cases that are actually relevant, but it has to be
> conceivable that the same driver get used on future hardware without
> changes.
Yes, I meant that when the platform data information is not available in
ACPI namespace, then (and only then) pass the data by means of different
IDs.
Preferably this information is included in the ACPI namespace.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V4] acpi:apd:add AMD ACPI2Platform device support for x86 system.
On Thu, Feb 05, 2015 at 11:07:17AM +0800, Ken Xue wrote: > >From d22789f089ee644413a144633f368f45cb0ac9d8 Mon Sep 17 00:00:00 2001 > From: Ken Xue > Date: Thu, 5 Feb 2015 11:04:44 +0800 > Subject: [PATCH] acpi:apd:add AMD ACPI2Platform device support for x86 system. > > This new feature is to interpret AMD specific ACPI device to > platform device such as I2C, UART found on AMD CZ and later chipsets. It > based on example intel LPSS. Now, it can support AMD I2C & UART. You are missing Signed-off-by here. Anyway looks good to me, Acked-by: Mika Westerberg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V4] acpi:apd:add AMD ACPI2Platform device support for x86 system.
On Fri, Feb 06, 2015 at 08:34:52AM +0800, Ken Xue wrote: > >From 1a6a3a5c0815cb1f52ec0a2b9601edfa9bfebe81 Mon Sep 17 00:00:00 2001 > From: Ken Xue > Date: Fri, 6 Feb 2015 08:27:51 +0800 > Subject: [PATCH] acpi:apd:add AMD ACPI2Platform device support for x86 system. > > This new feature is to interpret AMD specific ACPI device to > platform device such as I2C, UART, GPIO found on AMD CZ and > later chipsets. It based on example intel LPSS. Now, it can > support AMD I2C, UART and GPIO. > > Signed-off-by: Ken Xue Acked-by: Mika Westerberg -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi:apd:add AMD ACPI2Platform device support for x86 system.
On Mon, Feb 02, 2015 at 05:50:52PM +0800, Ken Xue wrote:
> >From b9654ecbfaebde00aee746a024eec9fe8de24b97 Mon Sep 17 00:00:00 2001
> From: Ken Xue
> Date: Mon, 2 Feb 2015 17:32:24 +0800
> Subject: [PATCH] This new feature is to interpret AMD specific ACPI device to
> platform device such as I2C, UART found on AMD CZ and later chipsets. It
> based on example INTEL LPSS. Now, it can support AMD I2C & UART.
Looks good to me. There are few smallish issues still, see below.
> Signed-off-by: Ken Xue
> ---
> arch/x86/Kconfig| 11 +++
> drivers/acpi/Makefile | 2 +-
> drivers/acpi/acpi_apd.c | 208
>
> drivers/acpi/internal.h | 2 +
> drivers/acpi/scan.c | 1 +
> 5 files changed, 223 insertions(+), 1 deletion(-)
> create mode 100644 drivers/acpi/acpi_apd.c
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 0dc9d01..ddf8d42 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -496,6 +496,17 @@ config X86_INTEL_LPSS
> things like clock tree (common clock framework) and pincontrol
> which are needed by the LPSS peripheral drivers.
>
> +config X86_AMD_PLATFORM_DEVICE
> + bool "AMD ACPI2Platform devices support"
> + depends on ACPI
> + select COMMON_CLK
> + select PINCTRL
> + ---help---
> + Select to interpret AMD specific ACPI device to platform device
> + such as I2C, UART found on AMD Carrizo and later chipsets. Selecting
> + this option enables things like clock tree (common clock framework)
> + and pinctrl.
> +
> config IOSF_MBI
> tristate "Intel SoC IOSF Sideband support for SoC platforms"
> depends on PCI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index f74317c..0071141 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -40,7 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
> acpi-y += ec.o
> acpi-$(CONFIG_ACPI_DOCK) += dock.o
> acpi-y += pci_root.o pci_link.o pci_irq.o
> -acpi-y += acpi_lpss.o
> +acpi-y += acpi_lpss.o acpi_apd.o
> acpi-y += acpi_platform.o
> acpi-y += acpi_pnp.o
> acpi-y += int340x_thermal.o
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> new file mode 100644
> index 000..b875ef6
> --- /dev/null
> +++ b/drivers/acpi/acpi_apd.c
> @@ -0,0 +1,208 @@
> +/*
> + * AMD ACPI support for ACPI2platform device.
> + *
> + * Copyright (c) 2014,2015 AMD Corporation.
> + * Authors: Ken Xue
> + * Wu, Jeff
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
Empty line here.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "internal.h"
> +
> +
Delete this empty line.
> +ACPI_MODULE_NAME("acpi_apd");
> +struct apd_private_data;
> +
> +/**
> + * device flags of acpi_apd_dev_desc.
> + * ACPI_APD_SYSFS : add device attributes in sysfs
> + * ACPI_APD_PM : attach power domain to device
> + * ACPI_APD_PM_ON : power on device when attach power domain
> + */
> +#define ACPI_APD_SYSFS BIT(0)
> +#define ACPI_APD_PM BIT(1)
> +#define ACPI_APD_PM_ON BIT(2)
> +
> +/**
> + * struct apd_device_desc - a descriptor for apd device
> + * @flags: device flags like ACPI_APD_SYSFS ACPI_APD_PM ACPI_APD_PM_ON
> + * @fixed_clk_rate: fixed rate input clock source for acpi device;
> + * 0 means no fixed rate input clock source
> + * @setup: a hook routine to set device resource during create platform
> device
> + *
> + * device description defined as acpi_device_id.driver_data
> + */
> +struct apd_device_desc {
> + unsigned int flags;
> + unsigned int fixed_clk_rate;
> + int (*setup)(struct apd_private_data *pdata);
> +};
> +
> +struct apd_private_data {
> + struct clk *clk;
> + struct acpi_device *adev;
> + struct apd_device_desc *dev_desc;
> +};
> +
> +
Ditto.
> +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> +#define APD_ADDR(desc) ((unsigned long)&desc)
> +
> +static int acpi_apd_setup(struct apd_private_data *pdata)
> +{
> + struct apd_device_desc *dev_desc = pdata->dev_desc;
> + struct clk *clk = ERR_PTR(-ENODEV);
> +
> + if (dev_desc->fixed_clk_rate) {
> + clk = clk_register_fixed_rate(&pdata->adev->dev,
> + dev_name(&pdata->adev->dev),
> + NULL, CLK_IS_ROOT,
> + dev_desc->fixed_clk_rate);
> + clk_register_clkdev(clk, NULL, dev_name(&pdata->adev->dev));
> + pdata->clk = clk;
> + }
> +
> + return 0;
> +}
> +
> +static
Re: [PATCH V2] acpi:apd:add AMD ACPI2Platform device support for x86 system.
On Tue, Feb 03, 2015 at 09:04:55AM +0800, Ken Xue wrote: > as you said, platform_drv_probe calls dev_pm_domain_attach(). but > platform_drv_probe just is a default probe routine. Not all platform > device drivers use this probe routine. so, codes here may be still > necessary. Are you saying that for platform devices there is some other path to get a driver probed, other than platform_drv_probe()? Can you point me to it? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi:apd:add AMD ACPI2Platform device support for x86 system.
On Tue, Feb 03, 2015 at 05:55:25PM +0800, Ken Xue wrote: > On Tue, 2015-02-03 at 11:53 +0200, mika.westerb...@linux.intel.com > wrote: > > On Tue, Feb 03, 2015 at 09:04:55AM +0800, Ken Xue wrote: > > > as you said, platform_drv_probe calls dev_pm_domain_attach(). but > > > platform_drv_probe just is a default probe routine. Not all platform > > > device drivers use this probe routine. so, codes here may be still > > > necessary. > > > > Are you saying that for platform devices there is some other path to get > > a driver probed, other than platform_drv_probe()? Can you point me to > > it? > >From the codes, i can see there is a possibility that drv->driver.probe > may not be set in __platform_driver_register. But i really can not point > out a use case that platform device driver without probe. > so, it is safe to remove "dev_pm_domain_attach" in > "acpi_apd_platform_notify". right? In that case, I think so, yes. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 2/9] Input: goodix - reset device at init
On Fri, Oct 30, 2015 at 09:33:28AM -0700, Dmitry Torokhov wrote:
> cpi_dev_add_driver_gpios() does not really help with generic drivers
> (unless we keep adding more and more board specific data to them). How
> about we keep track of names used and only allow conversion for the
> first name used, like in the patch below?
That works but it misses one case: When you have optional GPIOs and not
all of them are provided. Currently it ends up the same situation
returning the same GPIO.
I've commented below how we could handle that case as well.
> --
> Dmitry
>
> gpiolib: tighten up ACPI legacy gpio lookups
>
> From: Dmitry Torokhov
>
> We should not fall back to the legacy unnamed gpio lookup style if the
> driver requests gpios with different names, because we'll give out the same
> gpio twice. Let's keep track of the names that were used for the device and
> only do the fallback for the first name used.
>
> Signed-off-by: Dmitry Torokhov
> ---
> drivers/gpio/gpiolib.c | 42 +-
> 1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5db3445..4ae5447 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1738,6 +1738,45 @@ static struct gpio_desc *of_find_gpio(struct device
> *dev, const char *con_id,
> return desc;
> }
>
> +struct acpi_gpio_lookup {
> + struct list_head node;
> + struct device *dev;
> + const char *con_id;
> +};
> +
> +static DEFINE_MUTEX(acpi_gpio_lookup_lock);
> +static LIST_HEAD(acpi_gpio_lookup_list);
> +
> +static bool acpi_can_fallback_crs(struct device *dev, const char *con_id)
> +{
> + struct acpi_gpio_lookup *l, *lookup = NULL;
I'm thinking if we here do
struct acpi_device *adev = ACPI_COMPANION(dev);
/* Never fallback if the device has properties */
if (adev->data.properties || adev->driver_gpios)
return false;
> +
> + mutex_lock(&acpi_gpio_lookup_lock);
> +
> + list_for_each_entry(l, &acpi_gpio_lookup_list, node) {
> + if (l->dev == dev) {
> + lookup = l;
> + break;
> + }
> + }
> +
> + if (!lookup) {
> + lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
> + if (lookup) {
> + lookup->dev = dev;
> + lookup->con_id = con_id;
> + list_add_tail(&lookup->node, &acpi_gpio_lookup_list);
> + }
> + }
> +
> + mutex_lock(&acpi_gpio_lookup_lock);
> +
> + return lookup &&
> + ((!lookup->con_id && !con_id) ||
> + (lookup->con_id && con_id &&
> + strcmp(lookup->con_id, con_id) == 0));
> +}
> +
> static struct gpio_desc *acpi_find_gpio(struct device *dev, const char
> *con_id,
> unsigned int idx,
> enum gpio_lookup_flags *flags)
> @@ -1765,7 +1804,8 @@ static struct gpio_desc *acpi_find_gpio(struct device
> *dev, const char *con_id,
>
> /* Then from plain _CRS GPIOs */
> if (IS_ERR(desc)) {
> - desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
> + if (acpi_can_fallback_crs(dev, con_id))
> + desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
and here we could do
if (!acpi_can_fallback_crs(dev, con_id))
return ERR_PTR(-ENOENT);
desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
So instead return -ENOENT so that gpiod_get_index_optional() handles the
missing optional GPIO properly.
> if (IS_ERR(desc))
> return desc;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 0/6] Patch series to support Thunderbolt without any BIOS support
On Fri, Jul 26, 2019 at 12:52:58PM +, Nicholas Johnson wrote: > Patch series rebased to 5.3-rc1. > > If possible, please have a quick read over while I am away (2019-07-27 > to mid 2019-08-04), so I can fix any mistakes or make simple changes to > get problems out of the way for a more thorough examination later. > > Thanks! > > Kind regards, > Nicholas > > Nicholas Johnson (6): > PCI: Consider alignment of hot-added bridges when distributing > available resources > PCI: In extend_bridge_window() change available to new_size > PCI: Change extend_bridge_window() to set resource size directly > PCI: Allow extend_bridge_window() to shrink resource if necessary > PCI: Add hp_mmio_size and hp_mmio_pref_size parameters > PCI: Fix bug resulting in double hpmemsize being assigned to MMIO > window Hi Bjorn, What's the status of this series? I don't see them in v5.4-rc1. Thanks!
Re: [PATCH v8 1/6] PCI: Consider alignment of hot-added bridges when distributing available resources
Hi Nicholas,
On Fri, Jul 26, 2019 at 12:53:19PM +, Nicholas Johnson wrote:
> Rewrite pci_bus_distribute_available_resources to better handle bridges
> with different resource alignment requirements. Pass more details
> arguments recursively to track the resource start and end addresses
> relative to the initial hotplug bridge. This is especially useful for
> Thunderbolt with native PCI enumeration, enabling external graphics
> cards and other devices with bridge alignment higher than 1MB.
>
> Change extend_bridge_window to resize the actual resource, rather than
> using add_list and dev_res->add_size. If an additional resource entry
> exists for the given resource, zero out the add_size field to avoid it
> interfering. Because add_size is considered optional when allocating,
> using add_size could cause issues in some cases, because successful
> resource distribution requires sizes to be guaranteed. Such cases
> include hot-adding nested hotplug bridges in one enumeration, and
> potentially others which are yet to be encountered.
>
> Solves bug report: https://bugzilla.kernel.org/show_bug.cgi?id=199581
Here better to use:
Link: https://bugzilla.kernel.org/show_bug.cgi?id=199581
> Reported-by: Mika Westerberg
This solves the issue I reported so,
Tested-by: Mika Westerberg
There are a couple of comments below.
> Signed-off-by: Nicholas Johnson
> ---
> drivers/pci/setup-bus.c | 148 +++-
> 1 file changed, 71 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 79b1fa651..6835fd64c 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1840,12 +1840,10 @@ static void extend_bridge_window(struct pci_dev
> *bridge, struct resource *res,
> }
>
> static void pci_bus_distribute_available_resources(struct pci_bus *bus,
> - struct list_head *add_list,
> - resource_size_t available_io,
> - resource_size_t available_mmio,
> - resource_size_t available_mmio_pref)
> + struct list_head *add_list, struct resource io,
> + struct resource mmio, struct resource mmio_pref)
You pass a copy of each resource because you modify it inplace. I wonder
if it makes more sense to explicitly take a copy here with comments?
> {
> - resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
> + resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
> unsigned int normal_bridges = 0, hotplug_bridges = 0;
> struct resource *io_res, *mmio_res, *mmio_pref_res;
> struct pci_dev *dev, *bridge = bus->self;
> @@ -1855,15 +1853,29 @@ static void
> pci_bus_distribute_available_resources(struct pci_bus *bus,
> mmio_pref_res = &bridge->resource[PCI_BRIDGE_RESOURCES + 2];
>
> /*
> - * Update additional resource list (add_list) to fill all the
> - * extra resource space available for this port except the space
> - * calculated in __pci_bus_size_bridges() which covers all the
> - * devices currently connected to the port and below.
> + * The alignment of this bridge is yet to be considered, hence it must
> + * be done now before extending its bridge window.
>*/
> - extend_bridge_window(bridge, io_res, add_list, available_io);
> - extend_bridge_window(bridge, mmio_res, add_list, available_mmio);
> + align = pci_resource_alignment(bridge, io_res);
> + if (!io_res->parent && align)
> + io.start = ALIGN(io.start, align);
> +
> + align = pci_resource_alignment(bridge, mmio_res);
> + if (!mmio_res->parent && align)
> + mmio.start = ALIGN(mmio.start, align);
> +
> + align = pci_resource_alignment(bridge, mmio_pref_res);
> + if (!mmio_pref_res->parent && align)
> + mmio_pref.start = ALIGN(mmio_pref.start, align);
> +
> + /*
> + * Update the resources to fill as much remaining resource space in the
> + * parent bridge as possible, while considering alignment.
> + */
> + extend_bridge_window(bridge, io_res, add_list, resource_size(&io));
> + extend_bridge_window(bridge, mmio_res, add_list, resource_size(&mmio));
> extend_bridge_window(bridge, mmio_pref_res, add_list,
> - available_mmio_pref);
> + resource_size(&mmio_pref));
I think this should be aligned like:
extend_bridge_window(bridge, mmio_pref_res, add_list,
resource_size(&mmio_pref));
>
> /*
>* Calculate how many hotplug bridges and normal bridges there
> @@ -1884,108 +1896,90 @@ static void
> pci_bus_distribute_available_resources(struct pci_bus *bus,
>*/
> if (hotplug_bridges + normal_bridges == 1) {
> dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
> - if (dev->
Re: [PATCH v8 2/6] PCI: In extend_bridge_window() change available to new_size
On Fri, Jul 26, 2019 at 12:53:41PM +, Nicholas Johnson wrote: > In extend_bridge_window() change "available" parameter name to new_size. > This makes more sense as this parameter represents the new size for the > window. > > Signed-off-by: Nicholas Johnson Reviewed-by: Mika Westerberg
Re: [PATCH v8 3/6] PCI: Change extend_bridge_window() to set resource size directly
On Fri, Jul 26, 2019 at 12:54:01PM +, Nicholas Johnson wrote: > Change extend_bridge_window() to set resource size directly instead of > using additional resource lists. > > Because additional resource lists are optional resources, any algorithm > that requires guaranteed allocation that uses them cannot be guaranteed > to work. > > Remove the resource from add_list. If it is set to zero size and left, > it can cause significant problems when it comes to assigning resources. It would be good if you would open bit more what kind of significant problems this may result. > Signed-off-by: Nicholas Johnson Reviewed-by: Mika Westerberg
Re: [PATCH v8 4/6] PCI: Allow extend_bridge_window() to shrink resource if necessary
On Fri, Jul 26, 2019 at 12:54:22PM +, Nicholas Johnson wrote: > Remove checks for resource size in extend_bridge_window(). This is > necessary to allow the pci_bus_distribute_available_resources() to > function when the kernel parameter pci=hpmemsize=nn[KMG] is used to > allocate resources. Because the kernel parameter sets the size of all > hotplug bridges to be the same, there are problems when nested hotplug > bridges are encountered. Fitting a downstream hotplug bridge with size X > and normal bridges with size Y into parent hotplug bridge with size X is > impossible, and hence the downstream hotplug bridge needs to shrink to > fit into its parent. Maybe you could show the topology here which needs shrinking. > Add check for if bridge is extended or shrunken and adjust pci_dbg to > reflect this. > > Reset the resource if its new size is zero (if we have run out of a > bridge window resource). If it is set to zero size and left, it can > cause significant problems when it comes to enabling devices. Same comment here about explaining the "significant problems". > > Signed-off-by: Nicholas Johnson > --- > drivers/pci/setup-bus.c | 16 +++- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > index a072781ab..7e1dc892a 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -1823,13 +1823,19 @@ static void extend_bridge_window(struct pci_dev > *bridge, struct resource *res, Since it is also shrinking now maybe name it adjust_bridge_window() instead?
Re: [PATCH v8 5/6] PCI: Add hp_mmio_size and hp_mmio_pref_size parameters
On Fri, Jul 26, 2019 at 12:54:44PM +, Nicholas Johnson wrote:
> Add kernel parameter pci=hpmmiosize=nn[KMG] to set MMIO bridge window
> size for hotplug bridges.
>
> Add kernel parameter pci=hpmmioprefsize=nn[KMG] to set MMIO_PREF bridge
> window size for hotplug bridges.
>
> Leave pci=hpmemsize=nn[KMG] unchanged, to prevent disruptions to
> existing users. This sets both MMIO and MMIO_PREF to the same size.
>
> The two new parameters conform to the style of pci=hpiosize=nn[KMG].
>
> Signed-off-by: Nicholas Johnson
This does not apply anymore because of 003d3b2c5f83 ("PCI: Make
pci_hotplug_io_size, mem_size, and bus_size private") so I think you
need to rebase this on top of current mainline.
> ---
> .../admin-guide/kernel-parameters.txt | 9 ++-
> drivers/pci/pci.c | 17 ++---
> drivers/pci/setup-bus.c | 25 +++
> include/linux/pci.h | 3 ++-
> 4 files changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index 46b826fcb..9bc54cb99 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3467,8 +3467,15 @@
> hpiosize=nn[KMG]The fixed amount of bus space which is
> reserved for hotplug bridge's IO window.
> Default size is 256 bytes.
> + hpmmiosize=nn[KMG] The fixed amount of bus space which is
> + reserved for hotplug bridge's MMIO window.
> + Default size is 2 megabytes.
> + hpmmioprefsize=nn[KMG] The fixed amount of bus space which is
> + reserved for hotplug bridge's MMIO_PREF window.
> + Default size is 2 megabytes.
> hpmemsize=nn[KMG] The fixed amount of bus space which is
> - reserved for hotplug bridge's memory window.
> + reserved for hotplug bridge's MMIO and
> + MMIO_PREF window.
> Default size is 2 megabytes.
> hpbussize=nnThe minimum amount of additional bus numbers
> reserved for buses below a hotplug bridge.
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 29ed5ec1a..6b3857cad 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -85,10 +85,12 @@ unsigned long pci_cardbus_io_size =
> DEFAULT_CARDBUS_IO_SIZE;
> unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
>
> #define DEFAULT_HOTPLUG_IO_SIZE (256)
> -#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
> +#define DEFAULT_HOTPLUG_MMIO_SIZE(2*1024*1024)
> +#define DEFAULT_HOTPLUG_MMIO_PREF_SIZE (2*1024*1024)
> /* pci=hpmemsize=nnM,hpiosize=nn can override this */
> unsigned long pci_hotplug_io_size = DEFAULT_HOTPLUG_IO_SIZE;
> -unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
> +unsigned long pci_hotplug_mmio_size = DEFAULT_HOTPLUG_MMIO_SIZE;
> +unsigned long pci_hotplug_mmio_pref_size = DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
>
> #define DEFAULT_HOTPLUG_BUS_SIZE 1
> unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
> @@ -6281,8 +6283,17 @@ static int __init pci_setup(char *str)
> pcie_ecrc_get_policy(str + 5);
> } else if (!strncmp(str, "hpiosize=", 9)) {
> pci_hotplug_io_size = memparse(str + 9, &str);
> + } else if (!strncmp(str, "hpmmiosize=", 11)) {
> + pci_hotplug_mmio_size =
> + memparse(str + 11, &str);
I would keep this in single line disregarding the 80 char limit.
> + } else if (!strncmp(str, "hpmmioprefsize=", 15)) {
> + pci_hotplug_mmio_pref_size =
> + memparse(str + 15, &str);
Ditto
> } else if (!strncmp(str, "hpmemsize=", 10)) {
> - pci_hotplug_mem_size = memparse(str + 10, &str);
> + pci_hotplug_mmio_size =
> + memparse(str + 10, &str);
Ditto.
> + pci_hotplug_mmio_pref_size =
> + memparse(str + 10, &str);
Ditto.
> } else if (!strncmp(str, "hpbussize=", 10)) {
> pci_hotplug_bus_size =
> simple_strtoul(str + 10, &str, 0);
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 7e1dc892a..345ecf16d 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1178,7 +1178,8 @@ void __pci_bus_size
Re: [PATCH v8 6/6] PCI: Fix bug resulting in double hpmemsize being assigned to MMIO window
On Fri, Jul 26, 2019 at 12:55:03PM +, Nicholas Johnson wrote: > Background > == I don't think the above are needed. > Currently, the kernel can sometimes assign the MMIO_PREF window > additional size into the MMIO window, resulting in double the MMIO > additional size, even if the MMIO_PREF window was successful. > > This happens if in the first pass, the MMIO_PREF succeeds but the MMIO > fails. In the next pass, because MMIO_PREF is already assigned, the > attempt to assign MMIO_PREF returns an error code instead of success > (nothing more to do, already allocated). > > Example of problem (more context can be found in the bug report URL): Maybe add bit more context in the changelog. Also explain how the problem can be reproduced. > Mainline kernel: > pci :06:01.0: BAR 14: assigned [mem 0x9010-0xa00f] = 256M > pci :06:04.0: BAR 14: assigned [mem 0xa020-0xb01f] = 256M > > Patched kernel: > pci :06:01.0: BAR 14: assigned [mem 0x9010-0x980f] = 128M > pci :06:04.0: BAR 14: assigned [mem 0x9820-0xa01f] = 128M > > This was using pci=realloc,hpmemsize=128M,nocrs - on the same machine > with the same configuration, with a Ubuntu mainline kernel and a kernel > patched with this patch series. > > This patch is vital for the next patch in the series. The next patch There is no next patch in the patch series ;-) > allows the user to specify MMIO and MMIO_PREF independently. If the > MMIO_PREF is set to be very large, this bug will end up more than > doubling the MMIO size. The bug results in the MMIO_PREF being added to > the MMIO window, which means doubling if MMIO_PREF size == MMIO size. > With a large MMIO_PREF, without this patch, the MMIO window will likely > fail to be assigned altogether due to lack of 32-bit address space. > > Patch notes > == Here also the above two lines are not needed. > Change find_free_bus_resource() to not skip assigned resources with > non-null parent. > > Add checks in pbus_size_io() and pbus_size_mem() to return success if > resource returned from find_free_bus_resource() is already allocated. > > This avoids pbus_size_io() and pbus_size_mem() returning error code to > __pci_bus_size_bridges() when a resource has been successfully assigned > in a previous pass. This fixes the existing behaviour where space for a > resource could be reserved multiple times in different parent bridge > windows. This also greatly reduces the number of failed BAR messages in > dmesg when Linux assigns resources. > > See related from Logan Gunthorpe (same problem, different solution): > https://lore.kernel.org/lkml/20190531171216.20532-2-log...@deltatee.com/T/#u Link: https://lore.kernel.org/lkml/20190531171216.20532-2-log...@deltatee.com/T/#u > Solves bug report: https://bugzilla.kernel.org/show_bug.cgi?id=203243 Link: https://bugzilla.kernel.org/show_bug.cgi?id=203243 The patch itself looks good to me.
Re: [PATCH v8 0/6] Patch series to support Thunderbolt without any BIOS support
On Fri, Oct 04, 2019 at 08:08:03AM -0500, Bjorn Helgaas wrote: > On Thu, Oct 03, 2019 at 03:19:46PM +0300, mika.westerb...@linux.intel.com > wrote: > > On Fri, Jul 26, 2019 at 12:52:58PM +, Nicholas Johnson wrote: > > > Patch series rebased to 5.3-rc1. > > > > > > If possible, please have a quick read over while I am away (2019-07-27 > > > to mid 2019-08-04), so I can fix any mistakes or make simple changes to > > > get problems out of the way for a more thorough examination later. > > > > > > Thanks! > > > > > > Kind regards, > > > Nicholas > > > > > > Nicholas Johnson (6): > > > PCI: Consider alignment of hot-added bridges when distributing > > > available resources > > > PCI: In extend_bridge_window() change available to new_size > > > PCI: Change extend_bridge_window() to set resource size directly > > > PCI: Allow extend_bridge_window() to shrink resource if necessary > > > PCI: Add hp_mmio_size and hp_mmio_pref_size parameters > > > PCI: Fix bug resulting in double hpmemsize being assigned to MMIO > > > window > > > > Hi Bjorn, > > > > What's the status of this series? I don't see them in v5.4-rc1. > > They're still on my to-do list but are currently languishing because > they touch critical but complicated code that I don't understand and > nobody else has chimed in to help review them. Testing reports would > also be helpful. I will test this next week as it solves one issue I reported some time ago. I can also help you to review this, at least parts touching extend_bridge_window() and pci_bus_distribute_available_resources(), because those functions were added by me ;-)
Re: [PATCH v8 4/6] PCI: Allow extend_bridge_window() to shrink resource if necessary
On Wed, Oct 23, 2019 at 09:16:10AM +, Nicholas Johnson wrote: > On Tue, Oct 08, 2019 at 03:09:07PM +0300, mika.westerb...@linux.intel.com > wrote: > > On Fri, Jul 26, 2019 at 12:54:22PM +, Nicholas Johnson wrote: > > > Remove checks for resource size in extend_bridge_window(). This is > > > necessary to allow the pci_bus_distribute_available_resources() to > > > function when the kernel parameter pci=hpmemsize=nn[KMG] is used to > > > allocate resources. Because the kernel parameter sets the size of all > > > hotplug bridges to be the same, there are problems when nested hotplug > > > bridges are encountered. Fitting a downstream hotplug bridge with size X > > > and normal bridges with size Y into parent hotplug bridge with size X is > > > impossible, and hence the downstream hotplug bridge needs to shrink to > > > fit into its parent. > > > > Maybe you could show the topology here which needs shrinking. > > > > > Add check for if bridge is extended or shrunken and adjust pci_dbg to > > > reflect this. > > > > > > Reset the resource if its new size is zero (if we have run out of a > > > bridge window resource). If it is set to zero size and left, it can > > > cause significant problems when it comes to enabling devices. > > > > Same comment here about explaining the "significant problems". > I have in the past, but because the problems are very hard to describe > succinctly, it just turns into a > nightmare. I can try to do it again. > > > > > > > Signed-off-by: Nicholas Johnson > > > > > > --- > > > drivers/pci/setup-bus.c | 16 +++- > > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > > index a072781ab..7e1dc892a 100644 > > > --- a/drivers/pci/setup-bus.c > > > +++ b/drivers/pci/setup-bus.c > > > @@ -1823,13 +1823,19 @@ static void extend_bridge_window(struct pci_dev > > > *bridge, struct resource *res, > > > > Since it is also shrinking now maybe name it adjust_bridge_window() instead? > I am happy to do this. > > If we can drop the pci_dbg() calls, then I might be able to drop this > function entirely. During the development of this patch, that is exactly > what I did. How important are the pci_dbg calls to you? Well they are still useful when debugging resource allocation issues (and they match similar we do when extending number of buses). I would like to keep them if possible.
Re: [PATCH 0/1] Add support for setting MMIO PREF hotplug bridge size
On Wed, Oct 23, 2019 at 08:36:59AM +, Nicholas Johnson wrote: > This patch adds support for two new kernel parameters. This patch has > been in the making for quite some time, and has changed several times > based on feedback. > > I realised I was making the mistake of putting it as part of my > Thunderbolt patch series. Although the other patches in the series are > very important for my goal, I realised that they are just a heap of > patches that are not Thunderbolt-specific. The only thing that is > Thunderbolt-related is the intended use case. > > I hope that posting this alone can ease the difficulty of reviewing it. > > Nicholas Johnson (1): > PCI: Add hp_mmio_size and hp_mmio_pref_size parameters > > .../admin-guide/kernel-parameters.txt | 9 ++- > drivers/pci/pci.c | 17 ++--- > drivers/pci/pci.h | 3 ++- > drivers/pci/setup-bus.c | 25 +++ > 4 files changed, 38 insertions(+), 16 deletions(-) If you want to add cover letter in the "normal way" so that threading is preserved you can do something like 'git format-patch --cover-letter ...', then edit the -...patch and send it along with the other patches using git send-email.
Re: [PATCH 1/1] PCI: Add hp_mmio_size and hp_mmio_pref_size parameters
On Wed, Oct 23, 2019 at 08:37:48AM +, Nicholas Johnson wrote:
> } else if (!strncmp(str, "hpmemsize=", 10)) {
> - pci_hotplug_mem_size = memparse(str + 10, &str);
> + pci_hotplug_mmio_size =
> + memparse(str + 10, &str);
> + pci_hotplug_mmio_pref_size =
> + memparse(str + 10, &str);
Does this actually work correctly? The first memparse(str + 10, &str)
modifies str so the next call will not start from the correct position
anymore.
Otherwise the patch looks good to me.
Re: [PATCH 1/1] PCI: Add hp_mmio_size and hp_mmio_pref_size parameters
On Wed, Oct 23, 2019 at 09:57:17AM +, Nicholas Johnson wrote:
> On Wed, Oct 23, 2019 at 12:47:43PM +0300, mika.westerb...@linux.intel.com
> wrote:
> > On Wed, Oct 23, 2019 at 08:37:48AM +, Nicholas Johnson wrote:
> > > } else if (!strncmp(str, "hpmemsize=", 10)) {
> > > - pci_hotplug_mem_size = memparse(str + 10, &str);
> > > + pci_hotplug_mmio_size =
> > > + memparse(str + 10, &str);
> > > + pci_hotplug_mmio_pref_size =
> > > + memparse(str + 10, &str);
> >
> > Does this actually work correctly? The first memparse(str + 10, &str)
> > modifies str so the next call will not start from the correct position
> > anymore.
> I have been using this for a long time now and have not had any issues.
> Does it modify str? I thought that was done by the loop.
If you add "hpmemsize=xxx" in the command line and print both
pci_hotplug_mmio_size and pci_hotplug_mmio_pref_size after the
assignment, do they have the same value? If yes, then there is no
problem.
> Can somebody else please weigh in here? I am worried now, and I want to
> be sure. If it is a problem, then I will have to read it into
> pci_hotplug_mmio_size and then set:
>
> pci_hotplug_mmio_pref_size = pci_hotplug_mmio_size
Yup.
Re: [PATCH 1/1] PCI: Add hp_mmio_size and hp_mmio_pref_size parameters
On Wed, Oct 23, 2019 at 10:33:42AM +, Nicholas Johnson wrote:
> On Wed, Oct 23, 2019 at 01:03:59PM +0300, mika.westerb...@linux.intel.com
> wrote:
> > On Wed, Oct 23, 2019 at 09:57:17AM +, Nicholas Johnson wrote:
> > > On Wed, Oct 23, 2019 at 12:47:43PM +0300, mika.westerb...@linux.intel.com
> > > wrote:
> > > > On Wed, Oct 23, 2019 at 08:37:48AM +, Nicholas Johnson wrote:
> > > > > } else if (!strncmp(str, "hpmemsize=", 10)) {
> > > > > - pci_hotplug_mem_size = memparse(str +
> > > > > 10, &str);
> > > > > + pci_hotplug_mmio_size =
> > > > > + memparse(str + 10, &str);
> > > > > + pci_hotplug_mmio_pref_size =
> > > > > + memparse(str + 10, &str);
> > > >
> > > > Does this actually work correctly? The first memparse(str + 10, &str)
> > > > modifies str so the next call will not start from the correct position
> > > > anymore.
> > > I have been using this for a long time now and have not had any issues.
> > > Does it modify str? I thought that was done by the loop.
> >
> > If you add "hpmemsize=xxx" in the command line and print both
> > pci_hotplug_mmio_size and pci_hotplug_mmio_pref_size after the
> > assignment, do they have the same value? If yes, then there is no
> > problem.
> Looking at lib/cmdline.c line 125, it looks like there is no point in me
> testing it. It looks like you are right.
>
> What is the better fix?
>
> pci_hotplug_mmio_size = pci_hotplug_mmio_pref_size = memparse(str + 10, &str);
>
> ^ Could be too long, even if we are ignoring the 80-character limit.
I prefer this:
pci_hotplug_mmio_size = memparse(str + 10,
&str);
pci_hotplug_mmio_pref_size =
pci_hotplug_mmio_size;
And you can ignore the 80-char limit. The above is much more readable
IMHO.
Re: [PATCH v2 1/1] PCI: Add hp_mmio_size and hp_mmio_pref_size parameters
On Wed, Oct 23, 2019 at 12:12:29PM +, Nicholas Johnson wrote: > Add kernel parameter pci=hpmmiosize=nn[KMG] to set MMIO bridge window > size for hotplug bridges. > > Add kernel parameter pci=hpmmioprefsize=nn[KMG] to set MMIO_PREF bridge > window size for hotplug bridges. > > Leave pci=hpmemsize=nn[KMG] unchanged, to prevent disruptions to > existing users. This sets both MMIO and MMIO_PREF to the same size. > > The two new parameters conform to the style of pci=hpiosize=nn[KMG]. > > Signed-off-by: Nicholas Johnson Reviewed-by: Mika Westerberg
Re: [PATCH] PCI: Consider alignment of hot-added bridges when distributing available resources
Hi Nicholas, On Thu, Jan 31, 2019 at 09:51:05AM +, Nicholas Johnson wrote: > New systems with Thunderbolt are starting to use native PCI enumeration. > Mika Westerberg's patch "PCI: Distribute available resources to hotplug > capable PCIe downstream ports" > (https://patchwork.kernel.org/patch/9972155/) adds code to expand > downstream PCI hotplug bridges to consume all remaining resource space > in the parent bridge. It is a crucial patch for supporting Thunderbolt > native enumeration on Linux. > > However, it does not consider bridge alignment in all cases, which rules > out hot-adding an external graphics processor at the end of a > Thunderbolt daisy chain. Hot-adding such a device will likely fail to > work with the existing code. I think I tried to solve the exact issue some time ago, see: https://bugzilla.kernel.org/show_bug.cgi?id=199581 I verified with the same setup that with your patch applied the above problem is gone, so your patch solves that issue :) > It also might disrupt the operation of > existing devices or prevent the subsequent hot-plugging of lower aligned > devices if the kernel frees and reallocates upstream resources to > attempt assign the resources that failed to assign in the first pass. By > Intel's ruling, Thunderbolt external graphics processors are generally > meant to function only as the first and only device in the chain. > However, there is no technical reason that prevents it from being > possible if sufficient resources are available, and people are likely to > attempt such configurations with Thunderbolt devices if they own such > hardware. Hence, I argue that we should improve the user experience and > reduce the number of constraints placed on the user by always > considering resource alignment, which will make such configurations > possible. > > The other problem with the patch is that it is incompatible with > resources allocated by "pci=hpmemsize=nnM" due to a check which does not > allow for dev_res->add_size to be reduced. This check also makes a big > assumption that the hpmemsize is small but non-zero, and no action has > been taken to ensure that. In the current state, any bridge smaller than > hpmemsize will likely fail to size correctly, which will cause major > issues if the default were to change, or if the user also wants to > configure non-Thunderbolt hotplug bridges simultaneously. I argue that > if assumptions and limitations can be removed with no risks and adverse > effects, then it should be done. I fully agree. > The former problem is solved by rewriting the > pci_bus_distribute_available_resources() function with more information > passed in the arguments, eliminating assumptions about the initial > bridge alignment. My patch makes no assumptions about the alignment of > hot-added bridges, allowing for any device to be hot-added, given > sufficient resources are available. > > The latter problem is solved by removing the check preventing the > shrinking of dev_res->add_size, which allows for the distribution of > resources if hpmemsize is non-zero. It can be made to work with zero > hpmemsize with two-line patches in pbus_size_mem() and pbus_size_io(), > or by modifying extend_bridge_window() to add a new entry to the > additional resource list if one does not exist. In theory, and by my > testing, the removal of this check does not affect the functionality of > the resource distribution with firmware-allocated resources. But it does > enable the same functionality when using pci=hpmemsize=nnM, which was > not possible prior. This may be one piece of the puzzle needed to > support Thunderbolt add-in cards that support native PCI enumeration, > without any platform dependencies. > > I have tested this proposed patch extensively. Using Linux-allocated > resources, it works perfectly. I have two Gigabyte GC-TITAN RIDGE > Thunderbolt 3 add-in cards in my desktop, and a Dell XPS 9370 with the > Dell XPS 9380 Thunderbolt NVM40 firmware flashed. My peripherals are > three HP ZBook Thunderbolt 3 docks, two external graphics enclosures > with AMD Fiji XT in both, a Promise SANLink3 N1 (AQC107S), and a Dell > Thunderbolt 3 NVMe SSD. All configurations of these devices worked well, > and I can no longer make it fail if I try to. My testing with > firmware-allocated resources is limited due to using computers with > Alpine Ridge BIOS support. However, I did get manage to test the patch > with firmware-allocated resources by enabling the Alpine Ridge BIOS > support and forcing pcie_ports=native, and the results were perfect. > > Mika Westerberg has agreed to test this on an official platform with > native enumeration firmware support to be sure that it works in this > situation. It is also appropriate that he reviews these changes as he > wrote the original code and any changes made to Thunderbolt-critical > code cannot be allowed to break any of the base requirements for > Thunderbolt. I would like to thank him for putting up with my emai
Re: [PATCH] PCI: Consider alignment of hot-added bridges when distributing available resources
Hi, Please indicate in the $subject that this is second version of the patch as explained in: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format For example here the $subject should look like [PATCH v2] PCI: Consider alignment of hot-added bridges... You can use git format-patch --subject-prefix="PATCH v2" ... to generate such patch. On Sat, Feb 02, 2019 at 04:25:02PM +, Nicholas Johnson wrote: > New systems with Thunderbolt are starting to use native PCI enumeration. > Mika Westerberg's patch "PCI: Distribute available resources to hotplug > capable PCIe downstream ports" > (https://patchwork.kernel.org/patch/9972155/) adds code to expand > downstream PCI hotplug bridges to consume all remaining resource space > in the parent bridge. It is a crucial patch for supporting Thunderbolt > native enumeration on Linux. > > However, it does not consider bridge alignment in all cases, which rules > out hot-adding an external graphics processor at the end of a > Thunderbolt daisy chain. Hot-adding such a device will likely fail to > work with the existing code. It also might disrupt the operation of > existing devices or prevent the subsequent hot-plugging of lower aligned > devices if the kernel frees and reallocates upstream resources to > attempt assign the resources that failed to assign in the first pass. By > Intel's ruling, Thunderbolt external graphics processors are generally > meant to function only as the first and only device in the chain. > However, there is no technical reason that prevents it from being > possible if sufficient resources are available, and people are likely to > attempt such configurations with Thunderbolt devices if they own such > hardware. Hence, I argue that we should improve the user experience and > reduce the number of constraints placed on the user by always > considering resource alignment, which will make such configurations > possible. > > The other problem with the patch is that it is incompatible with > resources allocated by "pci=hpmemsize=nnM" due to a check which does not > allow for dev_res->add_size to be reduced. This check also makes a big > assumption that the hpmemsize is small but non-zero, and no action has > been taken to ensure that. In the current state, any bridge smaller than > hpmemsize will likely fail to size correctly, which will cause major > issues if the default were to change, or if the user also wants to > configure non-Thunderbolt hotplug bridges simultaneously. I argue that > if assumptions and limitations can be removed with no risks and adverse > effects, then it should be done. > > The former problem is solved by rewriting the > pci_bus_distribute_available_resources() function with more information > passed in the arguments, eliminating assumptions about the initial > bridge alignment. My patch makes no assumptions about the alignment of > hot-added bridges, allowing for any device to be hot-added, given > sufficient resources are available. > > The latter problem is solved by removing the check preventing the > shrinking of dev_res->add_size, which allows for the distribution of > resources if hpmemsize is non-zero. It can be made to work with zero > hpmemsize with two-line patches in pbus_size_mem() and pbus_size_io(), > or by modifying extend_bridge_window() to add a new entry to the > additional resource list if one does not exist. In theory, and by my > testing, the removal of this check does not affect the functionality of > the resource distribution with firmware-allocated resources. But it does > enable the same functionality when using pci=hpmemsize=nnM, which was > not possible prior. This may be one piece of the puzzle needed to > support Thunderbolt add-in cards that support native PCI enumeration, > without any platform dependencies. > > I have tested this proposed patch extensively. Using Linux-allocated > resources, it works perfectly. I have two Gigabyte GC-TITAN RIDGE > Thunderbolt 3 add-in cards in my desktop, and a Dell XPS 9370 with the > Dell XPS 9380 Thunderbolt NVM40 firmware flashed. My peripherals are > three HP ZBook Thunderbolt 3 docks, two external graphics enclosures > with AMD Fiji XT in both, a Promise SANLink3 N1 (AQC107S), and a Dell > Thunderbolt 3 NVMe SSD. All configurations of these devices worked well, > and I can no longer make it fail if I try to. My testing with > firmware-allocated resources is limited due to using computers with > Alpine Ridge BIOS support. However, I did get manage to test the patch > with firmware-allocated resources by enabling the Alpine Ridge BIOS > support and forcing pcie_ports=native, and the results were perfect. > > Mika Westerberg has agreed to test this on an official platform with > native enumeration firmware support to be sure that it works in this > situation. It is also appropriate that he reviews these changes as he > wrote the original code and any changes made to Thunderbolt-critical > code
Re: [PATCH v6 1/4] PCI: Consider alignment of hot-added bridges when distributing available resources
On Sat, Jun 15, 2019 at 02:56:36PM -0500, Bjorn Helgaas wrote:
> Mika, this patch changes code you added in 1a5767725cec ("PCI:
> Distribute available resources to hotplug-capable bridges"). Is there
> any chance you could help review this?
Sure, I'll take a look and comment separately.
> On Wed, May 22, 2019 at 02:30:44PM +, Nicholas Johnson wrote:
> > Rewrite pci_bus_distribute_available_resources to better handle bridges
> > with different resource alignment requirements. Pass more details
> > arguments recursively to track the resource start and end addresses
> > relative to the initial hotplug bridge. This is especially useful for
> > Thunderbolt with native PCI enumeration, enabling external graphics
> > cards and other devices with bridge alignment higher than 0x10
> > bytes.
> >
> > Change extend_bridge_window to resize the actual resource, rather than
> > using add_list and dev_res->add_size. If an additional resource entry
> > exists for the given resource, zero out the add_size field to avoid it
> > interfering. Because add_size is considered optional when allocating,
> > using add_size could cause issues in some cases, because successful
> > resource distribution requires sizes to be guaranteed. Such cases
> > include hot-adding nested hotplug bridges in one enumeration, and
> > potentially others which are yet to be encountered.
> >
> > Signed-off-by: Nicholas Johnson
> > ---
> > drivers/pci/setup-bus.c | 169
> > 1 file changed, 84 insertions(+), 85 deletions(-)
> >
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 0cdd5ff38..1b5b851ca 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1835,12 +1835,10 @@ static void extend_bridge_window(struct pci_dev
> > *bridge, struct resource *res,
> > }
> >
> > static void pci_bus_distribute_available_resources(struct pci_bus *bus,
> > - struct list_head *add_list,
> > - resource_size_t available_io,
> > - resource_size_t available_mmio,
> > - resource_size_t available_mmio_pref)
> > + struct list_head *add_list, struct resource io,
> > + struct resource mmio, struct resource mmio_pref)
>
> Follow the parameter indentation style of the rest of the file.
>
> > {
> > - resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
> > + resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
> > unsigned int normal_bridges = 0, hotplug_bridges = 0;
> > struct resource *io_res, *mmio_res, *mmio_pref_res;
> > struct pci_dev *dev, *bridge = bus->self;
> > @@ -1850,29 +1848,36 @@ static void
> > pci_bus_distribute_available_resources(struct pci_bus *bus,
> > mmio_pref_res = &bridge->resource[PCI_BRIDGE_RESOURCES + 2];
> >
> > /*
> > -* Update additional resource list (add_list) to fill all the
> > -* extra resource space available for this port except the space
> > -* calculated in __pci_bus_size_bridges() which covers all the
> > -* devices currently connected to the port and below.
> > +* The alignment of this bridge is yet to be considered, hence it must
> > +* be done now before extending its bridge window. A single bridge
> > +* might not be able to occupy the whole parent region if the alignment
> > +* differs - for example, an external GPU at the end of a Thunderbolt
> > +* daisy chain.
>
> The example seems needlessly specific. There isn't anything GPU- or
> Thunderbolt-specific about this, is there?
>
> Bridge windows can be aligned to any multiple of 1MB. But a device
> BAR must be aligned on its size, so any BAR larger than 1MB should be
> able to cause this, e.g.,
>
> [mem 0x10-0x3f] (bridge A 3MB window)
> [mem 0x20-0x3f] (bridge B 2MB window)
> [mem 0x20-0x3f] (device 2MB BAR)
>
> Bridge B *could* occupy the the entire 3MB parent region, but it
> doesn't need to. But you say it "might not be *able* to", so maybe
> you're thinking of something different?
>
> > - extend_bridge_window(bridge, io_res, add_list, available_io);
> > - extend_bridge_window(bridge, mmio_res, add_list, available_mmio);
> > - extend_bridge_window(bridge, mmio_pref_res, add_list,
> > -available_mmio_pref);
> > + align = pci_resource_alignment(bridge, io_res);
> > + if (!io_res->parent && align)
> > + io.start = ALIGN(io.start, align);
> > +
> > + align = pci_resource_alignment(bridge, mmio_res);
> > + if (!mmio_res->parent && align)
> > + mmio.start = ALIGN(mmio.start, align);
> > +
> > + align = pci_resource_alignment(bridge, mmio_pref_res);
> > + if (!mmio_pref_res->parent && align)
> > + mmio_pref.start = ALIGN(mmio_pref.start, align);
> >
> > /*
> > -* Calculate the total amount of extra resource
Re: [PATCH v6 1/4] PCI: Consider alignment of hot-added bridges when distributing available resources
On Wed, May 22, 2019 at 02:30:44PM +, Nicholas Johnson wrote:
> Rewrite pci_bus_distribute_available_resources to better handle bridges
> with different resource alignment requirements. Pass more details
> arguments recursively to track the resource start and end addresses
> relative to the initial hotplug bridge. This is especially useful for
> Thunderbolt with native PCI enumeration, enabling external graphics
> cards and other devices with bridge alignment higher than 0x10
Instead of 0x10 you could say 1MB here.
> bytes.
>
> Change extend_bridge_window to resize the actual resource, rather than
> using add_list and dev_res->add_size. If an additional resource entry
> exists for the given resource, zero out the add_size field to avoid it
> interfering. Because add_size is considered optional when allocating,
> using add_size could cause issues in some cases, because successful
> resource distribution requires sizes to be guaranteed. Such cases
> include hot-adding nested hotplug bridges in one enumeration, and
> potentially others which are yet to be encountered.
>
> Signed-off-by: Nicholas Johnson
The logic makes sense to me but since you probably need to spin another
revision anyway please find a couple of additional comments below.
> ---
> drivers/pci/setup-bus.c | 169
> 1 file changed, 84 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 0cdd5ff38..1b5b851ca 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1835,12 +1835,10 @@ static void extend_bridge_window(struct pci_dev
> *bridge, struct resource *res,
> }
>
> static void pci_bus_distribute_available_resources(struct pci_bus *bus,
> - struct list_head *add_list,
> - resource_size_t available_io,
> - resource_size_t available_mmio,
> - resource_size_t available_mmio_pref)
> + struct list_head *add_list, struct resource io,
> + struct resource mmio, struct resource mmio_pref)
> {
> - resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
> + resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
> unsigned int normal_bridges = 0, hotplug_bridges = 0;
> struct resource *io_res, *mmio_res, *mmio_pref_res;
> struct pci_dev *dev, *bridge = bus->self;
> @@ -1850,29 +1848,36 @@ static void
> pci_bus_distribute_available_resources(struct pci_bus *bus,
> mmio_pref_res = &bridge->resource[PCI_BRIDGE_RESOURCES + 2];
>
> /*
> - * Update additional resource list (add_list) to fill all the
> - * extra resource space available for this port except the space
> - * calculated in __pci_bus_size_bridges() which covers all the
> - * devices currently connected to the port and below.
> + * The alignment of this bridge is yet to be considered, hence it must
> + * be done now before extending its bridge window. A single bridge
> + * might not be able to occupy the whole parent region if the alignment
> + * differs - for example, an external GPU at the end of a Thunderbolt
> + * daisy chain.
As Bjorn also commented there is nothing Thunderbolt specific here so I
would leave it out of the comment because it is kind of confusing.
>*/
> - extend_bridge_window(bridge, io_res, add_list, available_io);
> - extend_bridge_window(bridge, mmio_res, add_list, available_mmio);
> - extend_bridge_window(bridge, mmio_pref_res, add_list,
> - available_mmio_pref);
> + align = pci_resource_alignment(bridge, io_res);
> + if (!io_res->parent && align)
> + io.start = ALIGN(io.start, align);
> +
> + align = pci_resource_alignment(bridge, mmio_res);
> + if (!mmio_res->parent && align)
> + mmio.start = ALIGN(mmio.start, align);
> +
> + align = pci_resource_alignment(bridge, mmio_pref_res);
> + if (!mmio_pref_res->parent && align)
> + mmio_pref.start = ALIGN(mmio_pref.start, align);
>
> /*
> - * Calculate the total amount of extra resource space we can
> - * pass to bridges below this one. This is basically the
> - * extra space reduced by the minimal required space for the
> - * non-hotplug bridges.
> + * Update the resources to fill as much remaining resource space in the
> + * parent bridge as possible, while considering alignment.
>*/
> - remaining_io = available_io;
> - remaining_mmio = available_mmio;
> - remaining_mmio_pref = available_mmio_pref;
> + extend_bridge_window(bridge, io_res, add_list, resource_size(&io));
> + extend_bridge_window(bridge, mmio_res, add_list, resource_size(&mmio));
> + extend_bridge_window(bridge, mmio_pref_res, add_list,
> + resource_size(&mmio_pref));
Re: [PATCH 00/24] Thunderbolt security levels and NVM firmware upgrade
On Wed, May 24, 2017 at 07:32:45PM +, Jamet, Michael wrote: > I talked to our BIOS expert today. Here is his advice to debugging further: > > It looks like something may have been wrong from system (BIOS, FW, others...) > perspective. > On reboot need to enter EFI shell and check resources of > pci :01:00.0: bridge. > At the EFI shell, this bridge MUST be either configured or absent. > > I would start this way, once we have this info, we may circle back to > him and look into next debugging step. Thanks, I'll try this today.
Re: [PATCH 00/24] Thunderbolt security levels and NVM firmware upgrade
On Thu, May 25, 2017 at 10:20:10AM +0300, mika.westerb...@linux.intel.com wrote: > On Wed, May 24, 2017 at 07:32:45PM +, Jamet, Michael wrote: > > I talked to our BIOS expert today. Here is his advice to debugging further: > > > > It looks like something may have been wrong from system (BIOS, FW, > > others...) perspective. > > On reboot need to enter EFI shell and check resources of > > pci :01:00.0: bridge. > > At the EFI shell, this bridge MUST be either configured or absent. > > > > I would start this way, once we have this info, we may circle back to > > him and look into next debugging step. > > Thanks, I'll try this today. This is the contents dumped directly from EFI shell when a device is connected. It seems that the vendor_id/device_id is 0x but the rest of the config seems to be present (although not fully configured): PCI Segment 00 Bus 01 Device 00 Func 00 [EFI 000100] : FF FF FF FF 00 00 10 00-00 00 04 06 00 00 01 00 ** 0010: 00 00 00 00 00 00 00 00-00 00 00 00 01 01 00 00 ** 0020: 00 00 00 00 01 00 01 00-00 00 00 00 00 00 00 00 ** 0030: 00 00 00 00 80 00 00 00-00 00 00 00 FF 01 00 00 ** 0040: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 ** 0050: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 ** 0060: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 ** 0070: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 ** 0080: 01 88 C3 FF 08 00 00 00-05 AC 80 00 00 00 00 00 ** 0090: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 ** 00A0: 00 00 00 00 00 00 00 00-00 00 00 00 0D C0 00 00 ** 00B0: 22 22 11 11 00 00 00 00-00 00 00 00 00 00 00 00 *""..* 00C0: 10 00 52 00 20 80 E8 07-10 28 10 00 43 5C 45 00 *..R. (..C\E.* 00D0: 00 00 23 10 00 00 00 00-00 00 00 00 00 00 00 00 *..#.* 00E0: 00 00 00 00 00 08 00 00-00 00 00 00 0E 00 00 00 ** 00F0: 03 00 1E 00 00 00 00 00-00 00 00 00 00 00 00 00 ** I wonder how Linux manages to find the device if vendor_id/device_id reads 0x?
Re: [PATCH 00/24] Thunderbolt security levels and NVM firmware upgrade
On Thu, May 25, 2017 at 11:04:08AM +0300, mika.westerb...@linux.intel.com wrote: > On Thu, May 25, 2017 at 10:20:10AM +0300, mika.westerb...@linux.intel.com > wrote: > > On Wed, May 24, 2017 at 07:32:45PM +, Jamet, Michael wrote: > > > I talked to our BIOS expert today. Here is his advice to debugging > > > further: > > > > > > It looks like something may have been wrong from system (BIOS, FW, > > > others...) perspective. > > > On reboot need to enter EFI shell and check resources of > > > pci :01:00.0: bridge. > > > At the EFI shell, this bridge MUST be either configured or absent. > > > > > > I would start this way, once we have this info, we may circle back to > > > him and look into next debugging step. > > > > Thanks, I'll try this today. > > > This is the contents dumped directly from EFI shell when a device is > connected. It seems that the vendor_id/device_id is 0x but the rest > of the config seems to be present (although not fully configured): > > PCI Segment 00 Bus 01 Device 00 Func 00 [EFI 000100] > : FF FF FF FF 00 00 10 00-00 00 04 06 00 00 01 00 > ** > 0010: 00 00 00 00 00 00 00 00-00 00 00 00 01 01 00 00 > ** > 0020: 00 00 00 00 01 00 01 00-00 00 00 00 00 00 00 00 > ** > 0030: 00 00 00 00 80 00 00 00-00 00 00 00 FF 01 00 00 > ** > 0040: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > ** > 0050: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > ** > 0060: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > ** > 0070: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > ** > 0080: 01 88 C3 FF 08 00 00 00-05 AC 80 00 00 00 00 00 > ** > 0090: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > ** > 00A0: 00 00 00 00 00 00 00 00-00 00 00 00 0D C0 00 00 > ** > 00B0: 22 22 11 11 00 00 00 00-00 00 00 00 00 00 00 00 > *""..* > 00C0: 10 00 52 00 20 80 E8 07-10 28 10 00 43 5C 45 00 *..R. > (..C\E.* > 00D0: 00 00 23 10 00 00 00 00-00 00 00 00 00 00 00 00 > *..#.* > 00E0: 00 00 00 00 00 08 00 00-00 00 00 00 0E 00 00 00 > ** > 00F0: 03 00 1E 00 00 00 00 00-00 00 00 00 00 00 00 00 > ** > > I wonder how Linux manages to find the device if vendor_id/device_id > reads 0x? OK, here's the explanation. When Linux initializes ACPI (this happens before PCI initial scan), it calls acpi_initialize_objects(). This in turn causes _INI methods of devices to be executed. Now, the _SB.PCI0._INI() ends up calling \_GPE.TINI() which executes Thunderbolt specific OSUP() method. Purpose of this method is to overwrite vendor_id/device_id to the correct values with the assumption that the OS has already done the initial PCI scan. In case of Linux this is not true and that is the reason the upstream port is found half-initialized leading to the failure.
Re: [PATCH 00/24] Thunderbolt security levels and NVM firmware upgrade
On Thu, May 25, 2017 at 03:03:07PM +0300, mika.westerb...@linux.intel.com wrote: > On Thu, May 25, 2017 at 11:04:08AM +0300, mika.westerb...@linux.intel.com > wrote: > > On Thu, May 25, 2017 at 10:20:10AM +0300, mika.westerb...@linux.intel.com > > wrote: > > > On Wed, May 24, 2017 at 07:32:45PM +, Jamet, Michael wrote: > > > > I talked to our BIOS expert today. Here is his advice to debugging > > > > further: > > > > > > > > It looks like something may have been wrong from system (BIOS, FW, > > > > others...) perspective. > > > > On reboot need to enter EFI shell and check resources of > > > > pci :01:00.0: bridge. > > > > At the EFI shell, this bridge MUST be either configured or absent. > > > > > > > > I would start this way, once we have this info, we may circle back to > > > > him and look into next debugging step. > > > > > > Thanks, I'll try this today. > > > > > > This is the contents dumped directly from EFI shell when a device is > > connected. It seems that the vendor_id/device_id is 0x but the rest > > of the config seems to be present (although not fully configured): > > > > PCI Segment 00 Bus 01 Device 00 Func 00 [EFI 000100] > > : FF FF FF FF 00 00 10 00-00 00 04 06 00 00 01 00 > > ** > > 0010: 00 00 00 00 00 00 00 00-00 00 00 00 01 01 00 00 > > ** > > 0020: 00 00 00 00 01 00 01 00-00 00 00 00 00 00 00 00 > > ** > > 0030: 00 00 00 00 80 00 00 00-00 00 00 00 FF 01 00 00 > > ** > > 0040: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > > ** > > 0050: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > > ** > > 0060: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > > ** > > 0070: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > > ** > > 0080: 01 88 C3 FF 08 00 00 00-05 AC 80 00 00 00 00 00 > > ** > > 0090: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 > > ** > > 00A0: 00 00 00 00 00 00 00 00-00 00 00 00 0D C0 00 00 > > ** > > 00B0: 22 22 11 11 00 00 00 00-00 00 00 00 00 00 00 00 > > *""..* > > 00C0: 10 00 52 00 20 80 E8 07-10 28 10 00 43 5C 45 00 *..R. > > (..C\E.* > > 00D0: 00 00 23 10 00 00 00 00-00 00 00 00 00 00 00 00 > > *..#.* > > 00E0: 00 00 00 00 00 08 00 00-00 00 00 00 0E 00 00 00 > > ** > > 00F0: 03 00 1E 00 00 00 00 00-00 00 00 00 00 00 00 00 > > ** > > > > I wonder how Linux manages to find the device if vendor_id/device_id > > reads 0x? > > OK, here's the explanation. > > When Linux initializes ACPI (this happens before PCI initial scan), it > calls acpi_initialize_objects(). This in turn causes _INI methods of > devices to be executed. Now, the _SB.PCI0._INI() ends up calling > \_GPE.TINI() which executes Thunderbolt specific OSUP() method. Purpose > of this method is to overwrite vendor_id/device_id to the correct values > with the assumption that the OS has already done the initial PCI scan. > > In case of Linux this is not true and that is the reason the upstream > port is found half-initialized leading to the failure. We finally found out what the problem is. In short the above _SB.PCI0._INI() (and OSUP()) gets called correctly but this is only part of the story. When OSUP() is called it rewrites the vendor/deviceid and then signals that certain GPE handler can continue to trigger the SMI handler which should enumerate all the devices before PCI scan happens. In Linux we enable GPEs later, after PCI scan so we only see partially configured PCI bridges (as the SMI handler has not run yet). Rafael's patch series here: https://lkml.org/lkml/2017/8/9/1017 addresses this so that we enable GPEs earlier among other things.
Re: [PATCH v2 00/10] spi-nor: intel-spi: Various fixes and enhancements
On Wed, Sep 13, 2017 at 10:11:21AM +0800, Bin Meng wrote: > Hi Joakim, > > On Tue, Sep 12, 2017 at 1:44 AM, Joakim Tjernlund > wrote: > > On Mon, 2017-09-11 at 02:41 -0700, Bin Meng wrote: > >> This series does several bug fixes and clean ups against the intel-spi > >> spi-nor driver, as well as enhancements to make the driver independent > >> on the underlying BIOS/bootloader. > >> > >> At present the driver uses the HW sequencer for the read/write/erase on > >> all supported platforms, read_reg/write_reg for BXT, and the SW sequencer > >> for read_reg/write_reg for BYT/LPT. The way the driver uses the HW and SW > >> sequencer relies on some programmed register settings and hence creates > >> unneeded dependencies with the underlying BIOS/bootloader. For example, > >> the driver unfortunately does not work as expected when booting from > >> Intel Baytrail FSP based bootloaders like U-Boot, as the Baytrail FSP > >> does not set up some SPI controller settings to make the driver happy. > >> Now such limitation has been removed with this series. > > > > Hi Bin > > > > Just starting to test these on Rangeley and got a question: We have two SPI > > flashes on CS0 resp. CS1 > > and the mtd driver seems to only map the first of those flashes. Is this > > intentional or > > are we missing something? > > > > All the boards I have tested only have one SPI flash. Mika, any comments? So I don't have such boards either. However, I think the other CS is mapped to bit 24 of the flash address. So once you try to address higher than 16MB it should activate the other CS instead. Not 100% sure, though but for example Intel C620 chipset datasheet [1] seems to have additional bits in address register (there is also another CS for TPM). [1] https://www.intel.com/content/www/us/en/chipsets/c620-series-chipset-datasheet.html
Re: [PATCH 00/24] Thunderbolt security levels and NVM firmware upgrade
On Sat, May 20, 2017 at 09:15:17AM +, Levy, Amir (Jer) wrote:
> > I created a udev rule that will automatically authorize the dock and cable.
> > #dell cable
> > ACTION=="add", SUBSYSTEM=="thunderbolt", ATTR{authorized}=="0",
> > ATTR{vendor}=="0xd4", ATTR{device}=="0xb051", ATTR{authorized}="1"
> > #dell dock
> > ACTION=="add", SUBSYSTEM=="thunderbolt", ATTR{authorized}=="0",
> > ATTR{vendor}=="0xd4", ATTR{device}=="0xb054", ATTR{authorized}="1"
> >
>
> Note that the udev rule should authorize the cable first and then the dock.
That should be fine, the devices appear in order closest to the host and
get added to the system in that order so udev should see them in that
order as well. Also the cable device will be parent to the dock.
Re: [PATCH 2/2] thunderbolt: Mask ring interrupt properly when polling starts
On Mon, Nov 27, 2017 at 04:21:34PM +, Bernat, Yehezkel wrote:
> On Mon, 2017-11-27 at 14:48 +0300, Mika Westerberg wrote:
> > When ring enters polling mode we are expected to mask the ring
> > interrupt
> > before the callback is called. However, the current code actually
> > unmasks it probably because of a copy-paste mistake.
> >
> > Mask the interrupt properly from now on.
> >
> > Fixes: 4ffe722eefcb ("thunderbolt: Add polling mode for rings")
> > Signed-off-by: Mika Westerberg
> > ---
> > drivers/thunderbolt/nhi.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> > index 419a7a90bce0..f45bcbc63738 100644
> > --- a/drivers/thunderbolt/nhi.c
> > +++ b/drivers/thunderbolt/nhi.c
> > @@ -339,7 +339,7 @@ static void __ring_interrupt(struct tb_ring
> > *ring)
> > return;
> >
> > if (ring->start_poll) {
> > - __ring_interrupt_mask(ring, false);
> > + __ring_interrupt_mask(ring, true);
> > ring->start_poll(ring->poll_data);
> > } else {
> > schedule_work(&ring->work);
>
> Acked-by: Yehezkel Bernat
Applied with Yehezkel's ACK.
Re: Linux ACPI DSDT table editing tools
On Wed, Feb 20, 2013 at 07:36:32AM +, Pallala, Ramakrishna wrote: > I am looking for tools(Linux or w*s) for editing ACPI tables. > Can you advise me some good tools. Also I would like to know how BIOS > engineers populate the ACPI tables? Which tools they use? Dunno about the BIOS engineers but I use tools from acpica (acpica.org). There is ASL (dis)assembler and other useful tools in that. In addition there is tools/power/acpi/acpidump.c in the kernel source tree which is extremely useful. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
