* [PATCH resend] arm: dts: ls1021a: add reboot node to .dtsi
From: Rasmus Villemoes @ 2017-12-05 8:12 UTC (permalink / raw)
To: Shawn Guo, Rob Herring, Mark Rutland, Russell King
Cc: Rasmus Villemoes, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <1510566980-26602-1-git-send-email-rasmus.villemoes@prevas.dk>
The LS1021A can be reset via the dcfg regmap in the same way as the
arm64 layerscape SoCs, so add the corresponding DT node.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
arch/arm/boot/dts/ls1021a.dtsi | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index 9319e1f0f1d8..3ff2b8a9f01a 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -108,6 +108,13 @@
<GIC_SPI 139 IRQ_TYPE_LEVEL_HIGH>;
};
+ reboot {
+ compatible = "syscon-reboot";
+ regmap = <&dcfg>;
+ offset = <0xb0>;
+ mask = <0x02>;
+ };
+
soc {
compatible = "simple-bus";
#address-cells = <2>;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 3/8] mfd: axp20x: probe axp20x_adc driver for AXP813
From: Maxime Ripard @ 2017-12-05 8:08 UTC (permalink / raw)
To: Quentin Schulz
Cc: sre-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, wens-jdAy2FN1RRM,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, jic23-DgEjT+Ai2ygdnm+yROfE0A,
lee.jones-QSEj5FYQhm4dnm+yROfE0A, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-iio-u79uwXL29TY76Z2rM5mHXA, icenowy-h8G6r0blFSE,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
In-Reply-To: <302c93023d8c4a54cafe321d912df484e39727b7.1512396054.git-series.quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 857 bytes --]
On Mon, Dec 04, 2017 at 03:12:49PM +0100, Quentin Schulz wrote:
> This makes the axp20x_adc driver probe with platform device id
> "axp813-adc".
>
> Signed-off-by: Quentin Schulz <quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> drivers/mfd/axp20x.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 2468b43..42e54d1 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -878,7 +878,9 @@ static struct mfd_cell axp813_cells[] = {
> .resources = axp803_pek_resources,
> }, {
> .name = "axp20x-regulator",
> - }
> + }, {
> + .name = "axp813-adc",
> + },
Any particular reason you're not adding it to the DT?
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path
From: Geert Uytterhoeven @ 2017-12-05 8:01 UTC (permalink / raw)
To: Frank Rowand
Cc: Geert Uytterhoeven, Pantelis Antoniou, Rob Herring, Colin King,
Dan Carpenter, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <b6392405-390d-d7dc-91b2-1338fd605e65-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Frank,
On Tue, Dec 5, 2017 at 3:07 AM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 12/04/17 10:47, Geert Uytterhoeven wrote:
>> If of_resolve_phandles() fails, free_overlay_changeset() is called in
>> the error path. However, that function returns early if the list hasn't
>> been initialized yet, before freeing the object.
>>
>> Explicitly calling kfree() instead would solve that issue. However, that
>> complicates matter, by having to consider which of two different methods
>> to use to dispose of the same object.
>>
>> Hence make free_overlay_changeset() consider initialization state of the
>> different parts of the object, making it always safe to call (once!) to
>> dispose of a (partially) initialized overlay_changeset:
>> - Only destroy the changeset if the list was initialized,
>> - Ignore uninitialized IDs (zero).
>>
>> Reported-by: Colin King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> Fixes: f948d6d8b792bb90 ("of: overlay: avoid race condition between applying multiple overlays")
>> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
>> ---
>> drivers/of/overlay.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 3b7a3980ff50d6bf..312cd658bec0083b 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -630,11 +630,10 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>> {
>> int i;
>>
>> - if (!ovcs->cset.entries.next)
>> - return;
>> - of_changeset_destroy(&ovcs->cset);
>> + if (ovcs->cset.entries.next)
>> + of_changeset_destroy(&ovcs->cset);
>>
>
> OK
>
>> - if (ovcs->id)
>> + if (ovcs->id > 0)
>
> Instead of this change, could you please make a change in init_overlay_changeset()?
>
> Current init_overlay_changeset():
>
> ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
> if (ovcs->id <= 0)
> return ovcs->id;
>
> My proposed version:
>
> ret = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
> if (ret <= 0)
> return ret;
> ovcs->id = ret;
Sure.
>> idr_remove(&ovcs_idr, ovcs->id);
>>
>> for (i = 0; i < ovcs->count; i++) {
>>
>
> Also, the previous version of the patch, and the discussion around the resulting
> bug make me think that I should not have moved 'kfree(ovcs)' into
> free_overlay_changeset(), because that kfree is then not very visible in the
> error path of of_overlay_apply(). Could you remove 'kfree(ovcs)' from
> free_overlay_changeset(), and instead call it immediately after each call
> to free_overlay_changeset()?
Actually I like that free_overlay_changeset() takes care of the deallocation,
especially in light of the kojectification op top from bbb-overlays, which
means you cannot just call kfree(ovcs) anymore (I know this won't go upstream
anytime soon, but I need overlay configfs for my development and testing).
Perhaps the allocation of ovcs should be moved into free_overlay_changeset(),
and the latter being renamed to alloc_overlay_changeset()?
That way allocation and freeing become symmetrical.
It would move the allocation under the mutexes, though.
What do you think?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/1] at24: support eeproms that do not auto-rollover reads.
From: Sakari Ailus @ 2017-12-05 7:44 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: Sven Van Asbroeck, robh+dt, mark.rutland, wsa,
Bartosz Golaszewski, nsekhar, david, javier, divagar.mohandass,
devicetree, linux-kernel, linux-i2c, Sven Van Asbroeck
In-Reply-To: <CAGngYiXR83+0phkeAvJYYEm8HvmN-c9d0B9p1BRiPHyUz1ogAA@mail.gmail.com>
On Mon, Dec 04, 2017 at 05:24:33PM -0500, Sven Van Asbroeck wrote:
> > If this is truly specific to at24, then vendor prefix would be appropriate,
> > plus it'd go to an at24 specific binding file. However if it isn't I'd just
> > remove the above sentence. I guess the latter?
>
> Yes, no-read-rollover is truly specific to at24.c, because it applies only
> to i2c multi-address chips. The at25 is spi based so cannot have multiple
> addresses.
>
> So yes, "at24,no-read-rollover" would perhaps be a better name.
>
> Regarding an at24 specific binding file. You're saying I should create
> Documentation/devicetree/bindings/eeprom/at24.txt ? Should I indicate
> that at24.txt "inherits from" eeprom.txt? Note that at25.txt does not
> currently do this.
Hmm. I actually missed we didn't have one to begin with. at25.txt exists
and it documents a number of properties specific to at25, so if at24 will
have an at24-specific property, then I think it should go to a separate
file.
Aren't there really other chips which need this? It'd be (a little bit)
easier to just remove the sentence. :-)
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply
* [PATCH 2/2] ARM: dts: keystone-k2g-evm: Enable UART 2
From: Vignesh R @ 2017-12-05 6:48 UTC (permalink / raw)
To: Santosh Shilimkar
Cc: Rob Herring, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Vignesh R,
Franklin S Cooper Jr
In-Reply-To: <20171205064812.15758-1-vigneshr-l0cyMroinI0@public.gmane.org>
From: Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org>
66AK2G GP EVM has a Baseboard Management Controller (BMC) on board.
This controller handles things like setting the SoCs boot mode along
with controlling the on board character LCD display module.
Enable UART2 which communicates with the BMC. This enables userspace
applications to display something on the onboard LCD controlled by the
BMC.
Signed-off-by: Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org>
Signed-off-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
---
arch/arm/boot/dts/keystone-k2g-evm.dts | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/keystone-k2g-evm.dts b/arch/arm/boot/dts/keystone-k2g-evm.dts
index 8d100217e38f..9737730ddc21 100644
--- a/arch/arm/boot/dts/keystone-k2g-evm.dts
+++ b/arch/arm/boot/dts/keystone-k2g-evm.dts
@@ -115,6 +115,13 @@
K2G_CORE_IOPAD(0x121c) (BUFFER_CLASS_B | PULL_DISABLE | MUX_MODE0) /* qspi_csn0.qspi_csn0 */
>;
};
+
+ uart2_pins: pinmux_uart2_pins {
+ pinctrl-single,pins = <
+ K2G_CORE_IOPAD(0x11ec) (BUFFER_CLASS_B | PULL_DISABLE | MUX_MODE0) /* uart2_rxd.uart2_rxd */
+ K2G_CORE_IOPAD(0x11f0) (BUFFER_CLASS_B | PIN_PULLDOWN | MUX_MODE0) /* uart2_txd.uart2_txd */
+ >;
+ };
};
&uart0 {
@@ -263,3 +270,9 @@
};
};
};
+
+&uart2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart2_pins>;
+ status = "okay";
+};
--
2.15.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 1/2] ARM: dts: keystone-k2g: Add UART 1 and 2 instances
From: Vignesh R @ 2017-12-05 6:48 UTC (permalink / raw)
To: Santosh Shilimkar
Cc: devicetree, Vignesh R, linux-kernel, Rob Herring,
linux-arm-kernel, Franklin S Cooper Jr
In-Reply-To: <20171205064812.15758-1-vigneshr@ti.com>
From: Franklin S Cooper Jr <fcooper@ti.com>
Add DT nodes for two other UART instances of 66AK2G SoC.
Also add power domain and clock domain nodes to UART 0
Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
Signed-off-by: Vignesh R <vigneshr@ti.com>
---
arch/arm/boot/dts/keystone-k2g.dtsi | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi b/arch/arm/boot/dts/keystone-k2g.dtsi
index ef82c0a6e607..3c48a9f5c882 100644
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -28,6 +28,8 @@
aliases {
serial0 = &uart0;
+ serial1 = &uart1;
+ serial2 = &uart2;
i2c0 = &i2c0;
i2c1 = &i2c1;
i2c2 = &i2c2;
@@ -114,7 +116,32 @@
reg-io-width = <4>;
reg = <0x02530c00 0x100>;
interrupts = <GIC_SPI 164 IRQ_TYPE_EDGE_RISING>;
- clock-frequency = <200000000>;
+ clocks = <&k2g_clks 0x2c 0>;
+ power-domains = <&k2g_pds 0x2c>;
+ status = "disabled";
+ };
+
+ uart1: serial@02531000 {
+ compatible = "ti,da830-uart", "ns16550a";
+ current-speed = <115200>;
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ reg = <0x02531000 0x100>;
+ interrupts = <GIC_SPI 165 IRQ_TYPE_EDGE_RISING>;
+ clocks = <&k2g_clks 0x2d 0>;
+ power-domains = <&k2g_pds 0x2d>;
+ status = "disabled";
+ };
+
+ uart2: serial@02531400 {
+ compatible = "ti,da830-uart", "ns16550a";
+ current-speed = <115200>;
+ reg-shift = <2>;
+ reg-io-width = <4>;
+ reg = <0x02531400 0x100>;
+ interrupts = <GIC_SPI 166 IRQ_TYPE_EDGE_RISING>;
+ clocks = <&k2g_clks 0x2e 0>;
+ power-domains = <&k2g_pds 0x2e>;
status = "disabled";
};
--
2.15.0
^ permalink raw reply related
* [PATCH 0/2] 66AK2G: Add DT entry for UART1 and UART2
From: Vignesh R @ 2017-12-05 6:48 UTC (permalink / raw)
To: Santosh Shilimkar
Cc: devicetree, Vignesh R, linux-kernel, Rob Herring,
linux-arm-kernel, Franklin S Cooper Jr
This series adds DT entry for the remaining two UART instances on 66AK2G
SoC.
Based on linux-next.
Franklin S Cooper Jr (2):
ARM: dts: keystone-k2g: Add UART 1 and 2 instances
ARM: dts: keystone-k2g-evm: Enable UART 2
arch/arm/boot/dts/keystone-k2g-evm.dts | 13 +++++++++++++
arch/arm/boot/dts/keystone-k2g.dtsi | 29 ++++++++++++++++++++++++++++-
2 files changed, 41 insertions(+), 1 deletion(-)
--
2.15.0
^ permalink raw reply
* [PATCH v2 2/2] regulator: sc2731: Add regulator driver to support Spreadtrum SC2731 PMIC
From: Erick Chen @ 2017-12-05 6:35 UTC (permalink / raw)
To: broonie, robh+dt, mark.rutland, lgirdwood
Cc: linux-kernel, devicetree, baolin.wang, baolin.wang, erick.chen
In-Reply-To: <d1e3a79db9b9a0b6c3116e06b34942b92b629ccc.1512455819.git.erick.chen@spreadtrum.com>
Add regulator driver for Spreadtrum SC2731 device.
It has 17 general purpose LDOs, BUCKs generator and
digital output to control regulators.
Signed-off-by: Erick Chen <erick.chen@spreadtrum.com>
Reviewed-by: Baolin Wang <baolin.wang@spreadtrum.com>
---
Changes since v1:
- Remove of_device_id table.
- Change subsys_init() to module_init().
- Change the file's license format.
---
drivers/regulator/Kconfig | 7 +
drivers/regulator/Makefile | 1 +
drivers/regulator/sc2731-regulator.c | 256 ++++++++++++++++++++++++++++++++++
3 files changed, 264 insertions(+)
create mode 100644 drivers/regulator/sc2731-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 96cd55f..b27417c 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -744,6 +744,13 @@ config REGULATOR_S5M8767
via I2C bus. S5M8767A have 9 Bucks and 28 LDOs output and
supports DVS mode with 8bits of output voltage control.
+config REGULATOR_SC2731
+ tristate "Spreadtrum SC2731 power regulator driver"
+ depends on MFD_SC27XX_PMIC || COMPILE_TEST
+ help
+ This driver provides support for the voltage regulators on the
+ SC2731 PMIC.
+
config REGULATOR_SKY81452
tristate "Skyworks Solutions SKY81452 voltage regulator"
depends on MFD_SKY81452
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 80ffc57..19fea09 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_REGULATOR_RT5033) += rt5033-regulator.o
obj-$(CONFIG_REGULATOR_S2MPA01) += s2mpa01.o
obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
+obj-$(CONFIG_REGULATOR_SC2731) += sc2731-regulator.o
obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o
obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
diff --git a/drivers/regulator/sc2731-regulator.c b/drivers/regulator/sc2731-regulator.c
new file mode 100644
index 0000000..794fcd5
--- /dev/null
+++ b/drivers/regulator/sc2731-regulator.c
@@ -0,0 +1,256 @@
+ //SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 Spreadtrum Communications Inc.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+/*
+ * SC2731 regulator lock register
+ */
+#define SC2731_PWR_WR_PROT_VALUE 0xf0c
+#define SC2731_WR_UNLOCK 0x6e7f
+
+/*
+ * SC2731 enable register
+ */
+#define SC2731_POWER_PD_SW 0xc28
+#define SC2731_LDO_CAMA0_PD 0xcfc
+#define SC2731_LDO_CAMA1_PD 0xd04
+#define SC2731_LDO_CAMMOT_PD 0xd0c
+#define SC2731_LDO_VLDO_PD 0xd6c
+#define SC2731_LDO_EMMCCORE_PD 0xd2c
+#define SC2731_LDO_SDCORE_PD 0xd74
+#define SC2731_LDO_SDIO_PD 0xd70
+#define SC2731_LDO_WIFIPA_PD 0xd4c
+#define SC2731_LDO_USB33_PD 0xd5c
+#define SC2731_LDO_CAMD0_PD 0xd7c
+#define SC2731_LDO_CAMD1_PD 0xd84
+#define SC2731_LDO_CON_PD 0xd8c
+#define SC2731_LDO_CAMIO_PD 0xd94
+#define SC2731_LDO_SRAM_PD 0xd78
+
+/*
+ * SC2731 enable mask
+ */
+#define SC2731_DCDC_CPU0_PD_MASK BIT(4)
+#define SC2731_DCDC_CPU1_PD_MASK BIT(3)
+#define SC2731_DCDC_RF_PD_MASK BIT(11)
+#define SC2731_LDO_CAMA0_PD_MASK BIT(0)
+#define SC2731_LDO_CAMA1_PD_MASK BIT(0)
+#define SC2731_LDO_CAMMOT_PD_MASK BIT(0)
+#define SC2731_LDO_VLDO_PD_MASK BIT(0)
+#define SC2731_LDO_EMMCCORE_PD_MASK BIT(0)
+#define SC2731_LDO_SDCORE_PD_MASK BIT(0)
+#define SC2731_LDO_SDIO_PD_MASK BIT(0)
+#define SC2731_LDO_WIFIPA_PD_MASK BIT(0)
+#define SC2731_LDO_USB33_PD_MASK BIT(0)
+#define SC2731_LDO_CAMD0_PD_MASK BIT(0)
+#define SC2731_LDO_CAMD1_PD_MASK BIT(0)
+#define SC2731_LDO_CON_PD_MASK BIT(0)
+#define SC2731_LDO_CAMIO_PD_MASK BIT(0)
+#define SC2731_LDO_SRAM_PD_MASK BIT(0)
+
+/*
+ * SC2731 vsel register
+ */
+#define SC2731_DCDC_CPU0_VOL 0xc54
+#define SC2731_DCDC_CPU1_VOL 0xc64
+#define SC2731_DCDC_RF_VOL 0xcb8
+#define SC2731_LDO_CAMA0_VOL 0xd00
+#define SC2731_LDO_CAMA1_VOL 0xd08
+#define SC2731_LDO_CAMMOT_VOL 0xd10
+#define SC2731_LDO_VLDO_VOL 0xd28
+#define SC2731_LDO_EMMCCORE_VOL 0xd30
+#define SC2731_LDO_SDCORE_VOL 0xd38
+#define SC2731_LDO_SDIO_VOL 0xd40
+#define SC2731_LDO_WIFIPA_VOL 0xd50
+#define SC2731_LDO_USB33_VOL 0xd60
+#define SC2731_LDO_CAMD0_VOL 0xd80
+#define SC2731_LDO_CAMD1_VOL 0xd88
+#define SC2731_LDO_CON_VOL 0xd90
+#define SC2731_LDO_CAMIO_VOL 0xd98
+#define SC2731_LDO_SRAM_VOL 0xdB0
+
+/*
+ * SC2731 vsel register mask
+ */
+#define SC2731_DCDC_CPU0_VOL_MASK GENMASK(8, 0)
+#define SC2731_DCDC_CPU1_VOL_MASK GENMASK(8, 0)
+#define SC2731_DCDC_RF_VOL_MASK GENMASK(8, 0)
+#define SC2731_LDO_CAMA0_VOL_MASK GENMASK(7, 0)
+#define SC2731_LDO_CAMA1_VOL_MASK GENMASK(7, 0)
+#define SC2731_LDO_CAMMOT_VOL_MASK GENMASK(7, 0)
+#define SC2731_LDO_VLDO_VOL_MASK GENMASK(7, 0)
+#define SC2731_LDO_EMMCCORE_VOL_MASK GENMASK(7, 0)
+#define SC2731_LDO_SDCORE_VOL_MASK GENMASK(7, 0)
+#define SC2731_LDO_SDIO_VOL_MASK GENMASK(7, 0)
+#define SC2731_LDO_WIFIPA_VOL_MASK GENMASK(7, 0)
+#define SC2731_LDO_USB33_VOL_MASK GENMASK(7, 0)
+#define SC2731_LDO_CAMD0_VOL_MASK GENMASK(6, 0)
+#define SC2731_LDO_CAMD1_VOL_MASK GENMASK(6, 0)
+#define SC2731_LDO_CON_VOL_MASK GENMASK(6, 0)
+#define SC2731_LDO_CAMIO_VOL_MASK GENMASK(6, 0)
+#define SC2731_LDO_SRAM_VOL_MASK GENMASK(6, 0)
+
+enum sc2731_regulator_id {
+ SC2731_BUCK_CPU0,
+ SC2731_BUCK_CPU1,
+ SC2731_BUCK_RF,
+ SC2731_LDO_CAMA0,
+ SC2731_LDO_CAMA1,
+ SC2731_LDO_CAMMOT,
+ SC2731_LDO_VLDO,
+ SC2731_LDO_EMMCCORE,
+ SC2731_LDO_SDCORE,
+ SC2731_LDO_SDIO,
+ SC2731_LDO_WIFIPA,
+ SC2731_LDO_USB33,
+ SC2731_LDO_CAMD0,
+ SC2731_LDO_CAMD1,
+ SC2731_LDO_CON,
+ SC2731_LDO_CAMIO,
+ SC2731_LDO_SRAM,
+};
+
+static const struct regulator_ops sc2731_regu_linear_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_linear,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+};
+
+#define SC2731_REGU_LINEAR(_id, en_reg, en_mask, vreg, vmask, \
+ vstep, vmin, vmax) { \
+ .name = #_id, \
+ .of_match = of_match_ptr(#_id), \
+ .ops = &sc2731_regu_linear_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .id = SC2731_##_id, \
+ .owner = THIS_MODULE, \
+ .min_uV = vmin, \
+ .n_voltages = ((vmax) - (vmin)) / (vstep) + 1, \
+ .uV_step = vstep, \
+ .enable_is_inverted = true, \
+ .enable_val = 0, \
+ .enable_reg = en_reg, \
+ .enable_mask = en_mask, \
+ .vsel_reg = vreg, \
+ .vsel_mask = vmask, \
+}
+
+static struct regulator_desc regulators[] = {
+ SC2731_REGU_LINEAR(BUCK_CPU0, SC2731_POWER_PD_SW,
+ SC2731_DCDC_CPU0_PD_MASK, SC2731_DCDC_CPU0_VOL,
+ SC2731_DCDC_CPU0_VOL_MASK, 3125, 400000, 1996875),
+ SC2731_REGU_LINEAR(BUCK_CPU1, SC2731_POWER_PD_SW,
+ SC2731_DCDC_CPU1_PD_MASK, SC2731_DCDC_CPU1_VOL,
+ SC2731_DCDC_CPU1_VOL_MASK, 3125, 400000, 1996875),
+ SC2731_REGU_LINEAR(BUCK_RF, SC2731_POWER_PD_SW, SC2731_DCDC_RF_PD_MASK,
+ SC2731_DCDC_RF_VOL, SC2731_DCDC_RF_VOL_MASK,
+ 3125, 600000, 2196875),
+ SC2731_REGU_LINEAR(LDO_CAMA0, SC2731_LDO_CAMA0_PD,
+ SC2731_LDO_CAMA0_PD_MASK, SC2731_LDO_CAMA0_VOL,
+ SC2731_LDO_CAMA0_VOL_MASK, 10000, 1200000, 3750000),
+ SC2731_REGU_LINEAR(LDO_CAMA1, SC2731_LDO_CAMA1_PD,
+ SC2731_LDO_CAMA1_PD_MASK, SC2731_LDO_CAMA1_VOL,
+ SC2731_LDO_CAMA1_VOL_MASK, 10000, 1200000, 3750000),
+ SC2731_REGU_LINEAR(LDO_CAMMOT, SC2731_LDO_CAMMOT_PD,
+ SC2731_LDO_CAMMOT_PD_MASK, SC2731_LDO_CAMMOT_VOL,
+ SC2731_LDO_CAMMOT_VOL_MASK, 10000, 1200000, 3750000),
+ SC2731_REGU_LINEAR(LDO_VLDO, SC2731_LDO_VLDO_PD,
+ SC2731_LDO_VLDO_PD_MASK, SC2731_LDO_VLDO_VOL,
+ SC2731_LDO_VLDO_VOL_MASK, 10000, 1200000, 3750000),
+ SC2731_REGU_LINEAR(LDO_EMMCCORE, SC2731_LDO_EMMCCORE_PD,
+ SC2731_LDO_EMMCCORE_PD_MASK, SC2731_LDO_EMMCCORE_VOL,
+ SC2731_LDO_EMMCCORE_VOL_MASK, 10000, 1200000,
+ 3750000),
+ SC2731_REGU_LINEAR(LDO_SDCORE, SC2731_LDO_SDCORE_PD,
+ SC2731_LDO_SDCORE_PD_MASK, SC2731_LDO_SDCORE_VOL,
+ SC2731_LDO_SDCORE_VOL_MASK, 10000, 1200000, 3750000),
+ SC2731_REGU_LINEAR(LDO_SDIO, SC2731_LDO_SDIO_PD,
+ SC2731_LDO_SDIO_PD_MASK, SC2731_LDO_SDIO_VOL,
+ SC2731_LDO_SDIO_VOL_MASK, 10000, 1200000, 3750000),
+ SC2731_REGU_LINEAR(LDO_WIFIPA, SC2731_LDO_WIFIPA_PD,
+ SC2731_LDO_WIFIPA_PD_MASK, SC2731_LDO_WIFIPA_VOL,
+ SC2731_LDO_WIFIPA_VOL_MASK, 10000, 1200000, 3750000),
+ SC2731_REGU_LINEAR(LDO_USB33, SC2731_LDO_USB33_PD,
+ SC2731_LDO_USB33_PD_MASK, SC2731_LDO_USB33_VOL,
+ SC2731_LDO_USB33_VOL_MASK, 10000, 1200000, 3750000),
+ SC2731_REGU_LINEAR(LDO_CAMD0, SC2731_LDO_CAMD0_PD,
+ SC2731_LDO_CAMD0_PD_MASK, SC2731_LDO_CAMD0_VOL,
+ SC2731_LDO_CAMD0_VOL_MASK, 6250, 1000000, 1793750),
+ SC2731_REGU_LINEAR(LDO_CAMD1, SC2731_LDO_CAMD1_PD,
+ SC2731_LDO_CAMD1_PD_MASK, SC2731_LDO_CAMD1_VOL,
+ SC2731_LDO_CAMD1_VOL_MASK, 6250, 1000000, 1793750),
+ SC2731_REGU_LINEAR(LDO_CON, SC2731_LDO_CON_PD,
+ SC2731_LDO_CON_PD_MASK, SC2731_LDO_CON_VOL,
+ SC2731_LDO_CON_VOL_MASK, 6250, 1000000, 1793750),
+ SC2731_REGU_LINEAR(LDO_CAMIO, SC2731_LDO_CAMIO_PD,
+ SC2731_LDO_CAMIO_PD_MASK, SC2731_LDO_CAMIO_VOL,
+ SC2731_LDO_CAMIO_VOL_MASK, 6250, 1000000, 1793750),
+ SC2731_REGU_LINEAR(LDO_SRAM, SC2731_LDO_SRAM_PD,
+ SC2731_LDO_SRAM_PD_MASK, SC2731_LDO_SRAM_VOL,
+ SC2731_LDO_SRAM_VOL_MASK, 6250, 1000000, 1793750),
+};
+
+static int sc2731_regulator_unlock(struct regmap *regmap)
+{
+ return regmap_write(regmap, SC2731_PWR_WR_PROT_VALUE,
+ SC2731_WR_UNLOCK);
+}
+
+static int sc2731_regulator_probe(struct platform_device *pdev)
+{
+ int i, ret;
+ struct regmap *regmap;
+ struct regulator_config config = { };
+ struct regulator_dev *rdev;
+
+ regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!regmap) {
+ dev_err(&pdev->dev, "failed to get regmap.\n");
+ return -ENODEV;
+ }
+
+ ret = sc2731_regulator_unlock(regmap);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to release regulator lock\n");
+ return ret;
+ }
+
+ config.dev = &pdev->dev;
+ config.regmap = regmap;
+
+ for (i = 0; i < ARRAY_SIZE(regulators); i++) {
+ rdev = devm_regulator_register(&pdev->dev, ®ulators[i],
+ &config);
+ if (IS_ERR(rdev)) {
+ dev_err(&pdev->dev, "failed to register regulator %s\n",
+ regulators[i].name);
+ return PTR_ERR(rdev);
+ }
+ }
+
+ return 0;
+}
+
+static struct platform_driver sc2731_regulator_driver = {
+ .driver = {
+ .name = "sc27xx-regulator",
+ },
+ .probe = sc2731_regulator_probe,
+};
+
+module_platform_driver(sc2731_regulator_driver);
+
+MODULE_AUTHOR("Chen Junhui <erick.chen@spreadtrum.com>");
+MODULE_DESCRIPTION("Spreadtrum SC2731 regulator driver");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 1/2] dt-bindings: regulator: Add Spreadtrum SC2731 regulator documentation
From: Erick Chen @ 2017-12-05 6:35 UTC (permalink / raw)
To: broonie-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, lgirdwood-Re5JQEeQqe8AvxtiuMwx3w
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
baolin.wang-QSEj5FYQhm4dnm+yROfE0A,
baolin.wang-lxIno14LUO0EEoCn2XhGlw,
erick.chen-lxIno14LUO0EEoCn2XhGlw
This patch adds support for the Spreadtrum SC2731
voltage regulator device.
Signed-off-by: Erick Chen <erick.chen-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
---
Changes since v1:
- Remove reg property.
- Remove regulators sub-node.
---
.../bindings/regulator/sprd,sc2731-regulator.txt | 43 ++++++++++++++++++++
1 file changed, 43 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/sprd,sc2731-regulator.txt
diff --git a/Documentation/devicetree/bindings/regulator/sprd,sc2731-regulator.txt b/Documentation/devicetree/bindings/regulator/sprd,sc2731-regulator.txt
new file mode 100644
index 0000000..63dc078
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/sprd,sc2731-regulator.txt
@@ -0,0 +1,43 @@
+Spreadtrum SC2731 Voltage regulators
+
+The SC2731 integrates low-voltage and low quiescent current DCDC/LDO.
+14 LDO and 3 DCDCs are designed for external use. All DCDCs/LDOs have
+their own bypass (power-down) control signals. External tantalum or MLCC
+ceramic capacitors are recommended to use with these LDOs.
+
+Required properties:
+ - compatible: should be "sprd,sc27xx-regulator".
+
+List of regulators provided by this controller. It is named according to
+its regulator type, BUCK_<name> and LDO_<name>. The definition for each
+of these nodes is defined using the standard binding for regulators at
+Documentation/devicetree/bindings/regulator/regulator.txt.
+
+The valid names for regulators are:
+BUCK:
+ BUCK_CPU0, BUCK_CPU1, BUCK_RF
+LDO:
+ LDO_CAMA0, LDO_CAMA1, LDO_CAMMOT, LDO_VLDO, LDO_EMMCCORE, LDO_SDCORE,
+ LDO_SDIO, LDO_WIFIPA, LDO_USB33, LDO_CAMD0, LDO_CAMD1, LDO_CON,
+ LDO_CAMIO, LDO_SRAM
+
+Example:
+ regulators {
+ compatible = "sprd,sc27xx-regulator";
+
+ vddarm0: BUCK_CPU0 {
+ regulator-name = "vddarm0";
+ regulator-min-microvolt = <400000>;
+ regulator-max-microvolt = <1996875>;
+ regulator-ramp-delay = <25000>;
+ regulator-always-on;
+ };
+
+ vddcama0: LDO_CAMA0 {
+ regulator-name = "vddcama0";
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <3750000>;
+ regulator-enable-ramp-delay = <100>;
+ };
+ ...
+ };
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH] arm64: dts: allwinner: a64: bananapi-m64: Add LED device node
From: Chen-Yu Tsai @ 2017-12-05 6:19 UTC (permalink / raw)
To: Jagan Teki
Cc: Chen-Yu Tsai, Maxime Ripard, devicetree, linux-arm-kernel,
linux-sunxi
In-Reply-To: <CAMty3ZC2-sVbCgzKV3nZQfG4CtNXJdntuPhjyr_Wbn1xXJc=Dw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Dec 5, 2017 at 2:15 PM, Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
> On Fri, Dec 1, 2017 at 9:57 PM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
>> The Bananapi-M64 has 3 LEDS in red, green, and blue. These are toggled
>> via GPIO lines, which drive transistors that control current across the
>> LEDS. The red LED is by default on, via an additional pull-up on the
>> control line. We consider this means that it is a power indicator.
>> So we set the "default-on" property for it.
>>
>> The pingroups the GPIO lines belong to require external regulators be
>> enabled to be able to drive the GPIO high. These regulators also have
>> other purposes. However the pin controller does not have bindings for
>> regulators. Here we just set them to always-on.
>>
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>> .../boot/dts/allwinner/sun50i-a64-bananapi-m64.dts | 36 ++++++++++++++++++++++
>> 1 file changed, 36 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
>> index 4a8d3f83a36e..a6975670cd1c 100644
>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
>> @@ -60,6 +60,26 @@
>> stdout-path = "serial0:115200n8";
>> };
>>
>> + leds {
>> + compatible = "gpio-leds";
>> +
>> + pwr-led {
>> + label = "bananapi-m64:red:pwr";
>> + gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* PD24 */
>> + default-state = "on";
>> + };
>> +
>> + green {
>> + label = "bananapi-m64:green:user";
>> + gpios = <&pio 4 14 GPIO_ACTIVE_HIGH>; /* PE14 */
>> + };
>> +
>> + blue {
>> + label = "bananapi-m64:blue:user";
>> + gpios = <&pio 4 15 GPIO_ACTIVE_HIGH>; /* PE15 */
>> + };
>> + };
>> +
>> wifi_pwrseq: wifi_pwrseq {
>> compatible = "mmc-pwrseq-simple";
>> reset-gpios = <&r_pio 0 2 GPIO_ACTIVE_LOW>; /* PL2 */
>> @@ -153,6 +173,17 @@
>>
>> #include "axp803.dtsi"
>>
>> +®_aldo1 {
>> + /*
>> + * This regulator also drives the PE pingroup GPIOs,
>> + * which also controls two LEDs.
>> + */
>> + regulator-always-on;
>> + regulator-min-microvolt = <2800000>;
>> + regulator-max-microvolt = <2800000>;
>
> why 2.8v?
That is what is written on the schematics. Plus it seems camera
modules are meant to use 2.8v.
>
>> + regulator-name = "afvcc-csi";
>> +};
>> +
>> ®_aldo2 {
>> regulator-always-on;
>> regulator-min-microvolt = <1800000>;
>> @@ -168,6 +199,11 @@
>> };
>>
>> ®_dc1sw {
>> + /*
>> + * This regulator also indirectly drives the PD pingroup GPIOs,
>> + * which also controls the power LED.
>> + */
>> + regulator-always-on;
>
> This make rgmii phy always on, does this make emac power-drop?
emac power-drop? Please elaborate.
ChenYu
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] arm64: dts: allwinner: a64: bananapi-m64: Add LED device node
From: Jagan Teki @ 2017-12-05 6:15 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: Maxime Ripard, devicetree, linux-arm-kernel, linux-sunxi
In-Reply-To: <20171201162733.23156-1-wens-jdAy2FN1RRM@public.gmane.org>
On Fri, Dec 1, 2017 at 9:57 PM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> The Bananapi-M64 has 3 LEDS in red, green, and blue. These are toggled
> via GPIO lines, which drive transistors that control current across the
> LEDS. The red LED is by default on, via an additional pull-up on the
> control line. We consider this means that it is a power indicator.
> So we set the "default-on" property for it.
>
> The pingroups the GPIO lines belong to require external regulators be
> enabled to be able to drive the GPIO high. These regulators also have
> other purposes. However the pin controller does not have bindings for
> regulators. Here we just set them to always-on.
>
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> ---
> .../boot/dts/allwinner/sun50i-a64-bananapi-m64.dts | 36 ++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> index 4a8d3f83a36e..a6975670cd1c 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> @@ -60,6 +60,26 @@
> stdout-path = "serial0:115200n8";
> };
>
> + leds {
> + compatible = "gpio-leds";
> +
> + pwr-led {
> + label = "bananapi-m64:red:pwr";
> + gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* PD24 */
> + default-state = "on";
> + };
> +
> + green {
> + label = "bananapi-m64:green:user";
> + gpios = <&pio 4 14 GPIO_ACTIVE_HIGH>; /* PE14 */
> + };
> +
> + blue {
> + label = "bananapi-m64:blue:user";
> + gpios = <&pio 4 15 GPIO_ACTIVE_HIGH>; /* PE15 */
> + };
> + };
> +
> wifi_pwrseq: wifi_pwrseq {
> compatible = "mmc-pwrseq-simple";
> reset-gpios = <&r_pio 0 2 GPIO_ACTIVE_LOW>; /* PL2 */
> @@ -153,6 +173,17 @@
>
> #include "axp803.dtsi"
>
> +®_aldo1 {
> + /*
> + * This regulator also drives the PE pingroup GPIOs,
> + * which also controls two LEDs.
> + */
> + regulator-always-on;
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
why 2.8v?
> + regulator-name = "afvcc-csi";
> +};
> +
> ®_aldo2 {
> regulator-always-on;
> regulator-min-microvolt = <1800000>;
> @@ -168,6 +199,11 @@
> };
>
> ®_dc1sw {
> + /*
> + * This regulator also indirectly drives the PD pingroup GPIOs,
> + * which also controls the power LED.
> + */
> + regulator-always-on;
This make rgmii phy always on, does this make emac power-drop?
--
Jagan Teki
Senior Linux Kernel Engineer | Amarula Solutions
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2] ARM: dts: sun8i: h3: Add dts file for Libre Computer Board ALL-H3-CC H3 ver.
From: Chen-Yu Tsai @ 2017-12-05 6:06 UTC (permalink / raw)
To: Maxime Ripard
Cc: Chen-Yu Tsai, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
The Libre Computer Board ALL-H3-CC from Libre Technology is a Raspberry
Pi B+ form factor single board computer based on the Allwinner H3 SoC.
The board has 1GB DDR3 SDRAM, provided by 4 2Gb chips. The mounting holes
and connectors are in the exact same position as on the Raspberry Pi B+.
Raspberry Pi B+ like peripherals supported on this board include:
- Power input through micro-USB connector (without USB OTG)
- Native 100 Mbps ethernet using the internal PHY, as opposed to
USB-based on the RPi
- 4x USB 2.0 host ports, directly connected to the SoC, as opposed to
being connected through a USB 2.0 hub on the RPi
- TV and audio output on a 3.5mm TRRS jack
- HDMI output
- Micro-SD card slot
- Standard RPi B+ GPIO header, with the standard peripherals routed to
the same pins.
* 5V, 3.3V power, and ground
* I2C0 on the H3 is routed to I2C1 pins on the RPi header
* I2C1 on the H3 is routed to I2C0 pins on the RPi header
* UART1 on the H3 is routed to UART0 pins on the RPi header
* SPI0 on the H3 is routed to SPI0 pins on the RPi header,
with GPIO pin PA17 replacing the missing Chip Select 1
* I2S1 on the H3 is routed to PCM pins on the RPi header
- Additional peripherals from the H3 are available on different pins.
These include I2S0, JTAG, PWM1, SPDIF, SPI1, and UART3
In addition, there are a number of new features:
- Console UART header
- Consumer IR receiver
- Camera interface (not compatible with RPi)
- Onboard microphone
- eMMC expansion module port
- Heatsink mounting holes
- Power button
The power button requires corresponding software for the embedded
coprocessor to properly function.
This patch adds a dts file for this board that enables all "onboard"
peripherals currently supported. This means no display or camera
support.
Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
Changes since v1:
- Changed board compatible string
- Added LED color to LED labels
- Changed button label and key event to power button
- Added description of power button to commit message
There are two other variants [1] of this board, with different SoCs and
DRAM configuration. But the board remains the same, thanks to the SoCs
being pin compatible.
The EMAC/EPHY LED polarity driver issue has been fixed in v4.15-rc2. But
there was discussion to move this property under the PHY node instead.
[1] https://www.kickstarter.com/projects/librecomputer/libre-computer-board-tritium-sbc-linux-android-7-n
---
arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/sun8i-h3-libretech-all-h3-cc.dts | 196 +++++++++++++++++++++
2 files changed, 197 insertions(+)
create mode 100644 arch/arm/boot/dts/sun8i-h3-libretech-all-h3-cc.dts
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 3c139c63098f..aa68f719692f 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -953,6 +953,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
sun8i-h2-plus-orangepi-zero.dtb \
sun8i-h3-bananapi-m2-plus.dtb \
sun8i-h3-beelink-x2.dtb \
+ sun8i-h3-libretech-all-h3-cc.dtb \
sun8i-h3-nanopi-m1.dtb \
sun8i-h3-nanopi-m1-plus.dtb \
sun8i-h3-nanopi-neo.dtb \
diff --git a/arch/arm/boot/dts/sun8i-h3-libretech-all-h3-cc.dts b/arch/arm/boot/dts/sun8i-h3-libretech-all-h3-cc.dts
new file mode 100644
index 000000000000..d406571a0dd6
--- /dev/null
+++ b/arch/arm/boot/dts/sun8i-h3-libretech-all-h3-cc.dts
@@ -0,0 +1,196 @@
+/*
+ * Copyright (C) 2017 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
+ *
+ * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+ */
+
+/dts-v1/;
+#include "sun8i-h3.dtsi"
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+
+/ {
+ model = "Libre Computer Board ALL-H3-CC H3";
+ compatible = "libretech,all-h3-cc-h3", "allwinner,sun8i-h3";
+
+ aliases {
+ ethernet0 = &emac;
+ serial0 = &uart0;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ leds {
+ compatible = "gpio-leds";
+
+ pwr_led {
+ label = "librecomputer:green:pwr";
+ gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>; /* PL10 */
+ default-state = "on";
+ };
+
+ status_led {
+ label = "librecomputer:blue:status";
+ gpios = <&pio 0 7 GPIO_ACTIVE_HIGH>; /* PA7 */
+ };
+ };
+
+ gpio_keys {
+ compatible = "gpio-keys";
+
+ power {
+ label = "power";
+ linux,code = <KEY_POWER>;
+ gpios = <&r_pio 0 2 GPIO_ACTIVE_LOW>; /* PL2 */
+ };
+ };
+
+ reg_vcc1v2: vcc1v2 {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc1v2";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ regulator-boot-on;
+ vin-supply = <®_vcc5v0>;
+ gpio = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
+ enable-active-high;
+ };
+
+ reg_vcc3v3: vcc3v3 {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc3v3";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ vin-supply = <®_vcc5v0>;
+ };
+
+ /* This represents the board's 5V input */
+ reg_vcc5v0: vcc5v0 {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc5v0";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ };
+
+ reg_vcc_dram: vcc-dram {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc-dram";
+ regulator-min-microvolt = <1500000>;
+ regulator-max-microvolt = <1500000>;
+ regulator-always-on;
+ regulator-boot-on;
+ vin-supply = <®_vcc5v0>;
+ gpio = <&r_pio 0 9 GPIO_ACTIVE_HIGH>; /* PL9 */
+ enable-active-high;
+ };
+
+ reg_vcc_io: vcc-io {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc-io";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ regulator-boot-on;
+ vin-supply = <®_vcc3v3>;
+ gpio = <&r_pio 0 5 GPIO_ACTIVE_LOW>; /* PL5 */
+ };
+
+ reg_vdd_cpux: vdd-cpux {
+ compatible = "regulator-fixed";
+ regulator-name = "vdd-cpux";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ regulator-boot-on;
+ vin-supply = <®_vcc5v0>;
+ gpio = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /* PL8 */
+ enable-active-high;
+ };
+};
+
+&codec {
+ allwinner,audio-routing =
+ "Line Out", "LINEOUT",
+ "MIC1", "Mic",
+ "Mic", "MBIAS";
+ status = "okay";
+};
+
+&ehci0 {
+ status = "okay";
+};
+
+&ehci1 {
+ status = "okay";
+};
+
+&ehci2 {
+ status = "okay";
+};
+
+&ehci3 {
+ status = "okay";
+};
+
+&emac {
+ phy-handle = <&int_mii_phy>;
+ phy-mode = "mii";
+ allwinner,leds-active-low;
+ status = "okay";
+};
+
+&ir {
+ pinctrl-names = "default";
+ pinctrl-0 = <&ir_pins_a>;
+ status = "okay";
+};
+
+&mmc0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&mmc0_pins_a>;
+ vmmc-supply = <®_vcc_io>;
+ bus-width = <4>;
+ cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
+ cd-inverted;
+ status = "okay";
+};
+
+&ohci0 {
+ status = "okay";
+};
+
+&ohci1 {
+ status = "okay";
+};
+
+&ohci2 {
+ status = "okay";
+};
+
+&ohci3 {
+ status = "okay";
+};
+
+&uart0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart0_pins_a>;
+ status = "okay";
+};
+
+&usb_otg {
+ dr_mode = "host";
+ status = "okay";
+};
+
+&usbphy {
+ /* VBUS on USB ports are always on */
+ usb0_vbus-supply = <®_vcc5v0>;
+ usb1_vbus-supply = <®_vcc5v0>;
+ usb2_vbus-supply = <®_vcc5v0>;
+ usb3_vbus-supply = <®_vcc5v0>;
+ status = "okay";
+};
--
2.15.0
^ permalink raw reply related
* Re: [PATCH v10 3/6] clk: qcom: Add A53 PLL support
From: Bjorn Andersson @ 2017-12-05 6:03 UTC (permalink / raw)
To: Georgi Djakov
Cc: sboyd, jassisinghbrar, robh, mturquette, linux-clk, linux-kernel,
linux-arm-msm, devicetree
In-Reply-To: <20171201170224.25053-4-georgi.djakov@linaro.org>
On Fri 01 Dec 09:02 PST 2017, Georgi Djakov wrote:
> The CPUs on Qualcomm MSM8916-based platforms are clocked by two PLLs,
> a primary (A53) CPU PLL and a secondary fixed-rate GPLL0. These sources
> are connected to a mux and half-integer divider, which is feeding the
> CPU cores.
>
> This patch adds support for the primary CPU PLL which generates the
> higher range of frequencies above 1GHz.
>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> .../devicetree/bindings/clock/qcom,a53pll.txt | 22 +++++
> drivers/clk/qcom/Kconfig | 10 ++
> drivers/clk/qcom/Makefile | 1 +
> drivers/clk/qcom/a53-pll.c | 109 +++++++++++++++++++++
> 4 files changed, 142 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53pll.txt
> create mode 100644 drivers/clk/qcom/a53-pll.c
>
> diff --git a/Documentation/devicetree/bindings/clock/qcom,a53pll.txt b/Documentation/devicetree/bindings/clock/qcom,a53pll.txt
> new file mode 100644
> index 000000000000..e3fa8118eaee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,a53pll.txt
> @@ -0,0 +1,22 @@
> +Qualcomm MSM8916 A53 PLL Binding
> +--------------------------------
> +The A53 PLL on MSM8916 platforms is the main CPU PLL used used for frequencies
> +above 1GHz.
> +
> +Required properties :
> +- compatible : Shall contain only one of the following:
> +
> + "qcom,msm8916-a53pll"
> +
> +- reg : shall contain base register location and length
> +
> +- #clock-cells : must be set to <0>
> +
> +Example:
> +
> + a53pll: clock@b016000 {
> + compatible = "qcom,msm8916-a53pll";
> + reg = <0xb016000 0x40>;
> + #clock-cells = <0>;
> + };
> +
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 9f6c278deead..81ac7b9378fe 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -12,6 +12,16 @@ config COMMON_CLK_QCOM
> select REGMAP_MMIO
> select RESET_CONTROLLER
>
> +config QCOM_A53PLL
> + bool "MSM8916 A53 PLL"
> + depends on COMMON_CLK_QCOM
> + default ARCH_QCOM
> + help
> + Support for the A53 PLL on MSM8916 devices. It provides
> + the CPU with frequencies above 1GHz.
> + Say Y if you want to support higher CPU frequencies on MSM8916
> + devices.
> +
> config QCOM_CLK_RPM
> tristate "RPM based Clock Controller"
> depends on COMMON_CLK_QCOM && MFD_QCOM_RPM
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 26410d31446b..e767c60c24ec 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -32,5 +32,6 @@ obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o
> obj-$(CONFIG_MSM_MMCC_8960) += mmcc-msm8960.o
> obj-$(CONFIG_MSM_MMCC_8974) += mmcc-msm8974.o
> obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
> +obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
> obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
> obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
> diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> new file mode 100644
> index 000000000000..b2bb8e9437f1
> --- /dev/null
> +++ b/drivers/clk/qcom/a53-pll.c
> @@ -0,0 +1,109 @@
> +/*
> + * Copyright (c) 2017, Linaro Limited
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "clk-pll.h"
> +#include "clk-regmap.h"
> +
> +static const struct pll_freq_tbl a53pll_freq[] = {
> + { 998400000, 52, 0x0, 0x1, 0 },
> + { 1094400000, 57, 0x0, 0x1, 0 },
> + { 1152000000, 62, 0x0, 0x1, 0 },
> + { 1209600000, 63, 0x0, 0x1, 0 },
> + { 1248000000, 65, 0x0, 0x1, 0 },
> + { 1363200000, 71, 0x0, 0x1, 0 },
> + { 1401600000, 73, 0x0, 0x1, 0 },
> +};
> +
> +static const struct regmap_config a53pll_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .max_register = 0x40,
> + .fast_io = true,
> +};
> +
> +static int qcom_a53pll_remove(struct platform_device *pdev)
> +{
> + of_clk_del_provider(pdev->dev.of_node);
> + return 0;
> +}
> +
> +static int qcom_a53pll_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct regmap *regmap;
> + struct resource *res;
> + struct clk_pll *pll;
> + void __iomem *base;
> + struct clk_init_data init = { };
> + int ret;
> +
> + pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> + if (!pll)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + regmap = devm_regmap_init_mmio(dev, base, &a53pll_regmap_config);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + pll->l_reg = 0x04;
> + pll->m_reg = 0x08;
> + pll->n_reg = 0x0c;
> + pll->config_reg = 0x14;
> + pll->mode_reg = 0x00;
> + pll->status_reg = 0x1c;
> + pll->status_bit = 16;
> + pll->freq_tbl = a53pll_freq;
> +
> + init.name = "a53pll";
> + init.parent_names = (const char *[]){ "xo" };
> + init.num_parents = 1;
> + init.ops = &clk_pll_sr2_ops;
> + init.flags = CLK_IS_CRITICAL;
> + pll->clkr.hw.init = &init;
> +
> + ret = devm_clk_register_regmap(dev, &pll->clkr);
> + if (ret) {
> + dev_err(dev, "failed to register regmap clock: %d\n", ret);
> + return ret;
> + }
> +
> + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get,
> + &pll->clkr.hw);
> + if (ret) {
> + dev_err(dev, "failed to add clock provider: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id qcom_a53pll_match_table[] = {
> + { .compatible = "qcom,msm8916-a53pll" },
> + { }
> +};
> +
> +static struct platform_driver qcom_a53pll_driver = {
> + .probe = qcom_a53pll_probe,
> + .remove = qcom_a53pll_remove,
> + .driver = {
> + .name = "qcom-a53pll",
> + .of_match_table = qcom_a53pll_match_table,
> + },
> +};
> +
> +builtin_platform_driver(qcom_a53pll_driver);
^ permalink raw reply
* Re: [PATCH v10 2/6] mailbox: qcom: Create APCS child device for clock controller
From: Bjorn Andersson @ 2017-12-05 6:01 UTC (permalink / raw)
To: Georgi Djakov
Cc: sboyd, jassisinghbrar, robh, mturquette, linux-clk, linux-kernel,
linux-arm-msm, devicetree
In-Reply-To: <20171201170224.25053-3-georgi.djakov@linaro.org>
On Fri 01 Dec 09:02 PST 2017, Georgi Djakov wrote:
> There is a clock controller functionality provided by the APCS hardware
> block of msm8916 devices. The device-tree would represent an APCS node
> with both mailbox and clock provider properties.
> Create a platform child device for the clock controller functionality so
> the driver can probe and use APCS as parent.
>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> drivers/mailbox/qcom-apcs-ipc-mailbox.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> index ab344bc6fa63..57bde0dfd12f 100644
> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> @@ -29,6 +29,7 @@ struct qcom_apcs_ipc {
>
> struct regmap *regmap;
> unsigned long offset;
> + struct platform_device *clk;
> };
>
> static const struct regmap_config apcs_regmap_config = {
> @@ -96,6 +97,14 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
> return ret;
> }
>
> + if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global")) {
> + apcs->clk = platform_device_register_data(&pdev->dev,
> + "qcom-apcs-msm8916-clk",
> + -1, NULL, 0);
> + if (IS_ERR(apcs->clk))
> + dev_err(&pdev->dev, "failed to register APCS clk\n");
> + }
> +
> platform_set_drvdata(pdev, apcs);
>
> return 0;
> @@ -104,8 +113,10 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
> static int qcom_apcs_ipc_remove(struct platform_device *pdev)
> {
> struct qcom_apcs_ipc *apcs = platform_get_drvdata(pdev);
> + struct platform_device *clk = apcs->clk;
>
> mbox_controller_unregister(&apcs->mbox);
> + platform_device_unregister(clk);
>
> return 0;
> }
^ permalink raw reply
* Re: [PATCH v10 6/6] clk: qcom: Add APCS clock controller support
From: Bjorn Andersson @ 2017-12-05 6:01 UTC (permalink / raw)
To: Georgi Djakov
Cc: sboyd, jassisinghbrar, robh, mturquette, linux-clk, linux-kernel,
linux-arm-msm, devicetree
In-Reply-To: <20171201170224.25053-7-georgi.djakov@linaro.org>
On Fri 01 Dec 09:02 PST 2017, Georgi Djakov wrote:
[..]
> diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
> new file mode 100644
> index 000000000000..f71039ff2347
> --- /dev/null
> +++ b/drivers/clk/qcom/apcs-msm8916.c
> @@ -0,0 +1,149 @@
> +/*
> + * Qualcomm APCS clock controller driver
> + *
> + * Copyright (c) 2017, Linaro Limited
> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
> + *
> + * SPDX-License-Identifier: GPL-2.0
The SPDX-License-Identifier should be on the first line in the file,
commented by //
> + */
> +
[..]
> +static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
> +{
> + struct device *dev = pdev->dev.parent;
Call this "parent" instead.
> + struct device_node *np = dev->of_node;
> + struct clk_regmap_mux_div *a53cc;
> + struct regmap *regmap;
> + struct clk_init_data init = { };
> + int ret;
> +
> + regmap = dev_get_regmap(dev, NULL);
> + if (IS_ERR(regmap)) {
> + ret = PTR_ERR(regmap);
> + dev_err(dev, "failed to get regmap: %d\n", ret);
dev_* prints should be on &pdev->dev and not on parent device.
> + return ret;
> + }
> +
> + a53cc = devm_kzalloc(dev, sizeof(*a53cc), GFP_KERNEL);
Perform this allocation on behalf of this device (i.e. &pdev->dev and
not parent)
> + if (!a53cc)
> + return -ENOMEM;
> +
> + init.name = "a53mux";
> + init.parent_names = gpll0_a53cc;
> + init.num_parents = ARRAY_SIZE(gpll0_a53cc);
> + init.ops = &clk_regmap_mux_div_ops;
> + init.flags = CLK_SET_RATE_PARENT;
> +
> + a53cc->clkr.hw.init = &init;
> + a53cc->clkr.regmap = regmap;
> + a53cc->reg_offset = 0x50;
> + a53cc->hid_width = 5;
> + a53cc->hid_shift = 0;
> + a53cc->src_width = 3;
> + a53cc->src_shift = 8;
> + a53cc->parent_map = gpll0_a53cc_map;
> +
> + a53cc->pclk = devm_clk_get(dev, NULL);
> + if (IS_ERR(a53cc->pclk)) {
> + ret = PTR_ERR(a53cc->pclk);
> + dev_err(dev, "failed to get clk: %d\n", ret);
> + return ret;
> + }
> +
> + a53cc->clk_nb.notifier_call = a53cc_notifier_cb;
> + ret = clk_notifier_register(a53cc->pclk, &a53cc->clk_nb);
> + if (ret) {
> + dev_err(dev, "failed to register clock notifier: %d\n", ret);
> + return ret;
> + }
> +
> + ret = devm_clk_register_regmap(dev, &a53cc->clkr);
This you can do on the &pdev->dev, it won't find a regmap on this node
and will try the parent.
> + if (ret) {
> + dev_err(dev, "failed to register regmap clock: %d\n", ret);
> + goto err;
> + }
> +
> + ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get,
Be explicit here and do parent->of_node.
> + &a53cc->clkr.hw);
> + if (ret) {
> + dev_err(dev, "failed to add clock provider: %d\n", ret);
> + goto err;
> + }
> +
> + platform_set_drvdata(pdev, a53cc);
> +
> + return 0;
> +
> +err:
> + clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb);
> + return ret;
> +}
> +
> +static int qcom_apcs_msm8916_clk_remove(struct platform_device *pdev)
> +{
> + struct clk_regmap_mux_div *a53cc = platform_get_drvdata(pdev);
> +
> + clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb);
> + of_clk_del_provider(pdev->dev.of_node);
You registered the provider on pdev->dev->parent.of_node.
> +
> + return 0;
> +}
> +
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH 5/6] cpufreq: Add DVFS support for Armada 37xx
From: Viresh Kumar @ 2017-12-05 5:54 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Rafael J. Wysocki, linux-pm, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Rob Herring, devicetree, Thomas Petazzoni,
linux-arm-kernel, Antoine Tenart, Miquèl Raynal,
Nadav Haklai, Victor Gu, Marcin Wojtas, Wilson Ding, Hua Jing,
Neta Zur Hershkovits, Evan Wang
In-Reply-To: <20171201112508.14121-6-gregory.clement@free-electrons.com>
On 01-12-17, 12:25, Gregory CLEMENT wrote:
> This patch adds DVFS support for the Armada 37xx SoCs
>
> There are up to four CPU frequency loads for Armada 37xx controlled by
> the hardware.
>
> This driver associates the CPU load level to a frequency, then the
> hardware will switch while selecting a load level.
>
> The hardware also can associate a voltage for each level (AVS support)
> but it is not yet supported
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
> drivers/cpufreq/Kconfig.arm | 7 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/armada-37xx-cpufreq.c | 241 ++++++++++++++++++++++++++++++++++
> 3 files changed, 249 insertions(+)
> create mode 100644 drivers/cpufreq/armada-37xx-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 17625115c67f..3018ff0d068f 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -19,6 +19,13 @@ config ACPI_CPPC_CPUFREQ
>
> If in doubt, say N.
>
> +config ARM_ARMADA_37XX_CPUFREQ
> + tristate "Armada 37xx CPUFreq support"
> + depends on ARCH_MVEBU
> + help
> + This adds the CPUFreq driver support for Marvell Armada 37xx SoCs.
> + The Armada 37xx PMU supports 4 frequency and VDD levels.
> +
> # big LITTLE core layer and glue drivers
> config ARM_BIG_LITTLE_CPUFREQ
> tristate "Generic ARM big LITTLE CPUfreq driver"
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index d762e76887e7..e07715ce8844 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ) += arm_big_little.o
> # LITTLE drivers, so that it is probed last.
> obj-$(CONFIG_ARM_DT_BL_CPUFREQ) += arm_big_little_dt.o
>
> +obj-$(CONFIG_ARM_ARMADA_37XX_CPUFREQ) += armada-37xx-cpufreq.o
> obj-$(CONFIG_ARM_BRCMSTB_AVS_CPUFREQ) += brcmstb-avs-cpufreq.o
> obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o
> obj-$(CONFIG_ARCH_DAVINCI) += davinci-cpufreq.o
> diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
> new file mode 100644
> index 000000000000..40c9a744cc6e
> --- /dev/null
> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> @@ -0,0 +1,241 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * CPU frequency scaling support for Armada 37xx platform.
> + *
> + * Copyright (C) 2017 Marvell
> + *
> + * Gregory CLEMENT <gregory.clement@free-electrons.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +/* Power management in North Bridge register set */
> +#define ARMADA_37XX_NB_L0L1 0x18
> +#define ARMADA_37XX_NB_L2L3 0x1C
> +#define ARMADA_37XX_NB_TBG_DIV_OFF 13
> +#define ARMADA_37XX_NB_TBG_DIV_MASK 0x7
> +#define ARMADA_37XX_NB_CLK_SEL_OFF 11
> +#define ARMADA_37XX_NB_CLK_SEL_MASK 0x1
> +#define ARMADA_37XX_NB_CLK_SEL_TBG 0x1
> +#define ARMADA_37XX_NB_TBG_SEL_OFF 9
> +#define ARMADA_37XX_NB_TBG_SEL_MASK 0x3
> +#define ARMADA_37XX_NB_VDD_SEL_OFF 6
> +#define ARMADA_37XX_NB_VDD_SEL_MASK 0x3
> +#define ARMADA_37XX_NB_CONFIG_SHIFT 16
Looks like you have added tabs after #define as well? Perhaps a space
is good enough there.
> +#define ARMADA_37XX_NB_DYN_MOD 0x24
> +#define ARMADA_37XX_NB_CLK_SEL_EN BIT(26)
> +#define ARMADA_37XX_NB_TBG_EN BIT(28)
> +#define ARMADA_37XX_NB_DIV_EN BIT(29)
> +#define ARMADA_37XX_NB_VDD_EN BIT(30)
> +#define ARMADA_37XX_NB_DFS_EN BIT(31)
> +#define ARMADA_37XX_NB_CPU_LOAD 0x30
> +#define ARMADA_37XX_NB_CPU_LOAD_MASK 0x3
> +#define ARMADA_37XX_DVFS_LOAD_0 0
> +#define ARMADA_37XX_DVFS_LOAD_1 1
> +#define ARMADA_37XX_DVFS_LOAD_2 2
> +#define ARMADA_37XX_DVFS_LOAD_3 3
> +
> +/*
> + * On Armada 37xx the Power management manages 4 level of CPU load,
> + * each level can be associated with a CPU clock source, a CPU
> + * divider, a VDD level, etc...
> + */
> +#define LOAD_LEVEL_NR 4
> +
> +struct armada_37xx_dvfs {
> + u32 cpu_freq_max;
> + u8 divider[LOAD_LEVEL_NR];
> +};
> +
> +static struct armada_37xx_dvfs armada_37xx_dvfs[] = {
> + {.cpu_freq_max = 1200*1000*1000, .divider = {1, 2, 4, 6} },
> + {.cpu_freq_max = 1000*1000*1000, .divider = {1, 2, 4, 5} },
> + {.cpu_freq_max = 800*1000*1000, .divider = {1, 2, 3, 4} },
> + {.cpu_freq_max = 600*1000*1000, .divider = {2, 4, 5, 6} },
> +};
> +
> +static struct armada_37xx_dvfs *armada_37xx_cpu_freq_info_get(u32 freq)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(armada_37xx_dvfs); i++) {
> + if (freq == armada_37xx_dvfs[i].cpu_freq_max)
> + return &armada_37xx_dvfs[i];
> + }
> +
> + pr_err("Unsupported CPU frequency %d MHz\n", freq/1000000);
> + return NULL;
> +}
> +
> +/*
> + * Setup the four level managed by the hardware. Once the four level
> + * will be configured then the DVFS will be enabled.
> + */
> +static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
> + struct clk *clk, u8 *divider)
> +{
> + int load_level;
> + struct clk *parent;
> +
> + for (load_level = 0; load_level < LOAD_LEVEL_NR; load_level++) {
> + unsigned int reg, mask, val, offset = 0;
> +
> + if (load_level <= ARMADA_37XX_DVFS_LOAD_1)
> + reg = ARMADA_37XX_NB_L0L1;
> + else
> + reg = ARMADA_37XX_NB_L2L3;
> +
> + if (load_level == ARMADA_37XX_DVFS_LOAD_0 ||
> + load_level == ARMADA_37XX_DVFS_LOAD_2)
> + offset += ARMADA_37XX_NB_CONFIG_SHIFT;
> +
> + /* Set cpu clock source, for all the level we use TBG */
> + val = ARMADA_37XX_NB_CLK_SEL_TBG << ARMADA_37XX_NB_CLK_SEL_OFF;
> + mask = (ARMADA_37XX_NB_CLK_SEL_MASK
> + << ARMADA_37XX_NB_CLK_SEL_OFF);
> +
> + /*
> + * Set cpu divider based on the pre-computed array in
> + * order to have balanced step.
> + */
> + val |= divider[load_level] << ARMADA_37XX_NB_TBG_DIV_OFF;
> + mask |= (ARMADA_37XX_NB_TBG_DIV_MASK
> + << ARMADA_37XX_NB_TBG_DIV_OFF);
> +
> + /* Set VDD divider which is actually the load level. */
> + val |= load_level << ARMADA_37XX_NB_VDD_SEL_OFF;
> + mask |= (ARMADA_37XX_NB_VDD_SEL_MASK
> + << ARMADA_37XX_NB_VDD_SEL_OFF);
> +
> + val <<= offset;
> + mask <<= offset;
> +
> + regmap_update_bits(base, reg, mask, val);
> + }
> +
> + /*
> + * Set cpu clock source, for all the level we keep the same
> + * clock source that the one already configured. For this one
> + * we need to use the clock framework
> + */
> + parent = clk_get_parent(clk);
> + clk_set_parent(clk, parent);
> +}
> +
> +static void __init armada37xx_cpufreq_disable_dvfs(struct regmap *base)
> +{
> + unsigned int reg = ARMADA_37XX_NB_DYN_MOD,
> + mask = ARMADA_37XX_NB_DFS_EN;
> +
> + regmap_update_bits(base, reg, mask, 0);
> +}
> +
> +static void __init armada37xx_cpufreq_enable_dvfs(struct regmap *base)
> +{
> + unsigned int val, reg = ARMADA_37XX_NB_CPU_LOAD,
> + mask = ARMADA_37XX_NB_CPU_LOAD_MASK;
> +
> + /* Start with the highest load (0) */
> + val = ARMADA_37XX_DVFS_LOAD_0;
> + regmap_update_bits(base, reg, mask, val);
> +
> + /* Now enable DVFS for the CPUs */
> + reg = ARMADA_37XX_NB_DYN_MOD;
> + mask = ARMADA_37XX_NB_CLK_SEL_EN | ARMADA_37XX_NB_TBG_EN |
> + ARMADA_37XX_NB_DIV_EN | ARMADA_37XX_NB_VDD_EN |
> + ARMADA_37XX_NB_DFS_EN;
> +
> + regmap_update_bits(base, reg, mask, mask);
> +}
> +
> +static int __init armada37xx_cpufreq_driver_init(void)
> +{
> + struct armada_37xx_dvfs *dvfs;
> + struct platform_device *pdev;
> + unsigned int cur_frequency;
> + struct regmap *nb_pm_base;
> + struct device *cpu_dev;
> + int load_level, ret;
> + struct clk *clk;
> +
> + nb_pm_base =
> + syscon_regmap_lookup_by_compatible("marvell,armada-3700-nb-pm");
> +
> + if (IS_ERR(nb_pm_base))
> + return -ENODEV;
> +
> + /* Before doing any configuration on the DVFS first, disable it */
> + armada37xx_cpufreq_disable_dvfs(nb_pm_base);
> +
> + /*
> + * On CPU 0 register the operating points supported (which are
> + * the nominal CPU frequency and full integer divisions of
> + * it).
> + */
> + cpu_dev = get_cpu_device(0);
> + if (!cpu_dev) {
> + dev_err(cpu_dev, "Cannot get CPU\n");
> + return -ENODEV;
> + }
> +
> + clk = clk_get(cpu_dev, 0);
> + if (IS_ERR(clk)) {
> + dev_err(cpu_dev, "Cannot get clock for CPU0\n");
> + return PTR_ERR(clk);
> + }
> +
> + /* Get nominal (current) CPU frequency */
> + cur_frequency = clk_get_rate(clk);
> + if (!cur_frequency) {
> + dev_err(cpu_dev, "Failed to get clock rate for CPU\n");
> + return -EINVAL;
> + }
> +
> + dvfs = armada_37xx_cpu_freq_info_get(cur_frequency);
> + if (!dvfs)
> + return -EINVAL;
> +
> + armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
> +
> + /*
> + * In case of a failure of dev_pm_opp_add(), we don't bother
> + * with cleaning up the registered OPP (there's no function to
> + * do so),
What do you mean by the comment within ()? We do have
dev_pm_opp_remove() helper.
> and simply cancel the registration of the cpufreq
> + * device.
> + */
> + for (load_level = ARMADA_37XX_DVFS_LOAD_0; load_level < LOAD_LEVEL_NR;
> + load_level++) {
> + unsigned long freq = dvfs->divider[load_level];
> +
> + ret = dev_pm_opp_add(cpu_dev, freq, 0);
> + if (ret)
> + return ret;
> + }
> +
> + /* Now that everything is setup, enable the DVFS at hardware level */
> + armada37xx_cpufreq_enable_dvfs(nb_pm_base);
> +
> + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> +
> + return PTR_ERR_OR_ZERO(pdev);
> +}
> +/* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
> +late_initcall(armada37xx_cpufreq_driver_init);
> +
> +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>");
> +MODULE_DESCRIPTION("Armada 37xx cpufreq driver");
> +MODULE_LICENSE("GPL");
> --
> 2.15.0
--
viresh
^ permalink raw reply
* Re: [PATCH v10 5/6] dt-bindings: mailbox: qcom: Document the APCS clock binding
From: Bjorn Andersson @ 2017-12-05 5:49 UTC (permalink / raw)
To: Georgi Djakov
Cc: sboyd, jassisinghbrar, robh, mturquette, linux-clk, linux-kernel,
linux-arm-msm, devicetree
In-Reply-To: <20171201170224.25053-6-georgi.djakov@linaro.org>
On Fri 01 Dec 09:02 PST 2017, Georgi Djakov wrote:
> Update the binding documentation for APCS to mention that the APCS
> hardware block also expose a clock controller functionality.
>
> The APCS clock controller is a mux and half-integer divider. It has the
> main CPU PLL as an input and provides the clock for the application CPU.
>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
Nice!
Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> .../bindings/mailbox/qcom,apcs-kpss-global.txt | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> index fb961c310f44..16964f0c1773 100644
> --- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> +++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> @@ -15,12 +15,21 @@ platforms.
> Usage: required
> Value type: <prop-encoded-array>
> Definition: must specify the base address and size of the global block
> +- clocks:
> + Usage: required if #clocks-cells property is present
> + Value type: <phandle>
> + Definition: phandle to the input PLL, which feeds the APCS mux/divider
>
> - #mbox-cells:
> Usage: required
> Value type: <u32>
> Definition: as described in mailbox.txt, must be 1
>
> +- #clock-cells:
> + Usage: optional
> + Value type: <u32>
> + Definition: as described in clock.txt, must be 0
> +
>
> = EXAMPLE
> The following example describes the APCS HMSS found in MSM8996 and part of the
> @@ -44,3 +53,12 @@ GLINK RPM referencing the "rpm_hlos" doorbell therein.
> mbox-names = "rpm_hlos";
> };
>
> +Below is another example of the APCS binding on MSM8916 platforms:
> +
> + apcs: mailbox@b011000 {
> + compatible = "qcom,msm8916-apcs-kpss-global";
> + reg = <0xb011000 0x1000>;
> + #mbox-cells = <1>;
> + clocks = <&a53pll>;
> + #clock-cells = <0>;
> + };
^ permalink raw reply
* Re: [PATCH V1 4/4] qcom: spmi-wled: Add auto-calibration logic support
From: Bjorn Andersson @ 2017-12-05 5:40 UTC (permalink / raw)
To: Kiran Gunda
Cc: linux-arm-msm, Lee Jones, Daniel Thompson, Jingoo Han,
Richard Purdie, Jacek Anaszewski, Pavel Machek, Rob Herring,
Mark Rutland, Bartlomiej Zolnierkiewicz, linux-leds, devicetree,
linux-kernel, linux-fbdev, linux-arm-msm-owner
In-Reply-To: <1510834717-21765-5-git-send-email-kgunda@codeaurora.org>
On Thu 16 Nov 04:18 PST 2017, Kiran Gunda wrote:
> The auto-calibration algorithm checks if the current WLED sink
> configuration is valid. It tries enabling every sink and checks
> if the OVP fault is observed. Based on this information it
> detects and enables the valid sink configuration. Auto calibration
> will be triggered when the OVP fault interrupts are seen frequently
> thereby it tries to fix the sink configuration.
>
So it's not auto "calibration" it's auto "detection" of strings?
When is this feature needed?
> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> ---
> .../bindings/leds/backlight/qcom-spmi-wled.txt | 5 +
> drivers/video/backlight/qcom-spmi-wled.c | 304 ++++++++++++++++++++-
> 2 files changed, 306 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> index d39ee93..f06c0cd 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> @@ -94,6 +94,11 @@ The PMIC is connected to the host processor via SPMI bus.
> Definition: Interrupt names associated with the interrupts.
> Currently supported interrupts are "sc-irq" and "ovp-irq".
>
> +- qcom,auto-calibration
qcom,auto-string-detect?
> + Usage: optional
> + Value type: <bool>
> + Definition: Enables auto-calibration of the WLED sink configuration.
> +
> Example:
>
> qcom-wled@d800 {
> diff --git a/drivers/video/backlight/qcom-spmi-wled.c b/drivers/video/backlight/qcom-spmi-wled.c
> index 8b2a77a..aee5c56 100644
> --- a/drivers/video/backlight/qcom-spmi-wled.c
> +++ b/drivers/video/backlight/qcom-spmi-wled.c
> @@ -38,11 +38,14 @@
> #define QCOM_WLED_CTRL_SC_FAULT_BIT BIT(2)
>
> #define QCOM_WLED_CTRL_INT_RT_STS 0x10
> +#define QCOM_WLED_CTRL_OVP_FLT_RT_STS_BIT BIT(1)
The use of BIT() makes this a mask and not a bit number, so if you just
drop that you can afford to spell out the "FAULT" like the data sheet
does. Perhaps even making it QCOM_WLED_CTRL_OVP_FAULT_STATUS ?
>
> #define QCOM_WLED_CTRL_MOD_ENABLE 0x46
> #define QCOM_WLED_CTRL_MOD_EN_MASK BIT(7)
> #define QCOM_WLED_CTRL_MODULE_EN_SHIFT 7
>
> +#define QCOM_WLED_CTRL_FDBK_OP 0x48
This is called WLED_CTRL_FEEDBACK_CONTROL, why the need to make it
unreadable?
> +
> #define QCOM_WLED_CTRL_SWITCH_FREQ 0x4c
> #define QCOM_WLED_CTRL_SWITCH_FREQ_MASK GENMASK(3, 0)
>
> @@ -99,6 +102,7 @@ struct qcom_wled_config {
> int ovp_irq;
> bool en_cabc;
> bool ext_pfet_sc_pro_en;
> + bool auto_calib_enabled;
> };
>
> struct qcom_wled {
> @@ -108,18 +112,25 @@ struct qcom_wled {
> struct mutex lock;
> struct qcom_wled_config cfg;
> ktime_t last_sc_event_time;
> + ktime_t start_ovp_fault_time;
> u16 sink_addr;
> u16 ctrl_addr;
> + u16 auto_calibration_ovp_count;
> u32 brightness;
> u32 sc_count;
> bool prev_state;
> bool ovp_irq_disabled;
> + bool auto_calib_done;
> + bool force_mod_disable;
> };
>
> static int qcom_wled_module_enable(struct qcom_wled *wled, int val)
> {
> int rc;
>
> + if (wled->force_mod_disable)
> + return 0;
> +
> rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
> QCOM_WLED_CTRL_MOD_ENABLE, QCOM_WLED_CTRL_MOD_EN_MASK,
> val << QCOM_WLED_CTRL_MODULE_EN_SHIFT);
> @@ -187,12 +198,10 @@ static int qcom_wled_set_brightness(struct qcom_wled *wled, u16 brightness)
> v[1] = (brightness >> 8) & 0xf;
>
> for (i = 0; (string_cfg >> i) != 0; i++) {
> - if (string_cfg & BIT(i)) {
Why was this check here in the first place, if it's now fine to
configure the brightness of all strings?
Also, a single-string config of 0b0001 will only set brightness on the
first string, while 0b1000 will set brightness on all strings.
> rc = regmap_bulk_write(wled->regmap, wled->sink_addr +
> QCOM_WLED_SINK_BRIGHT_LSB_REG(i), v, 2);
> if (rc < 0)
> return rc;
> - }
> }
>
> return 0;
> @@ -294,6 +303,262 @@ static irqreturn_t qcom_wled_sc_irq_handler(int irq, void *_wled)
> return IRQ_HANDLED;
> }
>
> +#define AUTO_CALIB_BRIGHTNESS 200
> +static int qcom_wled_auto_calibrate(struct qcom_wled *wled)
> +{
> + int rc = 0, i;
> + u32 sink_config = 0, int_sts;
> + u8 reg = 0, sink_test = 0, sink_valid = 0;
> + u8 string_cfg = wled->cfg.string_cfg;
> +
> + /* read configured sink configuration */
> + rc = regmap_read(wled->regmap, wled->sink_addr +
> + QCOM_WLED_SINK_CURR_SINK_EN, &sink_config);
> + if (rc < 0) {
> + pr_err("Failed to read SINK configuration rc=%d\n", rc);
> + goto failed_calib;
> + }
> +
> + /* disable the module before starting calibration */
> + rc = regmap_update_bits(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_MOD_ENABLE,
> + QCOM_WLED_CTRL_MOD_EN_MASK, 0);
> + if (rc < 0) {
> + pr_err("Failed to disable WLED module rc=%d\n", rc);
> + goto failed_calib;
> + }
Any error handling beyond this point seems to leave the backlight off
(indefinitely?), this does seem like potentially bad user experience...
In particular I wonder about the case when this would happen at some
random time, minutes, hours, days, months after the device was booted.
> +
> + /* set low brightness across all sinks */
> + rc = qcom_wled_set_brightness(wled, AUTO_CALIB_BRIGHTNESS);
> + if (rc < 0) {
> + pr_err("Failed to set brightness for calibration rc=%d\n", rc);
> + goto failed_calib;
> + }
> +
> + if (wled->cfg.en_cabc) {
> + for (i = 0; (string_cfg >> i) != 0; i++) {
> + reg = 0;
> + rc = regmap_update_bits(wled->regmap, wled->sink_addr +
> + QCOM_WLED_SINK_CABC_REG(i),
> + QCOM_WLED_SINK_CABC_MASK, reg);
Just replace "reg" with 0.
> + if (rc < 0)
> + goto failed_calib;
> + }
> + }
> +
> + /* disable all sinks */
> + rc = regmap_write(wled->regmap,
> + wled->sink_addr + QCOM_WLED_SINK_CURR_SINK_EN, 0);
> + if (rc < 0) {
> + pr_err("Failed to disable all sinks rc=%d\n", rc);
> + goto failed_calib;
> + }
> +
> + /* iterate through the strings one by one */
> + for (i = 0; (string_cfg >> i) != 0; i++) {
> + sink_test = 1 << (QCOM_WLED_SINK_CURR_SINK_SHFT + i);
BIT(QCOM_WLED_SINK_CURR_SINK_SHFT + i);
> +
> + /* Enable feedback control */
> + rc = regmap_write(wled->regmap, wled->ctrl_addr +
> + QCOM_WLED_CTRL_FDBK_OP, i + 1);
> + if (rc < 0) {
> + pr_err("Failed to enable feedback for SINK %d rc = %d\n",
> + i + 1, rc);
> + goto failed_calib;
> + }
> +
> + /* enable the sink */
> + rc = regmap_write(wled->regmap, wled->sink_addr +
> + QCOM_WLED_SINK_CURR_SINK_EN, sink_test);
> + if (rc < 0) {
> + pr_err("Failed to configure SINK %d rc=%d\n",
> + i + 1, rc);
> + goto failed_calib;
> + }
> +
> + /* Enable the module */
> + rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
> + QCOM_WLED_CTRL_MOD_ENABLE,
> + QCOM_WLED_CTRL_MOD_EN_MASK,
> + QCOM_WLED_CTRL_MOD_EN_MASK);
I like the use of regmap_update_bits(..., MASK, MASK) it's clean, but
makes me wonder why it's done differently in qcom_wled_module_enable().
> + if (rc < 0) {
> + pr_err("Failed to enable WLED module rc=%d\n", rc);
> + goto failed_calib;
> + }
> +
> + usleep_range(QCOM_WLED_SOFT_START_DLY_US,
> + QCOM_WLED_SOFT_START_DLY_US + 1000);
> +
> + rc = regmap_read(wled->regmap, wled->ctrl_addr +
> + QCOM_WLED_CTRL_INT_RT_STS, &int_sts);
> + if (rc < 0) {
> + pr_err("Error in reading WLED_INT_RT_STS rc=%d\n", rc);
> + goto failed_calib;
> + }
> +
> + if (int_sts & QCOM_WLED_CTRL_OVP_FAULT_BIT)
> + pr_debug("WLED OVP fault detected with SINK %d\n",
> + i + 1);
> + else
> + sink_valid |= sink_test;
> +
> + /* Disable the module */
> + rc = regmap_update_bits(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_MOD_ENABLE,
> + QCOM_WLED_CTRL_MOD_EN_MASK, 0);
> + if (rc < 0) {
> + pr_err("Failed to disable WLED module rc=%d\n", rc);
> + goto failed_calib;
> + }
> + }
> +
> + if (sink_valid == sink_config) {
> + pr_debug("WLED auto-calibration complete, default sink-config=%x OK!\n",
> + sink_config);
> + } else {
> + pr_warn("Invalid WLED default sink config=%x changing it to=%x\n",
> + sink_config, sink_valid);
> + sink_config = sink_valid;
> + }
> +
> + if (!sink_config) {
> + pr_warn("No valid WLED sinks found\n");
> + wled->force_mod_disable = true;
> + goto failed_calib;
> + }
> +
> + /* write the new sink configuration */
> + rc = regmap_write(wled->regmap,
> + wled->sink_addr + QCOM_WLED_SINK_CURR_SINK_EN,
> + sink_config);
> + if (rc < 0) {
> + pr_err("Failed to reconfigure the default sink rc=%d\n", rc);
> + goto failed_calib;
> + }
> +
> + /* MODULATOR_EN setting for valid sinks */
"Enable valid sinks"
> + for (i = 0; (string_cfg >> i) != 0; i++) {
> + if (wled->cfg.en_cabc) {
> + reg = QCOM_WLED_SINK_CABC_EN;
"reg" is a bad name of a variable holding the "value" to be written to a
register.
> + rc = regmap_update_bits(wled->regmap, wled->sink_addr +
> + QCOM_WLED_SINK_CABC_REG(i),
> + QCOM_WLED_SINK_CABC_MASK, reg);
Again, just inline the value in the function call.
> + if (rc < 0)
> + goto failed_calib;
> + }
> +
> + if (sink_config & (1 << (QCOM_WLED_SINK_CURR_SINK_SHFT + i)))
BIT(QCOM_WLED_SINK_CURR_SINK_SHFT + i)
> + reg = QCOM_WLED_SINK_REG_STR_MOD_EN;
> + else
> + reg = 0x0; /* disable modulator_en for unused sink */
> +
> + rc = regmap_write(wled->regmap, wled->sink_addr +
> + QCOM_WLED_SINK_MOD_EN_REG(i), reg);
> + if (rc < 0) {
> + pr_err("Failed to configure MODULATOR_EN rc=%d\n", rc);
> + goto failed_calib;
> + }
> + }
> +
> + /* restore the feedback setting */
> + rc = regmap_write(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_FDBK_OP, 0);
> + if (rc < 0) {
> + pr_err("Failed to restore feedback setting rc=%d\n", rc);
> + goto failed_calib;
> + }
> +
> + /* restore brightness */
> + rc = qcom_wled_set_brightness(wled, wled->brightness);
> + if (rc < 0) {
> + pr_err("Failed to set brightness after calibration rc=%d\n",
> + rc);
> + goto failed_calib;
> + }
> +
> + rc = regmap_update_bits(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_MOD_ENABLE,
> + QCOM_WLED_CTRL_MOD_EN_MASK,
> + QCOM_WLED_CTRL_MOD_EN_MASK);
> + if (rc < 0) {
> + pr_err("Failed to enable WLED module rc=%d\n", rc);
> + goto failed_calib;
> + }
> +
> + /* delay for WLED soft-start */
What comes after this that you want to delay?
This delay is used to make the OVP IRQ not fire immediately, but as
we've now successfully executed the string auto detection run we're
never going to do anything in the OVP handler.
> + usleep_range(QCOM_WLED_SOFT_START_DLY_US,
> + QCOM_WLED_SOFT_START_DLY_US + 1000);
> +
> +failed_calib:
> + return rc;
> +}
> +
> +#define WLED_AUTO_CAL_OVP_COUNT 5
> +#define WLED_AUTO_CAL_CNT_DLY_US 1000000 /* 1 second */
> +static bool qcom_wled_auto_cal_required(struct qcom_wled *wled)
> +{
> + s64 elapsed_time_us;
> +
> + /*
> + * Check if the OVP fault was an occasional one
> + * or if its firing continuously, the latter qualifies
> + * for an auto-calibration check.
> + */
> + if (!wled->auto_calibration_ovp_count) {
> + wled->start_ovp_fault_time = ktime_get();
> + wled->auto_calibration_ovp_count++;
> + } else {
> + elapsed_time_us = ktime_us_delta(ktime_get(),
> + wled->start_ovp_fault_time);
> + if (elapsed_time_us > WLED_AUTO_CAL_CNT_DLY_US)
> + wled->auto_calibration_ovp_count = 0;
> + else
> + wled->auto_calibration_ovp_count++;
> +
> + if (wled->auto_calibration_ovp_count >=
> + WLED_AUTO_CAL_OVP_COUNT) {
> + wled->auto_calibration_ovp_count = 0;
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> +static int qcom_wled_auto_calibrate_at_init(struct qcom_wled *wled)
I presume this function is expected to detect if there is a invalid
configuration at boot and try to figure out which strings are actually
wired.
> +{
> + int rc;
> + u32 fault_status = 0, rt_status = 0;
> +
> + if (!wled->cfg.auto_calib_enabled)
> + return 0;
> +
> + rc = regmap_read(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_INT_RT_STS,
> + &rt_status);
> + if (rc < 0)
> + pr_err("Failed to read RT status rc=%d\n", rc);
> +
> + rc = regmap_read(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_FAULT_STATUS,
> + &fault_status);
> + if (rc < 0)
> + pr_err("Failed to read fault status rc=%d\n", rc);
> +
> + if ((rt_status & QCOM_WLED_CTRL_OVP_FLT_RT_STS_BIT) ||
> + (fault_status & QCOM_WLED_CTRL_OVP_FAULT_BIT)) {
You should be able to drop the extra () around these.
> + mutex_lock(&wled->lock);
> + rc = qcom_wled_auto_calibrate(wled);
> + if (rc < 0)
> + pr_err("Failed auto-calibration rc=%d\n", rc);
qcom_wled_auto_calibrate() did already print, no need to repeat this.
> + else
> + wled->auto_calib_done = true;
> + mutex_unlock(&wled->lock);
> + }
> +
> + return rc;
> +}
> +
> static irqreturn_t qcom_wled_ovp_irq_handler(int irq, void *_wled)
> {
> struct qcom_wled *wled = _wled;
> @@ -319,6 +584,33 @@ static irqreturn_t qcom_wled_ovp_irq_handler(int irq, void *_wled)
> pr_err("WLED OVP fault detected, int_sts=%x fault_sts= %x\n",
> int_sts, fault_sts);
>
> + if (fault_sts & QCOM_WLED_CTRL_OVP_FAULT_BIT) {
> + if (wled->cfg.auto_calib_enabled && !wled->auto_calib_done) {
> + if (qcom_wled_auto_cal_required(wled)) {
So this will be invoked only once, iff we didn't boot with a faulty
configuration in which case the qcom_wled_auto_calibrate_at_init() has
already done this step and set auto_calib_done.
Which also would mean that all logic in this handler, beyond the
printouts, are only ever going to be executed zero or one times.
Why don't you just do auto-detection during probe (iff the flag is set
in DT) and you can remove all this extra logic?
> + mutex_lock(&wled->lock);
> + if (wled->cfg.ovp_irq > 0 &&
> + !wled->ovp_irq_disabled) {
> + disable_irq_nosync(wled->cfg.ovp_irq);
> + wled->ovp_irq_disabled = true;
> + }
> +
> + rc = qcom_wled_auto_calibrate(wled);
> + if (rc < 0)
> + pr_err("Failed auto-calibration rc=%d\n",
> + rc);
qcom_wled_auto_calibrate() did already print.
> + else
> + wled->auto_calib_done = true;
> +
> + if (wled->cfg.ovp_irq > 0 &&
> + wled->ovp_irq_disabled) {
> + enable_irq(wled->cfg.ovp_irq);
> + wled->ovp_irq_disabled = false;
> + }
> + mutex_unlock(&wled->lock);
> + }
> + }
> + }
> +
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH v3 2/3] mailbox: Add support for Hi3660 mailbox
From: Leo Yan @ 2017-12-05 5:13 UTC (permalink / raw)
To: Jassi Brar
Cc: Xu YiPing, Rob Herring, Mark Rutland, Wei Xu, Catalin Marinas,
Will Deacon, Devicetree List, Linux Kernel Mailing List,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Guodong Xu, Haojian Zhuang, suzhuangluan-C8/M+/jPZTeaMJb+Lgu22Q,
xuezhiliang-C8/M+/jPZTeaMJb+Lgu22Q,
kevin.wangtao-C8/M+/jPZTeaMJb+Lgu22Q, Zhong Kaihua
In-Reply-To: <CABb+yY1+uMhZ8nXxsuv4LFGqOkdH6AX-U_AeNJ0+j1+iA6=yhQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Dec 05, 2017 at 08:58:05AM +0530, Jassi Brar wrote:
> On Fri, Nov 17, 2017 at 2:54 PM, Xu YiPing <xuyiping-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org> wrote:
> > From: Kaihua Zhong <zhongkaihua-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> >
> > Hi3660 mailbox controller is used to send message within multiple
> > processors, MCU, HIFI, etc. It supports 32 mailbox channels and every
> > channel can only be used for single transferring direction. Once the
> > channel is enabled, it needs to specify the destination interrupt and
> > acknowledge interrupt, these two interrupt vectors are used to create
> > the connection between the mailbox and interrupt controllers.
> >
> > The data transferring supports two modes, one is named as "automatic
> > acknowledge" mode so after send message the kernel doesn't need to wait
> > for acknowledge from remote and directly return; there have another mode
> > is to rely on handling interrupt for acknowledge.
> >
> > This commit is for initial version driver, which only supports
> > "automatic acknowledge" mode to support CPU clock, which is the only
> > one consumer to use mailbox and has been verified. Later may enhance
> > this driver for interrupt mode (e.g. for supporting HIFI).
> >
> > Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Signed-off-by: Ruyi Wang <wangruyi-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Kaihua Zhong <zhongkaihua-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > ---
> > drivers/mailbox/Kconfig | 8 +
> > drivers/mailbox/Makefile | 2 +
> > drivers/mailbox/hi3660-mailbox.c | 322 +++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 332 insertions(+)
> > create mode 100644 drivers/mailbox/hi3660-mailbox.c
> >
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index c5731e5..4b5d6e9 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -108,6 +108,14 @@ config TI_MESSAGE_MANAGER
> > multiple processors within the SoC. Select this driver if your
> > platform has support for the hardware block.
> >
> > +config HI3660_MBOX
> > + tristate "Hi3660 Mailbox"
> > + depends on ARCH_HISI && OF
> > + help
> > + An implementation of the hi3660 mailbox. It is used to send message
> > + between application processors and other processors/MCU/DSP. Select
> > + Y here if you want to use Hi3660 mailbox controller.
> > +
> > config HI6220_MBOX
> > tristate "Hi6220 Mailbox"
> > depends on ARCH_HISI
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index d54e412..7d1bd51 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -26,6 +26,8 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
> >
> > obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
> >
> > +obj-$(CONFIG_HI3660_MBOX) += hi3660-mailbox.o
> > +
> > obj-$(CONFIG_HI6220_MBOX) += hi6220-mailbox.o
> >
> > obj-$(CONFIG_BCM_PDC_MBOX) += bcm-pdc-mailbox.o
> > diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
> > new file mode 100644
> > index 0000000..ba80834
> > --- /dev/null
> > +++ b/drivers/mailbox/hi3660-mailbox.c
> > @@ -0,0 +1,322 @@
> > +/*
> > + * Hisilicon's Hi3660 mailbox controller driver
> > + *
> > + * Copyright (c) 2017 Hisilicon Limited.
> > + * Copyright (c) 2017 Linaro Limited.
> > + *
> > + * Author: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, version 2 of the License.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> >
> We now use SPDX licence
Thanks for quick response, Jassi.
Will change to SPDX licence fashion.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#include "mailbox.h"
> > +
> > +#define MBOX_CHAN_MAX 32
> > +
> > +#define MBOX_RX (0x0)
> > +#define MBOX_TX (0x1)
> > +
> > +#define MBOX_BASE(mbox, ch) ((mbox)->base + ((ch) * 0x40))
> > +#define MBOX_SRC_REG (0x00)
> > +#define MBOX_DST_REG (0x04)
> > +#define MBOX_DCLR_REG (0x08)
> > +#define MBOX_DSTAT_REG (0x0c)
> > +#define MBOX_MODE_REG (0x10)
> > +#define MBOX_IMASK_REG (0x14)
> > +#define MBOX_ICLR_REG (0x18)
> > +#define MBOX_SEND_REG (0x1c)
> > +#define MBOX_DATA_REG (0x20)
> > +
> > +#define MBOX_IPC_LOCK_REG (0xa00)
> > +#define MBOX_IPC_UNLOCK (0x1acce551)
> > +
> > +#define MBOX_AUTOMATIC_ACK (1)
> > +
> Please, no brackets around numbers.
Sure, will remove brackets.
> > +#define MBOX_STATE_IDLE BIT(4)
> > +#define MBOX_STATE_ACK BIT(7)
> > +
> > +#define MBOX_MSG_LEN 8
> > +
> > +/**
> > + * Hi3660 mailbox channel device data
> > + *
> > + * A channel can be used for TX or RX, it can trigger remote
> > + * processor interrupt to notify remote processor and can receive
> > + * interrupt if has incoming message.
> > + *
> > + * @dst_irq: Interrupt vector for remote processor
> > + * @ack_irq: Interrupt vector for local processor
> > + */
> > +struct hi3660_mbox_dev {
> >
> Better than dev, maybe hi3660_chan_info ?
>
> > + unsigned int dst_irq;
> > + unsigned int ack_irq;
> > +};
> > +
> > +/**
> > + * Hi3660 mailbox controller data
> > + *
> > + * Mailbox controller includes 32 channels and can allocate
> > + * channel for message transferring.
> > + *
> > + * @dev: Device to which it is attached
> > + * @base: Base address of the register mapping region
> > + * @chan: Representation of channels in mailbox controller
> > + * @mdev: Representation of channel device data
> > + * @controller: Representation of a communication channel controller
> > + */
> > +struct hi3660_mbox {
> > + struct device *dev;
> > + void __iomem *base;
> > + struct mbox_chan chan[MBOX_CHAN_MAX];
> > + struct hi3660_mbox_dev mdev[MBOX_CHAN_MAX];
> >
> Maybe mchan, instead of mdev.
Will refactor structure name.
> > + struct mbox_controller controller;
> > +};
> > +
> > +static inline struct hi3660_mbox *to_hi3660_mbox(struct mbox_controller *mbox)
> > +{
> > + return container_of(mbox, struct hi3660_mbox, controller);
> > +}
> > +
> > +static int hi3660_mbox_check_state(struct mbox_chan *chan)
> > +{
> > + unsigned long ch = (unsigned long)chan->con_priv;
> > + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> > + struct hi3660_mbox_dev *mdev = &mbox->mdev[ch];
> > + void __iomem *base = MBOX_BASE(mbox, ch);
> > + unsigned long val;
> > + unsigned int state, ret;
> > +
> > + /* Mailbox is idle so directly bail out */
> > + state = readl_relaxed(base + MBOX_MODE_REG);
> > + if (state & MBOX_STATE_IDLE)
> > + return 0;
> > +
> > + /* Wait for acknowledge from remote */
> > + ret = readx_poll_timeout_atomic(readl_relaxed, base + MBOX_MODE_REG,
> > + val, (val & MBOX_STATE_ACK), 1000, 300000);
> > + if (ret) {
> > + dev_err(mbox->dev, "%s: timeout for receiving ack\n", __func__);
> > + return ret;
> > + }
> > +
> > + /* Ensure channel is released */
> > + writel_relaxed(0xffffffff, base + MBOX_IMASK_REG);
> > + writel_relaxed(BIT(mdev->ack_irq), base + MBOX_SRC_REG);
> > +
> > + return 0;
> > +}
> > +
> > +static int hi3660_mbox_unlock(struct mbox_chan *chan)
> > +{
> > + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> > + unsigned int val, retry = 3;
> > +
> > + do {
> > + writel_relaxed(MBOX_IPC_UNLOCK, mbox->base + MBOX_IPC_LOCK_REG);
> > +
> > + val = readl_relaxed(mbox->base + MBOX_IPC_LOCK_REG);
> > + if (!val)
> > + break;
> > +
> > + udelay(10);
> > + } while (retry--);
> > +
> > + return (!val) ? 0 : -ETIMEDOUT;
> > +}
> > +
> > +static int hi3660_mbox_acquire_channel(struct mbox_chan *chan)
> > +{
> > + unsigned long ch = (unsigned long)chan->con_priv;
> > + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> > + struct hi3660_mbox_dev *mdev = &mbox->mdev[ch];
> > + void __iomem *base = MBOX_BASE(mbox, ch);
> > + unsigned int val, retry;
> > +
> > + for (retry = 10; retry; retry--) {
> > + /* Check if channel has been acquired */
> > + if (readl_relaxed(base + MBOX_MODE_REG) & MBOX_STATE_IDLE) {
> > + writel_relaxed(BIT(mdev->ack_irq), base + MBOX_SRC_REG);
> > + val = readl_relaxed(base + MBOX_SRC_REG);
> > + if (val & BIT(mdev->ack_irq))
> > + break;
> > + }
> > + }
> > +
> > + return retry ? 0 : -ETIMEDOUT;
> > +}
> > +
> > +static int hi3660_mbox_send(struct mbox_chan *chan, u32 *msg)
> > +{
> > + unsigned long ch = (unsigned long)chan->con_priv;
> > + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> > + struct hi3660_mbox_dev *mdev = &mbox->mdev[ch];
> > + void __iomem *base = MBOX_BASE(mbox, ch);
> > + unsigned int i;
> > +
> > + /* Clear mask for destination interrupt */
> > + writel_relaxed(~BIT(mdev->dst_irq), base + MBOX_IMASK_REG);
> > +
> > + /* Config destination for interrupt vector */
> > + writel_relaxed(BIT(mdev->dst_irq), base + MBOX_DST_REG);
> > +
> > + /* Automatic acknowledge mode */
> > + writel_relaxed(MBOX_AUTOMATIC_ACK, base + MBOX_MODE_REG);
> > +
> > + /* Fill message data */
> > + for (i = 0; i < MBOX_MSG_LEN; i++)
> > + writel_relaxed(msg[i], base + MBOX_DATA_REG + i * 4);
> > +
> > + /* Trigger data transferring */
> > + writel_relaxed(BIT(mdev->ack_irq), base + MBOX_SEND_REG);
> > + return 0;
> > +}
> > +
> > +static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
> > +{
> > + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> > + int err;
> > +
> > + err = hi3660_mbox_check_state(chan);
> > + if (err) {
> > + dev_err(mbox->dev, "checking state failed\n");
> > + return err;
> > + }
> > +
> > + err = hi3660_mbox_unlock(chan);
> > + if (err) {
> > + dev_err(mbox->dev, "unlocking mailbox failed\n");
> > + return err;
> > + }
> > +
> > + err = hi3660_mbox_acquire_channel(chan);
> > + if (err) {
> > + dev_err(mbox->dev, "acquiring channel failed\n");
> > + return err;
> > + }
> > +
> Please remember .send_data() is called with irqs disabled spinlock
> All these polling functions hurt. Please move the above three checks
> in last_tx_done()
I have tried to move these three checks in last_tx_done(), since these
three functions tightly couple with mailbox state machine, so I failed
to move them to last_tx_done().
Anyway, I will try to move them to last_tx_done() for next version
patch.
Thanks,
Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver
From: Katsuhiro Suzuki @ 2017-12-05 4:48 UTC (permalink / raw)
To: 'Mark Brown',
Suzuki, Katsuhiro/鈴木 勝博
Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA,
Yamada, Masahiro/山田 真弘,
Masami Hiramatsu, Jassi Brar,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171204183934.rd4vin22ktukwpip-GFdadSzt00ze9xe1eoZjHA@public.gmane.org>
Hello,
Thanks a lot for your review.
> -----Original Message-----
> From: Mark Brown [mailto:broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Tuesday, December 5, 2017 3:40 AM
> To: Suzuki, Katsuhiro/鈴木 勝博 <suzuki.katsuhiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
> Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org; Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Yamada, Masahiro/山
> 田 真弘 <yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>; Masami Hiramatsu <masami.hiramatsu-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>; Jassi Brar
> <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 5/8] ASoC: uniphier: add support for UniPhier AIO driver
>
> On Wed, Nov 22, 2017 at 08:43:18PM +0900, Katsuhiro Suzuki wrote:
>
> > sound/soc/uniphier/Makefile | 4 +
> > sound/soc/uniphier/aio-core.c | 368 +++++++++++++++++++++
> > sound/soc/uniphier/aio-dma.c | 266 +++++++++++++++
> > sound/soc/uniphier/aio-regctrl.c | 699 +++++++++++++++++++++++++++++++++++++++
> > sound/soc/uniphier/aio-regctrl.h | 495 +++++++++++++++++++++++++++
> > sound/soc/uniphier/aio.h | 261 +++++++++++++++
>
> Please split this up more, it looks like there's at least two or three
> drivers in here and it winds up being quite large. There's at least a
> DMA and a DAI driver. Looking through this my overall impression is
> that this is a fairly large and complex audio subsystem with some DSP
> and routing capacity which is being handled in a board specific fashion
> rather than generically but it's kind of hard to tell as there's not
> much description of what's going on so I'm needing to reverse engineer
> things from the driver.
>
> The code itself looks fairly clean, it's mainly a case of trying to
> figure out if it's doing what it's supposed to with the limited
> documentation.
>
> > +int uniphier_aio_hw_params(struct snd_pcm_substream *substream,
> > + struct snd_pcm_hw_params *params,
> > + struct snd_soc_dai *dai)
> > +{
> > + struct uniphier_aio *aio = uniphier_priv(dai);
> > + struct uniphier_aio_sub *sub = &aio->sub[substream->stream];
> > +
> > + sub->params = *params;
> > + sub->setting = 1;
>
> So we don't validate the params at all?
>
> > + uniphier_aio_port_reset(sub);
> > + uniphier_aio_srcport_reset(sub);
>
> Is there a mux in the SoC here?
>
Sorry for confusing, It's not mux.
uniphier_srcport_reset() resets HW SRC (sampling rate converter) block.
Audio data out ports of UniPhier audio system have HW SRC.
> > +static const struct of_device_id uniphier_aio_of_match[] = {
> > +#ifdef CONFIG_SND_SOC_UNIPHIER_LD11
> > + {
> > + .compatible = "socionext,uniphier-ld11-aio",
> > + .data = &uniphier_aio_ld11_spec,
> > + },
> > + {
> > + .compatible = "socionext,uniphier-ld20-aio",
> > + .data = &uniphier_aio_ld20_spec,
> > + },
> > +#endif /* CONFIG_SND_SOC_UNIPHIER_LD11 */
>
> Why is there an ifdef here? There's no other conditional code in here,
> it seems pointless.
>
This config is used to support or not LD11 SoC.
aio-ld11.c is not build and 'uniphier_aio_ldxx_spec' is undefined if this config is disabled.
aio-ld11.c defines SoC dependent resources (port, HW ring buffer, DMA ch, etc.)
and fixed settings.
I know it's better to move such information into device-tree, but register areas of
UniPhier's audio system is very strange and interleaved. It's hard to split each nodes...
> > + for (j = 0; j < ARRAY_SIZE(aio->sub); j++) {
> > + struct uniphier_aio_sub *sub = &aio->sub[j];
> > +
> > + if (!sub->running)
> > + continue;
> > +
> > + spin_lock(&sub->spin);
> > + uniphier_aio_rb_sync(sub);
> > + uniphier_aio_rb_clear_int(sub);
> > + spin_unlock(&sub->spin);
>
> It's not 100% obvious what this does... a comment might help.
>
> > +int uniphier_aio_chip_init(struct uniphier_aio_chip *chip)
> > +{
> > + struct regmap *r = chip->regmap;
> > +
> > + regmap_update_bits(r, A2APLLCTR0,
> > + A2APLLCTR0_APLLXPOW_MASK,
> > + A2APLLCTR0_APLLXPOW_PWON);
> > +
> > + regmap_update_bits(r, A2APLLCTR1, A2APLLCTR1_APLL_MASK,
> > + A2APLLCTR1_APLLF2_33MHZ | A2APLLCTR1_APLLA2_33MHZ |
> > + A2APLLCTR1_APLLF1_36MHZ | A2APLLCTR1_APLLA1_36MHZ);
> > +
> > + regmap_update_bits(r, A2EXMCLKSEL0,
> > + A2EXMCLKSEL0_EXMCLK_MASK,
> > + A2EXMCLKSEL0_EXMCLK_OUTPUT);
> > +
> > + regmap_update_bits(r, A2AIOINPUTSEL, A2AIOINPUTSEL_RXSEL_MASK,
> > + A2AIOINPUTSEL_RXSEL_PCMI1_HDMIRX1 |
> > + A2AIOINPUTSEL_RXSEL_PCMI2_SIF |
> > + A2AIOINPUTSEL_RXSEL_PCMI3_EVEA |
> > + A2AIOINPUTSEL_RXSEL_IECI1_HDMIRX1);
>
> This definitely looks like there's some clocking and audio routing
> within the SoC which should be exposed to userspace, or at the very
> least machine driver configuration rather than being hard coded.
>
> > + switch (pc) {
> > + case IEC61937_PC_AC3:
> > + repet = OPORTMXREPET_STRLENGTH_AC3 |
> > + OPORTMXREPET_PMLENGTH_AC3;
> > + pause |= OPORTMXPAUDAT_PAUSEPD_AC3;
> > + break;
> > + case IEC61937_PC_MPA:
> > + repet = OPORTMXREPET_STRLENGTH_MPA |
> > + OPORTMXREPET_PMLENGTH_MPA;
> > + pause |= OPORTMXPAUDAT_PAUSEPD_MPA;
> > + break;
> > + case IEC61937_PC_MP3:
> > + repet = OPORTMXREPET_STRLENGTH_MP3 |
> > + OPORTMXREPET_PMLENGTH_MP3;
> > + pause |= OPORTMXPAUDAT_PAUSEPD_MP3;
> > + break;
>
> This looks awfully like compressed audio support... should there be
> integration with the compressed audio API/
Thanks, I'll try it. Is there Documentation in sound/designes/compress-offload.rst?
And best sample is... Intel's driver?
(Summary)
I think I should fix as follows:
- Split DMA, DAI patches from large one
- Validate parameters in hw_params
- Add description about HW SRC (or fix bad name 'srcport')
- Add comments about uniphier_aiodma_irq()
- Expose clocking and audio routing to userspace, or at the very
least machine driver configuration
- Support compress-audio API for S/PDIF
and post V2.
Regards,
--
Katsuhiro Suzuki
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V1 3/4] qcom: spmi-wled: Add support for OVP interrupt handling
From: Bjorn Andersson @ 2017-12-05 4:45 UTC (permalink / raw)
To: Kiran Gunda
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Lee Jones, Daniel Thompson,
Jingoo Han, Richard Purdie, Jacek Anaszewski, Pavel Machek,
Rob Herring, Mark Rutland, Bartlomiej Zolnierkiewicz,
linux-leds-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-owner-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1510834717-21765-4-git-send-email-kgunda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
On Thu 16 Nov 04:18 PST 2017, Kiran Gunda wrote:
> WLED peripheral has over voltage protection(OVP) circuitry and the OVP
> fault is notified through an interrupt. Though this fault condition rising
> is due to an incorrect hardware configuration is mitigated in the hardware,
> it still needs to be detected and handled. Add support for it.
>
> When WLED module is enabled, keep OVP fault interrupt disabled for 10 ms to
> account for soft start delay.
>
> Signed-off-by: Kiran Gunda <kgunda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
> .../bindings/leds/backlight/qcom-spmi-wled.txt | 7 +-
> drivers/video/backlight/qcom-spmi-wled.c | 83 ++++++++++++++++++++++
> 2 files changed, 87 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> index 768608c..d39ee93 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> @@ -92,7 +92,7 @@ The PMIC is connected to the host processor via SPMI bus.
> Usage: optional
> Value type: <string>
> Definition: Interrupt names associated with the interrupts.
> - Must be "sc-irq".
> + Currently supported interrupts are "sc-irq" and "ovp-irq".
>
As before, we know this is an IRQ, so omit the -irq from the name.
[..]
> diff --git a/drivers/video/backlight/qcom-spmi-wled.c b/drivers/video/backlight/qcom-spmi-wled.c
[..]
> @@ -115,6 +123,28 @@ static int qcom_wled_module_enable(struct qcom_wled *wled, int val)
> rc = regmap_update_bits(wled->regmap, wled->ctrl_addr +
> QCOM_WLED_CTRL_MOD_ENABLE, QCOM_WLED_CTRL_MOD_EN_MASK,
> val << QCOM_WLED_CTRL_MODULE_EN_SHIFT);
> + if (rc < 0)
> + return rc;
> + /*
> + * Wait for at least 10ms before enabling OVP fault interrupt after
> + * enabling the module so that soft start is completed. Keep the OVP
> + * interrupt disabled when the module is disabled.
> + */
> + if (val) {
> + usleep_range(QCOM_WLED_SOFT_START_DLY_US,
> + QCOM_WLED_SOFT_START_DLY_US + 1000);
This is sleeping in the brightness/enable code path, can you
schedule_delayed_work() instead to not block this code path
unnecessarily?
> +
> + if (wled->cfg.ovp_irq > 0 && wled->ovp_irq_disabled) {
> + enable_irq(wled->cfg.ovp_irq);
> + wled->ovp_irq_disabled = false;
> + }
> + } else {
> + if (wled->cfg.ovp_irq > 0 && !wled->ovp_irq_disabled) {
> + disable_irq(wled->cfg.ovp_irq);
> + wled->ovp_irq_disabled = true;
> + }
> + }
> +
> return rc;
> }
>
> @@ -264,12 +294,42 @@ static irqreturn_t qcom_wled_sc_irq_handler(int irq, void *_wled)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t qcom_wled_ovp_irq_handler(int irq, void *_wled)
> +{
> + struct qcom_wled *wled = _wled;
> + int rc;
> + u32 int_sts, fault_sts;
> +
> + rc = regmap_read(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_INT_RT_STS, &int_sts);
> + if (rc < 0) {
> + pr_err("Error in reading WLED_INT_RT_STS rc=%d\n", rc);
> + return IRQ_HANDLED;
> + }
> +
> + rc = regmap_read(wled->regmap, wled->ctrl_addr +
> + QCOM_WLED_CTRL_FAULT_STATUS, &fault_sts);
> + if (rc < 0) {
> + pr_err("Error in reading WLED_FAULT_STATUS rc=%d\n", rc);
> + return IRQ_HANDLED;
> + }
> +
> + if (fault_sts &
> + (QCOM_WLED_CTRL_OVP_FAULT_BIT | QCOM_WLED_CTRL_ILIM_FAULT_BIT))
> + pr_err("WLED OVP fault detected, int_sts=%x fault_sts= %x\n",
> + int_sts, fault_sts);
All this function does is print things to the log. When is this
information consumed and by whom? dev_dbg() instead?
> +
> + return IRQ_HANDLED;
> +}
> +
> static int qcom_wled_setup(struct qcom_wled *wled)
> {
> int rc, temp, i;
> u8 sink_en = 0;
> + u32 val;
> u8 string_cfg = wled->cfg.string_cfg;
> int sc_irq = wled->cfg.sc_irq;
> + int ovp_irq = wled->cfg.ovp_irq;
>
> rc = regmap_update_bits(wled->regmap,
> wled->ctrl_addr + QCOM_WLED_CTRL_OVP,
> @@ -367,6 +427,25 @@ static int qcom_wled_setup(struct qcom_wled *wled)
> }
> }
>
> + if (ovp_irq >= 0) {
As with the previous patch.
[..]
> @@ -539,6 +618,10 @@ static int qcom_wled_configure(struct qcom_wled *wled, struct device *dev)
[..]
> + wled->cfg.ovp_irq = platform_get_irq_byname(wled->pdev, "ovp-irq");
> + if (wled->cfg.ovp_irq < 0)
> + dev_dbg(&wled->pdev->dev, "ovp irq is not used\n");
> +
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V1 2/4] qcom: spmi-wled: Add support for short circuit handling
From: Bjorn Andersson @ 2017-12-05 4:35 UTC (permalink / raw)
To: Kiran Gunda
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Lee Jones, Daniel Thompson,
Jingoo Han, Richard Purdie, Jacek Anaszewski, Pavel Machek,
Rob Herring, Mark Rutland, Bartlomiej Zolnierkiewicz,
linux-leds-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-owner-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1510834717-21765-3-git-send-email-kgunda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
On Thu 16 Nov 04:18 PST 2017, Kiran Gunda wrote:
> Handle the short circuit(SC) interrupt and check if the SC interrupt
> is valid. Re-enable the module to check if it goes away. Disable the
> module altogether if the SC event persists.
>
> Signed-off-by: Kiran Gunda <kgunda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
> .../bindings/leds/backlight/qcom-spmi-wled.txt | 22 ++++
> drivers/video/backlight/qcom-spmi-wled.c | 126 ++++++++++++++++++++-
> 2 files changed, 142 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> index f1ea25b..768608c 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/qcom-spmi-wled.txt
> @@ -74,6 +74,26 @@ The PMIC is connected to the host processor via SPMI bus.
> Definition: Specify if cabc (content adaptive backlight control) is
> needed.
>
> +- qcom,ext-pfet-sc-pro-en
Please use readable names, rather than a bunch of abbreviations.
> + Usage: optional
> + Value type: <bool>
> + Definition: Specify if external PFET control for short circuit
> + protection is needed.
What does this mean? At least change the wording to "...protection is
used".
> +
> +- interrupts
> + Usage: optional
> + Value type: <prop encoded array>
> + Definition: Interrupts associated with WLED. Interrupts can be
> + specified as per the encoding listed under
> + Documentation/devicetree/bindings/spmi/
> + qcom,spmi-pmic-arb.txt.
> +
> +- interrupt-names
> + Usage: optional
> + Value type: <string>
> + Definition: Interrupt names associated with the interrupts.
> + Must be "sc-irq".
This is obviously an irq, so no need to include that in the name. I
would also prefer if you use the name "short" to make this easier to
read.
> +
> Example:
>
> qcom-wled@d800 {
> @@ -82,6 +102,8 @@ qcom-wled@d800 {
> reg-names = "qcom-wled-ctrl-base", "qcom-wled-sink-base";
> label = "backlight";
>
> + interrupts = <0x3 0xd8 0x2 IRQ_TYPE_EDGE_RISING>;
We tend to write these on the form <decimal, hex, decimal, enum>, please
follow this.
> + interrupt-names = "sc-irq";
> qcom,fs-current-limit = <25000>;
> qcom,current-boost-limit = <970>;
> qcom,switching-freq = <800>;
> diff --git a/drivers/video/backlight/qcom-spmi-wled.c b/drivers/video/backlight/qcom-spmi-wled.c
> index 14c3adc..7dbaaa7 100644
> --- a/drivers/video/backlight/qcom-spmi-wled.c
> +++ b/drivers/video/backlight/qcom-spmi-wled.c
> @@ -11,6 +11,9 @@
> * GNU General Public License for more details.
> */
>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/ktime.h>
> #include <linux/kernel.h>
> #include <linux/backlight.h>
> #include <linux/module.h>
> @@ -23,7 +26,13 @@
> #define QCOM_WLED_DEFAULT_BRIGHTNESS 2048
> #define QCOM_WLED_MAX_BRIGHTNESS 4095
>
> +#define QCOM_WLED_SC_DLY_MS 20
> +#define QCOM_WLED_SC_CNT_MAX 5
> +#define QCOM_WLED_SC_RESET_CNT_DLY_US 1000000
With times of this ballpark you can just use jiffies, with this just
being HZ.
> +
> /* WLED control registers */
> +#define QCOM_WLED_CTRL_FAULT_STATUS 0x08
> +
> #define QCOM_WLED_CTRL_MOD_ENABLE 0x46
> #define QCOM_WLED_CTRL_MOD_EN_MASK BIT(7)
> #define QCOM_WLED_CTRL_MODULE_EN_SHIFT 7
> @@ -37,6 +46,15 @@
> #define QCOM_WLED_CTRL_ILIM 0x4e
> #define QCOM_WLED_CTRL_ILIM_MASK GENMASK(2, 0)
>
> +#define QCOM_WLED_CTRL_SHORT_PROTECT 0x5e
> +#define QCOM_WLED_CTRL_SHORT_EN_MASK BIT(7)
> +
> +#define QCOM_WLED_CTRL_SEC_ACCESS 0xd0
> +#define QCOM_WLED_CTRL_SEC_UNLOCK 0xa5
> +
> +#define QCOM_WLED_CTRL_TEST1 0xe2
> +#define QCOM_WLED_EXT_FET_DTEST2 0x09
> +
> /* WLED sink registers */
> #define QCOM_WLED_SINK_CURR_SINK_EN 0x46
> #define QCOM_WLED_SINK_CURR_SINK_MASK GENMASK(7, 4)
> @@ -71,19 +89,23 @@ struct qcom_wled_config {
> u32 switch_freq;
> u32 fs_current;
> u32 string_cfg;
> + int sc_irq;
Keep data parsed directly from DT in the config and move this to
qcom_wled.
> bool en_cabc;
> + bool ext_pfet_sc_pro_en;
This name is long and hard to parse. "external_pfet" would be much
easier to read.
> };
>
> struct qcom_wled {
> const char *name;
> struct platform_device *pdev;
> struct regmap *regmap;
> + struct mutex lock;
> + struct qcom_wled_config cfg;
> + ktime_t last_sc_event_time;
> u16 sink_addr;
> u16 ctrl_addr;
> u32 brightness;
> + u32 sc_count;
> bool prev_state;
> -
> - struct qcom_wled_config cfg;
Moving this seems unnecessary.
> };
>
> static int qcom_wled_module_enable(struct qcom_wled *wled, int val)
> @@ -157,25 +179,26 @@ static int qcom_wled_update_status(struct backlight_device *bl)
> bl->props.state & BL_CORE_FBBLANK)
> brightness = 0;
>
> + mutex_lock(&wled->lock);
Is this lock necessary?
> +static irqreturn_t qcom_wled_sc_irq_handler(int irq, void *_wled)
> +{
> + struct qcom_wled *wled = _wled;
> + int rc;
> + u32 val;
> + s64 elapsed_time;
> +
> + rc = regmap_read(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_FAULT_STATUS, &val);
> + if (rc < 0) {
> + pr_err("Error in reading WLED_FAULT_STATUS rc=%d\n", rc);
> + return IRQ_HANDLED;
> + }
> +
> + wled->sc_count++;
> + pr_err("WLED short circuit detected %d times fault_status=%x\n",
> + wled->sc_count, val);
Who will read this and is it worth the extra read of FAULT_STATUS just
to produce this print?
> + mutex_lock(&wled->lock);
> + rc = qcom_wled_module_enable(wled, false);
> + if (rc < 0) {
> + pr_err("wled disable failed rc:%d\n", rc);
> + goto unlock_mutex;
> + }
> +
> + elapsed_time = ktime_us_delta(ktime_get(),
> + wled->last_sc_event_time);
> + if (elapsed_time > QCOM_WLED_SC_RESET_CNT_DLY_US) {
> + wled->sc_count = 0;
> + } else if (wled->sc_count > QCOM_WLED_SC_CNT_MAX) {
This isn't really "else elapsed_time was more than DLY_US". Split this
into:
if (elapsed_time > xyz)
wled->sc_count = 0;
if (wled->sc_count > QCOM_WLED_SC_CNT_MAX)
...
> + pr_err("SC trigged %d times, disabling WLED forever!\n",
"forever" as in "until someone turns it on again"?
> + wled->sc_count);
> + goto unlock_mutex;
> + }
> +
> + wled->last_sc_event_time = ktime_get();
> +
> + msleep(QCOM_WLED_SC_DLY_MS);
> + rc = qcom_wled_module_enable(wled, true);
> + if (rc < 0)
> + pr_err("wled enable failed rc:%d\n", rc);
> +
> +unlock_mutex:
> + mutex_unlock(&wled->lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int qcom_wled_setup(struct qcom_wled *wled)
> {
> int rc, temp, i;
> u8 sink_en = 0;
> u8 string_cfg = wled->cfg.string_cfg;
> + int sc_irq = wled->cfg.sc_irq;
>
> rc = regmap_update_bits(wled->regmap,
> wled->ctrl_addr + QCOM_WLED_CTRL_OVP,
> @@ -261,6 +334,39 @@ static int qcom_wled_setup(struct qcom_wled *wled)
> return rc;
> }
>
> + if (sc_irq >= 0) {
I think things will be cleaner if you let qcom_wled_setup() configure
the hardware based on the wled->cfg (as is done to this point) and then
deal with the interrupts in a separate code path from the probe
function.
> + rc = devm_request_threaded_irq(&wled->pdev->dev, sc_irq,
> + NULL, qcom_wled_sc_irq_handler, IRQF_ONESHOT,
> + "qcom_wled_sc_irq", wled);
> + if (rc < 0) {
> + pr_err("Unable to request sc(%d) IRQ(err:%d)\n",
> + sc_irq, rc);
sc_irq is just a number without meaning, no need to print it.
> + return rc;
> + }
> +
> + rc = regmap_update_bits(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_SHORT_PROTECT,
> + QCOM_WLED_CTRL_SHORT_EN_MASK,
> + QCOM_WLED_CTRL_SHORT_EN_MASK);
> + if (rc < 0)
> + return rc;
> +
> + if (wled->cfg.ext_pfet_sc_pro_en) {
> + /* unlock the secure access regisetr */
Spelling of register, and this operation does "Unlock the secure
register access" it doesn't unlock the secure access register.
> + rc = regmap_write(wled->regmap, wled->ctrl_addr +
> + QCOM_WLED_CTRL_SEC_ACCESS,
> + QCOM_WLED_CTRL_SEC_UNLOCK);
> + if (rc < 0)
> + return rc;
> +
> + rc = regmap_write(wled->regmap,
> + wled->ctrl_addr + QCOM_WLED_CTRL_TEST1,
> + QCOM_WLED_EXT_FET_DTEST2);
What is the relationship between DTEST2 and the external FET?
> + if (rc < 0)
> + return rc;
> + }
> + }
> +
> return 0;
> }
>
> @@ -271,6 +377,7 @@ static int qcom_wled_setup(struct qcom_wled *wled)
> .switch_freq = 11,
> .string_cfg = 0xf,
> .en_cabc = 0,
> + .ext_pfet_sc_pro_en = 1,
> };
>
> struct qcom_wled_var_cfg {
> @@ -376,6 +483,7 @@ static int qcom_wled_configure(struct qcom_wled *wled, struct device *dev)
> bool *val_ptr;
> } bool_opts[] = {
> { "qcom,en-cabc", &cfg->en_cabc, },
> + { "qcom,ext-pfet-sc-pro", &cfg->ext_pfet_sc_pro_en, },
> };
>
> prop_addr = of_get_address(dev->of_node, 0, NULL, NULL);
> @@ -427,6 +535,10 @@ static int qcom_wled_configure(struct qcom_wled *wled, struct device *dev)
> *bool_opts[i].val_ptr = true;
> }
>
> + wled->cfg.sc_irq = platform_get_irq_byname(wled->pdev, "sc-irq");
> + if (wled->cfg.sc_irq < 0)
> + dev_dbg(&wled->pdev->dev, "sc irq is not used\n");
> +
Move this to qcom_wled_probe() or into its own code path, together with
the rest of the sc_irq initialization.
And as you're not enabling or disabling it you can store it in a local
variable.
> return 0;
> }
>
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH RFC 1/2] dt-bindings: Add vendor prefix for Brava Home
From: Jagan Teki @ 2017-12-05 4:29 UTC (permalink / raw)
To: Rob Herring
Cc: Maxime Ripard, Chen-Yu Tsai, Icenowy Zheng, Mark Rutland,
Catalin Marinas, Will Deacon, Michael Trimarchi,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
devicetree, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Mark Janoff, Stuart Westerman, linux-sunxi, Jagan Teki
In-Reply-To: <20171204213605.vg3rpz7rscbomduv@rob-hp-laptop>
On Tue, Dec 5, 2017 at 3:06 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Dec 01, 2017 at 12:12:13AM +0530, Jagan Teki wrote:
>> Added 'brava' as a vendor prefix for Brava Home, Inc.
>> which is consumer electronics and IoT company.
>>
>> Signed-off-by: Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org>
>> ---
>> Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
>> 1 file changed, 1 insertion(+)
>
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Thanks, please ignore these RFC's as of now.
--
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.
^ permalink raw reply
* Re: [linux-sunxi] [PATCH 1/8] iio: adc: axp20x_adc: put ADC rate setting in a per-variant function
From: Chen-Yu Tsai @ 2017-12-05 3:35 UTC (permalink / raw)
To: Quentin Schulz
Cc: Sebastian Reichel, Rob Herring, Mark Rutland, Chen-Yu Tsai,
Russell King, Maxime Ripard, Jonathan Cameron, Lee Jones,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
open list:THERMAL, devicetree, linux-kernel, linux-arm-kernel,
linux-iio, Icenowy Zheng, linux-sunxi, Thomas Petazzoni
In-Reply-To: <e97d7332790facf3d10a58feabbfe7b2c0b90db7.1512396054.git-series.quentin.schulz@free-electrons.com>
On Mon, Dec 4, 2017 at 10:12 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> To prepare for a new comer that set a different register with different
> values, move rate setting in a function that is specific to each AXP
> variant.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> drivers/iio/adc/axp20x_adc.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> index a30a972..7274f4f 100644
> --- a/drivers/iio/adc/axp20x_adc.c
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -470,14 +470,18 @@ static const struct iio_info axp22x_adc_iio_info = {
> .read_raw = axp22x_read_raw,
> };
>
> -static int axp20x_adc_rate(int rate)
> +static int axp20x_adc_rate(struct axp20x_adc_iio *info, int rate)
> {
> - return AXP20X_ADC_RATE_HZ(rate);
> + return regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
> + AXP20X_ADC_RATE_MASK,
> + AXP20X_ADC_RATE_HZ(rate));
> }
>
> -static int axp22x_adc_rate(int rate)
> +static int axp22x_adc_rate(struct axp20x_adc_iio *info, int rate)
> {
> - return AXP22X_ADC_RATE_HZ(rate);
> + return regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
> + AXP20X_ADC_RATE_MASK,
> + AXP22X_ADC_RATE_HZ(rate));
> }
>
> struct axp_data {
> @@ -485,7 +489,7 @@ struct axp_data {
> int num_channels;
> struct iio_chan_spec const *channels;
> unsigned long adc_en1_mask;
> - int (*adc_rate)(int rate);
> + int (*adc_rate)(struct axp20x_adc_iio *info, int rate);
Could you also change the name of the callback, to say, adc_set_rate?
This would make it much clearer what the callback does. Previously
it was just a conversion helper.
ChenYu
> bool adc_en2;
> struct iio_map *maps;
> };
> @@ -554,8 +558,7 @@ static int axp20x_probe(struct platform_device *pdev)
> AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK);
>
> /* Configure ADCs rate */
> - regmap_update_bits(info->regmap, AXP20X_ADC_RATE, AXP20X_ADC_RATE_MASK,
> - info->data->adc_rate(100));
> + info->data->adc_rate(info, 100);
>
> ret = iio_map_array_register(indio_dev, info->data->maps);
> if (ret < 0) {
> --
> git-series 0.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH v3 2/3] mailbox: Add support for Hi3660 mailbox
From: Jassi Brar @ 2017-12-05 3:28 UTC (permalink / raw)
To: Xu YiPing
Cc: Rob Herring, Mark Rutland, Wei Xu, Catalin Marinas, Will Deacon,
Devicetree List, Linux Kernel Mailing List,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Guodong Xu, Haojian Zhuang, suzhuangluan-C8/M+/jPZTeaMJb+Lgu22Q,
xuezhiliang-C8/M+/jPZTeaMJb+Lgu22Q,
kevin.wangtao-C8/M+/jPZTeaMJb+Lgu22Q, Zhong Kaihua
In-Reply-To: <1510910672-1409-3-git-send-email-xuyiping-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
On Fri, Nov 17, 2017 at 2:54 PM, Xu YiPing <xuyiping-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org> wrote:
> From: Kaihua Zhong <zhongkaihua-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>
> Hi3660 mailbox controller is used to send message within multiple
> processors, MCU, HIFI, etc. It supports 32 mailbox channels and every
> channel can only be used for single transferring direction. Once the
> channel is enabled, it needs to specify the destination interrupt and
> acknowledge interrupt, these two interrupt vectors are used to create
> the connection between the mailbox and interrupt controllers.
>
> The data transferring supports two modes, one is named as "automatic
> acknowledge" mode so after send message the kernel doesn't need to wait
> for acknowledge from remote and directly return; there have another mode
> is to rely on handling interrupt for acknowledge.
>
> This commit is for initial version driver, which only supports
> "automatic acknowledge" mode to support CPU clock, which is the only
> one consumer to use mailbox and has been verified. Later may enhance
> this driver for interrupt mode (e.g. for supporting HIFI).
>
> Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Ruyi Wang <wangruyi-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Kaihua Zhong <zhongkaihua-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
> drivers/mailbox/Kconfig | 8 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/hi3660-mailbox.c | 322 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 332 insertions(+)
> create mode 100644 drivers/mailbox/hi3660-mailbox.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index c5731e5..4b5d6e9 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -108,6 +108,14 @@ config TI_MESSAGE_MANAGER
> multiple processors within the SoC. Select this driver if your
> platform has support for the hardware block.
>
> +config HI3660_MBOX
> + tristate "Hi3660 Mailbox"
> + depends on ARCH_HISI && OF
> + help
> + An implementation of the hi3660 mailbox. It is used to send message
> + between application processors and other processors/MCU/DSP. Select
> + Y here if you want to use Hi3660 mailbox controller.
> +
> config HI6220_MBOX
> tristate "Hi6220 Mailbox"
> depends on ARCH_HISI
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index d54e412..7d1bd51 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -26,6 +26,8 @@ obj-$(CONFIG_TI_MESSAGE_MANAGER) += ti-msgmgr.o
>
> obj-$(CONFIG_XGENE_SLIMPRO_MBOX) += mailbox-xgene-slimpro.o
>
> +obj-$(CONFIG_HI3660_MBOX) += hi3660-mailbox.o
> +
> obj-$(CONFIG_HI6220_MBOX) += hi6220-mailbox.o
>
> obj-$(CONFIG_BCM_PDC_MBOX) += bcm-pdc-mailbox.o
> diff --git a/drivers/mailbox/hi3660-mailbox.c b/drivers/mailbox/hi3660-mailbox.c
> new file mode 100644
> index 0000000..ba80834
> --- /dev/null
> +++ b/drivers/mailbox/hi3660-mailbox.c
> @@ -0,0 +1,322 @@
> +/*
> + * Hisilicon's Hi3660 mailbox controller driver
> + *
> + * Copyright (c) 2017 Hisilicon Limited.
> + * Copyright (c) 2017 Linaro Limited.
> + *
> + * Author: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
>
We now use SPDX licence
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include "mailbox.h"
> +
> +#define MBOX_CHAN_MAX 32
> +
> +#define MBOX_RX (0x0)
> +#define MBOX_TX (0x1)
> +
> +#define MBOX_BASE(mbox, ch) ((mbox)->base + ((ch) * 0x40))
> +#define MBOX_SRC_REG (0x00)
> +#define MBOX_DST_REG (0x04)
> +#define MBOX_DCLR_REG (0x08)
> +#define MBOX_DSTAT_REG (0x0c)
> +#define MBOX_MODE_REG (0x10)
> +#define MBOX_IMASK_REG (0x14)
> +#define MBOX_ICLR_REG (0x18)
> +#define MBOX_SEND_REG (0x1c)
> +#define MBOX_DATA_REG (0x20)
> +
> +#define MBOX_IPC_LOCK_REG (0xa00)
> +#define MBOX_IPC_UNLOCK (0x1acce551)
> +
> +#define MBOX_AUTOMATIC_ACK (1)
> +
Please, no brackets around numbers.
> +#define MBOX_STATE_IDLE BIT(4)
> +#define MBOX_STATE_ACK BIT(7)
> +
> +#define MBOX_MSG_LEN 8
> +
> +/**
> + * Hi3660 mailbox channel device data
> + *
> + * A channel can be used for TX or RX, it can trigger remote
> + * processor interrupt to notify remote processor and can receive
> + * interrupt if has incoming message.
> + *
> + * @dst_irq: Interrupt vector for remote processor
> + * @ack_irq: Interrupt vector for local processor
> + */
> +struct hi3660_mbox_dev {
>
Better than dev, maybe hi3660_chan_info ?
> + unsigned int dst_irq;
> + unsigned int ack_irq;
> +};
> +
> +/**
> + * Hi3660 mailbox controller data
> + *
> + * Mailbox controller includes 32 channels and can allocate
> + * channel for message transferring.
> + *
> + * @dev: Device to which it is attached
> + * @base: Base address of the register mapping region
> + * @chan: Representation of channels in mailbox controller
> + * @mdev: Representation of channel device data
> + * @controller: Representation of a communication channel controller
> + */
> +struct hi3660_mbox {
> + struct device *dev;
> + void __iomem *base;
> + struct mbox_chan chan[MBOX_CHAN_MAX];
> + struct hi3660_mbox_dev mdev[MBOX_CHAN_MAX];
>
Maybe mchan, instead of mdev.
> + struct mbox_controller controller;
> +};
> +
> +static inline struct hi3660_mbox *to_hi3660_mbox(struct mbox_controller *mbox)
> +{
> + return container_of(mbox, struct hi3660_mbox, controller);
> +}
> +
> +static int hi3660_mbox_check_state(struct mbox_chan *chan)
> +{
> + unsigned long ch = (unsigned long)chan->con_priv;
> + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> + struct hi3660_mbox_dev *mdev = &mbox->mdev[ch];
> + void __iomem *base = MBOX_BASE(mbox, ch);
> + unsigned long val;
> + unsigned int state, ret;
> +
> + /* Mailbox is idle so directly bail out */
> + state = readl_relaxed(base + MBOX_MODE_REG);
> + if (state & MBOX_STATE_IDLE)
> + return 0;
> +
> + /* Wait for acknowledge from remote */
> + ret = readx_poll_timeout_atomic(readl_relaxed, base + MBOX_MODE_REG,
> + val, (val & MBOX_STATE_ACK), 1000, 300000);
> + if (ret) {
> + dev_err(mbox->dev, "%s: timeout for receiving ack\n", __func__);
> + return ret;
> + }
> +
> + /* Ensure channel is released */
> + writel_relaxed(0xffffffff, base + MBOX_IMASK_REG);
> + writel_relaxed(BIT(mdev->ack_irq), base + MBOX_SRC_REG);
> +
> + return 0;
> +}
> +
> +static int hi3660_mbox_unlock(struct mbox_chan *chan)
> +{
> + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> + unsigned int val, retry = 3;
> +
> + do {
> + writel_relaxed(MBOX_IPC_UNLOCK, mbox->base + MBOX_IPC_LOCK_REG);
> +
> + val = readl_relaxed(mbox->base + MBOX_IPC_LOCK_REG);
> + if (!val)
> + break;
> +
> + udelay(10);
> + } while (retry--);
> +
> + return (!val) ? 0 : -ETIMEDOUT;
> +}
> +
> +static int hi3660_mbox_acquire_channel(struct mbox_chan *chan)
> +{
> + unsigned long ch = (unsigned long)chan->con_priv;
> + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> + struct hi3660_mbox_dev *mdev = &mbox->mdev[ch];
> + void __iomem *base = MBOX_BASE(mbox, ch);
> + unsigned int val, retry;
> +
> + for (retry = 10; retry; retry--) {
> + /* Check if channel has been acquired */
> + if (readl_relaxed(base + MBOX_MODE_REG) & MBOX_STATE_IDLE) {
> + writel_relaxed(BIT(mdev->ack_irq), base + MBOX_SRC_REG);
> + val = readl_relaxed(base + MBOX_SRC_REG);
> + if (val & BIT(mdev->ack_irq))
> + break;
> + }
> + }
> +
> + return retry ? 0 : -ETIMEDOUT;
> +}
> +
> +static int hi3660_mbox_send(struct mbox_chan *chan, u32 *msg)
> +{
> + unsigned long ch = (unsigned long)chan->con_priv;
> + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> + struct hi3660_mbox_dev *mdev = &mbox->mdev[ch];
> + void __iomem *base = MBOX_BASE(mbox, ch);
> + unsigned int i;
> +
> + /* Clear mask for destination interrupt */
> + writel_relaxed(~BIT(mdev->dst_irq), base + MBOX_IMASK_REG);
> +
> + /* Config destination for interrupt vector */
> + writel_relaxed(BIT(mdev->dst_irq), base + MBOX_DST_REG);
> +
> + /* Automatic acknowledge mode */
> + writel_relaxed(MBOX_AUTOMATIC_ACK, base + MBOX_MODE_REG);
> +
> + /* Fill message data */
> + for (i = 0; i < MBOX_MSG_LEN; i++)
> + writel_relaxed(msg[i], base + MBOX_DATA_REG + i * 4);
> +
> + /* Trigger data transferring */
> + writel_relaxed(BIT(mdev->ack_irq), base + MBOX_SEND_REG);
> + return 0;
> +}
> +
> +static int hi3660_mbox_send_data(struct mbox_chan *chan, void *msg)
> +{
> + struct hi3660_mbox *mbox = to_hi3660_mbox(chan->mbox);
> + int err;
> +
> + err = hi3660_mbox_check_state(chan);
> + if (err) {
> + dev_err(mbox->dev, "checking state failed\n");
> + return err;
> + }
> +
> + err = hi3660_mbox_unlock(chan);
> + if (err) {
> + dev_err(mbox->dev, "unlocking mailbox failed\n");
> + return err;
> + }
> +
> + err = hi3660_mbox_acquire_channel(chan);
> + if (err) {
> + dev_err(mbox->dev, "acquiring channel failed\n");
> + return err;
> + }
> +
Please remember .send_data() is called with irqs disabled spinlock
All these polling functions hurt. Please move the above three checks
in last_tx_done()
Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
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