Re: [PATCH v2 0/4] clk: meson: Add video clocks path
On Tue, 2018-11-06 at 15:57 +0100, Neil Armstrong wrote: > This patchset is an attempt to handle the Amlogic Meson GX Video clock > in the Common Clock Framework in order to move the video pipeline and > HDMI controller clock management out of the Meson DRM Driver. > > In order : > - Add support the VID_PLL fully programmable divider used right after the > HDMI PLL clock source. > - Fix the GXL HDMI PLL DCO > - Add the video clock bindings covering all the video graphics pipeline > and the HDMI controller. > - Add the clocks entries used in the video clock path > > The vid_pll programmable divider is introduced in its R/O form right now, > but will be extended to be R/W in a next iteration. > > All dividers are flagged with CLK_GET_RATE_NOCACHE, and all gates are > flagged > with CLK_IGNORE_UNUSED since they are currently directly handled by the > Meson DRM Driver. > > Once the DRM Driver is fully migrated to using the Common Clock Framework > to handle the video clock tree, the CLK_GET_RATE_NOCACHE and > CLK_IGNORE_UNUSED > will be dropped. > > Changes since v1 at [1]: > - Fixed comments from Martin > - Fixed GXL HDMI PLL DCO > - Added the missing HDMI controller clock > - Moved bindings to a separate patch > - Updated the commit logs > > [1] > https://lkml.kernel.org/r/1532079581-978-1-git-send-email-narmstr...@baylibre.com > > Neil Armstrong (4): > clk: meson: Add vid_pll divider driver > clk: meson-gxbb: Fix HDMI PLL for GXL SoCs > dt-bindings: clk: meson-gxbb: Add Video clock bindings > clk: meson-gxbb: Add video clocks > > drivers/clk/meson/Makefile| 2 +- > drivers/clk/meson/clkc.h | 6 + > drivers/clk/meson/gxbb.c | 773 > +- > drivers/clk/meson/gxbb.h | 26 +- > drivers/clk/meson/vid-pll-div.c | 91 > include/dt-bindings/clock/gxbb-clkc.h | 18 + > 6 files changed, 911 insertions(+), 5 deletions(-) > create mode 100644 drivers/clk/meson/vid-pll-div.c > Looks sane enough ;) Acked-by: Jerome Brunet
Re: [PATCH 0/4] nvmem: meson: efuse updates
On Mon, 2018-11-12 at 13:08 +, Srinivas Kandagatla wrote: > > On 30/10/18 10:22, Jerome Brunet wrote: > > The first change of this patchset just adds add error message in case > > of failure. If there is problem with the secure monitor, the > > SM_EFUSE_USER_MAX call will be first one to fail so it is better if it > > give us a clue to help debugging, instead af silently failing. > > > > Next this series adds the peripheral clock missing in this driver. > > Like many other device in amlogic's SoC, the efuse requires a > > peripheral clock to operate. ATM, the clock controller has > > CLK_IGNORE_UNUSED on this clock and we have been lucky enough that the > > bootloader left the clock enabled > > > > At some point, we would like to remove those CLK_IGNORE_UNUSED, so if a > > driver needs a clock, it needs to properly claim it. > > > > Srinivas, Kevin, > > The dts change needs to land before the actual driver change, to avoid > > breaking the efuse on our users. If there an agreement on this series, > > Kevin could you provide a tag to Srinivas ? > > These are not fixes to any bugs/regressions, so its new material which > can only go in next dev cycle! > It is not fixing a regression, indeed. It fixing a problem in the driver itself, I think I described this above. I don't expect this to go a fixes for 4.20. I was targeting next. > I guess that should also address the patch sequencing issue! Same issue still applies. If there is an agreement on this series, Patch 3 must land before patch 4 to avoid any problems for our users. I mentionning it because patch 3 is supposed to go through Kevin's tree, while the rest go through yours > > Also I need ack from dt-maintaners on clk bindings to pick patch 2 and 4. I understand for patch 2, but don't really get it for patch 4 ? > > thanks, > srini > > > Cheers > > Jerome
Re: [PATCH v3 3/3] ARM64: dts: meson-gxl: Add support for the Smartlabs SML-5442TW
On Thu, 2018-11-08 at 23:43 +0400, Christian Hewitt wrote:
> +/* This will enable the bluetooth module */
> +&gpio {
> + bt-en {
> + gpio-hog;
> + gpios = ;
> + output-high;
> + line-name = "bt-en";
> + };
> +};
Instead of this, is it possible to use the driver provided by
drivers/bluetooth/hci_qca.c ?
Re: [PATCH v2 3/3] ARM64: dts: meson-gxl: Add support for the Smartlabs SML-5442TW
On Thu, 2018-11-08 at 21:01 +0400, Christian Hewitt wrote:
> The Smartlabs SML-5442TW is broadly similar to the P231 reference design
> but with the following differences:
>
> - Yellow and Blue front-panel LEDs are available but disabled
> - Red/Green LED is used to signal off/on status
> - GPIOX_17 is set high to enable the QCA9377 wireless module
> - uart_AO can be accessed after opening the case; soldered pins exist
>
> Signed-off-by: Christian Hewitt
> ---
> arch/arm64/boot/dts/amlogic/Makefile | 1 +
> .../boot/dts/amlogic/meson-gxl-s905d-sml5442tw.dts | 316
> +
> 2 files changed, 317 insertions(+)
> create mode 100644 arch/arm64/boot/dts/amlogic/meson-gxl-s905d-
> sml5442tw.dts
>
> diff --git a/arch/arm64/boot/dts/amlogic/Makefile
> b/arch/arm64/boot/dts/amlogic/Makefile
> index c31f29d6..37d7dbd 100644
> --- a/arch/arm64/boot/dts/amlogic/Makefile
> +++ b/arch/arm64/boot/dts/amlogic/Makefile
> @@ -18,6 +18,7 @@ dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-nexbox-
> a95x.dtb
> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-p212.dtb
> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905d-p230.dtb
> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905d-p231.dtb
> +dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905d-sml5442tw.dtb
> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s805x-p241.dtb
> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905w-p281.dtb
> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905w-tx3-mini.dtb
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905d-sml5442tw.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxl-s905d-sml5442tw.dts
> new file mode 100644
> index 000..a081984
> --- /dev/null
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905d-sml5442tw.dts
> @@ -0,0 +1,316 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2016 Endless Computers, Inc.
> + * Author: Carlo Caione
> + * Copyright (c) 2018 BayLibre, SAS
> + * Author: Neil Armstrong
> + */
> +
> +/dts-v1/;
> +
> +#include "meson-gxl-s905d.dtsi"
> +
> +/ {
> + compatible = "smartlabs,sml5442tw", "amlogic,s905d", "amlogic,meson-
> gxl";
> + model = "Smartlabs SML-5442TW";
> +
> + aliases {
> + serial0 = &uart_AO;
> + serial1 = &uart_A;
> + ethernet0 = ðmac;
> + };
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + memory@0 {
> + device_type = "memory";
> + reg = <0x0 0x0 0x0 0x8000>;
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> +
> + yellow {
> + label = "sml5442tw:yellow";
> + gpios = <&gpio_ao GPIOAO_6 GPIO_ACTIVE_HIGH>;
> + default-state = "off";
> + };
> +
> + blue {
> + label = "sml5442tw:blue";
> + gpios = <&gpio GPIODV_28 GPIO_ACTIVE_HIGH>;
> + default-state = "off";
> + };
> +
> + green {
> + label = "sml5442tw:green";
> + gpios = <&gpio_ao GPIOAO_9 GPIO_ACTIVE_HIGH>;
> + default-state = "on";
> + };
> +
> + red {
> + label = "sml5442tw:red";
> + gpios = <&gpio GPIODV_27 GPIO_ACTIVE_HIGH>;
> + default-state = "off";
> + };
> + };
> +
> + hdmi_5v: regulator-hdmi-5v {
> + compatible = "regulator-fixed";
> +
> + regulator-name = "HDMI_5V";
> + regulator-min-microvolt = <500>;
> + regulator-max-microvolt = <500>;
> +
> + gpio = <&gpio GPIOH_3 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> + regulator-always-on;
> + };
> +
> + vddio_ao18: regulator-vddio_ao18 {
> + compatible = "regulator-fixed";
> + regulator-name = "VDDIO_AO18";
> + regulator-min-microvolt = <180>;
> + regulator-max-microvolt = <180>;
> + };
> +
> + vddio_boot: regulator-vddio_boot {
> + compatible = "regulator-fixed";
> + regulator-name = "VDDIO_BOOT";
> + regulator-min-microvolt = <180>;
> + regulator-max-microvolt = <180>;
> + };
> +
> + vddao_3v3: regulator-vddao_3v3 {
> + compatible = "regulator-fixed";
> + regulator-name = "VDDAO_3V3";
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> + };
> +
> + vcc_3v3: regulator-vcc_3v3 {
> + compatible = "regulator-fixed";
> + regulator-name = "VCC_3V3";
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> + };
> +
> + emmc_pwrseq: emmc-pwrseq {
> + compatible = "mmc-pwrseq-emmc";
> + reset-gpios = <&gpio BOOT_9 GPIO_ACTIVE_LOW>;
> + };
> +
> + wifi32k: wifi32k {
> +
Re: [PATCH 3/4] Documentation: bindings: Add missing Amlogic SCPI sensor bindings
On Thu, 2018-11-08 at 16:04 +, Sudeep Holla wrote: > On Thu, Nov 08, 2018 at 02:53:51PM +0100, Jerome Brunet wrote: > > amlogic,meson-gxbb-scpi-sensors is both the driver and DT but is not > > documented. Just add it to amlogic's scpi documentation > > > > Signed-off-by: Jerome Brunet > > --- > > Documentation/devicetree/bindings/arm/amlogic,scpi.txt | 7 +++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/arm/amlogic,scpi.txt > > b/Documentation/devicetree/bindings/arm/amlogic,scpi.txt > > index 7b9a861e9306..5ab59da052df 100644 > > --- a/Documentation/devicetree/bindings/arm/amlogic,scpi.txt > > +++ b/Documentation/devicetree/bindings/arm/amlogic,scpi.txt > > @@ -17,4 +17,11 @@ Required sub-node properties: > > - compatible : should be "amlogic,meson-gxbb-scp-shmem" for SRAM based > > shared > > memory on Amlogic GXBB SoC. > > > > +Sensor bindings for the sensors based on SCPI Message Protocol > > +-- > > +SCPI provides an API to access the various sensors on the SoC. > > + > > +Required properties: > > +- compatible : should be "amlogic,meson-gxbb-scpi-sensors". > > + > > Not sure if it's worth mentioning the reason for being able to identify > amlogic,meson-gxbb sensors from generic scpi ones. IIRC it was for the > unit values of these sensors are different ? But I will leave that to > DT or platform maintainers' taste. Yes Sudeep, the unit are different. If we probe on the generic one, it will "work" but the result is just not good. BTW, the point of this patch is merely to remove an existing issue. The scpi driver and meson-gx.dtsi are both refering to an undocumented compatible. > > -- > Regards, > Sudeep
Re: [PATCH 3/3] ARM64: dts: meson-gxl: Add support for the Smartlabs SML-5442TW
On Thu, 2018-11-08 at 19:33 +0400, Christian Hewitt wrote:
>
> +&audio {
> + status = "okay";
> +};
> +
> +&aiu_i2s_dma {
> + status = "okay";
> +};
> +
> +&i2s_dai {
> + status = "okay";
> +};
The audio stuff has not made it's way upstream (yet) please drop this
> +
> +&cvbs_vdac_port {
> + cvbs_vdac_out: endpoint {
> + remote-endpoint = <&cvbs_connector_in>;
> + };
> +};
> +
>
[ ... ]
> +
> +/* This is connected to the Bluetooth module: */
> +&uart_A {
> + status = "okay";
> + pinctrl-0 = <&uart_a_pins>, <&uart_a_cts_rts_pins>;
> + pinctrl-names = "default";
Did you forget to put 'uart-has-rtscts;' here ?
> +};
> +
> +/* This UART is brought out to the debug header */
> +&uart_AO {
> + status = "okay";
> + pinctrl-0 = <&uart_ao_a_pins>;
> + pinctrl-names = "default";
> +};
> +
> +&usb0 {
> + status = "okay";
> +};
Also, you sent a multipatch series. Such patchset should start with a cover
letter describing the general intent ... even if it is obvious.
Re: [PATCH v4 0/2] clk: meson-g12a: Add EE clock controller driver
On Thu, 2018-11-08 at 21:15 +0800, Jian Hu wrote: > Changes since v3 at[4] > -add fixed clocks clk_regmap definition Jian, When replying to the v1 of your clk_ao patchset : https://patchwork.kernel.org/patch/10562563/#22177627 I have explained that I would like to stop taking clock (such as xtal) from nowhere. Clocks (even the xtal) should be properly claimed through DT. I have specifically asked this to be taken into account for the EE controller. If you need an exemple on how to get the input clock from DT to your controller, please have look at the axg audio clock controller. This comment does not appear to be addressed in this version. Please make sure you have addressed all the comments of past reviews before reposting Thx > > Changes since v2 at[2] > -fix fixed clocks's descriptions > -fix aligment > -add enable bit for plls base on [3] patches > -add fixed clock gate bit > > Changes since v1 at[1] > -fix typo of 'Everything'. > -change the word 'AmLogic' to 'Amlogic' > -squash patch 1 and 2. > -delete usless message of "Trying obsolete regs". > -delete the empty line in include/dt-bindings/clock/g12a-clkc.h. > -rebase on top of the "next/drivers" branch, and add g12a clock patch. > -add CLK_MUX_ROUND_CLOSEST for g12a_sd_emmc_b_clk0_sel and > g12a_sd_emmc_c_clk0_sel. > > [1] > https://lkml.kernel.org/r/1531133549-25806-2-git-send-email-jian...@amlogic.com > [2] > https://lkml.kernel.org/r/1531728707-192230-2-git-send-email-jian...@amlogic.com > [3]https://lkml.kernel.org/r/20180717095617.12240-1-jbru...@baylibre.com > [4] > https://lkml.kernel.org/r/1533890858-113020-1-git-send-email-jian...@amlogic.com > > Jian Hu (2): > dt-bindings: clk: meson-g12a: Add G12A EE Clock Bindings > clk: meson-g12a: Add EE Clock controller driver > > .../bindings/clock/amlogic,gxbb-clkc.txt |1 + > drivers/clk/meson/Kconfig | 10 + > drivers/clk/meson/Makefile |1 + > drivers/clk/meson/g12a.c | 1134 > > drivers/clk/meson/g12a.h | 128 +++ > include/dt-bindings/clock/g12a-clkc.h | 93 ++ > 6 files changed, 1367 insertions(+) > create mode 100644 drivers/clk/meson/g12a.c > create mode 100644 drivers/clk/meson/g12a.h > create mode 100644 include/dt-bindings/clock/g12a-clkc.h >
Re: [PATCH v2] clk: meson-gxbb: set fclk_div3 as CLK_IS_CRITICAL
On Tue, 2018-11-06 at 10:43 -0800, Stephen Boyd wrote:
> Quoting Jerome Brunet (2018-11-05 15:08:20)
> > From: Christian Hewitt
> >
> > On the Khadas VIM2 (GXM) and LePotato (GXL) board there are problems
> > with reboot; e.g. a ~60 second delay between issuing reboot and the
> > board power cycling (and in some OS configurations reboot will fail
> > and require manual power cycling).
> >
> > Similar to 'commit c987ac6f1f088663b6dad39281071aeb31d450a8 ("clk:
> > meson-gxbb: set fclk_div2 as CLK_IS_CRITICAL")' the SCPI Cortex-M4
> > Co-Processor seems to depend on FCLK_DIV3 being operational.
> >
> > Until commit 05f814402d6174369b3b29832cbb5eb5ed287059 ("clk:
> > meson: add fdiv clock gates"), this clock was modeled and left on by
> > the bootloader.
> >
> > We don't have precise documentation about the SCPI Co-Processor and
> > its clock requirement so we are learning things the hard way.
> >
> > Marking this clock as critical solves the problem but it should not
> > be viewed as final solution. Ideally, the SCPI driver should claim
> > these clocks. We also depends on some clock hand-off mechanism
> > making its way to CCF, to make sure the clock stays on between its
> > registration and the SCPI driver probe.
> >
> > Fixes: 05f814402d61 ("clk: meson: add fdiv clock gates")
> > Signed-off-by: Christian Hewitt
> > Signed-off-by: Jerome Brunet
> > ---
>
> I can toss this into clk-fixes?
>
Sure, it would be great. Thx Stephen.
Re: [PATCH v5 3/3] clk: meson: add sub MMC clock controller driver
On Sun, 2018-11-04 at 02:01 +0800, Jianxin Pan wrote:
> Hi Jerome,
>
> Thanks for the review, we really appreciate your time.
>
> I'm very sorry maybe I don't catch all your meaning very well.
>
> Please see my comments below.
>
> On 2018/10/29 3:16, Jerome Brunet wrote:
> > On Thu, 2018-10-25 at 22:58 +0200, Martin Blumenstingl wrote:
> > > Hi Jerome,
> > >
> > > On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet
> > > wrote:
> > > [snip]
> > > > > > > +static void clk_regmap_div_init(struct clk_hw *hw)
> > > > > > > +{
> > > > > > > + struct clk_regmap *clk = to_clk_regmap(hw);
> > > > > > > + struct clk_regmap_div_data *div =
> > > > > > > clk_get_regmap_div_data(clk);
> > > > > > > + unsigned int val;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + ret = regmap_read(clk->map, div->offset, &val);
> > > > > > > + if (ret)
> > > > > > > + return;
> > > > > > >
> > > > > > > + val &= (clk_div_mask(div->width) << div->shift);
> > > > > > > + if (!val)
> > > > > > > + regmap_update_bits(clk->map, div->offset,
> > > > > > > +clk_div_mask(div->width) << div-
> > > > > > > >shift,
> > > > > > > +clk_div_mask(div->width));
> > > > > >
> > > > > > This is wrong for several reasons:
> > > > > > * You should hard code the initial value in the driver.
> > > > > > * If shift is not 0, I doubt this will give the expected result.
> > > > >
> > > > > The value 0x00 of divider means nand clock off then read/write nand
> > > > > register is forbidden.
> > > >
> > > > That is not entirely true, you can access the clock register or you'd
> > > > be in a
> > > > chicken and egg situation.
> > > >
> > > > > Should we set the initial value in nand driver, or in sub emmc clk
> > > > > driver?
> > > >
> > > > In the nand driver, which is the consumer of the clock. see my
> > > > previous comments
> > > > about it.
> > >
> > > an old version of this series had the code still in the NAND driver
> > > (by writing to the registers directly instead of using the clk API).
> > > this looks pretty much like a "sclk-div" to me (as I commented in v3
> > > of this series: [0]):
> > > - value 0 means disabled
> > > - positive divider values
> > > - (probably no duty control, but that's optional as far as I
> > > understand sclk-div)
> > > - uses max divider value when enabling the clock
> > >
> > > if switching to sclk-div works then we can get rid of some duplicate
> > > code
> >
> > It is possible:
> > There is a couple of things to note though:
> >
> > * sclk does not 'uses max divider value when enabling the clock': Since
> > this
> > divider can gate, it needs to save the divider value when disabling, since
> > the
> > divider value is no longer stored in the register,
> > On init, this cached value is saved as it is. If the divider is initially
> > disabled, we have to set the cached value to something that makes sense,
> > in case
> > the clock is enabled without a prior call to clk_set_rate().
> > > So in sclk, the clock setting is not changed nor hard coded in init, and
> > > this is
> > a very important difference.
> >
> I think It's ok for the latest sub mmc clock and nand driver now:
> 1. in mmc_clkc_register_clk_with_parent("div", ...) from mmc_clkc_probe():
>cached_div is set to div_max durning clk register，but is not set to div
> hw register.
>
> 2. In meson nand driver v6:
> https://lore.kernel.org/lkml/1541090542-19618-3-git-send-email-jianxin@amlogic.com
> 1) In meson_nfc_clk_init() from probe: get clock handle, then
> prepare_enable and set default rate.
>nfc->device_clk = devm_clk_get(nfc->dev, "device");
>ret = clk_prepare_enable(nfc->device_clk); //Here div hw
> register changed from 0 -> cached_div.
>default_clk_rate = clk_round_rate(nfc->device_clk, 2400);
>ret = clk_set_rate(nfc->device_clk, default_clk_rate); //Then
> register and cached_div are both updated to the default 24M.
> 2) In meson_nfc_select_chip(), set the actual frequency
> ret = clk_set_rate(nfc->device_clk, meson_chip->clk_rate); //Here
> register and cached_div are changed again.
> 3) if clk_disable() is called, set div hw register to zero, and
> cached_div keep unchagned.
>if clk_disable() is called again, cached_div is restored to div hw
> register.
You don't need to do all this in your NAND driver: enable - round - set_rate -
disable is a waste of time.
Directly calling set_rate(2400), with the clock still off, will have the
same result. Then if your HW needs this clock to be ON to access registers
(like you told us) you should probably turn it on.
>
> When enabling the clock, divider register does not need to be div_max.
> Any value is OK except ZERO, the cached_div from init or set_rate is ok
> >
> > * Even if sclk zero value means gated, it is still a zero based divider,
> > while
> > eMMC/Nand divider is one based. It this controller was to sclk, then
> > something
> > needs
Re: [PATCH v6 3/3] clk: meson: add sub MMC clock controller driver
On Fri, 2018-11-02 at 00:30 +0800, Jianxin Pan wrote:
> +struct meson_sclk_div_data mmc_clkc_div_data = {
> + .div = {
> + .reg_off = SD_EMMC_CLOCK,
> + .shift = (0),
> + .width = (6),
> + },
> + .hi = {
> + .reg_off = 0,
> + .shift = 0,
> + .width = 0,
> + },
> +};
Jianxin,
When replying to v5: https://patchwork.kernel.org/patch/10646723/#22288117
I think I have clearly explained that:
a. sclk needs some change you want to use it for the eMMC (not done )
b. you can't declare sclk statically like that since there is cached data in
it and this would forbib multiple instance of the controller which is not
acceptable for this pariticular controller
This is just not adressed in your series.
Also some comments from Martin about useless definition were already given on
past version.
Seeing this, I did not review the rest of series.
Please make sure you have addressed all the comments of past reviews before
reposting. It is OK to ask questions if things are unclear.
Jerome
Re: [PATCH 3/3] clk: meson: clk-pll: drop hard-coded rates from pll tables
On Sat, 2018-07-21 at 23:34 +0200, Martin Blumenstingl wrote: > On Sat, Jul 21, 2018 at 10:46 PM Jerome Brunet wrote: > > > > On Sat, 2018-07-21 at 22:16 +0200, Martin Blumenstingl wrote: > > > > We could even add ranges instead of table when we know the PLL supports > > > > a well-known continuous dividers range. > > > > > > I had a look at the sys_pll settings on Meson8b, here's what > > > Meson8/Meson8b/Meson8m2 support for sys_pll: > > > - 50..74 > > > - 76 > > > - 78 > > > - 80 > > > - 82 > > > - 84 > > > - 86 > > > - 88 > > > - 90 > > > - 92 > > > - 94 > > > - 96 > > > - 98 > > > > Are those values with the same predivider (n) value ? > > yes, all are using n = 1 The table proposed in this patch keeps things the way they were before the change. We could extend the table with these values in a follow up patch. If those value are with n=1, then I would guess that odd values from 75 to 97 work as well.
Re: [PATCH 14/15] ASoC: meson: add axg sound card DT binding documentation
On Wed, 2018-07-25 at 13:06 -0600, Rob Herring wrote: > > +- amlogic,aux-devs : List of phandles pointing to auxiliary devices > > +- amlogic,widgets : Please refer to widgets.txt. > > +- amlogic,routing : A list of the connections between audio components. > > audio-routing IIRC Wouldn't it make sense to do the same for widgets and aux-devs then ? audio-widgets ? audio-aux-devs ?
Re: [PATCH 10/14] arm64: dts: meson-axg: add linein codec
On Wed, 2018-07-25 at 21:18 +0200, Martin Blumenstingl wrote:
> Hi Jerome,
>
> On Tue, Jul 24, 2018 at 3:09 PM Jerome Brunet wrote:
> >
> > Add the es7241 analog to digital converter which is fed by the
> > lienin jack of the s400
> >
> > Signed-off-by: Jerome Brunet
> > ---
> > arch/arm64/boot/dts/amlogic/meson-axg-s400.dts | 10 ++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-axg-s400.dts
> > b/arch/arm64/boot/dts/amlogic/meson-axg-s400.dts
> > index 7489b88f27d7..fb101a1c6660 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-axg-s400.dts
> > +++ b/arch/arm64/boot/dts/amlogic/meson-axg-s400.dts
> > @@ -178,6 +178,16 @@
> > gpios = <&gpio_speaker 2 0>;
> > };
> > };
> > +
> > + linein: audio-codec@0 {
>
> there is a unit-address but no reg property - I am wondering if this is
> allowed?
> for the regulators we're using regulator- instead of
> regulator@
>
> (this applies to other patches in this series as well)
I did that because I tried to keep node-name generic as described in the DT
spec. The idea of numbering the nodes (without a reg property) in such a way
came from :
Documentation/devicetree/bindings/sound/simple-card.txt:
=> look at the multi dai link example. I'm doing exactly the same on the axg
card's dai-links. Nothing new here.
that being said, section 2.2.1 of the DT specs says:
" If the node has no reg property, the @unit-address must be omitted and the
node-name alone differentiates the node from other nodes at the same level in
the tree "
So apparently, this is used but not ok ?
I could make 'linein' the node-name instead of the label but that wouldn't be
very generic.
We could also have dai-link-2 instead dai-link@2 (audio-codec-3 instead of
audio-codec@3). I seems to be OK with the spec, but it also looks weird ...
Rob, do you have opinion on this ?
>
> > + #sound-dai-cells = <0>;
> > + compatible = "everest,es7241";
> > + VDDA-supply = <&vcc_3v3>;
> > + VDDP-supply = <&vcc_3v3>;
> > + VDDD-supply = <&vcc_3v3>;
> > + status = "okay";
> > + sound-name-prefix = "Linein";
>
> SPDIF output uses capital letters "SPDIFOUT"
> I am not familiar with the sound subsystem (thus I'm not sure where
> this shows up) so I am wondering if the naming should be consistent?
This string is used to prefix the name of the different widgets and controls
provided. It is useful when several devices provides component and control with
the same names. It happens when several instances of a component are used but
also with classic names, such as "Playback"
I don't think there is a rule regarding capitalization.
Some name are all capital in ASoC, some just have the first letter in upper
case. It doesn't change anything technically, and I personally don't care.
>
> Regards
> Martin
Re: [PATCH 00/14] arm64: dts: meson-axg: add audio support
On Wed, 2018-07-25 at 21:11 +0200, Martin Blumenstingl wrote: > nit-pick: one patch uses "arm64: dts: meson-axg: s400" in the subject > while other patches that are touching the s400 board aren't > if you have to re-send this series: can you please use the "arm64: > dts: meson-axg: s400:" prefix for all patches touching the s400 board? hum, do we really have such rule, or do you think we should add one ? Kevin, do you have opinion ? Not that I really mind either way, but prefixes rules are usually there to help maintainer filter the patches. Will such rule help in any way ?
Re: [PATCH 14/15] ASoC: meson: add axg sound card DT binding documentation
On Wed, 2018-07-25 at 13:06 -0600, Rob Herring wrote: > On Tue, Jul 17, 2018 at 05:36:41PM +0200, Jerome Brunet wrote: > > Add the DT binding documentation for axg sound card > > I see Mark already applied, but... It's very recent, no DT using it yet. I can easily have your 3 comments fixed if it is OK with Mark. I'll submit something for this tomorrow.
Re: [PATCH] ASoC: meson: axg-spdifout: select SND_PCM_IEC958
On Tue, 2018-07-24 at 11:36 +0200, Arnd Bergmann wrote:
> When CONFIG_SND_PCM_IEC958 is disabled, we get a link error for the
> new driver:
>
> sound/soc/meson/axg-spdifout.o: In function `axg_spdifout_hw_params':
> axg-spdifout.c:(.text+0x650): undefined reference to
> `snd_pcm_create_iec958_consumer_hw_params'
I used arm64 defconfig to do all my test and this symbol was selected
by the HDMI codec driver, which is probably why missed it.
Thanks a lot for fixing the problem Arnd.
>
> The other users use 'select', so we should do the same here.
>
> Fixes: 53eb4b7aaa04 ("ASoC: meson: add axg spdif output")
> Signed-off-by: Arnd Bergmann
Acked-by: Jerome Brunet
Re: [PATCH v2 0/4] ARM: amlogic: Add spifc support to Amlogic's GXBB family
On Mon, 2016-09-12 at 13:38 -0700, Kevin Hilman wrote: > Jerome Brunet writes: > > > > > This patch series adds the necessary pins, clocks and device tree > > nodes to > > enable the spifc controller on the GXBB family. I had to add the > > nand pins > > in pintctrl as the pinmux setting left by u-boot was conflicting > > with the > > spifc pinmux during my test on the P200. > > This series seems to be missing a patch which enables the SPIfc on > the > P200 board for use with the on-board NOR flash. > > Kevin > Indeed, I did not provide this patch, on purpose. The SPI-NOR at 4U2 on the P200 schematics was not present on the board I have. I assumed this was the case for all other P200 as well. In addition, to enable the SPI-NOR, you would also need to solder something at 4R3 (SPI_CS signal disconnected by default) Finally, all the SPIfc lines are shared with the NAND controller which, like the SPI-NOR, appears on the schematics (4CCN1) but is not soldered on the actual hardware. Of course, I can share such patch for testing purposes if you would like me to. Jerome. > > > > Changes since v1 at : http://lkml.kernel.org/r/1473261223-15412-1-g > > it-send-email-jbru...@baylibre.com > > * Omit patches : > > - dt-bindings: spi-meson: Add GXBB Compatible string > > - spi: meson: Add GXBB compatible > > Sent as dedicated series > > * Omit patch: > > - clk: gxbb: expose spifc clock > > Already applied > > * Rename SPI flash controller pins from spifc_* to nor_* to keep > > the > > name aligned with the datasheet > > > > Jerome Brunet (3): > > pinctrl: amlogic: gxbb: add spi nor pins > > pinctrl: amlogic: gxbb: add nand pins > > ARM64: dts: amlogic: add spi nor pins > > > > Neil Armstrong (1): > > ARM64: dts: meson-gxbb: Add SPIFC node > > > > arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 19 +++ > > drivers/pinctrl/meson/pinctrl-meson-gxbb.c | 37 > > + > > 2 files changed, 56 insertions(+)
Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller
On Tue, 2016-09-06 at 12:04 +0200, Thierry Reding wrote: > On Tue, Sep 06, 2016 at 11:14:45AM +0200, Neil Armstrong wrote: > > > > On 09/06/2016 11:07 AM, Thierry Reding wrote: > > > > > > On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote: > > > > > > > > Hi Thierry, > > > > > > [...] > > > > > > > > > > > > > > > > > > The second bug is in probe(), I understand the point to > > > > allocate > > > > dynamically the channels and attach them to each pwm chip, but > > > > when > > > > calling meson_pwm_init_channels() we get an OOPS because > > > > meson->chip.pwms[i] are allocated in pwmchip_add(). Moving > > > > meson_pwm_init_channels() would fix this, but in case of a clk > > > > PROBE_DEFER, we would need to remove back the pwmchip, which is > > > > a > > > > quite a bad design decision > > > > > > Ah yes... that one again. I remember running into that a while > > > ago with > > > some other driver. To be honest, I think that's a short-coming of > > > the > > > PWM subsystem and the fix would be for PWM chip registration to > > > be split > > > into two parts: pwm_chip_init() and pwm_chip_add(). That way, a > > > chip > > > would be initialized using pwm_chip_init() where the pwms array > > > would be > > > allocated, and pwm_chip_add() would register the chip with the > > > system. > > > > > > Currently a few drivers might be vulnerable to a race condition > > > between > > > registration and implementation (i.e. PWM channels aren't fully > > > set up > > > when they are exposed to users and sysfs). > > > > > > > > > > > The smartest fix I found was to allocate channels in probe, > > > > init them > > > > them attach them after pwmchip_add(): > > > > > > [...] > > > > > > > > > > > That's the race I was talking about above. I suppose it's not too > > > big an > > > issue since other drivers seem to manage, so I'm going to merge > > > your > > > fixed driver. > > > > ok thanks ! > > I've made a few tiny changes (reg -> offset, temporary variable to > track > &channels[i], ...) and pushed it all out. Hopefully that now fixes > any > of the remaining issues. > > > > > > > > > Unless you feel like taking a stab at the > > > pwm_chip_init()/pwm_chip_add() > > > split, in which case your driver would be the first to be race- > > > free. =) > > > > Having he driver upstream is a priority, but having it completely > > race-free would be great! I'll be happy to collaborate to a race- > > free > > pwmchip probe somehow ! > > Fair enough. I'll do some prototyping and keep you in the loop if I > come > up with something that I think will do. > > Thierry Hi Thierry, I have tested the latest version on the P200 (S905), channels E and F. It works as expected. Regards Tested-by: Jerome Brunet
