* Re: [PATCH v4 4/5] mm: make processing of movable_node arch-specific
From: Michael Ellerman @ 2016-10-26 10:52 UTC (permalink / raw)
To: Reza Arbab, Balbir Singh
Cc: Benjamin Herrenschmidt, Paul Mackerras, Rob Herring, Frank Rowand,
Andrew Morton, Bharata B Rao, Nathan Fontenot, Stewart Smith,
Alistair Popple, Aneesh Kumar K.V, Tang Chen, linux-kernel,
linuxppc-dev, devicetree, linux-mm
In-Reply-To: <20161026004929.h6v54dhehk4yvmwm@arbab-vm>
Reza Arbab <arbab@linux.vnet.ibm.com> writes:
> On Wed, Oct 26, 2016 at 09:34:18AM +1100, Balbir Singh wrote:
>>I still believe we need your changes, I was wondering if we've tested
>>it against normal memory nodes and checked if any memblock
>>allocations end up there. Michael showed me some memblock
>>allocations on node 1 of a two node machine with movable_node
>
> The movable_node option is x86-only. Both of those nodes contain normal
> memory, so allocations on both are allowed.
>
>>> Longer; if you use "movable_node", x86 can identify these nodes at
>>> boot. They call memblock_mark_hotplug() while parsing the SRAT. Then,
>>> when the zones are initialized, those markings are used to determine
>>> ZONE_MOVABLE.
>>>
>>> We have no analog of this SRAT information, so our movable nodes can
>>> only be created post boot, by hotplugging and explicitly onlining
>>> with online_movable.
>>
>>Is this true for all of system memory as well or only for nodes
>>hotplugged later?
>
> As far as I know, power has nothing like the SRAT that tells us, at
> boot, which memory is hotpluggable.
On pseries we have the ibm,dynamic-memory device tree property, which
can contain ranges of memory that are not yet "assigned to the
partition" - ie. can be hotplugged later.
So in general that statement is not true.
But I think you're focused on bare-metal, in which case you might be
right. But that doesn't mean we couldn't have a similar property, if
skiboot/hostboot knew what the ranges of memory were going to be.
cheers
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH 2/5] ARM: davinci: Don't append git rev to local version
From: Sekhar Nori @ 2016-10-26 10:54 UTC (permalink / raw)
To: David Lechner, Kevin Hilman
Cc: Rob Herring, Mark Rutland, Russell King, devicetree,
linux-arm-kernel, linux-kernel
In-Reply-To: <bf8cac01-910d-5afa-e840-f0d7ee96d22a@lechnology.com>
On Monday 24 October 2016 08:45 PM, David Lechner wrote:
> On 10/24/2016 06:35 AM, Sekhar Nori wrote:
>> On Saturday 22 October 2016 12:06 AM, David Lechner wrote:
>>> In the davinci default configuration, don't append the git revision to
>>> the local kernel version by. This seems like the more desirable default
>>> value.
>>
>> Why? To the contrary I actually quite like the fact that the git commit
>> is appended to version string. Makes it easy for me to cross-check that
>> I am booting the right image.
>>
>>>
>>> Signed-off-by: David Lechner <david@lechnology.com>
>>
>> Thanks,
>> Sekhar
>>
>
> Each time you make a commit, you get a new version, which installs
> another copy of the kernel modules on the device. This will fill up the
> SD card if you are making many commits.
Right, but thats easily fixable by removing existing modules before
installing new ones.
> Also, if someone wants to build the mainline kernel using the default
> configuration, it seems odd to have a git revision tacked on to the end
> even though you made no revisions.
If you checkout a tag and build, then no commit information is added.
Which I guess is what most end users will do.
I don't see this done in other defconfigs like omap2plus and multi_v7 as
well. I would like to keep it similar for davinci.
Thanks,
Sekhar
^ permalink raw reply
* Re: [PATCH] tpm_tis: Allow tpm_tis to be bound using DT
From: Jarkko Sakkinen @ 2016-10-26 10:55 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
In-Reply-To: <20161025232331.GA20339-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
On Tue, Oct 25, 2016 at 05:23:31PM -0600, Jason Gunthorpe wrote:
> This provides an open firwmare driver binding for tpm_tis. OF
> is useful on arches where ACPI/PNP is not used.
>
> The tcg,tpm_tis-mmio register map interface is specified by the TCG.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
> .../bindings/security/tpm/tpm_tis_mmio.txt | 22 ++++++++++++++++++++++
I'll do a proper review later on but should this change be a separate
commit or not?
/Jarkko
> drivers/char/tpm/Kconfig | 2 +-
> drivers/char/tpm/tpm_tis.c | 11 +++++++++++
> 3 files changed, 34 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_tis_mmio.txt
>
> A bunch of work was done previously on how the tis driver probes and
> binds, so DT enablement is now trivial.
>
> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_tis_mmio.txt b/Documentation/devicetree/bindings/security/tpm/tpm_tis_mmio.txt
> new file mode 100644
> index 000000000000..67661f6a27cd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_tis_mmio.txt
> @@ -0,0 +1,22 @@
> +Trusted Computing Group MMIO Trusted Platform Module
> +
> +The TCG defines multi vendor standard for accessing a TPM chip, this
> +is the standard protocol defined to access the TPM via MMIO. Typically
> +this interface will be implemented over Intel's LPC bus.
> +
> +Refer to the 'TCG PC Client Specific TPM Interface Specification (TIS)' TCG
> +publication for the specification.
> +
> +Required properties:
> +
> +- compatible: should specify the actual hardware chip followed by the
> + generic interface name "tcg,tpm_tis-spi";
> +- reg: The location of the MMIO registers, should be at least 0x5000 bytes
> +- interrupt: An optional interrupt indicating command completion.
> +
> +Example:
> +
> + tpm_tis@90000 {
> + compatible = "atmel,at97sc3204", "tcg,tpm_tis-mmio";
> + reg = <0x90000 0x5000>;
> + };
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 9faa0b1e7766..277186d3b668 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -32,7 +32,7 @@ config TCG_TIS_CORE
>
> config TCG_TIS
> tristate "TPM Interface Specification 1.2 Interface / TPM 2.0 FIFO Interface"
> - depends on X86
> + depends on X86 || OF
> select TCG_TIS_CORE
> ---help---
> If you have a TPM security chip that is compliant with the
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index eaf5730d79eb..5a63fafaca69 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -28,6 +28,8 @@
> #include <linux/wait.h>
> #include <linux/acpi.h>
> #include <linux/freezer.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> #include "tpm.h"
> #include "tpm_tis_core.h"
>
> @@ -354,12 +356,21 @@ static int tpm_tis_plat_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id tis_of_platform_match[] = {
> + {.compatible = "tcg,tpm_tis-mmio"},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, tis_of_platform_match);
> +#endif
> +
> static struct platform_driver tis_drv = {
> .probe = tpm_tis_plat_probe,
> .remove = tpm_tis_plat_remove,
> .driver = {
> .name = "tpm_tis",
> .pm = &tpm_tis_pm,
> + .of_match_table = of_match_ptr(tis_of_platform_match),
> },
> };
>
> --
> 2.1.4
>
------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive.
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
^ permalink raw reply
* Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation
From: Mark Rutland @ 2016-10-26 10:56 UTC (permalink / raw)
To: John Youn
Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring
In-Reply-To: <e7a864e5a2af12d5a8d97f2591065f4aa85f86cb.1477424426.git.johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
On Tue, Oct 25, 2016 at 12:42:46PM -0700, John Youn wrote:
> Add interrupt moderation interval binding for dwc3.
>
> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> ---
> Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index e3e6983..17de9fc 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -53,6 +53,7 @@ Optional properties:
> - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
> register for post-silicon frame length adjustment when the
> fladj_30mhz_sdbnd signal is invalid or incorrect.
> + - snps,imod_interval: the interrupt moderation interval.
As otherwise commented, s/_/-/
What is "interrupt moderation"? The cover mentions that this is to be
used for some kind of workaround, but it's not clear to me what this is,
and as such, whether it makes sense to describe it in this manner.
Thanks,
Mark.
>
> - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>
> --
> 2.10.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 3/5] ARM: davinci: enable gpio poweroff in default config
From: Sekhar Nori @ 2016-10-26 11:09 UTC (permalink / raw)
To: David Lechner, Kevin Hilman
Cc: Rob Herring, Mark Rutland, Russell King,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1477075018-20176-4-git-send-email-david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
On Saturday 22 October 2016 12:06 AM, David Lechner wrote:
> The gpio-poweroff driver is needed by LEGO MINDSTORMS EV3 (AM1808 based
> board).
>
> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
For defconfig patches, we have been using the davinci_all_defconfig
prefix (see git log --oneline arch/arm/configs/davinci_all_defconfig).
Applied to v4.10/defconfig with subject line changed to:
"ARM: davinci_all_defconfig: enable gpio poweroff driver"
Thanks,
Sekhar
--
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] tpm_tis: Allow tpm_tis to be bound using DT
From: Mark Rutland @ 2016-10-26 11:25 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
In-Reply-To: <20161025232331.GA20339-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
On Tue, Oct 25, 2016 at 05:23:31PM -0600, Jason Gunthorpe wrote:
> This provides an open firwmare driver binding for tpm_tis. OF
> is useful on arches where ACPI/PNP is not used.
>
> The tcg,tpm_tis-mmio register map interface is specified by the TCG.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
> .../bindings/security/tpm/tpm_tis_mmio.txt | 22 ++++++++++++++++++++++
> drivers/char/tpm/Kconfig | 2 +-
> drivers/char/tpm/tpm_tis.c | 11 +++++++++++
> 3 files changed, 34 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_tis_mmio.txt
>
> A bunch of work was done previously on how the tis driver probes and
> binds, so DT enablement is now trivial.
>
> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_tis_mmio.txt b/Documentation/devicetree/bindings/security/tpm/tpm_tis_mmio.txt
> new file mode 100644
> index 000000000000..67661f6a27cd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_tis_mmio.txt
> @@ -0,0 +1,22 @@
> +Trusted Computing Group MMIO Trusted Platform Module
> +
> +The TCG defines multi vendor standard for accessing a TPM chip, this
> +is the standard protocol defined to access the TPM via MMIO. Typically
> +this interface will be implemented over Intel's LPC bus.
> +
> +Refer to the 'TCG PC Client Specific TPM Interface Specification (TIS)' TCG
> +publication for the specification.
> +
> +Required properties:
> +
> +- compatible: should specify the actual hardware chip followed by the
> + generic interface name "tcg,tpm_tis-spi";
Please use '-' in comaptible strings rather than '_'.
Please also describe chip strings, even if those aren't use by the
driver, e.g.
compatible: should contain a string below for the chip, followed by
"tcg,tpm-tis-spi". Valid chip strings are:
* "atmel,at97sc3204"
> +- reg: The location of the MMIO registers, should be at least 0x5000 bytes
> +- interrupt: An optional interrupt indicating command completion.
> +
> +Example:
> +
> + tpm_tis@90000 {
> + compatible = "atmel,at97sc3204", "tcg,tpm_tis-mmio";
As above, that first compatible string should be listed, if it's
intended to be valid.
Otherwise, this looks fine to me.
Thanks,
Mark.
> + reg = <0x90000 0x5000>;
> + };
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 9faa0b1e7766..277186d3b668 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -32,7 +32,7 @@ config TCG_TIS_CORE
>
> config TCG_TIS
> tristate "TPM Interface Specification 1.2 Interface / TPM 2.0 FIFO Interface"
> - depends on X86
> + depends on X86 || OF
> select TCG_TIS_CORE
> ---help---
> If you have a TPM security chip that is compliant with the
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index eaf5730d79eb..5a63fafaca69 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -28,6 +28,8 @@
> #include <linux/wait.h>
> #include <linux/acpi.h>
> #include <linux/freezer.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> #include "tpm.h"
> #include "tpm_tis_core.h"
>
> @@ -354,12 +356,21 @@ static int tpm_tis_plat_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id tis_of_platform_match[] = {
> + {.compatible = "tcg,tpm_tis-mmio"},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, tis_of_platform_match);
> +#endif
> +
> static struct platform_driver tis_drv = {
> .probe = tpm_tis_plat_probe,
> .remove = tpm_tis_plat_remove,
> .driver = {
> .name = "tpm_tis",
> .pm = &tpm_tis_pm,
> + .of_match_table = of_match_ptr(tis_of_platform_match),
> },
> };
>
> --
> 2.1.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
>
------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive.
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
^ permalink raw reply
* Re: [PATCH 1/5] ARM: davinci: Compile MMC in kernel
From: Sekhar Nori @ 2016-10-26 11:33 UTC (permalink / raw)
To: David Lechner, Kevin Hilman
Cc: Rob Herring, Mark Rutland, Russell King,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1477075018-20176-2-git-send-email-david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
On Saturday 22 October 2016 12:06 AM, David Lechner wrote:
> This changes the davinci default configuration to compile the davinci
> MMC driver into the kernel. This allows booting from an SD card without
> requiring an initrd containing the kernel module.
>
> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
"build" is more relevant here than "compile" so I changed that. And also
used davinci_all_defconfig as detailed in last e-mail.
Applied to v4.10/defconfig
Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 01/12] dt: arm: shmobile: add H3ULCB board DT bindings
From: Geert Uytterhoeven @ 2016-10-26 11:33 UTC (permalink / raw)
To: Vladimir Barinov
Cc: Simon Horman, Magnus Damm, Rob Herring, Mark Rutland,
devicetree@vger.kernel.org, Linux-Renesas, Yusuke Goda
In-Reply-To: <936e63bd-4f01-891f-24da-4c34caf94bcf@cogentembedded.com>
Hi Vladimir,
On Wed, Oct 26, 2016 at 12:27 PM, Vladimir Barinov
<vladimir.barinov@cogentembedded.com> wrote:
> I've got this message from Renesas:
> "
>
> Please use H3ULCB/M3ULCB in upstream.
> # I got an agreement among parties.
>
> Board name in community: ULCB (= development code name)
> Official name (product/brand name): Starter Kit Pro/Premier
> "
>
> Renaming H3ULCB -> H3SK is not desired for Renesas.
>
> What do you think to append the product/brand name only? :
>
> - H3ULCB (RTP0RC7795SKB00010S)
> + H3ULCB (R-Car Starter Kit Premier, RTP0RC7795SKB00010S)
> compatible = "renesas,h3ulcb", "renesas,r8a7795";
Looks good to me.
And then we can add
M3ULCB (R-Car Starter Kit Pro, RTP0RC7796SKB00010S)
compatible = "renesas,m3ulcb", "renesas,r8a7796";
later.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: Add Allwinner Q8 tablets hardware manager
From: Hans de Goede @ 2016-10-26 11:46 UTC (permalink / raw)
To: Mark Rutland
Cc: devicetree, Arnd Bergmann, Greg Kroah-Hartman, Pantelis Antoniou,
Chen-Yu Tsai, Rob Herring, Maxime Ripard, linux-arm-kernel
In-Reply-To: <20161024173900.GP15620@leverpostej>
Hi,
On 24-10-16 19:39, Mark Rutland wrote:
> On Fri, Oct 14, 2016 at 09:53:31AM +0200, Hans de Goede wrote:
>> Hi Rob, Mark, et al.,
>
> Hi Hans,
>
> Apologies for the delay in replying to this.
No worries, I believe that 1 week is actually a pretty good
turn around time, esp. directly after a conference.
> I'd like to be clear that I do understand that there is a problem that
> needs to be addressed here. However, I do not believe that the *current*
> in-kernel approach is correct. More on that below.
Ok.
>> Mark, I know that we discussed this at ELCE and you clearly indicated
>> that according to you this does not belong in the kernel. I was a bit
>> surprised by this part of the discussion.
>>
>> I had posted a RFC earlier and Rob had indicated that given that the q8
>> tablets are a special case, as my code uses actual probing rather then some
>> pre-arranged id mechanism with say an eeprom, that doing this in a
>> non-generic manner would be ok for my special case.
>
> To some extent, Rob and I may have differing views here; I'm not
> entirely sure what Rob's view is, and I cannot talk on his behalf. I
> certainly must apologise for having not commented on said RFC, however.
>
>> So on to why I believe that the kernel is the best place to do this, at least
>> for my special use-case:
>>
>> 1. Configurability
>>
>> Since the q8 tablets do not have any id mechanism I'm probing i2c busses to
>> generate i2c client dt nodes with the right address and compatible.
>> Some info in these nodes (e.g. touchscreen firmware-name) is not probable at
>> all and gets set by heuristics. This heuristics may get things wrong.
>> So my current implementation offers kernel cmdline options to override this.
>
> As I mentioned at ELCE, one major concern I have with this approach is
> this probing, which in part relies on a collection of heuristics.
This is quite use-case specific, anyways, the probing is a 2 step process:
1) Identify which hardware there is in the tablet, this is pretty
reliable, we only detect a fix set of known possible touchscreens
and accelerometers, at known addresses and almost all have an id
register to check.
2) Determine *defaults* for various none probable settings, like guessing
which firmware to load into the touchscreen controllers, as there are
at least 2 ways the gsl1680 is wired up on these tablets and this
requires 2 different firmware files. This uses heuristics, to, as said,
determine the defaults all of the non-probable bits are overidable
through config options (currently kernel module options). Getting these
wrong is not dangerous to the hardware, but will work in a non-functional
or misbehaving (wrong coordinates) touchscreen.
Note with the models I've access to so far the heuristics score 100%
but I'm not sure how representative the 16 models I've access to are
(they are all different and have been bought over a span of multiple
years).
> I'm worried that this is very fragile, and sets us up for having to
> maintain a much larger collection of heuristics and/or a database of
> particular boards in future. This is fragile, defeats much of the gain
> from DT.
I understand your worries, as said I'm confident the actual probing
is safe and getting the heuristics wrong will result in misbehavior,
but not in any hardware damage or such.
> Worse, this could be completely incompatible with some arbitrary board
> that comes by in future,
I assume you mean an arbitrary q8 tablet here, as the probe code does
bind by board/machine compatible, so for a really arbitrary board
this code will never activate.
> because the kernel assumed something that was
> not true, which it would not have done if things were explicitly
> described to the kernel.
I understand your worry, but moving the probing code to say u-boot
will not change any of this, the kernel will get the explicit
description created by the u-boot probe code, but it would be
just as wrong.
So maybe we need to answer 2 questions in a row:
1) Do we want such probe code at all ?
My answer to this is yes, these (cheap) tablets are interesting to
e.g. the maker community and I would like them to run mainline
(and run mainline well), but given the way there are put together
this require some code to dynamically adapt to the batch of the
month somewhere
2) Where do we put this code ?
If we agree on 1 (I assume we do) then this becomes the real
question, at which point your worries about the kernel assuming
something which is not true because the probe code got it wrong
may become true regardless where the code lives.
So wrt this worries is all I can do is ask you to trust me to
not mess things up, just like we all trust driver authors, etc.
all the time to not mess things up.
> As I mentioned at ELCE, I'm not opposed to the concept of the kernel
> applying overlays or fixups based on a well-defined set of criteria;
> having some of that embedded in the DT itself would be remarkably
> helpful. However, I am very much not keen on loosely defined criteria as
> with here, as it couples the DT and kernel, and creates problems longer
> term, as described above.
Right, so again I think we need to split the discussion in 2 steps:
1) How do we apply the fixups, currently I'm using free-form changes
done from C-code. I can envision moving to something like the quirk
mechanism suggested by Pantelis in the past. Note this is not a perfect
fit for my rather corner-case use-case, but I can understand that in
general you want the variants to be described in dt, and activated
in some way, rather then have c-code make free-form changes to the dt
2) How do we select which fixups to apply. Again I can understand
you wanting some well defined mechanism for this, but again my
use-case is special and simply does not allow for this as there
is no id-eeprom to read or some such.
>> Although having to specify kernel cmdline options is not the plug and play
>> experience I want to give end-users most distros do have documentation on
>> how to do this and doing this is relatively easy for end-users. Moving this
>> to the bootloader means moving to some bootloader specific config mechanism
>> which is going to be quite hard (and possibly dangerous) for users to use.
>
> I have to ask, why is it more dangerous?
Because for normal end users meddling with the bootloader / with u-boot's
environment is like flashing a PC BIOS. Most (non technical) end users will
want to install u-boot once (or not at all) and then just have it work.
> Perhaps more difficult, but that can be solved,
More difficult means not doable for many users.
> if the manual
> corrections are simply a set of options to be passed to the kernel, I
> don't see why the bootloader cannot pick this up.
>
>> 2. Upgradability
>>
>> Most users treat the bootloader like they treat an x86 machine BIOS/EFI,
>> they will never upgrade it unless they really have to. This means that it
>> will be very difficult to get users to actual benefit from bug-fixes /
>> improvements done to the probing code. Where as the kernel on boards running
>> e.g. Debian or Fedora gets regular updates together with the rest of the
>> system.
>
> Given that DTBs are supposed to remain supported, users should find
> themselves with a system that continues to work, but may not have all
> the bells and whistles it could, much like elsewhere.
>
> While it's true that we have issues in this area, I don't think that's
> an argument for putting things into the kernel for this specific set of
> boards.
It is an argument to put much of the dynamic (dt) hardware support in
the kernel in general.
>> 3. Infrastructure
>>
>> The kernel simply has better infrastructure for doing these kind of things.
>
> At least on the DT front, a lot of work has gone into improving the
> infrastructure, e.g. the work that Free Electrons have done [1]. I
> appreciate that the infrastructure for things like poking SPI may not be
> as advanced.
>
> Which bits of infrastructure do you find lacking today?
Nothing really specific (I've not yet tried porting the probe code
to u-boot), but I just find working within the kernel easier in general,
since there really just is a lot more infrastructure. Note I'm the
upstream u-boot maintainer for the allwinner SoC support, so this
is not due to me being unfamiliar with u-boot.
>> Yes we could improve the bootloader, but the kernel is also improving
>> and likely at a faster rate. Besides that the purpose of the
>> bootloader is mostly to be simple and small, load the kernel and get
>> out of the way, not to be a general purpose os kernel. So it will
>> simply always have less infrastructure and that is a good thing,
>> otherwise we will be writing another general purpose os which is a
>> waste of effort.
>
> I think this conflates a number of details. Yes, we'd like firmware and
> bootloaders to be small, and yes, their infrastructure and feature
> support will be smaller than a general purpose OS.
>
> That doesn't imply that they cannot have features necessary to boostrap
> an OS.
>
> It's also not strictly necessary that the firmware or bootloader have
> the capability to do all this probing, as that could be contained in
> another part (e.g. a U-Boot application which is run once to detect the
> board details, logging this into a file).
>
> It's also possible to ship an intermediary stage (e.g. like the
> impedance matcher) which can be upgradeable independently of the kernel.
Yes there are other solutions, but they all involve a lot more
moving pieces (and thus will break) then a single isolated .c file
in the kernel, which is all this series adds.
Esp the intermediate solution just adds a ton of complexity with 0
gain.
>> 4. This is not a new board file
>>
>> Mark, at ELCE I got the feeling that your biggest worry / objection is
>> that this will bring back board files, but that is not the case, if you
>> look at the actual code it is nothing like a board-file at all. Where a
>> board file was instantiating device after device after device from c-code,
>> with maybe a few if-s in there to deal with board revisions. This code is
>> all about figuring out which accelerometer and touchscreen there are,
>> to then add nodes to the dt for this 2 devices, almost all the code is
>> probing, the actual dt modifying code is tiny and no direct device
>> instantiation is done at all.
>
> Sorry, but I must disagree. This code:
>
> (a) identifies a set of boards based on a top-level compatible string.
> i.e. it's sole purpose is to handle those boards.
>
> (b) assumes non-general properties of those boards (e.g. that poking
> certain SPI endpoints is safe).
>
> (c) applies arbitrary properties to the DT, applying in-built knowledge
> of those boards (in addition to deep structural knowledge of the
> DTB in question).
>
> To me, given the implicit knowledge, that qualifies as a "board file".
>
> As I mentioned at ELCE, if this had no knowledge of the boards in
> question, I would be less concerned. e.g. if there was a well-defined
> identification mechanism, describe in the DT, with fixups also defined
> in the DT.
And as I tried to explain before, for this specific use-case describing
all this board specific knowledge in a generic manner in dt is simply
impossible, unless we add a turing complete language to dt aka aml.
I've a feeling that you're mixing this, rather special, use-case with
the more generic use-case of daughter-boards for various small-board-computers
I agree that for the SBC use-case it makes sense to try and come up with
a shared core / dt bindings for much of this. Note that even this boards
will still need a board (or board-family) specific method for getting
the actual id from a daughter-board, but this board specific code could
then pass the id to some more general hw-manager core which starts applying
dt changes based on the id. But this assumes there is a single id to
uniquely identify the extensions, which in my case there simply is not.
>> 5. This is an exception, not the rule
>>
>> Yes this is introducing board (family of boards) specific c-code into the
>> kernel, so in a way it is reminiscent of board files. But sometimes this is
>> necessary, just look at all the vendor / platform specific code in the kernel
>> in drivers/platform/x86, or all the places where DMI strings are used to
>> uniquely identify a x86 board and adjust behavior.
>>
>> But this really is the exception, not the rule. I've written/upstreamed a
>> number of drivers for q8 tablets hardware and if you look at e.g. the
>> silead touchscreen driver then in linux-next this is already used for 5
>> ARM dts files where no board specific C-code is involved at all.
>>
>> So this again is very different from the board file era, where C-code
>> had to be used to fill device specific platform-data structs, here all
>> necessary info is contained in the dt-binding and there are many users
>> who do not need any board specific C-code in the kernel at all.
>>
>> So dt is working as it should and is avoiding board specific C-code for
>> the majority of the cases. But sometimes hardware is not as we ideally
>> would like it to be; and for those *exceptions* we are sometimes going
>> to need C-code in the kernel, just like there is "board" specific C-code
>> in the x86 code.
>
> Your talk convinced me that we're both going to see more variation
> within this family of boards, and that we'll see more families of boards
> following a similar patter. Given that, I think that we need a more
> general solution, as I commented on the RFC.
>
> That doesn't necessarily mean that this can't happen in the kernel, but
> it certainly needs to be more strictly defined, e.g. with match criteria
> and fixups explicit in the DTB.
The only answer I've to: "with match criteria and fixups explicit in the DTB"
is: ok, give my a turing complete language inside DTB then, anything else
will not suffice. So either we are doomed to reinvent ACPI; or we must
accept some board(family) specific C-code in the kernel.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v4 00/23] soc: renesas: Add R-Car RST driver for obtaining mode pin state
From: Geert Uytterhoeven @ 2016-10-26 11:56 UTC (permalink / raw)
To: Philipp Zabel
Cc: Geert Uytterhoeven, Michael Turquette, Stephen Boyd, Simon Horman,
Magnus Damm, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux-Renesas,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1477064730.2508.24.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Hi Philip,
On Fri, Oct 21, 2016 at 5:45 PM, Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> Am Freitag, den 21.10.2016, 15:17 +0200 schrieb Geert Uytterhoeven:
>> Hi Philipp, Mike, Stephen, Simon, Magnus,
>> (see questions *** below!)
>>
>> Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the
>> state of the mode pins either by a call from the platform code, or
>> directly by using a hardcoded register access. This is a bit messy, and
>> creates a dependency between driver and platform code.
>>
>> This patch series converts the various Renesas R-Car clock drivers
>> and support code from reading the mode pin states using a hardcoded
>> register access to using a new minimalistic R-Car RST driver.
[...]
>> - Philip: While this is a driver for a reset-controller, currently it
>> doesn't provide any reset-controller functionality. Hence I added it
>> to drivers/soc/renesas/. Is that OK for you?
>
> Absolutely, as long as the driver isn't even a reset controller
> provider, this is the right place.
Thank you!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 00/23] soc: renesas: Add R-Car RST driver for obtaining mode pin state
From: Geert Uytterhoeven @ 2016-10-26 12:00 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Philipp Zabel, Simon Horman, Magnus Damm,
devicetree@vger.kernel.org, Linux-Renesas,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1477055857-17936-1-git-send-email-geert+renesas@glider.be>
Hi Mike, Stephen,
On Fri, Oct 21, 2016 at 3:17 PM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the
> state of the mode pins either by a call from the platform code, or
> directly by using a hardcoded register access. This is a bit messy, and
> creates a dependency between driver and platform code.
>
> This patch series converts the various Renesas R-Car clock drivers
> and support code from reading the mode pin states using a hardcoded
> register access to using a new minimalistic R-Car RST driver.
>
> All R-Car clock drivers will rely on the presence in DT of a device node
> for the RST module. Backwards compatibility with old DTBs is retained
> only for R-Car Gen2, which has fallback code using its own private copy
> of rcar_gen2_read_mode_pins().
>
> After this, there is still one remaining user of
> rcar_gen2_read_mode_pins() left in platform code. A patch series to
> remove that user has already been posted, though ("[PATCH/RFT 0/4] ARM:
> shmobile: R-Car Gen2: Allow booting secondary CPU cores in debug mode").
> Since v3, the other user has been removed in commit 9f5ce39ddb8f68b3
> ("ARM: shmobile: rcar-gen2: Obtain extal frequency from DT").
>
> This series consists of 5 parts:
> A. Patches 1 and 2 add DT bindings and driver code for the R-Car RST
> driver,
> B. Patches 3-11 add device nodes for the RST modules to the R-Car DTS
> files,
> C. Patches 12-17 convert the clock drivers to call into the new R-Car
> RST driver,
> D. Patches 18-20 remove passing mode pin state to the clock drivers
> from the platform code,
> E. Patches 21-23 remove dead code from the clock drivers.
>
> As is usually the case with moving functionality from platform code to
> DT, there are lots of hard dependencies:
> - The DT updates in Part B can be merged as soon as the DT bindings in
> Part A have been approved,
> - The clock driver updates in Part C depend functionally on the driver
> code in Part A, and on the DT updates in Part B,
> - The board code cleanups in Part D depend on the clock driver updates
> in Part C,
> - The block driver cleanups in part E depend on the board code
> cleanups in part D.
>
> Hence to maintain the required lockstep between SoC driver, clock
> drivers, shmobile platform code, and shmobile DT, I propose to queue up
> all patches in a single branch against v4.9-rc1, and send pull requests
> to both Mike/Stephen (clock) and Simon (rest).
>
> ***
> - Mike/Stephen/Simon/Magnus: Are you OK with the suggested merge
> approach above?
Is this OK for you?
I'd like to move forward with this, as this is a prerequisite for adding
support for new SoCs (RZ/G) without adding more copies of
rcar_gen2_read_mode_pins(), and removing that function from platform code
for good.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v4 0/7] Add R8A7743/SK-RZG1M board support
From: Geert Uytterhoeven @ 2016-10-26 12:08 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Simon Horman, Linux-Renesas, Rob Herring, Mark Rutland,
devicetree@vger.kernel.org, Magnus Damm, Russell King,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <2719479.geOc9vqcRS@wasted.cogentembedded.com>
Hi Sergei,
On Fri, Oct 21, 2016 at 10:39 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Here's the set of 8 patches against Simon Horman's 'renesas.git' repo's
> 'renesas-devel-20161021-v4.9-rc1' tag. I'm adding the device tree support for
> the R8A7743-based SK-RZG1M board. The SoC is close to R8A7791 and the board
> seems identical to the R8A7791/Porter board. The device tree patches depend on
> the R8A7743 CPG/MSSR driver series just posted in order to compile and work.
They depend only on "[PATCH v3 1/2] ARM: shmobile: r8a7743: add CPG clock
index macros" of that series, right?
"[PATCH v3 2/2] clk: renesas: cpg-mssr: add R8A7743 support" is not needed,
and introduces an additional dependency on "[PATCH 1/3] clk: renesas:
cpg-mssr: add common R-Car Gen2 support".
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v3 1/2] ARM: shmobile: r8a7743: add CPG clock index macros
From: Geert Uytterhoeven @ 2016-10-26 12:12 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Michael Turquette, linux-clk, Stephen Boyd, Rob Herring,
Mark Rutland, devicetree@vger.kernel.org, Linux-Renesas,
Simon Horman
In-Reply-To: <21115355.YTPdgcs0id@wasted.cogentembedded.com>
On Fri, Oct 21, 2016 at 10:15 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Add macros usable by the device tree sources to reference the R8A7743 CPG
> clocks by index. The data comes from the table 7.2b in the revision 0.50
> of the RZ/G Series User's Manual.
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v3 2/2] clk: renesas: cpg-mssr: add R8A7743 support
From: Geert Uytterhoeven @ 2016-10-26 12:14 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Michael Turquette, linux-clk, Stephen Boyd, Rob Herring,
Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux-Renesas, Simon Horman
In-Reply-To: <5038285.9DcXKlHSpF-gHKXc3Y1Z8zGSmamagVegGFoWSdPRAKMAL8bYrjMMd8@public.gmane.org>
On Fri, Oct 21, 2016 at 10:17 PM, Sergei Shtylyov
<sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org> wrote:
> --- /dev/null
> +++ linux/drivers/clk/renesas/r8a7743-cpg-mssr.c
> @@ -0,0 +1,264 @@
> +static int __init r8a7743_cpg_mssr_init(struct device *dev)
> +{
> + const struct rcar_gen2_cpg_pll_config *cpg_pll_config;
> + u32 cpg_mode = rcar_gen2_read_modemr();
It would be good if we could appply "[PATCH v4 00/23] soc: renesas: Add R-Car
RST driver for obtaining mode pin state" first, and change this driveer to
use rcar_rst_read_mode_pins() instead.
> + cpg_pll_config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
> +
> + return rcar_gen2_cpg_init(cpg_pll_config);
> +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/4] mfd: ti_am335x_tscadc: store physical address
From: Lee Jones @ 2016-10-26 12:17 UTC (permalink / raw)
To: Mugunthan V N
Cc: Mark Rutland, devicetree, Lars-Peter Clausen, Vignesh R,
linux-iio, linux-kernel, Sekhar Nori, Russell King,
Andrew F . Davis, Tony Lindgren, Rob Herring, linux-arm-kernel,
Peter Meerwald-Stadler, Hartmut Knaack, linux-omap,
Jonathan Cameron
In-Reply-To: <f271b805-82c2-a45e-0de9-39edafd34d2e@ti.com>
On Fri, 30 Sep 2016, Mugunthan V N wrote:
> On Wednesday 28 September 2016 01:10 AM, Lee Jones wrote:
> > On Wed, 21 Sep 2016, Mugunthan V N wrote:
> >
> >> store the physical address of the device in its priv to use it
> >> for DMA addressing in the client drivers.
> >>
> >> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
> >> ---
> >> drivers/mfd/ti_am335x_tscadc.c | 1 +
> >> include/linux/mfd/ti_am335x_tscadc.h | 1 +
> >> 2 files changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> >> index c8f027b..0f3fab4 100644
> >> --- a/drivers/mfd/ti_am335x_tscadc.c
> >> +++ b/drivers/mfd/ti_am335x_tscadc.c
> >> @@ -183,6 +183,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)
> >> tscadc->irq = err;
> >>
> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> + tscadc->tscadc_phys_base = res->start;
> >
> > This is unusual. Can't you use a virt_to_phys() variant instead?
> >
>
> I tried using virt_to_phys(), but its not working for me.
> Also saw many drivers uses like this to get physical address
> ("git grep -n " res->start;" drivers/*").
Very well:
For my own reference:
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
Let me know how you wish this set to be handled.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: mfd: Add Altera Arria10 SR Monitor
From: Lee Jones @ 2016-10-26 12:51 UTC (permalink / raw)
To: tthayer
Cc: mark.rutland, devicetree, arnd, gregkh, linux-kernel, inux,
robh+dt, geert, dinguyen, davem, linux-arm-kernel
In-Reply-To: <1476394329-31696-2-git-send-email-tthayer@opensource.altera.com>
On Thu, 13 Oct 2016, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
>
> Add the Arria10 DevKit System Resource Chip register and state
> monitoring module to the MFD.
>
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> Note: This needs to be applied to the bindings document that
> was Acked & Applied but didn't reach the for-next branch.
> See https://patchwork.ozlabs.org/patch/629397/
> ---
> ---
> .../devicetree/bindings/mfd/altera-a10sr.txt | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/altera-a10sr.txt b/Documentation/devicetree/bindings/mfd/altera-a10sr.txt
> index ea151f2..0787ec6 100644
> --- a/Documentation/devicetree/bindings/mfd/altera-a10sr.txt
> +++ b/Documentation/devicetree/bindings/mfd/altera-a10sr.txt
> @@ -18,6 +18,7 @@ The A10SR consists of these sub-devices:
> Device Description
> ------ ----------
> a10sr_gpio GPIO Controller
> +a10sr_monitor Register and State Monitoring
>
> Arria10 GPIO
> Required Properties:
> @@ -27,6 +28,10 @@ Required Properties:
> the second cell is used to specify flags.
> See ../gpio/gpio.txt for more information.
>
> +Arria10 Register and State Monitor
> +Required Properties:
> +- compatible : Should be "altr,a10sr-mon"
Why not "-monitor"?
> Example:
>
> resource-manager@0 {
> @@ -43,4 +48,8 @@ Example:
> gpio-controller;
> #gpio-cells = <2>;
> };
> +
> + a10sr_monitor {
> + compatible = "altr,a10sr-mon";
> + };
> };
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
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 3/4] mfd: altr-a10sr: Add Arria10 SR Monitor
From: Lee Jones @ 2016-10-26 12:52 UTC (permalink / raw)
To: tthayer
Cc: robh+dt, mark.rutland, dinguyen, inux, arnd, gregkh, davem, geert,
devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <1476394329-31696-4-git-send-email-tthayer@opensource.altera.com>
On Thu, 13 Oct 2016, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
>
> Add the Altera Arria10 DevKit System Resource Monitor functionality
> to the MFD device.
>
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
> drivers/mfd/altera-a10sr.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mfd/altera-a10sr.c b/drivers/mfd/altera-a10sr.c
> index 06e1f7f..0942d7d 100644
> --- a/drivers/mfd/altera-a10sr.c
> +++ b/drivers/mfd/altera-a10sr.c
> @@ -33,6 +33,10 @@
> .name = "altr_a10sr_gpio",
> .of_compatible = "altr,a10sr-gpio",
> },
> + {
> + .name = "altr_a10sr_mon",
> + .of_compatible = "altr,a10sr-mon",
"-monitor" would be better in my opinion.
> + },
> };
>
> static bool altr_a10sr_reg_readable(struct device *dev, unsigned int reg)
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] ARM: sti: stih410-clocks: Add PROC_STFE as a critical clock
From: Lee Jones @ 2016-10-26 13:00 UTC (permalink / raw)
To: Peter Griffin
Cc: linux-arm-kernel, linux-kernel, kernel, patrice.chotard,
devicetree
In-Reply-To: <20161026085620.GA5476@griffinp-ThinkPad-X1-Carbon-2nd>
> > > > > If the clock is enabled when Linux boots, the Linux clock framework *needs*
> > > > > to assume the hardware may have been used in previous boot stages, and it should
> > > > > not attempt to disable the clock.
> > > >
> > > > None of the boot loaders we use do this.
> > >
> > > But the Linux kernel isn't just used by us. It is not uncommon for STB
> > > bootloaders to get information from the frontend as part of the boot process.
> >
> > Okay, this is the clincher. Since we need to support non-standard
> > bootloaders, it's difficult to guarantee that the clock will be
> > disabled at boot. For this reason, I believe that we can call this a
> > critical clock.
> That's good news as the STi maintainer already acked and applied the patch.
Matters not. That's why we have `git rebase` and `git revert`. ;)
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 0/7] Add R8A7743/SK-RZG1M board support
From: Sergei Shtylyov @ 2016-10-26 13:23 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Simon Horman, Linux-Renesas, Rob Herring, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Magnus Damm,
Russell King,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <CAMuHMdUNgnYc1O7DATNH1r9SQ72eqtxwFvqcen08Tq+Er3LuvQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 10/26/2016 03:08 PM, Geert Uytterhoeven wrote:
>> Here's the set of 8 patches against Simon Horman's 'renesas.git' repo's
>> 'renesas-devel-20161021-v4.9-rc1' tag. I'm adding the device tree support for
>> the R8A7743-based SK-RZG1M board. The SoC is close to R8A7791 and the board
>> seems identical to the R8A7791/Porter board. The device tree patches depend on
>> the R8A7743 CPG/MSSR driver series just posted in order to compile and work.
>
> They depend only on "[PATCH v3 1/2] ARM: shmobile: r8a7743: add CPG clock
> index macros" of that series, right?
>
> "[PATCH v3 2/2] clk: renesas: cpg-mssr: add R8A7743 support" is not needed,
How would "clocks" props _work_ without this patch?
> and introduces an additional dependency on "[PATCH 1/3] clk: renesas:
> cpg-mssr: add common R-Car Gen2 support".
Yes, of course...
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergei
--
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 0/7] Add R8A7743/SK-RZG1M board support
From: Geert Uytterhoeven @ 2016-10-26 13:27 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Simon Horman, Linux-Renesas, Rob Herring, Mark Rutland,
devicetree@vger.kernel.org, Magnus Damm, Russell King,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1a85f851-29ec-1c14-8905-8b0645f5bb1c@cogentembedded.com>
On Wed, Oct 26, 2016 at 3:23 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 10/26/2016 03:08 PM, Geert Uytterhoeven wrote:
>>> Here's the set of 8 patches against Simon Horman's 'renesas.git'
>>> repo's
>>> 'renesas-devel-20161021-v4.9-rc1' tag. I'm adding the device tree support
>>> for
>>> the R8A7743-based SK-RZG1M board. The SoC is close to R8A7791 and the
>>> board
>>> seems identical to the R8A7791/Porter board. The device tree patches
>>> depend on
>>> the R8A7743 CPG/MSSR driver series just posted in order to compile and
>>> work.
>>
>>
>> They depend only on "[PATCH v3 1/2] ARM: shmobile: r8a7743: add CPG clock
>> index macros" of that series, right?
>>
>> "[PATCH v3 2/2] clk: renesas: cpg-mssr: add R8A7743 support" is not
>> needed,
>
> How would "clocks" props _work_ without this patch?
Sorry, I was focusing too much on "compile"...
Got my coke, switching brain to overdrive mode...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 1/2] of, numa: Add function to disable of_node_to_nid().
From: Robert Richter @ 2016-10-26 13:43 UTC (permalink / raw)
To: David Daney
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Frank Rowand,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Will Deacon,
Catalin Marinas, Hanjun Guo, Ganapatrao Kulkarni, Gilbert Netzer,
David Daney
In-Reply-To: <1477431061-7258-2-git-send-email-ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 25.10.16 14:31:00, David Daney wrote:
> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>
> On arm64 NUMA kernels we can pass "numa=off" on the command line to
> disable NUMA. A side effect of this is that kmalloc_node() calls to
> non-zero nodes will crash the system with an OOPS:
>
> [ 0.000000] [<fffffc00081bba84>] __alloc_pages_nodemask+0xa4/0xe68
> [ 0.000000] [<fffffc00082163a8>] new_slab+0xd0/0x57c
> [ 0.000000] [<fffffc000821879c>] ___slab_alloc+0x2e4/0x514
> [ 0.000000] [<fffffc000823882c>] __slab_alloc+0x48/0x58
> [ 0.000000] [<fffffc00082195a0>] __kmalloc_node+0xd0/0x2e0
> [ 0.000000] [<fffffc00081119b8>] __irq_domain_add+0x7c/0x164
> [ 0.000000] [<fffffc0008b75d30>] its_probe+0x784/0x81c
> [ 0.000000] [<fffffc0008b75e10>] its_init+0x48/0x1b0
> .
> .
> .
>
> This is caused by code like this in kernel/irq/irqdomain.c
>
> domain = kzalloc_node(sizeof(*domain) + (sizeof(unsigned int) * size),
> GFP_KERNEL, of_node_to_nid(of_node));
>
> When NUMA is disabled, the concept of a node is really undefined, so
> of_node_to_nid() should unconditionally return NUMA_NO_NODE.
>
> Add __of_force_no_numa() to allow of_node_to_nid() to be forced to
> return NUMA_NO_NODE.
>
> The follow on patch will call this new function from the arm64 numa
> code.
Didn't that work before? numa=off just maps all mem to node 0. If mem
allocation is requested for another node it should just fall back to a
node with mem (node 0 then). I suspect there is something wrong with
the page initialization, see:
http://www.spinics.net/lists/arm-kernel/msg535191.html
https://bugzilla.redhat.com/show_bug.cgi?id=1387793
What is the complete oops?
So I think k*alloc_node() must be able to handle requests to
non-existing nodes. Otherwise your fix is incomplete, assume a failed
of_numa_init() causing a dummy init but still some devices reporting a
node.
-Robert
--
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 01/10] mfd: Juniper PTXPMB CPLD Multi-function core driver
From: Lee Jones @ 2016-10-26 13:50 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Rob Herring, Linus Walleij, Alexandre Courbot, Mark Rutland,
Frank Rowand, Wolfram Sang, David Woodhouse, Brian Norris,
Wim Van Sebroeck, Guenter Roeck, Peter Rosin, Debjit Ghosh,
Georgi Vlaev, Guenter Roeck, JawaharBalaji Thirumalaisamy,
Rajat Jain, devicetree, linux-kernel, linux-gpio, linux-i2c,
linux-mtd
In-Reply-To: <1475853451-22121-2-git-send-email-pantelis.antoniou@konsulko.com>
On Fri, 07 Oct 2016, Pantelis Antoniou wrote:
> From: Guenter Roeck <groeck@juniper.net>
>
> Add Juniper's PTXPMB FPGA CPLD driver. Those FPGAs
> are present in Juniper's PTX series of routers.
>
> There are two variants, the original which is found on the
> PTXPMB_P2020, PTXPMB_P2020_SPMB based on a Freescale P2020 SoC,
> and PTXPMB_P5040 based on a Freescale P5040 SoC.
>
> The new variant NGPMB is present on a new line of x86 based
> boards (currently only the Gladiator FPC).
>
> Both variants provide a hardware watchdog, i2c mux and a
> gpio block, with the i2c mux block being different.
>
> Signed-off-by: Debjit Ghosh <dghosh@juniper.net>
> Signed-off-by: Georgi Vlaev <gvlaev@juniper.net>
> Signed-off-by: Guenter Roeck <groeck@juniper.net>
> Signed-off-by: JawaharBalaji Thirumalaisamy <jawaharb@juniper.net>
> Signed-off-by: Rajat Jain <rajatjain@juniper.net>
> Signed-off-by: Tom Kavanagh <tkavanagh@juniper.net>
> [Ported from Juniper kernel]
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
> drivers/mfd/Kconfig | 15 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/ptxpmb-cpld-core.c | 406 ++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/ptxpmb_cpld.h | 140 ++++++++++++++
> 4 files changed, 562 insertions(+)
> create mode 100644 drivers/mfd/ptxpmb-cpld-core.c
> create mode 100644 include/linux/mfd/ptxpmb_cpld.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 2caf7e9..438666a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1340,6 +1340,21 @@ config TWL4030_POWER
> and load scripts controlling which resources are switched off/on
> or reset when a sleep, wakeup or warm reset event occurs.
>
> +config MFD_JUNIPER_CPLD
> + tristate "Juniper PTX PMB CPLD"
> + depends on (PTXPMB_COMMON || JNX_PTX_NGPMB)
> + default y if (PTXPMB_COMMON || JNX_PTX_NGPMB)
Nit: I'd like to see the default line above 'depends|select'.
> + select MFD_CORE
> + select I2C_MUX_PTXPMB
> + select GPIO_PTXPMB_CPLD
> + select JNX_PTXPMB_WDT
I don't think these should be selected blindly from here.
> + help
> + Select this to enable the PTX PMB CPLD multi-function kernel driver
What is PTX PMB?
> + for the applicable Juniper platforms.
What applicable platforms? Might wish to consider removing this part.
It would also be nice to mention what functionality this provides and
little more info besides "this enables".
> + This driver can be built as a module. If built as a module it will be
> + called "ptxpmb-cpld"
> +
> config MFD_TWL4030_AUDIO
> bool "TI TWL4030 Audio"
> depends on TWL4030_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 2bf6a1a..62decc9 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -148,6 +148,7 @@ obj-$(CONFIG_AB3100_CORE) += ab3100-core.o
> obj-$(CONFIG_AB3100_OTP) += ab3100-otp.o
> obj-$(CONFIG_AB8500_DEBUG) += ab8500-debugfs.o
> obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
> +obj-$(CONFIG_MFD_JUNIPER_CPLD) += ptxpmb-cpld-core.o
Is there only ever going to be one Juniper CPLD?
> obj-$(CONFIG_MFD_DB8500_PRCMU) += db8500-prcmu.o
> # ab8500-core need to come after db8500-prcmu (which provides the channel)
> obj-$(CONFIG_AB8500_CORE) += ab8500-core.o ab8500-sysctrl.o
> diff --git a/drivers/mfd/ptxpmb-cpld-core.c b/drivers/mfd/ptxpmb-cpld-core.c
> new file mode 100644
> index 0000000..18e60a4
> --- /dev/null
> +++ b/drivers/mfd/ptxpmb-cpld-core.c
> @@ -0,0 +1,406 @@
> +/*
> + * Juniper PTX PMB CPLD multi-function core driver
Please expand on what a PTX PMB is.
> + * Copyright (C) 2012 Juniper Networks
This needs updating.
Why is this required now and not in the past 4 years?
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + *
Superfluous line.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/sched.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/dmi.h>
> +#include <linux/mfd/core.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/ptxpmb_cpld.h>
> +#include <linux/jnx/jnx-subsys.h>
> +#include <linux/jnx/board_ids.h>
Alphabetical.
> +struct pmb_cpld_core {
> + struct device *dev;
> + struct pmb_boot_cpld __iomem *cpld;
This isn't how we usually handle IO registers.
> + spinlock_t lock;
> + int irq;
> + wait_queue_head_t wqh;
This is not a very good variable name.
> +};
> +
> +static const struct of_device_id pmb_cpld_of_ids[] = {
> + { .compatible = "jnx,ptxpmb-cpld", .data = (void *)CPLD_TYPE_PTXPMB },
> + { .compatible = "jnx,ngpmb-bcpld", .data = (void *)CPLD_TYPE_NGPMB },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, pmb_cpld_of_ids);
Place this just before it's about to be used. Since you're matching
on this, I would expect it be be located just above .probe().
> +static struct dmi_system_id gld_2t_dmi_data[] = {
> + {
> + .ident = "Juniper Networks Gladiator 2T FPC",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Juniper Networks Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "0BF9"),
> + },
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(dmi, gld_2t_dmi_data);
> +
> +static struct dmi_system_id gld_3t_dmi_data[] = {
> + {
> + .ident = "Juniper Networks Gladiator 3T FPC",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Juniper Networks Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "0BFA"),
> + },
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(dmi, gld_3t_dmi_data);
> +
> +static int ptxpmb_cpld_get_master(void *data)
> +{
> + struct pmb_cpld_core *cpld = data;
> + u8 s1;
s1 is not a good variable name.
> + s1 = ioread8(&cpld->cpld->i2c_host_sel) & CPLD_I2C_HOST_MSTR_MASK;
Why aren't you using readb()?
> + if ((s1 & CPLD_I2C_HOST0_MSTR) == CPLD_I2C_HOST0_MSTR)
> + return 0;
Define this.
> + if ((s1 & CPLD_I2C_HOST1_MSTR) == CPLD_I2C_HOST1_MSTR)
> + return 1;
And this.
> + return -1;
Why aren't you using Linux error numbers.
> +}
> +
> +static int ngpmb_cpld_get_master(void *data)
> +{
> + struct pmb_cpld_core *cpld = data;
> +
> + if (ioread8(&cpld->cpld->baseboard_status1) & NGPMB_MASTER_SELECT)
> + return 1;
> + else
> + return 0;
> +}
> +
> +static irqreturn_t pmb_cpld_core_interrupt(int irq, void *dev_data)
> +{
> + struct pmb_cpld_core *cpld = dev_data;
> +
> + pr_info("pmb_cpld_core_interrupt %d\n", irq);
This should be dgb at most, but doesn't really have any place in
Mainline code. Please remove it.
> + spin_lock(&cpld->wqh.lock);
> +
> + /* clear interrupt, wake up any handlers */
> + wake_up_locked(&cpld->wqh);
> +
> + spin_unlock(&cpld->wqh.lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static struct resource pmb_cpld_resources[] = {
> + {
> + .start = 0,
> + .end = sizeof(struct pmb_boot_cpld) - 1,
Define these properly.
> + .flags = IORESOURCE_MEM,
> + },
> +};
Use the DEFINE_RES_*() helpers.
> +static struct mfd_cell pmb_cpld_cells[] = {
> + {
> + .name = "jnx-ptxpmb-wdt",
> + .num_resources = ARRAY_SIZE(pmb_cpld_resources),
> + .resources = pmb_cpld_resources,
> + .of_compatible = "jnx,ptxpmb-wdt",
> + },
> + {
> + .name = "i2c-mux-ptxpmb-cpld",
> + .num_resources = ARRAY_SIZE(pmb_cpld_resources),
> + .resources = pmb_cpld_resources,
> + .of_compatible = "jnx,i2c-mux-ptxpmb-cpld",
> + },
> + {
> + .name = "gpio-ptxpmb-cpld",
> + .num_resources = ARRAY_SIZE(pmb_cpld_resources),
> + .resources = pmb_cpld_resources,
> + .of_compatible = "jnx,gpio-ptxpmb-cpld",
> + },
> +};
> +
> +static struct mfd_cell ngpmb_cpld_cells[] = {
> + {
> + .name = "jnx-ptxpmb-wdt",
> + .num_resources = ARRAY_SIZE(pmb_cpld_resources),
> + .resources = pmb_cpld_resources,
> + .of_compatible = "jnx,ptxpmb-wdt",
> + },
> + {
> + .name = "i2c-mux-ngpmb-bcpld",
> + .num_resources = ARRAY_SIZE(pmb_cpld_resources),
> + .resources = pmb_cpld_resources,
> + .of_compatible = "jnx,i2c-mux-ngpmb-bcpld",
> + },
> + {
> + .name = "gpio-ptxpmb-cpld",
> + .num_resources = ARRAY_SIZE(pmb_cpld_resources),
> + .resources = pmb_cpld_resources,
> + .of_compatible = "jnx,gpio-ptxpmb-cpld",
> + },
> +};
> +
> +static void cpld_ngpmb_init(struct pmb_cpld_core *cpld,
> + struct jnx_chassis_info *chinfo,
> + struct jnx_card_info *cinfo)
> +{
> + u8 s1, s2, val, chassis;
s1 and s1 are not good variable names.
status1 and status2 would be better.
> + s1 = ioread8(&cpld->cpld->baseboard_status1);
> + s2 = ioread8(&cpld->cpld->baseboard_status2);
> + chassis = (ioread8(&cpld->cpld->board.ngpmb.chassis_type)
> + & NGPMB_CHASSIS_TYPE_MASK) >> NGPMB_CHASSIS_TYPE_LSB;
> +
> + dev_info(cpld->dev, "Revision 0x%02X chassis type %s (0x%02X)\n",
> + ioread8(&cpld->cpld->cpld_rev),
> + chassis == NGPMB_CHASSIS_TYPE_POLARIS ? "PTX-1000" :
> + chassis == NGPMB_CHASSIS_TYPE_HENDRICKS ? "PTX-3000" :
> + "Unknown", chassis);
> +
> + /* Only the Gladiator 2t/3t FPC */
> + if (dmi_check_system(gld_2t_dmi_data) ||
> + dmi_check_system(gld_3t_dmi_data)) {
> + /* Take SAM FPGA out of reset */
> + val = ioread8(&cpld->cpld->gpio_2);
> + iowrite8(val | NGPMB_GPIO2_TO_BASEBRD_LSB, &cpld->cpld->gpio_2);
> + mdelay(10);
> + } else {
> + /*
> + * Get the PAM FPGA out of reset,
> + * and wait for 100ms as per HW manual
> + */
> + val = ioread8(&cpld->cpld->reset);
> + iowrite8(val & ~NGPMB_PCIE_OTHER_RESET, &cpld->cpld->reset);
> + mdelay(100);
> + }
> +
> + /* No Card / Chassis info needed in stand alone mode */
> + if (!(s1 & NGPMB_PMB_STANDALONE) || !(s1 & NGPMB_BASEBRD_STANDALONE))
> + return;
> +
> + cinfo->type = JNX_BOARD_TYPE_FPC;
> + cinfo->slot = (s1 & NGPMB_BASEBRD_SLOT_MASK) >> NGPMB_BASEBRD_SLOT_LSB;
> +
> + if (((s2 & NGPMB_BASEBRD_TYPE_MASK) >> NGPMB_BASEBRD_TYPE_LSB) !=
> + NGPMB_BASEBRD_TYPE_MX) {
> + if (dmi_check_system(gld_2t_dmi_data))
> + cinfo->assembly_id = JNX_ID_GLD_2T_FPC;
> + else if (dmi_check_system(gld_3t_dmi_data))
> + cinfo->assembly_id = JNX_ID_GLD_3T_FPC;
> + else
> + cinfo->assembly_id = JNX_ID_POLARIS_MLC;
> + }
> +
> + /*
> + * Multi chassis configuration. These bits are not
> + * valid for Gladiator.
> + */
> + if (!(dmi_check_system(gld_2t_dmi_data) ||
> + dmi_check_system(gld_3t_dmi_data))) {
> + if (ioread8(&cpld->cpld->board.ngpmb.sys_config) &
> + NGPMB_SYS_CONFIG_MULTI_CHASSIS) {
> + chinfo->multichassis = 1;
"true"?
> + chinfo->chassis_no =
"no" is ambiguous.
chassis_id?
> + ioread8(&cpld->cpld->board.ngpmb.chassis_id);
> + }
> + }
> +
> + switch (chassis) {
> + case NGPMB_CHASSIS_TYPE_POLARIS:
> + chinfo->platform = JNX_PRODUCT_POLARIS;
> + break;
> + case NGPMB_CHASSIS_TYPE_HENDRICKS:
> + chinfo->platform = JNX_PRODUCT_HENDRICKS;
> + break;
> + default:
> + chinfo->platform = 0;
Define this.
> + break;
> + };
> + chinfo->get_master = ngpmb_cpld_get_master;
Where is get_master() called from?
I think you're probably better off exporting the call, rather than
using call-backs.
> +}
> +
> +static void cpld_ptxpmb_init(struct pmb_cpld_core *cpld,
> + struct jnx_chassis_info *chinfo,
> + struct jnx_card_info *cinfo)
> +{
> + u8 s1, s2;
> +
> + s1 = ioread8(&cpld->cpld->baseboard_status1);
> + s2 = ioread8(&cpld->cpld->baseboard_status2);
> +
> + dev_info(cpld->dev, "Revision 0x%02x carrier type 0x%x [%s]\n",
> + ioread8(&cpld->cpld->cpld_rev), s2 & 0x1f,
> + (s1 & 0X3F) == 0X1F ? "standalone"
> + : (s2 & 0x10) ? "FPC" : "SPMB");
> +
> + if ((s1 & 0x3f) != 0x1f) { /* not standalone */
Define these, and all the other magic numbers in this driver.
I won't say "define this" again in this review.
> + cinfo->slot = s1 & 0x0f;
> + if (s2 & 0x10) { /* fpc */
Pointless comment. What is an FPC?
> + cinfo->type = JNX_BOARD_TYPE_FPC;
> + switch (s2 & 0x0f) {
> + case 0x00: /* Sangria */
> + cinfo->assembly_id = JNX_ID_SNG_VDV_BASE_P2;
> + chinfo->platform = JNX_PRODUCT_SANGRIA;
> + break;
> + case 0x01: /* Tiny */
> + chinfo->platform = JNX_PRODUCT_TINY;
> + break;
> + case 0x02: /* Hercules */
> + chinfo->platform = JNX_PRODUCT_HERCULES;
> + break;
> + case 0x03: /* Hendricks */
> + cinfo->assembly_id = JNX_ID_HENDRICKS_FPC_P2;
> + chinfo->platform = JNX_PRODUCT_HENDRICKS;
> + break;
> + default: /* unknown */
> + break;
> + }
> + } else { /* spmb */
> + cinfo->type = JNX_BOARD_TYPE_SPMB;
> + switch (s2 & 0x0f) {
> + case 0x00: /* Sangria */
> + cinfo->assembly_id = JNX_ID_SNG_PMB;
> + chinfo->platform = JNX_PRODUCT_SANGRIA;
> + break;
> + default: /* unknown */
> + break;
> + }
> + }
> + }
> + chinfo->get_master = ptxpmb_cpld_get_master;
> +}
> +
> +static int pmb_cpld_core_probe(struct platform_device *pdev)
> +{
> + static struct pmb_cpld_core *cpld;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct ptxpmb_mux_data *pdata = dev->platform_data;
> + int i, error, mfd_size;
> + int cpld_type = CPLD_TYPE_PTXPMB;
> + const struct of_device_id *match;
> + struct mfd_cell *mfd_cells;
> +
> + struct jnx_chassis_info chinfo = {
> + .chassis_no = 0,
> + .multichassis = 0,
> + .master_data = NULL,
> + .platform = -1,
> + .get_master = NULL,
> + };
> + struct jnx_card_info cinfo = {
> + .type = JNX_BOARD_TYPE_UNKNOWN,
> + .slot = -1,
> + .assembly_id = -1,
> + };
> +
> + cpld = devm_kzalloc(dev, sizeof(*cpld), GFP_KERNEL);
> + if (!cpld)
> + return -ENOMEM;
> +
> + cpld->dev = dev;
> + dev_set_drvdata(dev, cpld);
> +
> + if (pdata) {
> + cpld_type = pdata->cpld_type;
> + } else {
> + match = of_match_device(pmb_cpld_of_ids, dev);
> + if (match)
> + cpld_type = (int)(unsigned long)match->data;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + cpld->cpld = devm_ioremap_resource(dev, res);
> + if (IS_ERR(cpld->cpld))
> + return PTR_ERR(cpld->cpld);
cpld->cpld is confusing. Please rename.
> + chinfo.master_data = cpld;
> +
> + cpld->irq = platform_get_irq(pdev, 0);
> + if (cpld->irq >= 0) {
> + error = devm_request_threaded_irq(dev, cpld->irq, NULL,
> + pmb_cpld_core_interrupt,
> + IRQF_TRIGGER_RISING |
> + IRQF_ONESHOT,
> + dev_name(dev), cpld);
> + if (error < 0)
> + return error;
> + }
> +
> + spin_lock_init(&cpld->lock);
> + init_waitqueue_head(&cpld->wqh);
> +
> + mfd_cells = pmb_cpld_cells;
> + mfd_size = ARRAY_SIZE(pmb_cpld_cells);
> +
> + switch (cpld_type) {
> + case CPLD_TYPE_PTXPMB:
> + cpld_ptxpmb_init(cpld, &chinfo, &cinfo);
> + break;
> + case CPLD_TYPE_NGPMB:
> + cpld_ngpmb_init(cpld, &chinfo, &cinfo);
> + mfd_cells = ngpmb_cpld_cells;
> + mfd_size = ARRAY_SIZE(ngpmb_cpld_cells);
> + break;
Are other types supported?
> + }
> +
> + if (pdata) {
> + for (i = 0; i < mfd_size; i++) {
> + mfd_cells[i].platform_data = pdata;
> + mfd_cells[i].pdata_size = sizeof(*pdata);
> + }
> + }
> +
> + error = mfd_add_devices(dev, pdev->id, mfd_cells,
The use of pdev->id here is unusual.
Are you sure this is what you want?
> + mfd_size, res, 0, NULL);
> + if (error < 0)
> + return error;
> +
> + jnx_register_chassis(&chinfo);
> + jnx_register_local_card(&cinfo);
> +
> + return 0;
> +}
> +
> +static int pmb_cpld_core_remove(struct platform_device *pdev)
> +{
> + jnx_unregister_local_card();
> + jnx_unregister_chassis();
> + mfd_remove_devices(&pdev->dev);
'\n' here.
> + return 0;
> +}
> +
> +static struct platform_driver pmb_cpld_core_driver = {
> + .probe = pmb_cpld_core_probe,
> + .remove = pmb_cpld_core_remove,
> + .driver = {
> + .name = "ptxpmb-cpld",
> + .of_match_table = pmb_cpld_of_ids,
> + .owner = THIS_MODULE,
Remove this line.
> + }
> +};
> +
Remove this line.
> +module_platform_driver(pmb_cpld_core_driver);
> +
> +MODULE_DESCRIPTION("Juniper PTX PMB CPLD Core Driver");
> +MODULE_AUTHOR("Guenter Roeck <groeck@juniper.net>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:ptxpmb-cpld");
> diff --git a/include/linux/mfd/ptxpmb_cpld.h b/include/linux/mfd/ptxpmb_cpld.h
> new file mode 100644
> index 0000000..e44afb4
> --- /dev/null
> +++ b/include/linux/mfd/ptxpmb_cpld.h
> @@ -0,0 +1,140 @@
> +/*---------------------------------------------------------------------------
> + *
> + * ptxpmb_cpld_core.h
> + * Copyright (c) 2012 Juniper Networks
> + *
> + *---------------------------------------------------------------------------
> + */
This it not how we write header comments.
> +#ifndef PTXPMB_CPLD_CORE_H
> +#define PTXPMB_CPLD_CORE_H
__MFD_*
> +struct pmb_boot_cpld {
> + u8 cpld_rev; /* 0x00 */
> + u8 reset;
> +#define CPLD_MAIN_RESET (1 << 0)
Use BIT() for all these "1 <<"s
> +#define CPLD_PHYCB_RESET (1 << 1)
> +#define CPLD_PHYSW_RESET (1 << 2) /* P2020 only */
> +#define NGPMB_PCIE_OTHER_RESET (1 << 3) /* PAM reset on MLC */
> + u8 reset_reason;
> +#define NGPMB_REASON_MON_A_FAIL (1 << 0)
> +#define NGPMB_REASON_WDT1 (1 << 1)
> +#define NGPMB_REASON_WDT2 (1 << 2)
> +#define NGPMB_REASON_WDT3 (1 << 3)
> +#define NGPMB_REASON_WDT4 (1 << 4)
> +#define NGPMB_REASON_RE_HRST (1 << 5)
> +#define NGPMB_REASON_PWR_ON (1 << 6)
> +#define NGPMB_REASON_RE_SRST (1 << 7)
> + u8 control;
> +#define CPLD_CONTROL_BOOTED_LED (1 << 0)
> +#define CPLD_CONTROL_WATCHDOG (1 << 6)
> +#define CPLD_CONTROL_RTC (1 << 7)
> +#define NGPMB_FLASH_SELECT (1 << 4)
> +#define NGPMB_FLASH_SWIZZ_ENA (1 << 5)
> + u8 sys_timer_cnt;
> + u8 watchdog_hbyte;
> + u8 watchdog_lbyte;
> + u8 unused1[1];
> + u8 baseboard_status1; /* 0x08 */
> +#define NGPMB_PMB_STANDALONE (1 << 0)
> +#define NGPMB_MASTER_SELECT (1 << 1)
> +#define NGPMB_BASEBRD_STANDALONE (1 << 2)
> +#define NGPMB_BASEBRD_SLOT_LSB 3
> +#define NGPMB_BASEBRD_SLOT_MASK 0xF8
> + u8 baseboard_status2;
> +#define NGPMB_BASEBRD_TYPE_LSB 5
> +#define NGPMB_BASEBRD_TYPE_MASK 0xE0
> +#define NGPMB_BASEBRD_TYPE_MX 0
> + u8 chassis_number;
> + u8 sys_config;
> + u8 i2c_group_sel; /* 0x0c */
> + u8 i2c_group_en;
> + u8 unused2[4];
> + u8 timer_irq_st; /* 0x12 */
> + u8 timer_irq_en;
> + u8 unused3[12];
> + u8 prog_jtag_control; /* 0x20 */
> + u8 gp_reset1; /* 0x21 */
> +#define CPLD_GP_RST1_PCISW (1 << 0)
> +#define CPLD_GP_RST1_SAM (1 << 1)
> +#define CPLD_GP_RST1_BRCM (1 << 2)
> + u8 gp_reset2; /* 0x22 */
> + u8 phy_control;
> + u8 gpio_1;
> + u8 gpio_2;
> +#define NGPMB_GPIO2_TO_BASEBRD_LSB (1 << 3)
> +#define NGPMB_I2C_GRP_SEL_LSB 0
> +#define NGPMB_I2C_GRP_SEL_MASK 0x03
> + u8 thermal_status;
> + u8 i2c_host_sel;
> +#define CPLD_I2C_HOST0_MSTR 0x09
> +#define CPLD_I2C_HOST1_MSTR 0x06
> +#define CPLD_I2C_HOST_MSTR_MASK 0x0f
> + u8 scratch[3];
> + u8 misc_status;
> + u8 i2c_bus_control; /* 0x2c */
> + union {
> + struct {
> + u8 mezz_present;
> + u8 unused1[4];
> + u8 i2c_group_sel_dbg; /* 0x31 */
> + u8 i2c_group_en_dbg; /* 0x32 */
> + u8 i2c_group_sel_force; /* 0x33 */
> + u8 i2c_group_en_force; /* 0x34 */
> + u8 unused2[0x4b];
> + } p2020;
> + struct {
> + u8 hdk_minor_version; /* 0x2d */
> + u8 hdk_feature_ind;
> + u8 hdk_pmb_srds_mode;
> + u8 hdk_pwr_fail_status;
> + u8 hdk_pmb_pwr_status;
> + u8 hdk_pmb_mezz_status;
> + u8 cpld_self_reset; /* 0x33 */
> + u8 unused[0x4c];
> + u8 hdk_bcpld_rcw[80];
> + } p5020;
> + struct {
> + u8 unused[3];
> + u8 chassis_id; /* 0x30 */
> + u8 chassis_type; /* 0x31 */
> +#define NGPMB_CHASSIS_TYPE_LSB 0
> +#define NGPMB_CHASSIS_TYPE_MASK 0x0F
> +#define NGPMB_CHASSIS_TYPE_POLARIS 0x0B
> +#define NGPMB_CHASSIS_TYPE_HENDRICKS 0x09
> + u8 sys_config; /* 0x32 */
> +#define NGPMB_SYS_CONFIG_MULTI_CHASSIS 0x01
> + } ngpmb;
> + struct {
> + u8 nv_win; /* 0x2d */
> + u8 nv_addr1;
> + u8 nv_addr2;
> + u8 nv_wr_data;
> + u8 nv_rd_data;
> + u8 nv_cmd;
> + u8 nv_done_bit;
> + } nvram;
> + } board;
> +};
Please use the standard conventions for detailing registers and bits.
> +#ifdef CONFIG_P2020_PTXPMB
> +#define CPLD_PHY_RESET (CPLD_PHYCB_RESET | CPLD_PHYSW_RESET)
> +#else
> +#define CPLD_PHY_RESET CPLD_PHYCB_RESET
> +#endif
> +
> +#define i2c_group_sel_force board.p2020.i2c_group_sel_force
> +#define i2c_group_en_force board.p2020.i2c_group_en_force
I've never seen this before. Please don't do it.
> +struct ptxpmb_mux_data {
> + int cpld_type;
> +#define CPLD_TYPE_PTXPMB 0 /* SPMB / Sangria FPC / Hendricks FPC */
> +#define CPLD_TYPE_NGPMB 1 /* MLC / Stout / Gladiator... */
> + int num_enable; /* Number of I2C enable pins */
> + int num_channels; /* Number of I2C channels used in a mux chip */
> + int parent_bus_num; /* parent i2c bus number */
> + int base_bus_num; /* 1st bus number, 0 if undefined */
> + bool use_force; /* Use i2c force registers if true */
> +};
Rename to *_mux_pdata for clarity.
Use kdoc throughout.
> +#endif /* PTXPMB_CPLD_CORE_H */
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] soc: qcom: Add SoC info driver
From: Imran Khan @ 2016-10-26 13:50 UTC (permalink / raw)
To: Arnd Bergmann
Cc: andy.gross, David Brown, Rob Herring, Mark Rutland,
open list:ARM/QUALCOMM SUPPORT, open list:ARM/QUALCOMM SUPPORT,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
In-Reply-To: <3567809.JctLPjIDdk@wuerfel>
On 10/26/2016 2:19 AM, Arnd Bergmann wrote:
> On Tuesday, October 25, 2016 3:23:34 PM CEST Imran Khan wrote:
>> On 10/21/2016 4:03 PM, Arnd Bergmann wrote:
>>>> +/* socinfo: sysfs functions */
>>>
>>> This seems overly verbose, having both raw and human-readable
>>> IDs is generally not necessary, pick one of the two. If you
>>> need any fields that we don't already support in soc_device,
>>> let's talk about adding them to the generic structure.
>>>
>>>
>>
>> Okay. I will go for human readable IDs. Can we add 2 more fields
>> in the generic structure.
>> These 2 fields would be:
>>
>> vendor: A string for vendor name
>> serial_number: A string containing serial number for the platform
>
>
> serial_number seems straightforward, adding this seems like a good
> idea. I don't understand yet what would go into the vendor field
> though. For this particular driver, is it always "Qualcomm", or
> would it be a third-party that makes a device based on that chip?
>
As we are talking about generic soc_device_attribute fields, I was hoping that
having a vendor field would be helpful as along with family it would provide
a more thorough information. Also as more than one foundries may be used for
a soc, can we have a field say foundry_id to provide this information.
> Arnd
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH 7/7] mfd: tps65217: Fix mismatched interrupt number
From: Lee Jones @ 2016-10-26 13:56 UTC (permalink / raw)
To: Milo Kim
Cc: bcousson-rdvid1DuHRBWk0Htik3J/w, Tony Lindgren,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161021140106.21531-8-woogyom.kim-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Fri, 21 Oct 2016, Milo Kim wrote:
> Enum value of 'tps65217_irq_type' is not matched with DT parsed hwirq
> number[*].
>
> The MFD driver gets the IRQ data by referencing hwirq, but the value is
> different. So, irq_to_tps65217_irq() returns mismatched IRQ data.
> Eventually, the power button driver enables not PB but USB interrupt
> when it is probed.
>
> According to the TPS65217 register map[**], USB interrupt is the LSB.
> This patch synchronizes TPS65217 IRQ index.
>
> [*] include/dt-bindings/mfd/tps65217.h
> [**] http://www.ti.com/lit/ds/symlink/tps65217.pdf
>
> Signed-off-by: Milo Kim <woogyom.kim-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> include/linux/mfd/tps65217.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mfd/tps65217.h b/include/linux/mfd/tps65217.h
> index 4ccda89..75a3a5f 100644
> --- a/include/linux/mfd/tps65217.h
> +++ b/include/linux/mfd/tps65217.h
> @@ -235,9 +235,9 @@ struct tps65217_bl_pdata {
> };
>
> enum tps65217_irq_type {
> - TPS65217_IRQ_PB,
> - TPS65217_IRQ_AC,
> TPS65217_IRQ_USB,
> + TPS65217_IRQ_AC,
> + TPS65217_IRQ_PB,
> TPS65217_NUM_IRQ
> };
This is why using enum for these types of assignments is sometimes
dangerous. It's probably best to be explicit.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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 2/2] phy: qcom-qmp: new qmp phy driver for qcom-chipsets
From: Srinivas Kandagatla @ 2016-10-26 13:57 UTC (permalink / raw)
To: Vivek Gautam, kishon-l0cyMroinI0, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1476873827-7191-3-git-send-email-vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Hi Vivek,
Thanks for consolidating qmp phy support for both usb and pcie.
On 19/10/16 11:43, Vivek Gautam wrote:
> Qualcomm SOCs have QMP phy controller that provides support
> to a number of controller, viz. PCIe, UFS, and USB.
> Add a new driver, based on generic phy framework, for this
> phy controller.
>
> USB3-phy changes: Based on phy-msm-ssusb-qmp driver available on
> msm-4.4 kernel @codeaurora[1].
> PCIe-phy changes: Based on msm8996-pcie-phy driver posted by
> Srinivas [2].
>
> [1] https://source.codeaurora.org/quic/la/kernel/msm-4.4/log/?h=caf/3.18/msm-3.18
> [2] https://patchwork.kernel.org/patch/9318947/
>
> Signed-off-by: Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Cc: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
very few minor nits which needs fixing in next version.
I have tested this patch on DB820c PCIE with ethernet and SATA ports.
Tested-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> .../devicetree/bindings/phy/qcom-qmp-phy.txt | 61 ++
> drivers/phy/Kconfig | 8 +
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-qcom-qmp.c | 1154 ++++++++++++++++++++
> 4 files changed, 1224 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> create mode 100644 drivers/phy/phy-qcom-qmp.c
>
> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> new file mode 100644
> index 0000000..90214aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> @@ -0,0 +1,61 @@
> +Qualcomm QMP PHY
> +----------------
> +
> +QMP phy controller supports physical layer functionality for a number of
> +controllers on Qualcomm chipsets, such as, PCIe, UFS, and USB.
> +
> +Required properties:
> + - compatible: compatible list, contains:
> + "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
> + "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996.
> + - reg: offset and length of the PHY register set.
> + - #phy-cells: must be 1
> + - Cell after phy phandle should be the port (lane) number.
> + - clocks: a list of phandles and clock-specifier pairs,
> + one for each entry in clock-names.
> + - clock-names: must be "cfg_ahb" for phy config clock,
> + "aux" for phy aux clock,
> + "ref_clk" for 19.2 MHz ref clk,
> + "ref_clk_src" for reference clock source,
> + "pipe<port-number>" for pipe clock specific to
> + each port/lane (Optional).
> + - resets: a list of phandles and reset controller specifier pairs,
> + one for each entry in reset-names.
> + - reset-names: must be "phy" for reset of phy block,
> + "common" for phy common block reset,
> + "cfg" for phy's ahb cfg block reset (Optional).
> + "port<port-number>" for reset specific to
> + each port/lane. (Optional)
> + - vdda-phy-supply: Phandle to a regulator supply to PHY core block.
> + - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
> +
> +Optional properties:
> + - vddp-ref-clk-supply: Phandle to a regulator supply to any specific refclk
> + pll block.
> +
> +Example:
> + pcie_phy: pciephy@34000 {
> + compatible = "qcom,qmp-14nm-pcie-phy";
Fix this according to the compatibilities listed above.
> + reg = <0x034000 0x3fff>;
> + #phy-cells = <1>;
> +
> + clocks = <&gcc GCC_PCIE_PHY_AUX_CLK>,
> + <&gcc GCC_PCIE_PHY_CFG_AHB_CLK>,
> + <&gcc GCC_PCIE_0_PIPE_CLK>,
> + <&gcc GCC_PCIE_1_PIPE_CLK>,
> + <&gcc GCC_PCIE_2_PIPE_CLK>;
> + clock-names = "aux", "cfg_ahb",
> + "pipe0", "pipe1", "pipe2";
> +
> + vdda-phy-supply = <&pm8994_l28>;
> + vdda-pll-supply = <&pm8994_l12>;
> +
> + resets = <&gcc GCC_PCIE_PHY_BCR>,
> + <&gcc GCC_PCIE_PHY_COM_BCR>,
> + <&gcc GCC_PCIE_PHY_COM_NOCSR_BCR>,
> + <&gcc GCC_PCIE_0_PHY_BCR>,
> + <&gcc GCC_PCIE_1_PHY_BCR>,
> + <&gcc GCC_PCIE_2_PHY_BCR>;
> + reset-names = "phy", "common", "cfg",
> + "lane0", "lane1", "lane2";
> + };
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 5547984..d5e2b50f 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -446,6 +446,14 @@ config PHY_STIH41X_USB
> Enable this to support the USB transceiver that is part of
> STMicroelectronics STiH41x SoC series.
>
> +config PHY_QCOM_QMP
> + tristate "Qualcomm QMP PHY Driver"
> + depends on OF && (ARCH_QCOM || COMPILE_TEST)
> + select GENERIC_PHY
> + help
> + Enable this to support the QMP PHY transceiver that is used
> + with controllers such as PCIe, UFS, and USB on Qualcomm chips.
> +
> config PHY_QCOM_QUSB2
> tristate "Qualcomm QUSB2 PHY Driver"
> depends on OF && (ARCH_QCOM || COMPILE_TEST)
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 848489d..fde9fba 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY) += phy-spear1340-miphy.o
> obj-$(CONFIG_PHY_XGENE) += phy-xgene.o
> obj-$(CONFIG_PHY_STIH407_USB) += phy-stih407-usb.o
> obj-$(CONFIG_PHY_STIH41X_USB) += phy-stih41x-usb.o
> +obj-$(CONFIG_PHY_QCOM_QMP) += phy-qcom-qmp.o
> obj-$(CONFIG_PHY_QCOM_QUSB2) += phy-qcom-qusb2.o
> obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs.o
> obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs-qmp-20nm.o
> diff --git a/drivers/phy/phy-qcom-qmp.c b/drivers/phy/phy-qcom-qmp.c
> new file mode 100644
> index 0000000..7e89179
> --- /dev/null
> +++ b/drivers/phy/phy-qcom-qmp.c
> @@ -0,0 +1,1154 @@
> +/*
> + * Copyright (c) 2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only 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.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#include <dt-bindings/phy/phy.h>
We would not need this once you have proper flags in cfg.
> +
> +
> +static inline void qphy_setbits(u32 bitmask, void __iomem *reg)
> +{
> + u32 val;
> +
> + val = readl_relaxed(reg);
> + val |= bitmask;
> + writel_relaxed(val, reg);
> +}
> +
> +static inline void qphy_clrbits(u32 bitmask, void __iomem *reg)
> +{
> + u32 val;
> +
> + val = readl_relaxed(reg);
> + val &= ~bitmask;
> + writel_relaxed(val, reg);
> +}
> +
> +unsigned int msm8996_pciephy_tx_offsets[] = { 0x1000, 0x2000, 0x3000 };
> +unsigned int msm8996_pciephy_rx_offsets[] = { 0x1200, 0x2200, 0x3200 };
> +unsigned int msm8996_pciephy_pcs_offsets[] = { 0x1400, 0x2400, 0x3400 };
> +
> +unsigned int msm8996_usb3phy_tx_offsets[] = { 0x200 };
> +unsigned int msm8996_usb3phy_rx_offsets[] = { 0x400 };
> +unsigned int msm8996_usb3phy_pcs_offsets[] = { 0x600 };
> +
> +const struct qmp_phy_init_cfg pciephy_init_cfg = {
> + .type = PHY_TYPE_PCIE,
> + .nlanes = 3,
> + .tx_offsets = msm8996_pciephy_tx_offsets,
> + .rx_offsets = msm8996_pciephy_rx_offsets,
> + .pcs_offsets = msm8996_pciephy_pcs_offsets,
> +
> + .phy_init_serdes_tbl = pciephy_serdes_init_tbl,
> + .phy_init_serdes_tbl_sz = ARRAY_SIZE(pciephy_serdes_init_tbl),
> + .phy_init_tx_tbl = pciephy_tx_init_tbl,
> + .phy_init_tx_tbl_sz = ARRAY_SIZE(pciephy_tx_init_tbl),
> + .phy_init_rx_tbl = pciephy_rx_init_tbl,
> + .phy_init_rx_tbl_sz = ARRAY_SIZE(pciephy_rx_init_tbl),
> + .phy_init_pcs_tbl = pciephy_pcs_init_tbl,
> + .phy_init_pcs_tbl_sz = ARRAY_SIZE(pciephy_pcs_init_tbl),
> + .has_phy_com_ctrl = true,
> + .regs = pciephy_regs_layout,
> + .mask_start_ctrl = (PHY_PCS_START | PHY_PLL_READY_GATE_EN),
> + .mask_pwr_dn_ctrl = (PHY_SW_PWRDN | PHY_REFCLK_DRV_DSBL),
> +};
> +
> +const struct qmp_phy_init_cfg usb3phy_init_cfg = {
> + .type = PHY_TYPE_USB3,
> + .nlanes = 1,
> + .tx_offsets = msm8996_usb3phy_tx_offsets,
> + .rx_offsets = msm8996_usb3phy_rx_offsets,
> + .pcs_offsets = msm8996_usb3phy_pcs_offsets,
> +
> + .phy_init_serdes_tbl = usb3phy_serdes_init_tbl,
> + .phy_init_serdes_tbl_sz = ARRAY_SIZE(usb3phy_serdes_init_tbl),
> + .phy_init_tx_tbl = usb3phy_tx_init_tbl,
> + .phy_init_tx_tbl_sz = ARRAY_SIZE(usb3phy_tx_init_tbl),
> + .phy_init_rx_tbl = usb3phy_rx_init_tbl,
> + .phy_init_rx_tbl_sz = ARRAY_SIZE(usb3phy_rx_init_tbl),
> + .phy_init_pcs_tbl = usb3phy_pcs_init_tbl,
> + .phy_init_pcs_tbl_sz = ARRAY_SIZE(usb3phy_pcs_init_tbl),
> + .regs = usb3phy_regs_layout,
> + .mask_start_ctrl = (PHY_SERDES_START | PHY_PCS_START),
> + .mask_pwr_dn_ctrl = PHY_SW_PWRDN,
> +};
> +
> +static void qcom_qmp_phy_configure(void __iomem *base,
> + unsigned int *regs_layout,
> + struct qmp_phy_init_tbl init_tbl[],
> + int init_tbl_sz)
> +{
> + int i;
> +
> + for (i = 0; i < init_tbl_sz; i++) {
> + if (init_tbl[i].in_layout)
> + writel_relaxed(init_tbl[i].cfg_val,
> + base + regs_layout[init_tbl[i].reg_offset]);
> + else
> + writel_relaxed(init_tbl[i].cfg_val,
> + base + init_tbl[i].reg_offset);
> + }
> +
> + /* flush buffered writes */
> + mb();
> +}
> +
> +static int qcom_qmp_phy_poweron(struct phy *phy)
> +{
> + struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
> + struct qcom_qmp_phy *qphy = phydesc->qphy;
> + int ret;
> +
> + dev_info(&phy->dev, "Powering on QMP phy\n");
> +
> + ret = regulator_enable(qphy->vdda_phy);
> + if (ret) {
> + dev_err(qphy->dev, "%s: vdda-phy enable failed, err=%d\n",
> + __func__, ret);
> + return ret;
> + }
> +
> + ret = regulator_enable(qphy->vdda_pll);
> + if (ret) {
> + dev_err(qphy->dev, "%s: vdda-pll enable failed, err=%d\n",
> + __func__, ret);
> + goto err_vdda_pll;
> + }
> +
> + if (qphy->vddp_ref_clk) {
> + ret = regulator_enable(qphy->vddp_ref_clk);
> + if (ret) {
> + dev_err(qphy->dev, "%s: vdda-ref-clk enable failed, err=%d\n",
> + __func__, ret);
> + goto err_vddp_refclk;
> + }
> + }
> +
> + if (!qphy->clk_enabled) {
> + clk_prepare_enable(qphy->ref_clk_src);
> + clk_prepare_enable(qphy->ref_clk);
> + clk_prepare_enable(phydesc->pipe_clk);
> + qphy->clk_enabled = true;
> + }
> +
> + return 0;
> +
> +err_vddp_refclk:
> + regulator_disable(qphy->vdda_pll);
> +err_vdda_pll:
> + regulator_disable(qphy->vdda_phy);
> + return ret;
> +}
> +
> +static int qcom_qmp_phy_poweroff(struct phy *phy)
> +{
> + struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
> + struct qcom_qmp_phy *qphy = phydesc->qphy;
> +
> + if (qphy->clk_enabled) {
> + clk_disable_unprepare(qphy->ref_clk_src);
> + clk_disable_unprepare(qphy->ref_clk);
> + clk_disable_unprepare(phydesc->pipe_clk);
> + qphy->clk_enabled = false;
> + }
> +
> + if (qphy->vddp_ref_clk)
> + regulator_disable(qphy->vddp_ref_clk);
> +
> + regulator_disable(qphy->vdda_pll);
> + regulator_disable(qphy->vdda_phy);
> +
> + return 0;
> +}
> +
> +static int qcom_qmp_phy_is_ready(struct qcom_qmp_phy *qphy,
> + void __iomem *pcs_status, u32 mask)
> +{
> + unsigned int init_timeout;
> +
> + init_timeout = PHY_READY_TIMEOUT_COUNT;
> + do {
> + if (readl_relaxed(pcs_status) & mask)
> + break;
> +
> + usleep_range(REFCLK_STABILIZATION_DELAY_US_MIN,
> + REFCLK_STABILIZATION_DELAY_US_MAX);
> + } while (--init_timeout);
> +
> + if (!init_timeout)
> + return -EBUSY;
> +
> + return 0;
> +}
> +
> +static int qcom_qmp_phy_com_init(struct qcom_qmp_phy *qphy)
> +{
> + const struct qmp_phy_init_cfg *cfg = qphy->cfg;
> + void __iomem *serdes = qphy->serdes;
> + int ret;
> +
> + mutex_lock(&qphy->phy_mutex);
> + if (qphy->init_count++) {
> + mutex_unlock(&qphy->phy_mutex);
> + return 0;
> + }
> +
> + ret = reset_control_deassert(qphy->phy_rst);
> + if (ret) {
> + dev_err(qphy->dev, "phy reset deassert failed\n");
mutex unlock
> + return ret;
> + }
> +
> + ret = reset_control_deassert(qphy->phycom_rst);
> + if (ret) {
> + dev_err(qphy->dev, "common reset deassert failed\n");
> + goto err_phycom_rst;
> + }
> +
> + if (qphy->phycfg_rst) {
> + ret = reset_control_deassert(qphy->phycfg_rst);
> + if (ret) {
> + dev_err(qphy->dev, "common reset deassert failed\n");
> + goto err_phycfg_rst;
> + }
> + }
> +
> + if (cfg->has_phy_com_ctrl) {
> + qphy_setbits(PHY_SW_PWRDN,
> + serdes + cfg->regs[QPHY_COM_POWER_DOWN_CONTROL]);
> + /* Make sure that above write is completed */
> + mb();
> + }
> +
> + /* Serdes configuration */
> + qcom_qmp_phy_configure(serdes, cfg->regs, cfg->phy_init_serdes_tbl,
> + cfg->phy_init_serdes_tbl_sz);
> +
> + if (cfg->has_phy_com_ctrl) {
> + qphy_clrbits(PHY_SW_RESET, serdes +
> + cfg->regs[QPHY_COM_SW_RESET]);
> + qphy_setbits(PHY_SERDES_START | PHY_PCS_START,
> + serdes + cfg->regs[QPHY_COM_START_CONTROL]);
> + /* Make sure that above write is completed */
> + mb();
> +
> + ret = qcom_qmp_phy_is_ready(qphy, serdes +
> + cfg->regs[QPHY_COM_PCS_READY_STATUS],
> + MASK_COM_PCS_READY);
> + if (ret) {
> + dev_err(qphy->dev,
> + "common control block init timed-out\n");
> + goto err_phy_comctrl;
> + }
> + }
> +
> + mutex_unlock(&qphy->phy_mutex);
> +
> + return 0;
> +
> +err_phy_comctrl:
mutex unlock
> + if (qphy->phycfg_rst)
> + reset_control_assert(qphy->phycfg_rst);
> +err_phycfg_rst:
> + reset_control_assert(qphy->phycom_rst);
> +err_phycom_rst:
> + reset_control_assert(qphy->phy_rst);
> + return ret;
> +}
> +
> +static int qcom_qmp_phy_com_exit(struct qcom_qmp_phy *qphy)
> +{
> + const struct qmp_phy_init_cfg *cfg = qphy->cfg;
> + void __iomem *serdes = qphy->serdes;
> +
> + mutex_lock(&qphy->phy_mutex);
> + if (--qphy->init_count) {
> + mutex_unlock(&qphy->phy_mutex);
> + return 0;
> + }
> +
> + if (cfg->has_phy_com_ctrl) {
> + qphy_setbits(PHY_SERDES_START | PHY_PCS_START,
> + serdes + cfg->regs[QPHY_COM_START_CONTROL]);
> + qphy_clrbits(PHY_SW_RESET, serdes +
> + cfg->regs[QPHY_COM_SW_RESET]);
> + qphy_setbits(PHY_SW_PWRDN,
> + serdes + cfg->regs[QPHY_COM_POWER_DOWN_CONTROL]);
> +
> + /* Make sure that above writes are completed */
> + mb();
> + }
> +
> + reset_control_assert(qphy->phy_rst);
> + reset_control_assert(qphy->phycom_rst);
> + if (qphy->phycfg_rst)
> + reset_control_assert(qphy->phycfg_rst);
> +
> + mutex_unlock(&qphy->phy_mutex);
> +
> + return 0;
> +}
> +
> +/* PHY Initialization */
> +static int qcom_qmp_phy_init(struct phy *phy)
> +{
> + struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
> + struct qcom_qmp_phy *qphy = phydesc->qphy;
> + const struct qmp_phy_init_cfg *cfg = qphy->cfg;
> + void __iomem *tx = phydesc->tx;
> + void __iomem *rx = phydesc->rx;
> + void __iomem *pcs = phydesc->pcs;
> + int ret;
> +
> + dev_info(qphy->dev, "Initializing QMP phy\n");
> +
> + /* enable interface clocks to program phy */
> + clk_prepare_enable(qphy->aux_clk);
> + clk_prepare_enable(qphy->cfg_ahb_clk);
> +
> + ret = qcom_qmp_phy_com_init(qphy);
> + if (ret)
> + goto err;
> +
> + if (phydesc->lane_rst) {
> + ret = reset_control_deassert(phydesc->lane_rst);
> + if (ret) {
> + dev_err(qphy->dev, "lane<%d> reset deassert failed\n",
> + phydesc->index);
> + goto err_lane_rst;
> + }
> + }
> +
> + /* Tx, Rx, and PCS configurations */
> + qcom_qmp_phy_configure(tx, cfg->regs, cfg->phy_init_tx_tbl,
> + cfg->phy_init_tx_tbl_sz);
> + qcom_qmp_phy_configure(rx, cfg->regs, cfg->phy_init_rx_tbl,
> + cfg->phy_init_rx_tbl_sz);
> + qcom_qmp_phy_configure(pcs, cfg->regs, cfg->phy_init_pcs_tbl,
> + cfg->phy_init_pcs_tbl_sz);
> +
> + /*
> + * Pull out PHY from POWER DOWN state:
> + * This is active low enable signal to power-down PHY.
> + */
> + qphy_setbits(cfg->mask_pwr_dn_ctrl,
> + pcs + QPHY_POWER_DOWN_CONTROL);
> + /* XXX: 10 us delay; given in PCIE HPG only */
> + usleep_range(POWER_DOWN_DELAY_US_MIN, POWER_DOWN_DELAY_US_MAX);
> +
> + /* start SerDes and Phy-Coding-Sublayer */
> + qphy_setbits(cfg->mask_start_ctrl, pcs + QPHY_START_CTRL);
> +
> + /* Pull PHY out of reset state */
> + qphy_clrbits(PHY_SW_RESET, pcs + QPHY_SW_RESET);
> + /* Make sure that above writes are completed */
> + mb();
> +
> + ret = qcom_qmp_phy_is_ready(qphy, pcs +
> + cfg->regs[QPHY_PCS_READY_STATUS],
> + MASK_PHYSTATUS);
> + if (ret) {
> + dev_err(qphy->dev, "phy initialization timed-out\n");
> + goto err_pcs_ready;
> + }
> +
> + return 0;
> +
> +err_pcs_ready:
> + if (phydesc->lane_rst)
> + reset_control_assert(phydesc->lane_rst);
> +err_lane_rst:
> + qcom_qmp_phy_com_exit(qphy);
> +err:
> + clk_disable_unprepare(qphy->cfg_ahb_clk);
> + clk_disable_unprepare(qphy->aux_clk);
> + return ret;
> +}
> +
> +static int qcom_qmp_phy_exit(struct phy *phy)
> +{
> + struct qmp_phy_desc *phydesc = phy_get_drvdata(phy);
> + struct qcom_qmp_phy *qphy = phydesc->qphy;
> + const struct qmp_phy_init_cfg *cfg = qphy->cfg;
> +
> + /* PHY reset */
> + qphy_setbits(PHY_SW_RESET, phydesc->pcs + QPHY_SW_RESET);
> +
> + /* stop SerDes and Phy-Coding-Sublayer */
> + qphy_clrbits(cfg->mask_start_ctrl, phydesc->pcs + QPHY_START_CTRL);
> +
> + /* Put PHY into POWER DOWN state: active low */
> + qphy_clrbits(cfg->mask_pwr_dn_ctrl,
> + phydesc->pcs + QPHY_POWER_DOWN_CONTROL);
> +
> + /* Make sure that above writes are completed */
> + mb();
> +
> + if (phydesc->lane_rst)
> + reset_control_assert(phydesc->lane_rst);
> +
> + qcom_qmp_phy_com_exit(qphy);
> +
> + clk_disable_unprepare(qphy->aux_clk);
> + clk_disable_unprepare(qphy->cfg_ahb_clk);
> +
> + return 0;
> +}
> +
> +
> +static int qcom_qmp_phy_regulator_init(struct device *dev)
> +{
> + struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + qphy->vdda_phy = devm_regulator_get(dev, "vdda-phy");
> + if (IS_ERR(qphy->vdda_phy)) {
> + ret = PTR_ERR(qphy->vdda_phy);
> + dev_err(dev, "failed to get vdda-phy, %d\n", ret);
> + return ret;
> + }
> +
> + qphy->vdda_pll = devm_regulator_get(dev, "vdda-pll");
> + if (IS_ERR(qphy->vdda_pll)) {
> + ret = PTR_ERR(qphy->vdda_pll);
> + dev_err(dev, "failed to get vdda-pll, %d\n", ret);
> + return ret;
> + }
> +
> + /* optional regulator */
> + qphy->vddp_ref_clk = devm_regulator_get(dev, "vddp-ref-clk");
Should mention this in bindings too.
> + if (IS_ERR(qphy->vddp_ref_clk)) {
> + ret = PTR_ERR(qphy->vddp_ref_clk);
> + dev_info(dev, "failed to get vddp-ref-clk, %d\n", ret);
> + qphy->vddp_ref_clk = NULL;
> + }
> +
> + return 0;
> +}
> +
> +static int qcom_qmp_phy_clk_init(struct device *dev)
> +{
> + struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
> + int ret;
> +
> + qphy->aux_clk = devm_clk_get(dev, "aux");
> + if (IS_ERR(qphy->aux_clk)) {
> + ret = PTR_ERR(qphy->aux_clk);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "failed to get aux_clk\n");
> + return ret;
> + }
> +
> + qphy->cfg_ahb_clk = devm_clk_get(dev, "cfg_ahb");
> + if (IS_ERR(qphy->cfg_ahb_clk)) {
> + ret = PTR_ERR(qphy->cfg_ahb_clk);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "failed to get cfg_ahb_clk\n");
> + return ret;
> + }
> +
> + /*
> + * ref_clk and ref_clk_src handles may not be available in
> + * all hardwares. So we don't return error in these cases.
> + */
This thing has to be mentioned in the bindings too.
> + qphy->ref_clk_src = devm_clk_get(dev, "ref_clk_src");
> + if (IS_ERR(qphy->ref_clk_src)) {
> + ret = PTR_ERR(qphy->ref_clk_src);
> + if (ret != -EPROBE_DEFER) {
> + qphy->ref_clk_src = NULL;
> + dev_dbg(dev, "failed to get ref_clk_src\n");
> + } else {
> + return ret;
> + }
> + }
> +
> + qphy->ref_clk = devm_clk_get(dev, "ref_clk");
> + if (IS_ERR(qphy->ref_clk)) {
> + ret = PTR_ERR(qphy->ref_clk);
> + if (ret != -EPROBE_DEFER) {
> + qphy->ref_clk = NULL;
> + dev_dbg(dev, "failed to get ref_clk\n");
> + } else {
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static struct phy *qcom_qmp_phy_xlate(struct device *dev,
> + struct of_phandle_args *args)
> +{
> + struct qcom_qmp_phy *qphy = dev_get_drvdata(dev);
> + int i;
> +
> + if (WARN_ON(args->args[0] >= qphy->cfg->nlanes))
> + return ERR_PTR(-ENODEV);
> +
> + for (i = 0; i < qphy->cfg->nlanes; i++) {
> + if (qphy->phys[i]->index == args->args[0])
> + break;
> + }
> +
> + if (i == qphy->cfg->nlanes)
> + return ERR_PTR(-ENODEV);
> +
> + return qphy->phys[i]->phy;
> +}
> +
> +static const struct phy_ops qcom_qmp_phy_gen_ops = {
> + .init = qcom_qmp_phy_init,
> + .exit = qcom_qmp_phy_exit,
> + .power_on = qcom_qmp_phy_poweron,
> + .power_off = qcom_qmp_phy_poweroff,
> + .owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id qcom_qmp_phy_of_match_table[] = {
> + {
> + .compatible = "qcom,msm8996-qmp-pcie-phy",
> + .data = &pciephy_init_cfg,
> + }, {
> + .compatible = "qcom,msm8996-qmp-usb3-phy",
> + .data = &usb3phy_init_cfg,
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_qmp_phy_of_match_table);
> +
> +static int qcom_qmp_phy_probe(struct platform_device *pdev)
> +{
> + struct qcom_qmp_phy *qphy;
> + struct device *dev = &pdev->dev;
> + struct device_node *child;
> + struct phy_provider *phy_provider;
> + struct resource *res;
> + const struct of_device_id *match;
> + void __iomem *base;
> + int ret = 0;
> + int id;
> +
> + qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
> + if (!qphy)
> + return -ENOMEM;
new line
> + qphy->dev = dev;
> + dev_set_drvdata(dev, qphy);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
new line
> + base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + /* per PHY serdes; usually located at base address */
> + qphy->serdes = base;
> +
> + mutex_init(&qphy->phy_mutex);
> +
> + /* Get the specific init parameters of QMP phy */
> + match = of_match_node(qcom_qmp_phy_of_match_table, dev->of_node);
> + qphy->cfg = match->data;
> +
> + ret = qcom_qmp_phy_clk_init(dev);
> + if (ret) {
> + dev_err(dev, "clock init failed\n");
> + return ret;
> + }
> +
> + ret = qcom_qmp_phy_regulator_init(dev);
> + if (ret) {
> + dev_err(dev, "regulator init failed\n");
> + return ret;
> + }
> +
> + qphy->phy_rst = devm_reset_control_get(dev, "phy");
> + if (IS_ERR(qphy->phy_rst)) {
> + dev_err(dev, "failed to get phy core reset\n");
> + return PTR_ERR(qphy->phy_rst);
> + }
> +
> + qphy->phycom_rst = devm_reset_control_get(dev, "common");
> + if (IS_ERR(qphy->phycom_rst)) {
> + dev_err(dev, "failed to get phy common reset\n");
> + return PTR_ERR(qphy->phycom_rst);
> + }
> +
> + qphy->phycfg_rst = devm_reset_control_get(dev, "cfg");
> + if (IS_ERR(qphy->phycfg_rst)) {
> + dev_err(dev, "failed to get phy ahb cfg reset\n");
> + qphy->phycfg_rst = NULL;
> + }
> +
> + qphy->phys = devm_kcalloc(dev, qphy->cfg->nlanes,
> + sizeof(*qphy->phys), GFP_KERNEL);
> + if (!qphy->phys)
> + return -ENOMEM;
> +
> + for (id = 0; id < qphy->cfg->nlanes; id++) {
> + struct phy *generic_phy;
> + struct qmp_phy_desc *phy_desc;
> + char prop_name[MAX_PROP_NAME];
> +
> + phy_desc = devm_kzalloc(dev, sizeof(*phy_desc), GFP_KERNEL);
> + if (!phy_desc) {
> + ret = -ENOMEM;
> + goto put_child;
> + }
> +
> + phy_desc->tx = base + qphy->cfg->tx_offsets[id];
> + phy_desc->rx = base + qphy->cfg->rx_offsets[id];
> + phy_desc->pcs = base + qphy->cfg->pcs_offsets[id];
> +
> + /*
> + * Get PHY's Pipe clock, if any; USB3 and PCIe are PIPE
> + * based phys, so they essentially have pipe clock
> + */
> + memset(&prop_name, 0, sizeof(prop_name));
> + snprintf(prop_name, MAX_PROP_NAME, "pipe%d", id);
> + phy_desc->pipe_clk = devm_clk_get(dev, prop_name);
> + if (IS_ERR(phy_desc->pipe_clk)) {
> + if (qphy->cfg->type == PHY_TYPE_PCIE ||
> + qphy->cfg->type == PHY_TYPE_USB3) {
> + ret = PTR_ERR(phy_desc->pipe_clk);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev,
> + "failed to get lane%d pipe_clk\n", id);
> + return ret;
> + } else {
> + phy_desc->pipe_clk = NULL;
> + }
> + }
Do we need to really need phy type for this?
You should proably make a flag in cfg to mark this rather then using phy
type. Hopefully it will also simplify the logic.
> +
> + /* Get lane reset, if any */
> + memset(&prop_name, 0, sizeof(prop_name));
> + snprintf(prop_name, MAX_PROP_NAME, "lane%d", id);
> + phy_desc->lane_rst = devm_reset_control_get(dev, prop_name);
> + if (IS_ERR(phy_desc->lane_rst)) {
> + if (qphy->cfg->type == PHY_TYPE_PCIE) {
> + dev_err(dev, "failed to get lane%d reset\n",
> + id);
> + ret = PTR_ERR(phy_desc->lane_rst);
> + goto put_child;
> + } else {
> + phy_desc->lane_rst = NULL;
> + }
> + }
Same comment as above.
> +
> + generic_phy = devm_phy_create(dev, NULL, &qcom_qmp_phy_gen_ops);
> + if (IS_ERR(generic_phy)) {
> + ret = PTR_ERR(generic_phy);
> + dev_err(dev, "failed to create qphy %d\n", ret);
> + goto put_child;
> + }
> +
> + phy_desc->phy = generic_phy;
> + phy_desc->index = id;
> + phy_desc->qphy = qphy;
> + phy_set_drvdata(generic_phy, phy_desc);
> + qphy->phys[id] = phy_desc;
> + }
> +
> + phy_provider = devm_of_phy_provider_register(dev, qcom_qmp_phy_xlate);
> + if (IS_ERR(phy_provider)) {
> + ret = PTR_ERR(phy_provider);
> + dev_err(dev, "failed to register qphy %d\n", ret);
> + goto put_child;
> + }
> +
> +put_child:
> + of_node_put(child);
> + return ret;
> +}
> +
> +static struct platform_driver qcom_qmp_phy_driver = {
> + .probe = qcom_qmp_phy_probe,
> + .driver = {
> + .name = "qcom_qmp_phy",
> + .of_match_table = of_match_ptr(qcom_qmp_phy_of_match_table),
> + },
> +};
> +
> +module_platform_driver(qcom_qmp_phy_driver);
> +
> +MODULE_AUTHOR("Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>");
> +MODULE_DESCRIPTION("Qualcomm QMP PHY driver");
> +MODULE_LICENSE("GPL v2");
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox