* Re: [PATCH v3] PCI: aardvark: Don't rely on jiffies while holding spinlock
From: Thomas Petazzoni @ 2019-09-27 9:45 UTC (permalink / raw)
To: Remi Pommarel
Cc: Bjorn Helgaas, linux-pci, Lorenzo Pieralisi, linux-kernel,
linux-arm-kernel
In-Reply-To: <20190927085502.1758-1-repk@triplefau.lt>
On Fri, 27 Sep 2019 10:55:02 +0200
Remi Pommarel <repk@triplefau.lt> wrote:
> advk_pcie_wait_pio() can be called while holding a spinlock (from
> pci_bus_read_config_dword()), then depends on jiffies in order to
> timeout while polling on PIO state registers. In the case the PIO
> transaction failed, the timeout will never happen and will also cause
> the cpu to stall.
>
> This decrements a variable and wait instead of using jiffies.
>
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Thanks!
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/3] ARM: dts: add Netronix E60K02 board common file
From: Marco Felsch @ 2019-09-27 9:47 UTC (permalink / raw)
To: Andreas Kemnade
Cc: mark.rutland, marex, devicetree, andrew.smirnov, shawnguo,
s.hauer, angus, linux-kernel, j.neuschaefer, robh+dt, linux-imx,
kernel, manivannan.sadhasivam, Discussions about the Letux Kernel,
festevam, linux-arm-kernel
In-Reply-To: <20190927061423.17278-2-andreas@kemnade.info>
Hi Andreas,
thanks for the patch.
On 19-09-27 08:14, Andreas Kemnade wrote:
> The Netronix board E60K02 can be found some several Ebook-Readers,
> at least the Kobo Clara HD and the Tolino Shine 3. The board
> is equipped with different SoCs.
>
> For now the following peripherals are included:
> - LED
> - Power Key
> - Cover (gpio via hall sensor)
> - RC5T619 PMIC (the kernel misses support for rtc and charger
> subdevices).
> - Backlight via lm3630a
> - Wifi sdio chip detection (mmc-powerseq and stuff)
>
> It is based on vendor kernel but heavily reworked due to many
> changed bindings.
>
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> backligt dependencies:
> module autoloading:
> https://patchwork.kernel.org/patch/11139987/
> enable-gpios property:
> https://patchwork.kernel.org/patch/11143795/
>
> arch/arm/boot/dts/e60k02.dtsi | 339 ++++++++++++++++++++++++++++++++++
> 1 file changed, 339 insertions(+)
> create mode 100644 arch/arm/boot/dts/e60k02.dtsi
>
> diff --git a/arch/arm/boot/dts/e60k02.dtsi b/arch/arm/boot/dts/e60k02.dtsi
> new file mode 100644
> index 000000000000..c4fa8e314e2e
> --- /dev/null
> +++ b/arch/arm/boot/dts/e60k02.dtsi
> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 Andreas Kemnade
> + * based on works
> + * Copyright 2016 Freescale Semiconductor, Inc.
> + * and
> + * Copyright (C) 2014 Ricoh Electronic Devices Co., Ltd
> + *
> + * Netronix E60K02 board common.
> + * This board is equipped with different SoCs and
> + * found in ebook-readers like the Kobo Clara HD (with i.MX6SLL) and
> + * the Tolino Shine 3 (with i.MX6SL)
> + */
> +
> +/ {
> +
> + memory {
> + reg = <0x80000000 0x80000000>;
> + };
> +
> + chosen {
> + stdout-path = &uart1;
> + };
> +
> + wifi_pwrseq: wifi_pwrseq {
> + compatible = "mmc-pwrseq-simple";
> + post-power-on-delay-ms = <20>;
> + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
Can you add a pinctrl-entry here please? The general rule is to mux
things where you use it.
> + };
Please reorder the whole dt alphabetical.
> +
> + regulators {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <0>;
Drop the regultors { } container especially if we only have a single
regulator.
> +
> + reg_sd3_vmmc: wifi_regulator {
Either use:
reg_sd3_vmmc: regulator-sd3-vmmc
or
reg_wifi: regulator-wifi
> + compatible = "regulator-fixed";
> + regulator-name = "SD3_SPWR";
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3000000>;
> +
> + gpio = <&gpio4 29 GPIO_ACTIVE_HIGH>;
Please add a pinctrl here to mux this gpio.
> + enable-active-high;
> +
> + };
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_led>;
Please move all muxing you made here into this file or add phandles so
the dts file need to add only the muxing stuff. This applies to all
pinctrl you made here.
> +
> + GLED {
> + gpios = <&gpio5 7 GPIO_ACTIVE_LOW>;
> + linux,default-trigger = "timer";
> + };
> + };
> +
> + gpio-keys {
> + compatible = "gpio-keys";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_gpio_keys>;
> + power {
> + label = "Power";
> + gpios = <&gpio5 8 GPIO_ACTIVE_LOW>;
> + linux,code = <KEY_POWER>;
Add missing header: dt-bindings/input/input.h to use this.
> + gpio-key,wakeup;
> + };
> + cover {
> + label = "Cover";
> + gpios = <&gpio5 12 GPIO_ACTIVE_LOW>;
> + linux,code = <SW_LID>;
> + linux,input-type = <0x05>; /* EV_SW */
In the header above EV_SW is also specified so please use it here.
> + gpio-key,wakeup;
> + };
> + };
> +
> +};
> +
> +
> +
Whitespaces
> +&audmux {
> + pinctrl-names = "default";
> + status = "disabled";
Why you mentioned a pinctrl-names here without the mux? Do we need the
status line here? The common case is that such devices are off by
default/the base dt.
> +};
> +
> +&snvs_rtc {
> + status = "disabled";
Same applies here.
> +};
> +
> +&i2c1 {
> + clock-frequency = <100000>;
> + pinctrl-names = "default","sleep";
> + pinctrl-0 = <&pinctrl_i2c1 &pinctrl_lm3630a_bl_gpio>;
The &pinctrl_lm3630a_bl_gpio should be moved into the lm3630a node.
> + pinctrl-1 = <&pinctrl_i2c1_sleep>;
> + status = "okay";
> +
> + lm3630a: lm3630a-i2c@36 {
please name it backlight@36
> + reg = <0x36>;
> + status = "ok";
status lines are always be the last and if it is okay you can drop it
because the default is okay.
> +
> + compatible = "ti,lm3630a";
> + enable-gpios = <&gpio2 10 0>;
Please use GPIO_ACTIVE_HIGH.
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led@0 {
> + reg = <0>;
> + led-sources = <0>;
> + label = "backlight_warm";
> + default-brightness = <0>;
> + max-brightness = <255>;
> + };
> +
> + led@1 {
> + reg = <1>;
> + led-sources = <1>;
> + label = "backlight_cold";
> + default-brightness = <0>;
> + max-brightness = <255>;
> + };
> +
> + };
> +};
> +
> +&i2c3 {
> + clock-frequency = <100000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_i2c3 &pinctrl_ricoh_gpio>;
Device mux goes into the device.
> + status = "okay";
> +
> + ricoh619: ricoh619-i2c@32 {
Please name it pmic@32
> + compatible = "ricoh,rc5t619";
> + reg = <0x32>;
> +
> + system-power-controller;
> + gpios = <&gpio5 11 GPIO_ACTIVE_LOW>;
> + gpio_chg = <&gpio5 15 GPIO_ACTIVE_LOW>;
> + gpio_bat_low = <&gpio3 28 GPIO_ACTIVE_LOW>;
> + };
> +
> +};
> +
> +&ricoh619
> +{
Nope. The whole bunch should be moved to the place above where add the
device.
> + compatible = "ricoh,rc5t619";
> +
> + regulators {
> + ricoh619_dcdc1_reg: DCDC1 {
please drop the ricoh619_ prefix.
> + regulator-name = "DCDC1";
> + regulator-min-microvolt = <300000>;
> + regulator-max-microvolt = <1875000>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <900000>;
regulator-suspend-microvolt is deprecated. Please use
regulator-suspend-min-microvolt and regulator-suspend-max-microvolt.
> + };
> + };
> +
> + /* Core3_3V3 */
> + ricoh619_dcdc2_reg: DCDC2 {
> + regulator-name = "DCDC2";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <3300000>;
> + };
> + };
> +
> + ricoh619_dcdc3_reg: DCDC3 {
> + regulator-name = "DCDC3";
> + regulator-min-microvolt = <300000>;
> + regulator-max-microvolt = <1875000>;
> + regulator-always-on;//
Remove //
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <1140000>;
> + };
> + };
> +
> + /* Core4_1V2 */
> + ricoh619_dcdc4_reg: DCDC4 {
> + regulator-name = "DCDC4";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <1140000>;
> + };
> + };
> +
> + /* Core4_1V8 */
> + ricoh619_dcdc5_reg: DCDC5 {
> + regulator-name = "DCDC5";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <1700000>;
> + };
> + };
> +
> + /* IR_3V3 */
> + ricoh619_ldo1_reg: LDO1 {
> + regulator-name = "LDO1";
> + //regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + /* Core1_3V3 */
> + ricoh619_ldo2_reg: LDO2 {
> + regulator-name = "LDO2";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <3000000>;
> + };
> + };
> +
> + /* Core5_1V2 */
> + ricoh619_ldo3_reg: LDO3 {
> + regulator-name = "LDO3";
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + ricoh619_ldo4_reg: LDO4 {
> + regulator-name = "LDO4";
> + regulator-boot-on;
> + };
> +
> + /* SPD_3V3 */
> + ricoh619_ldo5_reg: LDO5 {
> + regulator-name = "LDO5";
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + /* DDR_0V6 */
> + ricoh619_ldo6_reg: LDO6 {
> + regulator-name = "LDO6";
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + /* VDD_PWM */
> + ricoh619_ldo7_reg: LDO7 {
> + regulator-name = "LDO7";
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + /* ldo_1v8 */
> + ricoh619_ldo8_reg: LDO8 {
> + regulator-name = "LDO8";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + regulator-boot-on;
> + };
> +
> + ricoh619_ldo9_reg: LDO9 {
> + regulator-name = "LDO9";
> + regulator-boot-on;
> + };
> +
> + ricoh619_ldo10_reg: LDO10 {
> + regulator-name = "LDO10";
> + regulator-boot-on;
> + };
> +
> + ricoh619_ldortc1_reg: LDORTC1 {
> + regulator-name = "LDORTC1";
> + regulator-boot-on;
> + };
> +
> + ricoh619_ldortc2_reg: LDORTC2 {
> + regulator-name = "LDORTC2";
> + regulator-boot-on;
> + };
> + };
> +};
> +
> +&uart1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_uart1>;
> + status = "okay";
> +};
> +
> +&usdhc1 {
> + status = "disabled";
> +};
Can be dropped
> +
> +&usdhc2 {
> + pinctrl-names = "default", "state_100mhz", "state_200mhz","sleep";
> + pinctrl-0 = <&pinctrl_usdhc2>;
> + pinctrl-1 = <&pinctrl_usdhc2_100mhz>;
> + pinctrl-2 = <&pinctrl_usdhc2_200mhz>;
> + pinctrl-3 = <&pinctrl_usdhc2_sleep>;
> + non-removable;
> + status = "okay";
> +};
> +
> +&usdhc3 {
> + pinctrl-names = "default", "state_100mhz", "state_200mhz","sleep";
> + pinctrl-0 = <&pinctrl_usdhc3>, <&pinctrl_usdhc3_pwr>;
> + pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
> + pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
> + pinctrl-3 = <&pinctrl_usdhc3_sleep>, <&pinctrl_usdhc3_pwr>;
> + vmmc-supply = <®_sd3_vmmc>;
> + mmc-pwrseq = <&wifi_pwrseq>;
> + cap-power-off-card;
> + non-removable;
> + status = "okay";
> +};
> +
> +&usbotg1 {
> + pinctrl-names = "default";
> + disable-over-current;
> + srp-disable;
> + hnp-disable;
> + adp-disable;
> + status = "okay";
> +};
> +
> +
> +&ssi2 {
> + status = "disabled";
> +};
Can be dropped.
Thanks for your patch.
Regards,
Marco
> +
> --
> 2.20.1
>
>
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/2] clk: meson: a1: add support for Amlogic A1 clock driver
From: Jian Hu @ 2019-09-27 9:52 UTC (permalink / raw)
To: Jerome Brunet, Neil Armstrong
Cc: Rob Herring, Jianxin Pan, devicetree, Martin Blumenstingl,
Kevin Hilman, Michael Turquette, linux-kernel, Stephen Boyd,
Qiufang Dai, linux-amlogic, linux-clk, linux-arm-kernel
In-Reply-To: <1j1rw4mmzw.fsf@starbuckisacylon.baylibre.com>
Hi, Jerome
Thank you for review.
On 2019/9/25 23:09, Jerome Brunet wrote:
> On Wed 25 Sep 2019 at 19:44, Jian Hu <jian.hu@amlogic.com> wrote:
>
>> The Amlogic A1 clock includes three parts:
>> peripheral clocks, pll clocks, CPU clocks.
>> sys pll and CPU clocks will be sent in next patch.
>>
>> Unlike the previous series, there is no EE/AO domain
>> in A1 CLK controllers.
>>
>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>> Signed-off-by: Jianxin Pan <jianxin.pan@amlogic.com>
>> ---
>> arch/arm64/Kconfig.platforms | 1 +
>> drivers/clk/meson/Kconfig | 10 +
>> drivers/clk/meson/Makefile | 1 +
>> drivers/clk/meson/a1.c | 2617 ++++++++++++++++++++++++++++++++++++++++++
>> drivers/clk/meson/a1.h | 172 +++
>> 5 files changed, 2801 insertions(+)
>> create mode 100644 drivers/clk/meson/a1.c
>> create mode 100644 drivers/clk/meson/a1.h
>>
>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>> index 16d7614..a48f67d 100644
>> --- a/arch/arm64/Kconfig.platforms
>> +++ b/arch/arm64/Kconfig.platforms
>> @@ -138,6 +138,7 @@ config ARCH_MESON
>> select COMMON_CLK_AXG
>> select COMMON_CLK_G12A
>> select MESON_IRQ_GPIO
>> + select COMMON_CLK_A1
>
> This need to be separate patch addressed to Kevin once your driver is merged
>
ok, I will remove it in PATCH V2. And send it again after the driver is
merged.
>> help
>> This enables support for the arm64 based Amlogic SoCs
>> such as the s905, S905X/D, S912, A113X/D or S905X/D2
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index dabeb43..e6cb4c3 100644
>> --- a/drivers/clk/meson/Kconfig
>> +++ b/drivers/clk/meson/Kconfig
>> @@ -107,3 +107,13 @@ config COMMON_CLK_G12A
>> help
>> Support for the clock controller on Amlogic S905D2, S905X2 and S905Y2
>> devices, aka g12a. Say Y if you want peripherals to work.
>> +
>> +config COMMON_CLK_A1
>> + bool
>> + depends on ARCH_MESON
>> + select COMMON_CLK_MESON_REGMAP
>> + select COMMON_CLK_MESON_DUALDIV
>> + select COMMON_CLK_MESON_PLL
>> + help
>> + Support for the clock controller on Amlogic A113L device,
>> + aka a1. Say Y if you want peripherals to work.
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index 3939f21..6be3a8f 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -19,3 +19,4 @@ obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
>> obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
>> obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
>> obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>> +obj-$(CONFIG_COMMON_CLK_A1) += a1.o
>> diff --git a/drivers/clk/meson/a1.c b/drivers/clk/meson/a1.c
>> new file mode 100644
>> index 0000000..26edae0f
>> --- /dev/null
>> +++ b/drivers/clk/meson/a1.c
>> @@ -0,0 +1,2617 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/init.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of_address.h>
>> +#include "clk-mpll.h"
>> +#include "clk-pll.h"
>> +#include "clk-regmap.h"
>> +#include "vid-pll-div.h"
>> +#include "clk-dualdiv.h"
>> +#include "meson-eeclk.h"
>> +#include "a1.h"
>> +
>> +/* PLLs clock in gates, its parent is xtal */
>> +static struct clk_regmap a1_xtal_clktree = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = SYS_OSCIN_CTRL,
>> + .bit_idx = 0,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "xtal_clktree",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_data = &(const struct clk_parent_data) {
>> + .fw_name = "xtal",
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_IS_CRITICAL,
>
> Is CCF even expected to touch this ever ? what about RO ops ?
> Please review your other clocks with this in mind
>
the clock should not be changed at runtime.clk_regmap_gate_ro_ops
is a good idea. Set RO ops and remove the CLK_IS_CRITICAL flag.
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_xtal_fixpll = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = SYS_OSCIN_CTRL,
>> + .bit_idx = 1,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "xtal_fixpll",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_data = &(const struct clk_parent_data) {
>> + .fw_name = "xtal",
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_IS_CRITICAL,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_xtal_usb_phy = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = SYS_OSCIN_CTRL,
>> + .bit_idx = 2,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "xtal_usb_phy",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_data = &(const struct clk_parent_data) {
>> + .fw_name = "xtal",
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_IS_CRITICAL,
>
> How is an USB clock critical to the system ?
> Please review your other clocks with comment in mind ...
the usb clock does not affect the system,
remove the CLK_IS_CRITICAL flag
>
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_xtal_usb_ctrl = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = SYS_OSCIN_CTRL,
>> + .bit_idx = 3,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "xtal_usb_ctrl",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_data = &(const struct clk_parent_data) {
>> + .fw_name = "xtal",
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_IS_CRITICAL,
>> + },
>> +};
the usb clock does not affect the system,
remove the CLK_IS_CRITICAL flag
>> +
>> +static struct clk_regmap a1_xtal_hifipll = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = SYS_OSCIN_CTRL,
>> + .bit_idx = 4,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "xtal_hifipll",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_data = &(const struct clk_parent_data) {
>> + .fw_name = "xtal",
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_IS_CRITICAL,
>> + },
>> +};
CLK_IS_CRITICAL is need to lock hifi pll.
>> +
>> +static struct clk_regmap a1_xtal_syspll = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = SYS_OSCIN_CTRL,
>> + .bit_idx = 5,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "xtal_syspll",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_data = &(const struct clk_parent_data) {
>> + .fw_name = "xtal",
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_IS_CRITICAL,
>> + },
>> +};
>> +
when CPU clock is at fixed clock, sys pll
will be disabled, xtal_syspll will be disabled too.
when setting sys pll, call clk_set_rate to lock
sys pll, add RO ops to avoid disabling the clock
>> +static struct clk_regmap a1_xtal_dds = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = SYS_OSCIN_CTRL,
>> + .bit_idx = 6,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "xtal_dds",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_data = &(const struct clk_parent_data) {
>> + .fw_name = "xtal",
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_IS_CRITICAL,
>> + },
>> +};
CLK_IS_CRITICAL is need to lock dds
>> +
>> +/* fixed pll = 1536M
>> + *
>> + * fixed pll ----- fclk_div2 = 768M
>> + * |
>> + * ----- fclk_div3 = 512M
>> + * |
>> + * ----- fclk_div5 = 307.2M
>> + * |
>> + * ----- fclk_div7 = 219.4M
>> + */
>
> The framework will make those calculation ... you can remove this
>
ok, I will remote the comment.
>> +static struct clk_regmap a1_fixed_pll_dco = {
>> + .data = &(struct meson_clk_pll_data){
>> + .en = {
>> + .reg_off = ANACTRL_FIXPLL_CTRL0,
>> + .shift = 28,
>> + .width = 1,
>> + },
>> + .m = {
>> + .reg_off = ANACTRL_FIXPLL_CTRL0,
>> + .shift = 0,
>> + .width = 8,
>> + },
>> + .n = {
>> + .reg_off = ANACTRL_FIXPLL_CTRL0,
>> + .shift = 10,
>> + .width = 5,
>> + },
>> + .frac = {
>> + .reg_off = ANACTRL_FIXPLL_CTRL1,
>> + .shift = 0,
>> + .width = 19,
>> + },
>> + .l = {
>> + .reg_off = ANACTRL_FIXPLL_CTRL0,
>> + .shift = 31,
>> + .width = 1,
>> + },
>> + .rst = {
>> + .reg_off = ANACTRL_FIXPLL_CTRL0,
>> + .shift = 29,
>> + .width = 1,
>> + },
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "fixed_pll_dco",
>> + .ops = &meson_clk_pll_ro_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_xtal_fixpll.hw
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_fixed_pll = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = ANACTRL_FIXPLL_CTRL0,
>> + .bit_idx = 20,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "fixed_pll",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_fixed_pll_dco.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
>> +static const struct pll_params_table a1_hifi_pll_params_table[] = {
>> + PLL_PARAMS(128, 5), /* DCO = 614.4M */
>> +};
>> +
>> +static const struct reg_sequence a1_hifi_init_regs[] = {
>> + { .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x10022200 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00301000 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f19480 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x11f19480, .delay_us = 10 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x15f11480, .delay_us = 40 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001140 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
>> +};
>> +
>> +/*
>> + * The Meson A1 HIFI PLL is 614.4M, it requires
>> + * a strict register sequence to enable the PLL.
>> + * set meson_clk_pcie_pll_ops as its ops
>> + */
>
> Could you elaborate on this ? What need to be done to enable the clock ?
> Also the HIFI PLL used to be able to do a *LOT* of different rate which
> might be desirable for audio use case. Why is this one restricted to one
> particular rate ?
>
The audio working frequency are 44.1khz, 48khz and 192khz.
614.4M can meet the three frequency.
after the hifi pll, there are two dividers in Audio clock.
614.4M/3200 = 192khz
614.4M/12800 = 48khz
614,4M/13932 = 44.0999khz
>> +static struct clk_regmap a1_hifi_pll = {
>> + .data = &(struct meson_clk_pll_data){
>> + .en = {
>> + .reg_off = ANACTRL_HIFIPLL_CTRL0,
>> + .shift = 28,
>> + .width = 1,
>> + },
>> + .m = {
>> + .reg_off = ANACTRL_HIFIPLL_CTRL0,
>> + .shift = 0,
>> + .width = 8,
>> + },
>> + .n = {
>> + .reg_off = ANACTRL_HIFIPLL_CTRL0,
>> + .shift = 10,
>> + .width = 5,
>> + },
>> + .frac = {
>> + .reg_off = ANACTRL_HIFIPLL_CTRL1,
>> + .shift = 0,
>> + .width = 19,
>> + },
>> + .l = {
>> + .reg_off = ANACTRL_HIFIPLL_STS,
>> + .shift = 31,
>> + .width = 1,
>> + },
>> + .table = a1_hifi_pll_params_table,
>> + .init_regs = a1_hifi_init_regs,
>> + .init_count = ARRAY_SIZE(a1_hifi_init_regs),
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "hifi_pll",
>> + .ops = &meson_clk_pcie_pll_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_xtal_hifipll.hw
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static struct clk_fixed_factor a1_fclk_div2_div = {
>> + .mult = 1,
>> + .div = 2,
>> + .hw.init = &(struct clk_init_data){
>> + .name = "fclk_div2_div",
>> + .ops = &clk_fixed_factor_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_fixed_pll.hw
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_fclk_div2 = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = ANACTRL_FIXPLL_CTRL0,
>> + .bit_idx = 21,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "fclk_div2",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_fclk_div2_div.hw
>> + },
>> + .num_parents = 1,
>> + /*
>> + * add CLK_IS_CRITICAL flag to avoid being disabled by clk core
>> + * or its children clocks.
>> + */
>
> The meaning of this flag is already documented in clk-provider.h
> The reason why you need this flag is lot more interesting here ...
>
> Same below
ok, I will replace new comments here.
>
>> + .flags = CLK_IS_CRITICAL,
>> + },
>> +};
>> +
>> +static struct clk_fixed_factor a1_fclk_div3_div = {
>> + .mult = 1,
>> + .div = 3,
>> + .hw.init = &(struct clk_init_data){
>> + .name = "fclk_div3_div",
>> + .ops = &clk_fixed_factor_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_fixed_pll.hw
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_fclk_div3 = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = ANACTRL_FIXPLL_CTRL0,
>> + .bit_idx = 22,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "fclk_div3",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_fclk_div3_div.hw
>> + },
>> + .num_parents = 1,
>> + /*
>> + * add CLK_IS_CRITICAL flag to avoid being disabled by clk core
>> + * its children clocks.
>> + */
>> + .flags = CLK_IS_CRITICAL,
>> + },
>> +};
>> +
>> +static struct clk_fixed_factor a1_fclk_div5_div = {
>> + .mult = 1,
>> + .div = 5,
>> + .hw.init = &(struct clk_init_data){
>> + .name = "fclk_div5_div",
>> + .ops = &clk_fixed_factor_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_fixed_pll.hw
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_fclk_div5 = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = ANACTRL_FIXPLL_CTRL0,
>> + .bit_idx = 23,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "fclk_div5",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_fclk_div5_div.hw
>> + },
>> + .num_parents = 1,
>> + /*
>> + * add CLK_IS_CRITICAL flag to avoid being disabled by clk core
>> + * its children clocks.
>> + */
>> + .flags = CLK_IS_CRITICAL,
>> + },
>> +};
>> +
>> +static struct clk_fixed_factor a1_fclk_div7_div = {
>> + .mult = 1,
>> + .div = 7,
>> + .hw.init = &(struct clk_init_data){
>> + .name = "fclk_div7_div",
>> + .ops = &clk_fixed_factor_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_fixed_pll.hw
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_fclk_div7 = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = ANACTRL_FIXPLL_CTRL0,
>> + .bit_idx = 23,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "fclk_div7",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_fclk_div7_div.hw
>> + },
>> + .num_parents = 1,
>> + /*
>> + * add CLK_IS_CRITICAL flag to avoid being disabled by clk core
>> + * or its children clock.
>> + */
>> + .flags = CLK_IS_CRITICAL,
>> + },
>> +};
>> +
>> +/* sys clk = 64M */
>
> Again this comment does not help understanding the driver code, please
> remove
>
got it.
>> +static u32 mux_table_sys_ab_clk[] = { 0, 1, 2, 3};
>
> This table is useless
>
>> +static const struct clk_parent_data sys_ab_clk_parent_data[] = {
>> + { .fw_name = "xtal", },
>> + { .hw = &a1_fclk_div2.hw },
>> + { .hw = &a1_fclk_div3.hw },
>> + { .hw = &a1_fclk_div5.hw },
>> +};
>> +
>> +static struct clk_regmap a1_sys_b_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = SYS_CLK_CTRL0,
>> + .mask = 0x7,
>> + .shift = 26,
>> + .table = mux_table_sys_ab_clk,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "sys_b_sel",
>> + .ops = &clk_regmap_mux_ro_ops,
>> + .parent_data = sys_ab_clk_parent_data,
>> + .num_parents = ARRAY_SIZE(sys_ab_clk_parent_data),
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_sys_b_div = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = SYS_CLK_CTRL0,
>> + .shift = 16,
>> + .width = 10,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "sys_b_div",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_sys_b_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_sys_b = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = SYS_CLK_CTRL0,
>> + .bit_idx = 29,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "sys_b",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_sys_b_div.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>
> Why does this need to be critical ?
> The a1_sys_clk (leaf of the block) could possibly be critical (if you
> explain why) ... but the flag should not be needed in the middle of the
> block
>
ok, the a1_sys_clk should not be changed at runtime, it is the same with
the clk81 clock in G12A. Just use another name in A1.
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_sys_a_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = SYS_CLK_CTRL0,
>> + .mask = 0x7,
>> + .shift = 10,
>> + .table = mux_table_sys_ab_clk,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "sys_a_sel",
>> + .ops = &clk_regmap_mux_ro_ops,
>> + .parent_data = sys_ab_clk_parent_data,
>> + .num_parents = ARRAY_SIZE(sys_ab_clk_parent_data),
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_sys_a_div = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = SYS_CLK_CTRL0,
>> + .shift = 0,
>> + .width = 10,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "sys_a_div",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_sys_a_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_sys_a = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = SYS_CLK_CTRL0,
>> + .bit_idx = 13,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "sys_a",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_sys_a_div.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_sys_clk = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = SYS_CLK_CTRL0,
>> + .mask = 0x1,
>> + .shift = 31,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "sys_clk",
>> + .ops = &clk_regmap_mux_ro_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_sys_a.hw, &a1_sys_b.hw,
>> + },
>> + .num_parents = 2,
>
> SET_RATE_PARENT ?
>
the ops is RO, Maybe no flag is better.
>> + },
>> +};
>> +
>> +/* rtc 32k clock in */
>> +static struct clk_regmap a1_rtc_32k_clkin = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = RTC_BY_OSCIN_CTRL0,
>> + .bit_idx = 31,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "rtc_32k_clkin",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_data = &(const struct clk_parent_data) {
>> + .fw_name = "xtal",
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static const struct meson_clk_dualdiv_param a1_32k_div_table[] = {
>> + {
>> + .dual = 1,
>> + .n1 = 733,
>> + .m1 = 8,
>> + .n2 = 732,
>> + .m2 = 11,
>> + }, {}
>> +};
>> +
>> +static struct clk_regmap a1_rtc_32k_div = {
>> + .data = &(struct meson_clk_dualdiv_data){
>> + .n1 = {
>> + .reg_off = RTC_BY_OSCIN_CTRL0,
>> + .shift = 0,
>> + .width = 12,
>> + },
>> + .n2 = {
>> + .reg_off = RTC_BY_OSCIN_CTRL0,
>> + .shift = 12,
>> + .width = 12,
>> + },
>> + .m1 = {
>> + .reg_off = RTC_BY_OSCIN_CTRL1,
>> + .shift = 0,
>> + .width = 12,
>> + },
>> + .m2 = {
>> + .reg_off = RTC_BY_OSCIN_CTRL1,
>> + .shift = 12,
>> + .width = 12,
>> + },
>> + .dual = {
>> + .reg_off = RTC_BY_OSCIN_CTRL0,
>> + .shift = 28,
>> + .width = 1,
>> + },
>> + .table = a1_32k_div_table,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "rtc_32k_div",
>> + .ops = &meson_clk_dualdiv_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_rtc_32k_clkin.hw
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_rtc_32k_xtal = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = RTC_BY_OSCIN_CTRL1,
>> + .bit_idx = 24,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "rtc_32k_xtal",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_rtc_32k_clkin.hw
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static u32 rtc_32k_sel[] = { 0, 1 };
>
> useless
>
ok, I will delete the table.
>> +
>> +static struct clk_regmap a1_rtc_32k_sel = {
>> + .data = &(struct clk_regmap_mux_data) {
>> + .offset = RTC_CTRL,
>> + .mask = 0x3,
>> + .shift = 0,
>> + .table = rtc_32k_sel,
>> + .flags = CLK_MUX_ROUND_CLOSEST,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "rtc_32k_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_rtc_32k_xtal.hw,
>> + &a1_rtc_32k_div.hw,
>> + },
>> + .num_parents = 2,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +struct clk_regmap a1_rtc_clk = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = RTC_BY_OSCIN_CTRL0,
>> + .bit_idx = 30,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "rtc_clk",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_rtc_32k_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +/* dsp a clk */
>> +static u32 mux_table_dsp_ab[] = { 0, 1, 2, 3, 4, 7 };
>> +static const struct clk_parent_data dsp_ab_clk_parent_data[] = {
>> + { .fw_name = "xtal", },
>> + { .hw = &a1_fclk_div2.hw },
>> + { .hw = &a1_fclk_div3.hw },
>> + { .hw = &a1_fclk_div5.hw },
>> + { .hw = &a1_hifi_pll.hw },
>> + { .hw = &a1_rtc_clk.hw },
>> +};
>> +
>> +static struct clk_regmap a1_dspa_a_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = DSPA_CLK_CTRL0,
>> + .mask = 0x7,
>> + .shift = 10,
>> + .table = mux_table_dsp_ab,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dspa_a_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_data = dsp_ab_clk_parent_data,
>> + .num_parents = ARRAY_SIZE(dsp_ab_clk_parent_data),
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspa_a_div = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = DSPA_CLK_CTRL0,
>> + .shift = 0,
>> + .width = 10,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dspa_a_div",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspa_a_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspa_a = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = DSPA_CLK_CTRL0,
>> + .bit_idx = 13,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "dspa_a",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspa_a_div.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>
> This CLK_IGNORE_UNUSED, if justified, needs to be explained
>
ok, DSP cowork withe ARM in A1, dsp clock shoud not be changed at runtime.
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspa_b_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = DSPA_CLK_CTRL0,
>> + .mask = 0x7,
>> + .shift = 26,
>> + .table = mux_table_dsp_ab,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dspa_b_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_data = dsp_ab_clk_parent_data,
>> + .num_parents = ARRAY_SIZE(dsp_ab_clk_parent_data),
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspa_b_div = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = DSPA_CLK_CTRL0,
>> + .shift = 16,
>> + .width = 10,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dspa_b_div",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspa_b_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspa_b = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = DSPA_CLK_CTRL0,
>> + .bit_idx = 29,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "dspa_b",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspa_b_div.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspa_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = DSPA_CLK_CTRL0,
>> + .mask = 0x1,
>> + .shift = 15,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dspa_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_data = (const struct clk_parent_data []) {
>> + { .hw = &a1_dspa_a.hw },
>> + { .hw = &a1_dspa_b.hw },
>> + },
>> + .num_parents = 2,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspa_en_dspa = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = DSPA_CLK_EN,
>> + .bit_idx = 1,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "dspa_en_dspa",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspa_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspa_en_nic = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = DSPA_CLK_EN,
>> + .bit_idx = 0,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "dspa_en_nic",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspa_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
>> +/* dsp b clk */
>> +static struct clk_regmap a1_dspb_a_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = DSPB_CLK_CTRL0,
>> + .mask = 0x7,
>> + .shift = 10,
>> + .table = mux_table_dsp_ab,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dspb_a_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_data = dsp_ab_clk_parent_data,
>> + .num_parents = ARRAY_SIZE(dsp_ab_clk_parent_data),
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspb_a_div = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = DSPB_CLK_CTRL0,
>> + .shift = 0,
>> + .width = 10,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dspb_a_div",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspb_a_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspb_a = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = DSPB_CLK_CTRL0,
>> + .bit_idx = 13,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "dspb_a",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspb_a_div.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspb_b_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = DSPB_CLK_CTRL0,
>> + .mask = 0x7,
>> + .shift = 26,
>> + .table = mux_table_dsp_ab,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dspb_b_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_data = dsp_ab_clk_parent_data,
>> + .num_parents = ARRAY_SIZE(dsp_ab_clk_parent_data),
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspb_b_div = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = DSPB_CLK_CTRL0,
>> + .shift = 16,
>> + .width = 10,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dspb_b_div",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspb_b_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspb_b = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = DSPB_CLK_CTRL0,
>> + .bit_idx = 29,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "dspb_b",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspb_b_div.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspb_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = DSPB_CLK_CTRL0,
>> + .mask = 0x1,
>> + .shift = 15,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dspb_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspb_a.hw, &a1_dspb_b.hw,
>> + },
>> + .num_parents = 2,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspb_en_dspb = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = DSPB_CLK_EN,
>> + .bit_idx = 1,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "dspb_en_dspb",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspb_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dspb_en_nic = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = DSPB_CLK_EN,
>> + .bit_idx = 0,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "dspb_en_nic",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dspb_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
>> +/* 12M/24M clock */
>> +static struct clk_regmap a1_24m = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = CLK12_24_CTRL,
>> + .bit_idx = 11,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "24m",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_data = &(const struct clk_parent_data) {
>> + .fw_name = "xtal",
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static struct clk_fixed_factor a1_24m_div2 = {
>> + .mult = 1,
>> + .div = 2,
>> + .hw.init = &(struct clk_init_data){
>> + .name = "24m_div2",
>> + .ops = &clk_fixed_factor_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_24m.hw
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_12m = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = CLK12_24_CTRL,
>> + .bit_idx = 10,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "12m",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_24m_div2.hw
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_fclk_div2_divn_pre = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = CLK12_24_CTRL,
>> + .shift = 0,
>> + .width = 8,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "fclk_div2_divn_pre",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_fclk_div2.hw
>> + },
>> + .num_parents = 1,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_fclk_div2_divn = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = CLK12_24_CTRL,
>> + .bit_idx = 12,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "fclk_div2_divn",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_fclk_div2_divn_pre.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +/* gen clk */
>
> Either put something helpful or don't put anything
> For example explaining why there is a hole in the table bellow and what
> is parent 4 (if any) would be helpful.
ok, parent 2 is sys_pll_div16, sys pll and related clock are not
completeed, parent 4 is one of clock measurement source, it relies on
the clock measurement register configuration.
gen clock is a debug and monitor clock.
>
>> +static u32 gen_clk_table[] = { 0, 1, 3, 5, 6, 7, 8 };
>> +static const struct clk_parent_data gen_clk_parent_data[] = {
>> + { .fw_name = "xtal", },
>> + { .hw = &a1_rtc_clk.hw },
>> + { .hw = &a1_hifi_pll.hw },
>> + { .hw = &a1_fclk_div2.hw },
>> + { .hw = &a1_fclk_div3.hw },
>> + { .hw = &a1_fclk_div5.hw },
>> + { .hw = &a1_fclk_div7.hw },
>> +};
>> +
>> +static struct clk_regmap a1_gen_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = GEN_CLK_CTRL,
>> + .mask = 0xf,
>> + .shift = 12,
>> + .table = gen_clk_table,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "gen_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_data = gen_clk_parent_data,
>> + .num_parents = ARRAY_SIZE(gen_clk_parent_data),
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_gen_div = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = GEN_CLK_CTRL,
>> + .shift = 0,
>> + .width = 11,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "gen_div",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_gen_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_gen = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = GEN_CLK_CTRL,
>> + .bit_idx = 11,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "gen",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_gen_div.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_saradc_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = SAR_ADC_CLK_CTRL,
>> + .mask = 0x1,
>> + .shift = 9,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "saradc_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_data = (const struct clk_parent_data []) {
>> + { .fw_name = "xtal", },
>> + { .hw = &a1_sys_clk.hw, },
>> + },
>> + .num_parents = 2,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_saradc_div = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = SAR_ADC_CLK_CTRL,
>> + .shift = 0,
>> + .width = 8,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "saradc_div",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_saradc_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_saradc_clk = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = SAR_ADC_CLK_CTRL,
>> + .bit_idx = 8,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "saradc_clk",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_saradc_div.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +/* pwm a/b/c/d parent data */
>> +static const struct clk_parent_data pwm_parent_data[] = {
>> + { .fw_name = "xtal", },
>> + { .hw = &a1_sys_clk.hw },
>> +};
>> +
>> +/*
>> + * add CLK_IGNORE_UNUSED flag for pwm controller GATE
>> + * clk core will disable unused clock, it may disable
>> + * vddcore voltage which contrlled by one pwm in bl21.
> ^
> typo
>
>> + * add the flag to avoid changing cpu voltage.
>
> 80 char per line is ok
> This comment would be better placed next to the corresponding flag
>
> Also you could explain that flag could be removed when CCF has some
> hand-off mechanism (similar comments you can copy in other driver)
>
ok, contrlled --> controlled
I will move the comment next to the flag
I have gotten the similar comments in g12a.c
>> + */
>> +/* pwm a clk */
>> +static struct clk_regmap a1_pwm_a_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = PWM_CLK_AB_CTRL,
>> + .mask = 0x1,
>> + .shift = 9,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "pwm_a_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_data = pwm_parent_data,
>> + .num_parents = ARRAY_SIZE(pwm_parent_data),
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_pwm_a_div = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = PWM_CLK_AB_CTRL,
>> + .shift = 0,
>> + .width = 8,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "pwm_a_div",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_pwm_a_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_pwm_a = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = PWM_CLK_AB_CTRL,
>> + .bit_idx = 8,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "pwm_a",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_pwm_a_div.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
[...]
>> +/* usb bus clk */
>> +static const struct clk_parent_data usb_bus_parent_data[] = {
>> + { .fw_name = "xtal", },
>> + { .hw = &a1_sys_clk.hw },
>> + { .hw = &a1_fclk_div3.hw },
>> + { .hw = &a1_fclk_div5.hw },
>> +};
>> +
>> +static struct clk_regmap a1_usb_bus_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = USB_BUSCLK_CTRL,
>> + .mask = 0x3,
>> + .shift = 9,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "usb_bus_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_data = usb_bus_parent_data,
>> + .num_parents = ARRAY_SIZE(usb_bus_parent_data),
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>
> Again, every unused flag need to be properly explained
>
ok, I will confirm this flag
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_usb_bus_div = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = USB_BUSCLK_CTRL,
>> + .shift = 0,
>> + .width = 8,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "usb_bus_div",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_usb_bus_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_usb_bus = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = USB_BUSCLK_CTRL,
>> + .bit_idx = 8,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "usb_bus",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_usb_bus_div.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
[...]
>> +
>> +/* dmc clk */
>> +static const struct clk_hw *dmc_parent_hws[] = {
>> + &a1_fclk_div2.hw,
>> + &a1_fclk_div3.hw,
>> + &a1_fclk_div5.hw,
>> + &a1_hifi_pll.hw,
>> +};
>> +
>> +static struct clk_regmap a1_dmc_sel = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = DMC_CLK_CTRL,
>> + .mask = 0x3,
>> + .shift = 9,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dmc_sel",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_hws = dmc_parent_hws,
>> + .num_parents = ARRAY_SIZE(dmc_parent_hws),
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dmc_div = {
>> + .data = &(struct clk_regmap_div_data){
>> + .offset = DMC_CLK_CTRL,
>> + .shift = 0,
>> + .width = 8,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dmc_div",
>> + .ops = &clk_regmap_divider_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dmc_sel.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dmc_sel2 = {
>> + .data = &(struct clk_regmap_mux_data){
>> + .offset = DMC_CLK_CTRL,
>> + .mask = 0x1,
>> + .shift = 15,
>> + },
>> + .hw.init = &(struct clk_init_data){
>> + .name = "dmc_sel2",
>> + .ops = &clk_regmap_mux_ops,
>> + .parent_data = (const struct clk_parent_data []) {
>> + { .hw = &a1_dmc_div.hw },
>> + { .fw_name = "xtal", },
>> + },
>> + .num_parents = 2,
>> + .flags = CLK_SET_RATE_PARENT,
>> + },
>> +};
>> +
>> +static struct clk_regmap a1_dmc = {
>> + .data = &(struct clk_regmap_gate_data){
>> + .offset = DMC_CLK_CTRL,
>> + .bit_idx = 8,
>> + },
>> + .hw.init = &(struct clk_init_data) {
>> + .name = "dmc",
>> + .ops = &clk_regmap_gate_ops,
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &a1_dmc_sel2.hw
>> + },
>> + .num_parents = 1,
>> + /*
>> + * add CLK_IGNORE_UNUSED to avoid hangup
>> + * DDR clock should not change at runtime
>> + */
>
> CRITICAL ? RO ops maybe ?
RO ops is better
>
>> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> + },
>> +};
>> +
[...]
>> +
>> +#define MESON_GATE(_name, _reg, _bit) \
>> + MESON_PCLK(_name, _reg, _bit, &a1_sys_clk.hw)
>> +
>> +/* CLKTREE_SYS_CLK_EN0 */
>
> Full caps is not ok ... and once again, try to add comment which helps
ok, I will add some useful comment.
>
>> +static MESON_GATE(a1_clk_tree, SYS_CLK_EN0, 0);
>> +static MESON_GATE(a1_reset_ctrl, SYS_CLK_EN0, 1);
>> +static MESON_GATE(a1_analog_ctrl, SYS_CLK_EN0, 2);
>> +static MESON_GATE(a1_pwr_ctrl, SYS_CLK_EN0, 3);
>> +static MESON_GATE(a1_pad_ctrl, SYS_CLK_EN0, 4);
>> +static MESON_GATE(a1_sys_ctrl, SYS_CLK_EN0, 5);
>> +static MESON_GATE(a1_temp_sensor, SYS_CLK_EN0, 6);
>> +static MESON_GATE(a1_am2axi_dev, SYS_CLK_EN0, 7);
>> +static MESON_GATE(a1_spicc_b, SYS_CLK_EN0, 8);
>> +static MESON_GATE(a1_spicc_a, SYS_CLK_EN0, 9);
>> +static MESON_GATE(a1_clk_msr, SYS_CLK_EN0, 10);
>> +static MESON_GATE(a1_audio, SYS_CLK_EN0, 11);
>> +static MESON_GATE(a1_jtag_ctrl, SYS_CLK_EN0, 12);
>> +static MESON_GATE(a1_saradc, SYS_CLK_EN0, 13);
>> +static MESON_GATE(a1_pwm_ef, SYS_CLK_EN0, 14);
>> +static MESON_GATE(a1_pwm_cd, SYS_CLK_EN0, 15);
>> +static MESON_GATE(a1_pwm_ab, SYS_CLK_EN0, 16);
>> +static MESON_GATE(a1_cec, SYS_CLK_EN0, 17);
>> +static MESON_GATE(a1_i2c_s, SYS_CLK_EN0, 18);
>> +static MESON_GATE(a1_ir_ctrl, SYS_CLK_EN0, 19);
>> +static MESON_GATE(a1_i2c_m_d, SYS_CLK_EN0, 20);
>> +static MESON_GATE(a1_i2c_m_c, SYS_CLK_EN0, 21);
>> +static MESON_GATE(a1_i2c_m_b, SYS_CLK_EN0, 22);
>> +static MESON_GATE(a1_i2c_m_a, SYS_CLK_EN0, 23);
>> +static MESON_GATE(a1_acodec, SYS_CLK_EN0, 24);
>> +static MESON_GATE(a1_otp, SYS_CLK_EN0, 25);
>> +static MESON_GATE(a1_sd_emmc_a, SYS_CLK_EN0, 26);
>> +static MESON_GATE(a1_usb_phy, SYS_CLK_EN0, 27);
>> +static MESON_GATE(a1_usb_ctrl, SYS_CLK_EN0, 28);
>> +static MESON_GATE(a1_sys_dspb, SYS_CLK_EN0, 29);
>> +static MESON_GATE(a1_sys_dspa, SYS_CLK_EN0, 30);
>> +static MESON_GATE(a1_dma, SYS_CLK_EN0, 31);
>> +/* CLKTREE_SYS_CLK_EN1 */
>> +static MESON_GATE(a1_irq_ctrl, SYS_CLK_EN1, 0);
>> +static MESON_GATE(a1_nic, SYS_CLK_EN1, 1);
>> +static MESON_GATE(a1_gic, SYS_CLK_EN1, 2);
>> +static MESON_GATE(a1_uart_c, SYS_CLK_EN1, 3);
>> +static MESON_GATE(a1_uart_b, SYS_CLK_EN1, 4);
>> +static MESON_GATE(a1_uart_a, SYS_CLK_EN1, 5);
>> +static MESON_GATE(a1_sys_psram, SYS_CLK_EN1, 6);
>> +static MESON_GATE(a1_rsa, SYS_CLK_EN1, 8);
>> +static MESON_GATE(a1_coresight, SYS_CLK_EN1, 9);
>> +/* CLKTREE_AXI_CLK_EN */
>> +static MESON_GATE(a1_am2axi_vad, AXI_CLK_EN, 0);
>> +static MESON_GATE(a1_audio_vad, AXI_CLK_EN, 1);
>> +static MESON_GATE(a1_axi_dmc, AXI_CLK_EN, 3);
>> +static MESON_GATE(a1_axi_psram, AXI_CLK_EN, 4);
>> +static MESON_GATE(a1_ramb, AXI_CLK_EN, 5);
>> +static MESON_GATE(a1_rama, AXI_CLK_EN, 6);
>> +static MESON_GATE(a1_axi_spifc, AXI_CLK_EN, 7);
>> +static MESON_GATE(a1_axi_nic, AXI_CLK_EN, 8);
>> +static MESON_GATE(a1_axi_dma, AXI_CLK_EN, 9);
>> +static MESON_GATE(a1_cpu_ctrl, AXI_CLK_EN, 10);
>> +static MESON_GATE(a1_rom, AXI_CLK_EN, 11);
>> +static MESON_GATE(a1_prod_i2c, AXI_CLK_EN, 12);
>> +
[...]
>> +
>> +/* Convenience table to populate regmap in .probe
>> + * Peripheral clock register base address is 0xfe000800
>
> this info will be in DT
I will remove the address commit
>
>> + */
[...]
>> +
>> +/*
>> + * cpu clock register base address is 0xfd000080
>> + */
>> +static struct clk_regmap *const a1_cpu_clk_regmaps[] = {
>> + /* TODO */
>> +};
>> +
>> +/*
>> + * pll clock register base address is 0xfe007c00
>> + */
>> +static struct clk_regmap *const a1_pll_clk_regmaps[] = {
>> + &a1_fixed_pll_dco,
>> + &a1_fixed_pll,
>> + &a1_hifi_pll,
>> + &a1_fclk_div2,
>> + &a1_fclk_div3,
>> + &a1_fclk_div5,
>> + &a1_fclk_div7,
>> +};
>> +
>> +static struct regmap_config clkc_regmap_config = {
>> + .reg_bits = 32,
>> + .val_bits = 32,
>> + .reg_stride = 4,
>> +};
>> +
>> +static struct regmap *a1_regmap_resource(struct device *dev, char *name)
>> +{
>> + struct resource res;
>> + void __iomem *base;
>> + int i;
>> + struct device_node *node = dev->of_node;
>> +
>> + i = of_property_match_string(node, "reg-names", name);
>> + if (of_address_to_resource(node, i, &res))
>> + return ERR_PTR(-ENOENT);
>> +
>> + base = devm_ioremap_resource(dev, &res);
>> + if (IS_ERR(base))
>> + return ERR_CAST(base);
>> +
>> + clkc_regmap_config.max_register = resource_size(&res) - 4;
>> + clkc_regmap_config.name = devm_kasprintf(dev, GFP_KERNEL,
>> + "%s-%s", node->name,
>> + name);
>> + if (!clkc_regmap_config.name)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + return devm_regmap_init_mmio(dev, base, &clkc_regmap_config);
>> +}
>> +
>> +static int a1_clkc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct regmap *peripheral_map, *pll_map, *cpu_map;
>> + int ret, i;
>> +
>> + /* Get regmap for different clock area */
>> + peripheral_map = a1_regmap_resource(dev, "peripheral");
>> + if (IS_ERR(peripheral_map)) {
>> + dev_err(dev, "peripheral clk registers not found\n");
>> + return PTR_ERR(peripheral_map);
>> + }
>> +
>> + pll_map = a1_regmap_resource(dev, "pll");
>> + if (IS_ERR(pll_map)) {
>> + dev_err(dev, "pll clk registers not found\n");
>> + return PTR_ERR(pll_map);
>> + }
>> +
>> + cpu_map = a1_regmap_resource(dev, "cpu");
>> + if (IS_ERR(cpu_map)) {
>> + dev_err(dev, "cpu clk registers not found\n");
>> + return PTR_ERR(cpu_map);
>> + }
>> +
>> + /* Populate regmap for the regmap backed clocks */
>> + for (i = 0; i < ARRAY_SIZE(a1_clk_regmaps); i++)
>> + a1_clk_regmaps[i]->map = peripheral_map;
>> +
>> + for (i = 0; i < ARRAY_SIZE(a1_cpu_clk_regmaps); i++)
>> + a1_cpu_clk_regmaps[i]->map = cpu_map;
>> +
>> + for (i = 0; i < ARRAY_SIZE(a1_pll_clk_regmaps); i++)
>> + a1_pll_clk_regmaps[i]->map = pll_map;
>
> Unless you have a rock solid reason why you absolutely need to make
> single controller out of this 3 regions, Nack.
>
As comment replying in
Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml, I will
implement three clock drivers.
>> +
>> + for (i = 0; i < a1_hw_onecell_data.num; i++) {
>> + /* array might be sparse */
>> + if (!a1_hw_onecell_data.hws[i])
>> + continue;
>> +
>> + ret = devm_clk_hw_register(dev, a1_hw_onecell_data.hws[i]);
>> + if (ret) {
>> + dev_err(dev, "Clock registration failed\n");
>> + return ret;
>> + }
>> + }
>> +
>> + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>> + &a1_hw_onecell_data);
>> +}
>> +
>> +static const struct of_device_id clkc_match_table[] = {
>> + { .compatible = "amlogic,a1-clkc" },
>> + { /* sentinel */ }
>> +};
>> +
>> +static struct platform_driver a1_driver = {
>> + .probe = a1_clkc_probe,
>> + .driver = {
>> + .name = "a1-clkc",
>> + .of_match_table = clkc_match_table,
>> + },
>> +};
>> +
>> +builtin_platform_driver(a1_driver);
>> diff --git a/drivers/clk/meson/a1.h b/drivers/clk/meson/a1.h
>> new file mode 100644
>> index 0000000..f2291c4
>> --- /dev/null
>> +++ b/drivers/clk/meson/a1.h
>> @@ -0,0 +1,172 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef __A1_H
>> +#define __A1_H
>> +
>> +/*
>> + * peripheral clock controller register address
>> + * APB_BASE: APB0_BASE_ADDR = 0xfe000800
>
> Again this info is DT and might become wrong if your controller is ever
> compatible with another SoC
>
I will remove the address comment, the right address is 0xfe000080.
>> + */
>> +#define SYS_OSCIN_CTRL 0x0
>> +#define RTC_BY_OSCIN_CTRL0 0x4
>> +#define RTC_BY_OSCIN_CTRL1 0x8
>> +#define RTC_CTRL 0xc
>> +#define SYS_CLK_CTRL0 0x10
>> +#define AXI_CLK_CTRL0 0x14
>> +#define SYS_CLK_EN0 0x1c
>> +#define SYS_CLK_EN1 0x20
>> +#define AXI_CLK_EN 0x24
>> +#define DSPA_CLK_EN 0x28
>> +#define DSPB_CLK_EN 0x2c
>> +#define DSPA_CLK_CTRL0 0x30
>> +#define DSPB_CLK_CTRL0 0x34
>> +#define CLK12_24_CTRL 0x38
>> +#define GEN_CLK_CTRL 0x3c
>> +#define TIMESTAMP_CTRL0 0x40
>> +#define TIMESTAMP_CTRL1 0x44
>> +#define TIMESTAMP_CTRL2 0x48
>> +#define TIMESTAMP_VAL0 0x4c
>> +#define TIMESTAMP_VAL1 0x50
>> +#define TIMEBASE_CTRL0 0x54
>> +#define TIMEBASE_CTRL1 0x58
>> +#define SAR_ADC_CLK_CTRL 0xc0
>> +#define PWM_CLK_AB_CTRL 0xc4
>> +#define PWM_CLK_CD_CTRL 0xc8
>> +#define PWM_CLK_EF_CTRL 0xcc
>> +#define SPICC_CLK_CTRL 0xd0
>> +#define TS_CLK_CTRL 0xd4
>> +#define SPIFC_CLK_CTRL 0xd8
>> +#define USB_BUSCLK_CTRL 0xdc
>> +#define SD_EMMC_CLK_CTRL 0xe0
>> +#define CECA_CLK_CTRL0 0xe4
>> +#define CECA_CLK_CTRL1 0xe8
>> +#define CECB_CLK_CTRL0 0xec
>> +#define CECB_CLK_CTRL1 0xf0
>> +#define PSRAM_CLK_CTRL 0xf4
>> +#define DMC_CLK_CTRL 0xf8
>> +#define FCLK_DIV1_SEL 0xfc
>> +#define TST_CTRL 0x100
>> +/*
>> + * For PLL register offset
>> + * APB_BASE: APB0_BASE_ADDR = 0xfe007c00
>> + */
>> +#define ANACTRL_FIXPLL_CTRL0 0x80
>> +#define ANACTRL_FIXPLL_CTRL1 0x84
>> +#define ANACTRL_FIXPLL_CTRL2 0x88
>> +#define ANACTRL_FIXPLL_CTRL3 0x8c
>> +#define ANACTRL_FIXPLL_CTRL4 0x90
>> +#define ANACTRL_FIXPLL_STS 0x94
>> +#define ANACTRL_SYSPLL_CTRL0 0x100
>> +#define ANACTRL_SYSPLL_CTRL1 0x104
>> +#define ANACTRL_SYSPLL_CTRL2 0x108
>> +#define ANACTRL_SYSPLL_CTRL3 0x10c
>> +#define ANACTRL_SYSPLL_CTRL4 0x110
>> +#define ANACTRL_SYSPLL_STS 0x114
>> +#define ANACTRL_HIFIPLL_CTRL0 0x140
>> +#define ANACTRL_HIFIPLL_CTRL1 0x144
>> +#define ANACTRL_HIFIPLL_CTRL2 0x148
>> +#define ANACTRL_HIFIPLL_CTRL3 0x14c
>> +#define ANACTRL_HIFIPLL_CTRL4 0x150
>> +#define ANACTRL_HIFIPLL_STS 0x154
>> +#define ANACTRL_AUDDDS_CTRL0 0x180
>> +#define ANACTRL_AUDDDS_CTRL1 0x184
>> +#define ANACTRL_AUDDDS_CTRL2 0x188
>> +#define ANACTRL_AUDDDS_CTRL3 0x18c
>> +#define ANACTRL_AUDDDS_CTRL4 0x190
>> +#define ANACTRL_AUDDDS_STS 0x194
>> +#define ANACTRL_MISCTOP_CTRL0 0x1c0
>> +#define ANACTRL_POR_CNTL 0x208
>> +/*
>> + * CPU clok register offset
>> + * APB_BASE: APB1_BASE_ADDR = 0xfd000080
>> + */
>> +#define CPUCTRL_CLK_CTRL0 0x00
>> +#define CPUCTRL_CLK_CTRL1 0x04
>> +#define CPUCTRL_CLK_CTRL2 0x08
>> +#define CPUCTRL_CLK_CTRL3 0x0c
>> +#define CPUCTRL_CLK_CTRL4 0x10
>> +#define CPUCTRL_CLK_CTRL5 0x14
>> +#define CPUCTRL_CLK_CTRL6 0x18
>> +#define CPUCTRL_CLK_CTRL7 0x1c
>> +
>> +#define CLKID_FIXED_PLL_DCO 0
>> +#define CLKID_SYS_A_SEL 10
>> +#define CLKID_SYS_A_DIV 11
>> +#define CLKID_SYS_A 12
>> +#define CLKID_SYS_B_SEL 13
>> +#define CLKID_SYS_B_DIV 14
>> +#define CLKID_SYS_B 15
>> +#define CLKID_XTAL_CLKTREE 18
>> +#define CLKID_XTAL_FIXPLL 19
>> +#define CLKID_XTAL_USB_PHY 20
>> +#define CLKID_XTAL_USB_CTRL 21
>> +#define CLKID_XTAL_HIFIPLL 22
>> +#define CLKID_XTAL_SYSPLL 23
>> +#define CLKID_XTAL_DDS 24
>> +#define CLKID_DSPA_A_SEL 78
>> +#define CLKID_DSPA_A_DIV 79
>> +#define CLKID_DSPA_A 80
>> +#define CLKID_DSPA_B_SEL 81
>> +#define CLKID_DSPA_B_DIV 82
>> +#define CLKID_DSPA_B 83
>> +#define CLKID_DSPB_A_SEL 85
>> +#define CLKID_DSPB_A_DIV 86
>> +#define CLKID_DSPB_A 87
>> +#define CLKID_DSPB_B_SEL 88
>> +#define CLKID_DSPB_B_DIV 89
>> +#define CLKID_DSPB_B 90
>> +#define CLKID_RTC_32K_CLKIN 96
>> +#define CLKID_RTC_32K_DIV 97
>> +#define CLKID_RTC_32K_XTAL 98
>> +#define CLKID_RTC_32K_SEL 99
>> +#define CLKID_CECB_32K_CLKIN 106
>> +#define CLKID_CECB_32K_DIV 107
>> +#define CLKID_CECB_32K_SEL_PRE 108
>> +#define CLKID_CECB_32K_SEL 109
>> +#define CLKID_CECA_32K_CLKIN 101
>> +#define CLKID_CECA_32K_DIV 102
>> +#define CLKID_CECA_32K_SEL_PRE 103
>> +#define CLKID_CECA_32K_SEL 104
>> +#define CLKID_DIV2_PRE 113
>> +#define CLKID_24M_DIV2 115
>> +#define CLKID_GEN_SEL 116
>> +#define CLKID_GEN_DIV 117
>> +#define CLKID_SARADC_DIV 120
>> +#define CLKID_PWM_A_SEL 122
>> +#define CLKID_PWM_A_DIV 123
>> +#define CLKID_PWM_B_SEL 125
>> +#define CLKID_PWM_B_DIV 126
>> +#define CLKID_PWM_C_SEL 128
>> +#define CLKID_PWM_C_DIV 129
>> +#define CLKID_PWM_D_SEL 131
>> +#define CLKID_PWM_D_DIV 132
>> +#define CLKID_PWM_E_SEL 134
>> +#define CLKID_PWM_E_DIV 135
>> +#define CLKID_PWM_F_SEL 137
>> +#define CLKID_PWM_F_DIV 138
>> +#define CLKID_SPICC_SEL 140
>> +#define CLKID_SPICC_DIV 141
>> +#define CLKID_SPICC_SEL2 142
>> +#define CLKID_TS_DIV 144
>> +#define CLKID_SPIFC_SEL 146
>> +#define CLKID_SPIFC_DIV 147
>> +#define CLKID_SPIFC_SEL2 148
>> +#define CLKID_USB_BUS_SEL 150
>> +#define CLKID_USB_BUS_DIV 151
>> +#define CLKID_SD_EMMC_SEL 153
>> +#define CLKID_SD_EMMC_DIV 154
>> +#define CLKID_SD_EMMC_SEL2 155
>> +#define CLKID_PSRAM_SEL 157
>> +#define CLKID_PSRAM_DIV 158
>> +#define CLKID_PSRAM_SEL2 159
>> +#define CLKID_DMC_SEL 161
>> +#define CLKID_DMC_DIV 162
>> +#define CLKID_DMC_SEL2 163
>> +#define NR_CLKS 165
>> +
>> +#include <dt-bindings/clock/a1-clkc.h>
>> +
>> +#endif /* __A1_H */
>> --
>> 1.9.1
>
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet encryption
From: Pascal Van Leeuwen @ 2019-09-27 9:58 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jason A . Donenfeld, Catalin Marinas, Herbert Xu, Arnd Bergmann,
Ard Biesheuvel, Greg KH, Eric Biggers, Samuel Neves,
Linux Crypto Mailing List, Andy Lutomirski, Marc Zyngier,
Dan Carpenter, Will Deacon, David Miller, Linux ARM
In-Reply-To: <CAHk-=wjr1w7x9Rjre_ALnDLASYNjsEHxu6VJpk4eUwZXN0ntqw@mail.gmail.com>
> > That remark is just very stupid. The hardware ALREADY exists, and
> > more hardware is in the pipeline. Once this stuff is designed in, it
> > usually stays in for many years to come. And these are chips sold in
> > _serious_ quantities, to be used in things like wireless routers and
> > DSL, cable and FTTH modems, 5G base stations, etc. etc.
>
> Yes, I very much mentioned routers. I believe those can happen much
> more quickly.
>
> But I would very much hope that that is not the only situation where
> you'd see wireguard used.
>
Same here
> I'd want to see wireguard in an end-to-end situation from the very
> client hardware. So laptops, phones, desktops. Not the untrusted (to
> me) hw in between.
>
I don't see why the crypto HW would deserve any less trust than, say,
the CPU itself. I would say CPU's don't deserve that trust at the moment.
> > No, these are just the routers going into *everyone's* home. And 5G
> > basestations arriving at every other street corner. I wouldn't call
> > that rare, exactly.
>
> That's fine for a corporate tunnel between devices. Which is certainly
> one use case for wireguard.
>
> But if you want VPN for your own needs for security, you want it at
> the _client_. Not at the router box. So that case really does matter.
>
Personally, I would really like it in my router box so my CPU is free
to do useful work instead of boring crypto. And I know there's nothing
untrusted in between my client and the router box, so I don't need to
worry about security there. But hey, that's just me.
> And I really don't see the hardware happening in that space. So the
> bad crypto interfaces only make the client _worse_.
>
Fully agree. We don't focus on the client side with our HW anyway.
(but than there may be that router box in between that can help out)
> See?
>
> But on to the arguments that we actually agree on:
>
> > Hey, no argument there. I don't see any good reason why the key can't
> > be on the stack. I doubt any hardware would be able to DMA that as-is
> > directly, and in any case, key changes should be infrequent, so copying
> > it to some DMA buffer should not be a performance problem.
> > So maybe that's an area for improvement: allow that to be on the stack.
>
> It's not even just the stack. It's really that the crypto interfaces
> are *designed* so that you have to allocate things separately, and
> can't embed these things in your own data structures.
>
> And they are that way, because the crypto interfaces aren't actually
> about (just) hiding the hardware interface: they are about hiding
> _all_ the encryption details.
>
Well, that's the general idea of abstraction. It also allows for
swapping in any other cipher with minimal effort just _because_ the
details were hidden from the application. So it may cost you some
effort initially, but it may save you effort later.
> There's no way to say "hey, I know the crypto I use, I know the key
> size I have, I know the state size it needs, I can preallocate those
> AS PART of my own data structures".
>
> Because the interface is designed to be so "generic" that you simply
> can't do those things, they are all external allocations, which is
> inevitably slower when you don't have hardware.
>
Hmm, Ok, I see your point here. But most of those data structures
(like the key) should be allocated infrequently anyway, so you can
amortize that cost over _many_ crypto operations.
You _do_ realize that performing the key schedule for e.g. AES with
AES-NI also takes quite a lot of time? So you should keep your keys
alive and not reload them all the time anyway.
But I already agreed with you that there may be cases where you just
want to call the library function directly. Wireguard just isn't one
of those cases, IMHO.
> And you've shown that you don't care about that "don't have hardware"
> situation, and seem to think it's the only case that matters. That's
> your job, after all.
>
I don't recall putting it that strongly ... and I certainly never said
the HW acceleration thing is the _only_ case that matters. But it does
matter _significantly_ to me, for blatantly obvious reasons.
> But however much you try to claim otherwise, there's all these
> situations where the hardware just isn't there, and the crypto
> interface just forces nasty overhead for absolutely no good reason.
>
> > I already explained the reasons for _not_ doing direct calls above.
>
> And I've tried to explain how direct calls that do the synchronous
> thing efficiently would be possible, but then _if_ there is hardware,
> they can then fall back to an async interface.
>
OK, I did not fully get that latter part. I would be fine with such an
approach for use cases (i.e. fixed, known crypto) where that makes sense.
It would actually be better than calling the SW-only library directly
(which was my suggestion) as it would still allow HW acceleration as
an option ...
> > > So there is absolutely NO DOWNSIDE for hw accelerated crypto to just
> > > do it right, and use an interface like this:
> > >
> > > if (!chacha20poly1305_decrypt_sg(sg, sg, skb->len, NULL, 0,
> > > PACKET_CB(skb)->nonce, key->key,
> > > simd_context))
> > > return false;
> > >
> > Well, for one thing, a HW API should not expect the result to be
> > available when the function call returns. (if that's what you
> > mean here). That would just be WRONG.
>
> Right. But that also shouldn't mean that when you have synchronous
> hardware (ie CPU) you have to set everything up even though it will
> never be used.
>
> Put another way: even with hardware acceleration, the queuing
> interface should be a simple "do this" interface.
>
OK, I don't think we disagree there. I _like_ simple. As long as it
doesn't sacrifice functionality I care about.
> The current crypto interface is basically something that requires all
> the setup up-front, whether it's needed or not. And it forces those
> very inconvenient and slow external allocations.
>
But you should do the setup work (if by "setup" you mean things like
cipher allocation, key setup and request allocation) only _once_ in a
_long_ while. You can just keep using it for the lifetime of the
application (or key, for the key setup part).
If I look at my cipher fallback paths in the driver (the only places
where I actually get to _use_ the API from the "other" side), per
actual indivual request they _only_ do - the rest is all preallocated
earlier:
_set_callback()
_set_crypt()
_set_ad()
_encrypt() or _decrypt()
And now that I look at that, I think the _set_callback() could
move to the setup phase as it's always the same callback function.
Probably, in case of Wireguard, you could even move the _set_ad()
there as it's always zero and the crypto driver is not allowed
to overwrite it in the request struct anyway.
Also, I already agreed with you that _set_crypt(), _set_ad()
and _encrypt()/_decrypt() _could_ be conveniently wrapped into
one API call instead of 3 separate ones if we think that's worth it.
BUT ... actually ... I just looked at the actual _implementation_
and it turns out these are _inlineable_ functions defined in the
header file that _just_ write to some struct fields. So they
should not end up being function calls at all(!!).
_Only_ the _encrypt()/_decrypt() invocation will end up with a
true (indirect) function call.
So where are all those allocations you mention? Have you ever
actually _used_ the Crypto API for anything?
Yes, if you actually want to _queue_ requests you need to use one
request struct for every queued operation, but you could just
preallocate an array of them that you cycle through. No need to do
those allocations in the hot path.
So is your problem really with the API _itself_ or with incorrect/
inefficient _use_ of the API in some places?
> And I'm saying that causes problems, because it fundamentally means
> that you can't do a good job for the common CPU case, because you're
> paying all those costs even when you need absolutely none of them.
> Both at setup time, but also at run-time due to the extra indirection
> and cache misses etc.
>
There is some cost sure, but is it _significant_ for any use case that
_matters_? You started bringing up optimization rules, so how about
Amdahls law?
> > Again, HW acceleration does not depend on the indirection _at all_,
> > that's there for entirely different purposes I explained above.
> > HW acceleration _does_ depend greatly on a truly async ifc though.
>
> Can you realize that the world isn't just all hw acceleration?
>
Sure. But there's also a lot of HW acceleration _already out there_
that _could_ have been used if only the proper SW API's had existed.
Welcome to _my_ world.
> Can you admit that the current crypto interface is just horrid for the
> non-accelerated case?
>
Is agreeing that it is not perfect sufficient for you? :-)
> Can you perhaps then also think that "maybe there are better models".
>
Sure. There's always better. There's also good enough though ...
> > So queue requests on one side, handle results from the other side
> > in some callback func off of an interrupt handler.
>
> Actually, what you can do - and what people *have* done - is to admit
> that the synchronous case is real and important, and then design
> interfaces that work for that one too.
>
But they _do_ work for that case as well. I still haven't seen any
solid evidence that they are as horribly inefficient as you are
implying for _real life_ use cases. And even if they are, then there's
the question whether that is the fault of the API or incorrect use
thereof.
> You don't need to allocate resources ahead of time, and you don't have
> to disallow just having the state buffer allocated by the caller.
>
> So here's the *wrong* way to do it (and the way that crypto does it):
>
> - dynamically allocate buffers at "init time"
>
Why is that so "wrong"? It sure beats doing allocations on the hot path.
But yes, some stuff should be allowed to live on the stack. Some other
stuf can't be on the stack though, as that's gone when the calling
function exits while the background crypto processing still needs it.
And you don't want to have it on the stack initially and then have
to _copy_ it to some DMA-able location that you allocate on the fly
on the hot path if you _do_ want HW acceleration.
> - fill in "callback fields" etc before starting the crypto, whether
> they are needed or not
>
I think this can be done _once_ at request allocation time.
But it's just one function pointer write anyway. Is that significant?
Or: _if_ that is significant, you shouldn't be using the Crypto API for
that use case in the first place.
> - call a "decrypt" function that then uses the indirect functions you
> set up at init time, and possibly waits for it (or calls the callbacks
> you set up)
>
> note how it's all this "state machine" model where you add data to the
> state machine, and at some point you say "execute" and then either you
> wait for things or you get callbacks.
>
Not sure how splitting data setup over a few seperate "function" calls
suddenly makes it a "state machine model" ...
But yes, I can understand why the completion handling through this
callback function seems like unnecessary complication for the SW case.
> That makes sense for a hw crypto engine. It's how a lot of them work, after all.
>
Oh really?
Can't speak for other people stuff, but for our hardware you post a
request to it and then go off do other stuff while the HW does its thing
after which it will inform you it's done by means of an interrupt.
I don't see how this relates to the "statemachine model" above, there
is no persistent state involved, it's all included in the request.
The _only_ thing that matters is that you realize it's a pipeline that
needs to be kept filled and has latency >> throughput, just like your
CPU pipeline.
> But it makes _zero_ sense for the synchronous case. You did a lot of
> extra work for that case, and because it was all a styate machine, you
> did it particularly inefficiently: not only do you have those separate
> allocations with pointer following, the "decrypt()" call ends up doing
> an indirect call to the CPU implementation, which is just quite slow
> to begin with, particularly in this day and age with retpoline etc.
>
> So what's the alternative?
>
> I claim that a good interface would accept that "Oh, a lot of cases
> will be synchronous, and a lot of cases use one fixed
> encryption/decryption model".
>
> And it's quite doable. Instead of having those callback fields and
> indirection etc, you could have something more akin to this:
>
> - let the caller know what the state size is and allocate the
> synchronous state in its own data structures
>
> - let the caller just call a static "decrypt_xyz()" function for xyz
> decryptrion.
>
Fine for those few cases where the algorithm is known and fixed.
(You do realize that the primary use cases are IPsec, dmcrypt and
fscrypt where that is most definitely _not_ the case?)
Also, you're still ignoring the fact that there is not one, single,
optimal, CPU implementation either. You have to select that as well,
based on CPU features. So it's either an indirect function call that
would be well predictable - as it's always the same at that point in
the program - or it's a deep if-else tree (which might actually be
implemented by the compiler as an indirect (table) jump ...)
selecting the fastest implementation, either SW _or_ HW.
> - if you end up doing it synchronously, that function just returns
> "done". No overhead. No extra allocations. No unnecessary stuff. Just
> do it, using the buffers provided. End of story. Efficient and simple.
>
I don't see which "extra allocations" you would be saving here.
Those shouldn't happen in the hot path either way.
> - BUT.
>
> - any hardware could have registered itself for "I can do xyz", and
> the decrypt_xyz() function would know about those, and *if* it has a
> list of accelerators (hopefully sorted by preference etc), it would
> try to use them. And if they take the job (they might not - maybe
> their queues are full, maybe they don't have room for new keys at the
> moment, which might be a separate setup from the queues), the
> "decrypt_xyz()" function returns a _cookie_ for that job. It's
> probably a pre-allocated one (the hw accelerator might preallocate a
> fixed number of in-progress data structures).
>
> And once you have that cookie, and you see "ok, I didn't get the
> answer immediately" only THEN do you start filling in things like
> callback stuff, or maybe you set up a wait-queue and start waiting for
> it, or whatever".
>
I don't see the point of saving that single callback pointer write.
I mean, it's just _one_ CPU word memory write. Likely to the L1 cache.
But I can see the appeal of getting a "done" response on the _encrypt()/
_decrypt() call and then being able to immediately continue processing
the result data and having the async response handling separated off.
I think it should actually be possible to change the API to work like
that without breaking backward compatibility, i.e. define some flag
specifying you actually _want_ this behavior and then define some
return code that says "I'm done processing, carry on please".
> See the difference in models? One forces that asynchronous model, and
> actively penalizes the synchronous one.
>
> The other _allows_ an asynchronous model, but is fine with a synchronous one.
>
> > > aead_request_set_callback(req, 0, NULL, NULL);
> > >
> > This is just inevitable for HW acceration ...
>
> See above. It really isn't. You could do it *after* the fact,
>
Before ... after ... the point was you need it. And it's a totally
insignificant saving anyway.
> when
> you've gotten that ticket from the hardware. Then you say "ok, if the
> ticket is done, use these callbacks". Or "I'll now wait for this
> ticket to be done" (which is what the above does by setting the
> callbacks to zero).
>
> Wouldn't that be lovely for a user?
>
Yes and no.
Because the user would _still_ need to handle the case of callbacks.
In case the request _does_ go to the HW accelerator.
So you keep the main processing path clean I suppose, saving some
cycles there, but you still have this case of callbacks and having
multiple requests queued you need to handle as well. Which now
becomes a separate _exception_ case. You now have two distinct
processing paths you have to manage from your application.
How is that an _improvement_ for the user? (not withstanding that
it may be an improvement to SW only performance)
> I suspect it would be a nice model for a hw accelerator too. If you
> have full queues or have problems allocating new memory or whatever,
> you just let the code fall back to the synchronous interface.
>
HW drivers typically _do_ use SW fallback for cases they cannot
handle. Actually, that works very nicely with the current API,
with the fallback cipher just being attached to the original
requests' callback function ... i.e. just do a tail call to
the fallback cipher request.
> > Trust me, I have whole list of things I don't like about the
> > API myself, it's not exacty ideal for HW acceleration either.
>
> That';s the thing. It's actively detrimental for "I have no HW acceleration".
>
You keep asserting that with no evidence whatsoeever.
> And apparently it's not optimal for you guys either.
>
True, but I accept the fact that it needs to be that way because some
_other_ HW may drive that requirement. I accept the fact that I'm not
alone in the world.
> > But the point is - there are those case where you _don't_ know and
> > _that_ is what the Crypto API is for. And just generally, crypto
> > really _should_ be switchable.
>
> It's very much not what wireguard does.
>
And that's very much a part of Wireguard that is _broken_. I like
Wireguard for a lot of things, but it's single cipher focus is not
one of them. Especially since all crypto it uses comes from a single
source (DJB), which is frowned upon in the industry.
Crypto agility is a very important _security_ feature and the whole
argument Jason makes that it is actually a weakness is _bullshit_.
(Just because SSL _implemented_ this horribly wrong doesn't mean
it's a bad thing to do - it's not, it's actually _necessary_. As
the alternative would be to either continue using broken crypto
or wait _months_ for a new implementation to reach your devices
when the crypto gets broken somehow. Not good.)
> And honestly, most of the switchable ones have caused way more
> security problems than they have "fixed" by being switchable.
>
"most of the switchable ones"
You mean _just_ SSL/TLS. SSL/TLS before 1.3 just sucked security
wise, on so many levels. That has _nothing_ to do with the very
desirable feature of crypto agility. It _can_ be done properly and
securely. (for one thing, it does not _need_ to be negotiable)
> Linus
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it arch-independent.
From: Jianyong Wu (Arm Technology China) @ 2019-09-27 10:10 UTC (permalink / raw)
To: Suzuki Poulose, netdev@vger.kernel.org, yangbo.lu@nxp.com,
john.stultz@linaro.org, tglx@linutronix.de, pbonzini@redhat.com,
sean.j.christopherson@intel.com, maz@kernel.org,
richardcochran@gmail.com, Mark Rutland, Will Deacon
Cc: Justin He (Arm Technology China), kvm@vger.kernel.org,
Steve Capper, linux-kernel@vger.kernel.org,
Kaly Xin (Arm Technology China), nd, kvmarm@lists.cs.columbia.edu,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <2f338b57-b0b2-e439-6089-72e5f5e4f017@arm.com>
Hi Suzuki,
> -----Original Message-----
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> Sent: Thursday, September 26, 2019 9:06 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> maz@kernel.org; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
> <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it
> arch-independent.
>
> Hi Jianyong,
>
> On 26/09/2019 12:42, Jianyong Wu wrote:
> > Currently, ptp_kvm modules implementation is only for x86 which
> > includs large part of arch-specific code. This patch move all of
> > those code into new arch related file in the same directory.
> >
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > ---
> > drivers/ptp/Makefile | 1 +
> > drivers/ptp/{ptp_kvm.c => kvm_ptp.c} | 77 ++++++------------------
> > drivers/ptp/ptp_kvm_x86.c | 87
> ++++++++++++++++++++++++++++
> > include/asm-generic/ptp_kvm.h | 12 ++++
> > 4 files changed, 118 insertions(+), 59 deletions(-)
> > rename drivers/ptp/{ptp_kvm.c => kvm_ptp.c} (63%)
>
> minor nit: Could we not skip renaming the file ? Given you are following the
> ptp_kvm_* for the arch specific files and the header files, wouldn't it be
> good to keep ptp_kvm.c ?
>
If the module name ptp_kvm.ko is the same with its dependent object file ptp_kvm.o, warning will be given by compiler,
So I change the ptp_kvm.c to kvm_ptp.c to avoid that conflict.
Thanks
Jianyong Wu
> Rest looks fine.
>
> Cheers
> Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet encryption
From: Herbert Xu @ 2019-09-27 10:11 UTC (permalink / raw)
To: Pascal Van Leeuwen
Cc: Jason A . Donenfeld, Catalin Marinas, Arnd Bergmann,
Ard Biesheuvel, Greg KH, Eric Biggers, Samuel Neves, Will Deacon,
Linux Crypto Mailing List, Andy Lutomirski, Marc Zyngier,
Dan Carpenter, Linus Torvalds, David Miller, Linux ARM
In-Reply-To: <MN2PR20MB2973A696B92A8C5A74A738F1CA810@MN2PR20MB2973.namprd20.prod.outlook.com>
On Fri, Sep 27, 2019 at 09:58:14AM +0000, Pascal Van Leeuwen wrote:
>
> But I can see the appeal of getting a "done" response on the _encrypt()/
> _decrypt() call and then being able to immediately continue processing
> the result data and having the async response handling separated off.
This is how it works today. If your request can be fulfilled
right away, you will get a return value other than EINPROGRESS
and you just carry on, the completion callback never happens in
this case.
aesni-intel makes heavy use of this. In most cases it is sync.
It only goes async when the FPU is not available.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet encryption
From: Pascal Van Leeuwen @ 2019-09-27 10:11 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jason A . Donenfeld, Catalin Marinas, Herbert Xu, Arnd Bergmann,
Ard Biesheuvel, Greg KH, Eric Biggers, Samuel Neves,
Linux Crypto Mailing List, Andy Lutomirski, Marc Zyngier,
Dan Carpenter, Will Deacon, David Miller, Linux ARM
In-Reply-To: <CAHk-=wgu5-Wj=UY+iU+x=RcKN_ceUsKdfhsv2-E5TNocELU8Ag@mail.gmail.com>
> -----Original Message-----
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Sent: Friday, September 27, 2019 4:06 AM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Linux Crypto Mailing List <linux-
> crypto@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>; Herbert Xu
> <herbert@gondor.apana.org.au>; David Miller <davem@davemloft.net>; Greg KH
> <gregkh@linuxfoundation.org>; Jason A . Donenfeld <Jason@zx2c4.com>; Samuel Neves
> <sneves@dei.uc.pt>; Dan Carpenter <dan.carpenter@oracle.com>; Arnd Bergmann
> <arnd@arndb.de>; Eric Biggers <ebiggers@google.com>; Andy Lutomirski <luto@kernel.org>;
> Will Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Catalin Marinas
> <catalin.marinas@arm.com>
> Subject: Re: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet
> encryption
>
> On Thu, Sep 26, 2019 at 5:15 PM Pascal Van Leeuwen
> <pvanleeuwen@verimatrix.com> wrote:
> >
> > But even the CPU only thing may have several implementations, of which
> > you want to select the fastest one supported by the _detected_ CPU
> > features (i.e. SSE, AES-NI, AVX, AVX512, NEON, etc. etc.)
> > Do you think this would still be efficient if that would be some
> > large if-else tree? Also, such a fixed implementation wouldn't scale.
>
> Just a note on this part.
>
> Yes, with retpoline a large if-else tree is actually *way* better for
> performance these days than even just one single indirect call. I
> think the cross-over point is somewhere around 20 if-statements.
>
Yikes, that is just _horrible_ :-(
_However_ there's many CPU architectures out there that _don't_ need
the retpoline mitigation and would be unfairly penalized by the deep
if-else tree (as opposed to the indirect branch) for a problem they
did not cause in the first place.
Wouldn't it be more fair to impose the penalty on the CPU's actually
_causing_ this problem? Also because those are generally the more
powerful CPU's anyway, that would suffer the least from the overhead?
> But those kinds of things also are things that we already handle well
> with instruction rewriting, so they can actually have even less of an
> overhead than a conditional branch. Using code like
>
> if (static_cpu_has(X86_FEATURE_AVX2))
>
> actually ends up patching the code at run-time, so you end up having
> just an unconditional branch. Exactly because CPU feature choices
> often end up being in critical code-paths where you have
> one-or-the-other kind of setup.
>
> And yes, one of the big users of this is very much the crypto library code.
>
Ok, I didn't know about that. So I suppose we could have something
like if (static_soc_has(HW_CRYPTO_ACCELERATOR_XYZ)) ... Hmmm ...
> The code to do the above is disgusting, and when you look at the
> generated code you see odd unreachable jumps and what looks like a
> slow "bts" instruction that does the testing dynamically.
>
> And then the kernel instruction stream gets rewritten fairly early
> during the boot depending on the actual CPU capabilities, and the
> dynamic tests get overwritten by a direct jump.
>
> Admittedly I don't think the arm64 people go to quite those lengths,
> but it certainly wouldn't be impossible there either. It just takes a
> bit of architecture knowledge and a strong stomach ;)
>
> Linus
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it arch-independent.
From: Suzuki K Poulose @ 2019-09-27 10:12 UTC (permalink / raw)
To: Jianyong Wu (Arm Technology China), netdev@vger.kernel.org,
yangbo.lu@nxp.com, john.stultz@linaro.org, tglx@linutronix.de,
pbonzini@redhat.com, sean.j.christopherson@intel.com,
maz@kernel.org, richardcochran@gmail.com, Mark Rutland,
Will Deacon
Cc: Justin He (Arm Technology China), kvm@vger.kernel.org,
Steve Capper, linux-kernel@vger.kernel.org,
Kaly Xin (Arm Technology China), nd, kvmarm@lists.cs.columbia.edu,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <HE1PR0801MB167630F7B983A7F9DBB473DFF4810@HE1PR0801MB1676.eurprd08.prod.outlook.com>
On 27/09/2019 11:10, Jianyong Wu (Arm Technology China) wrote:
> Hi Suzuki,
>
>> -----Original Message-----
>> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Sent: Thursday, September 26, 2019 9:06 PM
>> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
>> netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
>> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
>> maz@kernel.org; richardcochran@gmail.com; Mark Rutland
>> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>
>> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
>> <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
>> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
>> <Justin.He@arm.com>; nd <nd@arm.com>
>> Subject: Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it
>> arch-independent.
>>
>> Hi Jianyong,
>>
>> On 26/09/2019 12:42, Jianyong Wu wrote:
>>> Currently, ptp_kvm modules implementation is only for x86 which
>>> includs large part of arch-specific code. This patch move all of
>>> those code into new arch related file in the same directory.
>>>
>>> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
>>> ---
>>> drivers/ptp/Makefile | 1 +
>>> drivers/ptp/{ptp_kvm.c => kvm_ptp.c} | 77 ++++++------------------
>>> drivers/ptp/ptp_kvm_x86.c | 87
>> ++++++++++++++++++++++++++++
>>> include/asm-generic/ptp_kvm.h | 12 ++++
>>> 4 files changed, 118 insertions(+), 59 deletions(-)
>>> rename drivers/ptp/{ptp_kvm.c => kvm_ptp.c} (63%)
>>
>> minor nit: Could we not skip renaming the file ? Given you are following the
>> ptp_kvm_* for the arch specific files and the header files, wouldn't it be
>> good to keep ptp_kvm.c ?
>>
> If the module name ptp_kvm.ko is the same with its dependent object file ptp_kvm.o, warning will be given by compiler,
> So I change the ptp_kvm.c to kvm_ptp.c to avoid that conflict.
Hmm, ok. How about ptp_kvm_common.c instead of kvm_ptp.c ?
Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it arch-independent.
From: Jianyong Wu (Arm Technology China) @ 2019-09-27 10:14 UTC (permalink / raw)
To: Suzuki Poulose, netdev@vger.kernel.org, yangbo.lu@nxp.com,
john.stultz@linaro.org, tglx@linutronix.de, pbonzini@redhat.com,
sean.j.christopherson@intel.com, maz@kernel.org,
richardcochran@gmail.com, Mark Rutland, Will Deacon
Cc: Justin He (Arm Technology China), kvm@vger.kernel.org,
Steve Capper, linux-kernel@vger.kernel.org,
Kaly Xin (Arm Technology China), nd, kvmarm@lists.cs.columbia.edu,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <4337dcf0-bd60-4db8-6c9f-cd718b89d2a4@arm.com>
Hi Suzuki,
> -----Original Message-----
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> Sent: Friday, September 27, 2019 6:12 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> maz@kernel.org; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
> <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it
> arch-independent.
>
>
>
> On 27/09/2019 11:10, Jianyong Wu (Arm Technology China) wrote:
> > Hi Suzuki,
> >
> >> -----Original Message-----
> >> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> >> Sent: Thursday, September 26, 2019 9:06 PM
> >> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> >> netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> >> tglx@linutronix.de; pbonzini@redhat.com;
> >> sean.j.christopherson@intel.com; maz@kernel.org;
> >> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
> Will
> >> Deacon <Will.Deacon@arm.com>
> >> Cc: linux-kernel@vger.kernel.org;
> >> linux-arm-kernel@lists.infradead.org;
> >> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
> >> <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> >> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> >> <Justin.He@arm.com>; nd <nd@arm.com>
> >> Subject: Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to
> >> make it arch-independent.
> >>
> >> Hi Jianyong,
> >>
> >> On 26/09/2019 12:42, Jianyong Wu wrote:
> >>> Currently, ptp_kvm modules implementation is only for x86 which
> >>> includs large part of arch-specific code. This patch move all of
> >>> those code into new arch related file in the same directory.
> >>>
> >>> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> >>> ---
> >>> drivers/ptp/Makefile | 1 +
> >>> drivers/ptp/{ptp_kvm.c => kvm_ptp.c} | 77 ++++++------------------
> >>> drivers/ptp/ptp_kvm_x86.c | 87
> >> ++++++++++++++++++++++++++++
> >>> include/asm-generic/ptp_kvm.h | 12 ++++
> >>> 4 files changed, 118 insertions(+), 59 deletions(-)
> >>> rename drivers/ptp/{ptp_kvm.c => kvm_ptp.c} (63%)
> >>
> >> minor nit: Could we not skip renaming the file ? Given you are
> >> following the
> >> ptp_kvm_* for the arch specific files and the header files, wouldn't
> >> it be good to keep ptp_kvm.c ?
> >>
> > If the module name ptp_kvm.ko is the same with its dependent object
> > file ptp_kvm.o, warning will be given by compiler, So I change the
> ptp_kvm.c to kvm_ptp.c to avoid that conflict.
>
> Hmm, ok. How about ptp_kvm_common.c instead of kvm_ptp.c ?
Yeah, it's a better name, I will change it next version.
Thanks
Jianyong Wu
>
> Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it arch-independent.
From: Suzuki K Poulose @ 2019-09-27 10:23 UTC (permalink / raw)
To: Jianyong Wu, netdev, yangbo.lu, john.stultz, tglx, pbonzini,
sean.j.christopherson, maz, richardcochran, Mark.Rutland,
Will.Deacon
Cc: justin.he, kvm, Steve.Capper, linux-kernel, Kaly.Xin, nd, kvmarm,
linux-arm-kernel
In-Reply-To: <20190926114212.5322-3-jianyong.wu@arm.com>
On 26/09/2019 12:42, Jianyong Wu wrote:
> Currently, ptp_kvm modules implementation is only for x86 which includs
> large part of arch-specific code. This patch move all of those code
> into new arch related file in the same directory.
>
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
...
> +int kvm_arch_ptp_get_clock_fn(unsigned long *cycle, struct timespec64 *tspec,
> + struct clocksource **cs)
> diff --git a/include/asm-generic/ptp_kvm.h b/include/asm-generic/ptp_kvm.h
> new file mode 100644
> index 000000000000..208e842bfa64
> --- /dev/null
> +++ b/include/asm-generic/ptp_kvm.h
> +int kvm_arch_ptp_get_clock_fn(long *cycle,
> + struct timespec64 *tspec, void *cs);
>
Conflicting types for kvm_arch_ptp_get_clock_fn() ?
Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v2 3/4] dt-bindings: medaitek: mt8183: add property "resets" && "reset-names"
From: Jiaxin Yu @ 2019-09-27 10:31 UTC (permalink / raw)
To: broonie, mark.rutland, robh+dt, linux, wim
Cc: alsa-devel, yong.liang, lgirdwood, jiaxin.yu, perex, tzungbi,
linux-mediatek, eason.yen, linux-arm-kernel
In-Reply-To: <1569580317-21181-1-git-send-email-jiaxin.yu@mediatek.com>
This patch add property "resets" && "reset-names" in examples so that we can
use reset controller to reset audio domain regs.
Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
---
Documentation/devicetree/bindings/sound/mt8183-afe-pcm.txt | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/mt8183-afe-pcm.txt b/Documentation/devicetree/bindings/sound/mt8183-afe-pcm.txt
index 396ba38619f6..1f1cba4152ce 100644
--- a/Documentation/devicetree/bindings/sound/mt8183-afe-pcm.txt
+++ b/Documentation/devicetree/bindings/sound/mt8183-afe-pcm.txt
@@ -4,6 +4,10 @@ Required properties:
- compatible = "mediatek,mt68183-audio";
- reg: register location and size
- interrupts: should contain AFE interrupt
+- resets: Must contain an entry for each entry in reset-names
+ See ../reset/reset.txt for details.
+- reset-names: should have these reset names:
+ "audiosys";
- power-domains: should define the power domain
- clocks: Must contain an entry for each entry in clock-names
- clock-names: should have these clock names:
@@ -20,6 +24,8 @@ Example:
compatible = "mediatek,mt8183-audio";
reg = <0 0x11220000 0 0x1000>;
interrupts = <GIC_SPI 161 IRQ_TYPE_LEVEL_LOW>;
+ resets = <&watchdog MT8183_TOPRGU_AUDIO_SW_RST>;
+ reset-names = "audiosys";
power-domains = <&scpsys MT8183_POWER_DOMAIN_AUDIO>;
clocks = <&infrasys CLK_INFRA_AUDIO>,
<&infrasys CLK_INFRA_AUDIO_26M_BCLK>,
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v2 0/4] ASoC: mt8183: fix audio playback slowly after playback
From: Jiaxin Yu @ 2019-09-27 10:31 UTC (permalink / raw)
To: broonie, mark.rutland, robh+dt, linux, wim
Cc: alsa-devel, yong.liang, lgirdwood, jiaxin.yu, perex, tzungbi,
linux-mediatek, eason.yen, linux-arm-kernel
This series patches add reset controller for MT8183, and audio will use it in
machine driver during bootup, they depend on the following patch:
1. this series add support reset-controller in infra
[v5,2/2] clk: reset: Modify reset-controller driver
https://patchwork.kernel.org/patch/11060419/
v2 changes:
1. remove "WIP" that in the title of patches
2. add hyper link for the patch that depends on
Jiaxin Yu (2):
dt-bindings: medaitek: mt8183: add property "resets" && "reset-names"
ASoC: mt8183: fix audio playback slowly after playback during bootup
yong.liang (2):
dt-bindings: mediatek: mt8183: Add #reset-cells
watchdog: mtk_wdt: mt8183: Add reset controller
.../bindings/sound/mt8183-afe-pcm.txt | 6 +
.../devicetree/bindings/watchdog/mtk-wdt.txt | 9 +-
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/mtk_wdt.c | 110 +++++++++++++++++-
.../reset-controller/mt8183-resets.h | 13 +++
sound/soc/mediatek/mt8183/mt8183-afe-pcm.c | 15 +++
6 files changed, 150 insertions(+), 4 deletions(-)
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v2 1/4] dt-bindings: mediatek: mt8183: Add #reset-cells
From: Jiaxin Yu @ 2019-09-27 10:31 UTC (permalink / raw)
To: broonie, mark.rutland, robh+dt, linux, wim
Cc: alsa-devel, yong.liang, yong.liang, lgirdwood, jiaxin.yu, perex,
tzungbi, linux-mediatek, eason.yen, linux-arm-kernel
In-Reply-To: <1569580317-21181-1-git-send-email-jiaxin.yu@mediatek.com>
From: "yong.liang" <yong.liang@mediatek.corp-partner.google.com>
Add #reset-cells property and update example
Signed-off-by: yong.liang <yong.liang@mediatek.corp-partner.google.com>
---
.../devicetree/bindings/watchdog/mtk-wdt.txt | 9 ++++++---
.../dt-bindings/reset-controller/mt8183-resets.h | 13 +++++++++++++
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
index 3ee625d0812f..ecb9ff784832 100644
--- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
@@ -16,11 +16,14 @@ Required properties:
Optional properties:
- timeout-sec: contains the watchdog timeout in seconds.
+- #reset-cells: Should be 1.
Example:
-wdt: watchdog@10000000 {
- compatible = "mediatek,mt6589-wdt";
- reg = <0x10000000 0x18>;
+watchdog: watchdog@10007000 {
+ compatible = "mediatek,mt8183-wdt",
+ "mediatek,mt6589-wdt";
+ reg = <0 0x10007000 0 0x100>;
timeout-sec = <10>;
+ #reset-cells = <1>;
};
diff --git a/include/dt-bindings/reset-controller/mt8183-resets.h b/include/dt-bindings/reset-controller/mt8183-resets.h
index 8804e34ebdd4..47dadcf3fd24 100644
--- a/include/dt-bindings/reset-controller/mt8183-resets.h
+++ b/include/dt-bindings/reset-controller/mt8183-resets.h
@@ -78,4 +78,17 @@
#define MT8183_INFRACFG_AO_I2C7_SW_RST 126
#define MT8183_INFRACFG_AO_I2C8_SW_RST 127
+#define MT8183_TOPRGU_MM_SW_RST 1
+#define MT8183_TOPRGU_MFG_SW_RST 2
+#define MT8183_TOPRGU_VENC_SW_RST 3
+#define MT8183_TOPRGU_VDEC_SW_RST 4
+#define MT8183_TOPRGU_IMG_SW_RST 5
+#define MT8183_TOPRGU_MD_SW_RST 7
+#define MT8183_TOPRGU_CONN_SW_RST 9
+#define MT8183_TOPRGU_CONN_MCU_SW_RST 12
+#define MT8183_TOPRGU_IPU0_SW_RST 14
+#define MT8183_TOPRGU_IPU1_SW_RST 15
+#define MT8183_TOPRGU_AUDIO_SW_RST 17
+#define MT8183_TOPRGU_CAMSYS_SW_RST 18
+
#endif /* _DT_BINDINGS_RESET_CONTROLLER_MT8183 */
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v2 2/4] watchdog: mtk_wdt: mt8183: Add reset controller
From: Jiaxin Yu @ 2019-09-27 10:31 UTC (permalink / raw)
To: broonie, mark.rutland, robh+dt, linux, wim
Cc: alsa-devel, yong.liang, lgirdwood, jiaxin.yu, perex, tzungbi,
linux-mediatek, eason.yen, linux-arm-kernel
In-Reply-To: <1569580317-21181-1-git-send-email-jiaxin.yu@mediatek.com>
From: "yong.liang" <yong.liang@mediatek.com>
Provide assert/deassert/reset API in watchdog driver.
Register reset controller for toprgu device in watchdog probe.
Signed-off-by: yong.liang <yong.liang@mediatek.com>
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/mtk_wdt.c | 110 ++++++++++++++++++++++++++++++++++++-
2 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 2e07caab9db2..629249fe5305 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -717,6 +717,7 @@ config MEDIATEK_WATCHDOG
tristate "Mediatek SoCs watchdog support"
depends on ARCH_MEDIATEK || COMPILE_TEST
select WATCHDOG_CORE
+ select RESET_CONTROLLER
help
Say Y here to include support for the watchdog timer
in Mediatek SoCs.
diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
index 9c3d0033260d..660fb0e48d8e 100644
--- a/drivers/watchdog/mtk_wdt.c
+++ b/drivers/watchdog/mtk_wdt.c
@@ -20,6 +20,10 @@
#include <linux/types.h>
#include <linux/watchdog.h>
#include <linux/delay.h>
+#include <linux/reset-controller.h>
+#include <linux/slab.h>
+#include <linux/reset.h>
+#include <linux/of_device.h>
#define WDT_MAX_TIMEOUT 31
#define WDT_MIN_TIMEOUT 1
@@ -44,17 +48,113 @@
#define WDT_SWRST 0x14
#define WDT_SWRST_KEY 0x1209
+#define WDT_SWSYSRST 0x18U
+#define WDT_SWSYS_RST_KEY 0x88000000
+
#define DRV_NAME "mtk-wdt"
#define DRV_VERSION "1.0"
static bool nowayout = WATCHDOG_NOWAYOUT;
static unsigned int timeout;
+struct toprgu_reset {
+ spinlock_t lock; /* Protects reset_controller access */
+ void __iomem *toprgu_swrst_base;
+ int regofs;
+ struct reset_controller_dev rcdev;
+};
+
struct mtk_wdt_dev {
struct watchdog_device wdt_dev;
void __iomem *wdt_base;
+ struct toprgu_reset reset_controller;
+ const struct mtk_wdt_compatible *dev_comp;
+};
+
+struct mtk_wdt_compatible {
+ int sw_rst_num;
+};
+
+static int toprgu_reset_assert(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ unsigned int tmp;
+ unsigned long flags;
+ struct toprgu_reset *data = container_of(rcdev,
+ struct toprgu_reset, rcdev);
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ tmp = __raw_readl(data->toprgu_swrst_base + data->regofs);
+ tmp |= BIT(id);
+ tmp |= WDT_SWSYS_RST_KEY;
+ writel(tmp, data->toprgu_swrst_base + data->regofs);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+
+ return 0;
+}
+
+static int toprgu_reset_deassert(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ unsigned int tmp;
+ unsigned long flags;
+ struct toprgu_reset *data = container_of(rcdev,
+ struct toprgu_reset, rcdev);
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ tmp = __raw_readl(data->toprgu_swrst_base + data->regofs);
+ tmp &= ~BIT(id);
+ tmp |= WDT_SWSYS_RST_KEY;
+ writel(tmp, data->toprgu_swrst_base + data->regofs);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+
+ return 0;
+}
+
+static int toprgu_reset(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ int ret;
+
+ ret = toprgu_reset_assert(rcdev, id);
+ if (ret)
+ return ret;
+
+ return toprgu_reset_deassert(rcdev, id);
+}
+
+static struct reset_control_ops toprgu_reset_ops = {
+ .assert = toprgu_reset_assert,
+ .deassert = toprgu_reset_deassert,
+ .reset = toprgu_reset,
};
+static void toprgu_register_reset_controller(struct platform_device *pdev,
+ int regofs)
+{
+ int ret;
+ struct mtk_wdt_dev *mtk_wdt = platform_get_drvdata(pdev);
+
+ spin_lock_init(&mtk_wdt->reset_controller.lock);
+
+ mtk_wdt->dev_comp = of_device_get_match_data(&pdev->dev);
+ mtk_wdt->reset_controller.toprgu_swrst_base = mtk_wdt->wdt_base;
+ mtk_wdt->reset_controller.regofs = regofs;
+ mtk_wdt->reset_controller.rcdev.owner = THIS_MODULE;
+ mtk_wdt->reset_controller.rcdev.nr_resets =
+ mtk_wdt->dev_comp->sw_rst_num;
+ mtk_wdt->reset_controller.rcdev.ops = &toprgu_reset_ops;
+ mtk_wdt->reset_controller.rcdev.of_node = pdev->dev.of_node;
+ ret = reset_controller_register(&mtk_wdt->reset_controller.rcdev);
+ if (ret != 0)
+ dev_err(&pdev->dev,
+ "couldn't register wdt reset controller: %d\n", ret);
+}
+
static int mtk_wdt_restart(struct watchdog_device *wdt_dev,
unsigned long action, void *data)
{
@@ -187,9 +287,12 @@ static int mtk_wdt_probe(struct platform_device *pdev)
if (unlikely(err))
return err;
- dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
+ dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
mtk_wdt->wdt_dev.timeout, nowayout);
+ mtk_wdt->dev_comp = of_device_get_match_data(&pdev->dev);
+ if (mtk_wdt->dev_comp)
+ toprgu_register_reset_controller(pdev, WDT_SWSYSRST);
return 0;
}
@@ -217,7 +320,12 @@ static int mtk_wdt_resume(struct device *dev)
}
#endif
+static const struct mtk_wdt_compatible mt8183_compat = {
+ .sw_rst_num = 18,
+};
+
static const struct of_device_id mtk_wdt_dt_ids[] = {
+ { .compatible = "mediatek,mt8183-wdt", .data = &mt8183_compat },
{ .compatible = "mediatek,mt6589-wdt" },
{ /* sentinel */ }
};
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v2 4/4] ASoC: mt8183: fix audio playback slowly after playback during bootup
From: Jiaxin Yu @ 2019-09-27 10:31 UTC (permalink / raw)
To: broonie, mark.rutland, robh+dt, linux, wim
Cc: alsa-devel, yong.liang, lgirdwood, jiaxin.yu, perex, tzungbi,
linux-mediatek, eason.yen, linux-arm-kernel
In-Reply-To: <1569580317-21181-1-git-send-email-jiaxin.yu@mediatek.com>
Before regmap_reinit_cache we must reset audio regs as default values.
So we use reset controller unit(toprgu) to reset audio hw.
Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
---
sound/soc/mediatek/mt8183/mt8183-afe-pcm.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/sound/soc/mediatek/mt8183/mt8183-afe-pcm.c b/sound/soc/mediatek/mt8183/mt8183-afe-pcm.c
index 4a31106d3471..721632386a50 100644
--- a/sound/soc/mediatek/mt8183/mt8183-afe-pcm.c
+++ b/sound/soc/mediatek/mt8183/mt8183-afe-pcm.c
@@ -11,6 +11,7 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/pm_runtime.h>
+#include <linux/reset.h>
#include "mt8183-afe-common.h"
#include "mt8183-afe-clk.h"
@@ -1089,6 +1090,7 @@ static int mt8183_afe_pcm_dev_probe(struct platform_device *pdev)
struct mtk_base_afe *afe;
struct mt8183_afe_private *afe_priv;
struct device *dev;
+ struct reset_control *rstc;
int i, irq_id, ret;
afe = devm_kzalloc(&pdev->dev, sizeof(*afe), GFP_KERNEL);
@@ -1126,6 +1128,19 @@ static int mt8183_afe_pcm_dev_probe(struct platform_device *pdev)
return ret;
}
+ rstc = devm_reset_control_get(dev, "audiosys");
+ if (IS_ERR(rstc)) {
+ ret = PTR_ERR(rstc);
+ dev_err(dev, "could not get audiosys reset:%d\n", ret);
+ return ret;
+ }
+
+ ret = reset_control_reset(rstc);
+ if (ret) {
+ dev_err(dev, "failed to trigger audio reset:%d\n", ret);
+ return ret;
+ }
+
/* enable clock for regcache get default value from hw */
afe_priv->pm_runtime_bypass_reg_ctl = true;
pm_runtime_get_sync(&pdev->dev);
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* RE: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it arch-independent.
From: Jianyong Wu (Arm Technology China) @ 2019-09-27 10:35 UTC (permalink / raw)
To: Suzuki Poulose, netdev@vger.kernel.org, yangbo.lu@nxp.com,
john.stultz@linaro.org, tglx@linutronix.de, pbonzini@redhat.com,
sean.j.christopherson@intel.com, maz@kernel.org,
richardcochran@gmail.com, Mark Rutland, Will Deacon
Cc: Justin He (Arm Technology China), kvm@vger.kernel.org,
Steve Capper, linux-kernel@vger.kernel.org,
Kaly Xin (Arm Technology China), nd, kvmarm@lists.cs.columbia.edu,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <47ceb25c-c9ff-e284-43bf-6cac7e128a98@arm.com>
Hi Suzuki,
> -----Original Message-----
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> Sent: Friday, September 27, 2019 6:23 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> maz@kernel.org; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
> <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it
> arch-independent.
>
>
>
> On 26/09/2019 12:42, Jianyong Wu wrote:
> > Currently, ptp_kvm modules implementation is only for x86 which
> > includs large part of arch-specific code. This patch move all of
> > those code into new arch related file in the same directory.
> >
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
>
> ...
>
> > +int kvm_arch_ptp_get_clock_fn(unsigned long *cycle, struct timespec64
> *tspec,
> > + struct clocksource **cs)
>
>
> > diff --git a/include/asm-generic/ptp_kvm.h
> > b/include/asm-generic/ptp_kvm.h new file mode 100644 index
> > 000000000000..208e842bfa64
> > --- /dev/null
> > +++ b/include/asm-generic/ptp_kvm.h
>
> > +int kvm_arch_ptp_get_clock_fn(long *cycle,
> > + struct timespec64 *tspec, void *cs);
> >
>
> Conflicting types for kvm_arch_ptp_get_clock_fn() ?
>
Yeah, need fix.
Thanks
Jianyong Wu
> Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet encryption
From: Pascal Van Leeuwen @ 2019-09-27 10:44 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jason A . Donenfeld, Catalin Marinas, Herbert Xu, Arnd Bergmann,
Ard Biesheuvel, Greg KH, Eric Biggers, Samuel Neves,
Linux Crypto Mailing List, Andy Lutomirski, Marc Zyngier,
Dan Carpenter, Will Deacon, David Miller, Linux ARM
In-Reply-To: <CAHk-=whqWh8ebZ7ryEv5tKKtO5VpOj2rWVy7wV+aHWGO7m9gAw@mail.gmail.com>
> -----Original Message-----
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Sent: Friday, September 27, 2019 4:54 AM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Linux Crypto Mailing List <linux-
> crypto@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>; Herbert Xu
> <herbert@gondor.apana.org.au>; David Miller <davem@davemloft.net>; Greg KH
> <gregkh@linuxfoundation.org>; Jason A . Donenfeld <Jason@zx2c4.com>; Samuel Neves
> <sneves@dei.uc.pt>; Dan Carpenter <dan.carpenter@oracle.com>; Arnd Bergmann
> <arnd@arndb.de>; Eric Biggers <ebiggers@google.com>; Andy Lutomirski <luto@kernel.org>;
> Will Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Catalin Marinas
> <catalin.marinas@arm.com>
> Subject: Re: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet
> encryption
>
> On Thu, Sep 26, 2019 at 6:30 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > And once you have that cookie, and you see "ok, I didn't get the
> > answer immediately" only THEN do you start filling in things like
> > callback stuff, or maybe you set up a wait-queue and start waiting for
> > it, or whatever".
>
> Side note: almost nobody does this.
>
> Almost every single async interface I've ever seen ends up being "only
> designed for async".
>
> And I think the reason is that everybody first does the simply
> synchronous interfaces, and people start using those, and a lot of
> people are perfectly happy with them. They are simple, and they work
> fine for the huge majority of users.
>
> And then somebody comes along and says "no, _we_ need to do this
> asynchronously", and by definition that person does *not* care for the
> synchronous case, since that interface already existed and was simpler
> and already was mostly sufficient for the people who used it, and so
> the async interface ends up being _only_ designed for the new async
> workflow. Because that whole new world was written with just that case
> is mind, and the synchronous case clearly didn't matter.
>
> So then you end up with that kind of dichotomous situation, where you
> have a strict black-and-white either-synchronous-or-async model.
>
> And then some people - quite reasonably - just want the simplicity of
> the synchronous code and it performs better for them because the
> interfaces are simpler and better suited to their lack of extra work.
>
> And other people feel they need the async code, because they can take
> advantage of it.
>
> And never the twain shall meet, because the async interface is
> actively _bad_ for the people who have sync workloads and the sync
> interface doesn't work for the async people.
>
> Non-crypto example: [p]read() vs aio_read(). They do the same thing
> (on a high level) apart from that sync/async issue. And there's no way
> to get the best of both worlds.
>
> Doing aio_read() on something that is already cached is actively much
> worse than just doing a synchronous read() of cached data.
>
> But aio_read() _can_ be much better if you know your workload doesn't
> cache well and read() blocks too much for you.
>
> There's no "read_potentially_async()" interface that just does the
> synchronous read for any cached portion of the data, and then delays
> just the IO parts and returns a "here, I gave you X bytes right now,
> use this cookie to wait for the rest".
>
> Maybe nobody would use it. But it really should be possibly to have
> interfaces where a good synchronous implementation is _possible_
> without the extra overhead, while also allowing async implementations.
>
That's the question. I've never seen such an API yet ...
You could also just accept that those are two wildly different use
cases with wildly different requirements and allow them to coexist,
while sharing as much of the low-level SW implementation code as
possible underneath. With the async API only used for those cases
where HW acceleration can make the difference.
I believe for hashes, the Crypto API still maintains an shash and
an ahash API. It works the other way around from how you would
like to see though, with ahash wrapping the shash in case of SW
implementations. Still, if you're sure you can't benefit from HW
acceleration you have the option of using the shash directly.
I don't know why the synchronous blkcipher API was deprecated,
that happened before I joined. IMHO it would make sense to have,
so users not interested in HW crypto are not burdened by it.
> Linus
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 0/5]arm64: vdso32: Address various issues
From: Catalin Marinas @ 2019-09-27 10:46 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: ard.biesheuvel, ndesaulniers, linux-kernel, tglx, will,
linux-arm-kernel
In-Reply-To: <20190926214342.34608-1-vincenzo.frascino@arm.com>
On Thu, Sep 26, 2019 at 10:43:37PM +0100, Vincenzo Frascino wrote:
> Vincenzo Frascino (5):
> arm64: vdso32: Introduce COMPAT_CC_IS_GCC
> arm64: vdso32: Detect binutils support for dmb ishld
> arm64: vdso32: Fix compilation warning
> arm64: Remove gettimeofday.S
> arm64: vdso32: Remove jump label config option in Makefile
You can add this as well:
Tested-by: Catalin Marinas <catalin.marinas@arm.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/1] arm64/sve: Fix wrong free for task->thread.sve_state
From: Julien Grall @ 2019-09-27 11:04 UTC (permalink / raw)
To: Masayoshi Mizuma, Catalin Marinas, Will Deacon, linux-arm-kernel
Cc: Masayoshi Mizuma, linux-kernel, Dave P Martin
In-Reply-To: <20190926190846.3072-2-msys.mizuma@gmail.com>
(+ Dave)
Hi,
Thank you for the patch.
On 26/09/2019 20:08, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
>
> The system which has SVE feature crashed because of
> the memory pointed by task->thread.sve_state was destroyed
> by someone.
>
> That is because sve_state is freed while the forking the
> child process. The child process has the pointer of sve_state
> which is same as the parent's because the child's task_struct
> is copied from the parent's one. If the copy_process()
> fails as an error on somewhere, for example, copy_creds(),
> then the sve_state is freed even if the parent is alive.
> The flow is as follows.
>
> copy_process
> p = dup_task_struct
> => arch_dup_task_struct
> *dst = *src; // copy the entire region.
> :
> retval = copy_creds
> if (retval < 0)
> goto bad_fork_free;
> :
> bad_fork_free:
> ...
> delayed_free_task(p);
> => free_task
> => arch_release_task_struct
> => fpsimd_release_task
> => __sve_free
> => kfree(task->thread.sve_state);
> // free the parent's sve_state
The flow makes sense to me and I agree you would end up to free the parent's state.
>
> Add a flag in task->thread which shows the fork is in progress.
> If the fork is in progress, that means the child has the pointer
> to the parent's sve_state, doesn't free the sve_state.
I haven't fully investigate it yet but I was wondering if we could just clear
task->thread.sve_state for the child in arch_dup_task_struct().
I saw the comment on top of function mentioning potential issue to do it there.
I understand that you may not be able to clear TIF_SVE in the function, but I
don't understand why clearing just task->thread.sve_state would be an issue.
The only risk I can see is TIF_SVE may be set with task->thread.sve_state to be
NULL. But this is a new task, so I don't think there are risk here to have it
unsync. Dave?
>
> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> Reported-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> ---
> arch/arm64/include/asm/processor.h | 1 +
> arch/arm64/kernel/fpsimd.c | 6 ++++--
> arch/arm64/kernel/process.c | 2 ++
> 3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 5623685c7d13..3ca3e350145a 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -143,6 +143,7 @@ struct thread_struct {
> unsigned long fault_address; /* fault info */
> unsigned long fault_code; /* ESR_EL1 value */
> struct debug_info debug; /* debugging */
> + unsigned int fork_in_progress;
> #ifdef CONFIG_ARM64_PTR_AUTH
> struct ptrauth_keys keys_user;
> #endif
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 37d3912cfe06..8641db4cb062 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -202,8 +202,10 @@ static bool have_cpu_fpsimd_context(void)
> */
> static void __sve_free(struct task_struct *task)
> {
> - kfree(task->thread.sve_state);
> - task->thread.sve_state = NULL;
> + if (!task->thread.fork_in_progress) {
> + kfree(task->thread.sve_state);
> + task->thread.sve_state = NULL;
> + }
> }
>
> static void sve_free(struct task_struct *task)
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index a47462def04b..8ac0ee4e5f76 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -347,6 +347,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
> if (current->mm)
> fpsimd_preserve_current_state();
> *dst = *src;
> + dst->thread.fork_in_progress = 1;
>
> return 0;
> }
> @@ -365,6 +366,7 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
> * and disable discard SVE state for p:
> */
> clear_tsk_thread_flag(p, TIF_SVE);
> + p->thread.fork_in_progress = 0;
> p->thread.sve_state = NULL;
>
> /*
>
Cheers,
--
Julien Grall
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet encryption
From: Pascal Van Leeuwen @ 2019-09-27 11:08 UTC (permalink / raw)
To: Pascal Van Leeuwen, Linus Torvalds
Cc: Jason A . Donenfeld, Catalin Marinas, Herbert Xu, Arnd Bergmann,
Ard Biesheuvel, Greg KH, Eric Biggers, Samuel Neves,
Linux Crypto Mailing List, Andy Lutomirski, Marc Zyngier,
Dan Carpenter, Will Deacon, David Miller, Linux ARM
In-Reply-To: <MN2PR20MB297359DCCE92EB1A1F13CE03CA810@MN2PR20MB2973.namprd20.prod.outlook.com>
> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf
> Of Pascal Van Leeuwen
> Sent: Friday, September 27, 2019 12:44 PM
> To: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Linux Crypto Mailing List <linux-
> crypto@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>; Herbert Xu
> <herbert@gondor.apana.org.au>; David Miller <davem@davemloft.net>; Greg KH
> <gregkh@linuxfoundation.org>; Jason A . Donenfeld <Jason@zx2c4.com>; Samuel Neves
> <sneves@dei.uc.pt>; Dan Carpenter <dan.carpenter@oracle.com>; Arnd Bergmann
> <arnd@arndb.de>; Eric Biggers <ebiggers@google.com>; Andy Lutomirski <luto@kernel.org>;
> Will Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Catalin Marinas
> <catalin.marinas@arm.com>
> Subject: RE: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet
> encryption
>
> > -----Original Message-----
> > From: Linus Torvalds <torvalds@linux-foundation.org>
> > Sent: Friday, September 27, 2019 4:54 AM
> > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Linux Crypto Mailing List <linux-
> > crypto@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>; Herbert Xu
> > <herbert@gondor.apana.org.au>; David Miller <davem@davemloft.net>; Greg KH
> > <gregkh@linuxfoundation.org>; Jason A . Donenfeld <Jason@zx2c4.com>; Samuel Neves
> > <sneves@dei.uc.pt>; Dan Carpenter <dan.carpenter@oracle.com>; Arnd Bergmann
> > <arnd@arndb.de>; Eric Biggers <ebiggers@google.com>; Andy Lutomirski
> <luto@kernel.org>;
> > Will Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Catalin Marinas
> > <catalin.marinas@arm.com>
> > Subject: Re: [RFC PATCH 18/18] net: wireguard - switch to crypto API for packet
> > encryption
> >
> > On Thu, Sep 26, 2019 at 6:30 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > And once you have that cookie, and you see "ok, I didn't get the
> > > answer immediately" only THEN do you start filling in things like
> > > callback stuff, or maybe you set up a wait-queue and start waiting for
> > > it, or whatever".
> >
> > Side note: almost nobody does this.
> >
> > Almost every single async interface I've ever seen ends up being "only
> > designed for async".
> >
> > And I think the reason is that everybody first does the simply
> > synchronous interfaces, and people start using those, and a lot of
> > people are perfectly happy with them. They are simple, and they work
> > fine for the huge majority of users.
> >
> > And then somebody comes along and says "no, _we_ need to do this
> > asynchronously", and by definition that person does *not* care for the
> > synchronous case, since that interface already existed and was simpler
> > and already was mostly sufficient for the people who used it, and so
> > the async interface ends up being _only_ designed for the new async
> > workflow. Because that whole new world was written with just that case
> > is mind, and the synchronous case clearly didn't matter.
> >
> > So then you end up with that kind of dichotomous situation, where you
> > have a strict black-and-white either-synchronous-or-async model.
> >
> > And then some people - quite reasonably - just want the simplicity of
> > the synchronous code and it performs better for them because the
> > interfaces are simpler and better suited to their lack of extra work.
> >
> > And other people feel they need the async code, because they can take
> > advantage of it.
> >
> > And never the twain shall meet, because the async interface is
> > actively _bad_ for the people who have sync workloads and the sync
> > interface doesn't work for the async people.
> >
> > Non-crypto example: [p]read() vs aio_read(). They do the same thing
> > (on a high level) apart from that sync/async issue. And there's no way
> > to get the best of both worlds.
> >
> > Doing aio_read() on something that is already cached is actively much
> > worse than just doing a synchronous read() of cached data.
> >
> > But aio_read() _can_ be much better if you know your workload doesn't
> > cache well and read() blocks too much for you.
> >
> > There's no "read_potentially_async()" interface that just does the
> > synchronous read for any cached portion of the data, and then delays
> > just the IO parts and returns a "here, I gave you X bytes right now,
> > use this cookie to wait for the rest".
> >
> > Maybe nobody would use it. But it really should be possibly to have
> > interfaces where a good synchronous implementation is _possible_
> > without the extra overhead, while also allowing async implementations.
> >
> That's the question. I've never seen such an API yet ...
>
> You could also just accept that those are two wildly different use
> cases with wildly different requirements and allow them to coexist,
> while sharing as much of the low-level SW implementation code as
> possible underneath. With the async API only used for those cases
> where HW acceleration can make the difference.
>
> I believe for hashes, the Crypto API still maintains an shash and
> an ahash API. It works the other way around from how you would
> like to see though, with ahash wrapping the shash in case of SW
> implementations. Still, if you're sure you can't benefit from HW
> acceleration you have the option of using the shash directly.
>
> I don't know why the synchronous blkcipher API was deprecated,
> that happened before I joined. IMHO it would make sense to have,
> so users not interested in HW crypto are not burdened by it.
>
>
Never mind. From what I just learned, you can achieve the same
thing with the skcipher API by just requesting a sync implementation.
Which would allow you to put your structs on the stack and would
not return from the encrypt()/decrypt() call until actually done.
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
From: Leonard Crestez @ 2019-09-27 11:16 UTC (permalink / raw)
To: Anson Huang, Marco Felsch, Aisheng Dong
Cc: shawnguo@kernel.org, s.hauer@pengutronix.de,
linux-kernel@vger.kernel.org, dl-linux-imx, kernel@pengutronix.de,
festevam@gmail.com, linux-arm-kernel@lists.infradead.org
In-Reply-To: <DB3PR0402MB391675F9BF6FCA315B124BEBF5810@DB3PR0402MB3916.eurprd04.prod.outlook.com>
On 27.09.2019 04:20, Anson Huang wrote:
>> On 2019-09-26 1:06 PM, Marco Felsch wrote:
>>> On 19-09-26 08:03, Anson Huang wrote:
>>>>> On 19-09-25 18:07, Anson Huang wrote:
>>>>>> The SCU firmware does NOT always have return value stored in
>>>>>> message header's function element even the API has response data,
>>>>>> those special APIs are defined as void function in SCU firmware, so
>>>>>> they should be treated as return success always.
>>>>>>
>>>>>> +static const struct imx_sc_rpc_msg whitelist[] = {
>>>>>> + { .svc = IMX_SC_RPC_SVC_MISC, .func =
>>>>> IMX_SC_MISC_FUNC_UNIQUE_ID },
>>>>>> + { .svc = IMX_SC_RPC_SVC_MISC, .func =
>>>>>> +IMX_SC_MISC_FUNC_GET_BUTTON_STATUS }, };
>>>>>
>>>>> Is this going to be extended in the near future? I see some upcoming
>>>>> problems here if someone uses a different scu-fw<->kernel
>>>>> combination as nxp would suggest.
>>>>
>>>> Could be, but I checked the current APIs, ONLY these 2 will be used
>>>> in Linux kernel, so I ONLY add these 2 APIs for now.
>>>
>>> Okay.
>>>
>>>> However, after rethink, maybe we should add another imx_sc_rpc API
>>>> for those special APIs? To avoid checking it for all the APIs called which
>> may impact some performance.
>>>> Still under discussion, if you have better idea, please advise, thanks!
>>
>> My suggestion is to refactor the code and add a new API for the this "no
>> error value" convention. Internally they can call a common function with
>> flags.
>
> If I understand your point correctly, that means the loop check of whether the API
> is with "no error value" for every API still NOT be skipped, it is just refactoring the code,
> right?
>> Right now developers who want to make SCFW calls in upstream need to
>> define the message struct in their driver based on protocol documentation.
>> This includes:
>>
>> * Binary layout of the message (a packed struct)
>> * If the message has a response (already a bool flag)
>> * If an error code is returned (this patch adds support for it)
>>
>> Since callers are already exposed to the binary protocol exposing them to
>> minor quirks of the calling convention also seems reasonable. Having the
>> low-level IPC code peek at message IDs seems like a hack; this belong at a
>> slightly higher level.
>
> A little confused, so what you suggested is to add make the imx_scu_call_rpc()
> becomes the "slightly higher level" API, then in this API, check the message IDs
> to decide whether to return error value, then calls a new API which will have
> the low-level IPC code, the this new API will have a flag passed from imx_scu_call_rpc()
> function, am I right?
No, I mean there should be no loop enumerating svc/func ids: *the caller
should know* that it's calling a func which doesn't return an error code
and call a different variant of imx_scu_call_rpc
Maybe add an internal __imx_scu_call_rpc_flags and turn the current
imx_scu_call_rpc into a wrapper.
--
Regards,
Leonard
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] ARM: dts: stm32: Enable high resolution timer
From: Marc Zyngier @ 2019-09-27 11:22 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: mark.rutland, devicetree, alexandre.torgue, linux-kernel, robh+dt,
linux-stm32, linux-arm-kernel
In-Reply-To: <20190927084819.645-1-benjamin.gaignard@st.com>
On 2019-09-27 09:48, Benjamin Gaignard wrote:
> Adding always-on makes arm arch_timer claim to be an high resolution
> timer.
> That is possible because power mode won't stop clocking the timer.
The "always-on" is not about the clock. It is about the comparator.
The clock itself is *guaranteed* to always tick. If it didn't, that'd
be
an integration bug, and a pretty bad one.
What you're claiming here is that your CPU never enters a low-power
mode?
Ever? I find this very hard to believe.
Furthermore, claiming that always-on is the way to force the arch-timer
to be an hrtimer is factually wrong. This is what happens *if* this is
the only timer in the system. The only case this is true is for virtual
machines. Anything else has a global timer somewhere that will allow
the arch timers to be used as an hrtimer.
I'm pretty sure you too have a global timer somewhere in your system.
Enable it, and enjoy hrtimers without having to lie about the
properties
of your system! ;-)
M.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
> arch/arm/boot/dts/stm32mp157c.dtsi | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi
> b/arch/arm/boot/dts/stm32mp157c.dtsi
> index 9b11654a0a39..74f64745d60d 100644
> --- a/arch/arm/boot/dts/stm32mp157c.dtsi
> +++ b/arch/arm/boot/dts/stm32mp157c.dtsi
> @@ -50,6 +50,7 @@
> <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> interrupt-parent = <&intc>;
> + always-on;
> };
>
> clocks {
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
From: Leonard Crestez @ 2019-09-27 11:22 UTC (permalink / raw)
To: Marco Felsch, Anson Huang
Cc: Aisheng Dong, shawnguo@kernel.org, s.hauer@pengutronix.de,
linux-kernel@vger.kernel.org, dl-linux-imx, kernel@pengutronix.de,
festevam@gmail.com, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190927090609.fyxdekkzrco7memt@pengutronix.de>
On 27.09.2019 12:06, Marco Felsch wrote:
> Hi Anson, Leonard,
>
> On 19-09-27 01:20, Anson Huang wrote:
>> Hi, Leonard
>>
>>> On 2019-09-26 1:06 PM, Marco Felsch wrote:
>>>> On 19-09-26 08:03, Anson Huang wrote:
>>>>>> On 19-09-25 18:07, Anson Huang wrote:
>>>>>>> The SCU firmware does NOT always have return value stored in
>>>>>>> message header's function element even the API has response data,
>>>>>>> those special APIs are defined as void function in SCU firmware, so
>>>>>>> they should be treated as return success always.
>>>>>>>
>>>>>>> +static const struct imx_sc_rpc_msg whitelist[] = {
>>>>>>> + { .svc = IMX_SC_RPC_SVC_MISC, .func =
>>>>>> IMX_SC_MISC_FUNC_UNIQUE_ID },
>>>>>>> + { .svc = IMX_SC_RPC_SVC_MISC, .func =
>>>>>>> +IMX_SC_MISC_FUNC_GET_BUTTON_STATUS }, };
>>>>>>
>>>>>> Is this going to be extended in the near future? I see some upcoming
>>>>>> problems here if someone uses a different scu-fw<->kernel
>>>>>> combination as nxp would suggest.
>>>>>
>>>>> Could be, but I checked the current APIs, ONLY these 2 will be used
>>>>> in Linux kernel, so I ONLY add these 2 APIs for now.
>>>>
>>>> Okay.
>>>>
>>>>> However, after rethink, maybe we should add another imx_sc_rpc API
>>>>> for those special APIs? To avoid checking it for all the APIs called which
>>> may impact some performance.
>>>>> Still under discussion, if you have better idea, please advise, thanks!
>>>
>>> My suggestion is to refactor the code and add a new API for the this "no
>>> error value" convention. Internally they can call a common function with
>>> flags.
>>
>>>> Adding a special api shouldn't be the right fix. Imagine if someone
>>>> (not a nxp-developer) wants to add a new driver. How could he be
>>>> expected to know which api he should use. The better abbroach would be
>>>> to fix the scu-fw instead of adding quirks..
>>
>> Yes, fixing SCU FW is the best solution, but we have talked to SCU FW owner, the SCU
>> FW released has been finalized, so the API implementation can NOT be changed, but
>> they will pay attention to this issue for new added APIs later. That means the number
>> of APIs having this issue a very limited.
>
> This means those APIs which already have this bug will not be fixed?
> IMHO this sounds a bit weird since this is a changeable peace of code ;)
It's not a bug, it's a documented feature ;)
>>> Right now developers who want to make SCFW calls in upstream need to
>>> define the message struct in their driver based on protocol documentation.
>>> This includes:
>>>
>>> * Binary layout of the message (a packed struct)
>>> * If the message has a response (already a bool flag)
>>> * If an error code is returned (this patch adds support for it)
>
> Why should I specify if a error code is returned?
Because you're already defining the message struct and you're already
specifying if a response is required.
The assumption is that anyone adding a SCFW call to a driver is already
looking at SCFW documentation which describes the binary message format.
--
Regards,
Leonard
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/4] arm64: defconfig: enable R8A774B1 SoC
From: Geert Uytterhoeven @ 2019-09-27 11:32 UTC (permalink / raw)
To: Biju Das
Cc: Fabrizio Castro, Linux-Renesas, Chris Paterson, Anson Huang,
Arnd Bergmann, Catalin Marinas, Maxime Ripard, Bjorn Andersson,
Marcin Juszkiewicz, Dinh Nguyen, Simon Horman, Jagan Teki,
Olof Johansson, Shawn Guo, Geert Uytterhoeven, Leonard Crestez,
Will Deacon, Linux ARM
In-Reply-To: <1568725530-55241-3-git-send-email-biju.das@bp.renesas.com>
On Tue, Sep 17, 2019 at 3:12 PM Biju Das <biju.das@bp.renesas.com> wrote:
> Enable the Renesas RZ/G2N (R8A774B1) SoC in the ARM64 defconfig.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v5.5.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v15 1/4] soc: mediatek: cmdq: define the instruction struct
From: Bibby Hsieh @ 2019-09-27 11:42 UTC (permalink / raw)
To: Matthias Brugger, Rob Herring, CK HU
Cc: devicetree, Nicolas Boichat, srv_heupstream, linux-kernel,
Houlong Wei, Dennis-YC Hsieh, linux-mediatek, Bibby Hsieh,
linux-arm-kernel
In-Reply-To: <20190927114254.6258-1-bibby.hsieh@mediatek.com>
Define an instruction structure for gce driver to append command.
This structure can make the client's code more readability.
Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
---
drivers/soc/mediatek/mtk-cmdq-helper.c | 106 +++++++++++++++++------
include/linux/mailbox/mtk-cmdq-mailbox.h | 10 +++
2 files changed, 90 insertions(+), 26 deletions(-)
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 7aa0517ff2f3..7af327b98d25 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -9,12 +9,24 @@
#include <linux/mailbox_controller.h>
#include <linux/soc/mediatek/mtk-cmdq.h>
-#define CMDQ_ARG_A_WRITE_MASK 0xffff
#define CMDQ_WRITE_ENABLE_MASK BIT(0)
#define CMDQ_EOC_IRQ_EN BIT(0)
#define CMDQ_EOC_CMD ((u64)((CMDQ_CODE_EOC << CMDQ_OP_CODE_SHIFT)) \
<< 32 | CMDQ_EOC_IRQ_EN)
+struct cmdq_instruction {
+ union {
+ u32 value;
+ u32 mask;
+ };
+ union {
+ u16 offset;
+ u16 event;
+ };
+ u8 subsys;
+ u8 op;
+};
+
static void cmdq_client_timeout(struct timer_list *t)
{
struct cmdq_client *client = from_timer(client, t, timer);
@@ -110,10 +122,10 @@ void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
}
EXPORT_SYMBOL(cmdq_pkt_destroy);
-static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
- u32 arg_a, u32 arg_b)
+static int cmdq_pkt_append_command(struct cmdq_pkt *pkt,
+ struct cmdq_instruction *inst)
{
- u64 *cmd_ptr;
+ struct cmdq_instruction *cmd_ptr;
if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) {
/*
@@ -129,8 +141,9 @@ static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
__func__, (u32)pkt->buf_size);
return -ENOMEM;
}
+
cmd_ptr = pkt->va_base + pkt->cmd_buf_size;
- (*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b;
+ *cmd_ptr = *inst;
pkt->cmd_buf_size += CMDQ_INST_SIZE;
return 0;
@@ -138,24 +151,42 @@ static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code,
int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value)
{
- u32 arg_a = (offset & CMDQ_ARG_A_WRITE_MASK) |
- (subsys << CMDQ_SUBSYS_SHIFT);
+ struct cmdq_instruction *inst = kzalloc(sizeof(*inst), GFP_KERNEL);
+ int err = 0;
+
+ if (!inst)
+ return -ENOMEM;
+
+ inst->op = CMDQ_CODE_WRITE;
+ inst->value = value;
+ inst->offset = offset;
+ inst->subsys = subsys;
- return cmdq_pkt_append_command(pkt, CMDQ_CODE_WRITE, arg_a, value);
+ err = cmdq_pkt_append_command(pkt, inst);
+ kfree(inst);
+
+ return err;
}
EXPORT_SYMBOL(cmdq_pkt_write);
int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
u16 offset, u32 value, u32 mask)
{
- u32 offset_mask = offset;
+ struct cmdq_instruction *inst = kzalloc(sizeof(*inst), GFP_KERNEL);
+ u16 offset_mask = offset;
int err = 0;
+ if (!inst)
+ return -ENOMEM;
+
if (mask != 0xffffffff) {
- err = cmdq_pkt_append_command(pkt, CMDQ_CODE_MASK, 0, ~mask);
+ inst->op = CMDQ_CODE_MASK;
+ inst->mask = ~mask;
+ err = cmdq_pkt_append_command(pkt, inst);
offset_mask |= CMDQ_WRITE_ENABLE_MASK;
}
- err |= cmdq_pkt_write(pkt, value, subsys, offset_mask);
+ err |= cmdq_pkt_write(pkt, subsys, offset_mask, value);
+ kfree(inst);
return err;
}
@@ -163,43 +194,66 @@ EXPORT_SYMBOL(cmdq_pkt_write_mask);
int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
{
- u32 arg_b;
+ struct cmdq_instruction *inst = kzalloc(sizeof(*inst), GFP_KERNEL);
+ int err = 0;
+
+ if (!inst)
+ return -ENOMEM;
if (event >= CMDQ_MAX_EVENT)
return -EINVAL;
- /*
- * WFE arg_b
- * bit 0-11: wait value
- * bit 15: 1 - wait, 0 - no wait
- * bit 16-27: update value
- * bit 31: 1 - update, 0 - no update
- */
- arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
+ inst->op = CMDQ_CODE_WFE;
+ inst->value = CMDQ_WFE_OPTION;
+ inst->event = event;
- return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event, arg_b);
+ err = cmdq_pkt_append_command(pkt, inst);
+ kfree(inst);
+
+ return err;
}
EXPORT_SYMBOL(cmdq_pkt_wfe);
int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
{
+ struct cmdq_instruction *inst = kzalloc(sizeof(*inst), GFP_KERNEL);
+ int err = 0;
+
+ if (!inst)
+ return -ENOMEM;
+
if (event >= CMDQ_MAX_EVENT)
return -EINVAL;
- return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event,
- CMDQ_WFE_UPDATE);
+ inst->op = CMDQ_CODE_WFE;
+ inst->value = CMDQ_WFE_UPDATE;
+ inst->event = event;
+
+ err = cmdq_pkt_append_command(pkt, inst);
+ kfree(inst);
+
+ return err;
}
EXPORT_SYMBOL(cmdq_pkt_clear_event);
static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
{
- int err;
+ struct cmdq_instruction *inst = kzalloc(sizeof(*inst), GFP_KERNEL);
+ int err = 0;
+
+ if (!inst)
+ return -ENOMEM;
/* insert EOC and generate IRQ for each command iteration */
- err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
+ inst->op = CMDQ_CODE_EOC;
+ inst->value = CMDQ_EOC_IRQ_EN;
+ err = cmdq_pkt_append_command(pkt, inst);
/* JUMP to end */
- err |= cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
+ inst->op = CMDQ_CODE_JUMP;
+ inst->value = CMDQ_JUMP_PASS;
+ err |= cmdq_pkt_append_command(pkt, inst);
+ kfree(inst);
return err;
}
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
index e6f54ef6698b..678760548791 100644
--- a/include/linux/mailbox/mtk-cmdq-mailbox.h
+++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
@@ -20,6 +20,16 @@
#define CMDQ_WFE_WAIT BIT(15)
#define CMDQ_WFE_WAIT_VALUE 0x1
+/*
+ * WFE arg_b
+ * bit 0-11: wait value
+ * bit 15: 1 - wait, 0 - no wait
+ * bit 16-27: update value
+ * bit 31: 1 - update, 0 - no update
+ */
+#define CMDQ_WFE_OPTION (CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | \
+ CMDQ_WFE_WAIT_VALUE)
+
/** cmdq event maximum */
#define CMDQ_MAX_EVENT 0x3ff
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox