| Message ID | 20230106200809.330769-3-william.zhang@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: bcm63xx-hsspi: driver and doc updates | expand |
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. > + - 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. > > 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 > > 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? > > 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. > + > required: > - compatible > - reg > - clocks > - clock-names > - - interrupts > + > +allOf: > + - $ref: "spi-controller.yaml#" No quotes. How this is related to this patch? > + - if: > + properties: > + compatible: > + contains: > + enum: > + - brcm,bcm6328-hsspi > + then: > + properties: > + reg: > + minItems: 1 Drop. reg-names now do not match. > + maxItems: 1 > + else: > + properties: > + reg: > + minItems: 2 > + maxItems: 2 > + reg-names: > + minItems: 2 > + maxItems: 2 > + brcm,use-cs-workaround: false > + required: > + - reg-names Best regards, Krzysztof
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? 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. > >> >> 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? > >> >> 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? > >> + maxItems: 1 >> + else: >> + properties: >> + reg: >> + minItems: 2 >> + maxItems: 2 >> + reg-names: >> + minItems: 2 >> + maxItems: 2 >> + brcm,use-cs-workaround: false >> + required: >> + - reg-names > Best regards, > Krzysztof >
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? > >> >>> >>> 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. >> >>> >>> 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. > Best regards, Krzysztof
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 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. 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. Are you saying these are not good example to follow? What are the issues with generic or family compatibles? Could you please elaborate? > > >> >>> >>>> >>>> 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. >>> >>>> >>>> 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 could potentially break the compatibility of those old device if the driver change to use reg name, right? >> > Best regards, > Krzysztof >
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. > >>>> >>>>> >>>>> 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? Best regards, Krzysztof
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"; in which case, having a fallback compatible on the SoC family that sees this IP being deployed is very useful for client programs of the DT (u-boot or kernel). As long as the fallback works, we use it, the day it stops and a quirk needs to be applied because SoC XYZ has a bug, match the SoC XYZ compatible string. FWIW, and feel free to rant at me, we have adopted this convention a while ago for STB chips whereby we want bindings to be defined with: <chip specific compatible>, <version of the IP>, <fallback> and the fallback may, or may not be matched, but defining in does not hurt at all, in fact it dramatically helps with the boot loader looking for specific nodes because it can search for the fallback. If the version specific compatible is not available, it does not get used.
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 >
On 01/10/2023 02:18 PM, 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"; > > in which case, having a fallback compatible on the SoC family that sees > this IP being deployed is very useful for client programs of the DT > (u-boot or kernel). As long as the fallback works, we use it, the day it > stops and a quirk needs to be applied because SoC XYZ has a bug, match > the SoC XYZ compatible string. > > FWIW, and feel free to rant at me, we have adopted this convention a > while ago for STB chips whereby we want bindings to be defined with: > > <chip specific compatible>, <version of the IP>, <fallback> > > and the fallback may, or may not be matched, but defining in does not > hurt at all, in fact it dramatically helps with the boot loader looking > for specific nodes because it can search for the fallback. > > If the version specific compatible is not available, it does not get used. Thanks Florian for jumping in! I was thinking to propose something with version info: brcm,bcmbca-hsspi-v1.0 brcm,bcmbca-hsspi-v1.1 To meet STB chip convention, then it would be: compatible = "brcm,bcm63138-hsspi", "brcm,bcmbca-hsspi-v1.0", "brcm,bcmbca-hsspi"; compatible = "brcm,bcm6756-hsspi", "brcm,bcmbca-hsspi-v1.1", "brcm,bcmbca-hsspi"; Although I am not a fan of having a chip specific compatible while we already have IP version, I am okay to have it to be consistent with Broadcom convention. We will need to remember to update this yaml file whenever we have a new chip.
On 11/01/2023 01:59, William Zhang wrote: >>>> 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? Yes. Best regards, Krzysztof
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
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"; 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. Does this sound good to you?
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. > > 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? Best regards, Krzysztof
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. Now I have to add more compatible to this driver and for each new chip with 1.0 in the future if any. With all the thoughts from you and Florian, I think it is better to use rev compatible in the driver but add on chip model compatible in the dts. > Best regards, > Krzysztof >
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? Best regards, Krzysztof
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 >
On 12/01/2023 20:50, William Zhang wrote: >>> 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 am sorry, this is ridiculous argument. It's like saying - people cannot understand what they are reading, therefore we need to present them obfuscated information so they will think something else than their minds created... > 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 No, it's not particularly better and you were questioning it just before... > 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 your ship sailed - you already have bindings using SoC versions... > > Anyway if you still does not like this idea, I will drop the rev info > and you have it your way. Best regards, Krzysztof
On 01/12/2023 11:41 PM, Krzysztof Kozlowski wrote: > On 12/01/2023 20:50, William Zhang wrote: >>>> 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 am sorry, this is ridiculous argument. It's like saying - people > cannot understand what they are reading, therefore we need to present > them obfuscated information so they will think something else than their > minds created... > This is clearly not to obfuscate. Rather it provide more accurate info about the binding. Is it a problem to have the correct and precise info to make it easier for people to understand? >> 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 > > No, it's not particularly better and you were questioning it just before... > Better than using the very old specific chip model number to bind all other new chips while I have a chance to update the doc now. I guess we have to agree to disagree. Enough discussion and I will send out v2 next week. Thanks for the review. >> 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 your ship sailed - you already have bindings using SoC versions... > >> >> Anyway if you still does not like this idea, I will drop the rev info >> and you have it your way. > > Best regards, > Krzysztof >
On 14/01/2023 04:17, William Zhang wrote: >>> 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 >> >> No, it's not particularly better and you were questioning it just before... >> > Better than using the very old specific chip model number to bind all > other new chips while I have a chance to update the doc now. It will be used to bind them anyway, it's already an ABI. > I guess we > have to agree to disagree. Enough discussion and I will send out v2 next > week. Thanks for the review. > >>> 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 your ship sailed - you already have bindings using SoC versions... As I said here... Best regards, Krzysztof
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: + + - 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 reg: - maxItems: 1 + items: + - description: main registers + - description: miscellaneous control registers + minItems: 1 + + reg-names: + items: + - const: hsspi + - const: spim-ctrl clocks: items: - - description: spi master reference clock - - description: spi master pll clock + - description: SPI master reference clock + - description: SPI master pll clock 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. + required: - compatible - reg - clocks - clock-names - - interrupts + +allOf: + - $ref: "spi-controller.yaml#" + - if: + properties: + compatible: + contains: + enum: + - brcm,bcm6328-hsspi + then: + properties: + reg: + minItems: 1 + maxItems: 1 + else: + properties: + reg: + minItems: 2 + maxItems: 2 + reg-names: + minItems: 2 + maxItems: 2 + brcm,use-cs-workaround: false + required: + - reg-names unevaluatedProperties: false @@ -50,3 +110,15 @@ examples: #address-cells = <1>; #size-cells = <0>; }; + - | + spi@ff801000 { + compatible = "brcm,bcmbca-hsspi"; + reg = <0xff801000 0x1000>, + <0xff802610 0x4>; + reg-names = "hsspi", "spim-ctrl"; + clocks = <&hsspi>, <&hsspi_pll>; + clock-names = "hsspi", "pll"; + num-cs = <8>; + #address-cells = <1>; + #size-cells = <0>; + };
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(-)