| Message ID | 20250104162915.332005-3-ivo.ivanov.ivanov1@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | soc: samsung: usi: implement support for USIv1 | expand |
On 1/5/25 11:18, Krzysztof Kozlowski wrote: > On Sat, Jan 04, 2025 at 06:29:14PM +0200, Ivaylo Ivanov wrote: >> >> reg: >> maxItems: 1 >> @@ -64,7 +75,6 @@ properties: >> >> samsung,mode: >> $ref: /schemas/types.yaml#/definitions/uint32 >> - enum: [0, 1, 2, 3] > Widest constraints stay here, so minimum/maximum. Why? If we are going to add new enum values specific for each USI version, isn't it better to selectively constrain them in the binding? > >> description: >> Selects USI function (which serial protocol to use). Refer to >> <include/dt-bindings/soc/samsung,exynos-usi.h> for valid USI mode values. >> @@ -101,18 +111,42 @@ required: >> - samsung,sysreg >> - samsung,mode >> >> +allOf: >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - samsung,exynos850-usi >> + then: >> + properties: >> + reg: >> + maxItems: 1 >> + >> + samsung,mode: >> + enum: [0, 1, 2, 3] >> + >> + required: >> + - reg >> + >> + else: >> + properties: >> + reg: false >> + samsung,clkreq-on: false >> + >> + samsung,mode: >> + enum: [4, 5, 6, 7, 8, 9, 10] > Is it really true? Previously for example GS101 had values 0-3, now you > claim has values 4-10, so an ABI change without explanation. How is it unexplained? Citing the commit message: "Add constants for choosing USIv1 configuration mode in device tree. Those are further used in the USI driver to figure out which value to write into SW_CONF register." USIv1 and v2 write different values to the SW_CONF register. For example: #define USI_V1_SW_CONF_UART 0x8 #define USI_V2_SW_CONF_UART BIT(0) .. [USI_V2_UART] = { .name = "uart", .val = USI_V2_SW_CONF_UART }, [USI_V1_UART] = { .name = "uart", .val = USI_V1_SW_CONF_UART }, .. Hence the decision to have separate constants for different USI versions, which in my opinion is cleaner than meshing the enums together and choosing what to use with IFs in the driver code. > >> + >> if: > Missing allOf: > >> properties: >> compatible: >> contains: >> enum: >> - samsung,exynos850-usi >> + - samsung,exynos8895-usi > Effect is not readable and not correct. You have two if:then:else. > Usually it is easier to just have separate if: for each group of > variants and define/constrain complete for such group within its if:. > >> >> then: >> properties: >> - reg: >> - maxItems: 1 >> - >> clocks: >> items: >> - description: Bus (APB) clock >> @@ -122,16 +156,13 @@ then: >> maxItems: 2 >> >> required: >> - - reg >> - clocks >> - clock-names >> >> else: >> properties: >> - reg: false >> clocks: false >> clock-names: false >> - samsung,clkreq-on: false >> >> additionalProperties: false >> >> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h >> index a01af169d..4c077c9a8 100644 >> --- a/include/dt-bindings/soc/samsung,exynos-usi.h >> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h >> @@ -13,5 +13,12 @@ >> #define USI_V2_UART 1 >> #define USI_V2_SPI 2 >> #define USI_V2_I2C 3 >> +#define USI_V1_NONE 4 > Drop, it is already there. > >> +#define USI_V1_I2C0 5 >> +#define USI_V1_I2C1 6 >> +#define USI_V1_I2C0_1 7 >> +#define USI_V1_SPI 8 > Drop > >> +#define USI_V1_UART 9 > Drop How so? These bring different configuration values. Could you specify how exactly you want me to implement these in the driver? Thanks for the feedback! Best regards, Ivaylo > >> +#define USI_V1_UART_I2C1 10 > > Best regards, > Krzysztof >
On 1/5/25 12:39, Krzysztof Kozlowski wrote: > On 05/01/2025 10:51, Ivaylo Ivanov wrote: >> On 1/5/25 11:18, Krzysztof Kozlowski wrote: >>> On Sat, Jan 04, 2025 at 06:29:14PM +0200, Ivaylo Ivanov wrote: >>>> >>>> reg: >>>> maxItems: 1 >>>> @@ -64,7 +75,6 @@ properties: >>>> >>>> samsung,mode: >>>> $ref: /schemas/types.yaml#/definitions/uint32 >>>> - enum: [0, 1, 2, 3] >>> Widest constraints stay here, so minimum/maximum. >> Why? > Because that's the coding style and that's how you define the field, > considering you might miss a variant in multiple if:then: . > > >> If we are going to add new enum values specific for each USI version, >> isn't it better to selectively constrain them in the binding? > You are supposed to constrained them. > > Again: widest constrains always stay in top level property. This applies > to all bindings, all fields. Repeated multiple times, so here is > standard example: > > https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127 Ah, I see now. Will get that fixed. > > >>>> description: >>>> Selects USI function (which serial protocol to use). Refer to >>>> <include/dt-bindings/soc/samsung,exynos-usi.h> for valid USI mode values. >>>> @@ -101,18 +111,42 @@ required: >>>> - samsung,sysreg >>>> - samsung,mode >>>> >>>> +allOf: >>>> + - if: >>>> + properties: >>>> + compatible: >>>> + contains: >>>> + enum: >>>> + - samsung,exynos850-usi >>>> + then: >>>> + properties: >>>> + reg: >>>> + maxItems: 1 >>>> + >>>> + samsung,mode: >>>> + enum: [0, 1, 2, 3] >>>> + >>>> + required: >>>> + - reg >>>> + >>>> + else: >>>> + properties: >>>> + reg: false >>>> + samsung,clkreq-on: false >>>> + >>>> + samsung,mode: >>>> + enum: [4, 5, 6, 7, 8, 9, 10] >>> Is it really true? Previously for example GS101 had values 0-3, now you >>> claim has values 4-10, so an ABI change without explanation. >> How is it unexplained? Citing the commit message: >> "Add constants for choosing USIv1 configuration mode in device tree. >> Those are further used in the USI driver to figure out which value to >> write into SW_CONF register." >> > I don't see reference here about GS101 and others. > >> USIv1 and v2 write different values to the SW_CONF register. For example: >> >> #define USI_V1_SW_CONF_UART 0x8 >> #define USI_V2_SW_CONF_UART BIT(0) >> >> .. >> [USI_V2_UART] = { .name = "uart", .val = USI_V2_SW_CONF_UART }, >> [USI_V1_UART] = { .name = "uart", .val = USI_V1_SW_CONF_UART }, >> .. >> >> Hence the decision to have separate constants for different USI versions, >> which in my opinion is cleaner than meshing the enums together and >> choosing what to use with IFs in the driver code. > This does not answer at all why GS101 receives now different values than > before. > > Explain why you are changing ABI. Oh I see what I've missed, it should be everything non-8895 having 0-3, not just the top-level compatible samsung,exynos850-usi. > >>>> + >>>> if: >>> Missing allOf: >>> >>>> properties: >>>> compatible: >>>> contains: >>>> enum: >>>> - samsung,exynos850-usi >>>> + - samsung,exynos8895-usi >>> Effect is not readable and not correct. You have two if:then:else. >>> Usually it is easier to just have separate if: for each group of >>> variants and define/constrain complete for such group within its if:. >>> >>>> >>>> then: >>>> properties: >>>> - reg: >>>> - maxItems: 1 >>>> - >>>> clocks: >>>> items: >>>> - description: Bus (APB) clock >>>> @@ -122,16 +156,13 @@ then: >>>> maxItems: 2 >>>> >>>> required: >>>> - - reg >>>> - clocks >>>> - clock-names >>>> >>>> else: >>>> properties: >>>> - reg: false >>>> clocks: false >>>> clock-names: false >>>> - samsung,clkreq-on: false >>>> >>>> additionalProperties: false >>>> >>>> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h >>>> index a01af169d..4c077c9a8 100644 >>>> --- a/include/dt-bindings/soc/samsung,exynos-usi.h >>>> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h >>>> @@ -13,5 +13,12 @@ >>>> #define USI_V2_UART 1 >>>> #define USI_V2_SPI 2 >>>> #define USI_V2_I2C 3 >>>> +#define USI_V1_NONE 4 >>> Drop, it is already there. >>> >>>> +#define USI_V1_I2C0 5 >>>> +#define USI_V1_I2C1 6 >>>> +#define USI_V1_I2C0_1 7 >>>> +#define USI_V1_SPI 8 >>> Drop >>> >>>> +#define USI_V1_UART 9 >>> Drop >> How so? These bring different configuration values. Could you specify how >> exactly you want me to implement these in the driver? > Heh, so the binding was made poorly for the driver and driver was > developed in a matching way, so now this became an argument. Binding and > drivers are independent, so whatever argument was in the driver does not > matter really. > > What I don't understand is downstream for USIv1 and USIv2 has it correct > - proper string values without mentioning any version. So, surprisingly > proper downstream binding, really rare case, was converted to something > tied to current driver implementation. > > You have only one sort of property - the mode how you configure the USI > engine. The mode can be: I2C, SPI, I2C0, I2C1 for special cases with two > I2C etc. > > The mode is not "USI_V1_I2C" because it is redundant. USI V1 cannot be > USI V2. You cannot tell USI to be v1 or v2. It is either v1 or v2. Only > one of these, thus encoding this information in the binding is meaningless. > > I even mentioned this in original binding review: > "so please drop everywhere v2 (bindings, symbols, Kconfig, > functions) except the compatible." > Well, then I missed to check on that, so now this mess has to be fixed. Yeah I get it now. Alright, I'll look into what I can do to unmangle this mess. Thanks again! Best regards, Ivaylo > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml index 5b046932f..6e32daa45 100644 --- a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml +++ b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml @@ -11,11 +11,21 @@ maintainers: - Krzysztof Kozlowski <krzk@kernel.org> description: | - USI IP-core provides selectable serial protocol (UART, SPI or High-Speed I2C). - USI shares almost all internal circuits within each protocol, so only one - protocol can be chosen at a time. USI is modeled as a node with zero or more - child nodes, each representing a serial sub-node device. The mode setting - selects which particular function will be used. + The USI IP-core provides configurable support for serial protocols, enabling + different serial communication modes depending on the version. + + In USIv1, configurations are available to enable either one or two protocols + simultaneously in select combinations - High-Speed I2C0, High-Speed + I2C1, SPI, UART, High-Speed I2C0 and I2C1 or both High-Speed + I2C1 and UART. + + In USIv2, only one protocol can be active at a time, either UART, SPI, or + High-Speed I2C. + + The USI core shares internal circuits across protocols, meaning only the + selected configuration is active at any given time. USI is modeled as a node + with zero or more child nodes, each representing a serial sub-node device. The + mode setting selects which particular function will be used. properties: $nodename: @@ -31,6 +41,7 @@ properties: - const: samsung,exynos850-usi - enum: - samsung,exynos850-usi + - samsung,exynos8895-usi reg: maxItems: 1 @@ -64,7 +75,6 @@ properties: samsung,mode: $ref: /schemas/types.yaml#/definitions/uint32 - enum: [0, 1, 2, 3] description: Selects USI function (which serial protocol to use). Refer to <include/dt-bindings/soc/samsung,exynos-usi.h> for valid USI mode values. @@ -101,18 +111,42 @@ required: - samsung,sysreg - samsung,mode +allOf: + - if: + properties: + compatible: + contains: + enum: + - samsung,exynos850-usi + then: + properties: + reg: + maxItems: 1 + + samsung,mode: + enum: [0, 1, 2, 3] + + required: + - reg + + else: + properties: + reg: false + samsung,clkreq-on: false + + samsung,mode: + enum: [4, 5, 6, 7, 8, 9, 10] + if: properties: compatible: contains: enum: - samsung,exynos850-usi + - samsung,exynos8895-usi then: properties: - reg: - maxItems: 1 - clocks: items: - description: Bus (APB) clock @@ -122,16 +156,13 @@ then: maxItems: 2 required: - - reg - clocks - clock-names else: properties: - reg: false clocks: false clock-names: false - samsung,clkreq-on: false additionalProperties: false diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h index a01af169d..4c077c9a8 100644 --- a/include/dt-bindings/soc/samsung,exynos-usi.h +++ b/include/dt-bindings/soc/samsung,exynos-usi.h @@ -13,5 +13,12 @@ #define USI_V2_UART 1 #define USI_V2_SPI 2 #define USI_V2_I2C 3 +#define USI_V1_NONE 4 +#define USI_V1_I2C0 5 +#define USI_V1_I2C1 6 +#define USI_V1_I2C0_1 7 +#define USI_V1_SPI 8 +#define USI_V1_UART 9 +#define USI_V1_UART_I2C1 10 #endif /* __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H */
Add constants for choosing USIv1 configuration mode in device tree. Those are further used in the USI driver to figure out which value to write into SW_CONF register. Modify the current USI IP-core bindings to include information about USIv1 and a compatible for exynos8895. Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> --- .../bindings/soc/samsung/exynos-usi.yaml | 55 +++++++++++++++---- include/dt-bindings/soc/samsung,exynos-usi.h | 7 +++ 2 files changed, 50 insertions(+), 12 deletions(-)