linux-kernel - Re: [PATCH] soc: hisilicon: Support HCCS driver on Kunpeng SoC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 25 Apr 2023 08:08:08 +0200
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Huisong Li" <lihuisong@...wei.com>,
        "Bjorn Andersson" <andersson@...nel.org>,
        "Matthias Brugger" <matthias.bgg@...il.com>,
        "AngeloGioacchino Del Regno" 
        <angelogioacchino.delregno@...labora.com>,
        "Shawn Guo" <shawnguo@...nel.org>
Cc:     linux-kernel@...r.kernel.org, soc@...nel.org,
        wanghuiqiang@...wei.com, tanxiaofei@...wei.com,
        liuyonglong@...wei.com, huangdaode@...wei.com,
        linux-acpi@...r.kernel.org, "Len Brown" <lenb@...nel.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        devicetree@...r.kernel.org, "Rob Herring" <robh+dt@...nel.org>,
        "Frank Rowand" <frowand.list@...il.com>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH] soc: hisilicon: Support HCCS driver on Kunpeng SoC

On Tue, Apr 25, 2023, at 05:04, lihuisong (C) wrote:
> 在 2023/4/24 16:09, Arnd Bergmann 写道:
>> On Mon, Apr 24, 2023, at 09:30, Huisong Li wrote:

>>         depends on ACPI
>>         depends on (ARM64 && ARCH_HISI) || COMPILE_TEST
> What do you think of adjusting it as below?
> menu "Hisilicon SoC drivers"
>      depends on ARCH_HISI || COMPILE_TEST
>
> config KUNPENG_HCCS
>      depends on ACPI
>      depends on ARM64 || COMPILE_TEST

Yes, that's perfect.

>>
>>> +
>>> +#include "kunpeng_hccs.h"
>>> +
>>> +/* PCC defines */
>>> +#define HCCS_PCC_SIGNATURE_MASK		0x50434300
>>> +#define HCCS_PCC_STATUS_CMD_COMPLETE	BIT(0)
>> Should these perhaps be in include/acpi/pcc.h? The 0x50434300
>> number is just "PCC\0", so it appears to not be HCCS specific.
> This is a PCC signature. As stated in the APCI,
> "The signature of a subspace is computed by a bitwiseor of the value 
> 0x50434300
> with the subspace ID. For example, subspace 3 has the signature 0x50434303."
>
> I am not sure if all driver need to use this fixed signature mask.
> As far as I know, cppc_acpi.c didn't use this signature and 
> xgene-hwmon.c used another mask defined in its driver.
> So I place it here.

I would still put it into the generic header, but it doesn't
really matter much, so do it whichever way you prefer. No need
for a separate patch if you decide to use the global header,
it can just be part of your normal patch.

>>> +
>>> +static int hccs_get_device_property(struct hccs_dev *hdev)
>>> +{
>>> +	struct device *dev = hdev->dev;
>>> +
>>> +	if (device_property_read_u32(dev, "device-flags", &hdev->flags)) {
>>> +		dev_err(hdev->dev, "no device-flags property.\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	if (device_property_read_u8(dev, "pcc-type", &hdev->type)) {
>>> +		dev_err(hdev->dev, "no pcc-type property.\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	if (device_property_read_u32(dev, "pcc-chan-id", &hdev->chan_id)) {
>>> +		dev_err(hdev->dev, "no pcc-channel property.\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	hdev->intr_mode = hccs_get_bit(hdev->flags, HCCS_DEV_FLAGS_INTR_B);
>>> +	if (!hccs_dev_property_supported(hdev))
>>> +		return -EOPNOTSUPP;
>>> +
>> Where are the device properties documented? I'm never quite sure how
>> to handle these for ACPI-only drivers, since we don't normally have the
>> bindings in Documentation/devicetree/bindings/, but it feels like there
>> should be some properly reviewed document somewhere else.
> These are ACPI-only, instead of DT.
> I will add a comment here as Krzysztof suggested.

I understand that they are ACPI-only, what I'm more interested here is
the general question of how we should document them, to ensure these
are handled consistently across drivers.

>>> --- /dev/null
>>> +++ b/drivers/soc/hisilicon/kunpeng_hccs.h
>>> @@ -0,0 +1,204 @@
>>> +/* SPDX-License-Identifier: GPL-2.0+ */
>>> +/* Copyright (c) 2023 Hisilicon Limited. */
>>> +
>>> +#ifndef __KUNPENG_HCCS_H__
>>> +#define __KUNPENG_HCCS_H__
>> Are you planning to add more drivers that share this file? If not,
>> just fold the contents into the driver itself.
> Yes, we will add more drivers in this file.

Ok.


       Arnd