Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH 02/16] dt-bindings: spi: Add bcmbca-hsspi controller support | From | William Zhang <> | Date | Tue, 10 Jan 2023 16:59:57 -0800 |
| |
On 01/10/2023 12:40 AM, Krzysztof Kozlowski wrote: > On 09/01/2023 20:13, William Zhang wrote: >> >> >> On 01/09/2023 12:56 AM, Krzysztof Kozlowski wrote: >>> On 09/01/2023 09:27, William Zhang wrote: >>>> Hi Krzysztof, >>>> >>>> On 01/08/2023 06:51 AM, Krzysztof Kozlowski wrote: >>>>> On 06/01/2023 21:07, William Zhang wrote: >>>>>> The new Broadcom Broadband BCMBCA SoCs includes a updated HSSPI >>>>>> controller. Add a new compatible string and required fields for the new >>>>>> driver. Also add myself and Kursad as the maintainers. >>>>>> >>>>>> Signed-off-by: William Zhang <william.zhang@broadcom.com> >>>>>> --- >>>>>> >>>>>> .../bindings/spi/brcm,bcm63xx-hsspi.yaml | 84 +++++++++++++++++-- >>>>>> 1 file changed, 78 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml >>>>>> index 45f1417b1213..56e69d4a1faf 100644 >>>>>> --- a/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml >>>>>> +++ b/Documentation/devicetree/bindings/spi/brcm,bcm63xx-hsspi.yaml >>>>>> @@ -4,22 +4,51 @@ >>>>>> $id: http://devicetree.org/schemas/spi/brcm,bcm63xx-hsspi.yaml# >>>>>> $schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>> >>>>>> -title: Broadcom BCM6328 High Speed SPI controller >>>>>> +title: Broadcom Broadband SoC High Speed SPI controller >>>>>> >>>>>> maintainers: >>>>>> + >>>>> >>>>> Drop blank line. >>>> will fix in v2. >>>> >>>>> >>>>>> + - William Zhang <william.zhang@broadcom.com> >>>>>> + - Kursad Oney <kursad.oney@broadcom.com> >>>>>> - Jonas Gorski <jonas.gorski@gmail.com> >>>>> >>>>>> >>>>>> +description: | >>>>>> + Broadcom Broadband SoC supports High Speed SPI master controller since the >>>>>> + early MIPS based chips such as BCM6328 and BCM63268. This controller was >>>>>> + carried over to recent ARM based chips, such as BCM63138, BCM4908 and BCM6858. >>>>>> + >>>>>> + It has a limitation that can not keep the chip select line active between >>>>>> + the SPI transfers within the same SPI message. This can terminate the >>>>>> + transaction to some SPI devices prematurely. The issue can be worked around by >>>>>> + either the controller's prepend mode or using the dummy chip select >>>>>> + workaround. This controller uses the compatible string brcm,bcm6328-hsspi. >>>>>> + >>>>>> + The newer SoCs such as BCM6756, BCM4912 and BCM6855 include an updated SPI >>>>>> + controller that add the capability to allow the driver to control chip select >>>>>> + explicitly. This solves the issue in the old controller. This new controller >>>>>> + uses the compatible string brcm,bcmbca-hsspi. >>>>>> + >>>>>> properties: >>>>>> compatible: >>>>>> - const: brcm,bcm6328-hsspi >>>>>> + enum: >>>>>> + - brcm,bcm6328-hsspi >>>>>> + - brcm,bcmbca-hsspi >>>>> >>>>> bca seems quite unspecific. Your description above mentions several >>>>> model numbers and "bca" is not listed as model. Compatibles cannot be >>>>> generic. >>>> "bca" is not model number, rather it is a group (broadband carrier >>>> access) of chip that share the same spi host controller IP. Agree it is >>>> not particularly specific but it differentiate from other broadcom spi >>>> controller ip used by other groups. We just don't have a specific name >>>> for this spi host controller but can we treat bcmbca as the ip name? >>> >>> 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. > >> But when company works on it, we have the >> visibility and access to all the chips and ip blocks in the family and >> IMHO it is very reasonable to use the IP family name for the same IP as >> these examples shows. > > No, because family of devices is not a versioned IP block. I wrote > before that families and wildcards are not allowed. > >> Are you saying these are not good example to >> follow? > > It's nothing related to your case. > >> What are the issues with generic or family compatibles? >> Could >> you please elaborate? > > They stop matching and some point and cause ABI breaks. We had several > cases where engineer believed "I have here family of devices" and then > later it turned out that the family is different. > > >> >>> >>> >>>> >>>>> >>>>>> >>>>>> reg: >>>>>> - maxItems: 1 >>>>>> + items: >>>>>> + - description: main registers >>>>>> + - description: miscellaneous control registers >>>>>> + minItems: 1 >>>>>> + >>>>>> + reg-names: >>>>>> + items: >>>>>> + - const: hsspi >>>>>> + - const: spim-ctrl >>>>> >>>>> This does not match reg >>>> Do you mean it does not match the description? >>> >>> No. reg can be 1 item but you state reg-names cannot. These are always >>> the same. If one is 1 item, second is as well. >>> >> I'll drop the "minItems: 1" from the reg property then. > > Then it won't be correct, because it would mean two items are required > always. Ah you are right. Add minItems: 1 for reg-name then. > >> >>>>> >>>>>> >>>>>> clocks: >>>>>> items: >>>>>> - - description: spi master reference clock >>>>>> - - description: spi master pll clock >>>>>> + - description: SPI master reference clock >>>>>> + - description: SPI master pll clock >>>>> >>>>> Really? You just added it in previous patch, didn't you? >>>> The previous patch was just word to word conversion of the text file. I >>>> will update that patch to include this change. >>>> >>>>> >>>>>> >>>>>> clock-names: >>>>>> items: >>>>>> @@ -29,12 +58,43 @@ properties: >>>>>> interrupts: >>>>>> maxItems: 1 >>>>>> >>>>>> + brcm,use-cs-workaround: >>>>>> + $ref: /schemas/types.yaml#/definitions/flag >>>>>> + description: | >>>>>> + Enable dummy chip select workaround for SPI transfers that can not be >>>>>> + supported by the default controller's prepend mode, i.e. delay or cs >>>>>> + change needed between SPI transfers. >>>>> >>>>> You need to describe what is the workaround. >>>> Will do. >>>>> >>>>>> + >>>>>> required: >>>>>> - compatible >>>>>> - reg >>>>>> - clocks >>>>>> - clock-names >>>>>> - - interrupts >>>>>> + >>>>>> +allOf: >>>>>> + - $ref: "spi-controller.yaml#" >>>>> >>>>> No quotes. How this is related to this patch? >>>> Will remove quote and put it in patch 1. >>>>> >>>>>> + - if: >>>>>> + properties: >>>>>> + compatible: >>>>>> + contains: >>>>>> + enum: >>>>>> + - brcm,bcm6328-hsspi >>>>>> + then: >>>>>> + properties: >>>>>> + reg: >>>>>> + minItems: 1 >>>>> >>>>> Drop. >>>>> >>>>> reg-names now do not match. >>>> Don't quite understand your comment. What do I need to drop and what is >>>> not matched? >>> >>> You need to add constraints for reg-names, same way as for reg. >>> Disallowing the reg-names also could work, but there won't be benefit in >>> it. Better to have uniform DTS. >>> >> I agree it is better to have the uniform DTS but the situation here is >> that the brcm,bcm6328-hsspi does not require reg name since there is >> only one register needed and it was already used in many chip dts for >> long time. If I enforce it to have the corresponding reg name, that > > No one told you to enforce to have a reg-names. > >> could potentially break the compatibility of those old device if the >> driver change to use reg name, right? > > How compatibility is broken by some optional, unrelated property? > I think I misunderstand what you said. You basically want the reg-name minItem/maxItem constraints for brcm,bcm6328-hsspi compatible as well so it is consistent for all the compatibles? I was confused and thought it is not needed as reg-name is not required for brcm,bcm6328-hsspi compatible.
> Best regards, > Krzysztof > [unhandled content-type:application/pkcs7-signature]
| ![\](/images/icornerr.gif) |