* Re: [PATCH v2 5/6] ARM: dts: Add generic interconnect target module node for MCAN
From: Tony Lindgren @ 2018-05-30 15:04 UTC (permalink / raw)
To: Faiz Abbas
Cc: linux-kernel, devicetree, linux-omap, linux-arm-kernel, linux-clk,
robh+dt, bcousson, paul, t-kristo
In-Reply-To: <20180530141133.3711-6-faiz_abbas@ti.com>
* Faiz Abbas <faiz_abbas@ti.com> [180530 14:12]:
> The ti-sysc driver provides support for manipulating the idlemodes
> and interconnect level resets.
...
> --- a/arch/arm/boot/dts/dra76x.dtsi
> +++ b/arch/arm/boot/dts/dra76x.dtsi
> @@ -11,6 +11,25 @@
> / {
> compatible = "ti,dra762", "ti,dra7";
>
> + ocp {
> +
> + target-module@0x42c00000 {
> + compatible = "ti,sysc-dra7-mcan";
> + ranges = <0x0 0x42c00000 0x2000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0x42c01900 0x4>,
> + <0x42c01904 0x4>,
> + <0x42c01908 0x4>;
> + reg-names = "rev", "sysc", "syss";
> + ti,sysc-mask = <(SYSC_OMAP4_SOFTRESET |
> + SYSC_DRA7_MCAN_ENAWAKEUP)>;
> + ti,syss-mask = <1>;
> + clocks = <&wkupaon_clkctrl DRA7_ADC_CLKCTRL 0>;
> + clock-names = "fck";
> + };
> + };
> +
> };
Looks good to me except I think the reset won't do anything currently
with ti-sysc.c unless you specfify also "ti,hwmods" for the module?
Can you please check? It might be worth adding the reset function to
ti-sysc.c for non "ti,hwmods" case and that just might remove the
need for any hwmod code for this module.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH 04/11] dt-bindings: spi: Move and adjust the bindings for the fsl-qspi driver
From: Boris Brezillon @ 2018-05-30 15:06 UTC (permalink / raw)
To: Frieder Schrempf
Cc: linux-mtd, linux-spi, dwmw2, computersforpeace, marek.vasut,
richard, miquel.raynal, broonie, david.wolfe, fabio.estevam,
prabhakar.kushwaha, yogeshnarayan.gaur, han.xu, Rob Herring,
Mark Rutland, devicetree, linux-kernel
In-Reply-To: <1527686082-15142-5-git-send-email-frieder.schrempf@exceet.de>
On Wed, 30 May 2018 15:14:33 +0200
Frieder Schrempf <frieder.schrempf@exceet.de> wrote:
> Move the documentation of the old SPI NOR driver to the place of the new
> SPI memory interface based driver and adjust the content to reflect the
> new drivers settings.
Maybe it's better to do that in 2 steps so that people can easily
identify what has changed in the bindings.
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@exceet.de>
> ---
> .../devicetree/bindings/mtd/fsl-quadspi.txt | 65 ------------------
> .../devicetree/bindings/spi/spi-fsl-qspi.txt | 69 ++++++++++++++++++++
> 2 files changed, 69 insertions(+), 65 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
> deleted file mode 100644
> index 483e9cf..0000000
> --- a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
> +++ /dev/null
> @@ -1,65 +0,0 @@
> -* Freescale Quad Serial Peripheral Interface(QuadSPI)
> -
> -Required properties:
> - - compatible : Should be "fsl,vf610-qspi", "fsl,imx6sx-qspi",
> - "fsl,imx7d-qspi", "fsl,imx6ul-qspi",
> - "fsl,ls1021a-qspi"
> - or
> - "fsl,ls2080a-qspi" followed by "fsl,ls1021a-qspi",
> - "fsl,ls1043a-qspi" followed by "fsl,ls1021a-qspi"
> - - reg : the first contains the register location and length,
> - the second contains the memory mapping address and length
> - - reg-names: Should contain the reg names "QuadSPI" and "QuadSPI-memory"
> - - interrupts : Should contain the interrupt for the device
> - - clocks : The clocks needed by the QuadSPI controller
> - - clock-names : Should contain the name of the clocks: "qspi_en" and "qspi".
> -
> -Optional properties:
> - - fsl,qspi-has-second-chip: The controller has two buses, bus A and bus B.
> - Each bus can be connected with two NOR flashes.
> - Most of the time, each bus only has one NOR flash
> - connected, this is the default case.
> - But if there are two NOR flashes connected to the
> - bus, you should enable this property.
> - (Please check the board's schematic.)
> - - big-endian : That means the IP register is big endian
> -
> -Example:
> -
> -qspi0: quadspi@40044000 {
> - compatible = "fsl,vf610-qspi";
> - reg = <0x40044000 0x1000>, <0x20000000 0x10000000>;
> - reg-names = "QuadSPI", "QuadSPI-memory";
> - interrupts = <0 24 IRQ_TYPE_LEVEL_HIGH>;
> - clocks = <&clks VF610_CLK_QSPI0_EN>,
> - <&clks VF610_CLK_QSPI0>;
> - clock-names = "qspi_en", "qspi";
> -
> - flash0: s25fl128s@0 {
> - ....
> - };
> -};
> -
> -Example showing the usage of two SPI NOR devices:
> -
> -&qspi2 {
> - pinctrl-names = "default";
> - pinctrl-0 = <&pinctrl_qspi2>;
> - status = "okay";
> -
> - flash0: n25q256a@0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "micron,n25q256a", "jedec,spi-nor";
> - spi-max-frequency = <29000000>;
> - reg = <0>;
> - };
> -
> - flash1: n25q256a@1 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "micron,n25q256a", "jedec,spi-nor";
> - spi-max-frequency = <29000000>;
> - reg = <1>;
> - };
> -};
> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-qspi.txt b/Documentation/devicetree/bindings/spi/spi-fsl-qspi.txt
> new file mode 100644
> index 0000000..0ee9cd8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-qspi.txt
> @@ -0,0 +1,69 @@
> +* Freescale Quad Serial Peripheral Interface(QuadSPI)
> +
> +Required properties:
> + - compatible : Should be "fsl,vf610-qspi", "fsl,imx6sx-qspi",
> + "fsl,imx7d-qspi", "fsl,imx6ul-qspi",
> + "fsl,ls1021a-qspi", "fsl,ls2080a-qspi"
> + or
> + "fsl,ls1043a-qspi" followed by "fsl,ls1021a-qspi"
> + - reg : the first contains the register location and length,
> + the second contains the memory mapping address and length
> + - reg-names: Should contain the reg names "QuadSPI" and "QuadSPI-memory"
> + - interrupts : Should contain the interrupt for the device
> + - clocks : The clocks needed by the QuadSPI controller
> + - clock-names : Should contain the name of the clocks: "qspi_en" and "qspi".
> +
> +Optional properties:
> + - big-endian : That means the IP registers format is big endian
> +
> +Required SPI slave node properties:
> + - reg: There are two buses (A and B) with two chip selects each.
> + This encodes to which bus and CS the flash is connected:
> + <0>: Bus A, CS 0
> + <1>: Bus A, CS 1
> + <2>: Bus B, CS 0
> + <3>: Bus B, CS 1
> +
> +Example:
> +
> +qspi0: quadspi@40044000 {
> + compatible = "fsl,vf610-qspi";
> + reg = <0x40044000 0x1000>, <0x20000000 0x10000000>;
> + reg-names = "QuadSPI", "QuadSPI-memory";
> + interrupts = <0 24 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&clks VF610_CLK_QSPI0_EN>,
> + <&clks VF610_CLK_QSPI0>;
> + clock-names = "qspi_en", "qspi";
> +
> + flash0: s25fl128s@0 {
> + ....
> + };
> +};
> +
> +Example showing the usage of two SPI NOR devices on bus A:
> +
> +&qspi2 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_qspi2>;
> + status = "okay";
> +
> + flash0: n25q256a@0 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "micron,n25q256a", "jedec,spi-nor";
> + spi-max-frequency = <29000000>;
> + spi-rx-bus-width = <4>;
> + spi-tx-bus-width = <4>;
> + reg = <0>;
> + };
> +
> + flash1: n25q256a@1 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "micron,n25q256a", "jedec,spi-nor";
> + spi-max-frequency = <29000000>;
> + spi-rx-bus-width = <4>;
> + spi-tx-bus-width = <4>;
> + reg = <1>;
> + };
> +};
^ permalink raw reply
* Re: [PATCH v2 6/6] ARM: dts: dra76x: Add MCAN node
From: Tony Lindgren @ 2018-05-30 15:07 UTC (permalink / raw)
To: Faiz Abbas
Cc: linux-kernel, devicetree, linux-omap, linux-arm-kernel, linux-clk,
robh+dt, bcousson, paul, t-kristo
In-Reply-To: <20180530141133.3711-7-faiz_abbas@ti.com>
* Faiz Abbas <faiz_abbas@ti.com> [180530 14:12]:
> From: Franklin S Cooper Jr <fcooper@ti.com>
>
> Add support for the MCAN peripheral which supports both classic
> CAN messages along with the new CAN-FD message.
...
> --- a/arch/arm/boot/dts/dra76x.dtsi
> +++ b/arch/arm/boot/dts/dra76x.dtsi
> @@ -27,6 +27,21 @@
> ti,syss-mask = <1>;
> clocks = <&wkupaon_clkctrl DRA7_ADC_CLKCTRL 0>;
> clock-names = "fck";
> +
> + m_can0: mcan@42c01a00 {
> + compatible = "bosch,m_can";
> + reg = <0x1a00 0x4000>, <0x0 0x18FC>;
> + reg-names = "m_can", "message_ram";
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "int0", "int1";
> + ti,hwmods = "mcan";
The "ti,hwmods" should be in the parent node now. But you may not even need
it, see the reset comment I made for the parent node patch.
> + clocks = <&mcan_clk>, <&l3_iclk_div>;
> + clock-names = "cclk", "hclk";
> + bosch,mram-cfg = <0x0 0 0 32 0 0 1 1>;
> + status = "disabled";
And then you should be able to also leave out status = "disabled" as the
hardware is there for sure and we can idle it. Then a board specific
dts file can set it to "disabled" if reallly needed. Setting everything
manually to "disabled" and then again to "enabled" is the wrong way
around, the default is "enabled".
Regards,
Tony
^ permalink raw reply
* Re: [PATCH 05/11] ARM: dts: Reflect change of FSL QSPI driver and remove unused properties
From: Boris Brezillon @ 2018-05-30 15:10 UTC (permalink / raw)
To: Frieder Schrempf
Cc: linux-mtd, linux-spi, dwmw2, computersforpeace, marek.vasut,
richard, miquel.raynal, broonie, david.wolfe, fabio.estevam,
prabhakar.kushwaha, yogeshnarayan.gaur, han.xu, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Rob Herring, Mark Rutland,
linux-arm-kernel, devicetree, linux-kernel
In-Reply-To: <1527686082-15142-6-git-send-email-frieder.schrempf@exceet.de>
On Wed, 30 May 2018 15:14:34 +0200
Frieder Schrempf <frieder.schrempf@exceet.de> wrote:
> The FSL QSPI driver was moved to the SPI framework and it now
> acts as a SPI controller. Therefore the subnodes need to set
> spi-[rx/tx]-bus-width = <4>, so quad mode is used just as before.
We should try to keep the current behavior even when
spi-[rx/tx]-bus-width are not defined. How about considering
spi-[rx/tx]-bus-width as board constraints and then let the core pick
the best mode based on these constraints plus the SPI NOR chip
limitations.
>
> Also the properties 'bus-num', 'fsl,spi-num-chipselects' and
> 'fsl,spi-flash-chipselects' were never read by the driver and
> can be removed.
>
> The 'reg' properties are adjusted to reflect the what bus and
> chipselect the flash is connected to, as the new driver needs
> this information.
>
> The property 'fsl,qspi-has-second-chip' is not needed anymore
> and will be removed after the old driver was disabled to avoid
> breaking ls1021a-moxa-uc-8410a.dts.
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@exceet.de>
> ---
> arch/arm/boot/dts/imx6sx-sdb-reva.dts | 8 ++++++--
> arch/arm/boot/dts/imx6sx-sdb.dts | 8 ++++++--
> arch/arm/boot/dts/imx6ul-14x14-evk.dtsi | 2 ++
> arch/arm/boot/dts/ls1021a-moxa-uc-8410a.dts | 5 ++---
> 4 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6sx-sdb-reva.dts b/arch/arm/boot/dts/imx6sx-sdb-reva.dts
> index e3533e7..1a6f680 100644
> --- a/arch/arm/boot/dts/imx6sx-sdb-reva.dts
> +++ b/arch/arm/boot/dts/imx6sx-sdb-reva.dts
> @@ -131,13 +131,17 @@
> #size-cells = <1>;
> compatible = "spansion,s25fl128s", "jedec,spi-nor";
> spi-max-frequency = <66000000>;
> + spi-rx-bus-width = <4>;
> + spi-tx-bus-width = <4>;
> };
>
> - flash1: s25fl128s@1 {
> - reg = <1>;
> + flash1: s25fl128s@2 {
> + reg = <2>;
Hm, you're breaking backward compat here. Can we try to re-use the
old numbering scheme instead of patching all DTs?
^ permalink raw reply
* Re: [PATCH 04/11] dt-bindings: spi: Move and adjust the bindings for the fsl-qspi driver
From: Frieder Schrempf @ 2018-05-30 15:14 UTC (permalink / raw)
To: Boris Brezillon
Cc: Mark Rutland, devicetree, yogeshnarayan.gaur, richard,
prabhakar.kushwaha, linux-kernel, Rob Herring, linux-spi,
marek.vasut, han.xu, broonie, linux-mtd, miquel.raynal,
fabio.estevam, david.wolfe, computersforpeace, dwmw2
In-Reply-To: <20180530170648.02c6fc41@bbrezillon>
Hi Boris,
On 30.05.2018 17:06, Boris Brezillon wrote:
> On Wed, 30 May 2018 15:14:33 +0200
> Frieder Schrempf <frieder.schrempf@exceet.de> wrote:
>
>> Move the documentation of the old SPI NOR driver to the place of the new
>> SPI memory interface based driver and adjust the content to reflect the
>> new drivers settings.
>
> Maybe it's better to do that in 2 steps so that people can easily
> identify what has changed in the bindings.
Ok, I can split this.
Thanks,
Frieder
>
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@exceet.de>
>> ---
>> .../devicetree/bindings/mtd/fsl-quadspi.txt | 65 ------------------
>> .../devicetree/bindings/spi/spi-fsl-qspi.txt | 69 ++++++++++++++++++++
>> 2 files changed, 69 insertions(+), 65 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
>> deleted file mode 100644
>> index 483e9cf..0000000
>> --- a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
>> +++ /dev/null
>> @@ -1,65 +0,0 @@
>> -* Freescale Quad Serial Peripheral Interface(QuadSPI)
>> -
>> -Required properties:
>> - - compatible : Should be "fsl,vf610-qspi", "fsl,imx6sx-qspi",
>> - "fsl,imx7d-qspi", "fsl,imx6ul-qspi",
>> - "fsl,ls1021a-qspi"
>> - or
>> - "fsl,ls2080a-qspi" followed by "fsl,ls1021a-qspi",
>> - "fsl,ls1043a-qspi" followed by "fsl,ls1021a-qspi"
>> - - reg : the first contains the register location and length,
>> - the second contains the memory mapping address and length
>> - - reg-names: Should contain the reg names "QuadSPI" and "QuadSPI-memory"
>> - - interrupts : Should contain the interrupt for the device
>> - - clocks : The clocks needed by the QuadSPI controller
>> - - clock-names : Should contain the name of the clocks: "qspi_en" and "qspi".
>> -
>> -Optional properties:
>> - - fsl,qspi-has-second-chip: The controller has two buses, bus A and bus B.
>> - Each bus can be connected with two NOR flashes.
>> - Most of the time, each bus only has one NOR flash
>> - connected, this is the default case.
>> - But if there are two NOR flashes connected to the
>> - bus, you should enable this property.
>> - (Please check the board's schematic.)
>> - - big-endian : That means the IP register is big endian
>> -
>> -Example:
>> -
>> -qspi0: quadspi@40044000 {
>> - compatible = "fsl,vf610-qspi";
>> - reg = <0x40044000 0x1000>, <0x20000000 0x10000000>;
>> - reg-names = "QuadSPI", "QuadSPI-memory";
>> - interrupts = <0 24 IRQ_TYPE_LEVEL_HIGH>;
>> - clocks = <&clks VF610_CLK_QSPI0_EN>,
>> - <&clks VF610_CLK_QSPI0>;
>> - clock-names = "qspi_en", "qspi";
>> -
>> - flash0: s25fl128s@0 {
>> - ....
>> - };
>> -};
>> -
>> -Example showing the usage of two SPI NOR devices:
>> -
>> -&qspi2 {
>> - pinctrl-names = "default";
>> - pinctrl-0 = <&pinctrl_qspi2>;
>> - status = "okay";
>> -
>> - flash0: n25q256a@0 {
>> - #address-cells = <1>;
>> - #size-cells = <1>;
>> - compatible = "micron,n25q256a", "jedec,spi-nor";
>> - spi-max-frequency = <29000000>;
>> - reg = <0>;
>> - };
>> -
>> - flash1: n25q256a@1 {
>> - #address-cells = <1>;
>> - #size-cells = <1>;
>> - compatible = "micron,n25q256a", "jedec,spi-nor";
>> - spi-max-frequency = <29000000>;
>> - reg = <1>;
>> - };
>> -};
>> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-qspi.txt b/Documentation/devicetree/bindings/spi/spi-fsl-qspi.txt
>> new file mode 100644
>> index 0000000..0ee9cd8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-qspi.txt
>> @@ -0,0 +1,69 @@
>> +* Freescale Quad Serial Peripheral Interface(QuadSPI)
>> +
>> +Required properties:
>> + - compatible : Should be "fsl,vf610-qspi", "fsl,imx6sx-qspi",
>> + "fsl,imx7d-qspi", "fsl,imx6ul-qspi",
>> + "fsl,ls1021a-qspi", "fsl,ls2080a-qspi"
>> + or
>> + "fsl,ls1043a-qspi" followed by "fsl,ls1021a-qspi"
>> + - reg : the first contains the register location and length,
>> + the second contains the memory mapping address and length
>> + - reg-names: Should contain the reg names "QuadSPI" and "QuadSPI-memory"
>> + - interrupts : Should contain the interrupt for the device
>> + - clocks : The clocks needed by the QuadSPI controller
>> + - clock-names : Should contain the name of the clocks: "qspi_en" and "qspi".
>> +
>> +Optional properties:
>> + - big-endian : That means the IP registers format is big endian
>> +
>> +Required SPI slave node properties:
>> + - reg: There are two buses (A and B) with two chip selects each.
>> + This encodes to which bus and CS the flash is connected:
>> + <0>: Bus A, CS 0
>> + <1>: Bus A, CS 1
>> + <2>: Bus B, CS 0
>> + <3>: Bus B, CS 1
>> +
>> +Example:
>> +
>> +qspi0: quadspi@40044000 {
>> + compatible = "fsl,vf610-qspi";
>> + reg = <0x40044000 0x1000>, <0x20000000 0x10000000>;
>> + reg-names = "QuadSPI", "QuadSPI-memory";
>> + interrupts = <0 24 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&clks VF610_CLK_QSPI0_EN>,
>> + <&clks VF610_CLK_QSPI0>;
>> + clock-names = "qspi_en", "qspi";
>> +
>> + flash0: s25fl128s@0 {
>> + ....
>> + };
>> +};
>> +
>> +Example showing the usage of two SPI NOR devices on bus A:
>> +
>> +&qspi2 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_qspi2>;
>> + status = "okay";
>> +
>> + flash0: n25q256a@0 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + compatible = "micron,n25q256a", "jedec,spi-nor";
>> + spi-max-frequency = <29000000>;
>> + spi-rx-bus-width = <4>;
>> + spi-tx-bus-width = <4>;
>> + reg = <0>;
>> + };
>> +
>> + flash1: n25q256a@1 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + compatible = "micron,n25q256a", "jedec,spi-nor";
>> + spi-max-frequency = <29000000>;
>> + spi-rx-bus-width = <4>;
>> + spi-tx-bus-width = <4>;
>> + reg = <1>;
>> + };
>> +};
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply
* Re: [PATCH v2 1/6] ARM: dra762: hwmod: Add MCAN support
From: Tero Kristo @ 2018-05-30 15:15 UTC (permalink / raw)
To: Tony Lindgren, Faiz Abbas
Cc: linux-kernel, devicetree, linux-omap, linux-arm-kernel, linux-clk,
robh+dt, bcousson, paul
In-Reply-To: <20180530145047.GC5705@atomide.com>
On 30/05/18 17:50, Tony Lindgren wrote:
> * Faiz Abbas <faiz_abbas@ti.com> [180530 14:12]:
>> From: Lokesh Vutla <lokeshvutla@ti.com>
>>
>> Add MCAN hwmod data and register it for dra762 silicons.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> ---
>> arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 32 +++++++++++++++++++++++
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
>> index 62352d1e6361..a2cd7f865a60 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
>> @@ -1355,6 +1355,29 @@ static struct omap_hwmod dra7xx_mailbox13_hwmod = {
>> },
>> };
>>
>> +/*
>> + * 'mcan' class
>> + *
>> + */
>> +static struct omap_hwmod_class dra76x_mcan_hwmod_class = {
>> + .name = "mcan",
>> +};
>> +
>> +/* mcan */
>> +static struct omap_hwmod dra76x_mcan_hwmod = {
>> + .name = "mcan",
>> + .class = &dra76x_mcan_hwmod_class,
>> + .clkdm_name = "wkupaon_clkdm",
>> + .main_clk = "mcan_clk",
>> + .prcm = {
>> + .omap4 = {
>> + .clkctrl_offs = DRA7XX_CM_WKUPAON_ADC_CLKCTRL_OFFSET,
>> + .context_offs = DRA7XX_RM_WKUPAON_ADC_CONTEXT_OFFSET,
>> + .modulemode = MODULEMODE_SWCTRL,
>> + },
>> + },
>> +};
>
> You should be now able to leave out at least the clkctrl_offs and modulemode
> here. Please also check if leaving out clkdm_name and main_clk now works :)
>
>> @@ -3818,6 +3841,14 @@ static struct omap_hwmod_ocp_if dra7xx_l4_per2__epwmss2 = {
>> .user = OCP_USER_MPU,
>> };
>>
>> +/* l3_main_1 -> mcan */
>> +static struct omap_hwmod_ocp_if dra76x_l3_main_1__mcan = {
>> + .master = &dra7xx_l3_main_1_hwmod,
>> + .slave = &dra76x_mcan_hwmod,
>> + .clk = "l3_iclk_div",
>> + .user = OCP_USER_MPU | OCP_USER_SDMA,
>> +};
>
> I think this we still need though for the clk. Tero, do you have any comments
> on what all clocks can now be left out?
For the OCP if part, I think that is still needed until we switch over
to full sysc driver. clkctrl_offs you probably also need because that is
used for mapping the omap_hwmod against a specific clkctrl clock. Those
can be only removed once we are done with hwmod (or figure out some
other way to assign the clkctrl clock to a hwmod.)
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* Re: [PATCH v2 0/6] mfd/regulator/clk: bd71837: ROHM BD71837 PMIC driver
From: Stephen Boyd @ 2018-05-30 15:23 UTC (permalink / raw)
To: Lee Jones, Matti Vaittinen
Cc: Matti Vaittinen, mturquette, robh+dt, mark.rutland, lgirdwood,
broonie, linux-clk, devicetree, linux-kernel, mikko.mutanen,
heikki.haikola
In-Reply-To: <20180530111649.GB8038@dell>
Quoting Lee Jones (2018-05-30 04:16:49)
> On Tue, 29 May 2018, Matti Vaittinen wrote:
>
> > Hello,
> >
> > On Tue, May 29, 2018 at 08:39:58AM +0100, Lee Jones wrote:
> > > On Mon, 28 May 2018, Matti Vaittinen wrote:
> > >
> > > > Patch series adding support for ROHM BD71837 PMIC.
> > > FYI, this patch-set is going to be difficult to manage since it was
> > > not sent 'threaded'.
> > >
> > > If/when you send a subsequent version, could you please ensure you
> > > send the set threaded so the patches keep in relation to one another
> > > as they are reviewed?
> >
> > Thanks for the guidance. I have not sent so many patches to community so
> > I am grateful also from all the practical tips =) Just one slight problem.
> > I have only seen emails being threaded when one is replying to an email.
> > So how should I send my patches in same thread? Just send first one and
> > then send subsequent patches as replies?
> >
> > I just killed some unused definitions and one unused variable from the
> > code so I am about to send new version. I'll try doing that as a threaded
> > series and resend all the patches as v3.
>
> You don't need to do this manually.
>
> Just use `git send-email` with the correct arguments.
>
I usually send with 'git send-email *.patch' so that git can do the
threading for me. Looks like these patches were sent with Mutt though,
so perhaps 'git format-patch | git imap-send' was used without the
--thread option on format-patch.
^ permalink raw reply
* Re: [PATCH v2 1/6] ARM: dra762: hwmod: Add MCAN support
From: Tony Lindgren @ 2018-05-30 15:28 UTC (permalink / raw)
To: Tero Kristo
Cc: Faiz Abbas, linux-kernel, devicetree, linux-omap,
linux-arm-kernel, linux-clk, robh+dt, bcousson, paul
In-Reply-To: <cfa9c9df-57a2-69f6-9509-440dc18292e7@ti.com>
* Tero Kristo <t-kristo@ti.com> [180530 15:18]:
> For the OCP if part, I think that is still needed until we switch over to
> full sysc driver. clkctrl_offs you probably also need because that is used
> for mapping the omap_hwmod against a specific clkctrl clock. Those can be
> only removed once we are done with hwmod (or figure out some other way to
> assign the clkctrl clock to a hwmod.)
Hmm might be worth testing. I thought your commit 70f05be32133
("ARM: OMAP2+: hwmod: populate clkctrl clocks for hwmods if available")
already parses the clkctrl from dts?
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Doug Anderson @ 2018-05-30 15:34 UTC (permalink / raw)
To: Mark Brown
Cc: David Collins, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <20180530150241.GO6920@sirena.org.uk>
Hi,
On Wed, May 30, 2018 at 8:02 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 30, 2018 at 07:46:50AM -0700, Doug Anderson wrote:
>> On Wed, May 30, 2018 at 2:37 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> >> Linux vote for the lowest voltage it's comfortable with. Linux keeps
>> >> track of the true voltage that the driver wants and will always change
>> >> its vote back to that before enabling. Thus (assuming Linux is OK
>> >> with 1.2 V - 1.4 V for a rail):
>
>> > That's pretty much what it should do anyway with normally designed
>> > hardware.
>
>> I guess the question is: do we insist that the driver include this
>> workaround, or are we OK with letting the hardware behave as the
>> hardware does?
>
> What you're describing sounds like what we should be doing normally, if
> we're not doing that we should probably be fixing the core.
I'm not convinced that this behavior makes sense to move to the core.
On most regulators I'd expect that when the regulator driver says to
turn them off that they will output no voltage. The reason RPMh is
special is that there's an aggregation outside of Linux. Specifically
I like to make it concrete use the example where both Linux and "the
modem" make requests for the same regulator and RPMh aggregates these
requests. This aggregation happens separately for "enabled" and
"voltage".
Thus if you consider:
Modem: vote for 1.3V and enabled=true
Linux: vote for 1.4V and enabled=false
The aggregated voltage is 1.4V and the aggregated enabled is true.
Said another way: Linux's vote for the voltage affected the state of
the rail even though Linux wanted the rail disabled.
In any other system when Linux disabled the regulator it wouldn't
matter that you left it set at 1.4V. Thus RPMh is special and IMO the
workaround belongs there.
-Doug
^ permalink raw reply
* Re: [PATCH v7 2/7] drivers/i2c: Add FSI-attached I2C master algorithm
From: Eddie James @ 2018-05-30 15:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Benjamin Herrenschmidt, Joel Stanley, Mark Rutland,
Greg Kroah-Hartman, Edward A. James
In-Reply-To: <CAHp75VexBiRQ1SPYK6=N-vmCAC9DhtpNKanYXrFshSM9WZb+UA@mail.gmail.com>
On 05/29/2018 06:42 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Add register definitions for FSI-attached I2C master and functions to
>> access those registers over FSI. Add an FSI driver so that our I2C bus
>> is probed up during an FSI scan.
> This looks like reinventing a wheel in some ways.
>
> See my comments below.
>
>> +/*
>> + * Copyright 2017 IBM Corporation
>> + *
>> + * Eddie James <eajames@us.ibm.com>
>> + *
>> + * 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; either version
>> + * 2 of the License, or (at your option) any later version.
>> + */
> We are using SPDX identifiers. Can you?
Sure.
>
>> +/* Find left shift from first set bit in m */
>> +#define MASK_TO_LSH(m) (__builtin_ffsll(m) - 1ULL)
> Oh. What about GENMASK()?
>
>> +/* Extract field m from v */
>> +#define GETFIELD(m, v) (((v) & (m)) >> MASK_TO_LSH(m))
>> +
>> +/* Set field m of v to val */
>> +#define SETFIELD(m, v, val) \
>> + (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
> Oh, what about https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h
> ?
Good idea, thanks.
>
>> +#define I2C_CMD_WITH_START 0x80000000
>> +#define I2C_CMD_WITH_ADDR 0x40000000
>> +#define I2C_CMD_RD_CONT 0x20000000
>> +#define I2C_CMD_WITH_STOP 0x10000000
>> +#define I2C_CMD_FORCELAUNCH 0x08000000
> BIT() ?
>
>> +#define I2C_CMD_ADDR 0x00fe0000
>> +#define I2C_CMD_READ 0x00010000
> GENMASK()? Though precisely here it might be good to leave explicit values.
>
>> +#define I2C_CMD_LEN 0x0000ffff
>> +#define I2C_MODE_CLKDIV 0xffff0000
>> +#define I2C_MODE_PORT 0x0000fc00
>> +#define I2C_MODE_ENHANCED 0x00000008
>> +#define I2C_MODE_DIAG 0x00000004
>> +#define I2C_MODE_PACE_ALLOW 0x00000002
>> +#define I2C_MODE_WRAP 0x00000001
> What are they? Masks? Bit fields? Just plain numbers?
>
>> +#define I2C_WATERMARK_HI 0x0000f000
>> +#define I2C_WATERMARK_LO 0x000000f0
> GENMASK() ?
>
>> +#define I2C_INT_INV_CMD 0x00008000
>> +#define I2C_INT_PARITY 0x00004000
>> +#define I2C_INT_BE_OVERRUN 0x00002000
>> +#define I2C_INT_BE_ACCESS 0x00001000
>> +#define I2C_INT_LOST_ARB 0x00000800
>> +#define I2C_INT_NACK 0x00000400
>> +#define I2C_INT_DAT_REQ 0x00000200
>> +#define I2C_INT_CMD_COMP 0x00000100
>> +#define I2C_INT_STOP_ERR 0x00000080
>> +#define I2C_INT_BUSY 0x00000040
>> +#define I2C_INT_IDLE 0x00000020
> BIT()
>
>> +#define I2C_INT_ENABLE 0x0000ff80
>> +#define I2C_INT_ERR 0x0000fcc0
>> +#define I2C_STAT_INV_CMD 0x80000000
>> +#define I2C_STAT_PARITY 0x40000000
>> +#define I2C_STAT_BE_OVERRUN 0x20000000
>> +#define I2C_STAT_BE_ACCESS 0x10000000
>> +#define I2C_STAT_LOST_ARB 0x08000000
>> +#define I2C_STAT_NACK 0x04000000
>> +#define I2C_STAT_DAT_REQ 0x02000000
>> +#define I2C_STAT_CMD_COMP 0x01000000
>> +#define I2C_STAT_STOP_ERR 0x00800000
>> +#define I2C_STAT_MAX_PORT 0x000f0000
>> +#define I2C_STAT_ANY_INT 0x00008000
>> +#define I2C_STAT_SCL_IN 0x00000800
>> +#define I2C_STAT_SDA_IN 0x00000400
>> +#define I2C_STAT_PORT_BUSY 0x00000200
>> +#define I2C_STAT_SELF_BUSY 0x00000100
> BIT()
>
>> +#define I2C_STAT_FIFO_COUNT 0x000000ff
> GENMASK()
>
>> +
>> +#define I2C_STAT_ERR 0xfc800000
>> +#define I2C_STAT_ANY_RESP 0xff800000
>> +#define I2C_ESTAT_FIFO_SZ 0xff000000
> GENMASK()
>
>> +#define I2C_ESTAT_SCL_IN_SY 0x00008000
>> +#define I2C_ESTAT_SDA_IN_SY 0x00004000
>> +#define I2C_ESTAT_S_SCL 0x00002000
>> +#define I2C_ESTAT_S_SDA 0x00001000
>> +#define I2C_ESTAT_M_SCL 0x00000800
>> +#define I2C_ESTAT_M_SDA 0x00000400
>> +#define I2C_ESTAT_HI_WATER 0x00000200
>> +#define I2C_ESTAT_LO_WATER 0x00000100
>> +#define I2C_ESTAT_PORT_BUSY 0x00000080
>> +#define I2C_ESTAT_SELF_BUSY 0x00000040
> BIT()
>
>> +#define I2C_ESTAT_VERSION 0x0000001f
> GENMASK()
>
>> + __be32 data_be;
> No need to have a suffix. If anything can go wrong we have a tool,
> it's called sparse. It will catch out inappropriate use of __bitwise
> types.
I already have a variable called data...
>
>> + __be32 data_be = cpu_to_be32(*data);
> cpu_to_be32p() IIUC?
Sure.
>
>> +static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
>> +{
>> + int rc;
>> + u32 mode = I2C_MODE_ENHANCED, extended_status, watermark = 0;
>> + u32 interrupt = 0;
> Redundant assignment.
No, I need to set the interrupt register to 0, so I must set this.
>
>> +
>> + /* since we use polling, disable interrupts */
>> + rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_INT_MASK, &interrupt);
>> + if (rc)
>> + return rc;
>> + return rc;
> Would be non-zero?
No, fsi_i2c_write_reg returns non-zero on error, zero on success. That
is what I want.
Thanks,
Eddie
>
>> +}
^ permalink raw reply
* Re: [PATCH v4 0/6] mfd/regulator/clk: bd71837: ROHM BD71837 PMIC driver
From: Mark Brown @ 2018-05-30 15:41 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, mturquette, sboyd, robh+dt, mark.rutland,
lee.jones, lgirdwood, linux-clk, devicetree, linux-kernel,
mikko.mutanen, heikki.haikola
In-Reply-To: <20180530125634.GE13528@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 949 bytes --]
On Wed, May 30, 2018 at 03:56:34PM +0300, Matti Vaittinen wrote:
> On Wed, May 30, 2018 at 12:00:00PM +0100, Mark Brown wrote:
> > The tradeoff with forced PWM mode is that the quality of regulation will
> > be a lot better, especially if the load changes suddenly (as things like
> > CPUs often do). Most hardware that's at all current is able respond to
> > changes in load and switch modes automatically when it's appropriate,
> > except possibly in some very low power modes.
> Yes. The mode switching is automatic. But there is this control bit for
> disabling automatic mode switching and forcing the PWM. Problem with
> these 4 last bucks is just that if regulator is in PFM (and it may be
> if not forced to PWM - due to this automatic switching) then the voltage
> change is not behaving well.
That sounds like the mode switching just isn't very good and needs a bit
of help so forcing the mode is probably going to do the right thing.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/6] ARM: dra762: hwmod: Add MCAN support
From: Tero Kristo @ 2018-05-30 15:41 UTC (permalink / raw)
To: Tony Lindgren
Cc: devicetree, paul, linux-kernel, robh+dt, Faiz Abbas, bcousson,
linux-omap, linux-clk, linux-arm-kernel
In-Reply-To: <20180530152825.GG5705@atomide.com>
On 30/05/18 18:28, Tony Lindgren wrote:
> * Tero Kristo <t-kristo@ti.com> [180530 15:18]:
>> For the OCP if part, I think that is still needed until we switch over to
>> full sysc driver. clkctrl_offs you probably also need because that is used
>> for mapping the omap_hwmod against a specific clkctrl clock. Those can be
>> only removed once we are done with hwmod (or figure out some other way to
>> assign the clkctrl clock to a hwmod.)
>
> Hmm might be worth testing. I thought your commit 70f05be32133
> ("ARM: OMAP2+: hwmod: populate clkctrl clocks for hwmods if available")
> already parses the clkctrl from dts?
It maps the clkctrl clock to be used by hwmod, if those are available.
We didn't add any specific clock entries to DT for mapping the actual
clkctrl clock without the hwmod_data hints yet though, as that was
deemed temporary solution only due to transition to interconnect driver.
I.e., you would need something like this in DT for every device node:
&uart3 {
clocks = <l4per_clkctrl UART3_CLK 0>;
clock-names = "clkctrl";
};
... which is currently not present.
Alternatively you could rename the main_clk in the hwmod_data to point
towards the clkctrl clock, but again, that would be a temporary solution
only.
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* Re: [PATCH v1 2/2] hwmon: npcm-pwm: add NPCM7xx PWM driver
From: Tomer Maimon @ 2018-05-30 15:44 UTC (permalink / raw)
To: Guenter Roeck
Cc: Rob Herring, Mark Rutland, jdelvare, Avi Fishman, Nancy Yuen,
Brendan Higgins, Patrick Venture, Joel Stanley, devicetree,
Linux Kernel Mailing List, linux-hwmon, OpenBMC Maillist,
linux-pwm, Thierry Reding
In-Reply-To: <20180529165617.GD2162@roeck-us.net>
[-- Attachment #1: Type: text/plain, Size: 17462 bytes --]
Hi Guenter,
Thanks for your prompt reply!
On 29 May 2018 at 19:56, Guenter Roeck <linux@roeck-us.net> wrote:
> On Tue, May 29, 2018 at 01:02:21PM +0300, Tomer Maimon wrote:
> > Add Nuvoton BMC NPCM7xx Pulse Width Modulation (PWM) driver.
> >
> > The Nuvoton BMC NPCM7xx has two identical PWM controller modules,
> > each module has four PWM controller outputs.
> >
>
> I don't see it guaranteed that all future NPCM7xx devices will be PWM
> controllers, much less that they will be compatible to the chip really
>
Actually all NPCM7xx family have PWM and FAN modules support,
> supported here. NPCM750, I presume ? I would suggest name the driver
> accordingly.
>
The compatible name can not be a family name like nuvoton,npcm7xx-pwm, only
a specific chip name. (in our case the NPCM750 is the full modules SOC)
still you think i should change the driver name? (note: all of our NPCM7xx
unique modules drivers are named npcm7xx-*.c or *-npcm7xx.c)
> As a generic pwm driver, not specifically tied to a fan controller,
> this driver does not belong into hwmon. It should be added to the pwm
> subsystem. Copying the maintainer and mailing list.
>
> In the NPCM7xx we have PWM and FAN controller modules, usually in the
aspect of our BMC clients the two module
are used together to control the fans.
But because in the NPCM7xx the PWM and the FAN controller are separate
modules we
thought to do two separate drivers in the hwmon
is it possible? or you think it is better to do one hwmon driver for the
PWM and the FAN controller.
note that we were going to submit soon also the FAN controller driver under
hwmon.
> If the pwm chip is used to control fans, the existing pwm-fan driver can
> then be used to create the necessary mapping from pwm controls to fans.
>
> Guenter
>
Thanks,
Tomer
>
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
> > drivers/hwmon/Kconfig | 6 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/npcm7xx-pwm.c | 363 ++++++++++++++++++++++++++++++
> ++++++++++++++
> > 3 files changed, 370 insertions(+)
> > create mode 100644 drivers/hwmon/npcm7xx-pwm.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 6ec307c93ece..693ba09cff8e 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1891,6 +1891,12 @@ config SENSORS_XGENE
> > If you say yes here you get support for the temperature
> > and power sensors for APM X-Gene SoC.
> >
> > +config SENSORS_NPCM7XX
> > + tristate "Nuvoton NPCM7XX PWM driver"
> > + help
> > + This driver provides support for Nuvoton NPCM7XX PWM
> > + controller.
> > +
> > if ACPI
> >
> > comment "ACPI drivers"
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index e7d52a36e6c4..24aad895a3bb 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -174,6 +174,7 @@ obj-$(CONFIG_SENSORS_W83L786NG) += w83l786ng.o
> > obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
> > obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
> > obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
> > +obj-$(CONFIG_SENSORS_NPCM7XX) += npcm7xx-pwm.o
> >
> > obj-$(CONFIG_PMBUS) += pmbus/
> >
> > diff --git a/drivers/hwmon/npcm7xx-pwm.c b/drivers/hwmon/npcm7xx-pwm.c
> > new file mode 100644
> > index 000000000000..6122ca82b94d
> > --- /dev/null
> > +++ b/drivers/hwmon/npcm7xx-pwm.c
> > @@ -0,0 +1,363 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2014-2018 Nuvoton Technology corporation.
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/clk.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_address.h>
> > +
> > +/* NPCM7XX PWM port base address */
> > +#define NPCM7XX_PWM_REG_PR 0x0
> > +#define NPCM7XX_PWM_REG_CSR 0x4
> > +#define NPCM7XX_PWM_REG_CR 0x8
> > +#define NPCM7XX_PWM_REG_CNRx(PORT) (0xC + (12 * PORT))
> > +#define NPCM7XX_PWM_REG_CMRx(PORT) (0x10 + (12 * PORT))
> > +#define NPCM7XX_PWM_REG_PDRx(PORT) (0x14 + (12 * PORT))
> > +#define NPCM7XX_PWM_REG_PIER 0x3C
> > +#define NPCM7XX_PWM_REG_PIIR 0x40
> > +
> > +#define NPCM7XX_PWM_CTRL_CH0_MODE_BIT BIT(3)
> > +#define NPCM7XX_PWM_CTRL_CH1_MODE_BIT BIT(11)
> > +#define NPCM7XX_PWM_CTRL_CH2_MODE_BIT BIT(15)
> > +#define NPCM7XX_PWM_CTRL_CH3_MODE_BIT BIT(19)
> > +
> > +#define NPCM7XX_PWM_CTRL_CH0_INV_BIT BIT(2)
> > +#define NPCM7XX_PWM_CTRL_CH1_INV_BIT BIT(10)
> > +#define NPCM7XX_PWM_CTRL_CH2_INV_BIT BIT(14)
> > +#define NPCM7XX_PWM_CTRL_CH3_INV_BIT BIT(18)
> > +
> > +#define NPCM7XX_PWM_CTRL_CH0_EN_BIT BIT(0)
> > +#define NPCM7XX_PWM_CTRL_CH1_EN_BIT BIT(8)
> > +#define NPCM7XX_PWM_CTRL_CH2_EN_BIT BIT(12)
> > +#define NPCM7XX_PWM_CTRL_CH3_EN_BIT BIT(16)
> > +
> > +/* Define the maximum PWM channel number */
> > +#define NPCM7XX_PWM_MAX_CHN_NUM 8
> > +#define NPCM7XX_PWM_MAX_CHN_NUM_IN_A_MODULE 4
> > +#define NPCM7XX_PWM_MAX_MODULES 2
> > +
> > +/* Define the Counter Register, value = 100 for match 100% */
> > +#define NPCM7XX_PWM_COUNTER_DEFALUT_NUM 255
> > +#define NPCM7XX_PWM_COMPARATOR_DEFALUT_NUM 127
> > +
> > +#define NPCM7XX_PWM_COMPARATOR_MAX 255
> > +
> > +
> > +/* default all PWM channels PRESCALE2 = 1 */
> > +#define NPCM7XX_PWM_PRESCALE2_DEFALUT_CH0 0x4
> > +#define NPCM7XX_PWM_PRESCALE2_DEFALUT_CH1 0x40
> > +#define NPCM7XX_PWM_PRESCALE2_DEFALUT_CH2 0x400
> > +#define NPCM7XX_PWM_PRESCALE2_DEFALUT_CH3 0x4000
> > +
> > +#define PWM_OUTPUT_FREQ_25KHZ 25000
> > +#define PWN_CNT_DEFAULT 256
> > +#define MIN_PRESCALE1 2
> > +#define NPCM7XX_PWM_PRESCALE_SHIFT_CH01 8
> > +
> > +#define NPCM7XX_PWM_PRESCALE2_DEFALUT (NPCM7XX_PWM_PRESCALE2_DEFALUT_CH0
> | \
> > + NPCM7XX_PWM_PRESCALE2_DEFALUT_CH1
> | \
> > + NPCM7XX_PWM_PRESCALE2_DEFALUT_CH2
> | \
> > + NPCM7XX_PWM_PRESCALE2_DEFALUT_CH3)
> > +
> > +#define NPCM7XX_PWM_CTRL_MODE_DEFALUT
> (NPCM7XX_PWM_CTRL_CH0_MODE_BIT | \
> > + NPCM7XX_PWM_CTRL_CH1_MODE_BIT | \
> > + NPCM7XX_PWM_CTRL_CH2_MODE_BIT | \
> > + NPCM7XX_PWM_CTRL_CH3_MODE_BIT)
> > +
> > +#define NPCM7XX_PWM_CTRL_EN_DEFALUT (NPCM7XX_PWM_CTRL_CH0_EN_BIT | \
> > + NPCM7XX_PWM_CTRL_CH1_EN_BIT | \
> > + NPCM7XX_PWM_CTRL_CH2_EN_BIT | \
> > + NPCM7XX_PWM_CTRL_CH3_EN_BIT)
> > +
> > +struct npcm7xx_pwm_data {
> > + unsigned long clk_freq;
> > + void __iomem *pwm_base[NPCM7XX_PWM_MAX_MODULES];
> > + struct mutex npcm7xx_pwm_lock[NPCM7XX_PWM_MAX_CHN_NUM];
> > +};
> > +
> > +static const struct of_device_id pwm_dt_id[];
> > +
> > +static int npcm7xx_pwm_config_set(struct npcm7xx_pwm_data *data, int
> channel,
> > + u16 val)
> > +{
> > + u32 PWMChannel = (channel % NPCM7XX_PWM_MAX_CHN_NUM_IN_A_MODULE);
> > + u32 n_module = (channel / NPCM7XX_PWM_MAX_CHN_NUM_IN_A_MODULE);
> > + u32 u32TmpBuf = 0, ctrl_en_bit;
> > +
> > + /*
> > + * Config PWM Comparator register for setting duty cycle
> > + */
> > + if (val < 0 || val > NPCM7XX_PWM_COMPARATOR_MAX)
> > + return -EINVAL;
> > +
> > + /* write new CMR value */
> > + iowrite32(val, data->pwm_base[n_module] +
> > + NPCM7XX_PWM_REG_CMRx(PWMChannel));
> > +
> > + u32TmpBuf = ioread32(data->pwm_base[n_module] +
> NPCM7XX_PWM_REG_CR);
> > +
> > + switch (PWMChannel) {
> > + case 0:
> > + ctrl_en_bit = NPCM7XX_PWM_CTRL_CH0_EN_BIT;
> > + break;
> > + case 1:
> > + ctrl_en_bit = NPCM7XX_PWM_CTRL_CH1_EN_BIT;
> > + break;
> > + case 2:
> > + ctrl_en_bit = NPCM7XX_PWM_CTRL_CH2_EN_BIT;
> > + break;
> > + case 3:
> > + ctrl_en_bit = NPCM7XX_PWM_CTRL_CH3_EN_BIT;
> > + break;
> > + default:
> > + return -ENODEV;
> > + }
> > +
> > + if (val == 0)
> > + /* Disable PWM */
> > + u32TmpBuf &= ~(ctrl_en_bit);
> > + else
> > + /* Enable PWM */
> > + u32TmpBuf |= ctrl_en_bit;
> > +
> > + mutex_lock(&data->npcm7xx_pwm_lock[n_module]);
> > + iowrite32(u32TmpBuf, data->pwm_base[n_module] +
> NPCM7XX_PWM_REG_CR);
> > + mutex_unlock(&data->npcm7xx_pwm_lock[n_module]);
> > +
> > + return 0;
> > +}
> > +
> > +static int npcm7xx_read_pwm(struct device *dev, u32 attr, int channel,
> > + long *val)
> > +{
> > + struct npcm7xx_pwm_data *data = dev_get_drvdata(dev);
> > + u32 PWMChannel = (channel % NPCM7XX_PWM_MAX_CHN_NUM_IN_A_MODULE);
> > + u32 n_module = (channel / NPCM7XX_PWM_MAX_CHN_NUM_IN_A_MODULE);
> > +
> > + if (IS_ERR(data))
> > + return PTR_ERR(data);
> > +
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + *val = (long)ioread32(data->pwm_base[n_module] +
> > + NPCM7XX_PWM_REG_CMRx(PWMChannel));
> > + return 0;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > +static int npcm7xx_write_pwm(struct device *dev, u32 attr, int channel,
> > + long val)
> > +{
> > + struct npcm7xx_pwm_data *data = dev_get_drvdata(dev);
> > + int err = 0;
> > +
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + err = npcm7xx_pwm_config_set(data, channel, (u16)val);
> > + break;
> > + default:
> > + err = -EOPNOTSUPP;
> > + break;
> > + }
> > +
> > + return err;
> > +}
> > +
> > +static umode_t npcm7xx_pwm_is_visible(const void *_data, u32 attr, int
> channel)
> > +{
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + return 0644;
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static int npcm7xx_read(struct device *dev, enum hwmon_sensor_types
> type,
> > + u32 attr, int channel, long *val)
> > +{
> > + switch (type) {
> > + case hwmon_pwm:
> > + return npcm7xx_read_pwm(dev, attr, channel, val);
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > +static int npcm7xx_write(struct device *dev, enum hwmon_sensor_types
> type,
> > + u32 attr, int channel, long val)
> > +{
> > + switch (type) {
> > + case hwmon_pwm:
> > + return npcm7xx_write_pwm(dev, attr, channel, val);
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > +static umode_t npcm7xx_is_visible(const void *data,
> > + enum hwmon_sensor_types type,
> > + u32 attr, int channel)
> > +{
> > + switch (type) {
> > + case hwmon_pwm:
> > + return npcm7xx_pwm_is_visible(data, attr, channel);
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static const u32 npcm7xx_pwm_config[] = {
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + 0
> > +};
> > +
> > +static const struct hwmon_channel_info npcm7xx_pwm = {
> > + .type = hwmon_pwm,
> > + .config = npcm7xx_pwm_config,
> > +};
> > +
> > +static const struct hwmon_channel_info *npcm7xx_info[] = {
> > + &npcm7xx_pwm,
> > + NULL
> > +};
> > +
> > +static const struct hwmon_ops npcm7xx_hwmon_ops = {
> > + .is_visible = npcm7xx_is_visible,
> > + .read = npcm7xx_read,
> > + .write = npcm7xx_write,
> > +};
> > +
> > +static const struct hwmon_chip_info npcm7xx_chip_info = {
> > + .ops = &npcm7xx_hwmon_ops,
> > + .info = npcm7xx_info,
> > +};
> > +
> > +static int npcm7xx_pwm_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct npcm7xx_pwm_data *data;
> > + struct resource res[NPCM7XX_PWM_MAX_MODULES];
> > + struct device *hwmon;
> > + struct clk *clk;
> > + int m, ch, res_cnt, ret;
> > + u32 Prescale_val, output_freq;
> > +
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + for (res_cnt = 0; res_cnt < NPCM7XX_PWM_MAX_MODULES ; res_cnt++) {
> > + ret = of_address_to_resource(dev->of_node, res_cnt,
> > + &res[res_cnt]);
> > + if (ret) {
> > + pr_err("PWM of_address_to_resource fail ret %d\n",
> > + ret);
> > + return -EINVAL;
> > + }
> > +
> > + data->pwm_base[res_cnt] =
> > + devm_ioremap_resource(dev, &(res[res_cnt]));
> > + pr_debug("pwm%d base is 0x%08X, res.start 0x%08X , size
> 0x%08X\n",
> > + res_cnt, (u32)data->pwm_base[res_cnt],
> > + res[res_cnt].start, resource_size(&(res[res_cnt]))
> );
> > +
> > + if (!data->pwm_base[res_cnt]) {
> > + pr_err("pwm probe failed: can't read pwm base
> address for resource %d.\n",
> > + res_cnt);
> > + return -ENOMEM;
> > + }
> > +
> > + mutex_init(&data->npcm7xx_pwm_lock[res_cnt]);
> > + }
> > +
> > + clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(clk))
> > + return -ENODEV;
> > +
> > + data->clk_freq = clk_get_rate(clk);
> > +
> > + /* Adjust NPCM7xx PWMs output frequency to ~25Khz */
> > + output_freq = data->clk_freq / PWN_CNT_DEFAULT;
> > + Prescale_val = DIV_ROUND_CLOSEST(output_freq,
> PWM_OUTPUT_FREQ_25KHZ);
> > +
> > + /* If Prescale_val = 0, then the prescale output clock is stopped
> */
> > + if (Prescale_val < MIN_PRESCALE1)
> > + Prescale_val = MIN_PRESCALE1;
> > + /*
> > + * Prescale_val need to decrement in one because in the PWM
> Prescale
> > + * register the Prescale value increment by one
> > + */
> > + Prescale_val--;
> > +
> > + /* Setting PWM Prescale Register value register to both modules */
> > + Prescale_val |= (Prescale_val << NPCM7XX_PWM_PRESCALE_SHIFT_CH01);
> > +
> > + for (m = 0; m < NPCM7XX_PWM_MAX_MODULES ; m++) {
> > + iowrite32(Prescale_val,
> > + data->pwm_base[m] + NPCM7XX_PWM_REG_PR);
> > + iowrite32(NPCM7XX_PWM_PRESCALE2_DEFALUT,
> > + data->pwm_base[m] + NPCM7XX_PWM_REG_CSR);
> > + iowrite32(NPCM7XX_PWM_CTRL_MODE_DEFALUT,
> > + data->pwm_base[m] + NPCM7XX_PWM_REG_CR);
> > +
> > + for (ch = 0; ch < NPCM7XX_PWM_MAX_CHN_NUM; ch++) {
> > + iowrite32(NPCM7XX_PWM_COUNTER_DEFALUT_NUM,
> > + data->pwm_base[m] +
> NPCM7XX_PWM_REG_CNRx(ch));
> > + iowrite32(NPCM7XX_PWM_COMPARATOR_DEFALUT_NUM,
> > + data->pwm_base[m] +
> NPCM7XX_PWM_REG_CMRx(ch));
> > + }
> > +
> > + iowrite32(NPCM7XX_PWM_CTRL_MODE_DEFALUT |
> > + NPCM7XX_PWM_CTRL_EN_DEFALUT,
> > + data->pwm_base[m] + NPCM7XX_PWM_REG_CR);
> > + }
> > +
> > + hwmon = devm_hwmon_device_register_with_info(dev, "npcm7xx_pwm",
> data,
> > + &npcm7xx_chip_info,
> NULL);
> > +
> > + if (IS_ERR(hwmon)) {
> > + pr_err("PWM Driver failed - devm_hwmon_device_register_with_groups
> failed\n");
> > + return PTR_ERR(hwmon);
> > + }
> > +
> > + pr_info("NPCM7XX PWM Driver probed, PWM output Freq %dHz\n",
> > + output_freq / ((Prescale_val & 0xf) + 1));
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id of_pwm_match_table[] = {
> > + { .compatible = "nuvoton,npcm750-pwm", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, of_pwm_match_table);
> > +
> > +static struct platform_driver npcm7xx_pwm_driver = {
> > + .probe = npcm7xx_pwm_probe,
> > + .driver = {
> > + .name = "npcm7xx_pwm",
> > + .of_match_table = of_pwm_match_table,
> > + },
> > +};
> > +
> > +module_platform_driver(npcm7xx_pwm_driver);
> > +
> > +MODULE_DESCRIPTION("Nuvoton NPCM7XX PWM Driver");
> > +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.14.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
[-- Attachment #2: Type: text/html, Size: 25883 bytes --]
^ permalink raw reply
* Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm
From: Eddie James @ 2018-05-30 15:47 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Benjamin Herrenschmidt, Joel Stanley, Mark Rutland,
Greg Kroah-Hartman, Edward A. James
In-Reply-To: <CAHp75Vd38BrJcULxoqj1mTKCEvMh9ZVYEvzCMAg2t9onZrSR6w@mail.gmail.com>
On 05/29/2018 06:19 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Add and initialize I2C adapters for each port on the FSI-attached I2C
>> master. Ports for each master are defined in the devicetree.
>> +#include <linux/of.h>
>
>> +static int fsi_i2c_set_port(struct fsi_i2c_port *port)
>> +{
>> + int rc;
>> + struct fsi_device *fsi = port->master->fsi;
>> + u32 mode, dummy = 0;
>> + u16 old_port;
>> +
>> + rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
>> + if (rc)
>> + return rc;
>> +
>> + old_port = GETFIELD(I2C_MODE_PORT, mode);
>> +
>> + if (old_port != port->port) {
> Why not simple
>
> if (port->port == GETFIELD())
> return 0;
>
> ?
Sure.
>
>> + /* reset engine when port is changed */
>> + rc = fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
>> + if (rc)
>> + return rc;
>> + }
>> + return rc;
> It's hardly would be non-zero, right?
No, fsi_i2c_read_reg and fsi_i2c_write_reg both return 0 on success and
non-zero on error. That is the desired behavior of this function also.
>
>> +}
>> static int fsi_i2c_probe(struct device *dev)
>> {
> Isn't below somehow repeats of_i2c_register_devices() ?
> Why not to use it?
Because I need to assign all these port structure fields. Also looks
like of_i2c_register_devices creates new devices; I just want an adapter
for each port.
>
>> + /* Add adapter for each i2c port of the master. */
>> + for_each_available_child_of_node(dev->of_node, np) {
>> + rc = of_property_read_u32(np, "reg", &port_no);
>> + if (rc || port_no > USHRT_MAX)
>> + continue;
>> +
>> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> + if (!port)
>> + break;
>> +
>> + port->master = i2c;
>> + port->port = port_no;
>> +
>> + port->adapter.owner = THIS_MODULE;
>> + port->adapter.dev.of_node = np;
>> + port->adapter.dev.parent = dev;
>> + port->adapter.algo = &fsi_i2c_algorithm;
>> + port->adapter.algo_data = port;
>> +
>> + snprintf(port->adapter.name, sizeof(port->adapter.name),
>> + "i2c_bus-%u", port_no);
>> +
>> + rc = i2c_add_adapter(&port->adapter);
>> + if (rc < 0) {
>> + dev_err(dev, "Failed to register adapter: %d\n", rc);
>> + devm_kfree(dev, port);
> This hurts my eyes. Why?!
What would you suggest instead?
>
>> + continue;
>> + }
>> +
>> + list_add(&port->list, &i2c->ports);
>> + }
>> +
>> dev_set_drvdata(dev, i2c);
>>
>> return 0;
>> }
>> + if (!list_empty(&i2c->ports)) {
> My gosh, this is done already in list_for_each*()
No, list_for_each_entry does NOT check if the list is empty or if the
first entry is NULL.
Thanks,
Eddie
>
>> + list_for_each_entry(port, &i2c->ports, list) {
>> + i2c_del_adapter(&port->adapter);
>> + }
>> + }
>
>
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Mark Brown @ 2018-05-30 15:48 UTC (permalink / raw)
To: Doug Anderson
Cc: David Collins, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <CAD=FV=XJd-udEE6az0xP7i6x0oGsnd=BSCyZ0Og_+RjNCMXnCQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]
On Wed, May 30, 2018 at 08:34:50AM -0700, Doug Anderson wrote:
> On Wed, May 30, 2018 at 8:02 AM, Mark Brown <broonie@kernel.org> wrote:
> > What you're describing sounds like what we should be doing normally, if
> > we're not doing that we should probably be fixing the core.
> I'm not convinced that this behavior makes sense to move to the core.
> On most regulators I'd expect that when the regulator driver says to
> turn them off that they will output no voltage. The reason RPMh is
Oh, you mean while the regulator is off... TBH I don't see a huge
problem doing that anyway and just reverting to the bottom of the
constraints when everything gets turned off since we have to see if we
need to adjust the voltage every time we enabled anyway.
> In any other system when Linux disabled the regulator it wouldn't
> matter that you left it set at 1.4V. Thus RPMh is special and IMO the
> workaround belongs there.
Without the core doing something the regulator isn't going to get told
that anything updated voltages anyway...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm
From: Eddie James @ 2018-05-30 15:49 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-i2c, Linux Kernel Mailing List, devicetree, Wolfram Sang,
Rob Herring, Benjamin Herrenschmidt, Joel Stanley, Mark Rutland,
Greg Kroah-Hartman, Edward A. James
In-Reply-To: <CAHp75Vd38BrJcULxoqj1mTKCEvMh9ZVYEvzCMAg2t9onZrSR6w@mail.gmail.com>
On 05/29/2018 06:19 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Add and initialize I2C adapters for each port on the FSI-attached I2C
>> master. Ports for each master are defined in the devicetree.
>> +#include <linux/of.h>
>
>> +static int fsi_i2c_set_port(struct fsi_i2c_port *port)
>> +{
>> + int rc;
>> + struct fsi_device *fsi = port->master->fsi;
>> + u32 mode, dummy = 0;
>> + u16 old_port;
>> +
>> + rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
>> + if (rc)
>> + return rc;
>> +
>> + old_port = GETFIELD(I2C_MODE_PORT, mode);
>> +
>> + if (old_port != port->port) {
> Why not simple
>
> if (port->port == GETFIELD())
> return 0;
>
> ?
>
>> + /* reset engine when port is changed */
>> + rc = fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
>> + if (rc)
>> + return rc;
>> + }
>> + return rc;
> It's hardly would be non-zero, right?
Sorry, misunderstood your comment here. You are correct, it can only be
zero.
Thanks,
Eddie
>
>> +}
>> static int fsi_i2c_probe(struct device *dev)
>> {
> Isn't below somehow repeats of_i2c_register_devices() ?
> Why not to use it?
>
>> + /* Add adapter for each i2c port of the master. */
>> + for_each_available_child_of_node(dev->of_node, np) {
>> + rc = of_property_read_u32(np, "reg", &port_no);
>> + if (rc || port_no > USHRT_MAX)
>> + continue;
>> +
>> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> + if (!port)
>> + break;
>> +
>> + port->master = i2c;
>> + port->port = port_no;
>> +
>> + port->adapter.owner = THIS_MODULE;
>> + port->adapter.dev.of_node = np;
>> + port->adapter.dev.parent = dev;
>> + port->adapter.algo = &fsi_i2c_algorithm;
>> + port->adapter.algo_data = port;
>> +
>> + snprintf(port->adapter.name, sizeof(port->adapter.name),
>> + "i2c_bus-%u", port_no);
>> +
>> + rc = i2c_add_adapter(&port->adapter);
>> + if (rc < 0) {
>> + dev_err(dev, "Failed to register adapter: %d\n", rc);
>> + devm_kfree(dev, port);
> This hurts my eyes. Why?!
>
>> + continue;
>> + }
>> +
>> + list_add(&port->list, &i2c->ports);
>> + }
>> +
>> dev_set_drvdata(dev, i2c);
>>
>> return 0;
>> }
>> + if (!list_empty(&i2c->ports)) {
> My gosh, this is done already in list_for_each*()
>
>> + list_for_each_entry(port, &i2c->ports, list) {
>> + i2c_del_adapter(&port->adapter);
>> + }
>> + }
>
>
^ permalink raw reply
* Re: [PATCH v4 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Mark Brown @ 2018-05-30 15:50 UTC (permalink / raw)
To: Doug Anderson
Cc: David Collins, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <CAD=FV=WTTKXwPchuWRh-RNTc8=X8Xo28oTqk=3wW4+SiTHhxuA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 697 bytes --]
On Wed, May 30, 2018 at 07:54:47AM -0700, Doug Anderson wrote:
> On Wed, May 30, 2018 at 3:37 AM, Mark Brown <broonie@kernel.org> wrote:
> > I'm confused as to why we are specifying the maximum current the device
> > can deliver in a given mode in the DT - surely that's a fixed property
> > of the hardware?
> Said another way: you're saying that you'd expect the "max-microamps"
> table to have one fewer element than the listed modes? So in the
> above example you'd have:
No, I'm saying that I don't know why that property exists at all. This
sounds like it's intended to be the amount of current the regulator can
deliver in each mode which is normally a design property of the silicon.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] arm64: dts: msm8996: Use dwc3-qcom glue driver for USB
From: Doug Anderson @ 2018-05-30 15:54 UTC (permalink / raw)
To: Manu Gautam
Cc: Andy Gross, Vivek Gautam, open list:ARM/QUALCOMM SUPPORT,
devicetree, linux-arm-msm, linux-usb, felipe.balbi,
Greg Kroah-Hartman, Bjorn Andersson
In-Reply-To: <20180530110400.30710-1-mgautam@codeaurora.org>
Hi,
On Wed, May 30, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
> Move from dwc3-of-simple to dwc3-qcom glue driver to
> support peripheral mode which requires qscratch wrapper
> programming on VBUS event.
Fixes: a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
> arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 2 ++
> arch/arm64/boot/dts/qcom/msm8996.dtsi | 6 ++++--
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> index f45a0ab30d30..83bc1b9ff6ef 100644
> --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
> @@ -108,6 +108,7 @@
>
> usb@6a00000 {
> status = "okay";
> + extcon = <&usb3_id>;
>
> dwc3@6a00000 {
> extcon = <&usb3_id>;
> @@ -124,6 +125,7 @@
>
> usb@7600000 {
> status = "okay";
> + extcon = <&usb2_id>;
>
> dwc3@7600000 {
> extcon = <&usb2_id>;
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> index 26292027ba9b..d30516c0db87 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -777,7 +777,8 @@
> };
>
> usb2: usb@7600000 {
Please update the unit address, which should match the new value for the "reg".
> - compatible = "qcom,dwc3";
> + compatible = "qcom,msm8996-dwc3", "qcom,dwc3";
> + reg = <0x76f8800 0x400>;
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> @@ -805,7 +806,8 @@
> };
>
> usb3: usb@6a00000 {
> - compatible = "qcom,dwc3";
> + compatible = "qcom,msm8996-dwc3", "qcom,dwc3";
> + reg = <0x6af8800 0x400>;
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
This patch is a little bit unfortunate since it demonstrates that a
patch that already landed (specifically commit a4333c3a6ba9 ("usb:
dwc3: Add Qualcomm DWC3 glue driver")) broke device tree backward
compatibility. Specifically if you get a kernel with that commit in
it but not this device tree change then you'll end up with broken USB.
Also if anyone happened to have a device tree stored somewhere out of
tree based on the old binding then they will of course be broken.
I will leave it to people more familiar with msm8996 to decide if this
issue is important to them and if we need to change the bindings to
maintain backward compatibility or if we should instead just land this
patch ASAP.
-Doug
^ permalink raw reply
* Re: [PATCH v2 1/6] ARM: dra762: hwmod: Add MCAN support
From: Tony Lindgren @ 2018-05-30 15:54 UTC (permalink / raw)
To: Tero Kristo
Cc: Faiz Abbas, linux-kernel, devicetree, linux-omap,
linux-arm-kernel, linux-clk, robh+dt, bcousson, paul
In-Reply-To: <b0476932-a6e9-6ef1-69ad-c9a8c55d9739@ti.com>
* Tero Kristo <t-kristo@ti.com> [180530 15:44]:
> On 30/05/18 18:28, Tony Lindgren wrote:
> > * Tero Kristo <t-kristo@ti.com> [180530 15:18]:
> > > For the OCP if part, I think that is still needed until we switch over to
> > > full sysc driver. clkctrl_offs you probably also need because that is used
> > > for mapping the omap_hwmod against a specific clkctrl clock. Those can be
> > > only removed once we are done with hwmod (or figure out some other way to
> > > assign the clkctrl clock to a hwmod.)
> >
> > Hmm might be worth testing. I thought your commit 70f05be32133
> > ("ARM: OMAP2+: hwmod: populate clkctrl clocks for hwmods if available")
> > already parses the clkctrl from dts?
>
> It maps the clkctrl clock to be used by hwmod, if those are available. We
> didn't add any specific clock entries to DT for mapping the actual clkctrl
> clock without the hwmod_data hints yet though, as that was deemed temporary
> solution only due to transition to interconnect driver. I.e., you would need
> something like this in DT for every device node:
>
> &uart3 {
> clocks = <l4per_clkctrl UART3_CLK 0>;
> clock-names = "clkctrl";
> };
>
> ... which is currently not present.
Hmm is that not the "fck" clkctrl clock we have already in
the dts files for the interconnect target modules?
We can also use pdata callbacks to pass the clock node if
needed. But I guess I don't quite still understand what we
are missing :)
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v9 01/15] ARM: Add Krait L2 register accessor functions
From: Stephen Boyd @ 2018-05-30 15:55 UTC (permalink / raw)
To: Bjorn Andersson, Sricharan R
Cc: robh, viresh.kumar, mark.rutland, mturquette, sboyd, linux,
andy.gross, david.brown, rjw, linux-arm-kernel, devicetree,
linux-kernel, linux-clk, linux-arm-msm, linux-soc, linux-pm,
linux
In-Reply-To: <664089d4-b30d-99bb-2021-12128b2895ba@codeaurora.org>
Quoting Sricharan R (2018-05-24 22:40:11)
> Hi Bjorn,
>
> On 5/24/2018 11:09 PM, Bjorn Andersson wrote:
> > On Tue 06 Mar 06:38 PST 2018, Sricharan R wrote:
> >
> >> From: Stephen Boyd <sboyd@codeaurora.org>
> >>
> >> Krait CPUs have a handful of L2 cache controller registers that
> >> live behind a cp15 based indirection register. First you program
> >> the indirection register (l2cpselr) to point the L2 'window'
> >> register (l2cpdr) at what you want to read/write. Then you
> >> read/write the 'window' register to do what you want. The
> >> l2cpselr register is not banked per-cpu so we must lock around
> >> accesses to it to prevent other CPUs from re-pointing l2cpdr
> >> underneath us.
> >>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Russell King <linux@arm.linux.org.uk>
> >> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> >
> > This should have your signed-off-by here as well.
> >
>
> ok.
>
> > Apart from that:
> >
> > Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >
>
Will these patches come around again? I'll do a quick sweep on them
today but I expect them to be resent.
^ permalink raw reply
* Re: [PATCH v1 0/3] USB host: Add USB ehci support for nuvoton npcm7xx
From: Avi Fishman @ 2018-05-30 16:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, stern
Cc: Avi Fishman, Tomer Maimon, Patrick Venture, Nancy Yuen,
Brendan Higgins, mathias.nyman, bjorn.andersson, jhogan, albeu,
chunfeng.yun, tony, baolu.lu, elder, digetx, kstewart, hdegoede,
Linux Kernel Mailing List, linux-usb, OpenBMC Maillist,
Rob Herring, Mark Rutland, devicetree, davem, mchehab+samsung,
akpm, rdu
In-Reply-To: <1527582935-31175-1-git-send-email-avifishman70@gmail.com>
From: Avi Fishman <AviFishman70@gmail.com>
This patch adds support for ehci controller for the Nuvoton npcm7xx platform.
Most of the code was taken from ehci-spear.c + specific initialization code
Avi Fishman (3):
USB host: Add USB ehci support for nuvoton npcm7xx platform
USB: DT probing support to ehci-npcm7xx
MAINTAINERS: add subfolders for nuvoton *npcm*
.../devicetree/bindings/usb/npcm7xx-usb.txt | 18 ++
MAINTAINERS | 3 +
drivers/usb/host/Kconfig | 8 +
drivers/usb/host/Makefile | 1 +
drivers/usb/host/ehci-npcm7xx.c | 212 +++++++++++++++++++++
5 files changed, 242 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/npcm7xx-usb.txt
create mode 100644 drivers/usb/host/ehci-npcm7xx.c
--
2.14.1
^ permalink raw reply
* Re: [PATCH v2 6/9] PM / Domains: Don't attach devices in genpd with multi PM domains
From: Jon Hunter @ 2018-05-30 16:06 UTC (permalink / raw)
To: Ulf Hansson, Rafael J . Wysocki, linux-pm
Cc: Greg Kroah-Hartman, Geert Uytterhoeven, Todor Tomov,
Rajendra Nayak, Viresh Kumar, Vincent Guittot, Kevin Hilman,
linux-kernel, linux-arm-kernel, linux-tegra, Rob Herring,
devicetree
In-Reply-To: <20180529100421.31022-7-ulf.hansson@linaro.org>
On 29/05/18 11:04, Ulf Hansson wrote:
> The power-domain DT property may now contain a list of PM domain
> specifiers, which represents that a device are partitioned across multiple
> PM domains. This leads to a new situation in genpd_dev_pm_attach(), as only
> one PM domain can be attached per device.
>
> To remain things simple for the most common configuration, when a single PM
> domain is used, let's treat the multiple PM domain case as being specific.
>
> In other words, let's change genpd_dev_pm_attach() to check for multiple PM
> domains and prevent it from attach any PM domain for this case. Instead,
> leave this to be managed separately, from following changes to genpd.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Suggested-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v2:
> - Minor update to changelog to mention "PM domain specifiers" rather
> than a "list of phandles".
>
> ---
> drivers/base/power/domain.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 7ebf7993273a..12a20f21974d 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2229,10 +2229,10 @@ static void genpd_dev_pm_sync(struct device *dev)
> * attaches the device to retrieved pm_domain ops.
> *
> * Returns 1 on successfully attached PM domain, 0 when the device don't need a
> - * PM domain or a negative error code in case of failures. Note that if a
> - * power-domain exists for the device, but it cannot be found or turned on,
> - * then return -EPROBE_DEFER to ensure that the device is not probed and to
> - * re-try again later.
> + * PM domain or when multiple power-domains exists for it, else a negative error
> + * code. Note that if a power-domain exists for the device, but it cannot be
> + * found or turned on, then return -EPROBE_DEFER to ensure that the device is
> + * not probed and to re-try again later.
> */
> int genpd_dev_pm_attach(struct device *dev)
> {
> @@ -2243,10 +2243,18 @@ int genpd_dev_pm_attach(struct device *dev)
> if (!dev->of_node)
> return 0;
>
> + /*
> + * Devices with multiple PM domains must be attached separately, as we
> + * can only attach one PM domain per device.
> + */
> + if (of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells") != 1)
> + return 0;
> +
> ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> "#power-domain-cells", 0, &pd_args);
> if (ret < 0)
> - return 0;
> + return ret;
>
> mutex_lock(&gpd_list_lock);
> pd = genpd_get_from_provider(&pd_args);
>
Acked-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Doug Anderson @ 2018-05-30 16:06 UTC (permalink / raw)
To: Mark Brown
Cc: David Collins, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <20180530154849.GQ6920@sirena.org.uk>
Hi,
On Wed, May 30, 2018 at 8:48 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 30, 2018 at 08:34:50AM -0700, Doug Anderson wrote:
>> On Wed, May 30, 2018 at 8:02 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > What you're describing sounds like what we should be doing normally, if
>> > we're not doing that we should probably be fixing the core.
>
>> I'm not convinced that this behavior makes sense to move to the core.
>> On most regulators I'd expect that when the regulator driver says to
>> turn them off that they will output no voltage. The reason RPMh is
>
> Oh, you mean while the regulator is off... TBH I don't see a huge
> problem doing that anyway and just reverting to the bottom of the
> constraints when everything gets turned off since we have to see if we
> need to adjust the voltage every time we enabled anyway.
>
>> In any other system when Linux disabled the regulator it wouldn't
>> matter that you left it set at 1.4V. Thus RPMh is special and IMO the
>> workaround belongs there.
>
> Without the core doing something the regulator isn't going to get told
> that anything updated voltages anyway...
I was just suggesting that when the core tells the regulator driver to
disable itself that the regulator driver tell RPMh to not only disable
itself but also (temporarily) vote for a lower voltage. When the core
tells the regulator to re-enable itself then the regulator driver
restores the original voltage vote (or applies any vote that might
have been attempted while the regulator was disabled). That wouldn't
require any regulator core changes.
-Doug
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Mark Brown @ 2018-05-30 16:07 UTC (permalink / raw)
To: Doug Anderson
Cc: David Collins, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <CAD=FV=X2u=hMyWinSDtim-PmFwAy5mXcwg3HeYojAHcsUFhV3g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 788 bytes --]
On Wed, May 30, 2018 at 09:06:16AM -0700, Doug Anderson wrote:
> On Wed, May 30, 2018 at 8:48 AM, Mark Brown <broonie@kernel.org> wrote:
> > Without the core doing something the regulator isn't going to get told
> > that anything updated voltages anyway...
> I was just suggesting that when the core tells the regulator driver to
> disable itself that the regulator driver tell RPMh to not only disable
> itself but also (temporarily) vote for a lower voltage. When the core
> tells the regulator to re-enable itself then the regulator driver
> restores the original voltage vote (or applies any vote that might
> have been attempted while the regulator was disabled). That wouldn't
> require any regulator core changes.
It needs something to tell it what the new voltage to set is.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Doug Anderson @ 2018-05-30 16:09 UTC (permalink / raw)
To: Mark Brown
Cc: David Collins, Liam Girdwood, Rob Herring, Mark Rutland,
linux-arm-msm, Linux ARM, devicetree, LKML, Rajendra Nayak,
Stephen Boyd
In-Reply-To: <20180530160744.GS6920@sirena.org.uk>
Hi,
On Wed, May 30, 2018 at 9:07 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, May 30, 2018 at 09:06:16AM -0700, Doug Anderson wrote:
>> On Wed, May 30, 2018 at 8:48 AM, Mark Brown <broonie@kernel.org> wrote:
>
>> > Without the core doing something the regulator isn't going to get told
>> > that anything updated voltages anyway...
>
>> I was just suggesting that when the core tells the regulator driver to
>> disable itself that the regulator driver tell RPMh to not only disable
>> itself but also (temporarily) vote for a lower voltage. When the core
>> tells the regulator to re-enable itself then the regulator driver
>> restores the original voltage vote (or applies any vote that might
>> have been attempted while the regulator was disabled). That wouldn't
>> require any regulator core changes.
>
> It needs something to tell it what the new voltage to set is.
The regulator driver has its own cache of what voltage was most
recently requested by Linux. It can use that, can't it?
-Doug
^ 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