Re: IOMMU vs Ryzen embedded EMMC controller
Hi Kurf On 9/27/2019 3:17 PM, Kurt Garloff wrote: > Hi Jörg, > > On 25/09/2019 17:42, Joerg Roedel wrote: >> On Wed, Sep 25, 2019 at 05:27:32PM +0200, Jiri >> Kosina wrote: >>> On Sat, 21 Sep 2019, Kurt Garloff wrote: [12916.740274] mmc0: sdhci: [12916.740337] mmc0: error -5 whilst initialising MMC card >>> Do you have BAR memory allocation failures in >>> dmesg with IOMMU on? > > No. The device is *not* treated as PCI device and > I still think that this is the source of the evil. > >>> Actually, sharing both working and non-working >>> dmesg, as well as >>> /proc/iomem contents, would be helpful. >> Yes, can you please grab dmesg from a boot with >> iommu enabled and add >> 'amd_iommu_dump' to the kernel command line? >> That should give some hints >> on what is going on. > > For now I attach a dmesg and iomem from the boot > with IOMMU enabled. > Nothing much interesting without IOMMU, sdhci-acpi > there just works -- let me know if you still want > me to send the kernel msg. > > Thanks for looking into this! > I have added Suravee from AMD in the mail loop. He works on IOMMU part. As per my understanding, it needs a patch in IOMMU driver for adding support of EMMC. Note that on Ryzen platform we have EMMC 5.0 as ACPI device. Thanks Nehal Shah
Re: [PATCH v17] i2c: Add drivers for the AMD PCIe MP2 I2C controller
Hi Elie, On 3/5/2019 8:43 PM, Elie Morisse wrote: > MP2 controllers have two separate busses, so may accommodate up to two I2C > adapters. Those adapters are listed in the ACPI namespace with the > "AMDI0011" HID, and probed by a platform driver. > > Communication with the MP2 takes place through MMIO registers, or through > DMA for more than 32 bytes transfers. > > This is major rework of the patch submitted by Nehal-bakulchandra Shah from > AMD (https://patchwork.kernel.org/patch/10597369/). > > Most of the event handling of v3 was rewritten to make it work with more > than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version > contains many other improvements. > > Signed-off-by: Elie Morisse > --- > Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): > -> Add fix for IOMMU > -> Add dependency of ACPI > -> Add locks to avoid the crash > > Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): > > -> fix for review comments > -> fix for more than 32 bytes write issue > > Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: > > -> support more than one bus/adapter > -> support more than one slave per bus > -> use the bus speed specified by the slaves declared in the DSDT instead of >assuming speed == 400kbits/s > -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply >use i2c_msg.buf > -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a >multiple of 4 by using memcpy_fromio and memcpy_toio > -> use streaming DMA mappings instead of allocating a coherent DMA buffer for >every >32 bytes read/write > -> properly check for timeouts during i2c_amd_xfer and increase it from 50 >jiffies to 250 msecs (which is more in line with other drivers) > -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success >event, instead of stalling i2c_amd_xfer > -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see >the point since it's already waiting for a i2c_busenable_complete event > -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want >parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) > -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which >are shared across the two busses/adapters > -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT >enumerates devices with the "AMDI0011" HID > -> set maximum length of reads/writes to 4095 (event's length field is 12 > bits) > -> basic PM support > -> style corrections to match the kernel code style, and tried to reduce code >duplication whenever possible > > Changes since v4 by Elie M.: > > -> fix missing typecast warning > -> removed the duplicated entry in Kconfig > > Changes since v5 by Elie M.: > > -> move DMA mapping from the platform driver to the PCI driver > -> attempt to find the platform device's PCI parent through the _DEP ACPI > method >(if not found take the first MP2 device registred in the i2c-amd-pci-mp2 >driver, like before) > -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 >driver > -> address other review comments by Bjorn Helgaas (meant for v3) > > Changes since v6 by Elie M.: > > -> remove unnecessary memcpy from the DMA buffer during > i2c_amd_read_completion > > Changes since v7 by Elie M.: > > -> merge the two modules into one named i2c-amd-mp2, delete now useless > exports > -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the >MP2 controller if a slave doesn't respond to a read or write request > -> unmap the DMA buffer before read/write_complete > -> fix bus_disable commands handling (no more errors during module exit) > -> prefer managed versions of pci_ and dev_ functions whenever possible > -> increase adapter retries from 0 to 3 > -> reduce code duplication in debugfs functions > -> address other review points by Bjorn Helgaas (except regarding the _DEP >hint) > > Changes since v8 by Elie M.: > > -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever > possible > -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf > -> defer probe() by the platform driver if no MP2 device has been probed > yet by the PCI driver (which should address Bjorn Helgaas' concern while > preserving the platform driver) > -> if the interrupt following a command doesn't get handled after 250ms, > zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more > seconds > -> include AMD_P2C_MSG3 and fix typo in debugfs output > -> cosmetic fixes, code reduction, and better comments > -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers > > Changes since v9 by Elie M.: > > -> remove the xfer_lock mutex which was redundant with i2c_adapter.bus_lock > -> platform device remove() fixes (protection against the very unlikely > case that both the PCI and platform devices
Re: [PATCH v16] i2c: Add drivers for the AMD PCIe MP2 I2C controller
Hi Elie, On 2/26/2019 10:20 PM, Wolfram Sang wrote: > Hi Elie, > >> My apologies for the time it took to apply those changes. > > No worries. I am super happy that you are still around and willing to > continue the work! > >> I will also start working on an alternate (v17?) version with only one >> module and replacing the platform driver by an ACPI lookup as >> requested by Bjorn Helgaas, unless Nehal clarifies how AMD plans to >> expand the platform driver. > > Sounds really good to me. Thanks for the patch. For our upcoming sensor hub design we need MP2-PCIE as independent driver.Hence i am requesting to keep the two separate modules. > Thanks, > >Wolfram > Regards Nehal Shah
Re: [PATCH v15] i2c: Add drivers for the AMD PCIe MP2 I2C controller
Hi Bjorn and Wolfram, On 2/7/2019 9:23 PM, Wolfram Sang wrote: > Hi Bjorn, > > thanks a lot for your additional information! > >> IMHO the split into two drivers is a bit of a mess and doesn't really >> correspond with the hardware, as I mentioned at [1]. The PCI device >> is the real hardware and the driver should claim that. AFAICT the >> ACPI device exists only to pass some config information to the PCI >> driver. I think the natural approach would be for the PCI driver to >> directly search the ACPI namespace for that config information. > > AFAIR the AMD folks insisted on the two driver setup because they need > it in the future? Maybe they can explain again here? > >> The fact that driver_find_device() is essentially unused except for a >> few very special cases is a good clue that there's probably a better >> way. > > Excactly this thinking made me recommend something else, too. Let's see > what we can come up with. First of really thanks for your valuable review. It may seem to be illogical to have two separate drivers, however as explained in past we are working on another solution for some upcoming thing. In that case we need MP2-PCI communication driver which will be reused. At this point of time i can't talk much about that but once solution is ready, we will be pushing that as well. Hence i sincerely requesting to have two separate driver. For rest of remaining comments will look into the same. Elie hope you will also have a look and we will have a common understanding. > Thanks, > >Wolfram > Thanks for consideration and your understanding. Regards Nehal Shah
Re: [PATCH v6 09/11] mmc: sdhci-acpi: Make PCI dependency explicit
Hi
On 1/7/2019 6:12 PM, Adrian Hunter wrote:
> On 7/01/19 1:17 PM, Rafael J. Wysocki wrote:
>> On Sat, Jan 5, 2019 at 11:06 AM Sinan Kaya wrote:
>>>
>>> After 'commit 5d32a66541c4 ("PCI/ACPI: Allow ACPI to be built without
>>> CONFIG_PCI set")' dependencies on CONFIG_PCI that previously were
>>> satisfied implicitly through dependencies on CONFIG_ACPI have to be
>>> specified directly. This driver relies on IOSF_MBI and IOSF_MBI depends
>>> on PCI. For this reason, add a direct dependency to CONFIG_PCI here.
>>>
>>> Fixes: 5d32a66541c46 ("PCI/ACPI: Allow ACPI to be built without CONFIG_PCI
>>> set")
>>> Signed-off-by: Sinan Kaya
>>> ---
>>> drivers/mmc/host/Kconfig | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>> index e26b8145efb3..1b9401fe94c0 100644
>>> --- a/drivers/mmc/host/Kconfig
>>> +++ b/drivers/mmc/host/Kconfig
>>> @@ -116,7 +116,7 @@ config MMC_RICOH_MMC
>>>
>>> config MMC_SDHCI_ACPI
>>> tristate "SDHCI support for ACPI enumerated SDHCI controllers"
>>> - depends on MMC_SDHCI && ACPI
>>> + depends on MMC_SDHCI && ACPI && PCI
>>> select IOSF_MBI if X86
>>> help
>>> This selects support for ACPI enumerated SDHCI controllers,
>>> --
>>
>> Any objections against this one from anyone?
>
> + QCOM and AMD contributors
>
No issues with us as we have separate PCI based driver and ACPI based driver.
Regards
Nehal Shah
Re: [PATCH v15] i2c: Add drivers for the AMD PCIe MP2 I2C controller
Hi Elie, On 12/27/2018 4:52 AM, Elie Morisse wrote: > MP2 controllers have two separate busses, so may accommodate up to two I2C > adapters. Those adapters are listed in the ACPI namespace with the > "AMDI0011" HID, and probed by a platform driver. > > Communication with the MP2 takes place through MMIO registers, or through > DMA for more than 32 bytes transfers. > > This is major rework of the patch submitted by Nehal-bakulchandra Shah from > AMD (https://patchwork.kernel.org/patch/10597369/). > > Most of the event handling of v3 was rewritten to make it work with more > than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version > contains many other improvements. > > Signed-off-by: Elie Morisse > --- > Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): > -> Add fix for IOMMU > -> Add dependency of ACPI > -> Add locks to avoid the crash > > Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): > > -> fix for review comments > -> fix for more than 32 bytes write issue > > Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: > > -> support more than one bus/adapter > -> support more than one slave per bus > -> use the bus speed specified by the slaves declared in the DSDT instead of >assuming speed == 400kbits/s > -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply >use i2c_msg.buf > -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a >multiple of 4 by using memcpy_fromio and memcpy_toio > -> use streaming DMA mappings instead of allocating a coherent DMA buffer for >every >32 bytes read/write > -> properly check for timeouts during i2c_amd_xfer and increase it from 50 >jiffies to 250 msecs (which is more in line with other drivers) > -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success >event, instead of stalling i2c_amd_xfer > -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see >the point since it's already waiting for a i2c_busenable_complete event > -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want >parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) > -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which >are shared across the two busses/adapters > -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT >enumerates devices with the "AMDI0011" HID > -> set maximum length of reads/writes to 4095 (event's length field is 12 > bits) > -> basic PM support > -> style corrections to match the kernel code style, and tried to reduce code >duplication whenever possible > > Changes since v4 by Elie M.: > > -> fix missing typecast warning > -> removed the duplicated entry in Kconfig > > Changes since v5 by Elie M.: > > -> move DMA mapping from the platform driver to the PCI driver > -> attempt to find the platform device's PCI parent through the _DEP ACPI > method >(if not found take the first MP2 device registred in the i2c-amd-pci-mp2 >driver, like before) > -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 >driver > -> address other review comments by Bjorn Helgaas (meant for v3) > > Changes since v6 by Elie M.: > > -> remove unnecessary memcpy from the DMA buffer during > i2c_amd_read_completion > > Changes since v7 by Elie M.: > > -> merge the two modules into one named i2c-amd-mp2, delete now useless > exports > -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the >MP2 controller if a slave doesn't respond to a read or write request > -> unmap the DMA buffer before read/write_complete > -> fix bus_disable commands handling (no more errors during module exit) > -> prefer managed versions of pci_ and dev_ functions whenever possible > -> increase adapter retries from 0 to 3 > -> reduce code duplication in debugfs functions > -> address other review points by Bjorn Helgaas (except regarding the _DEP >hint) > > Changes since v8 by Elie M.: > > -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever > possible > -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf > -> defer probe() by the platform driver if no MP2 device has been probed > yet by the PCI driver (which should address Bjorn Helgaas' concern while > preserving the platform driver) > -> if the interrupt following a command doesn't get handled after 250ms, > zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more > seconds > -> include AMD_P2C_MSG3 and fix typo in debugfs output > -> cosmetic fixes, code reduction, and better comments > -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers > > Changes since v9 by Elie M.: > > -> remove the xfer_lock mutex which was redundant with i2c_adapter.bus_lock > -> platform device remove() fixes (protection against the very unlikely > case that both the PCI and platform devic
Re: [PATCH v10] i2c: Add drivers for the AMD PCIe MP2 I2C controller
On 11/27/2018 10:09 PM, Elie Morisse wrote:
> Hi Nehal-bakulchandra,
>
>> There was intention behind to make two separate driver for pci and I2c.
> It has future usecase in our platforms and hence we made modular designed.
> So better to make it two separate driver.
>
> I merged the two modules into one because of Bjorn Helgaas' concern that
> the platform driver only exists to extract information from the ACPI
> namespace.
> And that didn't really address it tbh.
>
> Currently the minor advantages of having two drivers is that each
> bus/adapter on the MP2 has its entry in sysfs and can be individually
> (un-)bound, and less code duplication in the driver. Now if AMD plans to
> expand the platform driver, that would probably make more sense to keep two
> drivers and two modules.
>
As discussed last time yes we have usecase so better to keep separate driver.
> If the Linux maintainers consent to it I can re-split the module into two.
>
>> Another thing i would like to understand why _DEP method based hint is
> required, i didn't find need of it.
>
> In case there is more than one MP2 PCI device present on the same system,
> how do you determine which PCI device is an AMDI0011 ACPI device related to?
>
> I pasted the Yoga 530's DSDT here: https://paste.kde.org/pjobniftq
> The only available hints are the _DEP, method, and also perhaps the UID or
> _ADR:
>
>Device (I2CB)
> {
> Name (_HID, "AMDI0011") // _HID: Hardware ID
> Name (_UID, One) // _UID: Unique ID
> Name (_ADR, One) // _ADR: Address
> //...
> Name (_DEP, Package (0x01) // _DEP: Dependencies
> {
> ^PCI0.GP17.MP2C
> })
>
> If _DEP shouldn't be used, could _UID (or _ADR?) be assumed to refer to the
> MP2 device, in the order in which the PCI devices are enumerated? i.e
>
> UID
> 0 -> first MP2 bus 0
> 1 -> first MP2 bus 1
> 2 -> second MP2 bus 0
> 3 -> second MP2 bus 1
> etc.
>
> There's currently no such computer around with more than one MP2 PCI device
> at the moment, but does the documentation say something about this?
>
Yes as of now there is no future plan to have any additional MP2-I2C device.
>> Indeed PCI device owned by i2c-mp2 device so what is the need of moving
> DMA mapping to PCI driver?
>
> Not sure if I understand correctly, the DMA buffer has to be owned by the
> PCI device, not by the platform device/I2C adapter :
>
> i2c_common->dma_addr = dma_map_single(&privdata->pci_dev->dev, ...);
>
> That's why it seemed more natural to move the mapping into the PCI driver,
> and this was also the last occurrence of the platform driver accessing the
> pci_dev pointer. However if you plan to introduce non-PCI I2C controllers
> with the same split between small messages transferred through registers
> and large ones going through DMA then yes it'd make more sense to put it
> back in the platform driver. But is that really an issue for now?
Not issue now but as we want both drivers to be seperate then let the client
driver be owner.
> Btw I'll submit a v11 version of the patch which definitely fixes the
> timeouts on Lenovo Ideapad 530s soon (already fixed in
> https://github.com/Syniurge/i2c-amd-mp2).
>
> Elie
Just received will have a look :) thanks for the same.
Nehal
> Le lun. 26 nov. 2018 à 15:52, Shah, Nehal-bakulchandra <
> nehal-bakulchandra.s...@amd.com> a écrit :
>
>>
>> Hi Elie Morisse,
>>
>> On 11/14/2018 10:00 PM, Elie Morisse wrote:
>>> I2C communication takes place through iomapped registers, or through DMA
>>> for more than 32 bytes transfers.
>>>
>>> MP2 controllers have two separate buses, so may accommodate up to two I2C
>>> adapters. Those adapters are listed in the ACPI namespace with the
>>> "AMDI0011" HID, and probed by a platform driver.
>>>
>>> This is major rework of the patch submitted by Nehal-bakulchandra Shah
>> from
>>> AMD (https://patchwork.kernel.org/patch/10597369/).
>>>
>>> Most of the event handling of v3 was rewritten to make it work with more
>>> than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version
>>> contains many more improvements.
>>>
>>> Signed-off-by: Elie Morisse
>>> ---
>>> Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html
>> ):
>>> -> Add fix for IOMMU
>>> -> Add depedency of ACPI
>>> -> Add locks to avoid the crash
>>>
>>> Changes since v2 (https://patchwork.o
Re: [PATCH v10] i2c: Add drivers for the AMD PCIe MP2 I2C controller
Hi Elie Morisse, On 11/14/2018 10:00 PM, Elie Morisse wrote: > I2C communication takes place through iomapped registers, or through DMA > for more than 32 bytes transfers. > > MP2 controllers have two separate buses, so may accommodate up to two I2C > adapters. Those adapters are listed in the ACPI namespace with the > "AMDI0011" HID, and probed by a platform driver. > > This is major rework of the patch submitted by Nehal-bakulchandra Shah from > AMD (https://patchwork.kernel.org/patch/10597369/). > > Most of the event handling of v3 was rewritten to make it work with more > than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version > contains many more improvements. > > Signed-off-by: Elie Morisse > --- > Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): > -> Add fix for IOMMU > -> Add depedency of ACPI > -> Add locks to avoid the crash > > Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): > > -> fix for review comments > -> fix for more than 32 bytes write issue > > Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: > > -> support more than one bus/adapter > -> support more than one slave per bus > -> use the bus speed specified by the slaves declared in the DSDT instead of >assuming speed == 400kbits/s > -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply >use i2c_msg.buf > -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a >multiple of 4 by using memcpy_fromio and memcpy_toio > -> use streaming DMA mappings instead of allocating a coherent DMA buffer for >every >32 bytes read/write > -> properly check for timeouts during i2c_amd_xfer and increase it from 50 >jiffies to 250 msecs (which is more in line with other drivers) > -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success >event, instead of stalling i2c_amd_xfer > -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see >the point since it's already waiting for a i2c_busenable_complete event > -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want >parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) > -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which >are shared across the two busses/adapters > -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT >enumerates devices with the "AMDI0011" HID > -> set maximum length of reads/writes to 4095 (event's length field is 12 > bits) > -> basic PM support > -> style corrections to match the kernel code style, and tried to reduce code >duplication whenever possible > > Changes since v4 by Elie M.: > > -> fix missing typecast warning > -> removed the duplicated entry in Kconfig > > Changes since v5 by Elie M.: > > -> move DMA mapping from the platform driver to the PCI driver > -> attempt to find the platform device's PCI parent through the _DEP ACPI > method >(if not found take the first MP2 device registred in the i2c-amd-pci-mp2 >driver, like before) > -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 >driver > -> address other review comments by Bjorn Helgaas (meant for v3) > > Changes since v6 by Elie M.: > > -> remove unnecessary memcpy from the DMA buffer during > i2c_amd_read_completion > > Changes since v7 by Elie M.: > > -> merge the two modules into one named i2c-amd-mp2, delete now useless > exports > -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the >MP2 controller if a slave doesn't respond to a read or write request > -> unmap the DMA buffer before read/write_complete > -> fix bus_disable commands handling (no more errors during module exit) > -> prefer managed versions of pci_ and dev_ functions whenever possible > -> increase adapter retries from 0 to 3 > -> reduce code duplication in debugfs functions > -> address other review points by Bjorn Helgaas (except regarding the _DEP >hint) > > Changes since v8 by Elie M.: > > -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever > possible > -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf > -> defer probe() by the platform driver if no MP2 device has been probed > yet by the PCI driver (which should address Bjorn Helgaas' concern while > preserving the platform driver) > -> if the interrupt following a command doesn't get handled after 250ms, > zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more > seconds (there seems to be an interrupt issue with older Zen microcodes, > upgrade your amd microcode package if you experience timeouts) > -> include AMD_P2C_MSG3 and fix typo in debugfs output > -> cosmetic fixes, code reduction, and better comments > -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers > > Changes since v9 by Elie M.: > > -> remove the xfer_lock mutex which was redundant with i2c_ada
Re: [PATCH] pinctrl/amd: poll InterruptEnable bits in enable_irq
Hi On 3/26/2018 2:42 PM, Linus Walleij wrote: > On Mon, Mar 12, 2018 at 5:45 PM, Daniel Kurtz wrote: > >> In certain cases interrupt enablement will be delayed relative to when >> the InterruptEnable bits are written. One example of this is when >> a GPIO's "debounce" logice is first enabled. After enabling debounce, >> there is a 900 us "warm up" period during which InterruptEnable[0] >> (bit 11) will read as 0 despite being written 1. During this time >> InterruptSts will not be updated, nor will interrupts be delivered, even >> if the GPIO's interrupt configuration has been written to the register. >> >> To work around this delay, poll the InterruptEnable bits after setting >> them to ensure interrupts have truly been enabled in hardware before >> returning from the irq_enable handler. >> >> Signed-off-by: Daniel Kurtz > > Patch applied. > > I see the AMD people were not on CC so adding them here so they can > say if there is any problem with the approach. > > Daniel: maybe you should consider listing yourself as comaintainer of this > driver? > > Yours, > Linus Walleij > Looks good. Regards Nehal Shah
RE: [PATCH] pinctrl/amd: Use regular interrupt instead of chained
Hi Thomas, Thanks for the prompt reply. Agree on points. we will validate at our end and shall provide the update. Nehal -Original Message- From: Thomas Gleixner [mailto:t...@linutronix.de] Sent: Friday, May 26, 2017 12:19 PM To: Shah, Nehal-bakulchandra Cc: LKML ; Linus Walleij ; linux-g...@vger.kernel.org; Borislav Petkov ; Xue, Ken ; S-k, Shyam-sundar ; sta...@vger.kernel.org Subject: RE: [PATCH] pinctrl/amd: Use regular interrupt instead of chained Nehal, On Fri, 26 May 2017, Shah, Nehal-bakulchandra wrote: > Thanks for the patch. However, we have received this issue from > multiple people and different disro but it occurs only on Gigabyte > hardware. With reference AM4 ryzen board we are not facing this issue. > We are in discussion with gigabyte to check the BIOS part. Once we > have clarity on that, we can consider driver part. Also, this code is > running on multiple platform of different customers so changing > directly at this point of time may be risky in my point of view. > Requesting you to hold this patch till we get clarity on bios end. It does not matter at all whether this is a problem only on GB hardware. Fact is, that this happened and it will happen again. The patch does not change any functionality of the driver, it merily makes it more robust and spares users the bloody annoying experience of a non booting machine and the tedious task of figuring out why. The main objective of the kernel is robustness and not pleasing the ego of silicon vendors. We can't prevent the stupidity of BIOS people, we merily can deal with it. That patch should go into mainline ASAP and backported to stable in order to help those people who bought wreckaged hardware. Thanks, tglx
RE: [PATCH] pinctrl/amd: Use regular interrupt instead of chained
Hi Thomas,
Thanks for the patch. However, we have received this issue from multiple people
and different disro but it occurs only on Gigabyte hardware. With reference AM4
ryzen board we are not facing this issue.
We are in discussion with gigabyte to check the BIOS part. Once we have clarity
on that, we can consider driver part. Also, this code is running on multiple
platform of different customers so changing directly at this point of time may
be risky in my point of view. Requesting you to hold this patch till we get
clarity on bios end.
Thanks for your understanding.
Regards
Nehal
-Original Message-
From: linux-gpio-ow...@vger.kernel.org
[mailto:linux-gpio-ow...@vger.kernel.org] On Behalf Of Thomas Gleixner
Sent: Wednesday, May 24, 2017 2:54 AM
To: LKML
Cc: Linus Walleij ; linux-g...@vger.kernel.org;
Borislav Petkov ; Xue, Ken
Subject: [PATCH] pinctrl/amd: Use regular interrupt instead of chained
The AMD pinctrl driver uses a chained interrupt to demultiplex the GPIO
interrupts. Kevin Vandeventer reported, that his new AMD Ryzen locks up hard on
boot when the AMD pinctrl driver is initialized. The reason is an interrupt
storm. It's not clear whether that's caused by hardware or firmware or both.
Using chained interrupts on X86 is a dangerous endavour. If a system is
misconfigured or the hardware buggy there is no safety net to catch an
interrupt storm.
Convert the driver to use a regular interrupt for the demultiplex handler. This
allows the interrupt storm detector to catch the malfunction and lets the
system boot up.
This should be backported to stable because it's likely that more users run
into this problem as the AMD Ryzen machines are spreading.
Reported-by: Kevin Vandeventer
Link: https://bugzilla.suse.com/show_bug.cgi?id=1034261
Signed-off-by: Thomas Gleixner
---
drivers/pinctrl/pinctrl-amd.c | 91 ++
1 file changed, 41 insertions(+), 50 deletions(-)
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -495,64 +495,54 @@ static struct irq_chip amd_gpio_irqchip
.flags= IRQCHIP_SKIP_SET_WAKE,
};
-static void amd_gpio_irq_handler(struct irq_desc *desc)
+#define PIN_IRQ_PENDING(BIT(INTERRUPT_STS_OFF) | BIT(WAKE_STS_OFF))
+
+static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
{
- u32 i;
- u32 off;
- u32 reg;
- u32 pin_reg;
- u64 reg64;
- int handled = 0;
- unsigned int irq;
+ struct amd_gpio *gpio_dev = dev_id;
+ struct gpio_chip *gc = &gpio_dev->gc;
+ irqreturn_t ret = IRQ_NONE;
+ unsigned int i, irqnr;
unsigned long flags;
- struct irq_chip *chip = irq_desc_get_chip(desc);
- struct gpio_chip *gc = irq_desc_get_handler_data(desc);
- struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+ u32 *regs, regval;
+ u64 status, mask;
- chained_irq_enter(chip, desc);
- /*enable GPIO interrupt again*/
+ /* Read the wake status */
raw_spin_lock_irqsave(&gpio_dev->lock, flags);
- reg = readl(gpio_dev->base + WAKE_INT_STATUS_REG1);
- reg64 = reg;
- reg64 = reg64 << 32;
-
- reg = readl(gpio_dev->base + WAKE_INT_STATUS_REG0);
- reg64 |= reg;
+ status = readl(gpio_dev->base + WAKE_INT_STATUS_REG1);
+ status <<= 32;
+ status |= readl(gpio_dev->base + WAKE_INT_STATUS_REG0);
raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
- /*
-* first 46 bits indicates interrupt status.
-* one bit represents four interrupt sources.
- */
- for (off = 0; off < 46 ; off++) {
- if (reg64 & BIT(off)) {
- for (i = 0; i < 4; i++) {
- pin_reg = readl(gpio_dev->base +
- (off * 4 + i) * 4);
- if ((pin_reg & BIT(INTERRUPT_STS_OFF)) ||
- (pin_reg & BIT(WAKE_STS_OFF))) {
- irq = irq_find_mapping(gc->irqdomain,
- off * 4 + i);
- generic_handle_irq(irq);
- writel(pin_reg,
- gpio_dev->base
- + (off * 4 + i) * 4);
- handled++;
- }
- }
+ /* Bit 0-45 contain the relevant status bits */
+ status &= (1ULL << 46) - 1;
+ regs = gpio_dev->base;
+ for (mask = 1, irqnr = 0; status; mask <<= 1, regs += 4, irqnr += 4) {
+ if (!(status & mask))
+ continue;
+ status &= ~mask;
+
+ /* Each status bit covers four pins */
+ for (i = 0; i < 4; i++) {
+ regval
Kernel without RTC
Hi, Currently we are having hardware which does not have RTC. It is single processor system. However it does have TSC timer. Now, how to use scheduler with only TSC as current kernel scheduler leverage the RTC for scheduling? I had seen one old patch http://marc.info/?l=linux-kernel&m=112013203625990&w=2 But i guess this patch was later on not taken, It will be great if you can some pointers to move forward? Thanks Nehal
RE: [PATCH] i2c: designware: Fix regression when dynamic TAR update is disabled
It is better to revert the 63d0f0a6952a.
-Original Message-
From: Jarkko Nikula [mailto:jarkko.nik...@linux.intel.com]
Sent: Friday, February 10, 2017 4:19 PM
To: Suthikulpanit, Suravee ; De Marchi, Lucas
; mika.westerb...@linux.intel.com;
andriy.shevche...@linux.intel.com; Shah, Nehal-bakulchandra
Cc: w...@the-dreams.de; linux-kernel@vger.kernel.org;
linux-...@vger.kernel.org; S-k, Shyam-sundar
Subject: Re: [PATCH] i2c: designware: Fix regression when dynamic TAR update is
disabled
On 10.02.2017 08:38, Suravee Suthikulpanit wrote:
> At this points, my understanding is there are probably two options here:
>
> 1) Keep the commit 63d0f0a6952a (i2c: designware: detect when dynamic
> tar update
>is possible) and apply V2 of this patch in 4.10. We might need to
> back-port the change
>to v4.9 stable as well.
>
> 2) Revert the 63d0f0a6952a (i2c: designware: detect when dynamic tar
> update is possible) in 4.10,
>and also in v4.9 stable as well.
>
I'm fine with both options. Maybe revert as commit 0317e6c0f1dc ("i2c:
designware: do not disable adapter after transfer") was also reverted and this
revert doesn't seem to cause conflicts.
--
Jarkko
[PATCH] i2c: designware: Fix regression when dynamic TAR update is disabled
The following commit causes a regression when dynamic TAR update is
disabled:
commit 63d0f0a6952a1a02bc4f116b7da7c7887e46efa3 ("i2c: designware:
detect when dynamic tar update is possible")
In such case, the DW_IC_CON_10BITADDR_MASTER is R/W, and is changed
by the logic that's trying to detect dynamic TAR update.The original
value of DW_IC_CON_10BITADDR_MASTER bit should be restored.
Signed-off-by: Shah Nehal-Bakulchandra
Signed-off-by: Suravee Suthikulpanit
---
drivers/i2c/busses/i2c-designware-core.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index 6d81c56..0c57166 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -987,6 +987,11 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
(reg & DW_IC_CON_10BITADDR_MASTER)) {
dev->dynamic_tar_update_enabled = true;
dev_dbg(dev->dev, "Dynamic TAR update enabled");
+ } else {
+ /* If test is failed then restore the original value */
+ dev->dynamic_tar_update_enabled = false;
+ dev_dbg(dev->dev, "Dynamic TAR update disable restore the
value");
+ dw_writel(dev, reg, DW_IC_CON);
}
i2c_dw_release_lock(dev);
--
1.9.1
RE: [PATCH] pinctrl: amd: fix compilation warning
Thanks Linus for fixing this.
Nehal
-Original Message-
From: Linus Walleij [mailto:linus.wall...@linaro.org]
Sent: Tuesday, January 3, 2017 1:51 PM
To: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Cc: linux-g...@vger.kernel.org; Linus Walleij ; S-k,
Shyam-sundar ; Shah, Nehal-bakulchandra
Subject: [PATCH] pinctrl: amd: fix compilation warning
3bfd44306c65 ("pinctrl: amd: Add support for additional GPIO") created the
following warning:
drivers/pinctrl/pinctrl-amd.c: In function 'amd_gpio_dbg_show':
drivers/pinctrl/pinctrl-amd.c:210:3: warning: 'pin_num' may be used
uninitialized in this function [-Wmaybe-uninitialized]
for (; i < pin_num; i++) {
^
drivers/pinctrl/pinctrl-amd.c:172:21: warning: 'i' may be used uninitialized in
this function [-Wmaybe-uninitialized]
unsigned int bank, i, pin_num;
^
Fix this by adding a guarding default case for illegal bank numbers.
Cc: S-k Shyam-sundar
Cc: Nehal Shah
Signed-off-by: Linus Walleij
---
drivers/pinctrl/pinctrl-amd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 47b17100410e..c2203699f1ab 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -206,6 +206,9 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct
gpio_chip *gc)
i = 192;
pin_num = AMD_GPIO_PINS_BANK3 + i;
break;
+ default:
+ /* Illegal bank number, ignore */
+ continue;
}
for (; i < pin_num; i++) {
seq_printf(s, "pin%d\t", i);
--
2.9.3
RE: ATA failure regression
Hi Can someone please help me to debug this issue? Regards Nehal -Original Message- From: Shah, Nehal-bakulchandra Sent: Monday, September 26, 2016 3:45 PM To: 'Bharat Kumar Gogada' Cc: Deucher, Alexander ; linux-...@vger.kernel.org; hol...@ahsoftware.de; t...@kernel.org; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: RE: ATA failure regression Hi Bharat Thanks for the reply. I have observed following thing If IOMMU is enabled in BIOS and I use following option FCH SATA Debug Options --> Unused SATA Port Auto Shut Down Disabled. Issue is not happening. I have attached dmesg and lspci output with this option Also I have attached lspci log with IOMMU disabled. When issue is happening I am not able to take lspci logs as it stops in initramfs itself. I will try to get. Thanks Nehal -Original Message- From: Bharat Kumar Gogada [mailto:bharat.kumar.gog...@xilinx.com] Sent: Friday, September 23, 2016 3:40 PM To: Shah, Nehal-bakulchandra ; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; hol...@ahsoftware.de; t...@kernel.org; linux-...@vger.kernel.org Cc: Deucher, Alexander Subject: RE: ATA failure regression > Hi All, > > Resending this wider audience > > Currently I am working on AMD future platform. I am hitting the same > bug of ATA Failure Regression reported in past. > (https://patchwork.kernel.org/patch/6875661/) or > http://lkml.iu.edu/hypermail/linux/kernel/1507.3/01961.html > > I am newbie to this and because of this Ubuntu 16.04 is not booting. > If disable IOMMU and MSI obviously it works but that is not solution. > Even when I bisected it boiled to same place which was mentioned in past > discussion. > So here when you are disabling MSI, it is working with legacy interrupts or MSI-X. Can you post the end sata device lscpi -xxx -vvv content. Bharat
RE: ATA failure regression
Hi All, Resending this wider audience Currently I am working on AMD future platform. I am hitting the same bug of ATA Failure Regression reported in past. (https://patchwork.kernel.org/patch/6875661/) or http://lkml.iu.edu/hypermail/linux/kernel/1507.3/01961.html I am newbie to this and because of this Ubuntu 16.04 is not booting. If disable IOMMU and MSI obviously it works but that is not solution. Even when I bisected it boiled to same place which was mentioned in past discussion. Now my question is there anything required to be done for this future platform in order to make above patch working because it is already present in Ubuntu 16.04 kernel i.e. 4.4 kernel. Please help me to debug this issue. Looking forward to hear from you. Thanks and Regards Nehal Shah
ATA failure regression
Hi Jiang, Currently I am working on AMD future platform. I am hitting the same bug of ATA Failure Regression reported in past. (https://patchwork.kernel.org/patch/6875661/) or http://lkml.iu.edu/hypermail/linux/kernel/1507.3/01961.html I am newbie to this and because of this Ubuntu 16.04 is not booting. If disable IOMMU and MSI obviously it works but that is not solution. Even when I bisected it boiled to same place which was mentioned in past discussion. Now my question is there anything required to be done for this future platform in order to make above patch working because it is already present in Ubuntu 16.04 kernel i.e. 4.4 kernel. Please help me to debug this issue. Looking forward to hear from you. Thanks and Regards Nehal Shah
ACPI/APD Making Module
Hi, Current implementation of acpi_apd.c makes AMD I2C,GPIO and UART from acpi devices to platform devices. This is done as part of boot sequence. For some reason i would like to make it kernel module. The current implementation calls acpi_apd_create_device as part of attach callback. Now this entire process is part of acpi bus scan functionality. Can someone please help me if i can make this file into kernel module and still get the same functionality. Thanks Nehal
