* Re: [PATCH 0/8] i.MX7 PCIe related device tree changes
From: Tyler Baker @ 2017-04-19 18:28 UTC (permalink / raw)
To: Andrey Smirnov
Cc: Shawn Guo, yurovsky-Re5JQEeQqe8AvxtiuMwx3w, Sascha Hauer,
Fabio Estevam, Rob Herring, Mark Rutland, Russell King,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel
In-Reply-To: <20170413133242.5068-1-andrew.smirnov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 13 April 2017 at 06:32, Andrey Smirnov <andrew.smirnov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Shawn, everyone:
>
> This series includes changes made to device-tree in order to support
> PCIe on i.MX7 platform. They include:
>
> - Bringing 'anatop-enable-bit' property of ANATOP regulators back
> and extending it to all of the HW it is applicable to
>
> - Adding GPCv2 node for i.MX7 (which was missing, despite the
> irqchip driver for it being in the tree for quite some time)
>
> - Adding a PCIe node for i.MX7
>
> - Adding GPIO expander used by PCIe and enabling PCIe node from
> above on i.MX7 based Sabre board
>
> As usual, feedback is welcome.
>
> Thanks,
> Andrey Smrinov
>
> Andrey Smirnov (8):
> Revert "ARM: dts: imx: Remove unexistant property"
> ARM: dts: imx6: Specify 'anatop-enable-bit' where appropriate
> ARM: dts: imx7s: Adjust anatop-enable-bit for 'reg_1p0d'
> ARM: dts: imx7s: Add node for GPC
> ARM: dts: imx7s: Mark 'gpr' compatible with i.MX6 variant
> ARM: dts: imx7d-sdb: Add GPIO expander node
> ARM: dts: imx7d: Add node for PCIe controller
> ARM: dts: imx7d-sdb: Enable PCIe peripheral
FWIW:
Tested-by: Tyler Baker <tyler.baker-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
This whole series on top of v4.11-rc7, with the addition of the iMX7
GPCv2 and reset driver. Tested on a imx7d-cl-som with a CUK Killer
Doubleshot Wireless-AC 1535 wlan/bt radio using a mini pcie to m2
adapter. Confirmed that the radio was able to associate with an AP
using WPA2, and connected to multiple devices using 6lowpan over BLE.
Tyler
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT
From: Florian Fainelli @ 2017-04-19 18:13 UTC (permalink / raw)
To: Rafał Miłecki, Florian Fainelli,
Rafał Miłecki
Cc: Hauke Mehrtens, Rob Herring, Mark Rutland, Russell King,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <ab696d24-3544-a554-82d1-18839b29caa0-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
On 04/19/2017 11:10 AM, Rafał Miłecki wrote:
> On 04/19/2017 07:52 PM, Florian Fainelli wrote:
>> On 04/19/2017 10:35 AM, Rafał Miłecki wrote:
>>> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>>>> On 04/02/2017 02:25 PM, Rafał Miłecki wrote:
>>>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>>>> Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
>>>>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>>>
>>>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>>>> attached.
>>>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>>>> yet).
>>>>>>>
>>>>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>>> ---
>>>>>>> arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>>>> 1 file changed, 7 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> @@ -320,6 +320,13 @@
>>>>>>> };
>>>>>>> };
>>>>>>>
>>>>>>> + mdio@18003000 {
>>>>>>> + compatible = "brcm,iproc-mdio";
>>>>>>> + reg = <0x18003000 0x8>;
>>>>>>> + #size-cells = <1>;
>>>>>>> + #address-cells = <0>;
>>>>>>> + };
>>>>>>
>>>>>> This looks fine, but usually the block should be enabled on a
>>>>>> per-board
>>>>>> basis, such that there should be a status = "disabled" property
>>>>>> here by
>>>>>> default.
>>>>>
>>>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I
>>>>> guess
>>>>> it's
>>>>> for stuff that is always present on every SoC family board: rng, nand,
>>>>> spi to
>>>>> name few.
>>>>>
>>>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>>>> controller so it's enabled by default. Not every board has SPI
>>>>> flash, so
>>>>> it's
>>>>> disabled by default.
>>>>>
>>>>> It's there and it make sense to me. Is that OK or not?
>>>>
>>>> Even though there are devices that are always enabled on a given SoC,
>>>> because the board designs are always consistent does not necessarily
>>>> make them good candidates to be enabled at the .dtsi level. This is
>>>> particularly true when there are external connections to blocks (SPI,
>>>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>>>> default is safer as a starting point to begin with.
>>>
>>> In case of Northstar there is USB 3.0 PHY connected *internally* to this
>>> MDIO.
>>> I don't think any board manufacturer is able to rip SoC out of the MDIO
>>> or the
>>> USB 3.0 PHY.
>>
>> OK, then can you still resubmit a proper patch that a) puts that
>> information in the commit message, and b) also adds a proper label to
>> the mdio node such that it can later on be referenced by label in
>> board-level DTS files? By that I mean:
>>
>> mdio: mdio@18003000 {
>>
>> Thank you
>>
>>>
>>>
>>>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>>>> MDIO bus
>>>>> just devices may differ and should not be enabled by default.
>>>>
>>>> In which case, the only difference, for you would be to do to, at the
>>>> board-level DTS:
>>>>
>>>> &mdio {
>>>> status = "okay";
>>>>
>>>> phy@0 {
>>>> reg = <0>;
>>>> ...
>>>> };
>>>> };
>>>>
>>>> versus:
>>>>
>>>> &mdio {
>>>> phy@0 {
>>>> reg = <0>;
>>>> ...
>>>> };
>>>> };
>>>>
>>>> I think we can afford putting the mdio node's status property in each
>>>> board-level DTS and make it clear that way that it is enabled because
>>>> there are child nodes enabled?
>>>
>>> This will be a pretty big effort because every Northstar device I know
>>> has USB
>>> 3.0 PHY in the SoC.
>>
>> Adding a one liner is a "pretty big effort", for sure.
>
> Sorry, we got a misunderstanding here.
>
> I thought you meant adding something like this for every device:
>
> &mdio {
> status = "okay";
>
> usb3_phy: usb-phy@10 {
> compatible = "brcm,ns-ax-usb3-phy";
> reg = <0x10>;
> usb3-dmp-syscon = <&usb3_dmp>;
> #phy-cells = <0>;
> };
> };
Ah no, I would have just done the following in the per-board DTS:
&mdio {
status = "okay";
};
&usb3_phy {
status = "okay";
};
&xhci {
status = "okay";
};
Something like that.
>
> usb3_dmp: syscon@18105000 {
> reg = <0x18105000 0x1000>;
> };
>
> So I clearly missed something important. Did you want to have USB 3.0 PHY
> defined in the dtsi file?
Yes, I think it does make sense to have it defined in the .dtsi file
because it's internal to the SoC, however it should probably be marked
disabled by default, unless a board enables its xHCI controller, does
that make sense?
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT
From: Rafał Miłecki @ 2017-04-19 18:10 UTC (permalink / raw)
To: Florian Fainelli, Rafał Miłecki
Cc: Mark Rutland, devicetree, Hauke Mehrtens, Russell King,
Rob Herring, bcm-kernel-feedback-list, linux-arm-kernel
In-Reply-To: <221a0208-8a64-6bd8-f6b2-39e845520a83@gmail.com>
On 04/19/2017 07:52 PM, Florian Fainelli wrote:
> On 04/19/2017 10:35 AM, Rafał Miłecki wrote:
>> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>>> On 04/02/2017 02:25 PM, Rafał Miłecki wrote:
>>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>>> Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
>>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>>
>>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>>> attached.
>>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>>> yet).
>>>>>>
>>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>>> ---
>>>>>> arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>>> 1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> @@ -320,6 +320,13 @@
>>>>>> };
>>>>>> };
>>>>>>
>>>>>> + mdio@18003000 {
>>>>>> + compatible = "brcm,iproc-mdio";
>>>>>> + reg = <0x18003000 0x8>;
>>>>>> + #size-cells = <1>;
>>>>>> + #address-cells = <0>;
>>>>>> + };
>>>>>
>>>>> This looks fine, but usually the block should be enabled on a per-board
>>>>> basis, such that there should be a status = "disabled" property here by
>>>>> default.
>>>>
>>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>>>> it's
>>>> for stuff that is always present on every SoC family board: rng, nand,
>>>> spi to
>>>> name few.
>>>>
>>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>>> controller so it's enabled by default. Not every board has SPI flash, so
>>>> it's
>>>> disabled by default.
>>>>
>>>> It's there and it make sense to me. Is that OK or not?
>>>
>>> Even though there are devices that are always enabled on a given SoC,
>>> because the board designs are always consistent does not necessarily
>>> make them good candidates to be enabled at the .dtsi level. This is
>>> particularly true when there are external connections to blocks (SPI,
>>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>>> default is safer as a starting point to begin with.
>>
>> In case of Northstar there is USB 3.0 PHY connected *internally* to this
>> MDIO.
>> I don't think any board manufacturer is able to rip SoC out of the MDIO
>> or the
>> USB 3.0 PHY.
>
> OK, then can you still resubmit a proper patch that a) puts that
> information in the commit message, and b) also adds a proper label to
> the mdio node such that it can later on be referenced by label in
> board-level DTS files? By that I mean:
>
> mdio: mdio@18003000 {
>
> Thank you
>
>>
>>
>>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>>> MDIO bus
>>>> just devices may differ and should not be enabled by default.
>>>
>>> In which case, the only difference, for you would be to do to, at the
>>> board-level DTS:
>>>
>>> &mdio {
>>> status = "okay";
>>>
>>> phy@0 {
>>> reg = <0>;
>>> ...
>>> };
>>> };
>>>
>>> versus:
>>>
>>> &mdio {
>>> phy@0 {
>>> reg = <0>;
>>> ...
>>> };
>>> };
>>>
>>> I think we can afford putting the mdio node's status property in each
>>> board-level DTS and make it clear that way that it is enabled because
>>> there are child nodes enabled?
>>
>> This will be a pretty big effort because every Northstar device I know
>> has USB
>> 3.0 PHY in the SoC.
>
> Adding a one liner is a "pretty big effort", for sure.
Sorry, we got a misunderstanding here.
I thought you meant adding something like this for every device:
&mdio {
status = "okay";
usb3_phy: usb-phy@10 {
compatible = "brcm,ns-ax-usb3-phy";
reg = <0x10>;
usb3-dmp-syscon = <&usb3_dmp>;
#phy-cells = <0>;
};
};
usb3_dmp: syscon@18105000 {
reg = <0x18105000 0x1000>;
};
So I clearly missed something important. Did you want to have USB 3.0 PHY
defined in the dtsi file?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/3] drm/vc4: Don't try to initialize FBDEV if we're only bound to V3D.
From: Eric Anholt @ 2017-04-19 17:55 UTC (permalink / raw)
To: Daniel Vetter
Cc: Mark Rutland, devicetree@vger.kernel.org, Rob Herring,
Linux Kernel Mailing List, dri-devel
In-Reply-To: <CAKMK7uFSsA3c+wMfq_O9kSvu0cWLUA-bRqTHyryqi+1ZVeir_w@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 544 bytes --]
Daniel Vetter <daniel@ffwll.ch> writes:
> On Tue, Apr 18, 2017 at 9:11 PM, Eric Anholt <eric@anholt.net> wrote:
>> The FBDEV initialization would throw an error in dmesg, when we just
>> want to silently not initialize fbdev on a V3D-only VC4 instance.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>
> Hm, this shouldn't be an error really, you might want to hotplug more
> connectors later on. What exactly complains?
drm_fb_helper_init() throws an error if the passed in connector count is
0, so drm_fb_cma_helper() printks an error.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT
From: Florian Fainelli @ 2017-04-19 17:52 UTC (permalink / raw)
To: Rafał Miłecki, Florian Fainelli,
Rafał Miłecki
Cc: Hauke Mehrtens, Rob Herring, Mark Rutland, Russell King,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cd324440-ad88-0981-1577-822d39ee289d-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 04/19/2017 10:35 AM, Rafał Miłecki wrote:
> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>> On 04/02/2017 02:25 PM, Rafał Miłecki wrote:
>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>> Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
>>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>
>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>> attached.
>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>> yet).
>>>>>
>>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>> ---
>>>>> arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>> 1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> @@ -320,6 +320,13 @@
>>>>> };
>>>>> };
>>>>>
>>>>> + mdio@18003000 {
>>>>> + compatible = "brcm,iproc-mdio";
>>>>> + reg = <0x18003000 0x8>;
>>>>> + #size-cells = <1>;
>>>>> + #address-cells = <0>;
>>>>> + };
>>>>
>>>> This looks fine, but usually the block should be enabled on a per-board
>>>> basis, such that there should be a status = "disabled" property here by
>>>> default.
>>>
>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>>> it's
>>> for stuff that is always present on every SoC family board: rng, nand,
>>> spi to
>>> name few.
>>>
>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>> controller so it's enabled by default. Not every board has SPI flash, so
>>> it's
>>> disabled by default.
>>>
>>> It's there and it make sense to me. Is that OK or not?
>>
>> Even though there are devices that are always enabled on a given SoC,
>> because the board designs are always consistent does not necessarily
>> make them good candidates to be enabled at the .dtsi level. This is
>> particularly true when there are external connections to blocks (SPI,
>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>> default is safer as a starting point to begin with.
>
> In case of Northstar there is USB 3.0 PHY connected *internally* to this
> MDIO.
> I don't think any board manufacturer is able to rip SoC out of the MDIO
> or the
> USB 3.0 PHY.
OK, then can you still resubmit a proper patch that a) puts that
information in the commit message, and b) also adds a proper label to
the mdio node such that it can later on be referenced by label in
board-level DTS files? By that I mean:
mdio: mdio@18003000 {
Thank you
>
>
>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>> MDIO bus
>>> just devices may differ and should not be enabled by default.
>>
>> In which case, the only difference, for you would be to do to, at the
>> board-level DTS:
>>
>> &mdio {
>> status = "okay";
>>
>> phy@0 {
>> reg = <0>;
>> ...
>> };
>> };
>>
>> versus:
>>
>> &mdio {
>> phy@0 {
>> reg = <0>;
>> ...
>> };
>> };
>>
>> I think we can afford putting the mdio node's status property in each
>> board-level DTS and make it clear that way that it is enabled because
>> there are child nodes enabled?
>
> This will be a pretty big effort because every Northstar device I know
> has USB
> 3.0 PHY in the SoC.
Adding a one liner is a "pretty big effort", for sure.
>
>
>> NB: with a CONFIG_OF system, there is no automatic probing of MDIO child
>> devices because it relies on child nodes being declared, but you would
>> still get the driver to be probed and enabled, which is a waste of
>> resources at best.
>
> Right, but DT role is to describe device/board and not really care if
> operating
> system handles that efficiently.
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
From: Mathieu Poirier @ 2017-04-19 17:49 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
In-Reply-To: <1491485461-22800-7-git-send-email-leo.yan@linaro.org>
On 6 April 2017 at 07:30, Leo Yan <leo.yan@linaro.org> wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
> description for related info in "Part H: External Debug".
>
> Chapter H7 "The Sample-based Profiling Extension" introduces several
> sampling registers, e.g. we can check program counter value with
> combined CPU exception level, secure state, etc. So this is helpful for
> analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
> loop with IRQ disabled. In this case the CPU cannot switch context and
> handle any interrupt (including IPIs), as the result it cannot handle
> SMP call for stack dump.
>
> This patch is to enable coresight debug module, so firstly this driver
> is to bind apb clock for debug module and this is to ensure the debug
> module can be accessed from program or external debugger. And the driver
> uses sample-based registers for debug purpose, e.g. when system triggers
> panic, the driver will dump program counter and combined context
> registers (EDCIDSR, EDVIDSR); by parsing context registers so can
> quickly get to know CPU secure state, exception level, etc.
>
> Some of the debug module registers are located in CPU power domain, so
> this requires the CPU power domain stays on when access related debug
> registers, but the power management for CPU power domain is quite
> dependent on SoC integration for power management. For the platforms
> which with sane power controller implementations, this driver follows
> the method to set EDPRCR to try to pull the CPU out of low power state
> and then set 'no power down request' bit so the CPU has no chance to
> lose power.
>
> If the SoC has not followed up this design well for power management
> controller, the user should use the command line parameter or sysfs
> to constrain all or partial idle states to ensure the CPU power
> domain is enabled and access coresight CPU debug component safely.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> drivers/hwtracing/coresight/Kconfig | 14 +
> drivers/hwtracing/coresight/Makefile | 1 +
> drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++
> 3 files changed, 682 insertions(+)
> create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 130cb21..8d55d6d 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -89,4 +89,18 @@ config CORESIGHT_STM
> logging useful software events or data coming from various entities
> in the system, possibly running different OSs
>
> +config CORESIGHT_CPU_DEBUG
> + tristate "CoreSight CPU Debug driver"
> + depends on ARM || ARM64
> + depends on DEBUG_FS
> + help
> + This driver provides support for coresight debugging module. This
> + is primarily used to dump sample-based profiling registers when
> + system triggers panic, the driver will parse context registers so
> + can quickly get to know program counter (PC), secure state,
> + exception level, etc. Before use debugging functionality, platform
> + needs to ensure the clock domain and power domain are enabled
> + properly, please refer Documentation/trace/coresight-cpu-debug.txt
> + for detailed description and the example for usage.
> +
> endif
> diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
> index af480d9..433d590 100644
> --- a/drivers/hwtracing/coresight/Makefile
> +++ b/drivers/hwtracing/coresight/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
> coresight-etm4x-sysfs.o
> obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
> obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> +obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> new file mode 100644
> index 0000000..8470e31
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -0,0 +1,667 @@
> +/*
> + * Copyright (c) 2017 Linaro Limited. All rights reserved.
> + *
> + * Author: Leo Yan <leo.yan@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +#include <linux/amba/bus.h>
> +#include <linux/coresight.h>
> +#include <linux/cpu.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/pm_qos.h>
> +#include <linux/slab.h>
> +#include <linux/smp.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +
> +#include "coresight-priv.h"
> +
> +#define EDPCSR 0x0A0
> +#define EDCIDSR 0x0A4
> +#define EDVIDSR 0x0A8
> +#define EDPCSR_HI 0x0AC
> +#define EDOSLAR 0x300
> +#define EDPRCR 0x310
> +#define EDPRSR 0x314
> +#define EDDEVID1 0xFC4
> +#define EDDEVID 0xFC8
> +
> +#define EDPCSR_PROHIBITED 0xFFFFFFFF
> +
> +/* bits definition for EDPCSR */
> +#define EDPCSR_THUMB BIT(0)
> +#define EDPCSR_ARM_INST_MASK GENMASK(31, 2)
> +#define EDPCSR_THUMB_INST_MASK GENMASK(31, 1)
> +
> +/* bits definition for EDPRCR */
> +#define EDPRCR_COREPURQ BIT(3)
> +#define EDPRCR_CORENPDRQ BIT(0)
> +
> +/* bits definition for EDPRSR */
> +#define EDPRSR_DLK BIT(6)
> +#define EDPRSR_PU BIT(0)
> +
> +/* bits definition for EDVIDSR */
> +#define EDVIDSR_NS BIT(31)
> +#define EDVIDSR_E2 BIT(30)
> +#define EDVIDSR_E3 BIT(29)
> +#define EDVIDSR_HV BIT(28)
> +#define EDVIDSR_VMID GENMASK(7, 0)
> +
> +/*
> + * bits definition for EDDEVID1:PSCROffset
> + *
> + * NOTE: armv8 and armv7 have different definition for the register,
> + * so consolidate the bits definition as below:
> + *
> + * 0b0000 - Sample offset applies based on the instruction state, we
> + * rely on EDDEVID to check if EDPCSR is implemented or not
> + * 0b0001 - No offset applies.
> + * 0b0010 - No offset applies, but do not use in AArch32 mode
> + *
> + */
> +#define EDDEVID1_PCSR_OFFSET_MASK GENMASK(3, 0)
> +#define EDDEVID1_PCSR_OFFSET_INS_SET (0x0)
> +#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 (0x2)
> +
> +/* bits definition for EDDEVID */
> +#define EDDEVID_PCSAMPLE_MODE GENMASK(3, 0)
> +#define EDDEVID_IMPL_EDPCSR (0x1)
> +#define EDDEVID_IMPL_EDPCSR_EDCIDSR (0x2)
> +#define EDDEVID_IMPL_FULL (0x3)
> +
> +#define DEBUG_WAIT_SLEEP 1000
> +#define DEBUG_WAIT_TIMEOUT 32000
> +
> +struct debug_drvdata {
> + void __iomem *base;
> + struct device *dev;
> + int cpu;
> +
> + bool edpcsr_present;
> + bool edcidsr_present;
> + bool edvidsr_present;
> + bool pc_has_offset;
> +
> + u32 edpcsr;
> + u32 edpcsr_hi;
> + u32 edprsr;
> + u32 edvidsr;
> + u32 edcidsr;
> +};
> +
> +static DEFINE_MUTEX(debug_lock);
> +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
> +static int debug_count;
> +static struct dentry *debug_debugfs_dir;
> +
> +static bool debug_enable;
> +module_param_named(enable, debug_enable, bool, 0600);
> +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> + "(default is 0, which means is disabled by default)");
> +
> +static void debug_os_unlock(struct debug_drvdata *drvdata)
> +{
> + /* Unlocks the debug registers */
> + writel_relaxed(0x0, drvdata->base + EDOSLAR);
> + wmb();
> +}
> +
> +/*
> + * According to ARM DDI 0487A.k, before access external debug
> + * registers should firstly check the access permission; if any
> + * below condition has been met then cannot access debug
> + * registers to avoid lockup issue:
> + *
> + * - CPU power domain is powered off;
> + * - The OS Double Lock is locked;
> + *
> + * By checking EDPRSR can get to know if meet these conditions.
> + */
> +static bool debug_access_permitted(struct debug_drvdata *drvdata)
> +{
> + /* CPU is powered off */
> + if (!(drvdata->edprsr & EDPRSR_PU))
> + return false;
> +
> + /* The OS Double Lock is locked */
> + if (drvdata->edprsr & EDPRSR_DLK)
> + return false;
> +
> + return true;
> +}
> +
> +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
> +{
> + bool retried = false;
> + u32 edprcr;
> +
> +try_again:
> +
> + /*
> + * Send request to power management controller and assert
> + * DBGPWRUPREQ signal; if power management controller has
> + * sane implementation, it should enable CPU power domain
> + * in case CPU is in low power state.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + /* Wait for CPU to be powered up (timeout~=32ms) */
> + if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR,
> + drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU),
> + DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) {
> + /*
> + * Unfortunately the CPU cannot be powered up, so return
> + * back and later has no permission to access other
> + * registers. For this case, should disable CPU low power
> + * states to ensure CPU power domain is enabled!
> + */
> + pr_err("%s: power up request for CPU%d failed\n",
> + __func__, drvdata->cpu);
> + return;
> + }
> +
> + /*
> + * At this point the CPU is powered up, so set the no powerdown
> + * request bit so we don't lose power and emulate power down.
> + */
> + edprcr = readl_relaxed(drvdata->base + EDPRCR);
> + edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
> + writel_relaxed(edprcr, drvdata->base + EDPRCR);
> +
> + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
> +
> + /* Bail out if CPU is powered up */
> + if (likely(drvdata->edprsr & EDPRSR_PU))
> + return;
> +
> + /*
> + * Handle race condition if CPU has been waken up but it sleeps
> + * again if EDPRCR_CORENPDRQ has been flipped, so try to run
> + * waken flow one more time.
> + */
> + if (!retried) {
> + retried = true;
> + goto try_again;
> + }
> +}
> +
> +static void debug_read_regs(struct debug_drvdata *drvdata)
> +{
> + u32 save_edprcr;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Unlock os lock */
> + debug_os_unlock(drvdata);
> +
> + /* Save EDPRCR register */
> + save_edprcr = readl_relaxed(drvdata->base + EDPRCR);
> +
> + /*
> + * Ensure CPU power domain is enabled to let registers
> + * are accessiable.
> + */
> + debug_force_cpu_powered_up(drvdata);
> +
> + if (!debug_access_permitted(drvdata))
> + goto out;
> +
> + drvdata->edpcsr = readl_relaxed(drvdata->base + EDPCSR);
> +
> + /*
> + * As described in ARM DDI 0487A.k, if the processing
> + * element (PE) is in debug state, or sample-based
> + * profiling is prohibited, EDPCSR reads as 0xFFFFFFFF;
> + * EDCIDSR, EDVIDSR and EDPCSR_HI registers also become
> + * UNKNOWN state. So directly bail out for this case.
> + */
> + if (drvdata->edpcsr == EDPCSR_PROHIBITED)
> + goto out;
> +
> + /*
> + * A read of the EDPCSR normally has the side-effect of
> + * indirectly writing to EDCIDSR, EDVIDSR and EDPCSR_HI;
> + * at this point it's safe to read value from them.
> + */
> + if (IS_ENABLED(CONFIG_64BIT))
> + drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
> +
> + if (drvdata->edcidsr_present)
> + drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
> +
> + if (drvdata->edvidsr_present)
> + drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
> +
> +out:
> + /* Restore EDPRCR register */
> + writel_relaxed(save_edprcr, drvdata->base + EDPRCR);
> +
> + CS_LOCK(drvdata->base);
> +}
> +
> +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata)
> +{
> + unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
> + unsigned long pc;
> +
> + if (IS_ENABLED(CONFIG_64BIT))
> + return (unsigned long)drvdata->edpcsr_hi << 32 |
> + (unsigned long)drvdata->edpcsr;
> +
> + pc = (unsigned long)drvdata->edpcsr;
> +
> + if (drvdata->pc_has_offset) {
> + arm_inst_offset = 8;
> + thumb_inst_offset = 4;
> + }
> +
> + /* Handle thumb instruction */
> + if (pc & EDPCSR_THUMB) {
> + pc = (pc & EDPCSR_THUMB_INST_MASK) - thumb_inst_offset;
> + return pc;
> + }
> +
> + /*
> + * Handle arm instruction offset, if the arm instruction
> + * is not 4 byte alignment then it's possible the case
> + * for implementation defined; keep original value for this
> + * case and print info for notice.
> + */
> + if (pc & BIT(1))
> + pr_emerg("Instruction offset is implementation defined\n");
> + else
> + pc = (pc & EDPCSR_ARM_INST_MASK) - arm_inst_offset;
> +
> + return pc;
> +}
> +
> +static void debug_dump_regs(struct debug_drvdata *drvdata)
> +{
> + unsigned long pc;
> +
> + pr_emerg("\tEDPRSR: %08x (Power:%s DLK:%s)\n", drvdata->edprsr,
> + drvdata->edprsr & EDPRSR_PU ? "On" : "Off",
> + drvdata->edprsr & EDPRSR_DLK ? "Lock" : "Unlock");
> +
> + if (!debug_access_permitted(drvdata)) {
> + pr_emerg("No permission to access debug registers!\n");
> + return;
> + }
> +
> + if (drvdata->edpcsr == EDPCSR_PROHIBITED) {
> + pr_emerg("CPU is in Debug state or profiling is prohibited!\n");
> + return;
> + }
> +
> + pc = debug_adjust_pc(drvdata);
> + pr_emerg("\tEDPCSR: [<%p>] %pS\n", (void *)pc, (void *)pc);
> +
> + if (drvdata->edcidsr_present)
> + pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
> +
> + if (drvdata->edvidsr_present)
> + pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits VMID:%x)\n",
> + drvdata->edvidsr,
> + drvdata->edvidsr & EDVIDSR_NS ? "Non-secure" : "Secure",
> + drvdata->edvidsr & EDVIDSR_E3 ? "EL3" :
> + (drvdata->edvidsr & EDVIDSR_E2 ? "EL2" : "EL1/0"),
> + drvdata->edvidsr & EDVIDSR_HV ? 64 : 32,
> + drvdata->edvidsr & (u32)EDVIDSR_VMID);
> +}
> +
> +static void debug_init_arch_data(void *info)
> +{
> + struct debug_drvdata *drvdata = info;
> + u32 mode, pcsr_offset;
> + u32 eddevid, eddevid1;
> +
> + CS_UNLOCK(drvdata->base);
> +
> + /* Read device info */
> + eddevid = readl_relaxed(drvdata->base + EDDEVID);
> + eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
> +
> + CS_LOCK(drvdata->base);
> +
> + /* Parse implementation feature */
> + mode = eddevid & EDDEVID_PCSAMPLE_MODE;
> + pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
> +
> + drvdata->edpcsr_present = false;
> + drvdata->edcidsr_present = false;
> + drvdata->edvidsr_present = false;
> + drvdata->pc_has_offset = false;
> +
> + switch (mode) {
> + case EDDEVID_IMPL_FULL:
> + drvdata->edvidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR_EDCIDSR:
> + drvdata->edcidsr_present = true;
> + /* Fall through */
> + case EDDEVID_IMPL_EDPCSR:
> + /*
> + * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to
> + * define if has the offset for PC sampling value; if read
> + * back EDDEVID1.PCSROffset == 0x2, then this means the debug
> + * module does not sample the instruction set state when
> + * armv8 CPU in AArch32 state.
> + */
> + drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) ||
> + (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
Following my previous comment in patch 2/6 this condition needs to be
reviewed to deal with conditions where pcsr_offset == 0 on AArch64.
Probably
drvdata->edpcsr_present = ((IS_ENABLED(CONFIG_64BIT) &&
pcsr_offset != 0)) ||
(pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
> +
> + drvdata->pc_has_offset =
> + (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +/*
> + * Dump out information on panic.
> + */
> +static int debug_notifier_call(struct notifier_block *self,
> + unsigned long v, void *p)
> +{
> + int cpu;
> + struct debug_drvdata *drvdata;
> +
> + pr_emerg("ARM external debug module:\n");
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pr_emerg("CPU[%d]:\n", drvdata->cpu);
> +
> + debug_read_regs(drvdata);
> + debug_dump_regs(drvdata);
> + }
> +
> + return 0;
> +}
> +
> +static struct notifier_block debug_notifier = {
> + .notifier_call = debug_notifier_call,
> +};
> +
> +static int debug_enable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_get_sync(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static int debug_disable_func(void)
> +{
> + struct debug_drvdata *drvdata;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + drvdata = per_cpu(debug_drvdata, cpu);
> + if (!drvdata)
> + continue;
> +
> + pm_runtime_put(drvdata->dev);
> + }
> +
> + return atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static ssize_t debug_func_knob_write(struct file *f,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + u8 val;
> + int ret;
> +
> + ret = kstrtou8_from_user(buf, count, 2, &val);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&debug_lock);
> +
> + if (val == debug_enable)
> + goto out;
> +
> + if (val)
> + ret = debug_enable_func();
> + else
> + ret = debug_disable_func();
> +
> + if (ret) {
> + pr_err("%s: unable to %s debug function: %d\n",
> + __func__, val ? "enable" : "disable", ret);
> + goto err;
> + }
> +
> + debug_enable = val;
> +out:
> + ret = count;
> +err:
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static ssize_t debug_func_knob_read(struct file *f,
> + char __user *ubuf, size_t count, loff_t *ppos)
> +{
> + ssize_t ret;
> + char buf[2];
> +
> + mutex_lock(&debug_lock);
> +
> + buf[0] = '0' + debug_enable;
> + buf[1] = '\n';
> + ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
> +
> + mutex_unlock(&debug_lock);
> + return ret;
> +}
> +
> +static const struct file_operations debug_func_knob_fops = {
> + .open = simple_open,
> + .read = debug_func_knob_read,
> + .write = debug_func_knob_write,
> +};
> +
> +static int debug_func_init(void)
> +{
> + struct dentry *file;
> + int ret;
> +
> + /* Create debugfs node */
> + debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
> + if (!debug_debugfs_dir) {
> + pr_err("%s: unable to create debugfs directory\n", __func__);
> + return -ENOMEM;
> + }
> +
> + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
> + debug_debugfs_dir, NULL, &debug_func_knob_fops);
> + if (!file) {
> + pr_err("%s: unable to create enable knob file\n", __func__);
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + /* Use sysfs node to enable functionality */
> + if (!debug_enable)
> + return 0;
> +
> + /* Register function to be called for panic */
> + ret = atomic_notifier_chain_register(&panic_notifier_list,
> + &debug_notifier);
> + if (ret) {
> + pr_err("%s: unable to register notifier: %d\n",
> + __func__, ret);
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + debugfs_remove_recursive(debug_debugfs_dir);
> + return ret;
> +}
> +
> +static void debug_func_exit(void)
> +{
> + debugfs_remove_recursive(debug_debugfs_dir);
> +
> + /* Unregister panic notifier callback */
> + if (debug_enable)
> + atomic_notifier_chain_unregister(&panic_notifier_list,
> + &debug_notifier);
> +}
> +
> +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> + void __iomem *base;
> + struct device *dev = &adev->dev;
> + struct debug_drvdata *drvdata;
> + struct resource *res = &adev->res;
> + struct device_node *np = adev->dev.of_node;
> + int ret;
> +
> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> + if (per_cpu(debug_drvdata, drvdata->cpu)) {
> + dev_err(dev, "CPU%d drvdata has been initialized\n",
> + drvdata->cpu);
> + return -EBUSY;
> + }
> +
> + drvdata->dev = &adev->dev;
> + amba_set_drvdata(adev, drvdata);
> +
> + /* Validity for the resource is already checked by the AMBA core */
> + base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + drvdata->base = base;
> +
> + get_online_cpus();
> + per_cpu(debug_drvdata, drvdata->cpu) = drvdata;
> + ret = smp_call_function_single(drvdata->cpu,
> + debug_init_arch_data, drvdata, 1);
> + put_online_cpus();
> +
> + if (ret) {
> + dev_err(dev, "CPU%d debug arch init failed\n", drvdata->cpu);
> + goto err;
> + }
> +
> + if (!drvdata->edpcsr_present) {
> + dev_err(dev, "CPU%d sample-based profiling isn't implemented\n",
> + drvdata->cpu);
> + ret = -ENXIO;
> + goto err;
> + }
> +
> + if (!debug_count++) {
> + ret = debug_func_init();
> + if (ret)
> + goto err_func_init;
> + }
> +
> + if (!debug_enable)
> + pm_runtime_put(dev);
> +
> + dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu);
> + return 0;
> +
> +err_func_init:
> + debug_count--;
> +err:
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> + return ret;
> +}
> +
> +static int debug_remove(struct amba_device *adev)
> +{
> + struct device *dev = &adev->dev;
> + struct debug_drvdata *drvdata = amba_get_drvdata(adev);
> +
> + per_cpu(debug_drvdata, drvdata->cpu) = NULL;
> +
> + if (debug_enable)
> + pm_runtime_put(dev);
> +
> + if (!--debug_count)
> + debug_func_exit();
> +
> + return 0;
> +}
> +
> +static struct amba_id debug_ids[] = {
> + { /* Debug for Cortex-A53 */
> + .id = 0x000bbd03,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A57 */
> + .id = 0x000bbd07,
> + .mask = 0x000fffff,
> + },
> + { /* Debug for Cortex-A72 */
> + .id = 0x000bbd08,
> + .mask = 0x000fffff,
> + },
> + { 0, 0 },
> +};
> +
> +static struct amba_driver debug_driver = {
> + .drv = {
> + .name = "coresight-cpu-debug",
> + .suppress_bind_attrs = true,
> + },
> + .probe = debug_probe,
> + .remove = debug_remove,
> + .id_table = debug_ids,
> +};
> +
> +module_amba_driver(debug_driver);
> +
> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
> +MODULE_DESCRIPTION("ARM Coresight CPU Debug Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH] of: introduce event tracepoints for dynamic device_node lifecyle
From: Frank Rowand @ 2017-04-19 17:44 UTC (permalink / raw)
To: Tyrel Datwyler, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
Cc: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
nfont-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
mpe-Gsx/Oe8HsFggBc27wqDAHg, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mingo-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <1492475525-10827-1-git-send-email-tyreld-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
On 04/17/17 17:32, Tyrel Datwyler wrote:
> This patch introduces event tracepoints for tracking a device_nodes
> reference cycle as well as reconfig notifications generated in response
> to node/property manipulations.
>
> With the recent upstreaming of the refcount API several device_node
> underflows and leaks have come to my attention in the pseries (DLPAR) dynamic
> logical partitioning code (ie. POWER speak for hotplugging virtual and physcial
> resources at runtime such as cpus or IOAs). These tracepoints provide a
> easy and quick mechanism for validating the reference counting of
> device_nodes during their lifetime.
>
> Further, when pseries lpars are migrated to a different machine we
> perform a live update of our device tree to bring it into alignment with the
> configuration of the new machine. The of_reconfig_notify trace point
> provides a mechanism that can be turned for debuging the device tree
> modifications with out having to build a custom kernel to get at the
> DEBUG code introduced by commit 00aa3720.
Is the normal kernel built with CONFIG_DYNAMIC_DEBUG=y? If so, then
simply removing the "ifdef DEBUG" around the switch in
of_reconfig_notify() would solve that issue.
-Frank
> The following trace events are provided: of_node_get, of_node_put,
> of_node_release, and of_reconfig_notify. These trace points require a kernel
> built with ftrace support to be enabled. In a typical environment where
> debugfs is mounted at /sys/kernel/debug the entire set of tracepoints
> can be set with the following:
>
> echo "of:*" > /sys/kernel/debug/tracing/set_event
>
> or
>
> echo 1 > /sys/kernel/debug/tracing/of/enable
>
> The following shows the trace point data from a DLPAR remove of a cpu
> from a pseries lpar:
>
> cat /sys/kernel/debug/tracing/trace | grep "POWER8@10"
>
> cpuhp/23-147 [023] .... 128.324827:
> of_node_put: refcount=5, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324829:
> of_node_put: refcount=4, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324829:
> of_node_put: refcount=3, dn->full_name=/cpus/PowerPC,POWER8@10
> cpuhp/23-147 [023] .... 128.324831:
> of_node_put: refcount=2, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439000:
> of_node_put: refcount=1, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439002:
> of_reconfig_notify: action=DETACH_NODE, dn->full_name=/cpus/PowerPC,POWER8@10,
> prop->name=null, old_prop->name=null
> drmgr-7284 [009] .... 128.439015:
> of_node_put: refcount=0, dn->full_name=/cpus/PowerPC,POWER8@10
> drmgr-7284 [009] .... 128.439016:
> of_node_release: dn->full_name=/cpus/PowerPC,POWER8@10, dn->_flags=4
>
> Signed-off-by: Tyrel Datwyler <tyreld-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
> drivers/of/dynamic.c | 30 ++++++---------
> include/trace/events/of.h | 93 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 105 insertions(+), 18 deletions(-)
> create mode 100644 include/trace/events/of.h
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 888fdbc..85c0966 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -16,6 +16,9 @@
>
> #include "of_private.h"
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/of.h>
> +
> /**
> * of_node_get() - Increment refcount of a node
> * @node: Node to inc refcount, NULL is supported to simplify writing of
> @@ -25,8 +28,10 @@
> */
> struct device_node *of_node_get(struct device_node *node)
> {
> - if (node)
> + if (node) {
> kobject_get(&node->kobj);
> + trace_of_node_get(refcount_read(&node->kobj.kref.refcount), node->full_name);
> + }
> return node;
> }
> EXPORT_SYMBOL(of_node_get);
> @@ -38,8 +43,10 @@ struct device_node *of_node_get(struct device_node *node)
> */
> void of_node_put(struct device_node *node)
> {
> - if (node)
> + if (node) {
> + trace_of_node_put(refcount_read(&node->kobj.kref.refcount) - 1, node->full_name);
> kobject_put(&node->kobj);
> + }
> }
> EXPORT_SYMBOL(of_node_put);
>
> @@ -92,24 +99,9 @@ int of_reconfig_notifier_unregister(struct notifier_block *nb)
> int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)
> {
> int rc;
> -#ifdef DEBUG
> - struct of_reconfig_data *pr = p;
>
> - switch (action) {
> - case OF_RECONFIG_ATTACH_NODE:
> - case OF_RECONFIG_DETACH_NODE:
> - pr_debug("notify %-15s %s\n", action_names[action],
> - pr->dn->full_name);
> - break;
> - case OF_RECONFIG_ADD_PROPERTY:
> - case OF_RECONFIG_REMOVE_PROPERTY:
> - case OF_RECONFIG_UPDATE_PROPERTY:
> - pr_debug("notify %-15s %s:%s\n", action_names[action],
> - pr->dn->full_name, pr->prop->name);
> - break;
> + trace_of_reconfig_notify(action, p);
>
> - }
> -#endif
> rc = blocking_notifier_call_chain(&of_reconfig_chain, action, p);
> return notifier_to_errno(rc);
> }
> @@ -326,6 +318,8 @@ void of_node_release(struct kobject *kobj)
> struct device_node *node = kobj_to_device_node(kobj);
> struct property *prop = node->properties;
>
> + trace_of_node_release(node);
> +
> /* We should never be releasing nodes that haven't been detached. */
> if (!of_node_check_flag(node, OF_DETACHED)) {
> pr_err("ERROR: Bad of_node_put() on %s\n", node->full_name);
> diff --git a/include/trace/events/of.h b/include/trace/events/of.h
> new file mode 100644
> index 0000000..0d53271
> --- /dev/null
> +++ b/include/trace/events/of.h
> @@ -0,0 +1,93 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM of
> +
> +#if !defined(_TRACE_OF_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_OF_H
> +
> +#include <linux/of.h>
> +#include <linux/tracepoint.h>
> +
> +DECLARE_EVENT_CLASS(of_node_ref_template,
> +
> + TP_PROTO(int refcount, const char* dn_name),
> +
> + TP_ARGS(refcount, dn_name),
> +
> + TP_STRUCT__entry(
> + __string(dn_name, dn_name)
> + __field(int, refcount)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dn_name, dn_name);
> + __entry->refcount = refcount;
> + ),
> +
> + TP_printk("refcount=%d, dn->full_name=%s",
> + __entry->refcount, __get_str(dn_name))
> +);
> +
> +DEFINE_EVENT(of_node_ref_template, of_node_get,
> + TP_PROTO(int refcount, const char* dn_name),
> + TP_ARGS(refcount, dn_name));
> +
> +DEFINE_EVENT(of_node_ref_template, of_node_put,
> + TP_PROTO(int refcount, const char* dn_name),
> + TP_ARGS(refcount, dn_name));
> +
> +TRACE_EVENT(of_node_release,
> +
> + TP_PROTO(struct device_node *dn),
> +
> + TP_ARGS(dn),
> +
> + TP_STRUCT__entry(
> + __string(dn_name, dn->full_name)
> + __field(unsigned long, flags)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dn_name, dn->full_name);
> + __entry->flags = dn->_flags;
> + ),
> +
> + TP_printk("dn->full_name=%s, dn->_flags=%lu",
> + __get_str(dn_name), __entry->flags)
> +);
> +
> +#define of_reconfig_action_names \
> + {OF_RECONFIG_ATTACH_NODE, "ATTACH_NODE"}, \
> + {OF_RECONFIG_DETACH_NODE, "DETACH_NODE"}, \
> + {OF_RECONFIG_ADD_PROPERTY, "ADD_PROPERTY"}, \
> + {OF_RECONFIG_REMOVE_PROPERTY, "REMOVE_PROPERTY"}, \
> + {OF_RECONFIG_UPDATE_PROPERTY, "UPDATE_PROPERTY"}
> +
> +TRACE_EVENT(of_reconfig_notify,
> +
> + TP_PROTO(unsigned long action, struct of_reconfig_data *ord),
> +
> + TP_ARGS(action, ord),
> +
> + TP_STRUCT__entry(
> + __field(unsigned long, action)
> + __string(dn_name, ord->dn->full_name)
> + __string(prop_name, ord->prop ? ord->prop->name : "null")
> + __string(oldprop_name, ord->old_prop ? ord->old_prop->name : "null")
> + ),
> +
> + TP_fast_assign(
> + __entry->action = action;
> + __assign_str(dn_name, ord->dn->full_name);
> + __assign_str(prop_name, ord->prop ? ord->prop->name : "null");
> + __assign_str(oldprop_name, ord->old_prop ? ord->old_prop->name : "null");
> + ),
> +
> + TP_printk("action=%s, dn->full_name=%s, prop->name=%s, old_prop->name=%s",
> + __print_symbolic(__entry->action, of_reconfig_action_names),
> + __get_str(dn_name), __get_str(prop_name), __get_str(oldprop_name))
> +);
> +
> +#endif /* _TRACE_OF_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT
From: Rafał Miłecki @ 2017-04-19 17:35 UTC (permalink / raw)
To: Florian Fainelli, Rafał Miłecki
Cc: Hauke Mehrtens, Rob Herring, Mark Rutland, Russell King,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <94250925-7d5d-ac31-58ad-918406d904f1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 04/19/2017 06:43 PM, Florian Fainelli wrote:
> On 04/02/2017 02:25 PM, Rafał Miłecki wrote:
>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>> Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>
>>>> Northstar devices have MDIO bus that may contain various PHYs attached.
>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).
>>>>
>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>> ---
>>>> arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>> index acee36a61004..6a2afe7880ae 100644
>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>> @@ -320,6 +320,13 @@
>>>> };
>>>> };
>>>>
>>>> + mdio@18003000 {
>>>> + compatible = "brcm,iproc-mdio";
>>>> + reg = <0x18003000 0x8>;
>>>> + #size-cells = <1>;
>>>> + #address-cells = <0>;
>>>> + };
>>>
>>> This looks fine, but usually the block should be enabled on a per-board
>>> basis, such that there should be a status = "disabled" property here by
>>> default.
>>
>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>> it's
>> for stuff that is always present on every SoC family board: rng, nand,
>> spi to
>> name few.
>>
>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>> controller so it's enabled by default. Not every board has SPI flash, so
>> it's
>> disabled by default.
>>
>> It's there and it make sense to me. Is that OK or not?
>
> Even though there are devices that are always enabled on a given SoC,
> because the board designs are always consistent does not necessarily
> make them good candidates to be enabled at the .dtsi level. This is
> particularly true when there are external connections to blocks (SPI,
> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
> default is safer as a starting point to begin with.
In case of Northstar there is USB 3.0 PHY connected *internally* to this MDIO.
I don't think any board manufacturer is able to rip SoC out of the MDIO or the
USB 3.0 PHY.
>> I find MDIO situation quite simiar. It seems every Northstar board has
>> MDIO bus
>> just devices may differ and should not be enabled by default.
>
> In which case, the only difference, for you would be to do to, at the
> board-level DTS:
>
> &mdio {
> status = "okay";
>
> phy@0 {
> reg = <0>;
> ...
> };
> };
>
> versus:
>
> &mdio {
> phy@0 {
> reg = <0>;
> ...
> };
> };
>
> I think we can afford putting the mdio node's status property in each
> board-level DTS and make it clear that way that it is enabled because
> there are child nodes enabled?
This will be a pretty big effort because every Northstar device I know has USB
3.0 PHY in the SoC.
> NB: with a CONFIG_OF system, there is no automatic probing of MDIO child
> devices because it relies on child nodes being declared, but you would
> still get the driver to be probed and enabled, which is a waste of
> resources at best.
Right, but DT role is to describe device/board and not really care if operating
system handles that efficiently.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 2/8] doc: Add documentation for Coresight CPU debug
From: Mathieu Poirier @ 2017-04-19 17:25 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
linux-soc-u79uwXL29TY76Z2rM5mHXA, Mike Leach, Sudeep Holla
In-Reply-To: <1491485461-22800-3-git-send-email-leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On 6 April 2017 at 07:30, Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Update kernel-parameters.txt to add new parameter:
> coresight_cpu_debug.enable is a knob to enable debugging at boot time.
>
> Add detailed documentation, which contains the implementation, Mike
> Leach excellent summary for "clock and power domain". At the end some
> examples on how to enable the debugging functionality are provided.
>
> Suggested-by: Mike Leach <mike.leach-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 7 +
> Documentation/trace/coresight-cpu-debug.txt | 173 ++++++++++++++++++++++++
> 2 files changed, 180 insertions(+)
> create mode 100644 Documentation/trace/coresight-cpu-debug.txt
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index facc20a..cf90146 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -650,6 +650,13 @@
> /proc/<pid>/coredump_filter.
> See also Documentation/filesystems/proc.txt.
>
> + coresight_cpu_debug.enable
> + [ARM,ARM64]
> + Format: <bool>
> + Enable/disable the CPU sampling based debugging.
> + 0: default value, disable debugging
> + 1: enable debugging at boot time
> +
> cpuidle.off=1 [CPU_IDLE]
> disable the cpuidle sub-system
>
> diff --git a/Documentation/trace/coresight-cpu-debug.txt b/Documentation/trace/coresight-cpu-debug.txt
> new file mode 100644
> index 0000000..e7ad05e
> --- /dev/null
> +++ b/Documentation/trace/coresight-cpu-debug.txt
> @@ -0,0 +1,173 @@
> + Coresight CPU Debug Module
> + ==========================
> +
> + Author: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + Date: April 5th, 2017
> +
> +Introduction
> +------------
> +
> +Coresight CPU debug module is defined in ARMv8-a architecture reference manual
> +(ARM DDI 0487A.k) Chapter 'Part H: External debug', the CPU can integrate
> +debug module and it is mainly used for two modes: self-hosted debug and
> +external debug. Usually the external debug mode is well known as the external
> +debugger connects with SoC from JTAG port; on the other hand the program can
> +explore debugging method which rely on self-hosted debug mode, this document
> +is to focus on this part.
> +
> +The debug module provides sample-based profiling extension, which can be used
> +to sample CPU program counter, secure state and exception level, etc; usually
> +every CPU has one dedicated debug module to be connected. Based on self-hosted
> +debug mechanism, Linux kernel can access these related registers from mmio
> +region when the kernel panic happens. The callback notifier for kernel panic
> +will dump related registers for every CPU; finally this is good for assistant
> +analysis for panic.
> +
> +
> +Implementation
> +--------------
> +
> +- During driver registration, use EDDEVID and EDDEVID1 two device ID
> + registers to decide if sample-based profiling is implemented or not. On some
> + platforms this hardware feature is fully or partialy implemented; and if
> + this feature is not supported then registration will fail.
> +
> +- When write this doc, the debug driver mainly relies on three sampling
> + registers. The kernel panic callback notifier gathers info from EDPCSR
> + EDVIDSR and EDCIDSR; from EDPCSR we can get program counter, EDVIDSR has
> + information for secure state, exception level, bit width, etc; EDCIDSR is
> + context ID value which contains the sampled value of CONTEXTIDR_EL1.
> +
> +- The driver supports CPU running mode with either AArch64 or AArch32. The
> + registers naming convention is a bit different between them, AArch64 uses
> + 'ED' for register prefix (ARM DDI 0487A.k, chapter H9.1) and AArch32 uses
> + 'DBG' as prefix (ARM DDI 0487A.k, chapter G5.1). The driver is unified to
> + use AArch64 naming convention.
> +
> +- ARMv8-a (ARM DDI 0487A.k) and ARMv7-a (ARM DDI 0406C.b) have different
> + register bits definition. So the driver consolidates two difference:
> +
> + If PCSROffset=0b0000. ARMv8-a doesn't apply offset on PCSR, but ARMv7-a
> + defines "PCSR samples are offset by a value that depends on the instruction
> + set state".
I had to look in the documentation (ID092916) a little here. On ARMv8
if DEVID1.PCSROffset is 0 then the feature is not implemented - as
such the first part of the sentence should be corrected. The rest of
the sentence about ARMv7 is correct.
> For ARMv7-a, the driver checks furthermore if CPU runs with ARM
> + or thumb instruction set and calibrate PCSR value, the detailed description
> + for offset is in ARMv7-a ARM (ARM DDI 0406C.b) chapter C11.11.34 "DBGPCSR,
> + Program Counter Sampling Register".
> +
> + If PCSROffset=0b0010, ARMv8-a defines "EDPCSR implemented, and samples have
> + no offset applied and do not sample the instruction set state in AArch32
> + state". So for the case when CPU runs with AArch32 state and PCSROffset=
> + 0b0010, the driver considers PCSR doesn't really work.
Please remove the word "really" - it either works or it doesn't. So
on ARMv8 if DEVID1.PCSROffset is 0x2 and the CPU operates in AArch32
state, EDPCSR is not sampled. When the CPU operates in AArch64 state
EDPCSR is sampled and no offset are applied.
> +
> +
> +Clock and power domain
> +----------------------
> +
> +Before accessing debug registers, we should ensure the clock and power domain
> +have been enabled properly. In ARMv8-a ARM (ARM DDI 0487A.k) chapter 'H9.1
> +Debug registers', the debug registers are spread into two domains: the debug
> +domain and the CPU domain.
> +
> + +---------------+
> + | |
> + | |
> + +----------+--+ |
> + dbg_clk -->| |**| |<-- cpu_clk
> + | Debug |**| CPU |
> + dbg_pd -->| |**| |<-- cpu_pd
> + +----------+--+ |
> + | |
> + | |
> + +---------------+
> +
> +For debug domain, the user uses DT binding "clocks" and "power-domains" to
> +specify the corresponding clock source and power supply for the debug logic.
> +The driver calls the pm_runtime_{put|get} operations as needed to handle the
> +debug power domain.
> +
> +For CPU domain, the different SoC designs have different power management
> +schemes and finally this heavily impacts external debug module. So we can
> +divide into below cases:
> +
> +- On systems with a sane power controller which can behave correctly with
> + respect to CPU power domain, the CPU power domain can be controlled by
> + register EDPRCR in driver. The driver firstly writes bit EDPRCR.COREPURQ
> + to power up the CPU, and then writes bit EDPRCR.CORENPDRQ for emulation
> + of CPU power down. As result, this can ensure the CPU power domain is
> + powered on properly during the period when access debug related registers;
> +
> +- Some designs will power down an entire cluster if all CPUs on the cluster
> + are powered down - including the parts of the debug registers that should
> + remain powered in the debug power domain. The bits in EDPRCR are not
> + respected in these cases, so these designs do not really support debug over
> + power down in the way that the CoreSight / Debug designers anticipated.
> + This means that even checking EDPRSR has the potential to cause a bus hang
> + if the target register is unpowered.
> +
> + In this case, accessing to the debug registers while they are not powered
> + is a recipe for disaster; so we need preventing CPU low power states at boot
> + time or when user enable module at the run time. Please see chapter
> + "How to use the module" for detailed usage info for this.
> +
> +
> +Device Tree Bindings
> +--------------------
> +
> +See Documentation/devicetree/bindings/arm/coresight-cpu-debug.txt for details.
> +
> +
> +How to use the module
> +---------------------
> +
> +If you want to enable debugging functionality at boot time, you can add
> +"coresight_cpu_debug.enable=1" to the kernel command line parameter.
> +
> +The driver also can work as module, so can enable the debugging when insmod
> +module:
> +# insmod coresight_cpu_debug.ko debug=1
> +
> +When boot time or insmod module you have not enabled the debugging, the driver
> +uses the debugfs file system to provide a knob to dynamically enable or disable
> +debugging:
> +
> +To enable it, write a '1' into /sys/kernel/debug/coresight_cpu_debug/enable:
> +# echo 1 > /sys/kernel/debug/coresight_cpu_debug/enable
> +
> +To disable it, write a '0' into /sys/kernel/debug/coresight_cpu_debug/enable:
> +# echo 0 > /sys/kernel/debug/coresight_cpu_debug/enable
> +
> +As explained in chapter "Clock and power domain", if you are working on one
> +platform which has idle states to power off debug logic and the power
> +controller cannot work well for the request from EDPRCR, then you should
> +firstly constraint CPU idle states before enable CPU debugging feature; so can
> +ensure the accessing to debug logic.
> +
> +If you want to limit idle states at boot time, you can use "nohlt" or
> +"cpuidle.off=1" in the kernel command line.
> +
> +At the runtime you can disable idle states with below methods:
> +
> +Set latency request to /dev/cpu_dma_latency to disable all CPUs specific idle
> +states (if latency = 0uS then disable all idle states):
> +# echo "what_ever_latency_you_need_in_uS" > /dev/cpu_dma_latency
> +
> +Disable specific CPU's specific idle state:
> +# echo 1 > /sys/devices/system/cpu/cpu$cpu/cpuidle/state$state/disable
> +
> +
> +Output format
> +-------------
> +
> +Here is an example of the debugging output format:
> +
> +ARM external debug module:
> +CPU[0]:
> + EDPRSR: 0000000b (Power:On DLK:Unlock)
> + EDPCSR: [<ffff00000808eb54>] handle_IPI+0xe4/0x150
> + EDCIDSR: 00000000
> + EDVIDSR: 90000000 (State:Non-secure Mode:EL1/0 Width:64bits VMID:0)
> +CPU[1]:
> + EDPRSR: 0000000b (Power:On DLK:Unlock)
> + EDPCSR: [<ffff0000087a64c0>] debug_notifier_call+0x108/0x288
> + EDCIDSR: 00000000
> + EDVIDSR: 90000000 (State:Non-secure Mode:EL1/0 Width:64bits VMID:0)
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv3] Documentation: dt-bindings: Add binding documentation for TI clkctrl clocks
From: Stephen Boyd @ 2017-04-19 17:16 UTC (permalink / raw)
To: Tony Lindgren
Cc: Tero Kristo, Michael Turquette, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA, Paul Walmsley
In-Reply-To: <20170419170246.GA19537-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
On 04/19, Tony Lindgren wrote:
> * Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> [170419 09:58]:
> > On 02/13, Tero Kristo wrote:
> > > On 24/01/17 00:17, Tony Lindgren wrote:
> > >
> > > This binding works for me, so:
> > >
> > > Acked-by: Tero Kristo <t-kristo-l0cyMroinI0@public.gmane.org>
> > >
> >
> > This wasn't in the TI clk PR. Shall I merge this to clk-next?
>
> Tero has this included in his clkctrl series and there's one typo
> fix needed where the doc still says #clock-cells = <4> instead of <2>.
>
> I suggest let's wait a bit on this and let's try to get the clkctrl
> driver parts into next early after v4.12-rc1 if no issues.
>
> It seems the clkctrl series clock driver part is pretty much
> ready to go, the remaining issues seem to be in the hwmod code.
>
Ok thanks. I'll remove this from my queue.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] fpga: region: add missing DT documentation for config complete timeout
From: Moritz Fischer @ 2017-04-19 17:16 UTC (permalink / raw)
To: Tobias Klauser
Cc: Alan Tull, Moritz Fischer, Rob Herring, Mark Rutland,
linux-fpga-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170418134003.13830-1-tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org>
Hi Tobias,
On Tue, Apr 18, 2017 at 03:40:03PM +0200, Tobias Klauser wrote:
> Commit 42d5ec954719 ("fpga: add config complete timeout") introduced the
> config complete property but didn't include the corresponding DT binding
> documentation. Add it now.
>
> Signed-off-by: Tobias Klauser <tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org>
Reviewed-by: Moritz Fischer <mdf-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> Documentation/devicetree/bindings/fpga/fpga-region.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> index 81bf3adba24b..6db8aeda461a 100644
> --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> @@ -193,6 +193,8 @@ Optional properties:
> - region-freeze-timeout-us : The maximum time in microseconds to wait for
> bridges to successfully become disabled before the region has been
> programmed.
> +- config-complete-timeout-us : The maximum time in microseconds time for the
> + FPGA to go to operating mode after the region has been programmed.
> - child nodes : devices in the FPGA after programming.
>
> In the example below, when an overlay is applied targeting fpga-region0,
> --
> 2.12.2
>
>
Looks good to me, Maybe Rob has got something,
Moritz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv3] Documentation: dt-bindings: Add binding documentation for TI clkctrl clocks
From: Tony Lindgren @ 2017-04-19 17:02 UTC (permalink / raw)
To: Stephen Boyd
Cc: Tero Kristo, Michael Turquette, devicetree, linux-clk, linux-omap,
Paul Walmsley
In-Reply-To: <20170419165522.GL7065@codeaurora.org>
* Stephen Boyd <sboyd@codeaurora.org> [170419 09:58]:
> On 02/13, Tero Kristo wrote:
> > On 24/01/17 00:17, Tony Lindgren wrote:
> > >Texas Instruments omap variant SoCs starting with omap4 have a clkctrl
> > >clock controller instance for each interconnect target module. The clkctrl
> > >controls functional and interface clocks for the module.
> > >
> > >The clkctrl clocks are currently handled by arch/arm/mach-omap2 hwmod code.
> > >With this binding and a related clock device driver we can start moving the
> > >clkctrl clock handling to live in drivers/clk/ti.
> > >
> > >Note that this binding allows keeping the clockdomain related parts out of
> > >drivers/clock. The CLKCTCTRL and DYNAMICDEP registers can be handled by
> > >a separate driver in drivers/soc/ti and genpd. If the clockdomain driver
> > >needs to know it's clocks, we can just set the the clkctrl device
> > >instances to be children of the related clockdomain device.
> > >
> > >Each clkctrl clock can have multiple optional gate clocks, and multiple
> > >optional mux clocks. To represent this in device tree, it seems that
> > >it is best done using four clock cells #clock-cells = <2> property.
> > >
> > >The reasons for using #clock-cells = <2> are:
> > >
> > >1. We need to specify the clkctrl offset from the instance base. Otherwise
> > > we end up with a large number of device tree nodes that need to be
> > > patched when new clocks are discovered in a clkctrl clock with minor
> > > hardware revision changes for example
> > >
> > >2. On omap5 CM_L3INIT_USB_HOST_HS_CLKCTRL has ten OPTFCLKEN bits. So we
> > > need to use a separate cell for optional gate clocks to avoid address
> > > space conflicts
> > >
> > >There is probably no need to list input clocks for each clkctrl clock
> > >instance in the binding. If we want to add them, the standard clocks
> > >binding can be used for that.
> > >
> > >For hardware reference, see omap4430 TRM "Table 3-1312. L4PER_CM2 Registers
> > >Mapping Summary" for example. It shows one instance of a clkctrl clock
> > >controller with multiple clkctrl registers.
> > >
> > >Cc: Paul Walmsley <paul@pwsan.com>
> > >Acked-by: Rob Herring <robh@kernel.org>
> > >Signed-off-by: Tony Lindgren <tony@atomide.com>
> >
> > This binding works for me, so:
> >
> > Acked-by: Tero Kristo <t-kristo@ti.com>
> >
>
> This wasn't in the TI clk PR. Shall I merge this to clk-next?
Tero has this included in his clkctrl series and there's one typo
fix needed where the doc still says #clock-cells = <4> instead of <2>.
I suggest let's wait a bit on this and let's try to get the clkctrl
driver parts into next early after v4.12-rc1 if no issues.
It seems the clkctrl series clock driver part is pretty much
ready to go, the remaining issues seem to be in the hwmod code.
Regards,
Tony
^ permalink raw reply
* Re: [PATCHv3] Documentation: dt-bindings: Add binding documentation for TI clkctrl clocks
From: Stephen Boyd @ 2017-04-19 16:55 UTC (permalink / raw)
To: Tero Kristo
Cc: Tony Lindgren, Michael Turquette, devicetree, linux-clk,
linux-omap, Paul Walmsley
In-Reply-To: <dca1ef84-77d1-2b58-7913-91f3b69fde6d@ti.com>
On 02/13, Tero Kristo wrote:
> On 24/01/17 00:17, Tony Lindgren wrote:
> >Texas Instruments omap variant SoCs starting with omap4 have a clkctrl
> >clock controller instance for each interconnect target module. The clkctrl
> >controls functional and interface clocks for the module.
> >
> >The clkctrl clocks are currently handled by arch/arm/mach-omap2 hwmod code.
> >With this binding and a related clock device driver we can start moving the
> >clkctrl clock handling to live in drivers/clk/ti.
> >
> >Note that this binding allows keeping the clockdomain related parts out of
> >drivers/clock. The CLKCTCTRL and DYNAMICDEP registers can be handled by
> >a separate driver in drivers/soc/ti and genpd. If the clockdomain driver
> >needs to know it's clocks, we can just set the the clkctrl device
> >instances to be children of the related clockdomain device.
> >
> >Each clkctrl clock can have multiple optional gate clocks, and multiple
> >optional mux clocks. To represent this in device tree, it seems that
> >it is best done using four clock cells #clock-cells = <2> property.
> >
> >The reasons for using #clock-cells = <2> are:
> >
> >1. We need to specify the clkctrl offset from the instance base. Otherwise
> > we end up with a large number of device tree nodes that need to be
> > patched when new clocks are discovered in a clkctrl clock with minor
> > hardware revision changes for example
> >
> >2. On omap5 CM_L3INIT_USB_HOST_HS_CLKCTRL has ten OPTFCLKEN bits. So we
> > need to use a separate cell for optional gate clocks to avoid address
> > space conflicts
> >
> >There is probably no need to list input clocks for each clkctrl clock
> >instance in the binding. If we want to add them, the standard clocks
> >binding can be used for that.
> >
> >For hardware reference, see omap4430 TRM "Table 3-1312. L4PER_CM2 Registers
> >Mapping Summary" for example. It shows one instance of a clkctrl clock
> >controller with multiple clkctrl registers.
> >
> >Cc: Paul Walmsley <paul@pwsan.com>
> >Acked-by: Rob Herring <robh@kernel.org>
> >Signed-off-by: Tony Lindgren <tony@atomide.com>
>
> This binding works for me, so:
>
> Acked-by: Tero Kristo <t-kristo@ti.com>
>
This wasn't in the TI clk PR. Shall I merge this to clk-next?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
From: Mathieu Poirier @ 2017-04-19 16:50 UTC (permalink / raw)
To: Leo Yan
Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
Catalin Marinas, Will Deacon, Andy Gross, David Brown,
Suzuki K Poulose, Stephen Boyd, linux-doc,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-arm-msm, linux-soc,
Mike Leach, Sudeep Holla
In-Reply-To: <20170419153045.GB23319@leoy-linaro>
On 19 April 2017 at 09:30, Leo Yan <leo.yan@linaro.org> wrote:
> On Wed, Apr 19, 2017 at 08:52:12AM -0600, Mathieu Poirier wrote:
>
> [...]
>
>> >> > +static bool debug_enable;
>> >> > +module_param_named(enable, debug_enable, bool, 0600);
>> >> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
>> >> > + "(default is 0, which means is disabled by default)");
>> >>
>> >> For this driver we have a debugFS interface so I question the validity of a
>> >> kernel module parameter. Other than adding complexity to the code it offers no
>> >> real added value. If a user is to insmod a module, it is just as easy to switch
>> >> on the functionality using debugFS in a second step.
>> >
>> > This module parameter can be used for kernel command line, so
>> > it's useful when user wants to dynamically turn on/off the
>> > functionality at boot up time.
>> >
>> > Does this make sense for you? Removing this parameter is okay for
>> > me, but this means users need to decide if use it by Kernel config
>> > with static building in. This is a bit contradictory with before's
>> > discussion.
>>
>> My hope was to use the kernel command line and the debugFS interface,
>> avoiding the module parameter. Look at what the baycom_par and
>> blacklist drivers are doing with the "__setup()" function and see if
>> we can void a module parameter. If not then let it be, unless someone
>> else has a better idea.
>>
>> [1]. drivers/net/hamradio/baycom_par.c
>> [2]. drivers/s390/cio/blacklist.c
>
> This driver supports module mode. So we can choose to use either module
> parameter or __setup(). But as described in the file
> Documentation/admin-guide/kernel-parameters.rst, the module parameter
> is more flexible to be used at boot time or insmod the module:
>
> "Parameters for modules which are built into the kernel need to be
> specified on the kernel command line. modprobe looks through the
> kernel command line (/proc/cmdline) and collects module parameters
> when it loads a module, so the kernel command line can be used for
> loadable modules too."
>
> __setup() cannot support module mode, and when use __setup(), we need
> register one callback function for it. The callback function is
> friendly to parse complex parameters rather than module parameter.
> but it's not necessary for this case.
__setup() definitely supports module - the baycom driver is a good
example of that.
But as you pointed out kernel-parameters.rst is pretty clear on how to
proceed. As such disregard my comment and proceed with a module
parameter.
>
> So I'm bias to use module parameter :)
>
> Thanks,
> Leo Yan
^ permalink raw reply
* Re: [PATCH v2] clk/axs10x: introduce AXS10X pll driver
From: sboyd @ 2017-04-19 16:49 UTC (permalink / raw)
To: Vlad Zakharov
Cc: mark.rutland@arm.com, linux-kernel@vger.kernel.org,
Jose.Abreu@synopsys.com, mturquette@baylibre.com,
devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
linux-snps-arc@lists.infradead.org
In-Reply-To: <1491408370.9650.24.camel@synopsys.com>
On 04/05, Vlad Zakharov wrote:
> Hi Stephen,
>
> On Tue, 2017-04-04 at 18:35 -0700, Stephen Boyd wrote:
> > > + .pll_table = (struct pll_of_table []){
> > > + {
> > > + .prate = 27000000,
> >
> > Can this be another clk in the framework instead of hardcoding
> > the parent rate?
>
> In fact there is another clk in the framework that represents this parent clock. But this field is needed to get
> appropriate pll_cfg_table as it depends on parent clock frequency. Below in pll_cfg_get function we are searching for
> the correct table comparing .parent_node field with real hardware parent clock frequency:
> ---------------------------------->8------------------------------------
> for (i = 0; pll_table[i].prate != 0; i++)
> if (pll_table[i].prate == prate)
> return pll_table[i].pll_cfg_table;
> ---------------------------------->8------------------------------------
When is that done though? During round_rate and recalc_rate the
parent frequency is passed into the function, so it should be
possible to use that if the tree is properly expressed.
>
> >
> > > + .pll_cfg_table = (struct pll_cfg []){
> > > + { 25200000, 1, 84, 90 },
> > > + { 50000000, 1, 100, 54 },
> > > + { 74250000, 1, 44, 16 },
> > > + { },
> > > + },
> > > + },
> > > + /* Used as list limiter */
> > > + { },
> >
> > There's only ever one, so I'm confused why we're making a list.
>
> By this patch we only add support of core arc pll and pgu pll and today they are clocked by the only parent clocks
> introduced here. But other plls on axs10x may be driven by different or configurable clocks, so in such cases we will
> have more than one entry in this list. And we are going to add more supported plls to this driver in the nearest future.
Ok.
>
> > > +
> > > + clk = clk_register(NULL, &pll_clk->hw);
> > > + if (IS_ERR(clk)) {
> > > + pr_err("failed to register %s clock (%ld)\n",
> > > + node->name, PTR_ERR(clk));
> > > + kfree(pll_clk);
> > > + return;
> > > + }
> > > +
> > > + of_clk_add_provider(node, of_clk_src_simple_get, clk);
> >
> > Can you please use the clk_hw based provider and clk registration
> > functions?
>
> Sure. Could you be so kind to explain what is the difference between hw and non-hw based provider and clk registration
> functions please? In which cases they are preferred?
>
We're trying to split the consumer and provider APIs along struct
clk_hw and struct clk respectively. If we can have drivers only
registers clk_hw pointers and never get back anything but an
error code, then we can force consumers to always go through the
clk_get() family of APIs. Then we can easily tell who is a
provider, who is a consumer, and who is a provider + a consumer.
Right now this isn't always clear cut because clk_hw has access
to struct clk, and also clk_register() returns a clk pointer, but
it doesn't really get used by anything in a provider driver,
unless provider drivers are doing something with the consumer
API.
> >
> > > +}
> > > +
> > > +CLK_OF_DECLARE(axs10x_pll_clock, "snps,axs10x-arc-pll-clock", of_pll_clk_setup);
> >
> > Does this need to be CLK_OF_DECLARE_DRIVER? I mean does the
> > driver need to probe and also have this of declare happen? Is the
> > PLL special and needs to be used for the timers?
>
> It is special and is used for the timers, so we have to CLK_OF_DECLARE it. On the other hand similar pll is used to
> drive PGU clock frequency and other subsystems and so we add usual probe func.
>
Presumably we'll have different compatible strings for the
different PLLs then? CLK_OF_DECLARE() will make it so that the
device node that matches never gets a ->probe() from a
platform_driver called on it. If you want it to be called twice,
then you need to use CLK_OF_DECLARE_DRIVER() instead.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2 1/2] clk: imx7d: fix USDHC NAND clock
From: Stephen Boyd @ 2017-04-19 16:44 UTC (permalink / raw)
To: Stefan Agner
Cc: shawnguo-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
aisheng.dong-3arQi8VN3Tc, fabio.estevam-3arQi8VN3Tc,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <9f0b9e83bc2a46bacb323d3ff2cfcf64-XLVq0VzYD2Y@public.gmane.org>
On 04/19, Stefan Agner wrote:
> On 2017-04-19 09:14, Stephen Boyd wrote:
> > On 04/10, Stefan Agner wrote:
> >> The USDHC NAND root clock is not gated by any CCM clock gate. Remove
> >> the bogus gate definition.
> >>
> >> Signed-off-by: Stefan Agner <stefan-XLVq0VzYD2Y@public.gmane.org>
> >> ---
> >
> > Can this be applied? It's followed by a dtsi change and there is
> > zero information about if the two depend on each other. Please
> > add cover letters for these sorts of things in the future
> > indicating how you expect merging to work.
>
> This can be merged. The two changes are independent.
>
> They look kind of dependent, but really aren't since
> IMX7D_NAND_USDHC_BUS_ROOT_CLK is still in the init_on list. Should have
> explicitly mentioned that, sorry about that.
>
Thanks. Applied to clk-next.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT
From: Florian Fainelli @ 2017-04-19 16:43 UTC (permalink / raw)
To: Rafał Miłecki, Florian Fainelli,
Rafał Miłecki
Cc: Hauke Mehrtens, Rob Herring, Mark Rutland, Russell King,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <c3a0455a-821f-d8a8-bf8a-7ad06ad0466e-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
On 04/02/2017 02:25 PM, Rafał Miłecki wrote:
> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>> Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>
>>> Northstar devices have MDIO bus that may contain various PHYs attached.
>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).
>>>
>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>> ---
>>> arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>> index acee36a61004..6a2afe7880ae 100644
>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>> @@ -320,6 +320,13 @@
>>> };
>>> };
>>>
>>> + mdio@18003000 {
>>> + compatible = "brcm,iproc-mdio";
>>> + reg = <0x18003000 0x8>;
>>> + #size-cells = <1>;
>>> + #address-cells = <0>;
>>> + };
>>
>> This looks fine, but usually the block should be enabled on a per-board
>> basis, such that there should be a status = "disabled" property here by
>> default.
>
> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
> it's
> for stuff that is always present on every SoC family board: rng, nand,
> spi to
> name few.
>
> It makes some sense, consider e.g. spi. Every Northstar board has SPI
> controller so it's enabled by default. Not every board has SPI flash, so
> it's
> disabled by default.
>
> It's there and it make sense to me. Is that OK or not?
Even though there are devices that are always enabled on a given SoC,
because the board designs are always consistent does not necessarily
make them good candidates to be enabled at the .dtsi level. This is
particularly true when there are external connections to blocks (SPI,
NAND, USB, Ethernet, MDIO to name a few), having them disabled by
default is safer as a starting point to begin with.
>
> I find MDIO situation quite simiar. It seems every Northstar board has
> MDIO bus
> just devices may differ and should not be enabled by default.
In which case, the only difference, for you would be to do to, at the
board-level DTS:
&mdio {
status = "okay";
phy@0 {
reg = <0>;
...
};
};
versus:
&mdio {
phy@0 {
reg = <0>;
...
};
};
I think we can afford putting the mdio node's status property in each
board-level DTS and make it clear that way that it is enabled because
there are child nodes enabled?
NB: with a CONFIG_OF system, there is no automatic probing of MDIO child
devices because it relies on child nodes being declared, but you would
still get the driver to be probed and enabled, which is a waste of
resources at best.
Thanks
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC 2/2] mux: mmio-based syscon mux controller
From: Peter Rosin @ 2017-04-19 16:42 UTC (permalink / raw)
To: Philipp Zabel, Steve Longerbeam
Cc: Rob Herring, Mark Rutland, Sakari Ailus,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <1492619554.2970.176.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On 2017-04-19 18:32, Philipp Zabel wrote:
> On Wed, 2017-04-19 at 09:23 -0700, Steve Longerbeam wrote:
>>
>> On 04/19/2017 08:27 AM, Philipp Zabel wrote:
>>> On Wed, 2017-04-19 at 13:58 +0200, Peter Rosin wrote:
>>>> On 2017-04-19 13:50, Philipp Zabel wrote:
>>>>> On Thu, 2017-04-13 at 18:09 -0700, Steve Longerbeam wrote:
>>>>>>
>>>>>> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
>>>>>>> This adds a driver for mmio-based syscon multiplexers controlled by a
>>>>>>> single bitfield in a syscon register range.
>>>>>>>
>>>>>>> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>>>>> ---
>>>>>>> drivers/mux/Kconfig | 13 +++++
>>>>>>> drivers/mux/Makefile | 1 +
>>>>>>> drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>> 3 files changed, 144 insertions(+)
>>>>>>> create mode 100644 drivers/mux/mux-syscon.c
>>>>>>>
>>>>>>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
>>>>>>> index 86668b4d2fc52..a5e6a3b01ac24 100644
>>>>>>> --- a/drivers/mux/Kconfig
>>>>>>> +++ b/drivers/mux/Kconfig
>>>>>>> @@ -43,4 +43,17 @@ config MUX_GPIO
>>>>>>> To compile the driver as a module, choose M here: the module will
>>>>>>> be called mux-gpio.
>>>>>>>
>>>>>>> +config MUX_SYSCON
>>>>>>
>>>>>> my preference would be CONFIG_MUX_MMIO.
>>>>>>
>>>>>>> + tristate "MMIO bitfield-controlled Multiplexer"
>>>>>>
>>>>>> "MMIO register bitfield-controlled Multiplexer"
>>>>>>
>>>>>> The rest looks good to me.
>>>>>
>>>>> I'll change those. mux-syscon.c should probably be renamed to
>>>>> mux-mmio.c, too.
>>>>
>>>> I think I disagree. But I'm not familiar with syscon so I don't know.
>>>> IIUC, syscon uses regmap to do mmio and this driver requires syscon
>>>> to get at the regmap, and in the end this driver doesn't know anything
>>>> about mmio. All it knows is syscon/regmap.
>>>
>>> That is a good point. Right now there is nothing MMIO about the driver
>>> except for the hardware that I want it to handle.
>>>
>>>> If some warped syscon
>>>> thing shows up that wraps something other than mmio in its regmap,
>>>> this driver wouldn't care about it. And syscon is something that
>>>> is also known in the DT world. Given that, I think everything in this
>>>> driver should be named syscon and not mmio.
>>>>
>>
>> My argument against using the name "syscon" in the device tree is that
>> it is referring to a subsystem in the Linux kernel. Besides the fact
>> that "syscon" does not clearly describe, at least to me, what sort of
>> device this mux is.
>
> If I'm not mistaken, this point was not about the DT compatible
> property, just about the driver name.
>
> I'm also in favor of keeping the "syscon" name out of the device tree as
> far as it is still possible, for the same reasons. The i.MX6 muxes are
> MMIO register bitfield muxes, but not "syscon muxes".
The corresponding mux in the i2c-mux "sub-sub-system" is named i2c-mux-reg.
It is about "raw" mmio, i.e. w/o syscon and regmap. Just for the record...
Cheers,
peda
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 02/16] mfd: madera: Add common support for Cirrus Logic Madera codecs
From: Richard Fitzgerald @ 2017-04-19 16:42 UTC (permalink / raw)
To: Lee Jones
Cc: gnurou, alsa-devel, jason, devicetree, linus.walleij, patches,
linux-kernel, linux-gpio, robh+dt, broonie, tglx
In-Reply-To: <20170412125455.qi6rilfy2bffbxtp@dell>
On Wed, 2017-04-12 at 13:54 +0100, Lee Jones wrote:
> On Wed, 05 Apr 2017, Richard Fitzgerald wrote:
>
> > This adds the generic core support for Cirrus Logic "Madera" class codecs.
> > These are complex audio codec SoCs with a variety of digital and analogue
> > I/O, onboard audio processing and DSPs, and other features.
> >
> > These codecs are all based off a common set of hardware IP so can be
> > supported by a core of common code (with a few minor device-to-device
> > variations).
> >
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > Signed-off-by: Nikesh Oswal <Nikesh.Oswal@wolfsonmicro.com>
> > Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> > ---
> > Documentation/devicetree/bindings/mfd/madera.txt | 79 +++
> > MAINTAINERS | 3 +
> > drivers/mfd/Kconfig | 23 +
> > drivers/mfd/Makefile | 4 +
> > drivers/mfd/madera-core.c | 689 +++++++++++++++++++++++
> > drivers/mfd/madera-i2c.c | 130 +++++
> > drivers/mfd/madera-spi.c | 131 +++++
> > drivers/mfd/madera.h | 52 ++
> > include/linux/mfd/madera/core.h | 175 ++++++
> > include/linux/mfd/madera/pdata.h | 88 +++
> > 10 files changed, 1374 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mfd/madera.txt
> > create mode 100644 drivers/mfd/madera-core.c
> > create mode 100644 drivers/mfd/madera-i2c.c
> > create mode 100644 drivers/mfd/madera-spi.c
> > create mode 100644 drivers/mfd/madera.h
> > create mode 100644 include/linux/mfd/madera/core.h
> > create mode 100644 include/linux/mfd/madera/pdata.h
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/madera.txt b/Documentation/devicetree/bindings/mfd/madera.txt
> > new file mode 100644
> > index 0000000..a6c3260
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/madera.txt
> > @@ -0,0 +1,79 @@
> > +Cirrus Logic Madera class audio codecs multi-function device
> > +
> > +These devices are audio SoCs with extensive digital capabilities and a range
> > +of analogue I/O.
> > +
> > +See also the child driver bindings in:
> > +bindings/extcon/extcon-madera.txt
> > +bindings/gpio/gpio-madera.txt
> > +bindings/interrupt-controller/cirrus,madera.txt
> > +bindings/pinctrl/cirrus,madera-pinctrl.txt
> > +bindings/regulator/madera-ldo1.txt
> > +bindings/regulator/madera-micsupp.txt
> > +bindings/sound/madera.txt
> > +
> > +Required properties:
> > +
> > + - compatible : One of the following chip-specific strings:
> > + "cirrus,cs47l35"
> > + "cirrus,cs47l85"
> > + "cirrus,cs47l90"
> > + "cirrus,cs47l91"
> > + "cirrus,wm1840"
> > +
> > + - reg : I2C slave address when connected using I2C, chip select number when
> > + using SPI.
> > +
> > + - DCVDD-supply : Power supply for the device as defined in
> > + bindings/regulator/regulator.txt
> > + Mandatory on CS47L35, CS47L90, CS47L91
> > + Optional on CS47L85, WM1840
> > +
> > + - AVDD-supply, DBVDD1-supply, DBVDD2-supply, CPVDD1-supply, CPVDD2-supply :
> > + Power supplies for the device
> > +
> > + - DBVDD3-supply, DBVDD4-supply : Power supplies for the device
> > + (CS47L85, CS47L90, CS47L91, WM1840)
> > +
> > + - SPKVDDL-supply, SPKVDDR-supply : Power supplies for the device
> > + (CS47L85, WM1840)
> > +
> > + - SPKVDD-supply : Power supply for the device
> > + (CS47L35)
> > +
> > +Optional properties:
> > +
> > + - MICVDD-supply : Power supply, only need to be specified if
> > + powered externally
> > +
> > + - reset-gpios : One entry specifying the GPIO controlling /RESET.
> > + As defined in bindings/gpio.txt.
> > + Although optional, it is strongly recommended to use a hardware reset
> > +
> > + - MICBIASx : Initial data for the MICBIAS regulators, as covered in
> > + Documentation/devicetree/bindings/regulator/regulator.txt.
> > + One for each MICBIAS generator (MICBIAS1, MICBIAS2, ...)
> > + (all codecs)
> > +
> > + One for each output pin (MICBIAS1A, MIBCIAS1B, MICBIAS2A, ...)
> > + (all except CS47L85, WM1840)
> > +
> > + The following following additional property is supported for the generator
> > + nodes:
> > + - cirrus,ext-cap : Set to 1 if the MICBIAS has external decoupling
> > + capacitors attached.
> > +
> > +Example:
> > +
> > +codec: cs47l85@0 {
>
> Node names should be generic.
>
> You can swap these round if you want, so:
>
> cs47l85: codec@0 {
>
> ... is valid.
>
> > + compatible = "cirrus,cs47l85";
> > + reg = <0>;
> > +
> > + reset-gpios = <&gpio 0>;
> > +
> > + MICBIAS1 {
> > + regulator-min-microvolt = <900000>;
> > + regulator-max-microvolt = <3300000>;
> > + cirrus,ext-cap = <1>;
> > + };
> > +};
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 02995c9..d28e53f 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3266,7 +3266,10 @@ L: patches@opensource.wolfsonmicro.com
> > T: git https://github.com/CirrusLogic/linux-drivers.git
> > W: https://github.com/CirrusLogic/linux-drivers/wiki
> > S: Supported
> > +F: Documentation/devicetree/bindings/mfd/madera.txt
> > F: include/linux/mfd/madera/*
> > +F: drivers/mfd/madera*
> > +F: drivers/mfd/cs47l*
> >
> > CLEANCACHE API
> > M: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index ce3a918..f0f9979 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -203,6 +203,29 @@ config MFD_CROS_EC_SPI
> > response time cannot be guaranteed, we support ignoring
> > 'pre-amble' bytes before the response actually starts.
> >
> > +config MFD_MADERA
> > + bool
> > + select REGMAP
> > + select MFD_CORE
> > +
> > +config MFD_MADERA_I2C
> > + tristate "Cirrus Logic Madera codecs with I2C"
> > + select MFD_MADERA
> > + select REGMAP_I2C
> > + depends on I2C
> > + help
> > + Support for the Cirrus Logic Madera platform audio SoC
> > + core functionality controlled via I2C.
> > +
> > +config MFD_MADERA_SPI
> > + tristate "Cirrus Logic Madera codecs with SPI"
> > + select MFD_MADERA
> > + select REGMAP_SPI
> > + depends on SPI_MASTER
> > + help
> > + Support for the Cirrus Logic Madera platform audio SoC
> > + core functionality controlled via SPI.
> > +
> > config MFD_ASIC3
> > bool "Compaq ASIC3"
> > depends on GPIOLIB && ARM
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index fa86dbe..c41f6c9 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -72,6 +72,10 @@ obj-$(CONFIG_MFD_WM8350_I2C) += wm8350-i2c.o
> > wm8994-objs := wm8994-core.o wm8994-irq.o wm8994-regmap.o
> > obj-$(CONFIG_MFD_WM8994) += wm8994.o
> >
> > +obj-$(CONFIG_MFD_MADERA) += madera-core.o
> > +obj-$(CONFIG_MFD_MADERA_I2C) += madera-i2c.o
> > +obj-$(CONFIG_MFD_MADERA_SPI) += madera-spi.o
> > +
> > obj-$(CONFIG_TPS6105X) += tps6105x.o
> > obj-$(CONFIG_TPS65010) += tps65010.o
> > obj-$(CONFIG_TPS6507X) += tps6507x.o
> > diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
> > new file mode 100644
> > index 0000000..ab5fe9b
> > --- /dev/null
> > +++ b/drivers/mfd/madera-core.c
> > @@ -0,0 +1,689 @@
> > +/*
> > + * Core MFD support for Cirrus Logic Madera codecs
> > + *
> > + * Copyright 2015-2017 Cirrus Logic
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/notifier.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/regulator/machine.h>
> > +#include <linux/regulator/of_regulator.h>
> > +
> > +#include <linux/mfd/madera/core.h>
> > +#include <linux/mfd/madera/registers.h>
> > +
> > +#include "madera.h"
> > +
> > +#define CS47L35_SILICON_ID 0x6360
> > +#define CS47L85_SILICON_ID 0x6338
> > +#define CS47L90_SILICON_ID 0x6364
> > +
> > +#define MADERA_32KZ_MCLK2 1
> > +
> > +static const char * const madera_core_supplies[] = {
> > + "AVDD",
> > + "DBVDD1",
> > +};
> > +
> > +static const struct mfd_cell madera_ldo1_devs[] = {
> > + { .name = "madera-ldo1", .of_compatible = "cirrus,madera-ldo1" },
> > +};
> > +
> > +static const struct mfd_cell cs47l35_devs[] = {
> > + { .name = "madera-pinctrl", .of_compatible = "cirrus,madera-pinctrl" },
> > + { .name = "madera-irq", },
>
> I believe this should be "interrupt-controller".
>
I don't think that's the case. I checked other irchip drivers and they
have no particular standard naming. At least one is called
"something-irq", none are called "something-interrupt-controller"
> irq is ambiguous.
>
I can't really see what other driver this could be confused with,
especially as it's specified as madera-* so the alternative drivers are
limited. Given the limited set of drivers I think it's clear enough it's
the driver for the IRQ and a longer name doesn't add information.
> Same goes for the ones below.
>
Ditto. madera-micsupp will be replaced by the existing arizona-micsupp.
Most gpio drivers are called "something-gpio". Extcon is the name of a
driver subsystem so should be sufficiently clear. Likewise madera-codec.
> > + { .name = "madera-micsupp", .of_compatible = "cirrus,madera-micsupp" },
> > + { .name = "madera-gpio", .of_compatible = "cirrus,madera-gpio" },
> > + { .name = "madera-extcon", .of_compatible = "cirrus,madera-extcon" },
> > + { .name = "cs47l35-codec", .of_compatible = "cirrus,cs47l35-codec" },
> > + { .name = "madera-haptics", .of_compatible = "cirrus,madera-haptics" },
> > +};
> > +
> > +static const struct mfd_cell cs47l85_devs[] = {
> > + { .name = "madera-pinctrl", .of_compatible = "cirrus,madera-pinctrl" },
> > + { .name = "madera-irq", },
> > + { .name = "madera-micsupp", .of_compatible = "cirrus,madera-micsupp" },
> > + { .name = "madera-gpio", .of_compatible = "cirrus,madera-gpio" },
> > + { .name = "madera-extcon", .of_compatible = "cirrus,madera-extcon" },
> > + { .name = "cs47l85-codec", .of_compatible = "cirrus,cs47l85-codec" },
> > + { .name = "madera-haptics", .of_compatible = "cirrus,madera-haptics" },
> > +};
> > +
> > +static const struct mfd_cell cs47l90_devs[] = {
> > + { .name = "madera-pinctrl", .of_compatible = "cirrus,madera-pinctrl" },
> > + { .name = "madera-irq", },
> > + { .name = "madera-micsupp", .of_compatible = "cirrus,madera-micsupp" },
> > + { .name = "madera-gpio", .of_compatible = "cirrus,madera-gpio" },
> > + { .name = "madera-extcon", .of_compatible = "cirrus,madera-extcon" },
> > + { .name = "cs47l90-codec", .of_compatible = "cirrus,cs47l90-codec" },
> > + { .name = "madera-haptics", .of_compatible = "cirrus,madera-haptics" },
> > +};
> > +
> > +const char *madera_name_from_type(enum madera_type type)
> > +{
> > + switch (type) {
> > + case CS47L35:
> > + return "CS47L35";
> > + case CS47L85:
> > + return "CS47L85";
> > + case CS47L90:
> > + return "CS47L90";
> > + case CS47L91:
> > + return "CS47L91";
> > + case WM1840:
> > + return "WM1840";
> > + default:
> > + return "Unknown";
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(madera_name_from_type);
> > +
> > +#define MADERA_BOOT_POLL_MAX_INTERVAL_US 5000
> > +#define MADERA_BOOT_POLL_TIMEOUT_US 25000
> > +
> > +static int madera_wait_for_boot(struct madera *madera)
> > +{
> > + unsigned int val;
> > + int ret;
> > +
> > + /*
> > + * We can't use an interrupt as we need to runtime resume to do so,
> > + * so we poll the status bit. This won't race with the interrupt
> > + * handler because it will be blocked on runtime resume.
> > + */
> > + ret = regmap_read_poll_timeout(madera->regmap,
> > + MADERA_IRQ1_RAW_STATUS_1,
> > + val,
> > + (val & MADERA_BOOT_DONE_STS1),
> > + MADERA_BOOT_POLL_MAX_INTERVAL_US,
> > + MADERA_BOOT_POLL_TIMEOUT_US);
> > + /*
> > + * BOOT_DONE defaults to unmasked on boot so we must ack it.
> > + * Do this unconditionally to avoid interrupt storms
> > + */
> > + regmap_write(madera->regmap, MADERA_IRQ1_STATUS_1,
> > + MADERA_BOOT_DONE_EINT1);
> > +
> > + if (ret)
> > + dev_err(madera->dev, "Polling BOOT_DONE_STS failed: %d\n", ret);
>
> Why isn't this under the call to regmap_read_poll_timeout()?
>
It was intended to make it clear that we must ack the BOOT_DONE now no
matter what and to avoid the potential with them the other way around of
someone adding more code in the if (or just a ret) and so accidentally
failing to do the ack. I could swap them but I think I prefer keeping
them this way and changing the comment to say this.
> > + pm_runtime_mark_last_busy(madera->dev);
> > +
> > + return ret;
> > +}
> > +
> > +static int madera_soft_reset(struct madera *madera)
> > +{
> > + int ret;
> > +
> > + ret = regmap_write(madera->regmap, MADERA_SOFTWARE_RESET, 0);
> > + if (ret != 0) {
> > + dev_err(madera->dev, "Failed to soft reset device: %d\n", ret);
> > + return ret;
> > + }
> > + usleep_range(1000, 2000);
>
> Why have you chosen 1000 => 2000?
>
> If you obtained specific information from the datasheet, please quote
> it in the comment here.
>
> > + return 0;
> > +}
> > +
> > +static void madera_enable_hard_reset(struct madera *madera)
> > +{
> > + if (madera->reset_gpio)
> > + gpiod_set_value_cansleep(madera->reset_gpio, 0);
> > +}
> > +
> > +static void madera_disable_hard_reset(struct madera *madera)
> > +{
> > + if (madera->reset_gpio) {
> > + gpiod_set_value_cansleep(madera->reset_gpio, 1);
> > + usleep_range(1000, 2000);
> > + }
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int madera_runtime_resume(struct device *dev)
> > +{
> > + struct madera *madera = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + dev_dbg(madera->dev, "Leaving sleep mode\n");
>
> Nit: Less code to just use 'dev', no?
>
> > + ret = regulator_enable(madera->dcvdd);
> > + if (ret) {
> > + dev_err(madera->dev, "Failed to enable DCVDD: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + regcache_cache_only(madera->regmap, false);
> > + regcache_cache_only(madera->regmap_32bit, false);
> > +
> > + ret = madera_wait_for_boot(madera);
> > + if (ret)
> > + goto err;
> > +
> > + ret = regcache_sync(madera->regmap);
> > + if (ret) {
> > + dev_err(madera->dev,
> > + "Failed to restore 16-bit register cache\n");
> > + goto err;
> > + }
> > +
> > + ret = regcache_sync(madera->regmap_32bit);
> > + if (ret) {
> > + dev_err(madera->dev,
> > + "Failed to restore 32-bit register cache\n");
> > + goto err;
> > + }
> > +
> > + return 0;
> > +
> > +err:
> > + regcache_cache_only(madera->regmap_32bit, true);
> > + regcache_cache_only(madera->regmap, true);
> > + regulator_disable(madera->dcvdd);
> > +
> > + return ret;
> > +}
> > +
> > +static int madera_runtime_suspend(struct device *dev)
> > +{
> > + struct madera *madera = dev_get_drvdata(dev);
> > +
> > + dev_dbg(madera->dev, "Entering sleep mode\n");
> > +
> > + regcache_cache_only(madera->regmap, true);
> > + regcache_mark_dirty(madera->regmap);
> > + regcache_cache_only(madera->regmap_32bit, true);
> > + regcache_mark_dirty(madera->regmap_32bit);
> > +
> > + regulator_disable(madera->dcvdd);
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +const struct dev_pm_ops madera_pm_ops = {
> > + SET_RUNTIME_PM_OPS(madera_runtime_suspend,
> > + madera_runtime_resume,
> > + NULL)
> > +};
> > +EXPORT_SYMBOL_GPL(madera_pm_ops);
> > +
> > +unsigned int madera_get_num_micbias(struct madera *madera)
> > +{
> > +
> > + switch (madera->type) {
> > + case CS47L35:
> > + return 2;
> > + case CS47L85:
> > + case WM1840:
> > + return 4;
> > + case CS47L90:
> > + case CS47L91:
> > + return 2;
> > + default:
> > + dev_warn(madera->dev, "No micbias known for codec %s\n",
> > + madera_name_from_type(madera->type));
> > + return 0;
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(madera_get_num_micbias);
>
> Looks very subsystem specific. Where is this called from?
>
Child drivers will use this. It's a global property of the chip, not
specific to one particular subsystem, so I don't see a better place than
the parent MFD.
> > +unsigned int madera_get_num_childbias(struct madera *madera,
> > + unsigned int micbias)
> > +{
> > + /*
> > + * micbias argument reserved for future codecs that don't
> > + * have the same number of children on each micbias
> > + */
> > +
> > + switch (madera->type) {
> > + case CS47L35:
> > + return 2;
> > + case CS47L85:
> > + case WM1840:
> > + return 0;
> > + case CS47L90:
> > + case CS47L91:
> > + return 4;
> > + default:
> > + dev_warn(madera->dev, "No child micbias known for codec %s\n",
> > + madera_name_from_type(madera->type));
> > + return 0;
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(madera_get_num_childbias);
>
> As above.
>
Reason as above
> > +#ifdef CONFIG_OF
> > +const struct of_device_id madera_of_match[] = {
> > + { .compatible = "cirrus,cs47l35", .data = (void *)CS47L35 },
> > + { .compatible = "cirrus,cs47l85", .data = (void *)CS47L85 },
> > + { .compatible = "cirrus,cs47l90", .data = (void *)CS47L90 },
> > + { .compatible = "cirrus,cs47l91", .data = (void *)CS47L91 },
> > + { .compatible = "cirrus,wm1840", .data = (void *)WM1840 },
> > + {},
> > +};
> > +EXPORT_SYMBOL_GPL(madera_of_match);
> > +
> > +unsigned long madera_of_get_type(struct device *dev)
> > +{
> > + const struct of_device_id *id = of_match_device(madera_of_match, dev);
> > +
> > + if (id)
> > + return (unsigned long)id->data;
> > + else
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(madera_of_get_type);
> > +#endif
> > +
> > +static int madera_prop_get_core_pdata(struct madera *madera)
>
> Will this ever do more than obtain a GPIO?
>
> If not, please consider renaming the function.
>
> > +{
> > + int ret;
> > +
> > + madera->reset_gpio = devm_gpiod_get_optional(madera->dev,
> > + "reset",
> > + GPIOD_OUT_LOW);
> > + if (IS_ERR(madera->reset_gpio)) {
> > + ret = PTR_ERR(madera->reset_gpio);
> > +
> > + if (ret == -EPROBE_DEFER)
> > + return ret;
> > + else if ((ret < 0) && (ret != -EINVAL))
> > + dev_warn(madera->dev,
> > + "DT property reset-gpio is malformed: %d\n",
> > + ret);
> > + }
>
> This hunk looks like it could be simplified.
>
> > + return 0;
> > +}
> > +
> > +static void madera_configure_micbias(struct madera *madera)
> > +{
> > + unsigned int num_micbias = madera_get_num_micbias(madera);
> > + struct madera_micbias_pdata *pdata;
> > + struct regulator_init_data *init_data;
> > + unsigned int num_child_micbias;
> > + unsigned int val, mask, reg;
> > + int i, j, ret;
> > +
> > + for (i = 0; i < num_micbias; i++) {
> > + pdata = &madera->pdata.micbias[i];
> > + init_data = &pdata->init_data;
> > +
> > + if (!init_data->constraints.max_uV &&
> > + !init_data->constraints.valid_ops_mask)
> > + continue; /* pdata not set */
> > +
> > + /* Apply default for bypass mode */
> > + if (!init_data->constraints.max_uV)
> > + init_data->constraints.max_uV = 2800;
> > +
> > + val = (init_data->constraints.max_uV - 1500000) / 100000;
> > + val <<= MADERA_MICB1_LVL_SHIFT;
> > +
> > + mask = MADERA_MICB1_LVL_MASK | MADERA_MICB1_EXT_CAP |
> > + MADERA_MICB1_BYPASS | MADERA_MICB1_RATE;
> > +
> > + if (pdata->ext_cap)
> > + val |= MADERA_MICB1_EXT_CAP;
> > +
> > + /* if no child biases the discharge is set in the parent */
> > + num_child_micbias = madera_get_num_childbias(madera, i + 1);
> > + if (num_child_micbias == 0) {
> > + mask |= MADERA_MICB1_DISCH;
> > +
> > + switch (init_data->constraints.active_discharge) {
> > + case REGULATOR_ACTIVE_DISCHARGE_ENABLE:
> > + val |= MADERA_MICB1_DISCH;
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > +
> > + if (init_data->constraints.soft_start)
> > + val |= MADERA_MICB1_RATE;
> > +
> > + if (init_data->constraints.valid_ops_mask &
> > + REGULATOR_CHANGE_BYPASS)
> > + val |= MADERA_MICB1_BYPASS;
> > +
> > + reg = MADERA_MIC_BIAS_CTRL_1 + i;
> > + ret = regmap_update_bits(madera->regmap, reg, mask, val);
> > + if (ret)
> > + dev_warn(madera->dev, "Failed to write 0x%x (%d)\n",
> > + reg, ret);
> > +
> > + dev_dbg(madera->dev, "Set MICBIAS_CTRL_%d mask=0x%x val=0x%x\n",
> > + i + 1, mask, val);
> > +
> > + /* Configure the child micbias pins */
> > + val = 0;
> > + mask = 0;
> > + for (j = 0; j < num_child_micbias; j++) {
> > + mask |= (MADERA_MICB1A_DISCH << (j * 4));
> > +
> > + init_data = &pdata->pin[j].init_data;
> > + switch (init_data->constraints.active_discharge) {
> > + case REGULATOR_ACTIVE_DISCHARGE_ENABLE:
> > + val |= (MADERA_MICB1A_DISCH << (j * 4));
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > +
> > + if (mask) {
> > + reg = MADERA_MIC_BIAS_CTRL_5 + (i * 2);
> > + ret = regmap_update_bits(madera->regmap, reg, mask, val);
> > + if (ret)
> > + dev_warn(madera->dev,
> > + "Failed to write 0x%x (%d)\n",
> > + reg, ret);
> > +
> > + dev_dbg(madera->dev,
> > + "Set MICBIAS_CTRL_%d mask=0x%x val=0x%x\n",
> > + i + 5, mask, val);
> > + }
> > + }
> > +}
>
> This 'stuff' looks like it should be moved out to the sub-device
> drivers.
>
> > +int madera_dev_init(struct madera *madera)
> > +{
> > + struct device *dev = madera->dev;
> > + const char *name;
> > + unsigned int hwid;
> > + int (*patch_fn)(struct madera *) = NULL;
> > + const struct mfd_cell *mfd_devs;
> > + int n_devs = 0;
> > + int i, ret;
> > +
> > + dev_set_drvdata(madera->dev, madera);
> > + BLOCKING_INIT_NOTIFIER_HEAD(&madera->notifier);
> > +
> > + if (dev_get_platdata(madera->dev)) {
> > + memcpy(&madera->pdata, dev_get_platdata(madera->dev),
> > + sizeof(madera->pdata));
> > +
> > + /* We use 0 in pdata to indicate a GPIO has not been set */
> > + if (madera->pdata.reset > 0) {
> > + /* Start out with /RESET asserted */
> > + ret = devm_gpio_request_one(madera->dev,
> > + madera->pdata.reset,
> > + GPIOF_DIR_OUT | GPIOF_INIT_LOW,
> > + "madera reset");
> > + if (ret) {
> > + dev_err(dev, "Failed to request /RESET: %d\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + madera->reset_gpio = gpio_to_desc(madera->pdata.reset);
> > + }
> > + } else {
> > + ret = madera_prop_get_core_pdata(madera);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (!madera->reset_gpio)
> > + dev_warn(madera->dev,
> > + "Running without reset GPIO is not recommended\n");
>
> I suggest moving all of the above into madera_prop_get_core_pdata()
> and renaming it to madera_get_gpio().
>
> It also looks like it could be simplified to reduce indentation.
>
> > + regcache_cache_only(madera->regmap, true);
> > + regcache_cache_only(madera->regmap_32bit, true);
> > +
> > + for (i = 0; i < ARRAY_SIZE(madera_core_supplies); i++)
> > + madera->core_supplies[i].supply = madera_core_supplies[i];
> > +
> > + madera->num_core_supplies = ARRAY_SIZE(madera_core_supplies);
> > +
> > + switch (madera->type) {
> > + case CS47L35:
> > + case CS47L90:
> > + case CS47L91:
>
> Perhaps a comment here to say why these devices do not require LDO1
> devices.
>
> > + break;
> > + case CS47L85:
> > + case WM1840:
> > + ret = mfd_add_devices(madera->dev, PLATFORM_DEVID_NONE,
> > + madera_ldo1_devs,
> > + ARRAY_SIZE(madera_ldo1_devs),
> > + NULL, 0, NULL);
> > + if (ret != 0) {
>
> Please use these checks in order of preference:
>
> if (ret)
> if (ret < 0)
> if (ret != 0)
>
> ... depending on the situation.
>
> Here the former will do.
>
> > + dev_err(dev, "Failed to add LDO1 child: %d\n", ret);
> > + return ret;
> > + }
> > + break;
> > + default:
> > + dev_err(madera->dev, "Unknown device type %d\n", madera->type);
> > + return -ENODEV;
> > + }
> > +
> > + ret = devm_regulator_bulk_get(dev, madera->num_core_supplies,
> > + madera->core_supplies);
> > + if (ret) {
> > + dev_err(dev, "Failed to request core supplies: %d\n", ret);
> > + goto err_devs;
> > + }
> > +
> > + /*
> > + * Don't use devres here because the only device we have to get
> > + * against is the MFD device and DCVDD will likely be supplied by
> > + * one of its children. Meaning that the regulator will be
> > + * destroyed by the time devres calls regulator put.
> > + */
> > + madera->dcvdd = regulator_get_exclusive(madera->dev, "DCVDD");
> > + if (IS_ERR(madera->dcvdd)) {
> > + ret = PTR_ERR(madera->dcvdd);
> > + dev_err(dev, "Failed to request DCVDD: %d\n", ret);
> > + goto err_devs;
> > + }
> > +
> > + ret = regulator_bulk_enable(madera->num_core_supplies,
> > + madera->core_supplies);
> > + if (ret) {
> > + dev_err(dev, "Failed to enable core supplies: %d\n", ret);
> > + goto err_dcvdd;
> > + }
> > +
> > + ret = regulator_enable(madera->dcvdd);
> > + if (ret) {
> > + dev_err(dev, "Failed to enable DCVDD: %d\n", ret);
> > + goto err_enable;
> > + }
> > +
> > + madera_disable_hard_reset(madera);
> > +
> > + regcache_cache_only(madera->regmap, false);
> > + regcache_cache_only(madera->regmap_32bit, false);
> > +
> > + /*
> > + * Verify that this is a chip we know about before we
> > + * starting doing any writes to its registers
> > + */
> > + ret = regmap_read(madera->regmap, MADERA_SOFTWARE_RESET, &hwid);
> > + if (ret) {
> > + dev_err(dev, "Failed to read ID register: %d\n", ret);
> > + goto err_reset;
> > + }
> > +
> > + switch (hwid) {
> > + case CS47L35_SILICON_ID:
> > + case CS47L85_SILICON_ID:
> > + case CS47L90_SILICON_ID:
> > + break;
> > + default:
> > + dev_err(madera->dev, "Unknown device ID: %x\n", hwid);
> > + ret = -EINVAL;
> > + goto err_reset;
> > + }
> > +
> > + /* If we don't have a reset GPIO use a soft reset */
> > + if (!madera->reset_gpio) {
> > + ret = madera_soft_reset(madera);
> > + if (ret)
> > + goto err_reset;
> > + }
> > +
> > + ret = madera_wait_for_boot(madera);
> > + if (ret) {
> > + dev_err(madera->dev, "Device failed initial boot: %d\n", ret);
> > + goto err_reset;
> > + }
> > +
> > + ret = regmap_read(madera->regmap, MADERA_HARDWARE_REVISION,
> > + &madera->rev);
> > + if (ret) {
> > + dev_err(dev, "Failed to read revision register: %d\n", ret);
> > + goto err_reset;
> > + }
> > + madera->rev &= MADERA_HW_REVISION_MASK;
> > +
> > + name = madera_name_from_type(madera->type);
> > +
> > + switch (hwid) {
> > + case CS47L35_SILICON_ID:
> > + if (IS_ENABLED(CONFIG_MFD_CS47L35)) {
> > + switch (madera->type) {
> > + case CS47L35:
> > + patch_fn = cs47l35_patch;
> > + mfd_devs = cs47l35_devs;
> > + n_devs = ARRAY_SIZE(cs47l35_devs);
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > + break;
> > + case CS47L85_SILICON_ID:
> > + if (IS_ENABLED(CONFIG_MFD_CS47L85)) {
> > + switch (madera->type) {
> > + case CS47L85:
> > + case WM1840:
> > + patch_fn = cs47l85_patch;
> > + mfd_devs = cs47l85_devs;
> > + n_devs = ARRAY_SIZE(cs47l85_devs);
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > + break;
> > + case CS47L90_SILICON_ID:
> > + if (IS_ENABLED(CONFIG_MFD_CS47L90)) {
> > + switch (madera->type) {
> > + case CS47L90:
> > + case CS47L91:
> > + patch_fn = cs47l90_patch;
> > + mfd_devs = cs47l90_devs;
> > + n_devs = ARRAY_SIZE(cs47l90_devs);
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + if (!n_devs) {
> > + dev_err(madera->dev, "Device ID 0x%x not a %s\n", hwid, name);
> > + ret = -ENODEV;
> > + goto err_reset;
> > + }
> > +
> > + dev_info(dev, "%s silicon revision %d\n", name, madera->rev);
> > +
> > + /* Apply hardware patch */
> > + if (patch_fn) {
> > + ret = patch_fn(madera);
> > + if (ret) {
> > + dev_err(madera->dev, "Failed to apply patch %d\n", ret);
> > + goto err_reset;
> > + }
> > + }
> > +
> > + /* Init 32k clock sourced from MCLK2 */
> > + ret = regmap_update_bits(madera->regmap,
> > + MADERA_CLOCK_32K_1,
> > + MADERA_CLK_32K_ENA_MASK | MADERA_CLK_32K_SRC_MASK,
> > + MADERA_CLK_32K_ENA | MADERA_32KZ_MCLK2);
> > + if (ret) {
> > + dev_err(madera->dev, "Failed to init 32k clock: %d\n", ret);
> > + goto err_reset;
> > + }
> > +
> > + madera_configure_micbias(madera);
> > +
> > + pm_runtime_set_active(madera->dev);
> > + pm_runtime_enable(madera->dev);
> > + pm_runtime_set_autosuspend_delay(madera->dev, 100);
> > + pm_runtime_use_autosuspend(madera->dev);
> > +
> > + ret = mfd_add_devices(madera->dev, PLATFORM_DEVID_NONE,
> > + mfd_devs, n_devs,
> > + NULL, 0, NULL);
> > + if (ret) {
> > + dev_err(madera->dev, "Failed to add subdevices: %d\n", ret);
> > + goto err_pm_runtime;
> > + }
> > +
> > + return 0;
> > +
> > +err_pm_runtime:
> > + pm_runtime_disable(madera->dev);
> > +err_reset:
> > + madera_enable_hard_reset(madera);
> > + regulator_disable(madera->dcvdd);
> > +err_enable:
> > + regulator_bulk_disable(madera->num_core_supplies,
> > + madera->core_supplies);
> > +err_dcvdd:
> > + regulator_put(madera->dcvdd);
> > +err_devs:
> > + mfd_remove_devices(dev);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(madera_dev_init);
> > +
> > +int madera_dev_exit(struct madera *madera)
> > +{
> > + /* Prevent any IRQs being serviced while we clean up */
> > + disable_irq(madera->irq);
> > +
> > + /*
> > + * DCVDD could be supplied by a child node, we must disable it before
> > + * removing the children, and prevent PM runtime from turning it back on
> > + */
> > + pm_runtime_disable(madera->dev);
> > +
> > + regulator_disable(madera->dcvdd);
> > + regulator_put(madera->dcvdd);
> > +
> > + mfd_remove_devices(madera->dev);
> > + madera_enable_hard_reset(madera);
> > +
> > + regulator_bulk_disable(madera->num_core_supplies,
> > + madera->core_supplies);
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(madera_dev_exit);
> > diff --git a/drivers/mfd/madera-i2c.c b/drivers/mfd/madera-i2c.c
> > new file mode 100644
> > index 0000000..8c90780
> > --- /dev/null
> > +++ b/drivers/mfd/madera-i2c.c
> > @@ -0,0 +1,130 @@
> > +/*
> > + * I2C bus interface to Cirrus Logic Madera codecs
> > + *
> > + * Copyright 2015-2017 Cirrus Logic
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/of.h>
> > +
> > +#include <linux/mfd/madera/core.h>
> > +
> > +#include "madera.h"
> > +
> > +static int madera_i2c_probe(struct i2c_client *i2c,
> > + const struct i2c_device_id *id)
> > +{
> > + struct madera *madera;
> > + const struct regmap_config *regmap_16bit_config = NULL;
> > + const struct regmap_config *regmap_32bit_config = NULL;
> > + unsigned long type;
> > + int ret;
> > +
> > + if (i2c->dev.of_node)
> > + type = madera_of_get_type(&i2c->dev);
>
> Just call this madera_get_type()
That would be ambiguous about what type information we are getting. As
the codecs have an ID it's possible we may in the future want to use
auto-detection to override or refine the "compatible" info, the
madera->type is the ID we're going to run with so we need to make it
clear that this is the OF type info.
> and do the OF || !OF checking in
> there.
>
I tried rearranging the code like that but it just made it more complex
and less clear. I like the original way. Bear in mind that that the
alternative ID (if there's no DT) is in different structs depending
whether it's I2C or SPI so that can't be fetched by a generic function
and passing it in as an arg just obscures what's going on at this point
in the code. I think I like this original version for clarity of what's
happening, though I've renamed the function to madera_get_type_from_of.
> > + else
> > + type = id->driver_data;
> > +
> > + switch (type) {
> > + case CS47L35:
> > + if (IS_ENABLED(CONFIG_MFD_CS47L35)) {
> > + regmap_16bit_config = &cs47l35_16bit_i2c_regmap;
> > + regmap_32bit_config = &cs47l35_32bit_i2c_regmap;
> > + }
> > + break;
> > + case CS47L85:
> > + case WM1840:
> > + if (IS_ENABLED(CONFIG_MFD_CS47L85)) {
> > + regmap_16bit_config = &cs47l85_16bit_i2c_regmap;
> > + regmap_32bit_config = &cs47l85_32bit_i2c_regmap;
> > + }
> > + break;
> > + case CS47L90:
> > + case CS47L91:
> > + if (IS_ENABLED(CONFIG_MFD_CS47L90)) {
> > + regmap_16bit_config = &cs47l90_16bit_i2c_regmap;
> > + regmap_32bit_config = &cs47l90_32bit_i2c_regmap;
> > + }
> > + break;
> > + default:
> > + dev_err(&i2c->dev,
> > + "Unknown Madera I2C device type %ld\n", type);
> > + return -EINVAL;
> > + }
> > +
> > + if (!regmap_16bit_config) {
> > + dev_err(&i2c->dev,
> > + "Kernel does not include support for %s\n",
> > + madera_name_from_type(type));
> > + return -EINVAL;
> > + }
> > +
> > + madera = devm_kzalloc(&i2c->dev, sizeof(*madera), GFP_KERNEL);
> > + if (!madera)
> > + return -ENOMEM;
> > +
> > + madera->regmap = devm_regmap_init_i2c(i2c, regmap_16bit_config);
> > + if (IS_ERR(madera->regmap)) {
> > + ret = PTR_ERR(madera->regmap);
> > + dev_err(&i2c->dev,
> > + "Failed to allocate 16-bit register map: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + madera->regmap_32bit = devm_regmap_init_i2c(i2c, regmap_32bit_config);
> > + if (IS_ERR(madera->regmap_32bit)) {
> > + ret = PTR_ERR(madera->regmap_32bit);
> > + dev_err(&i2c->dev,
> > + "Failed to allocate 32-bit register map: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + madera->type = type;
> > + madera->dev = &i2c->dev;
> > + madera->irq = i2c->irq;
> > +
> > + return madera_dev_init(madera);
> > +}
> > +
> > +static int madera_i2c_remove(struct i2c_client *i2c)
> > +{
> > + struct madera *madera = dev_get_drvdata(&i2c->dev);
> > +
> > + madera_dev_exit(madera);
>
> Nit: \n here please.
>
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id madera_i2c_id[] = {
> > + { "cs47l35", CS47L35 },
> > + { "cs47l85", CS47L85 },
> > + { "cs47l90", CS47L90 },
> > + { "cs47l91", CS47L91 },
> > + { "wm1840", WM1840 },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, madera_i2c_id);
> > +
> > +static struct i2c_driver madera_i2c_driver = {
> > + .driver = {
> > + .name = "madera",
> > + .pm = &madera_pm_ops,
> > + .of_match_table = of_match_ptr(madera_of_match),
> > + },
> > + .probe = madera_i2c_probe,
> > + .remove = madera_i2c_remove,
> > + .id_table = madera_i2c_id,
> > +};
> > +
> > +module_i2c_driver(madera_i2c_driver);
> > +
> > +MODULE_DESCRIPTION("Madera I2C bus interface");
> > +MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.wolfsonmicro.com>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/mfd/madera-spi.c b/drivers/mfd/madera-spi.c
> > new file mode 100644
> > index 0000000..e7e13f0
> > --- /dev/null
> > +++ b/drivers/mfd/madera-spi.c
> > @@ -0,0 +1,131 @@
> > +/*
> > + * SPI bus interface to Cirrus Logic Madera codecs
> > + *
> > + * Copyright 2015-2017 Cirrus Logic
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/of.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include <linux/mfd/madera/core.h>
> > +
> > +#include "madera.h"
> > +
> > +static int madera_spi_probe(struct spi_device *spi)
> > +{
> > + const struct spi_device_id *id = spi_get_device_id(spi);
> > + struct madera *madera;
> > + const struct regmap_config *regmap_16bit_config = NULL;
> > + const struct regmap_config *regmap_32bit_config = NULL;
> > + unsigned long type;
> > + int ret;
> > +
> > + if (spi->dev.of_node)
> > + type = madera_of_get_type(&spi->dev);
> > + else
> > + type = id->driver_data;
>
> As above.
>
> > + switch (type) {
> > + case CS47L35:
> > + if (IS_ENABLED(CONFIG_MFD_CS47L35)) {
> > + regmap_16bit_config = &cs47l35_16bit_spi_regmap;
> > + regmap_32bit_config = &cs47l35_32bit_spi_regmap;
> > + }
> > + break;
> > + case CS47L85:
> > + case WM1840:
> > + if (IS_ENABLED(CONFIG_MFD_CS47L85)) {
> > + regmap_16bit_config = &cs47l85_16bit_spi_regmap;
> > + regmap_32bit_config = &cs47l85_32bit_spi_regmap;
> > + }
> > + break;
> > + case CS47L90:
> > + case CS47L91:
> > + if (IS_ENABLED(CONFIG_MFD_CS47L90)) {
> > + regmap_16bit_config = &cs47l90_16bit_spi_regmap;
> > + regmap_32bit_config = &cs47l90_32bit_spi_regmap;
> > + }
> > + break;
> > + default:
> > + dev_err(&spi->dev,
> > + "Unknown Madera SPI device type %ld\n", type);
> > + return -EINVAL;
> > + }
> > +
> > + if (!regmap_16bit_config) {
> > + dev_err(&spi->dev,
> > + "Kernel does not include support for %s\n",
> > + madera_name_from_type(type));
> > + return -EINVAL;
> > + }
> > +
> > + madera = devm_kzalloc(&spi->dev, sizeof(*madera), GFP_KERNEL);
> > + if (!madera)
> > + return -ENOMEM;
> > +
> > + madera->regmap = devm_regmap_init_spi(spi, regmap_16bit_config);
> > + if (IS_ERR(madera->regmap)) {
> > + ret = PTR_ERR(madera->regmap);
> > + dev_err(&spi->dev,
> > + "Failed to allocate 16-bit register map: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + madera->regmap_32bit = devm_regmap_init_spi(spi, regmap_32bit_config);
> > + if (IS_ERR(madera->regmap_32bit)) {
> > + ret = PTR_ERR(madera->regmap_32bit);
> > + dev_err(&spi->dev,
> > + "Failed to allocate 32-bit register map: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + madera->type = type;
> > + madera->dev = &spi->dev;
> > + madera->irq = spi->irq;
> > +
> > + return madera_dev_init(madera);
> > +}
> > +
> > +static int madera_spi_remove(struct spi_device *spi)
> > +{
> > + struct madera *madera = spi_get_drvdata(spi);
> > +
> > + madera_dev_exit(madera);
>
> As above.
>
> > + return 0;
> > +}
> > +
> > +static const struct spi_device_id madera_spi_ids[] = {
> > + { "cs47l35", CS47L35 },
> > + { "cs47l85", CS47L85 },
> > + { "cs47l90", CS47L90 },
> > + { "cs47l91", CS47L91 },
> > + { "wm1840", WM1840 },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(spi, madera_spi_ids);
> > +
> > +static struct spi_driver madera_spi_driver = {
> > + .driver = {
> > + .name = "madera",
> > + .owner = THIS_MODULE,
> > + .pm = &madera_pm_ops,
> > + .of_match_table = of_match_ptr(madera_of_match),
> > + },
> > + .probe = madera_spi_probe,
> > + .remove = madera_spi_remove,
> > + .id_table = madera_spi_ids,
> > +};
> > +
> > +module_spi_driver(madera_spi_driver);
> > +
> > +MODULE_DESCRIPTION("Madera SPI bus interface");
> > +MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.wolfsonmicro.com>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/mfd/madera.h b/drivers/mfd/madera.h
> > new file mode 100644
> > index 0000000..57f6add
> > --- /dev/null
> > +++ b/drivers/mfd/madera.h
> > @@ -0,0 +1,52 @@
> > +/*
> > + * madera.h -- MFD internals for Cirrus Logic Madera codecs
>
> Please remove the file name from this header.
>
> > + * Copyright 2015-2016 Cirrus Logic
>
> This needs updating.
>
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef MADERA_MFD_H
> > +#define MADERA_MFD_H
> > +
> > +#include <linux/pm.h>
> > +#include <linux/of.h>
>
> Alphabetical.
>
> > +struct madera;
> > +
> > +extern const struct dev_pm_ops madera_pm_ops;
> > +extern const struct of_device_id madera_of_match[];
> > +
> > +int madera_dev_init(struct madera *madera);
> > +int madera_dev_exit(struct madera *madera);
> > +
> > +#ifdef CONFIG_OF
> > +unsigned long madera_of_get_type(struct device *dev);
> > +#else
> > +static inline unsigned long madera_of_get_type(struct device *dev)
> > +{
> > + return 0;
> > +}
> > +#endif
>
> If you move to a generic get_type approach you can remove these
> lines.
>
> > +extern const struct regmap_config cs47l35_16bit_spi_regmap;
> > +extern const struct regmap_config cs47l35_32bit_spi_regmap;
> > +extern const struct regmap_config cs47l35_16bit_i2c_regmap;
> > +extern const struct regmap_config cs47l35_32bit_i2c_regmap;
> > +int cs47l35_patch(struct madera *madera);
> > +
> > +extern const struct regmap_config cs47l85_16bit_spi_regmap;
> > +extern const struct regmap_config cs47l85_32bit_spi_regmap;
> > +extern const struct regmap_config cs47l85_16bit_i2c_regmap;
> > +extern const struct regmap_config cs47l85_32bit_i2c_regmap;
> > +int cs47l85_patch(struct madera *madera);
> > +
> > +extern const struct regmap_config cs47l90_16bit_spi_regmap;
> > +extern const struct regmap_config cs47l90_32bit_spi_regmap;
> > +extern const struct regmap_config cs47l90_16bit_i2c_regmap;
> > +extern const struct regmap_config cs47l90_32bit_i2c_regmap;
> > +int cs47l90_patch(struct madera *madera);
>
> Where do these live?
>
In the other files added by the following 3 patches.
> > +#endif
> > diff --git a/include/linux/mfd/madera/core.h b/include/linux/mfd/madera/core.h
> > new file mode 100644
> > index 0000000..59b05f8
> > --- /dev/null
> > +++ b/include/linux/mfd/madera/core.h
> > @@ -0,0 +1,175 @@
> > +/*
> > + * MFD internals for Cirrus Logic Madera codecs
> > + *
> > + * Copyright 2015-2017 Cirrus Logic
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef MADERA_CORE_H
> > +#define MADERA_CORE_H
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/regmap.h>
> > +#include <linux/notifier.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/mfd/madera/pdata.h>
> > +#include <linux/irqchip/irq-madera.h>
> > +#include <sound/madera-pdata.h>
>
> Alphabetical.
>
> > +enum madera_type {
> > + CS47L35 = 1,
> > + CS47L85 = 2,
> > + CS47L90 = 3,
> > + CS47L91 = 4,
> > + WM1840 = 7,
> > +};
> > +
> > +#define MADERA_MAX_CORE_SUPPLIES 2
> > +#define MADERA_MAX_GPIOS 40
> > +
> > +#define CS47L35_NUM_GPIOS 16
> > +#define CS47L85_NUM_GPIOS 40
> > +#define CS47L90_NUM_GPIOS 38
> > +
> > +
> > +/* Notifier events */
> > +#define MADERA_NOTIFY_VOICE_TRIGGER 0x1
> > +#define MADERA_NOTIFY_HPDET 0x2
> > +#define MADERA_NOTIFY_MICDET 0x4
> > +
> > +/* GPIO Function Definitions */
> > +#define MADERA_GP_FN_ALTERNATE 0x00
> > +#define MADERA_GP_FN_GPIO 0x01
> > +#define MADERA_GP_FN_DSP_GPIO 0x02
> > +#define MADERA_GP_FN_IRQ1 0x03
> > +#define MADERA_GP_FN_IRQ2 0x04
> > +#define MADERA_GP_FN_FLL1_CLOCK 0x10
> > +#define MADERA_GP_FN_FLL2_CLOCK 0x11
> > +#define MADERA_GP_FN_FLL3_CLOCK 0x12
> > +#define MADERA_GP_FN_FLLAO_CLOCK 0x13
> > +#define MADERA_GP_FN_FLL1_LOCK 0x18
> > +#define MADERA_GP_FN_FLL2_LOCK 0x19
> > +#define MADERA_GP_FN_FLL3_LOCK 0x1A
> > +#define MADERA_GP_FN_FLLAO_LOCK 0x1B
> > +#define MADERA_GP_FN_OPCLK_OUT 0x40
> > +#define MADERA_GP_FN_OPCLK_ASYNC_OUT 0x41
> > +#define MADERA_GP_FN_PWM1 0x48
> > +#define MADERA_GP_FN_PWM2 0x49
> > +#define MADERA_GP_FN_SPDIF_OUT 0x4C
> > +#define MADERA_GP_FN_HEADPHONE_DET 0x50
> > +#define MADERA_GP_FN_MIC_DET 0x58
> > +#define MADERA_GP_FN_DRC1_SIGNAL_DETECT 0x80
> > +#define MADERA_GP_FN_DRC2_SIGNAL_DETECT 0x81
> > +#define MADERA_GP_FN_ASRC1_IN1_LOCK 0x88
> > +#define MADERA_GP_FN_ASRC1_IN2_LOCK 0x89
> > +#define MADERA_GP_FN_ASRC2_IN1_LOCK 0x8A
> > +#define MADERA_GP_FN_ASRC2_IN2_LOCK 0x8B
> > +#define MADERA_GP_FN_DSP_IRQ1 0xA0
> > +#define MADERA_GP_FN_DSP_IRQ2 0xA1
> > +#define MADERA_GP_FN_DSP_IRQ3 0xA2
> > +#define MADERA_GP_FN_DSP_IRQ4 0xA3
> > +#define MADERA_GP_FN_DSP_IRQ5 0xA4
> > +#define MADERA_GP_FN_DSP_IRQ6 0xA5
> > +#define MADERA_GP_FN_DSP_IRQ7 0xA6
> > +#define MADERA_GP_FN_DSP_IRQ8 0xA7
> > +#define MADERA_GP_FN_DSP_IRQ9 0xA8
> > +#define MADERA_GP_FN_DSP_IRQ10 0xA9
> > +#define MADERA_GP_FN_DSP_IRQ11 0xAA
> > +#define MADERA_GP_FN_DSP_IRQ12 0xAB
> > +#define MADERA_GP_FN_DSP_IRQ13 0xAC
> > +#define MADERA_GP_FN_DSP_IRQ14 0xAD
> > +#define MADERA_GP_FN_DSP_IRQ15 0xAE
> > +#define MADERA_GP_FN_DSP_IRQ16 0xAF
> > +#define MADERA_GP_FN_HPOUT1L_SC 0xB0
> > +#define MADERA_GP_FN_HPOUT1R_SC 0xB1
> > +#define MADERA_GP_FN_HPOUT2L_SC 0xB2
> > +#define MADERA_GP_FN_HPOUT2R_SC 0xB3
> > +#define MADERA_GP_FN_HPOUT3L_SC 0xB4
> > +#define MADERA_GP_FN_HPOUT4R_SC 0xB5
> > +#define MADERA_GP_FN_SPKOUTL_SC 0xB6
> > +#define MADERA_GP_FN_SPKOUTR_SC 0xB7
> > +#define MADERA_GP_FN_HPOUT1L_ENA 0xC0
> > +#define MADERA_GP_FN_HPOUT1R_ENA 0xC1
> > +#define MADERA_GP_FN_HPOUT2L_ENA 0xC2
> > +#define MADERA_GP_FN_HPOUT2R_ENA 0xC3
> > +#define MADERA_GP_FN_HPOUT3L_ENA 0xC4
> > +#define MADERA_GP_FN_HPOUT4R_ENA 0xC5
> > +#define MADERA_GP_FN_SPKOUTL_ENA 0xC6
> > +#define MADERA_GP_FN_SPKOUTR_ENA 0xC7
> > +#define MADERA_GP_FN_HPOUT1L_DIS 0xD0
> > +#define MADERA_GP_FN_HPOUT1R_DIS 0xD1
> > +#define MADERA_GP_FN_HPOUT2L_DIS 0xD2
> > +#define MADERA_GP_FN_HPOUT2R_DIS 0xD3
> > +#define MADERA_GP_FN_HPOUT3L_DIS 0xD4
> > +#define MADERA_GP_FN_HPOUT4R_DIS 0xD5
> > +#define MADERA_GP_FN_SPKOUTL_DIS 0xD6
> > +#define MADERA_GP_FN_SPKOUTR_DIS 0xD7
> > +#define MADERA_GP_FN_SPK_SHUTDOWN 0xE0
> > +#define MADERA_GP_FN_SPK_OVH_SHUTDOWN 0xE1
> > +#define MADERA_GP_FN_SPK_OVH_WARN 0xE2
> > +#define MADERA_GP_FN_TIMER1_STATUS 0x140
> > +#define MADERA_GP_FN_TIMER2_STATUS 0x141
> > +#define MADERA_GP_FN_TIMER3_STATUS 0x142
> > +#define MADERA_GP_FN_TIMER4_STATUS 0x143
> > +#define MADERA_GP_FN_TIMER5_STATUS 0x144
> > +#define MADERA_GP_FN_TIMER6_STATUS 0x145
> > +#define MADERA_GP_FN_TIMER7_STATUS 0x146
> > +#define MADERA_GP_FN_TIMER8_STATUS 0x147
> > +#define MADERA_GP_FN_EVENTLOG1_FIFO_STS 0x150
> > +#define MADERA_GP_FN_EVENTLOG2_FIFO_STS 0x151
> > +#define MADERA_GP_FN_EVENTLOG3_FIFO_STS 0x152
> > +#define MADERA_GP_FN_EVENTLOG4_FIFO_STS 0x153
> > +#define MADERA_GP_FN_EVENTLOG5_FIFO_STS 0x154
> > +#define MADERA_GP_FN_EVENTLOG6_FIFO_STS 0x155
> > +#define MADERA_GP_FN_EVENTLOG7_FIFO_STS 0x156
> > +#define MADERA_GP_FN_EVENTLOG8_FIFO_STS 0x157
> > +
> > +struct snd_soc_dapm_context;
> > +
> > +struct madera {
> > + struct regmap *regmap;
> > + struct regmap *regmap_32bit;
> > +
> > + struct device *dev;
> > +
> > + enum madera_type type;
> > + unsigned int rev;
> > +
> > + struct gpio_desc *reset_gpio;
> > +
> > + int num_core_supplies;
> > + struct regulator_bulk_data core_supplies[MADERA_MAX_CORE_SUPPLIES];
> > + struct regulator *dcvdd;
> > + bool internal_dcvdd;
> > +
> > + struct madera_pdata pdata;
> > +
> > + struct device *irq_dev;
> > + int irq;
> > +
> > + unsigned int out_clamp[MADERA_MAX_OUTPUT];
> > + unsigned int out_shorted[MADERA_MAX_OUTPUT];
> > + unsigned int hp_ena;
> > +
> > + struct snd_soc_dapm_context *dapm;
> > +
> > + struct blocking_notifier_head notifier;
> > +};
>
> Please supply a kerneldoc header for this struct.
>
It's internal to the madera drivers, only for use by the child drivers
so I don't think a kerneldoc makes sense but I can add a comment to
state that it's internal
> > +unsigned int madera_get_num_micbias(struct madera *madera);
> > +unsigned int madera_get_num_childbias(struct madera *madera,
> > + unsigned int micbias);
> > +
> > +const char *madera_name_from_type(enum madera_type type);
> > +
> > +static inline int madera_call_notifiers(struct madera *madera,
> > + unsigned long event,
> > + void *data)
> > +{
> > + return blocking_notifier_call_chain(&madera->notifier, event, data);
> > +}
> > +#endif
> > diff --git a/include/linux/mfd/madera/pdata.h b/include/linux/mfd/madera/pdata.h
> > new file mode 100644
> > index 0000000..6d930aa
> > --- /dev/null
> > +++ b/include/linux/mfd/madera/pdata.h
> > @@ -0,0 +1,88 @@
> > +/*
> > + * Platform data for Cirrus Logic Madera codecs
> > + *
> > + * Copyright 2015-2017 Cirrus Logic
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef MADERA_PDATA_H
> > +#define MADERA_PDATA_H
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/regulator/machine.h>
> > +
>
> Why the '\n'?
>
> > +#include <linux/regulator/madera-ldo1.h>
> > +#include <linux/regulator/madera-micsupp.h>
> > +#include <linux/irqchip/irq-madera-pdata.h>
> > +#include <sound/madera-pdata.h>
>
> Alphabetical
>
> > +#define MADERA_MAX_MICBIAS 4
> > +#define MADERA_MAX_CHILD_MICBIAS 4
> > +
> > +#define MADERA_MAX_GPSW 2
> > +
> > +struct pinctrl_map;
> > +
> > +/** MICBIAS pin configuration */
>
> Kerneldoc comment with no kerneldoc ??
>
> Same as below.
>
> > +struct madera_micbias_pin_pdata {
> > + /** Regulator configuration for pin switch */
>
> Just use Kerneldoc instead.
>
> Same for all of these structs.
>
> > + struct regulator_init_data init_data;
> > +};
> > +
> > +/** Regulator configuration for an on-chip MICBIAS */
> > +struct madera_micbias_pdata {
> > + /** Configuration of the MICBIAS generator */
> > + struct regulator_init_data init_data;
> > +
> > + bool ext_cap; /** External capacitor fitted */
> > +
> > + /**
> > + * Configuration for each output pin from this MICBIAS
> > + * (Not used on CS47L85 and WM1840)
> > + */
> > + struct madera_micbias_pin_pdata pin[MADERA_MAX_CHILD_MICBIAS];
> > +};
> > +
> > +struct madera_pdata {
> > + /** GPIO controlling /RESET, if any */
> > + int reset;
> > +
> > + /** Substruct of pdata for the LDO1 regulator */
> > + struct madera_ldo1_pdata ldo1;
> > +
> > + /** Substruct of pdata for the MICSUPP regulator */
> > + struct madera_micsupp_pdata micsupp;
> > +
> > + /** Substruct of pdata for the irqchip driver */
> > + struct madera_irqchip_pdata irqchip;
> > +
> > + /** Base GPIO */
> > + int gpio_base;
> > +
> > + /**
> > + * Array of GPIO configurations
> > + * See Documentation/pinctrl.txt
> > + */
> > + const struct pinctrl_map *gpio_configs;
> > + int n_gpio_configs;
> > +
> > + /** MICBIAS configurations */
> > + struct madera_micbias_pdata micbias[MADERA_MAX_MICBIAS];
> > +
> > + /**
> > + * Substructure of pdata for the ASoC codec driver
> > + * See include/sound/madera-pdata.h
> > + */
> > + struct madera_codec_pdata codec;
> > +
> > + /**
> > + * General purpose switch mode setting
> > + * See the SW1_MODE field in the datasheet for the available values
> > + */
> > + u32 gpsw[MADERA_MAX_GPSW];
> > +};
> > +
> > +#endif
>
^ permalink raw reply
* Re: [PATCH v13 02/10] dt-bindings: document devicetree bindings for mux-controllers and gpio-mux
From: Philipp Zabel @ 2017-04-19 16:34 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Greg Kroah-Hartman, Mark Rutland, devicetree,
Lars-Peter Clausen, Wolfram Sang, linux-iio, Jonathan Corbet,
linux-doc, kernel, Paul Gortmaker, Rob Herring, linux-i2c,
Peter Meerwald-Stadler, Hartmut Knaack, Colin Ian King,
Andrew Morton, Jonathan Cameron
In-Reply-To: <687b26d7-7d80-e8e1-5aa8-a4e8bc070e42@axentia.se>
On Wed, 2017-04-19 at 13:23 +0200, Peter Rosin wrote:
> On 2017-04-19 13:05, Philipp Zabel wrote:
> > On Wed, 2017-04-19 at 12:41 +0200, Peter Rosin wrote:
> >> On 2017-04-19 11:17, Philipp Zabel wrote:
> >>> On Tue, 2017-04-18 at 15:36 +0200, Peter Rosin wrote:
> >>>> If I got things wrong when I skimmed whatever I came across, and if the
> >>>> mmio register is the only mux control option in the stars, it becomes
> >>>> less obvious... It's of course still possible to hook into the mux
> >>>> subsystem, but the benefit is questionable. And you do get the extra
> >>>> device tree node. You could of course also implement a mux driver
> >>>> outside of drivers/mux and thus make use of the mux api, but it's tiny
> >>>> and any benefit is truly small.
> >>>
> >>> What I wondered mostly is whether it would be a good idea to move the
> >>> OF-graph ports into the mux controller node, and let the video capture
> >>> device be the consumer of the mux.
> >>> But this wouldn't fit well with the clear split between the mux
> >>> controller and the actual mux hardware in the mux DT bindings.
> >>
> >> I have tried to do something similar. I think. The current
> >> drivers/i2c/muxes/i2c-mux-gpio.c is a good candidate for the same thing
> >> IIUC.
> >>
> >> That dedicated driver and the general purpose i2c mux driver does pretty
> >> much the same thing with these two DT snippets:
> >>
> >> Dedicated i2c-mux-gpio DT snippet:
> >>
> >> i2c-mux {
> >> compatible = "i2c-mux-gpio";
> >> i2c-parent = <&i2c1>;
> >>
> >> mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
> >>
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >>
> >> i2c@1 {
> >> ...
> >> };
> >>
> >> i2c@3 {
> >> ...
> >> };
> >> };
> >>
> >> General purpose mux DT snippet:
> >>
> >> mux: mux-controller {
> >> compatible = "gpio-mux";
> >> #mux-control-cells = <0>;
> >>
> >> mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
> >> };
> >>
> >> i2c-mux {
> >> compatible = "i2c-mux";
> >> i2c-parent = <&i2c1>;
> >>
> >> mux-controls = <&mux>;
> >>
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >>
> >> i2c@1 {
> >> ...
> >> };
> >>
> >> i2c@3 {
> >> ...
> >> };
> >> };
> >
> > Yes, replace i2c-mux with video-mux and the i2c@x nodes with port@x
> > nodes, and this is very close to what I am thinking about.
> >
> >> I would love to find a way to cleanly get the mux framework to handle
> >> the first DT as well, and thus being able to obsolete the dedicated
> >> i2c-mux-gpio driver. I have not figured out how to accomplish that
> >> without abusing the driver-model to a point that it's not working.
> >> Help with that task is dearly appreciated.
> >>
> >> What I have stumbled on, I think, is that two drivers needs to be
> >> instantiated from the same DT node. At the same time, I need the
> >> mux framework to handle the current out-of-node thing with a
> >> phandle as well, so that several mux consumers can share a common
> >> mux controller. My understanding of these matters are apparently not
> >> deep enough...
> >
> > Not necessarily, if the framework could export a function to create a
> > gpio/mmio mux_chip on a given device and the gpio-mux and *-mux-gpio
> > drivers just reuse that.
>
> I've been up that creek. Why should the gpio mux be special cased?
You are right, this does not scale.
> That's not clean, the implication is that all mux consumers need
> to handle the gpio case and have a special compatible for that
> case etc. Then someone thinks the DT should look equally "clean" for
> some i2c based mux, and the weeds start piling up. This is exactly
> what we don't want. We want the mux consumer drivers to be totally
> agnostic about the fact that they happen to use a gpio mux.
If you want to have i2c-mux-gpio and i2c-mux compatibles, and a single
driver to handle them both, it must at least match both compatibles, so
it can't be completely agnostic.
Why not then have it call
if (/* compatible == "i2c-mux" */)
mux = devm_mux_control_get(dev, NULL);
else /* if (compatible == "i2c-mux-gpio/mmio/etc.") */
mux = devm_mux_control_create(dev);
? The mux framework core could hold a list of those <usage>-mux-<type>
compatibles and dispatch creation of the correct mux (or mux platform
device, if necessary).
regards
Philipp
^ permalink raw reply
* Re: [RFC 2/2] mux: mmio-based syscon mux controller
From: Philipp Zabel @ 2017-04-19 16:32 UTC (permalink / raw)
To: Steve Longerbeam
Cc: Peter Rosin, Rob Herring, Mark Rutland, Sakari Ailus,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <c45fb784-e4ef-c5a8-3ae9-df1870c9e4a6-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Wed, 2017-04-19 at 09:23 -0700, Steve Longerbeam wrote:
>
> On 04/19/2017 08:27 AM, Philipp Zabel wrote:
> > On Wed, 2017-04-19 at 13:58 +0200, Peter Rosin wrote:
> >> On 2017-04-19 13:50, Philipp Zabel wrote:
> >>> On Thu, 2017-04-13 at 18:09 -0700, Steve Longerbeam wrote:
> >>>>
> >>>> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
> >>>>> This adds a driver for mmio-based syscon multiplexers controlled by a
> >>>>> single bitfield in a syscon register range.
> >>>>>
> >>>>> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >>>>> ---
> >>>>> drivers/mux/Kconfig | 13 +++++
> >>>>> drivers/mux/Makefile | 1 +
> >>>>> drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>> 3 files changed, 144 insertions(+)
> >>>>> create mode 100644 drivers/mux/mux-syscon.c
> >>>>>
> >>>>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> >>>>> index 86668b4d2fc52..a5e6a3b01ac24 100644
> >>>>> --- a/drivers/mux/Kconfig
> >>>>> +++ b/drivers/mux/Kconfig
> >>>>> @@ -43,4 +43,17 @@ config MUX_GPIO
> >>>>> To compile the driver as a module, choose M here: the module will
> >>>>> be called mux-gpio.
> >>>>>
> >>>>> +config MUX_SYSCON
> >>>>
> >>>> my preference would be CONFIG_MUX_MMIO.
> >>>>
> >>>>> + tristate "MMIO bitfield-controlled Multiplexer"
> >>>>
> >>>> "MMIO register bitfield-controlled Multiplexer"
> >>>>
> >>>> The rest looks good to me.
> >>>
> >>> I'll change those. mux-syscon.c should probably be renamed to
> >>> mux-mmio.c, too.
> >>
> >> I think I disagree. But I'm not familiar with syscon so I don't know.
> >> IIUC, syscon uses regmap to do mmio and this driver requires syscon
> >> to get at the regmap, and in the end this driver doesn't know anything
> >> about mmio. All it knows is syscon/regmap.
> >
> > That is a good point. Right now there is nothing MMIO about the driver
> > except for the hardware that I want it to handle.
> >
> >> If some warped syscon
> >> thing shows up that wraps something other than mmio in its regmap,
> >> this driver wouldn't care about it. And syscon is something that
> >> is also known in the DT world. Given that, I think everything in this
> >> driver should be named syscon and not mmio.
> >>
>
> My argument against using the name "syscon" in the device tree is that
> it is referring to a subsystem in the Linux kernel. Besides the fact
> that "syscon" does not clearly describe, at least to me, what sort of
> device this mux is.
If I'm not mistaken, this point was not about the DT compatible
property, just about the driver name.
I'm also in favor of keeping the "syscon" name out of the device tree as
far as it is still possible, for the same reasons. The i.MX6 muxes are
MMIO register bitfield muxes, but not "syscon muxes".
regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 03/10] dt-bindings: arm: mediatek: document clk bindings for MT6797
From: Stephen Boyd @ 2017-04-19 16:27 UTC (permalink / raw)
To: Mars Cheng
Cc: Matthias Brugger, CC Hwang, Loda Chou, Miles Chen, Jades Shih,
Yingjoe Chen, Kevin-CW Chen, linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
wsd_upstream-NuS5LvNUpcJWk0Htik3J/w
In-Reply-To: <1491614435-23754-4-git-send-email-mars.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
On 04/08, Mars Cheng wrote:
> From: Kevin-CW Chen <kevin-cw.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>
> This patch adds the binding documentation for apmixedsys, imgsys,
> infracfg, mmsys, topckgen, vdecsys and vencsys for MT6797.
>
> Signed-off-by: Kevin-CW Chen <kevin-cw.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Mars Cheng <mars.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
Applied to clk-next
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 1/2] clk: imx7d: fix USDHC NAND clock
From: Stefan Agner @ 2017-04-19 16:23 UTC (permalink / raw)
To: Stephen Boyd
Cc: shawnguo, kernel, aisheng.dong, fabio.estevam, robh+dt,
mark.rutland, linux-arm-kernel, devicetree, linux-clk,
linux-kernel
In-Reply-To: <20170419161435.GA7065@codeaurora.org>
On 2017-04-19 09:14, Stephen Boyd wrote:
> On 04/10, Stefan Agner wrote:
>> The USDHC NAND root clock is not gated by any CCM clock gate. Remove
>> the bogus gate definition.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>
> Can this be applied? It's followed by a dtsi change and there is
> zero information about if the two depend on each other. Please
> add cover letters for these sorts of things in the future
> indicating how you expect merging to work.
This can be merged. The two changes are independent.
They look kind of dependent, but really aren't since
IMX7D_NAND_USDHC_BUS_ROOT_CLK is still in the init_on list. Should have
explicitly mentioned that, sorry about that.
--
Stefan
^ permalink raw reply
* Re: [RFC 2/2] mux: mmio-based syscon mux controller
From: Steve Longerbeam @ 2017-04-19 16:23 UTC (permalink / raw)
To: Philipp Zabel, Peter Rosin
Cc: Rob Herring, Mark Rutland, Sakari Ailus,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <1492615640.2970.145.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On 04/19/2017 08:27 AM, Philipp Zabel wrote:
> On Wed, 2017-04-19 at 13:58 +0200, Peter Rosin wrote:
>> On 2017-04-19 13:50, Philipp Zabel wrote:
>>> On Thu, 2017-04-13 at 18:09 -0700, Steve Longerbeam wrote:
>>>>
>>>> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
>>>>> This adds a driver for mmio-based syscon multiplexers controlled by a
>>>>> single bitfield in a syscon register range.
>>>>>
>>>>> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>>> ---
>>>>> drivers/mux/Kconfig | 13 +++++
>>>>> drivers/mux/Makefile | 1 +
>>>>> drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 144 insertions(+)
>>>>> create mode 100644 drivers/mux/mux-syscon.c
>>>>>
>>>>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
>>>>> index 86668b4d2fc52..a5e6a3b01ac24 100644
>>>>> --- a/drivers/mux/Kconfig
>>>>> +++ b/drivers/mux/Kconfig
>>>>> @@ -43,4 +43,17 @@ config MUX_GPIO
>>>>> To compile the driver as a module, choose M here: the module will
>>>>> be called mux-gpio.
>>>>>
>>>>> +config MUX_SYSCON
>>>>
>>>> my preference would be CONFIG_MUX_MMIO.
>>>>
>>>>> + tristate "MMIO bitfield-controlled Multiplexer"
>>>>
>>>> "MMIO register bitfield-controlled Multiplexer"
>>>>
>>>> The rest looks good to me.
>>>
>>> I'll change those. mux-syscon.c should probably be renamed to
>>> mux-mmio.c, too.
>>
>> I think I disagree. But I'm not familiar with syscon so I don't know.
>> IIUC, syscon uses regmap to do mmio and this driver requires syscon
>> to get at the regmap, and in the end this driver doesn't know anything
>> about mmio. All it knows is syscon/regmap.
>
> That is a good point. Right now there is nothing MMIO about the driver
> except for the hardware that I want it to handle.
>
>> If some warped syscon
>> thing shows up that wraps something other than mmio in its regmap,
>> this driver wouldn't care about it. And syscon is something that
>> is also known in the DT world. Given that, I think everything in this
>> driver should be named syscon and not mmio.
>>
My argument against using the name "syscon" in the device tree is that
it is referring to a subsystem in the Linux kernel. Besides the fact
that "syscon" does not clearly describe, at least to me, what sort of
device this mux is. "regmap" also has similar problem in that it refers
to a Linux subsystem, although it is clearer to me at least, what the
mux is composed of (a mapped register). Personally I still like mmio,
most embedded systems access registers via MMIO, and the name is not
referring to any specific Linux subsystem.
Steve
>> Or?
>
> Well, ...
>
> the driver could be extended to do actual MMIO if the syscon is not
> found. This would work only if it has exclusive access to its register.
>
> On the other hand, the driver could also be made to match against
> compatible = "bitfield-mux",
> for example, and allow handling muxes inside SPI or I2C controlled MFD
> devices that provide a syscon regmap, as you describe:
>
> spi-host {
> mfd-device {
> compatible = "some-spi-regmap-device";
>
> mux {
> compatible = "bitfield-mux";
> };
> };
> };
>
> regards
> Philipp
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 05/10] clk: mediatek: add clk support for MT6797
From: Stephen Boyd @ 2017-04-19 16:19 UTC (permalink / raw)
To: Mars Cheng
Cc: Matthias Brugger, CC Hwang, Loda Chou, Miles Chen, Jades Shih,
Yingjoe Chen, Kevin-CW Chen, linux-clk, linux-kernel,
linux-mediatek, devicetree, wsd_upstream
In-Reply-To: <1491614435-23754-6-git-send-email-mars.cheng@mediatek.com>
On 04/08, Mars Cheng wrote:
> From: Kevin-CW Chen <kevin-cw.chen@mediatek.com>
>
> Add MT6797 clock support, include topckgen, apmixedsys, infracfg
> and subsystem clocks
>
> Signed-off-by: Kevin-CW Chen <kevin-cw.chen@mediatek.com>
> Signed-off-by: Mars Cheng <mars.cheng@mediatek.com>
> Tested-by: Matthias Brugger <matthias.bgg@gmail.com>
> Acked-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
Applied to clk-next
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ 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