| Message ID | 20211130111325.29328-2-semen.protsenko@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | soc: samsung: Add USI driver | expand |
On Tue, 30 Nov 2021 at 21:31, Rob Herring <robh@kernel.org> wrote: > > On Tue, Nov 30, 2021 at 01:13:21PM +0200, Sam Protsenko wrote: > > Add constants for choosing USIv2 configuration mode in device tree. > > Those are further used in USI driver to figure out which value to write > > into SW_CONF register. Also document USIv2 IP-core bindings. > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > Changes in v2: > > - Combined dt-bindings doc and dt-bindings header patches > > - Added i2c node to example in bindings doc > > - Added mentioning of shared internal circuits > > - Added USI_V2_NONE value to bindings header > > > > .../bindings/soc/samsung/exynos-usi.yaml | 135 ++++++++++++++++++ > > include/dt-bindings/soc/samsung,exynos-usi.h | 17 +++ > > 2 files changed, 152 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml > > create mode 100644 include/dt-bindings/soc/samsung,exynos-usi.h > > > > diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml > > new file mode 100644 > > index 000000000000..a822bc62b3cd > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml > > @@ -0,0 +1,135 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/soc/samsung/exynos-usi.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Samsung's Exynos USI (Universal Serial Interface) binding > > + > > +maintainers: > > + - Sam Protsenko <semen.protsenko@linaro.org> > > + - Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > > + > > +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. > > + > > + Refer to next bindings documentation for information on protocol subnodes that > > + can exist under USI node: > > + > > + [1] Documentation/devicetree/bindings/serial/samsung_uart.yaml > > + [2] Documentation/devicetree/bindings/i2c/i2c-exynos5.txt > > + [3] Documentation/devicetree/bindings/spi/spi-samsung.txt > > + > > +properties: > > + $nodename: > > + pattern: "^usi@[0-9a-f]+$" > > + > > + compatible: > > + const: samsung,exynos-usi-v2 > > Use SoC based compatibles. > In this particular case, I'd really prefer to have it like this. Most likely we'll only have USIv1 and USIv1 in the end, and I think that would be more clear to have USI version in compatible, rather than SoC name. Please let me know if you have a strong opinion on this one -- if so I'll re-send. > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + items: > > + - description: Bus (APB) clock > > + - description: Operating clock for UART/SPI/I2C protocol > > + > > + clock-names: > > + items: > > + - const: pclk > > + - const: ipclk > > + > > + ranges: true > > + > > + "#address-cells": > > + const: 1 > > + > > + "#size-cells": > > + const: 1 > > + > > + samsung,sysreg: > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > + description: > > + Should be phandle/offset pair. The phandle to System Register syscon node > > + (for the same domain where this USI controller resides) and the offset > > + of SW_CONF register for this USI controller. > > + > > + samsung,mode: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + Selects USI function (which serial protocol to use). Refer to > > + <include/dt-bindings/soc/samsung,exynos-usi.h> for valid USI mode values. > > This seems to be redundant. Just check which child is enabled. > I think it's not that easy. Soon we'll have USIv1 support added, and that has some weird configurations, like having dual I2C mode (two child I2C nodes must be enabled) and UART+I2C mode, etc. Looks like it might take some not very elegant logic to figure out which exactly mode value should be written in SW_CONF register in that way, it's much easier to just specify mode in USI node. Also, that reflects hardware better: we actually write that specified mode to SW_CONF register. Also, later we might want to be able to switch that mode via SysFS, e.g. for testing purposes. Current design seems to be better suited for some things like that. Please let me know if you have a strong opinion on this one, or it's ok to leave it as is. All other comments are addressed and will be present in v3. Thanks for the review! > > + > > + samsung,clkreq-on: > > + type: boolean > > + description: > > + Enable this property if underlying protocol requires the clock to be > > + continuously provided without automatic gating. As suggested by SoC > > + manual, it should be set in case of SPI/I2C slave, UART Rx and I2C > > + multi-master mode. Usually this property is needed if USI mode is set > > + to "UART". > > + > > + This property is optional. > > + > > +patternProperties: > > + # All other properties should be child nodes > > + "^.*@[0-9a-f]+$": > > Only 'serial', 'spi', or 'i2c' are valid. > > > + type: object > > + description: Child node describing underlying USI serial protocol > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - clock-names > > + - ranges > > + - "#address-cells" > > + - "#size-cells" > > + - samsung,sysreg > > + - samsung,mode > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/soc/samsung,exynos-usi.h> > > + > > + usi0: usi@138200c0 { > > + compatible = "samsung,exynos-usi-v2"; > > + reg = <0x138200c0 0x20>; > > + samsung,sysreg = <&sysreg_peri 0x1010>; > > + samsung,mode = <USI_V2_UART>; > > + samsung,clkreq-on; /* needed for UART mode */ > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges; > > + clocks = <&cmu_peri 32>, <&cmu_peri 31>; > > + clock-names = "pclk", "ipclk"; > > + status = "disabled"; > > Why are you disabling your example? Remove status. > > > + > > + serial_0: serial@13820000 { > > + compatible = "samsung,exynos850-uart"; > > + reg = <0x13820000 0xc0>; > > + interrupts = <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&cmu_peri 32>, <&cmu_peri 31>; > > + clock-names = "uart", "clk_uart_baud0"; > > + status = "disabled"; > > + }; > > + > > + hsi2c_0: i2c@13820000 { > > + compatible = "samsung,exynosautov9-hsi2c"; > > + reg = <0x13820000 0xc0>; > > + interrupts = <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + clocks = <&cmu_peri 32>, <&cmu_peri 31>; > > + clock-names = "hsi2c_pclk", "hsi2c"; > > + status = "disabled"; > > + }; > > + }; > > diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h > > new file mode 100644 > > index 000000000000..a01af169d249 > > --- /dev/null > > +++ b/include/dt-bindings/soc/samsung,exynos-usi.h > > @@ -0,0 +1,17 @@ > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ > > +/* > > + * Copyright (c) 2021 Linaro Ltd. > > + * Author: Sam Protsenko <semen.protsenko@linaro.org> > > + * > > + * Device Tree bindings for Samsung Exynos USI (Universal Serial Interface). > > + */ > > + > > +#ifndef __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H > > +#define __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H > > + > > +#define USI_V2_NONE 0 > > +#define USI_V2_UART 1 > > +#define USI_V2_SPI 2 > > +#define USI_V2_I2C 3 > > + > > +#endif /* __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H */ > > -- > > 2.30.2 > > > >
On Fri, Dec 3, 2021 at 1:36 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Tue, 30 Nov 2021 at 21:31, Rob Herring <robh@kernel.org> wrote: > > > > On Tue, Nov 30, 2021 at 01:13:21PM +0200, Sam Protsenko wrote: > > > Add constants for choosing USIv2 configuration mode in device tree. > > > Those are further used in USI driver to figure out which value to write > > > into SW_CONF register. Also document USIv2 IP-core bindings. > > > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > --- > > > Changes in v2: > > > - Combined dt-bindings doc and dt-bindings header patches > > > - Added i2c node to example in bindings doc > > > - Added mentioning of shared internal circuits > > > - Added USI_V2_NONE value to bindings header > > > > > > .../bindings/soc/samsung/exynos-usi.yaml | 135 ++++++++++++++++++ > > > include/dt-bindings/soc/samsung,exynos-usi.h | 17 +++ > > > 2 files changed, 152 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml > > > create mode 100644 include/dt-bindings/soc/samsung,exynos-usi.h > > > > > > diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml > > > new file mode 100644 > > > index 000000000000..a822bc62b3cd > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml > > > @@ -0,0 +1,135 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/soc/samsung/exynos-usi.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Samsung's Exynos USI (Universal Serial Interface) binding > > > + > > > +maintainers: > > > + - Sam Protsenko <semen.protsenko@linaro.org> > > > + - Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > > > + > > > +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. > > > + > > > + Refer to next bindings documentation for information on protocol subnodes that > > > + can exist under USI node: > > > + > > > + [1] Documentation/devicetree/bindings/serial/samsung_uart.yaml > > > + [2] Documentation/devicetree/bindings/i2c/i2c-exynos5.txt > > > + [3] Documentation/devicetree/bindings/spi/spi-samsung.txt > > > + > > > +properties: > > > + $nodename: > > > + pattern: "^usi@[0-9a-f]+$" > > > + > > > + compatible: > > > + const: samsung,exynos-usi-v2 > > > > Use SoC based compatibles. > > > > In this particular case, I'd really prefer to have it like this. Most > likely we'll only have USIv1 and USIv1 in the end, and I think that > would be more clear to have USI version in compatible, rather than SoC > name. Please let me know if you have a strong opinion on this one -- > if so I'll re-send. Fine if you have some evidence the ratio of versions to SoC are much more than 1:1 and the versions correspond to something (IOW, you aren't making them up). We went down the version # path with QCom and in the end about every SoC had a different version. > > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + clocks: > > > + items: > > > + - description: Bus (APB) clock > > > + - description: Operating clock for UART/SPI/I2C protocol > > > + > > > + clock-names: > > > + items: > > > + - const: pclk > > > + - const: ipclk > > > + > > > + ranges: true > > > + > > > + "#address-cells": > > > + const: 1 > > > + > > > + "#size-cells": > > > + const: 1 > > > + > > > + samsung,sysreg: > > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > > + description: > > > + Should be phandle/offset pair. The phandle to System Register syscon node > > > + (for the same domain where this USI controller resides) and the offset > > > + of SW_CONF register for this USI controller. > > > + > > > + samsung,mode: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: > > > + Selects USI function (which serial protocol to use). Refer to > > > + <include/dt-bindings/soc/samsung,exynos-usi.h> for valid USI mode values. > > > > This seems to be redundant. Just check which child is enabled. > > > > I think it's not that easy. Soon we'll have USIv1 support added, and > that has some weird configurations, like having dual I2C mode (two > child I2C nodes must be enabled) and UART+I2C mode, etc. So you are going to turn around and make this an array? If you already know you have changes, I'd rather review this all at once. > Looks like it > might take some not very elegant logic to figure out which exactly > mode value should be written in SW_CONF register in that way, it's > much easier to just specify mode in USI node. Also, that reflects > hardware better: we actually write that specified mode to SW_CONF > register. You just have to compare the child node names or compatibles. > Also, later we might want to be able to switch that mode via > SysFS, e.g. for testing purposes. Current design seems to be better > suited for some things like that. The binding should have no impact on that. If for testing, use debugfs. > Please let me know if you have a strong opinion on this one, or it's > ok to leave it as is. > > All other comments are addressed and will be present in v3. Thanks for > the review!
diff --git a/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml new file mode 100644 index 000000000000..a822bc62b3cd --- /dev/null +++ b/Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml @@ -0,0 +1,135 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/soc/samsung/exynos-usi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Samsung's Exynos USI (Universal Serial Interface) binding + +maintainers: + - Sam Protsenko <semen.protsenko@linaro.org> + - Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> + +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. + + Refer to next bindings documentation for information on protocol subnodes that + can exist under USI node: + + [1] Documentation/devicetree/bindings/serial/samsung_uart.yaml + [2] Documentation/devicetree/bindings/i2c/i2c-exynos5.txt + [3] Documentation/devicetree/bindings/spi/spi-samsung.txt + +properties: + $nodename: + pattern: "^usi@[0-9a-f]+$" + + compatible: + const: samsung,exynos-usi-v2 + + reg: + maxItems: 1 + + clocks: + items: + - description: Bus (APB) clock + - description: Operating clock for UART/SPI/I2C protocol + + clock-names: + items: + - const: pclk + - const: ipclk + + ranges: true + + "#address-cells": + const: 1 + + "#size-cells": + const: 1 + + samsung,sysreg: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: + Should be phandle/offset pair. The phandle to System Register syscon node + (for the same domain where this USI controller resides) and the offset + of SW_CONF register for this USI controller. + + samsung,mode: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Selects USI function (which serial protocol to use). Refer to + <include/dt-bindings/soc/samsung,exynos-usi.h> for valid USI mode values. + + samsung,clkreq-on: + type: boolean + description: + Enable this property if underlying protocol requires the clock to be + continuously provided without automatic gating. As suggested by SoC + manual, it should be set in case of SPI/I2C slave, UART Rx and I2C + multi-master mode. Usually this property is needed if USI mode is set + to "UART". + + This property is optional. + +patternProperties: + # All other properties should be child nodes + "^.*@[0-9a-f]+$": + type: object + description: Child node describing underlying USI serial protocol + +required: + - compatible + - reg + - clocks + - clock-names + - ranges + - "#address-cells" + - "#size-cells" + - samsung,sysreg + - samsung,mode + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/soc/samsung,exynos-usi.h> + + usi0: usi@138200c0 { + compatible = "samsung,exynos-usi-v2"; + reg = <0x138200c0 0x20>; + samsung,sysreg = <&sysreg_peri 0x1010>; + samsung,mode = <USI_V2_UART>; + samsung,clkreq-on; /* needed for UART mode */ + #address-cells = <1>; + #size-cells = <1>; + ranges; + clocks = <&cmu_peri 32>, <&cmu_peri 31>; + clock-names = "pclk", "ipclk"; + status = "disabled"; + + serial_0: serial@13820000 { + compatible = "samsung,exynos850-uart"; + reg = <0x13820000 0xc0>; + interrupts = <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&cmu_peri 32>, <&cmu_peri 31>; + clock-names = "uart", "clk_uart_baud0"; + status = "disabled"; + }; + + hsi2c_0: i2c@13820000 { + compatible = "samsung,exynosautov9-hsi2c"; + reg = <0x13820000 0xc0>; + interrupts = <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>; + #address-cells = <1>; + #size-cells = <0>; + clocks = <&cmu_peri 32>, <&cmu_peri 31>; + clock-names = "hsi2c_pclk", "hsi2c"; + status = "disabled"; + }; + }; diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h new file mode 100644 index 000000000000..a01af169d249 --- /dev/null +++ b/include/dt-bindings/soc/samsung,exynos-usi.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ +/* + * Copyright (c) 2021 Linaro Ltd. + * Author: Sam Protsenko <semen.protsenko@linaro.org> + * + * Device Tree bindings for Samsung Exynos USI (Universal Serial Interface). + */ + +#ifndef __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H +#define __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H + +#define USI_V2_NONE 0 +#define USI_V2_UART 1 +#define USI_V2_SPI 2 +#define USI_V2_I2C 3 + +#endif /* __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H */
Add constants for choosing USIv2 configuration mode in device tree. Those are further used in USI driver to figure out which value to write into SW_CONF register. Also document USIv2 IP-core bindings. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- Changes in v2: - Combined dt-bindings doc and dt-bindings header patches - Added i2c node to example in bindings doc - Added mentioning of shared internal circuits - Added USI_V2_NONE value to bindings header .../bindings/soc/samsung/exynos-usi.yaml | 135 ++++++++++++++++++ include/dt-bindings/soc/samsung,exynos-usi.h | 17 +++ 2 files changed, 152 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/samsung/exynos-usi.yaml create mode 100644 include/dt-bindings/soc/samsung,exynos-usi.h