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

["lihuisong (C)" <lihuisong@huawei.com>]

在 2023/4/25 14:08, Arnd Bergmann 写道:
> 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.
ok, keep it the way it is now.
>
>>>> +
>>>> +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.
These device properties are reported by ACPI table in firmware.
They are fixed in platform firmware. The user cannot modify them.
Do we need to document them?
>
>>>> --- /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
> .