[PATCH v3 0/2] iommu/smmu-v3: Workaround for hisilicon 161010801 erratum(reserve HW MSI)
by shameer
On certain HiSilicon platforms (Hip06/Hip07) the GIC ITS and
PCIe RC deviates from the standard implementation and this breaks
PCIe MSI functionality when SMMU is enabled.
The HiSilicon erratum 161010801 describes this limitation of certain
HiSilicon platforms to support the SMMU mappings for MSI transactions.
On these platforms GICv3 ITS translator is presented with the deviceID
by extending the MSI payload data to 64 bits to include the deviceID.
Hence, the PCIe controller on this platforms has to differentiate the
MSI payload against other DMA payload and has to modify the MSI payload.
This basically makes it difficult for this platforms to have a SMMU
translation for MSI.
This patch implements a ACPI table based quirk to reserve the hw msi
regions in the smmu-v3 driver which means these address regions will
not be translated and will be excluded from iova allocations.
To implement this quirk, the following changes are incorporated:
1. Added a generic helper function to IORT code to retrieve and
reserve the HW ITS address regions.
2. Added quirk to SMMUv3 to reserve HW ITS address regions based
on IORT SMMUv3 model.
This is based on the following patches:
1. https://patchwork.kernel.org/patch/9740733/
2. https://patchwork.kernel.org/patch/9730491/
Thanks,
Shameer
Changelog:
v2 --> v3
Addressed comments from Lorenzo and Robin:
-Removed dev_is_pci() check in smmuV3 driver.
-Don't treat device not having an ITS mapping as an error in
iort helper function.
v1 --> v2
-patch 2/2: Invoke iort helper fn based on fwnode type(acpi).
RFCv2 -->PATCH
-Incorporated Lorenzo's review comments.
RFC v1 --> RFC v2
Based on Robin's review comments,
-Removed the generic erratum framework.
-Using IORT/MADT tables to retrieve the ITS base addr instead
of vendor specific CSRT table.
shameer (2):
acpi:iort: Add an IORT helper function to reserve HW ITS address
regions for IOMMU drivers
iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801
drivers/acpi/arm64/iort.c | 91 ++++++++++++++++++++++++++++++++++++++--
drivers/iommu/arm-smmu-v3.c | 30 +++++++++++--
drivers/irqchip/irq-gic-v3-its.c | 3 +-
include/linux/acpi_iort.h | 7 +++-
4 files changed, 122 insertions(+), 9 deletions(-)
--
1.9.1
4 years
Re: [Devel] [PATCH] ACPI: SPCR: Use access width to determine mmio usage
by Rafael J. Wysocki
On Tue, Jun 20, 2017 at 11:27 PM, Jon Mason <jon.mason(a)broadcom.com> wrote:
> On Tue, Jun 20, 2017 at 2:33 PM, Loc Ho <lho(a)apm.com> wrote:
>> Hi Jon,
>>
>>>
>>> >> >>>>>> The current SPCR code does not check the access width of the mmio, and
>>> >> >>>>>> uses a default of 8bit register accesses. This prevents devices that
>>> >> >>>>>> only do 16 or 32bit register accesses from working. By simply checking
>>> >> >>>>>> this field and setting the mmio string appropriately, this issue can be
>>> >> >>>>>> corrected. To prevent any legacy issues, the code will default to 8bit
>>> >> >>>>>> accesses if the value is anything but 16 or 32.
>>> >> >>>>>
>>> >> >>>>> Thanks for this. Just as an FYI I've a running discussion with Microsoft
>>> >> >>>>> about defining additional UART subtypes in the DBG2 for special case
>>> >> >>>>> UARTs. Specifically, I want to address AppliedMicro's special 8250 dw IP
>>> >> >>>>> that also has a non-standard clock. At this time, there is general
>>> >> >>>>> agreement to use the access width for some cases rather than defining
>>> >> >>>>> yet more subtypes - so your patch is good.
>>> >> >>>>>
>>> >> >>>>> Loc/Applied: please track this thread, incorporate feedback, and also
>>> >> >>>>> track the other general recent discussions of 8250 dw from this week.
>>> >> >>>>
>>> >> >>>> Thanks for forward me this patch. This patch does not work with X-Gene
>>> >> >>>> v1 and v2 SoC's. As BIOS SPCR encodes these info as:
>>> >> >>>>
>>> >> >>>> Bit Width: 32
>>> >> >>>> Bit Offset: 0
>>> >> >>>> Encoded Access Width: 01 (Byte Access)
>>> >> >>>>
>>> >> >>>> With this patch, it would use the "mmio" instead the "mmio32" as with
>>> >> >>>> this patch - https://patchwork.kernel.org/patch/9460959
>>> >> >>>
>>> >> >>> I think this is why we need the DBG2 subtype for Applied X-Gene1. I'm
>>> >> >>> hoping the update to the SPCR/DBG2 spec is done soon.
>>> >> >>
>>> >> >> We can't rely on the BIOS change to support this new subtype as we
>>> >> >> have system that is already in production deployment. When these
>>> >> >> system upgrade to new version of the OS (stock, RHELSA, or whatever),
>>> >> >> they will break. We need the patch from
>>> >> >> https://patchwork.kernel.org/patch/9460959/ rolled upstream.
>>> >> >
>>> >> > There is no reason why the patch you reference cannot co-exist with
>>> >> > the one I am submitting here. In this case, my patch would set it to
>>> >> > mmio, then the patch you link above would reset it to mmio32.
>>> >> > Personally, I would recommend a big, fat comment on why this extra
>>> >> > step is necessary, but it should work as desired. Alternatively, we
>>> >> > could add some kind of quirk library (similar to
>>> >> > qdf2400_erratum_44_present) where the OEM/OEM Table ID is referenced
>>> >> > and workaround applied. Thoughts?
>>> >>
>>> >> That's was my first version but after seeing both versions, I think
>>> >> they are better solution as it works for more SoC's than just our. As
>>> >> you had suggested, we should apply your patch and
>>> >> https://patchwork.kernel.org/patch/9460959. The third patch -
>>> >> https://patchwork.kernel.org/patch/9462183/ - conflicts with your.
>>> >>
>>> >> Summary:
>>> >> 1. Applied your - https://lkml.org/lkml/2017/5/4/450
>>> >> 2. Applied this one - https://patchwork.kernel.org/patch/9460959/
>>> >>
>>> >> -Loc
>>> >
>>> > What if we simply applied the following (100% untested) patch to add
>>> > the quirk framework I was suggesting? It can be applied on top of the
>>> > patch I submitted previously.
>>>
>>> It is a bit more complex that this simple patch. How about this one
>>> (my original version). As for Jon Master question on McDivitt, not
>>> sure what they use for the ACPI table for SPCR. If they used our
>>> reference, then this might work for them too. This version would limit
>>> to just the existent firmware or until the SPCR table gets changed.
>>>
>>>
>>> tty: 8250: Workaround for APM X-Gene 8250 UART 32-alignment errata
>>>
>>> APM X-Gene verion 1 and 2 have an 8250 UART with its register
>>> aligned to 32-bit. The SPCR always assumes fully compatible
>>> 8250. This causes no console with ACPI boot as the console
>>> will not match X-Gene UART port due to the lack of mmio32
>>> option.
>>>
>>> Signed-off-by: Loc Ho <lho(a)apm.com>
>>> ---
>>> drivers/acpi/spcr.c | 21 +++++++++++++++++++++
>>> 1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/acpi/spcr.c b/drivers/acpi/spcr.c
>>> index 3afa8c1..77b45a0 100644
>>> --- a/drivers/acpi/spcr.c
>>> +++ b/drivers/acpi/spcr.c
>>> @@ -36,6 +36,25 @@ static bool qdf2400_erratum_44_present(struct
>>> acpi_table_header *h)
>>> return false;
>>> }
>>>
>>> +/*
>>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>>> + * register aligned to 32-bit. This function detects this errata condition.
>>> + */
>>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>>> +{
>>> + if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>>> + return false;
>>> +
>>> + if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE))
>>> + return false;
>>> +
>>> + if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>>> + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>>> + return true;
>>> +
>>> + return false;
>>> +}
>>> +
>>> /**
>>> * parse_spcr() - parse ACPI SPCR table and add preferred console
>>> *
>>> @@ -115,6 +134,8 @@ int __init parse_spcr(bool earlycon)
>>>
>>> if (qdf2400_erratum_44_present(&table->header))
>>> uart = "qdf2400_e44";
>>> + if (xgene_8250_erratum_present(table))
>>> + iotype = "mmio32";
>>>
>>> snprintf(opts, sizeof(opts), "%s,%s,0x%llx,%d", uart, iotype,
>>> table->serial_port.address, baud_rate);
>>>
>>
>> I didn't see a follow up email on this. What was the conclusion to
>> this patch series?
>
> I have not received an ack, nack, or gtfo from any of the maintainers
> of this file. Per
> ./scripts/get_maintainer.pl drivers/acpi/spcr.c
> "Rafael J. Wysocki" <rjw(a)rjwysocki.net> (supporter:ACPI)
> Len Brown <lenb(a)kernel.org> (supporter:ACPI)
> linux-acpi(a)vger.kernel.org (open list:ACPI)
> linux-kernel(a)vger.kernel.org (open list)
>
> Is there someone else I should be directing this patch through?
Generally, whoever is going to be affected by this change.
git seems to tell me that the spcr.c file went in through the tty tree
and Aleksey introduced it.
I can apply it if no one has any objections.
Thanks,
Rafael
4 years, 1 month
Re: [Devel] [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation
by Andy Shevchenko
On Fri, Jun 30, 2017 at 8:42 PM, Hans de Goede <hdegoede(a)redhat.com> wrote:
> On 30-06-17 19:40, Andy Shevchenko wrote:
>> On Fri, Jun 30, 2017 at 8:37 PM, Hans de Goede <hdegoede(a)redhat.com>
>> wrote:
>>> On 30-06-17 18:37, Andy Shevchenko wrote:
>>>> On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires
> Care to share that? Between me and Benjamin one of us can hopefully
> find the time to test / finish it (should be trivial really).
Not tested at all.
--
With Best Regards,
Andy Shevchenko
4 years, 1 month
Re: [Devel] [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation
by Andy Shevchenko
On Fri, Jun 30, 2017 at 8:37 PM, Hans de Goede <hdegoede(a)redhat.com> wrote:
> On 30-06-17 18:37, Andy Shevchenko wrote:
>> On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires
> ACPI i2c drivers still need an empty i2c_device_id table I've
> fixing this on my TODO but it has been buried in other stuff.
>
> Benjamin if (not saying you should, but if) you want to take a look at
> this, fixing the need for the empty table for ACPI devices should be
> easy. The problem is these lines in drivers/i2c/i2c-core.c:
> i2c_device_probe():
>
> /*
> * An I2C ID table is not mandatory, if and only if, a suitable
> Device
> * Tree match table entry is supplied for the probing device.
> */
> if (!driver->id_table &&
> !i2c_of_match_device(dev->driver->of_match_table, client))
> return -ENODEV;
>
> Which needs to be extended to also check for an ACPI match AFAIK
> you can NOT just replace this with i2c_device_match because that would
> break manually binding a driver through sysfs.
I have a stashed change for that, just have no time to look closer.
--
With Best Regards,
Andy Shevchenko
4 years, 1 month
Re: [Devel] [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation
by Andy Shevchenko
On Fri, Jun 30, 2017 at 6:57 PM, Benjamin Tissoires
<benjamin.tissoires(a)redhat.com> wrote:
> On Jun 29 2017 or thereabouts, Andy Shevchenko wrote:
>> On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
>> <benjamin.tissoires(a)redhat.com> wrote:
>> What devices (laptops, tablets) have it?
>> Surface 3. What else?
>
> So far, Surface 3 only. It's a Microsoft PNPId, so I guess they control
> which device has it. Maybe the model after the Surface 3 (reduced
> platform) will have such chip, but for now, it's unknown.
Please, extend introduction in commit message to state above.
>> > I couldn't manage to get the IRQ correctly triggered, so I am using a
>> > good old polling thread to check for changes.
>>
>> It might be
It seems I didn't finished the sentence here.
I though it might be actually ACPI event, GPE or direct IRQ (when GPIO
chip should not disable it, though if it's the case it likely a BIOS
bug for this hardware).
>> > + help
>> > + Select this option to enable support for ACPI operation
>> > + region of the Surface 3 battery platform driver.
>>
>> > +/*
>> > + * Supports for the power IC on the Surface 3 tablet.
>>
>> Shouldn't it go to drivers/acpi/pmic folder ?
>
> Already answered later in the thread, so yes, I'll move it there.
Actually Hans did a good point, so, feel free to use drivers/platform/x86.
>> And did you check if it have actual chip IP vendor name and model?
>> Most likely it's a TI (based?) solution.
>
> As mentioned, I have strictly no idea. I can not crack open the Surface
> 3 without breaking the warranty (I already had to return it once because
> the disk crashed).
We have one indeed cracked (screen is broken for good :-) ), so, I
would check what I can find there.
> And I do not find anything brand-related under Windows either:
> - it's called "Surface Platform Power Driver"
> - and the driver is provided by Microsoft
Fair enough.
>> > +static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
>> > +{
>>
>> > + memcpy(bix->serial, buf + 7, 3);
>> > + memcpy(bix->serial + 3, buf, 6);
>> > + bix->serial[9] = '\0';
>>
>> snprintf()?
>
> probably :)
I would do this until we have an evidence that it contains non-printable symbols
(or, in case you want to fix ahead, make the buffer 4 times bigger and use %*pE)
>> > + memcpy(bix->OEM, buf, 3);
>> > + bix->OEM[4] = '\0';
>>
>> snprintf() ?
Ditto.
>> > + snprintf(prefix, ARRAY_SIZE(prefix), "%s: ", bat0->name);
>>
>> > + prefix[127] = '\0';
>>
>> Why?
>
> Just me being paranoid in case the code doesn't follow the spec... Yeah, I'll
> remove it.
snprintf() despite n in the name takes care of terminating NUL.
>> > +static int mshw0011_probe(struct i2c_client *client,
>> > + const struct i2c_device_id *id)
>> > +{
>>
>> > + data->notify_version = version == MSHW0011_EV_2_5;
>>
>> 0x1ff as version sounds hmm suspicious.
>
> So after a little bit of digging, it appears those values were taken
> from the DSDT:
> https://bugzilla.kernel.org/attachment.cgi?id=187171 line 11694.
>
> It appears 0x3F is EV 2.1 and before, and 0x1FF is EV 2.5 and above.
> The returned value is not a version of the chip, just a flag to know
> which path we are taking in the DSM.
>
> The name is probably not the best.
63 and 511 looks too suspicious to be a version. Rather block size, a
mask or alike.
>> > +static const struct i2c_device_id mshw0011_id[] = {
>> > + { }
>> > +};
>> > +MODULE_DEVICE_TABLE(i2c, mshw0011_id);
>>
>> ->probe_new(), please.
>
> Correct
>
>>
>> If I2C framework is _still_ broken we need to fix that part.
>
> I haven't check, so let's see for v3.
Cc: Wolfram for v3 and ask him directly. Last time I checked it looks
like I2C core doesn't care about ACPI when ->probe_new() is used.
--
With Best Regards,
Andy Shevchenko
4 years, 1 month
Re: [Devel] [PATCH v2] ACPI: surface3_power: MSHW0011 rev-eng implementation
by Andy Shevchenko
+Cc: Hans (he might give some advice regarding to the below)
On Thu, Jun 29, 2017 at 3:10 PM, Benjamin Tissoires
<benjamin.tissoires(a)redhat.com> wrote:
> MSHW0011 replaces the battery firmware by using ACPI operation regions.
> The values have been obtained by reverse engineering, and are subject to
> errors. Looks like it works on overall pretty well.
What devices (laptops, tablets) have it?
Surface 3. What else?
> I couldn't manage to get the IRQ correctly triggered, so I am using a
> good old polling thread to check for changes.
It might be
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=106231
> +config ACPI_SURFACE3_POWER_OPREGION
> + tristate "Surface 3 battery platform operation region support"
depends on ACPI ?
> + help
> + Select this option to enable support for ACPI operation
> + region of the Surface 3 battery platform driver.
> +/*
> + * Supports for the power IC on the Surface 3 tablet.
Shouldn't it go to drivers/acpi/pmic folder ?
And did you check if it have actual chip IP vendor name and model?
Most likely it's a TI (based?) solution.
> + */
> +/*
> + * Further findings regarding the 2 chips declared in the MSHW0011 are:
> + * - there are 2 chips declared:
> + * . 0x22 seems to control the ADP1 line status (and probably the charger)
> + * . 0x55 controls the battery directly
> + * - the battery chip uses a SMBus protocol (using plain SMBus allows non
> + * destructive commands):
> + * . the commands/registers used are in the range 0x00..0x7F
> + * . if bit 8 (0x80) is set in the SMBus command, the returned value is the
> + * same as when it is not set. There is a high chance this bit is the
> + * read/write
> + * . the various registers semantic as been deduced by observing the register
> + * dumps.
All of this sounds familiar if look at what Hans discovered while was
doing INT33FE support.
Hans, does above ring any bell to you?
> + */
> +static bool dump_registers;
> +module_param_named(dump_registers, dump_registers, bool, 0644);
> +MODULE_PARM_DESC(dump_registers,
> + "Dump the SMBus register at probe (debugging only).");
I'm not a fan of module parameter. Why not to use debugfs?
> +#define ACPI_BATTERY_STATE_DISCHARGING 0x1
> +#define ACPI_BATTERY_STATE_CHARGING 0x2
> +#define ACPI_BATTERY_STATE_CRITICAL 0x4
BIT() ?
> +#define MSHW0011_EV_2_5 0x1ff
Is it mask? GENMASK() then.
> +
> +static int mshw0011_i2c_read_block(struct i2c_client *client, u8 reg, u8 *buf,
> + int len)
> +{
> + int status, i;
> +
> + for (i = 0; i < len; i++) {
> + status = i2c_smbus_read_byte_data(client, reg + i);
> + if (status < 0) {
> + buf[i] = 0xff;
> + continue;
> + }
Hmm... This looks weird. Why can you read entire block at once?
> +
> + buf[i] = (u8)status;
> + }
> +
> + return 0;
> +}
> +static int
> +mshw0011_notify(struct mshw0011_data *cdata, u8 arg1, u8 arg2,
> + unsigned int *ret_value)
> +{
> + static const uuid_le mshw0011_guid =
guid_t, please :-)
> + GUID_INIT(0x3F99E367, 0x6220, 0x4955,
> + 0x8B, 0x0F, 0x06, 0xEF, 0x2A, 0xE7, 0x94, 0x12);
> + *ret_value = 0;
> + for (i = 0; i < obj->buffer.length; i++)
> + *ret_value |= obj->buffer.pointer[i] << (i * 8);
> +
> + ACPI_FREE(obj);
> + return 0;
> +}
> +static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
> +{
> + memcpy(bix->serial, buf + 7, 3);
> + memcpy(bix->serial + 3, buf, 6);
> + bix->serial[9] = '\0';
snprintf()?
> + bix->cycle_count = le16_to_cpu(ret);
non-x86 ?
> + memcpy(bix->OEM, buf, 3);
> + bix->OEM[4] = '\0';
snprintf() ?
> +
> + return 0;
> +}
> +static int mshw0011_bst(struct mshw0011_data *cdata, struct bst *bst)
> +{
> + struct i2c_client *client = cdata->bat0;
> + int rate, capacity, voltage, state;
> + s16 tmp;
> +
> + rate = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_RATE);
> + if (rate < 0)
> + return rate;
> +
> + capacity = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CAPACITY);
> + if (capacity < 0)
> + return capacity;
> +
> + voltage = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_VOLTAGE);
> + if (voltage < 0)
> + return voltage;
> +
> + tmp = le16_to_cpu(rate);
Do we need that on x86?
> + bst->battery_present_rate = abs((s32)tmp);
> +
> + state = 0;
> + if ((s32) tmp > 0)
See above, perhaps using rate directly.
> + state |= ACPI_BATTERY_STATE_CHARGING;
> + else if ((s32) tmp < 0)
Ditto.
> + bst->battery_remaining_capacity = le16_to_cpu(capacity);
> + bst->battery_present_voltage = le16_to_cpu(voltage);
non-x86 ?
> +}
> +
ret = 0; ?
...
> + switch (gsb->cmd.arg1) {
> + case MSHW0011_CMD_BAT0_STA:
> + ret = 0;
See above.
> + break;
> + case MSHW0011_CMD_BAT0_BIX:
> + ret = mshw0011_bix(cdata, &gsb->bix);
> + break;
> + case MSHW0011_CMD_BAT0_BTP:
> + ret = 0;
Ditto.
> + cdata->trip_point = gsb->cmd.arg2;
> + break;
> + case MSHW0011_CMD_BAT0_BST:
> + ret = mshw0011_bst(cdata, &gsb->bst);
> + break;
> + default:
> + pr_info("command(0x%02x) is not supported.\n", gsb->cmd.arg1);
> + ret = AE_BAD_PARAMETER;
> + goto err;
> + }
> +
> + out:
> + gsb->ret = status;
> + gsb->status = 0;
> +
> + err:
> + ACPI_FREE(ares);
> + return ret;
> +}
> +
> +static int mshw0011_install_space_handler(struct i2c_client *client)
> +{
> + acpi_handle handle;
> + struct mshw0011_handler_data *data;
> + acpi_status status;
> +
> + handle = ACPI_HANDLE(&client->dev);
> +
> + if (!handle)
> + return -ENODEV;
> +
> + data = kzalloc(sizeof(struct mshw0011_handler_data),
> + GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->client = client;
> + status = acpi_bus_attach_private_data(handle, (void *)data);
> + if (ACPI_FAILURE(status)) {
> + kfree(data);
> + return -ENOMEM;
> + }
> +
> + status = acpi_install_address_space_handler(handle,
> + ACPI_ADR_SPACE_GSBUS,
> + &mshw0011_space_handler,
> + NULL,
> + data);
> + if (ACPI_FAILURE(status)) {
> + dev_err(&client->dev, "Error installing i2c space handler\n");
> + acpi_bus_detach_private_data(handle);
> + kfree(data);
> + return -ENOMEM;
> + }
> +
> + acpi_walk_dep_device_list(handle);
> + return 0;
> +}
> +
> +static void mshw0011_remove_space_handler(struct i2c_client *client)
> +{
> + acpi_handle handle = ACPI_HANDLE(&client->dev);
> + struct mshw0011_handler_data *data;
> + acpi_status status;
> +
> + if (!handle)
> + return;
> +
> + acpi_remove_address_space_handler(handle,
> + ACPI_ADR_SPACE_GSBUS,
> + &mshw0011_space_handler);
> +
> + status = acpi_bus_get_private_data(handle, (void **)&data);
> + if (ACPI_SUCCESS(status))
> + kfree(data);
> +
> + acpi_bus_detach_private_data(handle);
> +}
> +
> +static int acpi_find_i2c(struct acpi_resource *ares, void *data)
> +{
> + struct mshw0011_lookup *lookup = data;
> +
> + if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> + return 1;
> +
> + if (lookup->n++ == lookup->index && !lookup->addr)
> + lookup->addr = ares->data.i2c_serial_bus.slave_address;
> +
> + return 1;
> +}
> +
> +static int mshw0011_i2c_resource_lookup(struct mshw0011_data *cdata,
> + unsigned int index)
> +{
> + struct i2c_client *client = cdata->adp1;
> + struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> + struct mshw0011_lookup lookup = {
> + .cdata = cdata,
> + .index = index,
> + };
> + struct list_head res_list;
> + int ret;
> +
> + INIT_LIST_HEAD(&res_list);
> +
> + ret = acpi_dev_get_resources(adev, &res_list, acpi_find_i2c, &lookup);
> + if (ret < 0)
> + return ret;
> +
> + acpi_dev_free_resource_list(&res_list);
> +
> + if (!lookup.addr)
> + return -ENOENT;
> +
> + return lookup.addr;
> +}
Strange you have above functions here. It's a copy paste from I2C
core. Please, think about way of deduplicating it.
> +static void mshw0011_dump_registers(struct i2c_client *client,
> + struct i2c_client *bat0, u8 end_register)
> +{
> + char *rd_buf;
> + char prefix[128];
128 is too way big for a prefix.
> + unsigned int length = end_register + 1;
> + int error;
> +
> + snprintf(prefix, ARRAY_SIZE(prefix), "%s: ", bat0->name);
> + prefix[127] = '\0';
Why?
> + rd_buf = kzalloc(length, GFP_KERNEL);
> + error = mshw0011_i2c_read_block(bat0, 0, rd_buf, length);
> + print_hex_dump(KERN_INFO, prefix, DUMP_PREFIX_OFFSET, 16, 1,
> + rd_buf, length, true);
If you switch to debugfs it makes things a bit more easier to handle I think.
> +
> + kfree(rd_buf);
> +}
> +static int mshw0011_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + data->notify_version = version == MSHW0011_EV_2_5;
0x1ff as version sounds hmm suspicious.
> +static const struct i2c_device_id mshw0011_id[] = {
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, mshw0011_id);
->probe_new(), please.
If I2C framework is _still_ broken we need to fix that part.
> +#ifdef CONFIG_ACPI
Is it going to be non-ACPI at all? See my proposal to Kconfig as well.
> + .acpi_match_table = ACPI_PTR(mshw0011_acpi_match),
ACPI_PTR() might be gone
--
With Best Regards,
Andy Shevchenko
4 years, 1 month
ACPICA version 20170629 released
by Moore, Robert
29 June 2017. Summary of changes for version 20170629:
This release is available at https://acpica.org/downloads
1) ACPICA kernel-resident subsystem:
Tables: Implemented a deferred ACPI table verification. This is useful for operating systems where the tables cannot be verified in the early initialization stage due to early memory mapping limitations on some architectures. Lv Zheng.
Tables: Removed the signature validation for dynamically loaded tables. Provides compatibility with other ACPI implementations. Previously, only SSDT tables were allowed, as per the ACPI specification. Now, any table signature can be used via the Load() operator. Lv Zheng.
Tables: Fixed several mutex issues that could cause errors during table acquisition. Lv Zheng.
Tables: Fixed a problem where an ACPI warning could be generated if a null pointer was passed to the AcpiPutTable interface. Lv Zheng.
Tables: Added a mechanism to handle imbalances for the AcpiGetTable and AcpiPutTable interfaces. This applies to the "late stage" table loading when the use of AcpiPutTable is no longer required (since the system memory manager is fully running and available). Lv Zheng.
Fixed/Reverted a regression during processing of resource descriptors that contain only a single EndTag. Fixes an AE_AML_NO_RESOURCE_END_TAG exception in this case.
Headers: IORT/SMMU support: Updated the SMMU models for Revision C of the I/O Remapping specification. Robin Murphy <robin.murphy(a)arm.com>
Interpreter: Fixed a possible fault if an Alias operator with an invalid or duplicate target is encountered during Alias creation in AcpiExCreateAlias. Alex James <theracermaster(a)gmail.com>
Added an option to use designated initializers for function pointers. Kees Cook <keescook(a)google.com>
2) iASL Compiler/Disassembler and Tools:
iASL: Allow compilation of External declarations with target pathnames that refer to existing named objects within the table. Erik Schmauss.
iASL: Fixed a regression when compiling FieldUnits. Fixes an error if a FieldUnit name also is declared via External in the same table. Erik Schmauss.
iASL: Allow existing scope names within pathnames used in External statements. For example:
External (ABCD.EFGH) // ABCD exists, but EFGH is truly external
Device (ABCD)
iASL: IORT ACPI table: Implemented changes required to decode the new Proximity Domain for the SMMUv3 IORT. Disassembler and Data Table compiler. Ganapatrao Kulkarni <ganapatrao.kulkarni(a)cavium.com>
Disassembler: Don't abort disassembly on errors from External() statements. Erik Schmauss.
Disassembler: fixed a possible fault when one of the Create*Field operators references a Resource Template. ACPICA Bugzilla 1396.
iASL: In the source code, resolved some naming inconsistences across the parsing support. Fixes confusion between "Parse Op" and "Parse Node". Adds a new file, aslparseop.c
4 years, 1 month
Re: [Devel] [PATCH 0/2] Make cppc acpi driver aware of pcc subspace ids
by Rafael J. Wysocki
On Tuesday, June 13, 2017 02:17:09 PM George Cherian wrote:
> The current cppc acpi driver works with only one pcc subspace id.
> It maintains and registers only one pcc channel even if the acpi table has
> different pcc subspace ids.
>
> As per ACPI 6.2 spec all PCC registers, for all processors in the same
> performance domain (as defined by _PSD), must be defined to be in the same
> subspace. The series tries to address the same by making cppc acpi driver
> aware of multiple possible pcc subspace ids.
>
> Patch 1 : In preparation to share the MAX_PCC_SUBSPACE definition with cppc acpi
> driver
> Patch 2 : Make the cppc acpi driver aware of multiple pcc subspace ids.
>
>
> George Cherian (2):
> mailbox: PCC: Move the MAX_PCC_SUBSPACES definition to header file
> ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids
>
> drivers/acpi/cppc_acpi.c | 179 +++++++++++++++++++++++++----------------------
> drivers/mailbox/pcc.c | 1 -
> include/acpi/pcc.h | 1 +
> 3 files changed, 97 insertions(+), 84 deletions(-)
I need Ashwing and Prashanth to look at this series and let me know what they
think.
Thanks,
Rafael
4 years, 1 month
[RESEND PATCH v9 3/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126
by Geetha sowjanya
From: Geetha Sowjanya <geethasowjanya.akula(a)cavium.com>
Cavium ThunderX2 SMMU doesn't support MSI and also doesn't have unique irq
lines for gerror, eventq and cmdq-sync.
New named irq "combined" is set as a errata workaround, which allows to
share the irq line by register single irq handler for all the interrupts.
Signed-off-by: Geetha sowjanya <gakula(a)caviumnetworks.com>
---
Documentation/arm64/silicon-errata.txt | 1 +
.../devicetree/bindings/iommu/arm,smmu-v3.txt | 6 +
drivers/acpi/arm64/iort.c | 57 ++++++++---
drivers/iommu/arm-smmu-v3.c | 100 ++++++++++++++-----
4 files changed, 121 insertions(+), 43 deletions(-)
diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
index 4693a32..42422f6 100644
--- a/Documentation/arm64/silicon-errata.txt
+++ b/Documentation/arm64/silicon-errata.txt
@@ -63,6 +63,7 @@ stable kernels.
| Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 |
| Cavium | ThunderX SMMUv2 | #27704 | N/A |
| Cavium | ThunderX2 SMMUv3| #74 | N/A |
+| Cavium | ThunderX2 SMMUv3| #126 | N/A |
| | | | |
| Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 |
| | | | |
diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
index e7855cf..c9abbf3 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
@@ -26,6 +26,12 @@ the PCIe specification.
* "priq" - PRI Queue not empty
* "cmdq-sync" - CMD_SYNC complete
* "gerror" - Global Error activated
+ * "combined" - The combined interrupt is optional,
+ and should only be provided if the
+ hardware supports just a single,
+ combined interrupt line.
+ If provided, then the combined interrupt
+ will be used in preference to any others.
- #iommu-cells : See the generic IOMMU binding described in
devicetree/bindings/pci/pci-iommu.txt
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index bd97b7d..21c1ed3 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -828,6 +828,18 @@ static int __init arm_smmu_v3_count_resources(struct acpi_iort_node *node)
return num_res;
}
+static bool arm_smmu_v3_is_combined_irq(struct acpi_iort_smmu_v3 *smmu)
+{
+ /*
+ * Cavium ThunderX2 implementation doesn't not support unique
+ * irq line. Use single irq line for all the SMMUv3 interrupts.
+ */
+ if (smmu->model == ACPI_IORT_SMMU_V3_CAVIUM_CN99XX)
+ return true;
+
+ return false;
+}
+
static unsigned long arm_smmu_v3_resource_size(struct acpi_iort_smmu_v3 *smmu)
{
/*
@@ -855,26 +867,39 @@ static void __init arm_smmu_v3_init_resources(struct resource *res,
res[num_res].flags = IORESOURCE_MEM;
num_res++;
+ if (arm_smmu_v3_is_combined_irq(smmu)) {
+ int irq = smmu->event_gsiv;
- if (smmu->event_gsiv)
- acpi_iort_register_irq(smmu->event_gsiv, "eventq",
- ACPI_EDGE_SENSITIVE,
- &res[num_res++]);
-
- if (smmu->pri_gsiv)
- acpi_iort_register_irq(smmu->pri_gsiv, "priq",
- ACPI_EDGE_SENSITIVE,
- &res[num_res++]);
-
- if (smmu->gerr_gsiv)
- acpi_iort_register_irq(smmu->gerr_gsiv, "gerror",
+ if (!(irq == smmu->pri_gsiv && irq == smmu->gerr_gsiv &&
+ irq == smmu->sync_gsiv)) {
+ pr_warn(FW_BUG "Incosistent combined GSIV configuration\n");
+ return;
+ }
+ acpi_iort_register_irq(smmu->event_gsiv, "combined",
ACPI_EDGE_SENSITIVE,
&res[num_res++]);
+ } else {
- if (smmu->sync_gsiv)
- acpi_iort_register_irq(smmu->sync_gsiv, "cmdq-sync",
- ACPI_EDGE_SENSITIVE,
- &res[num_res++]);
+ if (smmu->event_gsiv)
+ acpi_iort_register_irq(smmu->event_gsiv, "eventq",
+ ACPI_EDGE_SENSITIVE,
+ &res[num_res++]);
+
+ if (smmu->pri_gsiv)
+ acpi_iort_register_irq(smmu->pri_gsiv, "priq",
+ ACPI_EDGE_SENSITIVE,
+ &res[num_res++]);
+
+ if (smmu->gerr_gsiv)
+ acpi_iort_register_irq(smmu->gerr_gsiv, "gerror",
+ ACPI_EDGE_SENSITIVE,
+ &res[num_res++]);
+
+ if (smmu->sync_gsiv)
+ acpi_iort_register_irq(smmu->sync_gsiv, "cmdq-sync",
+ ACPI_EDGE_SENSITIVE,
+ &res[num_res++]);
+ }
}
static bool __init arm_smmu_v3_is_coherent(struct acpi_iort_node *node)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 2dea4a9..fd5a48c 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -605,6 +605,7 @@ struct arm_smmu_device {
struct arm_smmu_priq priq;
int gerr_irq;
+ int combined_irq;
unsigned long ias; /* IPA */
unsigned long oas; /* PA */
@@ -1314,6 +1315,24 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev)
return IRQ_HANDLED;
}
+static irqreturn_t arm_smmu_combined_irq_thread(int irq, void *dev)
+{
+ struct arm_smmu_device *smmu = dev;
+
+ arm_smmu_evtq_thread(irq, dev);
+ if (smmu->features & ARM_SMMU_FEAT_PRI)
+ arm_smmu_priq_thread(irq, dev);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t arm_smmu_combined_irq_handler(int irq, void *dev)
+{
+ arm_smmu_gerror_handler(irq, dev);
+ arm_smmu_cmdq_sync_handler(irq, dev);
+ return IRQ_WAKE_THREAD;
+}
+
/* IO_PGTABLE API */
static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
{
@@ -2230,18 +2249,9 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
devm_add_action(dev, arm_smmu_free_msis, dev);
}
-static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
+static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
{
- int ret, irq;
- u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
-
- /* Disable IRQs first */
- ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
- ARM_SMMU_IRQ_CTRLACK);
- if (ret) {
- dev_err(smmu->dev, "failed to disable irqs\n");
- return ret;
- }
+ int irq, ret;
arm_smmu_setup_msis(smmu);
@@ -2284,10 +2294,41 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
if (ret < 0)
dev_warn(smmu->dev,
"failed to enable priq irq\n");
- else
- irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
}
}
+}
+
+static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
+{
+ int ret, irq;
+ u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
+
+ /* Disable IRQs first */
+ ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
+ ARM_SMMU_IRQ_CTRLACK);
+ if (ret) {
+ dev_err(smmu->dev, "failed to disable irqs\n");
+ return ret;
+ }
+
+ irq = smmu->combined_irq;
+ if (irq) {
+ /*
+ * Cavium ThunderX2 implementation doesn't not support unique
+ * irq lines. Use single irq line for all the SMMUv3 interrupts.
+ */
+ ret = devm_request_threaded_irq(smmu->dev, irq,
+ arm_smmu_combined_irq_handler,
+ arm_smmu_combined_irq_thread,
+ IRQF_ONESHOT,
+ "arm-smmu-v3-combined-irq", smmu);
+ if (ret < 0)
+ dev_warn(smmu->dev, "failed to enable combined irq\n");
+ } else
+ arm_smmu_setup_unique_irqs(smmu);
+
+ if (smmu->features & ARM_SMMU_FEAT_PRI)
+ irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
/* Enable interrupt generation on the SMMU */
ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
@@ -2724,22 +2765,27 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
return PTR_ERR(smmu->base);
/* Interrupt lines */
- irq = platform_get_irq_byname(pdev, "eventq");
- if (irq > 0)
- smmu->evtq.q.irq = irq;
- irq = platform_get_irq_byname(pdev, "priq");
+ irq = platform_get_irq_byname(pdev, "combined");
if (irq > 0)
- smmu->priq.q.irq = irq;
+ smmu->combined_irq = irq;
+ else {
+ irq = platform_get_irq_byname(pdev, "eventq");
+ if (irq > 0)
+ smmu->evtq.q.irq = irq;
- irq = platform_get_irq_byname(pdev, "cmdq-sync");
- if (irq > 0)
- smmu->cmdq.q.irq = irq;
+ irq = platform_get_irq_byname(pdev, "priq");
+ if (irq > 0)
+ smmu->priq.q.irq = irq;
- irq = platform_get_irq_byname(pdev, "gerror");
- if (irq > 0)
- smmu->gerr_irq = irq;
+ irq = platform_get_irq_byname(pdev, "cmdq-sync");
+ if (irq > 0)
+ smmu->cmdq.q.irq = irq;
+ irq = platform_get_irq_byname(pdev, "gerror");
+ if (irq > 0)
+ smmu->gerr_irq = irq;
+ }
/* Probe the h/w */
ret = arm_smmu_device_hw_probe(smmu);
if (ret)
--
1.7.1
4 years, 1 month
[PATCH v4 0/2] acpi, gicv3-its, numa: Adding numa node mapping for ITS
by Ganapatrao Kulkarni
ACPI 6.2 have added SRAT subtable to define proximity domain for ITS
devices. This patchset updates acpi header file from acpica repo and
adds numa node mapping for ITS devices.
v4:
-Addressed review comments.
v3:
- Use static array to stash parsed data from srat its table.
v2:
- Incorporated comments from Lorenzo Pieralisi and Marc Zyngier.
v1: first patch
Ganapatrao Kulkarni (2):
ACPICA: ACPI 6.2: Add support for new SRAT subtable
acpi, gicv3-its, numa: Adding numa node mapping for gic-its units
drivers/irqchip/irq-gic-v3-its.c | 75 +++++++++++++++++++++++++++++++++++++++-
include/acpi/actbl1.h | 12 ++++++-
2 files changed, 85 insertions(+), 2 deletions(-)
--
1.8.1.4
4 years, 1 month
