| Message ID | 20210730144922.29111-1-semen.protsenko@linaro.org |
|---|---|
| Headers | show |
| Series | Add minimal support for Exynos850 SoC | expand |
On Fri, Jul 30, 2021 at 5:50 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > New device tree bindings are usually added in YAML format. Fix "SAMSUNG > SOC CLOCK DRIVERS" entry to cover both txt and yaml docs for Exynos > clock drivers. Fixes tag? -- With Best Regards, Andy Shevchenko
On Fri, Jul 30, 2021 at 5:51 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > For now it's just a stub driver to make serial driver work. Later it make the serial > will be implemented properly. > > This driver doesn't really change clocks, only registers the UART clock > as a fixed-rate clock. Without this clock driver the UART driver won't > work, as it's trying to obtain "uart" clock and fails if it's not able > to. > From drivers/tty/serial/samsung_tty.c: > > 8<------------------------------------------------------------------->8 > ourport->clk = clk_get(&platdev->dev, "uart"); > if (IS_ERR(ourport->clk)) { > pr_err("%s: Controller clock not found\n", > dev_name(&platdev->dev)); > ret = PTR_ERR(ourport->clk); > goto err; > } > 8<------------------------------------------------------------------->8 This is not needed in the commit message. > In order to get functional serial console we have to implement that get a functional > minimal clock driver with "uart" clock. It's not necessary to actually > configure clocks, as those are already configured in bootloader, so > kernel can rely on that for now. > 80 column limit is broken here to make checkpatch happy, otherwise it > swears about incorrect __initconst usage. Again, no need to be in the commit message, use the comment field for this (after the cutter '---' line below). > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > drivers/clk/samsung/Makefile | 1 + > drivers/clk/samsung/clk-exynos850.c | 63 +++++++++++++++++++++++++++++ > 2 files changed, 64 insertions(+) > create mode 100644 drivers/clk/samsung/clk-exynos850.c > > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile > index 028b2e27a37e..c46cf11e4d0b 100644 > --- a/drivers/clk/samsung/Makefile > +++ b/drivers/clk/samsung/Makefile > @@ -17,6 +17,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos5433.o > obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o > obj-$(CONFIG_EXYNOS_CLKOUT) += clk-exynos-clkout.o > obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos7.o > +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos850.o > obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o > obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o > obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o > diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c > new file mode 100644 > index 000000000000..3192ec9bb90b > --- /dev/null > +++ b/drivers/clk/samsung/clk-exynos850.c > @@ -0,0 +1,63 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2019 Samsung Electronics Co., Ltd. > + * Copyright (C) 2021 Linaro Ltd. > + * > + * Common Clock Framework support for Exynos850 SoC. > + */ > + > +#include <linux/clk.h> > +#include <linux/clkdev.h> > +#include <linux/clk-provider.h> > +#include <linux/of.h> > +#include <linux/of_address.h> + blank line? > +#include <dt-bindings/clock/exynos850.h> > + > +#include "clk.h" > + > +/* Fixed rate clocks generated outside the SoC */ > +static struct samsung_fixed_rate_clock exynos850_fixed_rate_ext_clks[] __initdata = { > + FRATE(OSCCLK, "fin_pll", NULL, 0, 26000000), > +}; > + > +/* > + * Model the UART clock as a fixed-rate clock for now, to make serial driver > + * work. This clock is already configured in the bootloader. > + */ > +static const struct samsung_fixed_rate_clock exynos850_peri_clks[] __initconst = { > + FRATE(DOUT_UART, "DOUT_UART", NULL, 0, 200000000), > +}; > + > +static const struct of_device_id ext_clk_match[] __initconst = { > + { .compatible = "samsung,exynos850-oscclk", .data = (void *)0 }, 0 is the default for static variables. > + {}, No comma needed for the terminator lines. > +}; > + > +void __init exynos850_clk_init(struct device_node *np) > +{ > + void __iomem *reg_base; > + struct samsung_clk_provider *ctx; > + if (!np) > + panic("%s: unable to determine soc\n", __func__); Check for the sake of additional code? > + reg_base = of_iomap(np, 0); This will fail when np == NULL. > + if (!reg_base) > + panic("%s: failed to map registers\n", __func__); > + > + ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS); > + if (!ctx) > + panic("%s: unable to allocate ctx\n", __func__); > + > + samsung_clk_of_register_fixed_ext(ctx, > + exynos850_fixed_rate_ext_clks, > + ARRAY_SIZE(exynos850_fixed_rate_ext_clks), > + ext_clk_match); > + > + samsung_clk_register_fixed_rate(ctx, exynos850_peri_clks, > + ARRAY_SIZE(exynos850_peri_clks)); > + > + samsung_clk_of_add_provider(np, ctx); > +} > + > +CLK_OF_DECLARE(exynos850_clk, "samsung,exynos850-clock", exynos850_clk_init); -- With Best Regards, Andy Shevchenko
On 30/07/2021 16:49, Sam Protsenko wrote: > This patch series adds initial platform support for Samsung Exynos850 > SoC [1]. With this patchset it's possible to run the kernel with BusyBox > rootfs as a RAM disk. More advanced platform support (like MMC driver > additions) will be added later. The idea is to keep the first submission > minimal to ease the review, and then build up on top of that. > > [1] https://www.samsung.com/semiconductor/minisite/exynos/products/mobileprocessor/exynos-850/ > Great work! What's the SoC revision number (should be accessible via /sys/bus/soc/devices/soc0/)? Recent wrap in numbering of Exynos chips might bring confusion... Best regards, Krzysztof
On 30/07/2021 16:49, Sam Protsenko wrote: > Document compatible string for Exynos850 SoC. Nothing else is changed, > as Exynos850 SoC uses already existing samsung pinctrl driver. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 1 + > 1 file changed, 1 insertion(+) > The patch should be first in the series - dt-bindings go at beginning. Although no need to resend just for this. If the resend happens, the fix ("pinctrl: samsung: Fix pinctrl bank pin count") should be sent separately (no mixing fixes and new features) because they are independent and usually easier for review. Best regards, Krzysztof
On 30/07/2021 17:06, Andy Shevchenko wrote: > On Fri, Jul 30, 2021 at 5:50 PM Sam Protsenko > <semen.protsenko@linaro.org> wrote: >> >> New device tree bindings are usually added in YAML format. Fix "SAMSUNG >> SOC CLOCK DRIVERS" entry to cover both txt and yaml docs for Exynos >> clock drivers. > > Fixes tag? No need because the pattern is correct at the moment. The patch does not make sense on its own and should be squashed with the next one. Best regards, Krzysztof
On 30/07/2021 16:49, Sam Protsenko wrote: > Add documentation for Exynos850 clock driver bindings and corresponding > clock ID constants, which will be used further both in clock driver and > in device tree files. Constants are grouped per domain basis (CMU) for > more convenient usage, but those are just unique numbers and have > nothing to do with register offsets, etc. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > .../bindings/clock/exynos850-clock.yaml | 70 +++++ > include/dt-bindings/clock/exynos850.h | 267 ++++++++++++++++++ > 2 files changed, 337 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/exynos850-clock.yaml > create mode 100644 include/dt-bindings/clock/exynos850.h > > diff --git a/Documentation/devicetree/bindings/clock/exynos850-clock.yaml b/Documentation/devicetree/bindings/clock/exynos850-clock.yaml > new file mode 100644 > index 000000000000..201c2b79e629 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/exynos850-clock.yaml > @@ -0,0 +1,70 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/exynos850-clock.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Clock bindings for Samsung Exynos850 clock controller What plans do you have for this binding? Is it complete? If yes, there is no point to have binding for each SoC clock controller and this should be merged into for example: Documentation/devicetree/bindings/clock/exynos5420-clock.txt First - convert exynos5420-clock.txt into YAML. Then add Exynos850. Other files can be also integrated. > + > +maintainers: > + - Sam Protsenko <semen.protsenko@linaro.org> I propose also: +Sylwester Nawrocki <s.nawrocki@samsung.com> +Tomasz Figa <tomasz.figa@gmail.com> +Chanwoo Choi <cw00.choi@samsung.com> > + > +description: | > + The Exynos850 clock controller generates and supplies clock to various > + controllers within the SoC. > Each clock is assigned an identifier and client > + nodes can use this identifier to specify the clock which they consume. Skip the sentence, it's generic clock provider/consumer description. > + > + All available clocks are defined as preprocessor macros in > + dt-bindings/clock/exynos850.h header and can be used in device tree sources. > + > +properties: > + compatible: > + const: samsung,exynos850-clock > + > + reg: > + maxItems: 1 > + > + '#clock-cells': > + const: 1 > + > +required: > + - compatible > + - reg > + - '#clock-cells' > + > +additionalProperties: false > + > +examples: > + # Clock controller node > + - | > + clock: clock-controller@0x120e0000 { > + compatible = "samsung,exynos850-clock"; > + reg = <0x0 0x120e0000 0x8000>; > + #clock-cells = <1>; > + }; > + > + # Required external clocks (should be provided in particular board DTS) > + - | > + fixed-rate-clocks { > + oscclk { Use a generic node name: s/oscclk/clock/ > + compatible = "samsung,exynos850-oscclk"; > + clock-frequency = <26000000>; > + }; > + }; > + > + # UART controller node that consumes the clock generated by the clock > + # controller > + - | > + #include <dt-bindings/clock/exynos850.h> > + > + serial_0: uart@13820000 { > + compatible = "samsung,exynos850-uart"; > + reg = <0x0 0x13820000 0x100>; > + interrupts = <GIC_SPI INTREQ__UART IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&uart0_bus>; > + clocks = <&clock GATE_UART_QCH>, <&clock DOUT_UART>; > + clock-names = "gate_uart_clk0", "uart"; > + }; > + > +... > diff --git a/include/dt-bindings/clock/exynos850.h b/include/dt-bindings/clock/exynos850.h > new file mode 100644 > index 000000000000..b197db4427fc > --- /dev/null > +++ b/include/dt-bindings/clock/exynos850.h > @@ -0,0 +1,267 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2019 Samsung Electronics Co., Ltd. > + * Copyright (C) 2021 Linaro Ltd. > + * > + * Device Tree binding constants for Exynos850 clock controller. > + */ > + > +#ifndef _DT_BINDINGS_CLOCK_EXYNOS_850_H > +#define _DT_BINDINGS_CLOCK_EXYNOS_850_H > + > +#define NONE (0 + 0) What type of clock is it? > +#define OSCCLK (0 + 1) Isn't this CLK_FIN_PLL? > + > +#define CLK_APM_BASE (10) > +#define UMUX_DLL_USER (CLK_APM_BASE + 0) Why having a base? We are not using it in any other bindings. > +#define UMUX_CLK_APM_BUS (CLK_APM_BASE + 1) > +#define GATE_APM_CMU_APM_QCH (CLK_APM_BASE + 2) > +#define GATE_GREBEINTEGRATION_QCH_GREBE (CLK_APM_BASE + 3) > +#define GATE_GREBEINTEGRATION_QCH_DBG (CLK_APM_BASE + 4) > +#define GATE_I3C_APM_PMIC_QCH_S_I3C (CLK_APM_BASE + 5) > +#define GATE_I3C_APM_PMIC_QCH (CLK_APM_BASE + 6) > +#define GATE_INTMEM_QCH (CLK_APM_BASE + 7) > +#define GATE_MAILBOX_APM_AP_QCH (CLK_APM_BASE + 8) > +#define GATE_MAILBOX_APM_CHUB_QCH (CLK_APM_BASE + 9) > +#define GATE_MAILBOX_APM_CP_QCH (CLK_APM_BASE + 10) > +#define GATE_MAILBOX_APM_GNSS_QCH (CLK_APM_BASE + 11) > +#define GATE_MAILBOX_APM_WLBT_QCH (CLK_APM_BASE + 12) > +#define GATE_MAILBOX_AP_CHUB_QCH (CLK_APM_BASE + 13) > +#define GATE_MAILBOX_AP_CP_QCH (CLK_APM_BASE + 14) > +#define GATE_MAILBOX_AP_CP_S_QCH (CLK_APM_BASE + 15) > +#define GATE_MAILBOX_AP_GNSS_QCH (CLK_APM_BASE + 16) > +#define GATE_MAILBOX_AP_WLBT_QCH (CLK_APM_BASE + 17) > +#define GATE_MAILBOX_CP_CHUB_QCH (CLK_APM_BASE + 18) > +#define GATE_MAILBOX_CP_GNSS_QCH (CLK_APM_BASE + 19) > +#define GATE_MAILBOX_CP_WLBT_QCH (CLK_APM_BASE + 20) > +#define GATE_MAILBOX_GNSS_CHUB_QCH (CLK_APM_BASE + 21) > +#define GATE_MAILBOX_GNSS_WLBT_QCH (CLK_APM_BASE + 22) > +#define GATE_MAILBOX_WLBT_ABOX_QCH (CLK_APM_BASE + 23) > +#define GATE_MAILBOX_WLBT_CHUB_QCH (CLK_APM_BASE + 24) > +#define GATE_PMU_INTR_GEN_QCH (CLK_APM_BASE + 25) > +#define GATE_ROM_CRC32_HOST_QCH (CLK_APM_BASE + 26) > +#define GATE_SPEEDY_APM_QCH (CLK_APM_BASE + 27) > +#define GATE_WDT_APM_QCH (CLK_APM_BASE + 28) > + > +#define CLK_AUD_BASE (50) > +#define UMUX_CLK_AUD_CPU_HCH (CLK_AUD_BASE + 0) > +#define GATE_ABOX_QCH_CPU (CLK_AUD_BASE + 1) > +#define GATE_ABOX_QCH_ACLK (CLK_AUD_BASE + 2) > +#define GATE_ABOX_QCH_BCLK0 (CLK_AUD_BASE + 3) > +#define GATE_ABOX_QCH_BCLK1 (CLK_AUD_BASE + 4) > +#define GATE_ABOX_QCH_FM (CLK_AUD_BASE + 5) > +#define GATE_ABOX_QCH_BCLK2 (CLK_AUD_BASE + 6) > +#define GATE_ABOX_QCH_CCLK_ASB (CLK_AUD_BASE + 7) > +#define GATE_ABOX_QCH_BCLK3 (CLK_AUD_BASE + 8) > +#define GATE_ABOX_QCH_BCLK4 (CLK_AUD_BASE + 9) > +#define GATE_ABOX_QCH_BCLK5 (CLK_AUD_BASE + 10) > +#define GATE_ABOX_QCH_BCLK6 (CLK_AUD_BASE + 11) > +#define GATE_ABOX_QCH_BCLK_CNT (CLK_AUD_BASE + 12) > +#define GATE_AUD_CMU_AUD_QCH (CLK_AUD_BASE + 13) > +#define GATE_GPIO_AUD_QCH (CLK_AUD_BASE + 14) > +#define GATE_SYSMMU_AUD_QCH_S1 (CLK_AUD_BASE + 15) > +#define GATE_WDT_AUD_QCH (CLK_AUD_BASE + 16) > +#define PLL_AUD_OUT (CLK_AUD_BASE + 17) > +#define DOUT_CLK_AUD_CPU (CLK_AUD_BASE + 18) > +#define DOUT_CLK_AUD_CPU_ACLK (CLK_AUD_BASE + 19) > +#define DOUT_CLK_AUD_CPU_PCLKDBG (CLK_AUD_BASE + 20) > +#define DOUT_CLK_AUD_BUSD (CLK_AUD_BASE + 21) > +#define DOUT_CLK_AUD_UAIF0 (CLK_AUD_BASE + 23) > +#define DOUT_CLK_AUD_UAIF1 (CLK_AUD_BASE + 24) > +#define DOUT_CLK_AUD_FM (CLK_AUD_BASE + 25) > +#define DOUT_CLK_AUD_BUSP (CLK_AUD_BASE + 26) > +#define DOUT_CLK_AUD_UAIF2 (CLK_AUD_BASE + 27) > +#define DOUT_CLK_AUD_CNT (CLK_AUD_BASE + 28) > +#define DOUT_CLK_AUD_UAIF3 (CLK_AUD_BASE + 29) > +#define DOUT_CLK_AUD_UAIF4 (CLK_AUD_BASE + 30) > +#define DOUT_CLK_AUD_UAIF5 (CLK_AUD_BASE + 31) > +#define DOUT_CLK_AUD_UAIF6 (CLK_AUD_BASE + 32) > +#define DOUT_CLK_AUD_AUDIF (CLK_AUD_BASE + 33) > +#define DOUT_CLK_AUD_MCLK (CLK_AUD_BASE + 34) > +#define UMUX_CLK_AUD_FM (CLK_AUD_BASE + 35) > + > +#define CLK_CHUB_BASE (100) > +#define GATE_BAAW_C_CHUB_QCH (CLK_CHUB_BASE + 0) > +#define GATE_BAAW_D_CHUB_QCH (CLK_CHUB_BASE + 1) > +#define GATE_CHUB_CMU_CHUB_QCH (CLK_CHUB_BASE + 2) > +#define GATE_CM4_CHUB_QCH (CLK_CHUB_BASE + 3) > +#define GATE_DMIC_AHB0_QCH (CLK_CHUB_BASE + 4) > +#define GATE_DMIC_IF_QCH_PCLK (CLK_CHUB_BASE + 5) > +#define GATE_DMIC_IF_QCH_DMIC_CLK (CLK_CHUB_BASE + 6) > +#define GATE_HWACG_SYS_DMIC0_QCH (CLK_CHUB_BASE + 7) > +#define GATE_PWM_CHUB_QCH (CLK_CHUB_BASE + 8) > +#define GATE_SWEEPER_C_CHUB_QCH (CLK_CHUB_BASE + 9) > +#define GATE_SWEEPER_D_CHUB_QCH (CLK_CHUB_BASE + 10) > +#define GATE_TIMER_CHUB_QCH (CLK_CHUB_BASE + 11) > +#define GATE_WDT_CHUB_QCH (CLK_CHUB_BASE + 12) > +#define GATE_U_DMIC_CLK_SCAN_MUX_QCH (CLK_CHUB_BASE + 13) > +#define DOUT_CLK_CHUB_BUS (CLK_CHUB_BASE + 14) > +#define DOUT_CLK_CHUB_DMIC_IF (CLK_CHUB_BASE + 15) > +#define DOUT_CLK_CHUB_DMIC_IF_DIV2 (CLK_CHUB_BASE + 16) > +#define DOUT_CLK_CHUB_DMIC (CLK_CHUB_BASE + 17) > + > +#define CLK_CMGP_BASE (150) > +#define UMUX_CLK_CMGP_USI_CMGP0 (CLK_CMGP_BASE + 0) > +#define UMUX_CLK_CMGP_USI_CMGP1 (CLK_CMGP_BASE + 1) > +#define GATE_ADC_CMGP_QCH_S0 (CLK_CMGP_BASE + 2) > +#define GATE_ADC_CMGP_QCH_S1 (CLK_CMGP_BASE + 3) > +#define GATE_ADC_CMGP_QCH_ADC (CLK_CMGP_BASE + 4) > +#define GATE_CMGP_CMU_CMGP_QCH (CLK_CMGP_BASE + 5) > +#define GATE_GPIO_CMGP_QCH (CLK_CMGP_BASE + 6) > +#define GATE_USI_CMGP0_QCH (CLK_CMGP_BASE + 7) > +#define GATE_USI_CMGP1_QCH (CLK_CMGP_BASE + 8) > +#define DOUT_CLK_CMGP_ADC (CLK_CMGP_BASE + 9) > +#define DOUT_CLK_CMGP_USI_CMGP0 (CLK_CMGP_BASE + 10) > +#define DOUT_CLK_CMGP_USI_CMGP1 (CLK_CMGP_BASE + 11) > + > +#define CLK_TOP_BASE (200) You seem to register all clocks under one CMU. Are you sure that this reflects the HW? It looks quite a big set of clocks for one block... It does not look scallable. > +#define GATE_CMU_TOP_CMUREF_QCH (CLK_TOP_BASE + 0) > +#define GATE_DFTMUX_CMU_QCH_CLK_CIS0 (CLK_TOP_BASE + 1) > +#define GATE_DFTMUX_CMU_QCH_CLK_CIS1 (CLK_TOP_BASE + 2) > +#define GATE_DFTMUX_CMU_QCH_CLK_CIS2 (CLK_TOP_BASE + 3) > +#define GATE_OTP_QCH (CLK_TOP_BASE + 4) > +#define GATE_ADM_AHB_SSS_QCH (CLK_TOP_BASE + 5) > +#define GATE_BAAW_P_CHUB_QCH (CLK_TOP_BASE + 6) > +#define GATE_BAAW_P_GNSS_QCH (CLK_TOP_BASE + 7) > +#define GATE_BAAW_P_MODEM_QCH (CLK_TOP_BASE + 8) > +#define GATE_BAAW_P_WLBT_QCH (CLK_TOP_BASE + 9) > + > +#define CLK_CORE_BASE (250) > +#define GATE_CCI_550_QCH (CLK_CORE_BASE + 0) > +#define GATE_CORE_CMU_CORE_QCH (CLK_CORE_BASE + 1) > +#define GATE_GIC_QCH (CLK_CORE_BASE + 2) > +#define GATE_GPIO_CORE_QCH (CLK_CORE_BASE + 3) > +#define GATE_MMC_EMBD_QCH (CLK_CORE_BASE + 4) > +#define GATE_PDMA_CORE_QCH (CLK_CORE_BASE + 5) > +#define GATE_RTIC_QCH (CLK_CORE_BASE + 6) > +#define GATE_SPDMA_CORE_QCH (CLK_CORE_BASE + 7) > +#define GATE_SSS_QCH (CLK_CORE_BASE + 8) > +#define GATE_TREX_D_CORE_QCH (CLK_CORE_BASE + 9) > +#define GATE_TREX_P_CORE_QCH (CLK_CORE_BASE + 10) > +#define GATE_CSSYS_DBG_QCH (CLK_CORE_BASE + 11) > +#define GATE_SECJTAG_QCH (CLK_CORE_BASE + 12) > +#define DOUT_CORE_MMC_EMBD (CLK_CORE_BASE + 13) > + > +#define CLK_DPU_BASE (300) > +#define GATE_DPU_QCH_S_DPP (CLK_DPU_BASE + 0) > +#define GATE_DPU_QCH_S_DMA (CLK_DPU_BASE + 1) > +#define GATE_DPU_QCH_S_DECON (CLK_DPU_BASE + 2) > +#define GATE_DPU_CMU_DPU_QCH (CLK_DPU_BASE + 3) > +#define GATE_SMMU_DPU_QCH (CLK_DPU_BASE + 4) > +#define DOUT_CLK_DPU_BUSP (CLK_DPU_BASE + 5) > + > +#define CLK_G3D_BASE (350) > +#define GATE_G3D_CMU_G3D_QCH (CLK_G3D_BASE + 0) > +#define GATE_GPU_QCH (CLK_G3D_BASE + 1) > +#define DOUT_CLK_G3D_BUSP (CLK_G3D_BASE + 2) > + > +#define CLK_HIS_BASE (400) > +#define GATE_GPIO_HSI_QCH (CLK_HIS_BASE + 0) > +#define GATE_HSI_CMU_HSI_QCH (CLK_HIS_BASE + 1) > +#define GATE_MMC_CARD_QCH (CLK_HIS_BASE + 2) > +#define GATE_USB20DRD_TOP_QCH_LINK (CLK_HIS_BASE + 3) > +#define GATE_USB20DRD_TOP_QCH_20CTRL (CLK_HIS_BASE + 4) > +#define GATE_USB20DRD_TOP_QCH_REFCLK (CLK_HIS_BASE + 5) > +#define GATE_USB20DRD_TOP_QCH_RTC (CLK_HIS_BASE + 6) > +#define PLL_MMC_OUT (CLK_HIS_BASE + 7) > +#define HSI_BUS (CLK_HIS_BASE + 8) Please prefix HSI/IS/MFCM with something reasonable. Most of other bindings prefix everything with CLK or ACLK/SCLK/PCLK/PHYCLK/DOUT, but having GATE/DOUT/MUX is reasonable as well. However here and further you miss prefixes. > +#define HSI_MMC_CARD (CLK_HIS_BASE + 9) > +#define HSI_USB20DRD (CLK_HIS_BASE + 10) > + > +#define CLK_IS_BASE (450) > +#define GATE_CSIS0_QCH (CLK_IS_BASE + 0) > +#define GATE_CSIS1_QCH (CLK_IS_BASE + 1) > +#define GATE_CSIS2_QCH (CLK_IS_BASE + 2) > +#define GATE_IS_CMU_IS_QCH (CLK_IS_BASE + 3) > +#define GATE_IS_TOP_QCH_S_00 (CLK_IS_BASE + 4) > +#define GATE_IS_TOP_QCH_S_02 (CLK_IS_BASE + 5) > +#define GATE_IS_TOP_QCH_S_03 (CLK_IS_BASE + 6) > +#define GATE_IS_TOP_QCH_S_04 (CLK_IS_BASE + 7) > +#define GATE_IS_TOP_QCH_S_05 (CLK_IS_BASE + 8) > +#define GATE_IS_TOP_QCH_S_06 (CLK_IS_BASE + 9) > +#define GATE_SYSMMU_IS0_QCH (CLK_IS_BASE + 10) > +#define GATE_SYSMMU_IS1_QCH (CLK_IS_BASE + 11) > +#define IS_BUS (CLK_IS_BASE + 12) > +#define IS_VRA (CLK_IS_BASE + 13) > +#define IS_ITP (CLK_IS_BASE + 14) > +#define IS_GDC (CLK_IS_BASE + 15) > +#define UMUX_CLK_IS_BUS (CLK_IS_BASE + 15) > +#define UMUX_CLK_IS_ITP (CLK_IS_BASE + 16) > +#define UMUX_CLK_IS_VRA (CLK_IS_BASE + 17) > +#define UMUX_CLK_IS_GDC (CLK_IS_BASE + 18) > +#define GATE_CLK_ITP (CLK_IS_BASE + 19) > +#define GATE_CLK_VRA (CLK_IS_BASE + 20) > +#define GATE_CLK_GDC (CLK_IS_BASE + 21) > +#define CIS_CLK0 (CLK_IS_BASE + 22) > +#define CIS_CLK1 (CLK_IS_BASE + 23) > +#define CIS_CLK2 (CLK_IS_BASE + 24) > + > +#define CLK_MFCMSCL_BASE (500) > +#define GATE_JPEG_QCH (CLK_MFCMSCL_BASE + 0) > +#define GATE_M2M_QCH (CLK_MFCMSCL_BASE + 1) > +#define GATE_MCSC_QCH (CLK_MFCMSCL_BASE + 2) > +#define GATE_MFC_QCH (CLK_MFCMSCL_BASE + 3) > +#define GATE_MFCMSCL_CMU_MFCMSCL_QCH (CLK_MFCMSCL_BASE + 4) > +#define GATE_SYSMMU_MFCMSCL_QCH (CLK_MFCMSCL_BASE + 5) > +#define GATE_CMU_MIF_CMUREF_QCH (CLK_MFCMSCL_BASE + 6) > +#define GATE_DMC_QCH (CLK_MFCMSCL_BASE + 7) > +#define GATE_MIF_CMU_MIF_QCH (CLK_MFCMSCL_BASE + 8) > +#define GATE_CMU_MIF1_CMU_REF_QCH (CLK_MFCMSCL_BASE + 9) > +#define GATE_DMC1_QCH (CLK_MFCMSCL_BASE + 10) > +#define GATE_MIF1_CMU_MIF1_QCH (CLK_MFCMSCL_BASE + 11) > +#define GATE_MODEM_CMU_MODEM_QCH (CLK_MFCMSCL_BASE + 12) > +#define DOUT_CLK_MFCMSCL_BUSP (CLK_MFCMSCL_BASE + 13) > +#define MFCMSCL_MFC (CLK_MFCMSCL_BASE + 14) > +#define MFCMSCL_M2M (CLK_MFCMSCL_BASE + 15) > +#define MFCMSCL_MCSC (CLK_MFCMSCL_BASE + 16) > +#define MFCMSCL_JPEG (CLK_MFCMSCL_BASE + 17) > +#define UMUX_CLKCMU_MFCMSCL_MFC (CLK_MFCMSCL_BASE + 18) > +#define UMUX_CLKCMU_MFCMSCL_M2M (CLK_MFCMSCL_BASE + 19) > +#define UMUX_CLKCMU_MFCMSCL_MCSC (CLK_MFCMSCL_BASE + 20) > +#define UMUX_CLKCMU_MFCMSCL_JPEG (CLK_MFCMSCL_BASE + 21) > + > +#define CLK_PERI_BASE (550) > +#define UMUX_CLKCMU_PERI_BUS_USER (CLK_PERI_BASE + 0) > +#define UMUX_CLKCMU_PERI_UART_USER (CLK_PERI_BASE + 1) > +#define UMUX_CLKCMU_PERI_HSI2C_USER (CLK_PERI_BASE + 2) > +#define UMUX_CLKCMU_PERI_SPI_USER (CLK_PERI_BASE + 3) > +#define GATE_BUSIF_TMU_QCH (CLK_PERI_BASE + 4) > +#define GATE_GPIO_PERI_QCH (CLK_PERI_BASE + 5) > +#define GATE_HSI2C_0_QCH (CLK_PERI_BASE + 6) > +#define GATE_HSI2C_1_QCH (CLK_PERI_BASE + 7) > +#define GATE_HSI2C_2_QCH (CLK_PERI_BASE + 8) > +#define GATE_I2C_0_QCH (CLK_PERI_BASE + 9) > +#define GATE_I2C_1_QCH (CLK_PERI_BASE + 10) > +#define GATE_I2C_2_QCH (CLK_PERI_BASE + 11) > +#define GATE_I2C_3_QCH (CLK_PERI_BASE + 12) > +#define GATE_I2C_4_QCH (CLK_PERI_BASE + 13) > +#define GATE_I2C_5_QCH (CLK_PERI_BASE + 14) > +#define GATE_I2C_6_QCH (CLK_PERI_BASE + 15) > +#define GATE_MCT_QCH (CLK_PERI_BASE + 16) > +#define GATE_OTP_CON_TOP_QCH (CLK_PERI_BASE + 17) > +#define GATE_PWM_MOTOR_QCH (CLK_PERI_BASE + 18) > +#define GATE_SPI_0_QCH (CLK_PERI_BASE + 19) > +#define GATE_UART_QCH (CLK_PERI_BASE + 20) > +#define GATE_WDT_0_QCH (CLK_PERI_BASE + 21) > +#define GATE_WDT_1_QCH (CLK_PERI_BASE + 22) > +#define DOUT_CLK_PERI_SPI_0 (CLK_PERI_BASE + 23) > +#define DOUT_CLK_PERI_HSI2C_0 (CLK_PERI_BASE + 24) > +#define DOUT_CLK_PERI_HSI2C_1 (CLK_PERI_BASE + 25) > +#define DOUT_CLK_PERI_HSI2C_2 (CLK_PERI_BASE + 26) > +#define DOUT_I2C_0 (CLK_PERI_BASE + 27) > +#define DOUT_I2C_1 (CLK_PERI_BASE + 28) > +#define DOUT_I2C_2 (CLK_PERI_BASE + 29) > +#define DOUT_I2C_3 (CLK_PERI_BASE + 30) > +#define DOUT_I2C_4 (CLK_PERI_BASE + 31) > +#define DOUT_I2C_5 (CLK_PERI_BASE + 32) > +#define DOUT_I2C_6 (CLK_PERI_BASE + 33) > +#define DOUT_UART (CLK_PERI_BASE + 34) > + > +#define CLK_CLKOUT_BASE (700) > +#define OSC_NFC (CLK_CLKOUT_BASE + 0) > +#define OSC_AUD (CLK_CLKOUT_BASE + 1) What are these? > + > +/* Must be greater than maximal clock ID */ Why? > +#define CLK_NR_CLKS (1125 + 1) > + > +#endif /* _DT_BINDINGS_CLOCK_EXYNOS_850_H */ > Best regards, Krzysztof
On 30/07/2021 16:49, Sam Protsenko wrote: > UART block is a part of USI (Universal Serial Interface) IP-core in > Samsung SoCs since Exynos9810 (e.g. in Exynos850). USI allows one to > enable one of three types of serial interface: UART, SPI or I2C. That's > possible because USI shares almost all internal circuits within each > protocol. USI also provides some additional registers so it's possible > to configure it. > > One USI register called USI_OPTION has reset value of 0x0. Because of > this the clock gating behavior is controlled by hardware (HWACG = > Hardware Auto Clock Gating), which simply means the serial won't work > after reset as is. In order to make it work, USI_OPTION[2:1] bits must > be set to 0b01, so that HWACG is controlled manually (by software). > Bits meaning: > - CLKREQ_ON = 1: clock is continuously provided to IP > - CLKSTOP_ON = 0: drive IP_CLKREQ to High (needs to be set along with > CLKREQ_ON = 1) > > USI is not present on older chips, like s3c2410, s3c2412, s3c2440, > s3c6400, s5pv210, exynos5433, exynos4210. So the new boolean field > '.has_usi' was added to struct s3c24xx_uart_info. USI registers will be > only actually accessed when '.has_usi' field is set to "1". > > This feature is needed for further serial enablement on Exynos850, but > some other new Exynos chips (like Exynos9810) may benefit from this > feature as well. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > drivers/tty/serial/samsung_tty.c | 33 +++++++++++++++++++++++++++++++- > include/linux/serial_s3c.h | 9 +++++++++ > 2 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > index 9fbc61151c2e..0f3cbd0b37e3 100644 > --- a/drivers/tty/serial/samsung_tty.c > +++ b/drivers/tty/serial/samsung_tty.c > @@ -65,6 +65,7 @@ enum s3c24xx_port_type { > struct s3c24xx_uart_info { > char *name; > enum s3c24xx_port_type type; > + unsigned int has_usi; > unsigned int port_type; > unsigned int fifosize; > unsigned long rx_fifomask; > @@ -1352,6 +1353,29 @@ static int apple_s5l_serial_startup(struct uart_port *port) > return ret; > } > > +static void exynos_usi_init(struct uart_port *port) > +{ > + struct s3c24xx_uart_port *ourport = to_ourport(port); > + struct s3c24xx_uart_info *info = ourport->info; > + > + if (!info->has_usi) > + return; > + > + /* > + * USI_RESET is an active High signal. Reset value of USI_RESET is 0x1 > + * to drive stable value to PAD. Due to this feature, the USI_RESET must > + * be cleared (set as 0x0) before starting a transaction. "before starting a transaction" suggests it is related with transaction or something before starting it. Don't you need it simply after reset or resume? > + */ > + wr_regl(port, USI_CON, USI_RESET); You are clearing entire register, not only USI_RESET bitfield. Is it really what you want? > + udelay(1); > + > + /* > + * Set the HWACG option bit in case of UART Rx mode. > + * CLKREQ_ON = 1, CLKSTOP_ON = 0 (set USI_OPTION[2:1] = 0x1). > + */ > + wr_regl(port, USI_OPTION, USI_HWACG_CLKREQ_ON); > +} > + > /* power power management control */ > > static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level, > @@ -1379,6 +1403,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level, > if (!IS_ERR(ourport->baudclk)) > clk_prepare_enable(ourport->baudclk); > > + exynos_usi_init(port); > break; > default: > dev_err(port->dev, "s3c24xx_serial: unknown pm %d\n", level); > @@ -2102,6 +2127,8 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport, > if (ret) > pr_warn("uart: failed to enable baudclk\n"); > > + exynos_usi_init(port); > + > /* Keep all interrupts masked and cleared */ > switch (ourport->info->type) { > case TYPE_S3C6400: > @@ -2750,10 +2777,11 @@ static struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = { > #endif > > #if defined(CONFIG_ARCH_EXYNOS) > -#define EXYNOS_COMMON_SERIAL_DRV_DATA \ > +#define EXYNOS_COMMON_SERIAL_DRV_DATA_USI(_has_usi) \ > .info = &(struct s3c24xx_uart_info) { \ > .name = "Samsung Exynos UART", \ > .type = TYPE_S3C6400, \ > + .has_usi = _has_usi, \ > .port_type = PORT_S3C6400, \ > .has_divslot = 1, \ > .rx_fifomask = S5PV210_UFSTAT_RXMASK, \ > @@ -2773,6 +2801,9 @@ static struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = { > .has_fracval = 1, \ > } \ > > +#define EXYNOS_COMMON_SERIAL_DRV_DATA \ > + EXYNOS_COMMON_SERIAL_DRV_DATA_USI(0) > + > static struct s3c24xx_serial_drv_data exynos4210_serial_drv_data = { > EXYNOS_COMMON_SERIAL_DRV_DATA, > .fifosize = { 256, 64, 16, 16 }, > diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h > index f6c3323fc4c5..013c2646863e 100644 > --- a/include/linux/serial_s3c.h > +++ b/include/linux/serial_s3c.h > @@ -28,6 +28,15 @@ > #define S3C2410_UFSTAT (0x18) > #define S3C2410_UMSTAT (0x1C) > > +/* USI Control Register offset */ > +#define USI_CON (0xC4) > +/* USI Option Register offset */ > +#define USI_OPTION (0xC8) > +/* USI_CON[0] = 0b0: clear USI global software reset (Active High) */ > +#define USI_RESET (0<<0) Just 0x0. I understand you wanted to hint it is a bit field, but the shift of 0 actually creates more questions. Best regards, Krzysztof
On 2021-07-30 15:49, Sam Protsenko wrote: > Samsung Exynos850 is ARMv8-based mobile-oriented SoC. > > Features: > * CPU: Cortex-A55 Octa (8 cores), up to 2 GHz > * Memory interface: LPDDR4/4x 2 channels (12.8 GB/s) > * SD/MMC: SD 3.0, eMMC5.1 DDR 8-bit > * Modem: 4G LTE, 3G, GSM/GPRS/EDGE > * RF: Quad GNSS, WiFi 5 (802.11ac), Bluetooth 5.0 > * GPU: Mali-G52 MP1 > * Codec: 1080p 60fps H64, HEVC, JPEG HW Codec > * Display: Full HD+ (2520x1080)@60fps LCD > * Camera: 16+5MP/13+8MP ISP, MIPI CSI 4/4/2, FD, DRC > * Connectivity: USB 2.0 DRD, USI (SPI/UART/I2C), HSI2C, I3C, ADC, > Audio > > This patch adds minimal SoC support. Particular board device tree files > can include exynos850.dtsi file to get SoC related nodes, and then > reference those nodes further as needed. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > .../boot/dts/exynos/exynos850-pinctrl.dtsi | 782 ++++++++++++++++++ > arch/arm64/boot/dts/exynos/exynos850-usi.dtsi | 30 + > arch/arm64/boot/dts/exynos/exynos850.dtsi | 245 ++++++ > 3 files changed, 1057 insertions(+) > create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi > create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi > create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi > > diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi > b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi > new file mode 100644 > index 000000000000..4cf0a22cc6db [...] > + gic: interrupt-controller@12a00000 { > + compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; One thing for sure, it cannot be both. And given that it is an A55-based SoC, it isn't either. It is more likely a GIC400. > + #interrupt-cells = <3>; > + #address-cells = <0>; > + interrupt-controller; > + reg = <0x0 0x12a01000 0x1000>, > + <0x0 0x12a02000 0x1000>, This is wrong. It is architecturally set to 8kB. > + <0x0 0x12a04000 0x2000>, > + <0x0 0x12a06000 0x2000>; > + interrupts = <GIC_PPI 9 > + (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; 4? With 8 CPUs? I also find it curious that you went through the unusual (and IMO confusing) effort to allocate a name to each and every SPI in the system, but didn't do it for any on the PPIs... > + }; > + > + timer { > + compatible = "arm,armv8-timer"; > + interrupts = <GIC_PPI 13 > + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 14 > + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 11 > + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 10 > + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>; > + clock-frequency = <26000000>; No, please. Fix the firmware to program CNTFRQ_EL0 on each and every CPU. This isn't 2012 anymore. You are also missing the hypervisor virtual timer interrupt. > + use-clocksource-only; > + use-physical-timer; Thankfully, these two properties do not exist. Thanks, M.
On 30/07/2021 17:18, Krzysztof Kozlowski wrote: > On 30/07/2021 16:49, Sam Protsenko wrote: >> This patch series adds initial platform support for Samsung Exynos850 >> SoC [1]. With this patchset it's possible to run the kernel with BusyBox >> rootfs as a RAM disk. More advanced platform support (like MMC driver >> additions) will be added later. The idea is to keep the first submission >> minimal to ease the review, and then build up on top of that. >> >> [1] https://www.samsung.com/semiconductor/minisite/exynos/products/mobileprocessor/exynos-850/ >> > > Great work! > > What's the SoC revision number (should be accessible via > /sys/bus/soc/devices/soc0/)? Recent wrap in numbering of Exynos chips > might bring confusion... Judging by vendor's sources it is quite confusing. It looks mostly like Exynos3830 but in few other cases it uses Exynos9 compatibles (Exynos9, Exynos9820). Only in few places there is Exynos850. Marketing department made it so confusing... The revision embedded in SoC would be very interesting. Anyway, judging by current versioning, there is a risk Samsung will come with a new chipset name conflicting with existing ones. It already overflowed. It's even worse with a thingy called "Exynos9 auto" which hides numbering even more. Best regards, Krzysztof
Hi Andy, On Fri, 30 Jul 2021 at 18:12, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Jul 30, 2021 at 5:51 PM Sam Protsenko > <semen.protsenko@linaro.org> wrote: > > > > For now it's just a stub driver to make serial driver work. Later it > > make the serial > > > will be implemented properly. > > > > This driver doesn't really change clocks, only registers the UART clock > > as a fixed-rate clock. Without this clock driver the UART driver won't > > work, as it's trying to obtain "uart" clock and fails if it's not able > > to. > > > > From drivers/tty/serial/samsung_tty.c: > > > > 8<------------------------------------------------------------------->8 > > ourport->clk = clk_get(&platdev->dev, "uart"); > > if (IS_ERR(ourport->clk)) { > > pr_err("%s: Controller clock not found\n", > > dev_name(&platdev->dev)); > > ret = PTR_ERR(ourport->clk); > > goto err; > > } > > 8<------------------------------------------------------------------->8 > > This is not needed in the commit message. > > > In order to get functional serial console we have to implement that > > get a functional > > > minimal clock driver with "uart" clock. It's not necessary to actually > > configure clocks, as those are already configured in bootloader, so > > kernel can rely on that for now. > > > 80 column limit is broken here to make checkpatch happy, otherwise it > > swears about incorrect __initconst usage. > > Again, no need to be in the commit message, use the comment field for > this (after the cutter '---' line below). > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > drivers/clk/samsung/Makefile | 1 + > > drivers/clk/samsung/clk-exynos850.c | 63 +++++++++++++++++++++++++++++ > > 2 files changed, 64 insertions(+) > > create mode 100644 drivers/clk/samsung/clk-exynos850.c > > > > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile > > index 028b2e27a37e..c46cf11e4d0b 100644 > > --- a/drivers/clk/samsung/Makefile > > +++ b/drivers/clk/samsung/Makefile > > @@ -17,6 +17,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos5433.o > > obj-$(CONFIG_EXYNOS_AUDSS_CLK_CON) += clk-exynos-audss.o > > obj-$(CONFIG_EXYNOS_CLKOUT) += clk-exynos-clkout.o > > obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos7.o > > +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos850.o > > obj-$(CONFIG_S3C2410_COMMON_CLK)+= clk-s3c2410.o > > obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o > > obj-$(CONFIG_S3C2412_COMMON_CLK)+= clk-s3c2412.o > > diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c > > new file mode 100644 > > index 000000000000..3192ec9bb90b > > --- /dev/null > > +++ b/drivers/clk/samsung/clk-exynos850.c > > @@ -0,0 +1,63 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (C) 2019 Samsung Electronics Co., Ltd. > > + * Copyright (C) 2021 Linaro Ltd. > > + * > > + * Common Clock Framework support for Exynos850 SoC. > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/clkdev.h> > > +#include <linux/clk-provider.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > + blank line? > > > +#include <dt-bindings/clock/exynos850.h> > > + > > +#include "clk.h" > > + > > +/* Fixed rate clocks generated outside the SoC */ > > +static struct samsung_fixed_rate_clock exynos850_fixed_rate_ext_clks[] __initdata = { > > + FRATE(OSCCLK, "fin_pll", NULL, 0, 26000000), > > +}; > > + > > +/* > > + * Model the UART clock as a fixed-rate clock for now, to make serial driver > > + * work. This clock is already configured in the bootloader. > > + */ > > +static const struct samsung_fixed_rate_clock exynos850_peri_clks[] __initconst = { > > + FRATE(DOUT_UART, "DOUT_UART", NULL, 0, 200000000), > > +}; > > + > > +static const struct of_device_id ext_clk_match[] __initconst = { > > > + { .compatible = "samsung,exynos850-oscclk", .data = (void *)0 }, > > 0 is the default for static variables. > > > + {}, > > No comma needed for the terminator lines. > > > +}; > > + > > +void __init exynos850_clk_init(struct device_node *np) > > +{ > > + void __iomem *reg_base; > > + struct samsung_clk_provider *ctx; > > > + if (!np) > > + panic("%s: unable to determine soc\n", __func__); > > Check for the sake of additional code? > > > + reg_base = of_iomap(np, 0); > > This will fail when np == NULL. > Thanks for the review! All your comments will be addressed in v2. > > + if (!reg_base) > > + panic("%s: failed to map registers\n", __func__); > > + > > + ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS); > > + if (!ctx) > > + panic("%s: unable to allocate ctx\n", __func__); > > + > > + samsung_clk_of_register_fixed_ext(ctx, > > + exynos850_fixed_rate_ext_clks, > > + ARRAY_SIZE(exynos850_fixed_rate_ext_clks), > > + ext_clk_match); > > + > > + samsung_clk_register_fixed_rate(ctx, exynos850_peri_clks, > > + ARRAY_SIZE(exynos850_peri_clks)); > > + > > + samsung_clk_of_add_provider(np, ctx); > > +} > > + > > +CLK_OF_DECLARE(exynos850_clk, "samsung,exynos850-clock", exynos850_clk_init); > > > -- > With Best Regards, > Andy Shevchenko
On Fri, 30 Jul 2021 at 18:25, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 30/07/2021 17:06, Andy Shevchenko wrote: > > On Fri, Jul 30, 2021 at 5:50 PM Sam Protsenko > > <semen.protsenko@linaro.org> wrote: > >> > >> New device tree bindings are usually added in YAML format. Fix "SAMSUNG > >> SOC CLOCK DRIVERS" entry to cover both txt and yaml docs for Exynos > >> clock drivers. > > > > Fixes tag? > > No need because the pattern is correct at the moment. The patch does not > make sense on its own and should be squashed with the next one. > Nice catch. Will do in v2, thanks! > > Best regards, > Krzysztof
Hi Krzysztof, On Fri, 30 Jul 2021 at 20:21, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 30/07/2021 17:18, Krzysztof Kozlowski wrote: > > On 30/07/2021 16:49, Sam Protsenko wrote: > >> This patch series adds initial platform support for Samsung Exynos850 > >> SoC [1]. With this patchset it's possible to run the kernel with BusyBox > >> rootfs as a RAM disk. More advanced platform support (like MMC driver > >> additions) will be added later. The idea is to keep the first submission > >> minimal to ease the review, and then build up on top of that. > >> > >> [1] https://www.samsung.com/semiconductor/minisite/exynos/products/mobileprocessor/exynos-850/ > >> > > > > Great work! > > Thanks, Krzysztof! And thank you for reviewing the whole series. > > What's the SoC revision number (should be accessible via > > /sys/bus/soc/devices/soc0/)? Recent wrap in numbering of Exynos chips > > might bring confusion... # cat /sys/devices/soc0/revision 0 > > Judging by vendor's sources it is quite confusing. It looks mostly like > Exynos3830 but in few other cases it uses Exynos9 compatibles (Exynos9, > Exynos9820). Only in few places there is Exynos850. Marketing department > made it so confusing... The revision embedded in SoC would be very > interesting. > As I understand, this SoC is called Exynos850 everywhere now. Exynos3830 is its old name, not used anymore. As you noticed from patch #2, it shares some definitions with Exynos9 SoC, so I guess some software is similar for both architectures. Not sure about hardware though, never worked with Exynos9 CPUs. Anyway, I asked Samsung representatives about naming, and it seems like we should stick to "Exynos850" name, even in code. > Anyway, judging by current versioning, there is a risk Samsung will come > with a new chipset name conflicting with existing ones. It already > overflowed. > > It's even worse with a thingy called "Exynos9 auto" which hides > numbering even more. > > > Best regards, > Krzysztof
On Fri, 30 Jul 2021 at 18:25, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 30/07/2021 16:49, Sam Protsenko wrote: > > Document compatible string for Exynos850 SoC. Nothing else is changed, > > as Exynos850 SoC uses already existing samsung pinctrl driver. > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 1 + > > 1 file changed, 1 insertion(+) > > > > The patch should be first in the series - dt-bindings go at beginning. > Although no need to resend just for this. > If the resend happens, the fix ("pinctrl: samsung: Fix pinctrl bank pin > count") should be sent separately (no mixing fixes and new features) > because they are independent and usually easier for review. > Thanks, will be done in v2. Also, just re-sent ("pinctrl: samsung: Fix pinctrl bank pin count") fix separately, as you recommended. > > Best regards, > Krzysztof
On 30/07/2021 21:02, Sam Protsenko wrote: > Hi Krzysztof, > > On Fri, 30 Jul 2021 at 20:21, Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: >> >> On 30/07/2021 17:18, Krzysztof Kozlowski wrote: >>> On 30/07/2021 16:49, Sam Protsenko wrote: >>>> This patch series adds initial platform support for Samsung Exynos850 >>>> SoC [1]. With this patchset it's possible to run the kernel with BusyBox >>>> rootfs as a RAM disk. More advanced platform support (like MMC driver >>>> additions) will be added later. The idea is to keep the first submission >>>> minimal to ease the review, and then build up on top of that. >>>> >>>> [1] https://www.samsung.com/semiconductor/minisite/exynos/products/mobileprocessor/exynos-850/ >>>> >>> >>> Great work! >>> > > Thanks, Krzysztof! And thank you for reviewing the whole series. > >>> What's the SoC revision number (should be accessible via >>> /sys/bus/soc/devices/soc0/)? Recent wrap in numbering of Exynos chips >>> might bring confusion... > > # cat /sys/devices/soc0/revision > 0 soc_id but you're right it won't be set for unknown SoCs. You need to extend drivers/soc/samsung/exynos-chipid.c to parse new values (E3830000 for product ID) and maybe new register offsets (previous offset is 0x0, for 3830 is 0x10 I think). Also revision mask might change. >> Judging by vendor's sources it is quite confusing. It looks mostly like >> Exynos3830 but in few other cases it uses Exynos9 compatibles (Exynos9, >> Exynos9820). Only in few places there is Exynos850. Marketing department >> made it so confusing... The revision embedded in SoC would be very >> interesting. >> > > As I understand, this SoC is called Exynos850 everywhere now. > Exynos3830 is its old name, not used anymore. As you noticed from > patch #2, it shares some definitions with Exynos9 SoC, so I guess some > software is similar for both architectures. Not sure about hardware > though, never worked with Exynos9 CPUs. Anyway, I asked Samsung > representatives about naming, and it seems like we should stick to > "Exynos850" name, even in code. Since the chip identifies itself as E3830000, I would prefer naming matching real product ID instead of what is pushed by marketing or sales representatives. The marketing names don't have to follow any engineering rules, they can be changed and renamed. Sales follows rather money and corporate rules, not consistency for upstream project. Best regards, Krzysztof
On 31/07/2021 09:29, Krzysztof Kozlowski wrote: > On 30/07/2021 21:02, Sam Protsenko wrote: >> Hi Krzysztof, >> >> On Fri, 30 Jul 2021 at 20:21, Krzysztof Kozlowski >> <krzysztof.kozlowski@canonical.com> wrote: >>> >>> On 30/07/2021 17:18, Krzysztof Kozlowski wrote: >>>> On 30/07/2021 16:49, Sam Protsenko wrote: >>>>> This patch series adds initial platform support for Samsung Exynos850 >>>>> SoC [1]. With this patchset it's possible to run the kernel with BusyBox >>>>> rootfs as a RAM disk. More advanced platform support (like MMC driver >>>>> additions) will be added later. The idea is to keep the first submission >>>>> minimal to ease the review, and then build up on top of that. >>>>> >>>>> [1] https://www.samsung.com/semiconductor/minisite/exynos/products/mobileprocessor/exynos-850/ >>>>> >>>> >>>> Great work! >>>> >> >> Thanks, Krzysztof! And thank you for reviewing the whole series. >> >>>> What's the SoC revision number (should be accessible via >>>> /sys/bus/soc/devices/soc0/)? Recent wrap in numbering of Exynos chips >>>> might bring confusion... >> >> # cat /sys/devices/soc0/revision >> 0 > > soc_id but you're right it won't be set for unknown SoCs. You need to > extend drivers/soc/samsung/exynos-chipid.c to parse new values (E3830000 > for product ID) and maybe new register offsets (previous offset is 0x0, > for 3830 is 0x10 I think). Also revision mask might change. > >>> Judging by vendor's sources it is quite confusing. It looks mostly like >>> Exynos3830 but in few other cases it uses Exynos9 compatibles (Exynos9, >>> Exynos9820). Only in few places there is Exynos850. Marketing department >>> made it so confusing... The revision embedded in SoC would be very >>> interesting. >>> >> >> As I understand, this SoC is called Exynos850 everywhere now. >> Exynos3830 is its old name, not used anymore. As you noticed from >> patch #2, it shares some definitions with Exynos9 SoC, so I guess some >> software is similar for both architectures. Not sure about hardware >> though, never worked with Exynos9 CPUs. Anyway, I asked Samsung >> representatives about naming, and it seems like we should stick to >> "Exynos850" name, even in code. > > > Since the chip identifies itself as E3830000, I would prefer naming > matching real product ID instead of what is pushed by marketing or sales > representatives. The marketing names don't have to follow any > engineering rules, they can be changed and renamed. Sales follows rather > money and corporate rules, not consistency for upstream project. On the other hand we have already two exceptions for naming inconsistency - Exynos3250 identifies itself as 3472 (which is confusing because 3250 is two core and there is a separate quad-core Exyons3472...) and Exynos5800 is actually marketing name for a revision of Exynos5422. Maybe indeed will be easier to go with the branded name 850... Best regards, Krzysztof
On Fri, 30 Jul 2021 at 19:32, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 30/07/2021 16:49, Sam Protsenko wrote: > > UART block is a part of USI (Universal Serial Interface) IP-core in > > Samsung SoCs since Exynos9810 (e.g. in Exynos850). USI allows one to > > enable one of three types of serial interface: UART, SPI or I2C. That's > > possible because USI shares almost all internal circuits within each > > protocol. USI also provides some additional registers so it's possible > > to configure it. > > > > One USI register called USI_OPTION has reset value of 0x0. Because of > > this the clock gating behavior is controlled by hardware (HWACG = > > Hardware Auto Clock Gating), which simply means the serial won't work > > after reset as is. In order to make it work, USI_OPTION[2:1] bits must > > be set to 0b01, so that HWACG is controlled manually (by software). > > Bits meaning: > > - CLKREQ_ON = 1: clock is continuously provided to IP > > - CLKSTOP_ON = 0: drive IP_CLKREQ to High (needs to be set along with > > CLKREQ_ON = 1) > > > > USI is not present on older chips, like s3c2410, s3c2412, s3c2440, > > s3c6400, s5pv210, exynos5433, exynos4210. So the new boolean field > > '.has_usi' was added to struct s3c24xx_uart_info. USI registers will be > > only actually accessed when '.has_usi' field is set to "1". > > > > This feature is needed for further serial enablement on Exynos850, but > > some other new Exynos chips (like Exynos9810) may benefit from this > > feature as well. > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > drivers/tty/serial/samsung_tty.c | 33 +++++++++++++++++++++++++++++++- > > include/linux/serial_s3c.h | 9 +++++++++ > > 2 files changed, 41 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > > index 9fbc61151c2e..0f3cbd0b37e3 100644 > > --- a/drivers/tty/serial/samsung_tty.c > > +++ b/drivers/tty/serial/samsung_tty.c > > @@ -65,6 +65,7 @@ enum s3c24xx_port_type { > > struct s3c24xx_uart_info { > > char *name; > > enum s3c24xx_port_type type; > > + unsigned int has_usi; > > unsigned int port_type; > > unsigned int fifosize; > > unsigned long rx_fifomask; > > @@ -1352,6 +1353,29 @@ static int apple_s5l_serial_startup(struct uart_port *port) > > return ret; > > } > > > > +static void exynos_usi_init(struct uart_port *port) > > +{ > > + struct s3c24xx_uart_port *ourport = to_ourport(port); > > + struct s3c24xx_uart_info *info = ourport->info; > > + > > + if (!info->has_usi) > > + return; > > + > > + /* > > + * USI_RESET is an active High signal. Reset value of USI_RESET is 0x1 > > + * to drive stable value to PAD. Due to this feature, the USI_RESET must > > + * be cleared (set as 0x0) before starting a transaction. > > "before starting a transaction" suggests it is related with transaction > or something before starting it. Don't you need it simply after reset or > resume? > Not sure what you are suggesting. USI_RESET is set to "1" at start up (means USI block hangs in reset state), so we have to make it "0" (that's what this code does); only then UART becomes functional and UART transactions can be performed. And exynos_usi_init() is called exactly where you hinted: at init and on resume. Anyway, the whole comment is confusing, I'll simplify and rework it in v2. Please let me know if I'm missing the point though. > > + */ > > + wr_regl(port, USI_CON, USI_RESET); > > You are clearing entire register, not only USI_RESET bitfield. Is it > really what you want? > Yeah, USI_CON[31:1] bits are reserved, and the reset value of this register is 0x00000001. But anyway, I'm going to rework that code like this, for clarity and consistence: 8<--------------------------------------------------------------------->8 /* Clear the software reset of USI block (it's set at startup) */ val = rd_regl(port, USI_CON); val &= ~(USI_RESET_MASK) wr_regl(port, USI_CON, val); udelay(1); /* Continuously provide the clock to USI IP w/o gating (for Rx mode) */ val = rd_regl(port, USI_OPTION); val &= ~USI_HWACG_MASK; val |= USI_HWACG_CLKREQ_ON; wr_regl(port, USI_OPTION, val); 8<--------------------------------------------------------------------->8 > > + udelay(1); > > + > > + /* > > + * Set the HWACG option bit in case of UART Rx mode. > > + * CLKREQ_ON = 1, CLKSTOP_ON = 0 (set USI_OPTION[2:1] = 0x1). > > + */ > > + wr_regl(port, USI_OPTION, USI_HWACG_CLKREQ_ON); > > +} > > + > > /* power power management control */ > > > > static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level, > > @@ -1379,6 +1403,7 @@ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level, > > if (!IS_ERR(ourport->baudclk)) > > clk_prepare_enable(ourport->baudclk); > > > > + exynos_usi_init(port); > > break; > > default: > > dev_err(port->dev, "s3c24xx_serial: unknown pm %d\n", level); > > @@ -2102,6 +2127,8 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport, > > if (ret) > > pr_warn("uart: failed to enable baudclk\n"); > > > > + exynos_usi_init(port); > > + > > /* Keep all interrupts masked and cleared */ > > switch (ourport->info->type) { > > case TYPE_S3C6400: > > @@ -2750,10 +2777,11 @@ static struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = { > > #endif > > > > #if defined(CONFIG_ARCH_EXYNOS) > > -#define EXYNOS_COMMON_SERIAL_DRV_DATA \ > > +#define EXYNOS_COMMON_SERIAL_DRV_DATA_USI(_has_usi) \ > > .info = &(struct s3c24xx_uart_info) { \ > > .name = "Samsung Exynos UART", \ > > .type = TYPE_S3C6400, \ > > + .has_usi = _has_usi, \ > > .port_type = PORT_S3C6400, \ > > .has_divslot = 1, \ > > .rx_fifomask = S5PV210_UFSTAT_RXMASK, \ > > @@ -2773,6 +2801,9 @@ static struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = { > > .has_fracval = 1, \ > > } \ > > > > +#define EXYNOS_COMMON_SERIAL_DRV_DATA \ > > + EXYNOS_COMMON_SERIAL_DRV_DATA_USI(0) > > + > > static struct s3c24xx_serial_drv_data exynos4210_serial_drv_data = { > > EXYNOS_COMMON_SERIAL_DRV_DATA, > > .fifosize = { 256, 64, 16, 16 }, > > diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h > > index f6c3323fc4c5..013c2646863e 100644 > > --- a/include/linux/serial_s3c.h > > +++ b/include/linux/serial_s3c.h > > @@ -28,6 +28,15 @@ > > #define S3C2410_UFSTAT (0x18) > > #define S3C2410_UMSTAT (0x1C) > > > > +/* USI Control Register offset */ > > +#define USI_CON (0xC4) > > +/* USI Option Register offset */ > > +#define USI_OPTION (0xC8) > > +/* USI_CON[0] = 0b0: clear USI global software reset (Active High) */ > > +#define USI_RESET (0<<0) > > Just 0x0. I understand you wanted to hint it is a bit field, but the > shift of 0 actually creates more questions. > After some consideration I decided to adhere to existing style and do something like this (in v2): 8<--------------------------------------------------------------------->8 #define USI_CON (0xC4) #define USI_OPTION (0xC8) #define USI_CON_RESET_CLEAR (0<<0) #define USI_CON_RESET_SET (1<<0) #define USI_CON_RESET_MASK (1<<0) #define USI_OPTION_HWACG_CLKREQ_ON (1<<1) #define USI_OPTION_HWACG_CLKSTOP_ON (1<<2) #define USI_OPTION_HWACG_MASK (3<<1) 8<--------------------------------------------------------------------->8 The whole reason for those comments was missing public TRM. But in the end I decided it just looks ugly. Also, this way I can do RMW operation (discussed above) in more logical way. Please let me know if code snippets above look good to you. > > Best regards, > Krzysztof
On Sat, 31 Jul 2021 at 11:12, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 31/07/2021 09:29, Krzysztof Kozlowski wrote: > > On 30/07/2021 21:02, Sam Protsenko wrote: > >> Hi Krzysztof, > >> > >> On Fri, 30 Jul 2021 at 20:21, Krzysztof Kozlowski > >> <krzysztof.kozlowski@canonical.com> wrote: > >>> > >>> On 30/07/2021 17:18, Krzysztof Kozlowski wrote: > >>>> On 30/07/2021 16:49, Sam Protsenko wrote: > >>>>> This patch series adds initial platform support for Samsung Exynos850 > >>>>> SoC [1]. With this patchset it's possible to run the kernel with BusyBox > >>>>> rootfs as a RAM disk. More advanced platform support (like MMC driver > >>>>> additions) will be added later. The idea is to keep the first submission > >>>>> minimal to ease the review, and then build up on top of that. > >>>>> > >>>>> [1] https://www.samsung.com/semiconductor/minisite/exynos/products/mobileprocessor/exynos-850/ > >>>>> > >>>> > >>>> Great work! > >>>> > >> > >> Thanks, Krzysztof! And thank you for reviewing the whole series. > >> > >>>> What's the SoC revision number (should be accessible via > >>>> /sys/bus/soc/devices/soc0/)? Recent wrap in numbering of Exynos chips > >>>> might bring confusion... > >> > >> # cat /sys/devices/soc0/revision > >> 0 > > > > soc_id but you're right it won't be set for unknown SoCs. You need to > > extend drivers/soc/samsung/exynos-chipid.c to parse new values (E3830000 > > for product ID) and maybe new register offsets (previous offset is 0x0, > > for 3830 is 0x10 I think). Also revision mask might change. > > > >>> Judging by vendor's sources it is quite confusing. It looks mostly like > >>> Exynos3830 but in few other cases it uses Exynos9 compatibles (Exynos9, > >>> Exynos9820). Only in few places there is Exynos850. Marketing department > >>> made it so confusing... The revision embedded in SoC would be very > >>> interesting. > >>> > >> > >> As I understand, this SoC is called Exynos850 everywhere now. > >> Exynos3830 is its old name, not used anymore. As you noticed from > >> patch #2, it shares some definitions with Exynos9 SoC, so I guess some > >> software is similar for both architectures. Not sure about hardware > >> though, never worked with Exynos9 CPUs. Anyway, I asked Samsung > >> representatives about naming, and it seems like we should stick to > >> "Exynos850" name, even in code. > > > > > > Since the chip identifies itself as E3830000, I would prefer naming > > matching real product ID instead of what is pushed by marketing or sales > > representatives. The marketing names don't have to follow any > > engineering rules, they can be changed and renamed. Sales follows rather > > money and corporate rules, not consistency for upstream project. > > On the other hand we have already two exceptions for naming > inconsistency - Exynos3250 identifies itself as 3472 (which is confusing > because 3250 is two core and there is a separate quad-core > Exyons3472...) and Exynos5800 is actually marketing name for a revision > of Exynos5422. Maybe indeed will be easier to go with the branded name > 850... > Well, chip engraving says "3830", but I was specifically told to stick to "850" in upstream kernel. I can presume there was some mix ups with this naming, and it might be the case it's better to stick to "850" exactly to avoid further confusion. Yes, I can see that EXYNOS3830_SOC_ID = 0xE3830000 in chipid driver, but we can return "EXYNOS850" string for that const, right? If you google "Exynos850" and "Exynos3830", it's obvious everybody uses the former, so I'd appreciate if we can stick to "850" in the end. > > Best regards, > Krzysztof
On 03/08/2021 01:06, Sam Protsenko wrote: (...) >>> diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h >>> index f6c3323fc4c5..013c2646863e 100644 >>> --- a/include/linux/serial_s3c.h >>> +++ b/include/linux/serial_s3c.h >>> @@ -28,6 +28,15 @@ >>> #define S3C2410_UFSTAT (0x18) >>> #define S3C2410_UMSTAT (0x1C) >>> >>> +/* USI Control Register offset */ >>> +#define USI_CON (0xC4) >>> +/* USI Option Register offset */ >>> +#define USI_OPTION (0xC8) >>> +/* USI_CON[0] = 0b0: clear USI global software reset (Active High) */ >>> +#define USI_RESET (0<<0) >> >> Just 0x0. I understand you wanted to hint it is a bit field, but the >> shift of 0 actually creates more questions. >> > > After some consideration I decided to adhere to existing style and do > something like this (in v2): > > 8<--------------------------------------------------------------------->8 > #define USI_CON (0xC4) > #define USI_OPTION (0xC8) > > #define USI_CON_RESET_CLEAR (0<<0) > #define USI_CON_RESET_SET (1<<0) > #define USI_CON_RESET_MASK (1<<0) > > #define USI_OPTION_HWACG_CLKREQ_ON (1<<1) > #define USI_OPTION_HWACG_CLKSTOP_ON (1<<2) > #define USI_OPTION_HWACG_MASK (3<<1) > 8<--------------------------------------------------------------------->8 > > The whole reason for those comments was missing public TRM. But in the > end I decided it just looks ugly. Also, this way I can do RMW > operation (discussed above) in more logical way. > > Please let me know if code snippets above look good to you. Please skip the USI_CON_RESET_CLEAR. There is no such pattern in the code. Clearing bit is an obvious operation and such code is already everywhere: val &= ~USI_CON_RESET (or &= ~USI_RESET_MASK) Therefore for USI_CON_RESET only: #define USI_CON_RESET (1<<0) #define USI_CON_RESET_MASK (1<<0) Best regards, Krzysztof
On Tue, 3 Aug 2021 at 10:37, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 03/08/2021 01:06, Sam Protsenko wrote: > > (...) > > >>> diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h > >>> index f6c3323fc4c5..013c2646863e 100644 > >>> --- a/include/linux/serial_s3c.h > >>> +++ b/include/linux/serial_s3c.h > >>> @@ -28,6 +28,15 @@ > >>> #define S3C2410_UFSTAT (0x18) > >>> #define S3C2410_UMSTAT (0x1C) > >>> > >>> +/* USI Control Register offset */ > >>> +#define USI_CON (0xC4) > >>> +/* USI Option Register offset */ > >>> +#define USI_OPTION (0xC8) > >>> +/* USI_CON[0] = 0b0: clear USI global software reset (Active High) */ > >>> +#define USI_RESET (0<<0) > >> > >> Just 0x0. I understand you wanted to hint it is a bit field, but the > >> shift of 0 actually creates more questions. > >> > > > > After some consideration I decided to adhere to existing style and do > > something like this (in v2): > > > > 8<--------------------------------------------------------------------->8 > > #define USI_CON (0xC4) > > #define USI_OPTION (0xC8) > > > > #define USI_CON_RESET_CLEAR (0<<0) > > #define USI_CON_RESET_SET (1<<0) > > #define USI_CON_RESET_MASK (1<<0) > > > > #define USI_OPTION_HWACG_CLKREQ_ON (1<<1) > > #define USI_OPTION_HWACG_CLKSTOP_ON (1<<2) > > #define USI_OPTION_HWACG_MASK (3<<1) > > 8<--------------------------------------------------------------------->8 > > > > The whole reason for those comments was missing public TRM. But in the > > end I decided it just looks ugly. Also, this way I can do RMW > > operation (discussed above) in more logical way. > > > > Please let me know if code snippets above look good to you. > > Please skip the USI_CON_RESET_CLEAR. There is no such pattern in the > code. Clearing bit is an obvious operation and such code is already > everywhere: > val &= ~USI_CON_RESET > > (or &= ~USI_RESET_MASK) > > Therefore for USI_CON_RESET only: > #define USI_CON_RESET (1<<0) > #define USI_CON_RESET_MASK (1<<0) > Sure, I'll make it so. > > Best regards, > Krzysztof
Hi Marc, On Fri, 30 Jul 2021 at 19:50, Marc Zyngier <maz@kernel.org> wrote: > > On 2021-07-30 15:49, Sam Protsenko wrote: > > Samsung Exynos850 is ARMv8-based mobile-oriented SoC. > > > > Features: > > * CPU: Cortex-A55 Octa (8 cores), up to 2 GHz > > * Memory interface: LPDDR4/4x 2 channels (12.8 GB/s) > > * SD/MMC: SD 3.0, eMMC5.1 DDR 8-bit > > * Modem: 4G LTE, 3G, GSM/GPRS/EDGE > > * RF: Quad GNSS, WiFi 5 (802.11ac), Bluetooth 5.0 > > * GPU: Mali-G52 MP1 > > * Codec: 1080p 60fps H64, HEVC, JPEG HW Codec > > * Display: Full HD+ (2520x1080)@60fps LCD > > * Camera: 16+5MP/13+8MP ISP, MIPI CSI 4/4/2, FD, DRC > > * Connectivity: USB 2.0 DRD, USI (SPI/UART/I2C), HSI2C, I3C, ADC, > > Audio > > > > This patch adds minimal SoC support. Particular board device tree files > > can include exynos850.dtsi file to get SoC related nodes, and then > > reference those nodes further as needed. > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > .../boot/dts/exynos/exynos850-pinctrl.dtsi | 782 ++++++++++++++++++ > > arch/arm64/boot/dts/exynos/exynos850-usi.dtsi | 30 + > > arch/arm64/boot/dts/exynos/exynos850.dtsi | 245 ++++++ > > 3 files changed, 1057 insertions(+) > > create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi > > create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi > > create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi > > > > diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi > > b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi > > new file mode 100644 > > index 000000000000..4cf0a22cc6db > > [...] > > > + gic: interrupt-controller@12a00000 { > > + compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; > > One thing for sure, it cannot be both. And given that it is > an A55-based SoC, it isn't either. It is more likely a GIC400. > Yes, it's GIC-400, thanks for pointing that out. Will fix that in v2. > > + #interrupt-cells = <3>; > > + #address-cells = <0>; > > + interrupt-controller; > > + reg = <0x0 0x12a01000 0x1000>, > > + <0x0 0x12a02000 0x1000>, > > This is wrong. It is architecturally set to 8kB. > Nice catch! Actually there is an error (typo?) in SoC's TRM, saying that Virtual Interface Control Register starts at 0x3000 offset (from 0x12a00000), where it obviously should be 0x4000, that's probably where this dts error originates from. Btw, I'm also seeing the same error in exynos7.dtsi. Though I don't have a TRM for Exynos7 SoCs, so not sure if I should go ahead and fix that too. Anyway, for Exynos850, I'll fix that in v2 series. > > + <0x0 0x12a04000 0x2000>, > > + <0x0 0x12a06000 0x2000>; > > + interrupts = <GIC_PPI 9 > > + (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; > > 4? With 8 CPUs? > Will be fixed in v2, thanks. > I also find it curious that you went through the unusual > (and IMO confusing) effort to allocate a name to each and > every SPI in the system, but didn't do it for any on the PPIs... > Yeah, after some consideration I removed the whole interrupts header and used hard-coded values instead. I probably felt it would be right thing to have, just because there is no public TRM for Exynos850, thus documenting interrupts somewhere would be nice. But that reasoning is wrong, as trying to mix that kind of documentation with code just clutters it. The right thing to do is probably just provide a public TRM, but that's not for me to decide, alas :) Anyway, will be fixed in v2. > > > + }; > > + > > + timer { > > + compatible = "arm,armv8-timer"; > > + interrupts = <GIC_PPI 13 > > + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, > > + <GIC_PPI 14 > > + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, > > + <GIC_PPI 11 > > + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, > > + <GIC_PPI 10 > > + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>; > > + clock-frequency = <26000000>; > > No, please. Fix the firmware to program CNTFRQ_EL0 on each > and every CPU. This isn't 2012 anymore. > Ok, will remove that property in v2. Though it looks like CNTFRQ_EL0 register can be only changed in EL3 execution level, so I'll have to ask the vendor to fix their BL31 or whatever. But that might take some time, so I'll have to keep "clock-frequency" workaround in my local tree for now, to make scheduler work. > You are also missing the hypervisor virtual timer interrupt. > Checked SoC TRM, there is no PPI for hypervisor virtual timer interrupt, and no mentioning of it at all. Likewise, I checked ARMv8 ARM and TRM, almost no description of it. Also, I checked other platforms, and seems like everyone does the same (having only 4 interrupts). And I wasn't able to find any documentation on that, so I guess I'll leave it as is, if you don't mind. > > + use-clocksource-only; > > + use-physical-timer; > > Thankfully, these two properties do not exist. > Yeah, that's just some leftover from vendor's kernel, overlook by me... Will remove in v2, thanks. > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
On Wed, 04 Aug 2021 15:39:38 +0100, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > You are also missing the hypervisor virtual timer interrupt. > > > > Checked SoC TRM, there is no PPI for hypervisor virtual timer > interrupt, and no mentioning of it at all. Likewise, I checked ARMv8 > ARM and TRM, almost no description of it. Also, I checked other > platforms, and seems like everyone does the same (having only 4 > interrupts). And I wasn't able to find any documentation on that, so I > guess I'll leave it as is, if you don't mind. I *do* mind, and other DTs being wrong isn't a good enough excuse! ;-) From the ARMv8 ARM (ARM DDI 0487G.b) <quote> D11.2.4 Timers In an implementation of the Generic Timer that includes EL3, if EL3 can use AArch64, the following timers are implemented: * An EL1 physical timer, that: - In Secure state, can be accessed from EL1. - In Non-secure state, can be accessed from EL1 unless those accesses are trapped to EL2. When this timer can be accessed from EL1, an EL1 control determines whether it can be accessed from EL0. * A Non-secure EL2 physical timer. * A Secure EL3 physical timer. An EL3 control determines whether this register is accessible from Secure EL1. * An EL1 virtual timer. * When FEAT_VHE is implemented, a Non-secure EL2 virtual timer. * When FEAT_SEL2 is implemented, a Secure EL2 physical timer. * When FEAT_SEL2 is implemented, a Secure EL2 virtual timer. </quote> Cortex-A55 being an ARMv8.2 implementation, it has FEAT_VHE, and thus it does have a NS-EL2 virtual timer. This is further confirmed by the TRM which documents CNTHV*_EL2 as valid system registers[1]. So the timer exists, the signal is routed out of the core, and it is likely that it is connected to the GIC. If the designers have omitted it, then it needs to be documented as such. Thanks, M. [1] https://developer.arm.com/documentation/100442/0100/register-descriptions/aarch64-system-registers/aarch64-architectural-system-register-summary -- Without deviation from the norm, progress is not possible.
On 04/08/2021 16:39, Sam Protsenko wrote: > Hi Marc, > > On Fri, 30 Jul 2021 at 19:50, Marc Zyngier <maz@kernel.org> wrote: >> >> On 2021-07-30 15:49, Sam Protsenko wrote: >>> Samsung Exynos850 is ARMv8-based mobile-oriented SoC. >>> >>> Features: >>> * CPU: Cortex-A55 Octa (8 cores), up to 2 GHz >>> * Memory interface: LPDDR4/4x 2 channels (12.8 GB/s) >>> * SD/MMC: SD 3.0, eMMC5.1 DDR 8-bit >>> * Modem: 4G LTE, 3G, GSM/GPRS/EDGE >>> * RF: Quad GNSS, WiFi 5 (802.11ac), Bluetooth 5.0 >>> * GPU: Mali-G52 MP1 >>> * Codec: 1080p 60fps H64, HEVC, JPEG HW Codec >>> * Display: Full HD+ (2520x1080)@60fps LCD >>> * Camera: 16+5MP/13+8MP ISP, MIPI CSI 4/4/2, FD, DRC >>> * Connectivity: USB 2.0 DRD, USI (SPI/UART/I2C), HSI2C, I3C, ADC, >>> Audio >>> >>> This patch adds minimal SoC support. Particular board device tree files >>> can include exynos850.dtsi file to get SoC related nodes, and then >>> reference those nodes further as needed. >>> >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> >>> --- >>> .../boot/dts/exynos/exynos850-pinctrl.dtsi | 782 ++++++++++++++++++ >>> arch/arm64/boot/dts/exynos/exynos850-usi.dtsi | 30 + >>> arch/arm64/boot/dts/exynos/exynos850.dtsi | 245 ++++++ >>> 3 files changed, 1057 insertions(+) >>> create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi >>> create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi >>> create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi >>> >>> diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi >>> b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi >>> new file mode 100644 >>> index 000000000000..4cf0a22cc6db >> >> [...] >> >>> + gic: interrupt-controller@12a00000 { >>> + compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; >> >> One thing for sure, it cannot be both. And given that it is >> an A55-based SoC, it isn't either. It is more likely a GIC400. >> > > Yes, it's GIC-400, thanks for pointing that out. Will fix that in v2. > >>> + #interrupt-cells = <3>; >>> + #address-cells = <0>; >>> + interrupt-controller; >>> + reg = <0x0 0x12a01000 0x1000>, >>> + <0x0 0x12a02000 0x1000>, >> >> This is wrong. It is architecturally set to 8kB. >> > > Nice catch! Actually there is an error (typo?) in SoC's TRM, saying > that Virtual Interface Control Register starts at 0x3000 offset (from > 0x12a00000), where it obviously should be 0x4000, that's probably > where this dts error originates from. Btw, I'm also seeing the same > error in exynos7.dtsi. What's the error exactly? The "Virtual interface control register" offset (3rd region) is set properly to 0x4000 on Exynos7. Also one for the Exynos5433 looks correct. > Though I don't have a TRM for Exynos7 SoCs, so > not sure if I should go ahead and fix that too. Anyway, for Exynos850, > I'll fix that in v2 series. However while we are at addresses - why are you using address-cells 2? It adds everywhere additional 0x0 before actual address. Best regards, Krzysztof
On Wed, 4 Aug 2021 at 18:01, Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 04 Aug 2021 15:39:38 +0100, > Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > You are also missing the hypervisor virtual timer interrupt. > > > > > > > Checked SoC TRM, there is no PPI for hypervisor virtual timer > > interrupt, and no mentioning of it at all. Likewise, I checked ARMv8 > > ARM and TRM, almost no description of it. Also, I checked other > > platforms, and seems like everyone does the same (having only 4 > > interrupts). And I wasn't able to find any documentation on that, so I > > guess I'll leave it as is, if you don't mind. > > I *do* mind, and other DTs being wrong isn't a good enough excuse! ;-) > > From the ARMv8 ARM (ARM DDI 0487G.b) > <quote> > D11.2.4 Timers > > In an implementation of the Generic Timer that includes EL3, if EL3 > can use AArch64, the following timers are implemented: > > * An EL1 physical timer, that: > - In Secure state, can be accessed from EL1. > - In Non-secure state, can be accessed from EL1 unless those > accesses are trapped to EL2. > When this timer can be accessed from EL1, an EL1 control > determines whether it can be accessed from EL0. > * A Non-secure EL2 physical timer. > * A Secure EL3 physical timer. An EL3 control determines whether this > register is accessible from Secure EL1. > * An EL1 virtual timer. > * When FEAT_VHE is implemented, a Non-secure EL2 virtual timer. > * When FEAT_SEL2 is implemented, a Secure EL2 physical timer. > * When FEAT_SEL2 is implemented, a Secure EL2 virtual timer. > </quote> > > Cortex-A55 being an ARMv8.2 implementation, it has FEAT_VHE, and thus > it does have a NS-EL2 virtual timer. This is further confirmed by the > TRM which documents CNTHV*_EL2 as valid system registers[1]. > > So the timer exists, the signal is routed out of the core, and it > is likely that it is connected to the GIC. > > If the designers have omitted it, then it needs to be documented as > such. > Ok, I've checked thoroughly all docs again, and it seems like there is no dedicated PPI number for this "EL2 Hypervisor Virtual Timer" in Exynos850 SoC. The timer instance itself might exist of course, but interrupt line is probably wasn't connected to GIC by SoC designers, at least it's not documented. Moreover, from [1,2] it looks like if it were existing it would have been PPI=12 (INTID=28). But in GIC-400 TRM this PPI is assigned to "Legacy FIQ signal", and all there is no PPI for Hypervisor Virtual Timer documented there as well. In Exynos850 TRM the source for this PPI's interrupt source is marked as "-", which means it's not used. So if you know something that I don't know -- please point me out the doc where this PPI line is documented. Otherwise I can add the comment to device tree, stating that this interrupt line is not present in SoC's GIC, i.e. something like this: 8<------------------------------------------------------------------------------->8 timer { compatible = "arm,armv8-timer"; interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>, <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>; /* Hypervisor Virtual Timer PPI is not present in this SoC GIC */ }; 8<------------------------------------------------------------------------------->8 Is that ok with you? [1] https://developer.arm.com/documentation/102379/0000/The-processor-timers?lang=en [2] https://gem5.googlesource.com/public/gem5/+/refs/heads/master/src/arch/arm/fastmodel/CortexA76/FastModelCortexA76.py#150 > Thanks, > > M. > > [1] https://developer.arm.com/documentation/100442/0100/register-descriptions/aarch64-system-registers/aarch64-architectural-system-register-summary > > -- > Without deviation from the norm, progress is not possible.
On Wed, 4 Aug 2021 at 21:36, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 04/08/2021 16:39, Sam Protsenko wrote: > > Hi Marc, > > > > On Fri, 30 Jul 2021 at 19:50, Marc Zyngier <maz@kernel.org> wrote: > >> > >> On 2021-07-30 15:49, Sam Protsenko wrote: > >>> Samsung Exynos850 is ARMv8-based mobile-oriented SoC. > >>> > >>> Features: > >>> * CPU: Cortex-A55 Octa (8 cores), up to 2 GHz > >>> * Memory interface: LPDDR4/4x 2 channels (12.8 GB/s) > >>> * SD/MMC: SD 3.0, eMMC5.1 DDR 8-bit > >>> * Modem: 4G LTE, 3G, GSM/GPRS/EDGE > >>> * RF: Quad GNSS, WiFi 5 (802.11ac), Bluetooth 5.0 > >>> * GPU: Mali-G52 MP1 > >>> * Codec: 1080p 60fps H64, HEVC, JPEG HW Codec > >>> * Display: Full HD+ (2520x1080)@60fps LCD > >>> * Camera: 16+5MP/13+8MP ISP, MIPI CSI 4/4/2, FD, DRC > >>> * Connectivity: USB 2.0 DRD, USI (SPI/UART/I2C), HSI2C, I3C, ADC, > >>> Audio > >>> > >>> This patch adds minimal SoC support. Particular board device tree files > >>> can include exynos850.dtsi file to get SoC related nodes, and then > >>> reference those nodes further as needed. > >>> > >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > >>> --- > >>> .../boot/dts/exynos/exynos850-pinctrl.dtsi | 782 ++++++++++++++++++ > >>> arch/arm64/boot/dts/exynos/exynos850-usi.dtsi | 30 + > >>> arch/arm64/boot/dts/exynos/exynos850.dtsi | 245 ++++++ > >>> 3 files changed, 1057 insertions(+) > >>> create mode 100644 arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi > >>> create mode 100644 arch/arm64/boot/dts/exynos/exynos850-usi.dtsi > >>> create mode 100644 arch/arm64/boot/dts/exynos/exynos850.dtsi > >>> > >>> diff --git a/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi > >>> b/arch/arm64/boot/dts/exynos/exynos850-pinctrl.dtsi > >>> new file mode 100644 > >>> index 000000000000..4cf0a22cc6db > >> > >> [...] > >> > >>> + gic: interrupt-controller@12a00000 { > >>> + compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; > >> > >> One thing for sure, it cannot be both. And given that it is > >> an A55-based SoC, it isn't either. It is more likely a GIC400. > >> > > > > Yes, it's GIC-400, thanks for pointing that out. Will fix that in v2. > > > >>> + #interrupt-cells = <3>; > >>> + #address-cells = <0>; > >>> + interrupt-controller; > >>> + reg = <0x0 0x12a01000 0x1000>, > >>> + <0x0 0x12a02000 0x1000>, > >> > >> This is wrong. It is architecturally set to 8kB. > >> > > > > Nice catch! Actually there is an error (typo?) in SoC's TRM, saying > > that Virtual Interface Control Register starts at 0x3000 offset (from > > 0x12a00000), where it obviously should be 0x4000, that's probably > > where this dts error originates from. Btw, I'm also seeing the same > > error in exynos7.dtsi. > > What's the error exactly? The "Virtual interface control register" > offset (3rd region) is set properly to 0x4000 on Exynos7. Also one for > the Exynos5433 looks correct. > The issue is that 2nd region's size is 0x1000, but it must be 0x2000. It's defined by GIC-400 architecture, as I understand. Please look at [1], table 3-1 has very specific offsets and sizes for each functional block, and each particular SoC must adhere to that spec. So having 0x1000 for 2nd region can't be correct. And because exynos7.dtsi has GIC-400 as well, and 0x1000 is specified there for 2nd region size too, so I presume there is the same mistake there. Can you please check the TRM for Exynos7 SoC (if you have one in your possession), and see if there is a typo there? E.g. in case of Exynos850 TRM I can see that in "Register Map Summary" section the offset for the first register (GICH_HCR) in "Virtual Interface Control Register" region is specified as 0x3000, where it should be 0x4000, so it's probably a typo. But the register description is correct, saying that: "Address = Base Address + 0x4000". [1] https://developer.arm.com/documentation/ddi0471/b/programmers-model/gic-400-register-map > > Though I don't have a TRM for Exynos7 SoCs, so > > not sure if I should go ahead and fix that too. Anyway, for Exynos850, > > I'll fix that in v2 series. > > > However while we are at addresses - why are you using address-cells 2? > It adds everywhere additional 0x0 before actual address. > Right. For "cpus" node I'll change the address-cells to 1 in my v2 series. I'll keep address-cells=2 for the root node, but I'm going to encapsulate some nodes into soc node (as you suggested earlier), where I'll make address-cells=1. That's pretty much how it's done in exynos7.dtsi and in exynos5433.dtsi, so I guess that's should be fine (to get rid of superfluous 0x0 and conform with other Exynos DTs)? > > Best regards, > Krzysztof
On 04/08/2021 23:30, Sam Protsenko wrote: >>> >>> Nice catch! Actually there is an error (typo?) in SoC's TRM, saying >>> that Virtual Interface Control Register starts at 0x3000 offset (from >>> 0x12a00000), where it obviously should be 0x4000, that's probably >>> where this dts error originates from. Btw, I'm also seeing the same >>> error in exynos7.dtsi. >> >> What's the error exactly? The "Virtual interface control register" >> offset (3rd region) is set properly to 0x4000 on Exynos7. Also one for >> the Exynos5433 looks correct. >> > > The issue is that 2nd region's size is 0x1000, but it must be 0x2000. > It's defined by GIC-400 architecture, as I understand. Please look at > [1], table 3-1 has very specific offsets and sizes for each functional > block, and each particular SoC must adhere to that spec. So having > 0x1000 for 2nd region can't be correct. And because exynos7.dtsi has > GIC-400 as well, and 0x1000 is specified there for 2nd region size > too, so I presume there is the same mistake there. I understand, the range length has indeed same mistake. However it does not matter that much There are no registers pass 0x10C (so pass 0x1000). This address space is not used. > Can you please check the TRM for Exynos7 SoC (if you have one in your > possession), and see if there is a typo there? E.g. in case of > Exynos850 TRM I can see that in "Register Map Summary" section the > offset for the first register (GICH_HCR) in "Virtual Interface Control > Register" region is specified as 0x3000, where it should be 0x4000, so > it's probably a typo. But the register description is correct, saying > that: "Address = Base Address + 0x4000". The starting addresses of each registers range is different issue and this one matters. Except same typo as you say, all looks good - they start at 0x4000. > > [1] https://developer.arm.com/documentation/ddi0471/b/programmers-model/gic-400-register-map > >>> Though I don't have a TRM for Exynos7 SoCs, so >>> not sure if I should go ahead and fix that too. Anyway, for Exynos850, >>> I'll fix that in v2 series. >> >> >> However while we are at addresses - why are you using address-cells 2? >> It adds everywhere additional 0x0 before actual address. >> > > Right. For "cpus" node I'll change the address-cells to 1 in my v2 > series. I'll keep address-cells=2 for the root node, but I'm going to > encapsulate some nodes into soc node (as you suggested earlier), where > I'll make address-cells=1. That's pretty much how it's done in > exynos7.dtsi and in exynos5433.dtsi, so I guess that's should be fine > (to get rid of superfluous 0x0 and conform with other Exynos DTs)? Yes, thanks. Best regards, Krzysztof
On Thu, 05 Aug 2021 08:17:14 +0100, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 04/08/2021 23:30, Sam Protsenko wrote: > >>> > >>> Nice catch! Actually there is an error (typo?) in SoC's TRM, saying > >>> that Virtual Interface Control Register starts at 0x3000 offset (from > >>> 0x12a00000), where it obviously should be 0x4000, that's probably > >>> where this dts error originates from. Btw, I'm also seeing the same > >>> error in exynos7.dtsi. > >> > >> What's the error exactly? The "Virtual interface control register" > >> offset (3rd region) is set properly to 0x4000 on Exynos7. Also one for > >> the Exynos5433 looks correct. > >> > > > > The issue is that 2nd region's size is 0x1000, but it must be 0x2000. > > It's defined by GIC-400 architecture, as I understand. Please look at > > [1], table 3-1 has very specific offsets and sizes for each functional > > block, and each particular SoC must adhere to that spec. So having > > 0x1000 for 2nd region can't be correct. And because exynos7.dtsi has > > GIC-400 as well, and 0x1000 is specified there for 2nd region size > > too, so I presume there is the same mistake there. > > I understand, the range length has indeed same mistake. However it does > not matter that much There are no registers pass 0x10C (so pass 0x1000). > This address space is not used. I have no idea which spec you are looking at, but the GICv2 architecture (of which GIC400 is an implementation) definitely has a register in the second 4kB page of the CPU interface. It contains the GICC_DIR register, which is used to deactivate an interrupt when EOIMode==1. Linux actively uses it when started at EL2. M. -- Without deviation from the norm, progress is not possible.
On 05/08/2021 09:30, Marc Zyngier wrote: > On Thu, 05 Aug 2021 08:17:14 +0100, > Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: >> >> On 04/08/2021 23:30, Sam Protsenko wrote: >>>>> >>>>> Nice catch! Actually there is an error (typo?) in SoC's TRM, saying >>>>> that Virtual Interface Control Register starts at 0x3000 offset (from >>>>> 0x12a00000), where it obviously should be 0x4000, that's probably >>>>> where this dts error originates from. Btw, I'm also seeing the same >>>>> error in exynos7.dtsi. >>>> >>>> What's the error exactly? The "Virtual interface control register" >>>> offset (3rd region) is set properly to 0x4000 on Exynos7. Also one for >>>> the Exynos5433 looks correct. >>>> >>> >>> The issue is that 2nd region's size is 0x1000, but it must be 0x2000. >>> It's defined by GIC-400 architecture, as I understand. Please look at >>> [1], table 3-1 has very specific offsets and sizes for each functional >>> block, and each particular SoC must adhere to that spec. So having >>> 0x1000 for 2nd region can't be correct. And because exynos7.dtsi has >>> GIC-400 as well, and 0x1000 is specified there for 2nd region size >>> too, so I presume there is the same mistake there. >> >> I understand, the range length has indeed same mistake. However it does >> not matter that much There are no registers pass 0x10C (so pass 0x1000). >> This address space is not used. > > I have no idea which spec you are looking at, but the GICv2 > architecture (of which GIC400 is an implementation) definitely has a > register in the second 4kB page of the CPU interface. It contains the > GICC_DIR register, which is used to deactivate an interrupt when > EOIMode==1. > > Linux actively uses it when started at EL2. I was checking Exynos TRM and it seems it has one more bug... The ARM datasheet [1] indeed mentions GICC_DIR at 0x1000. I'll add "Fixes" tag to my fix for Exynos7. https://developer.arm.com/documentation/ddi0471/b/programmers-model/cpu-interface-register-summary Best regards, Krzysztof
On Wed, 04 Aug 2021 19:37:24 +0100, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Wed, 4 Aug 2021 at 18:01, Marc Zyngier <maz@kernel.org> wrote: > > > > On Wed, 04 Aug 2021 15:39:38 +0100, > > Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > You are also missing the hypervisor virtual timer interrupt. > > > > > > > > > > Checked SoC TRM, there is no PPI for hypervisor virtual timer > > > interrupt, and no mentioning of it at all. Likewise, I checked ARMv8 > > > ARM and TRM, almost no description of it. Also, I checked other > > > platforms, and seems like everyone does the same (having only 4 > > > interrupts). And I wasn't able to find any documentation on that, so I > > > guess I'll leave it as is, if you don't mind. > > > > I *do* mind, and other DTs being wrong isn't a good enough excuse! ;-) > > > > From the ARMv8 ARM (ARM DDI 0487G.b) > > <quote> > > D11.2.4 Timers > > > > In an implementation of the Generic Timer that includes EL3, if EL3 > > can use AArch64, the following timers are implemented: > > > > * An EL1 physical timer, that: > > - In Secure state, can be accessed from EL1. > > - In Non-secure state, can be accessed from EL1 unless those > > accesses are trapped to EL2. > > When this timer can be accessed from EL1, an EL1 control > > determines whether it can be accessed from EL0. > > * A Non-secure EL2 physical timer. > > * A Secure EL3 physical timer. An EL3 control determines whether this > > register is accessible from Secure EL1. > > * An EL1 virtual timer. > > * When FEAT_VHE is implemented, a Non-secure EL2 virtual timer. > > * When FEAT_SEL2 is implemented, a Secure EL2 physical timer. > > * When FEAT_SEL2 is implemented, a Secure EL2 virtual timer. > > </quote> > > > > Cortex-A55 being an ARMv8.2 implementation, it has FEAT_VHE, and thus > > it does have a NS-EL2 virtual timer. This is further confirmed by the > > TRM which documents CNTHV*_EL2 as valid system registers[1]. > > > > So the timer exists, the signal is routed out of the core, and it > > is likely that it is connected to the GIC. > > > > If the designers have omitted it, then it needs to be documented as > > such. > > > > Ok, I've checked thoroughly all docs again, and it seems like there is > no dedicated PPI number for this "EL2 Hypervisor Virtual Timer" in > Exynos850 SoC. The timer instance itself might exist of course, but > interrupt line is probably wasn't connected to GIC by SoC designers, > at least it's not documented. Can you try and check this? You can directly program the virtual timer so that it has a pending interrupt, and then check the pending register on the same CPU to see if there is anything appearing there. > Moreover, from [1,2] it looks like if it were existing it would have > been PPI=12 (INTID=28). But in GIC-400 TRM this PPI is assigned to > "Legacy FIQ signal", No. That's only if you set the bypass bits in GICD_CTLR, which nobody with half a brain would consider doing. > and all there is no PPI for Hypervisor Virtual > Timer documented there as well. In Exynos850 TRM the source for this > PPI's interrupt source is marked as "-", which means it's not used. > > So if you know something that I don't know -- please point me out the > doc where this PPI line is documented. Otherwise I can add the comment > to device tree, stating that this interrupt line is not present in > SoC's GIC, i.e. something like this: > > 8<------------------------------------------------------------------------------->8 > timer { > compatible = "arm,armv8-timer"; > interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | > IRQ_TYPE_LEVEL_LOW)>, > <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | > IRQ_TYPE_LEVEL_LOW)>, > <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) | > IRQ_TYPE_LEVEL_LOW)>, > <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | > IRQ_TYPE_LEVEL_LOW)>; > /* Hypervisor Virtual Timer PPI is not present in this SoC GIC */ > }; > 8<------------------------------------------------------------------------------->8 > > Is that ok with you? I'd rather you verify the above first. And if you can't, I'd like a comment that is a bit more explicit: /* The vendor couldn't be bothered to wire the EL2 Virtual Timers */ Thanks, M. -- Without deviation from the norm, progress is not possible.
On Thu, 5 Aug 2021 at 10:39, Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 04 Aug 2021 19:37:24 +0100, > Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > On Wed, 4 Aug 2021 at 18:01, Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Wed, 04 Aug 2021 15:39:38 +0100, > > > Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > > > You are also missing the hypervisor virtual timer interrupt. > > > > > > > > > > > > > Checked SoC TRM, there is no PPI for hypervisor virtual timer > > > > interrupt, and no mentioning of it at all. Likewise, I checked ARMv8 > > > > ARM and TRM, almost no description of it. Also, I checked other > > > > platforms, and seems like everyone does the same (having only 4 > > > > interrupts). And I wasn't able to find any documentation on that, so I > > > > guess I'll leave it as is, if you don't mind. > > > > > > I *do* mind, and other DTs being wrong isn't a good enough excuse! ;-) > > > > > > From the ARMv8 ARM (ARM DDI 0487G.b) > > > <quote> > > > D11.2.4 Timers > > > > > > In an implementation of the Generic Timer that includes EL3, if EL3 > > > can use AArch64, the following timers are implemented: > > > > > > * An EL1 physical timer, that: > > > - In Secure state, can be accessed from EL1. > > > - In Non-secure state, can be accessed from EL1 unless those > > > accesses are trapped to EL2. > > > When this timer can be accessed from EL1, an EL1 control > > > determines whether it can be accessed from EL0. > > > * A Non-secure EL2 physical timer. > > > * A Secure EL3 physical timer. An EL3 control determines whether this > > > register is accessible from Secure EL1. > > > * An EL1 virtual timer. > > > * When FEAT_VHE is implemented, a Non-secure EL2 virtual timer. > > > * When FEAT_SEL2 is implemented, a Secure EL2 physical timer. > > > * When FEAT_SEL2 is implemented, a Secure EL2 virtual timer. > > > </quote> > > > > > > Cortex-A55 being an ARMv8.2 implementation, it has FEAT_VHE, and thus > > > it does have a NS-EL2 virtual timer. This is further confirmed by the > > > TRM which documents CNTHV*_EL2 as valid system registers[1]. > > > > > > So the timer exists, the signal is routed out of the core, and it > > > is likely that it is connected to the GIC. > > > > > > If the designers have omitted it, then it needs to be documented as > > > such. > > > > > > > Ok, I've checked thoroughly all docs again, and it seems like there is > > no dedicated PPI number for this "EL2 Hypervisor Virtual Timer" in > > Exynos850 SoC. The timer instance itself might exist of course, but > > interrupt line is probably wasn't connected to GIC by SoC designers, > > at least it's not documented. > > Can you try and check this? You can directly program the virtual timer > so that it has a pending interrupt, and then check the pending > register on the same CPU to see if there is anything appearing there. > > > Moreover, from [1,2] it looks like if it were existing it would have > > been PPI=12 (INTID=28). But in GIC-400 TRM this PPI is assigned to > > "Legacy FIQ signal", > > No. That's only if you set the bypass bits in GICD_CTLR, which nobody > with half a brain would consider doing. > > > and all there is no PPI for Hypervisor Virtual > > Timer documented there as well. In Exynos850 TRM the source for this > > PPI's interrupt source is marked as "-", which means it's not used. > > > > So if you know something that I don't know -- please point me out the > > doc where this PPI line is documented. Otherwise I can add the comment > > to device tree, stating that this interrupt line is not present in > > SoC's GIC, i.e. something like this: > > > > 8<------------------------------------------------------------------------------->8 > > timer { > > compatible = "arm,armv8-timer"; > > interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | > > IRQ_TYPE_LEVEL_LOW)>, > > <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | > > IRQ_TYPE_LEVEL_LOW)>, > > <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) | > > IRQ_TYPE_LEVEL_LOW)>, > > <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | > > IRQ_TYPE_LEVEL_LOW)>; > > /* Hypervisor Virtual Timer PPI is not present in this SoC GIC */ > > }; > > 8<------------------------------------------------------------------------------->8 > > > > Is that ok with you? > > I'd rather you verify the above first. And if you can't, I'd like a > comment that is a bit more explicit: > I'm afraid I won't be able to verify your idea: seems like CNTHV_EL2 can be only modified (or read) in EL2. I tried to read that reg anyway, which unsurprisingly resulted in el1_undef() BUG. The kernel on my board is running in EL1, and I don't have access to the source code for EL3 bootloaders. I have the source code for the last bootloader, but it's already running in EL1. > /* The vendor couldn't be bothered to wire the EL2 Virtual Timers */ > I'll add the comment as you suggested. I propose we come back to this issue later, either when the need for HV timer arises or when I have some means to test your theory about existing PPI. Thanks! > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
On Thu, 05 Aug 2021 16:30:23 +0100, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Thu, 5 Aug 2021 at 10:39, Marc Zyngier <maz@kernel.org> wrote: > > > > On Wed, 04 Aug 2021 19:37:24 +0100, > > Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > On Wed, 4 Aug 2021 at 18:01, Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Wed, 04 Aug 2021 15:39:38 +0100, > > > > Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > > > > > You are also missing the hypervisor virtual timer interrupt. > > > > > > > > > > > > > > > > Checked SoC TRM, there is no PPI for hypervisor virtual timer > > > > > interrupt, and no mentioning of it at all. Likewise, I checked ARMv8 > > > > > ARM and TRM, almost no description of it. Also, I checked other > > > > > platforms, and seems like everyone does the same (having only 4 > > > > > interrupts). And I wasn't able to find any documentation on that, so I > > > > > guess I'll leave it as is, if you don't mind. > > > > > > > > I *do* mind, and other DTs being wrong isn't a good enough excuse! ;-) > > > > > > > > From the ARMv8 ARM (ARM DDI 0487G.b) > > > > <quote> > > > > D11.2.4 Timers > > > > > > > > In an implementation of the Generic Timer that includes EL3, if EL3 > > > > can use AArch64, the following timers are implemented: > > > > > > > > * An EL1 physical timer, that: > > > > - In Secure state, can be accessed from EL1. > > > > - In Non-secure state, can be accessed from EL1 unless those > > > > accesses are trapped to EL2. > > > > When this timer can be accessed from EL1, an EL1 control > > > > determines whether it can be accessed from EL0. > > > > * A Non-secure EL2 physical timer. > > > > * A Secure EL3 physical timer. An EL3 control determines whether this > > > > register is accessible from Secure EL1. > > > > * An EL1 virtual timer. > > > > * When FEAT_VHE is implemented, a Non-secure EL2 virtual timer. > > > > * When FEAT_SEL2 is implemented, a Secure EL2 physical timer. > > > > * When FEAT_SEL2 is implemented, a Secure EL2 virtual timer. > > > > </quote> > > > > > > > > Cortex-A55 being an ARMv8.2 implementation, it has FEAT_VHE, and thus > > > > it does have a NS-EL2 virtual timer. This is further confirmed by the > > > > TRM which documents CNTHV*_EL2 as valid system registers[1]. > > > > > > > > So the timer exists, the signal is routed out of the core, and it > > > > is likely that it is connected to the GIC. > > > > > > > > If the designers have omitted it, then it needs to be documented as > > > > such. > > > > > > > > > > Ok, I've checked thoroughly all docs again, and it seems like there is > > > no dedicated PPI number for this "EL2 Hypervisor Virtual Timer" in > > > Exynos850 SoC. The timer instance itself might exist of course, but > > > interrupt line is probably wasn't connected to GIC by SoC designers, > > > at least it's not documented. > > > > Can you try and check this? You can directly program the virtual timer > > so that it has a pending interrupt, and then check the pending > > register on the same CPU to see if there is anything appearing there. > > > > > Moreover, from [1,2] it looks like if it were existing it would have > > > been PPI=12 (INTID=28). But in GIC-400 TRM this PPI is assigned to > > > "Legacy FIQ signal", > > > > No. That's only if you set the bypass bits in GICD_CTLR, which nobody > > with half a brain would consider doing. > > > > > and all there is no PPI for Hypervisor Virtual > > > Timer documented there as well. In Exynos850 TRM the source for this > > > PPI's interrupt source is marked as "-", which means it's not used. > > > > > > So if you know something that I don't know -- please point me out the > > > doc where this PPI line is documented. Otherwise I can add the comment > > > to device tree, stating that this interrupt line is not present in > > > SoC's GIC, i.e. something like this: > > > > > > 8<------------------------------------------------------------------------------->8 > > > timer { > > > compatible = "arm,armv8-timer"; > > > interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | > > > IRQ_TYPE_LEVEL_LOW)>, > > > <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | > > > IRQ_TYPE_LEVEL_LOW)>, > > > <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) | > > > IRQ_TYPE_LEVEL_LOW)>, > > > <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | > > > IRQ_TYPE_LEVEL_LOW)>; > > > /* Hypervisor Virtual Timer PPI is not present in this SoC GIC */ > > > }; > > > 8<------------------------------------------------------------------------------->8 > > > > > > Is that ok with you? > > > > I'd rather you verify the above first. And if you can't, I'd like a > > comment that is a bit more explicit: > > > > I'm afraid I won't be able to verify your idea: seems like CNTHV_EL2 > can be only modified (or read) in EL2. I tried to read that reg > anyway, which unsurprisingly resulted in el1_undef() BUG. The kernel > on my board is running in EL1, and I don't have access to the source > code for EL3 bootloaders. I have the source code for the last > bootloader, but it's already running in EL1. Excellent. Yet another half-usable machine on the block. Just what we need. > > > /* The vendor couldn't be bothered to wire the EL2 Virtual Timers */ > > > > I'll add the comment as you suggested. I propose we come back to this > issue later, either when the need for HV timer arises or when I have > some means to test your theory about existing PPI. If you depend on the vendor to get EL2 access, it is a lost cause, unfortunately. M.