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

[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.

>>> +
>>> +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