Re: [PATCH 02/16] dt-bindings: spi: Add bcmbca-hsspi controller support

From: William Zhang
Date: Thu Jan 12 2023 - 14:54:18 EST




On 01/12/2023 12:21 AM, Krzysztof Kozlowski wrote:
On 11/01/2023 19:44, William Zhang wrote:


On 01/11/2023 10:12 AM, Krzysztof Kozlowski wrote:
On 11/01/2023 19:04, William Zhang wrote:


On 01/11/2023 01:02 AM, Krzysztof Kozlowski wrote:
On 10/01/2023 23:18, Florian Fainelli wrote:
On 1/10/23 00:40, Krzysztof Kozlowski wrote:
No, it is discouraged in such forms. Family or IP block compatibles
should be prepended with a specific compatible. There were many issues
when people insisted on generic or family compatibles...

Otherwise we will have to have a compatible string with chip model for
each SoC even they share the same IP. We already have more than ten of
SoCs and the list will increase. I don't see this is a good solution too.

You will have to do it anyway even with generic fallback, so I don't get
what is here to gain... I also don't get why Broadcom should be here
special, different than others. Why it is not a good solution for
Broadcom SoCs but it is for others?

I saw a few other vendors like these qcom ones:
qcom,spi-qup.yaml
- qcom,spi-qup-v1.1.1 # for 8660, 8960 and 8064
- qcom,spi-qup-v2.1.1 # for 8974 and later
- qcom,spi-qup-v2.2.1 # for 8974 v2 and later
qcom,spi-qup.yaml
const: qcom,geni-spi

IP block version numbers are allowed when there is clear mapping between
version and SoCs using it. This is the case for Qualcomm because there
is such clear mapping documented and available for Qualcomm engineers
and also some of us (although not public).

I guess when individual who only has one particular board/chip and is
not aware of the IP family, it is understandable to use the chip
specific compatible string.

Family of devices is not a versioned IP block.

Would it be acceptable to define for instance:

- compatible = "brcm,bcm6868-hsspi", "brcm,bcmbca-hsspi";

Yes, this is perfectly valid. Although it does not solve William
concerns because it requires defining specific compatibles for all of
the SoCs.

Best regards,
Krzysztof

As I mentioned in another email, I would be okay to use these
compatibles to differentiate by ip rev and to conforms to brcm convention:
"brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.0", "brcm,bcmbca-hsspi";
"brcm,bcmXYZ-hsspi", "brcm,bcmbca-hsspi-v1.1", "brcm,bcmbca-hsspi";


Drop the version in such case, no benefits. I assume XYZ is the SoC
model, so for example 6868.

Yes XYZ is the SoC model

In the two drivers I included in this series, it will be bound to
brcm,bcmbca-hsspi-v1.0 (in additional to brcm,bcm6328-hsspi) and
brcm,bcmbca-hsspi-v1.1 respectively. This way we don't need to update
the driver with a new soc specific compatible whenever a new chips comes
out.

I don't understand why do you bring it now as an argument. You defined
before that your driver will bind to the generic bcmbca compatible, so
now it is not enough?

No as we are adding chip model specific info here. The existing driver
spi-bcm63xx-hsspi.c only binds to brcm,bcm6328-hsspi. This driver
supports all the chips with rev1.0 controller so I am using this 6328
string for other chips with v1.0 in the dts patch, which is not ideal.

Why? This is perfectly ideal and usual case. Why changing it?

Now I have to add more compatible to this driver and for each new chip
with 1.0 in the future if any.

Why you cannot use compatibility with older chipset?

IMHO it is really confusing that we have all the SoCs but have to bind to an antique SoC's spi controller compatible and people may think it is a mistake or typo when they don't know they are actually the same. I know there are usage like that but when we have clear knowledge of the IP block with rev info, I think it is much better to have a precise SoC model number and a general revision info in the compatible. As you know they are many usage of IP rev info in the compatible too. brcm,bcm6328-hsspi will stay so it does not break any existing dts reference to that.

Anyway if you still does not like this idea, I will drop the rev info and you have it your way.


Best regards,
Krzysztof

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature