* Re: [PATCH] powerpc: select MEMORY for FSL_IFC to not break existing .config files
From: Paul Gortmaker @ 2014-02-19 22:25 UTC (permalink / raw)
To: Scott Wood
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-next, Paul Mackerras,
linuxppc-dev, Prabhakar Kushwaha
In-Reply-To: <1392848350.6733.809.camel@snotra.buserror.net>
On 14-02-19 05:19 PM, Scott Wood wrote:
> On Wed, 2014-02-19 at 17:07 -0500, Paul Gortmaker wrote:
>> commit d2ae2e20fbdde5a65f3a5a153044ab1e5c53f7cc ("driver/memory:Move
>> Freescale IFC driver to a common driver") introduces this build
>> regression into the mpc85xx_defconfig:
>>
>> drivers/built-in.o: In function `fsl_ifc_nand_remove':
>> drivers/mtd/nand/fsl_ifc_nand.c:1147: undefined reference to `fsl_ifc_ctrl_dev'
>> drivers/mtd/nand/fsl_ifc_nand.c:1147: undefined reference to `fsl_ifc_ctrl_dev'
>> drivers/built-in.o: In function `fsl_ifc_nand_probe':
>> drivers/mtd/nand/fsl_ifc_nand.c:1031: undefined reference to `fsl_ifc_ctrl_dev'
>> drivers/mtd/nand/fsl_ifc_nand.c:1031: undefined reference to `fsl_ifc_ctrl_dev'
>> drivers/built-in.o: In function `match_bank':
>> drivers/mtd/nand/fsl_ifc_nand.c:1013: undefined reference to `convert_ifc_address'
>> drivers/built-in.o: In function `fsl_ifc_nand_probe':
>> drivers/mtd/nand/fsl_ifc_nand.c:1059: undefined reference to `fsl_ifc_ctrl_dev'
>> drivers/mtd/nand/fsl_ifc_nand.c:1080: undefined reference to `fsl_ifc_ctrl_dev'
>> drivers/mtd/nand/fsl_ifc_nand.c:1069: undefined reference to `fsl_ifc_ctrl_dev'
>> drivers/mtd/nand/fsl_ifc_nand.c:1069: undefined reference to `fsl_ifc_ctrl_dev'
>> make: *** [vmlinux] Error 1
>>
>> This happens because there is nothing to descend us into the
>> drivers/memory directory in the mpc85xx_defconfig. It wasn't
>> selecting CONFIG_MEMORY. So we never built drivers/memory/fsl_ifc.o
>> even with CONFIG_FSL_IFC=y, and so we have nothing to link against.
>>
>> In order to remain compatible with old config files and to avoid
>> such build failures, make CONFIG_FSL_IFC select CONFIG_MEMORY.
>> Also fix the whitespace issue (spaces vs. a tab) in the Kconfig.
>>
>> Cc: Prabhakar Kushwaha <prabhakar@freescale.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>> ---
>>
>> [This probably makes sense to go in via Greg's char-misc/char-misc-next
>> (vs. powerpc-next) since that is where the regression was introduced.]
>>
>> arch/powerpc/Kconfig | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 957bf344c0f5..d2d8d8f50610 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -738,7 +738,8 @@ config FSL_LBC
>>
>> config FSL_IFC
>> bool
>> - depends on FSL_SOC
>> + depends on FSL_SOC
>> + select MEMORY
>
> Why do we still have FSL_IFC in arch/powerpc/Kconfig if the driver has
> been moved? The NAND driver and other selectors of FSL_IFC can select
> MEMORY.
At the risk of a slightly larger footprint to the regression fix,
I'd agree that is a better overall solution. I'll code that up.
Unless folks want the regression fix as-is, and then the move?
Paul.
--
>
> -Scott
>
>
^ permalink raw reply
* Re: [PATCH] powerpc: select MEMORY for FSL_IFC to not break existing .config files
From: Scott Wood @ 2014-02-19 22:19 UTC (permalink / raw)
To: Paul Gortmaker
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-next, Paul Mackerras,
linuxppc-dev, Prabhakar Kushwaha
In-Reply-To: <1392847641-63777-1-git-send-email-paul.gortmaker@windriver.com>
On Wed, 2014-02-19 at 17:07 -0500, Paul Gortmaker wrote:
> commit d2ae2e20fbdde5a65f3a5a153044ab1e5c53f7cc ("driver/memory:Move
> Freescale IFC driver to a common driver") introduces this build
> regression into the mpc85xx_defconfig:
>
> drivers/built-in.o: In function `fsl_ifc_nand_remove':
> drivers/mtd/nand/fsl_ifc_nand.c:1147: undefined reference to `fsl_ifc_ctrl_dev'
> drivers/mtd/nand/fsl_ifc_nand.c:1147: undefined reference to `fsl_ifc_ctrl_dev'
> drivers/built-in.o: In function `fsl_ifc_nand_probe':
> drivers/mtd/nand/fsl_ifc_nand.c:1031: undefined reference to `fsl_ifc_ctrl_dev'
> drivers/mtd/nand/fsl_ifc_nand.c:1031: undefined reference to `fsl_ifc_ctrl_dev'
> drivers/built-in.o: In function `match_bank':
> drivers/mtd/nand/fsl_ifc_nand.c:1013: undefined reference to `convert_ifc_address'
> drivers/built-in.o: In function `fsl_ifc_nand_probe':
> drivers/mtd/nand/fsl_ifc_nand.c:1059: undefined reference to `fsl_ifc_ctrl_dev'
> drivers/mtd/nand/fsl_ifc_nand.c:1080: undefined reference to `fsl_ifc_ctrl_dev'
> drivers/mtd/nand/fsl_ifc_nand.c:1069: undefined reference to `fsl_ifc_ctrl_dev'
> drivers/mtd/nand/fsl_ifc_nand.c:1069: undefined reference to `fsl_ifc_ctrl_dev'
> make: *** [vmlinux] Error 1
>
> This happens because there is nothing to descend us into the
> drivers/memory directory in the mpc85xx_defconfig. It wasn't
> selecting CONFIG_MEMORY. So we never built drivers/memory/fsl_ifc.o
> even with CONFIG_FSL_IFC=y, and so we have nothing to link against.
>
> In order to remain compatible with old config files and to avoid
> such build failures, make CONFIG_FSL_IFC select CONFIG_MEMORY.
> Also fix the whitespace issue (spaces vs. a tab) in the Kconfig.
>
> Cc: Prabhakar Kushwaha <prabhakar@freescale.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>
> [This probably makes sense to go in via Greg's char-misc/char-misc-next
> (vs. powerpc-next) since that is where the regression was introduced.]
>
> arch/powerpc/Kconfig | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 957bf344c0f5..d2d8d8f50610 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -738,7 +738,8 @@ config FSL_LBC
>
> config FSL_IFC
> bool
> - depends on FSL_SOC
> + depends on FSL_SOC
> + select MEMORY
Why do we still have FSL_IFC in arch/powerpc/Kconfig if the driver has
been moved? The NAND driver and other selectors of FSL_IFC can select
MEMORY.
-Scott
^ permalink raw reply
* Re: [PATCH v3 3/3] powerpc/pseries: Report in kernel device tree update to drmgr
From: Tyrel Datwyler @ 2014-02-19 22:07 UTC (permalink / raw)
To: benh; +Cc: nfont, linuxppc-dev, Tyrel Datwyler
In-Reply-To: <1392843415-17153-5-git-send-email-tyreld@linux.vnet.ibm.com>
On 02/19/2014 12:56 PM, Tyrel Datwyler wrote:
> Traditionally it has been drmgr's responsibilty to update the device tree
> through the /proc/ppc64/ofdt interface after a suspend/resume operation.
> This patchset however has modified suspend/resume ops to preform that update
> entirely in the kernel during the resume. Therefore, a mechanism is required
> for drmgr to determine who is responsible for the update. This patch adds a
> show function to the "hibernate" attribute that returns 1 if the kernel
> updates the device tree after the resume and 0 if drmgr is responsible.
Ignore this one. Apparently, I forgot to clear it out of my patch
directory before I resent.
>
> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
> ---
> arch/powerpc/platforms/pseries/suspend.c | 25 ++++++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/pseries/suspend.c b/arch/powerpc/platforms/pseries/suspend.c
> index 1d9c580..b87b978 100644
> --- a/arch/powerpc/platforms/pseries/suspend.c
> +++ b/arch/powerpc/platforms/pseries/suspend.c
> @@ -192,7 +192,30 @@ out:
> return rc;
> }
>
> -static DEVICE_ATTR(hibernate, S_IWUSR, NULL, store_hibernate);
> +#define USER_DT_UPDATE 0
> +#define KERN_DT_UPDATE 1
> +
> +/**
> + * show_hibernate - Report device tree update responsibilty
> + * @dev: subsys root device
> + * @attr: device attribute struct
> + * @buf: buffer
> + *
> + * Report whether a device tree update is performed by the kernel after a
> + * resume, or if drmgr must coordinate the update from user space.
> + *
> + * Return value:
> + * 0 if drmgr is to initiate update, and 1 otherwise
> + **/
> +static ssize_t show_hibernate(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%d\n", KERN_DT_UPDATE);
> +}
> +
> +static DEVICE_ATTR(hibernate, S_IWUSR | S_IRUGO,
> + show_hibernate, store_hibernate);
>
> static struct bus_type suspend_subsys = {
> .name = "power",
>
^ permalink raw reply
* [PATCH] powerpc: select MEMORY for FSL_IFC to not break existing .config files
From: Paul Gortmaker @ 2014-02-19 22:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Arnd Bergmann, Paul Gortmaker, linux-next, Paul Mackerras,
linuxppc-dev, Prabhakar Kushwaha
commit d2ae2e20fbdde5a65f3a5a153044ab1e5c53f7cc ("driver/memory:Move
Freescale IFC driver to a common driver") introduces this build
regression into the mpc85xx_defconfig:
drivers/built-in.o: In function `fsl_ifc_nand_remove':
drivers/mtd/nand/fsl_ifc_nand.c:1147: undefined reference to `fsl_ifc_ctrl_dev'
drivers/mtd/nand/fsl_ifc_nand.c:1147: undefined reference to `fsl_ifc_ctrl_dev'
drivers/built-in.o: In function `fsl_ifc_nand_probe':
drivers/mtd/nand/fsl_ifc_nand.c:1031: undefined reference to `fsl_ifc_ctrl_dev'
drivers/mtd/nand/fsl_ifc_nand.c:1031: undefined reference to `fsl_ifc_ctrl_dev'
drivers/built-in.o: In function `match_bank':
drivers/mtd/nand/fsl_ifc_nand.c:1013: undefined reference to `convert_ifc_address'
drivers/built-in.o: In function `fsl_ifc_nand_probe':
drivers/mtd/nand/fsl_ifc_nand.c:1059: undefined reference to `fsl_ifc_ctrl_dev'
drivers/mtd/nand/fsl_ifc_nand.c:1080: undefined reference to `fsl_ifc_ctrl_dev'
drivers/mtd/nand/fsl_ifc_nand.c:1069: undefined reference to `fsl_ifc_ctrl_dev'
drivers/mtd/nand/fsl_ifc_nand.c:1069: undefined reference to `fsl_ifc_ctrl_dev'
make: *** [vmlinux] Error 1
This happens because there is nothing to descend us into the
drivers/memory directory in the mpc85xx_defconfig. It wasn't
selecting CONFIG_MEMORY. So we never built drivers/memory/fsl_ifc.o
even with CONFIG_FSL_IFC=y, and so we have nothing to link against.
In order to remain compatible with old config files and to avoid
such build failures, make CONFIG_FSL_IFC select CONFIG_MEMORY.
Also fix the whitespace issue (spaces vs. a tab) in the Kconfig.
Cc: Prabhakar Kushwaha <prabhakar@freescale.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
[This probably makes sense to go in via Greg's char-misc/char-misc-next
(vs. powerpc-next) since that is where the regression was introduced.]
arch/powerpc/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 957bf344c0f5..d2d8d8f50610 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -738,7 +738,8 @@ config FSL_LBC
config FSL_IFC
bool
- depends on FSL_SOC
+ depends on FSL_SOC
+ select MEMORY
config FSL_GTM
bool
--
1.8.5.2
^ permalink raw reply related
* Re: [RFC PATCH 2/3] topology: support node_numa_mem() for determining the fallback node
From: David Rientjes @ 2014-02-19 22:04 UTC (permalink / raw)
To: Christoph Lameter
Cc: Han Pingtian, Nishanth Aravamudan, Pekka Enberg,
Linux Memory Management List, Paul Mackerras, Anton Blanchard,
Matt Mackall, Joonsoo Kim, linuxppc-dev, Wanpeng Li
In-Reply-To: <alpine.DEB.2.10.1402181033480.28964@nuc>
On Tue, 18 Feb 2014, Christoph Lameter wrote:
> Its an optimization to avoid calling the page allocator to figure out if
> there is memory available on a particular node.
>
Thus this patch breaks with memory hot-add for a memoryless node.
^ permalink raw reply
* Re: [RFC PATCH 2/3] topology: support node_numa_mem() for determining the fallback node
From: David Rientjes @ 2014-02-19 22:03 UTC (permalink / raw)
To: Christoph Lameter
Cc: Han Pingtian, Nishanth Aravamudan, Pekka Enberg,
Linux Memory Management List, Paul Mackerras, Anton Blanchard,
Matt Mackall, Joonsoo Kim, linuxppc-dev, Wanpeng Li
In-Reply-To: <alpine.DEB.2.10.1402181356120.2910@nuc>
On Tue, 18 Feb 2014, Christoph Lameter wrote:
> Ok but since you have a virtualized environment: Why not provide a fake
> home node with fake memory that could be anywhere? This would avoid the
> whole problem of supporting such a config at the kernel level.
>
By acpi, the abstraction of a NUMA node can include any combination of
cpus, memory, I/O resources, networking, or storage devices. This allows
two memoryless nodes, for example, to have different proximity to memory.
^ permalink raw reply
* Re: ppc: RECLAIM_DISTANCE 10?
From: David Rientjes @ 2014-02-19 21:45 UTC (permalink / raw)
To: Michal Hocko; +Cc: linuxppc-dev, Anton Blanchard, LKML, linux-mm
In-Reply-To: <20140219091959.GD14783@dhcp22.suse.cz>
On Wed, 19 Feb 2014, Michal Hocko wrote:
> Interesting. So is the PPC NUMA basically about local vs. very distant?
The point is that it's impossible to tell how distant they are from one
NUMA domain to the next NUMA domain.
> Should REMOTE_DISTANCE reflect that as well? Or can we have
> distance < REMOTE_DISTANCE and it would still make sense to have
> zone_reclaim enabled?
>
Ppc doesn't want to allocate in a different NUMA domain unless required,
the latency of a remote access is too high. Everything that isn't in the
same domain has a distance >10 and is setup as 2^(domain hops) * 10. We
don't have the ability like with a SLIT to define remote nodes to have
local latency vs remote or costly latency so the safe setting is a
RECLAIM_DISTANCE of 10.
^ permalink raw reply
* Re: [PATCH] of: give priority to the compatible match in __of_match_node()
From: Paul Gortmaker @ 2014-02-19 21:23 UTC (permalink / raw)
To: Grant Likely, Rob Herring
Cc: devicetree@vger.kernel.org, Kevin Hao, Arnd Bergmann,
Chris Proctor, Stephen N Chivers, Rob Herring, Scott Wood,
linuxppc-dev, Sebastian Hesselbarth
In-Reply-To: <20140219204134.E4A2DC4088D@trevor.secretlab.ca>
On 14-02-19 03:41 PM, Grant Likely wrote:
> On Wed, 19 Feb 2014 13:25:54 -0500, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
>> On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@gmail.com> wrote:
>>> On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote:
>>>> When the device node do have a compatible property, we definitely
>>>> prefer the compatible match besides the type and name. Only if
>>>> there is no such a match, we then consider the candidate which
>>>> doesn't have compatible entry but do match the type or name with
>>>> the device node.
>>>>
>>>> This is based on a patch from Sebastian Hesselbarth.
>>>> http://patchwork.ozlabs.org/patch/319434/
>>>>
>>>> I did some code refactoring and also fixed a bug in the original patch.
>>>
>>> I'm inclined to just revert this once again and avoid possibly
>>> breaking yet another platform.
>>
>> Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot
>> on my sbc8548. It fails with:
>
> I think I've got it fixed now with the latest series. Please try the
> devicetree/merge branch on git://git.secretlab.ca/git/linux
That seems to work.
libphy: Freescale PowerQUICC MII Bus: probed
libphy: Freescale PowerQUICC MII Bus: probed
fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4
fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd
fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled
fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256
fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256
fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4
fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd
fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled
fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256
fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256
TCP: cubic registered
Initializing XFRM netlink socket
NET: Registered protocol family 17
...
root@sbc8548:~# cat /proc/version
Linux version 3.14.0-rc3-00024-g7119f42057c6 (paul@yow-lpgnfs-02) (gcc version 4.5.2 (GCC) ) #1 Wed Feb 19 16:08:17 EST 2014
root@sbc8548:~#
Paul.
--
>
> g.
>
>> -----------------------------------------------
>> libphy: Freescale PowerQUICC MII Bus: probed
>> mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520
>> PHY address 1312 is too large
>> libphy: Freescale PowerQUICC MII Bus: probed
>> libphy: Freescale PowerQUICC MII Bus: probed
>> mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520
>> PHY address 1312 is too large
>> libphy: Freescale PowerQUICC MII Bus: probed
>> TCP: cubic registered
>> Initializing XFRM netlink socket
>> NET: Registered protocol family 17
>> <fail nfs mount>
>> -----------------------------------------------
>>
>> On a normal boot, we should see this:
>> -----------------------------------------------
>> libphy: Freescale PowerQUICC MII Bus: probed
>> libphy: Freescale PowerQUICC MII Bus: probed
>> fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4
>> fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd
>> fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled
>> fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256
>> fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256
>> fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4
>> fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd
>> fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled
>> fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256
>> fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256
>> TCP: cubic registered
>> Initializing XFRM netlink socket
>> NET: Registered protocol family 17
>> -----------------------------------------------
>>
>>
>> Git bisect says:
>>
>> ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit
>> commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4
>> Author: Kevin Hao <haokexin@gmail.com>
>> Date: Tue Feb 18 15:57:30 2014 +0800
>>
>> of: reimplement the matching method for __of_match_node()
>>
>> In the current implementation of __of_match_node(), it will compare
>> each given match entry against all the node's compatible strings
>> with of_device_is_compatible().
>>
>> To achieve multiple compatible strings per node with ordering from
>> specific to generic, this requires given matches to be ordered from
>> specific to generic. For most of the drivers this is not true and
>> also an alphabetical ordering is more sane there.
>>
>> Therefore, we define a following priority order for the match, and
>> then scan all the entries to find the best match.
>> 1. specific compatible && type && name
>> 2. specific compatible && type
>> 3. specific compatible && name
>> 4. specific compatible
>> 5. general compatible && type && name
>> 6. general compatible && type
>> 7. general compatible && name
>> 8. general compatible
>> 9. type && name
>> 10. type
>> 11. name
>>
>> This is based on some pseudo-codes provided by Grant Likely.
>>
>> Signed-off-by: Kevin Hao <haokexin@gmail.com>
>> [grant.likely: Changed multiplier to 4 which makes more sense]
>> Signed-off-by: Grant Likely <grant.likely@linaro.org>
>>
>> :040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0
>> 8401b0e3903e23e973845ee75b26b04345d803d2 M drivers
>>
>> As a double check, I checked out the head of linux-next, and did a
>> revert of the above commit, and my sbc8548 can then boot properly.
>>
>> Not doing anything fancy ; using the defconfig exactly as-is, and
>> ensuring the dtb is fresh from linux-next HEAD of today.
>>
>> Thanks,
>> Paul.
>> --
>>
>>>
>>> However, I think I would like to see this structured differently. We
>>> basically have 2 ways of matching: the existing pre-3.14 way and the
>>> desired match on best compatible only. All new bindings should match
>>> with the new way and the old way needs to be kept for compatibility.
>>> So lets structure the code that way. Search the match table first for
>>> best compatible with name and type NULL, then search the table the old
>>> way. I realize it appears you are doing this, but it is not clear this
>>> is the intent of the code. I would like to see this written as a patch
>>> with commit 105353145eafb3ea919 reverted first and you add a new match
>>> function to call first and then fallback to the existing function.
>>>
>>> Rob
>>>
>>>>
>>>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>>> Signed-off-by: Kevin Hao <haokexin@gmail.com>
>>>> ---
>>>> drivers/of/base.c | 55 +++++++++++++++++++++++++++++++++++++------------------
>>>> 1 file changed, 37 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>> index ff85450d5683..9d655df458bd 100644
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>> @@ -730,32 +730,45 @@ out:
>>>> }
>>>> EXPORT_SYMBOL(of_find_node_with_property);
>>>>
>>>> +static int of_match_type_or_name(const struct device_node *node,
>>>> + const struct of_device_id *m)
>>>> +{
>>>> + int match = 1;
>>>> +
>>>> + if (m->name[0])
>>>> + match &= node->name && !strcmp(m->name, node->name);
>>>> +
>>>> + if (m->type[0])
>>>> + match &= node->type && !strcmp(m->type, node->type);
>>>> +
>>>> + return match;
>>>> +}
>>>> +
>>>> static
>>>> const struct of_device_id *__of_match_node(const struct of_device_id *matches,
>>>> const struct device_node *node)
>>>> {
>>>> const char *cp;
>>>> int cplen, l;
>>>> + const struct of_device_id *m;
>>>> + int match;
>>>>
>>>> if (!matches)
>>>> return NULL;
>>>>
>>>> cp = __of_get_property(node, "compatible", &cplen);
>>>> - do {
>>>> - const struct of_device_id *m = matches;
>>>> + while (cp && (cplen > 0)) {
>>>> + m = matches;
>>>>
>>>> /* Check against matches with current compatible string */
>>>> while (m->name[0] || m->type[0] || m->compatible[0]) {
>>>> - int match = 1;
>>>> - if (m->name[0])
>>>> - match &= node->name
>>>> - && !strcmp(m->name, node->name);
>>>> - if (m->type[0])
>>>> - match &= node->type
>>>> - && !strcmp(m->type, node->type);
>>>> - if (m->compatible[0])
>>>> - match &= cp
>>>> - && !of_compat_cmp(m->compatible, cp,
>>>> + if (!m->compatible[0]) {
>>>> + m++;
>>>> + continue;
>>>> + }
>>>> +
>>>> + match = of_match_type_or_name(node, m);
>>>> + match &= cp && !of_compat_cmp(m->compatible, cp,
>>>> strlen(m->compatible));
>>>> if (match)
>>>> return m;
>>>> @@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
>>>> }
>>>>
>>>> /* Get node's next compatible string */
>>>> - if (cp) {
>>>> - l = strlen(cp) + 1;
>>>> - cp += l;
>>>> - cplen -= l;
>>>> - }
>>>> - } while (cp && (cplen > 0));
>>>> + l = strlen(cp) + 1;
>>>> + cp += l;
>>>> + cplen -= l;
>>>> + }
>>>> +
>>>> + m = matches;
>>>> + /* Check against matches without compatible string */
>>>> + while (m->name[0] || m->type[0] || m->compatible[0]) {
>>>> + if (!m->compatible[0] && of_match_type_or_name(node, m))
>>>> + return m;
>>>> + m++;
>>>> + }
>>>>
>>>> return NULL;
>>>> }
>>>> --
>>>> 1.8.5.3
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> _______________________________________________
>>> Linuxppc-dev mailing list
>>> Linuxppc-dev@lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
^ permalink raw reply
* [PATCH v3 3/3] powerpc/pseries: Report in kernel device tree update to drmgr
From: Tyrel Datwyler @ 2014-02-19 20:56 UTC (permalink / raw)
To: benh; +Cc: nfont, linuxppc-dev, Tyrel Datwyler
In-Reply-To: <1392843415-17153-1-git-send-email-tyreld@linux.vnet.ibm.com>
Traditionally it has been drmgr's responsibilty to update the device tree
through the /proc/ppc64/ofdt interface after a suspend/resume operation.
This patchset however has modified suspend/resume ops to preform that update
entirely in the kernel during the resume. Therefore, a mechanism is required
for drmgr to determine who is responsible for the update. This patch adds a
show function to the "hibernate" attribute that returns 1 if the kernel
updates the device tree after the resume and 0 if drmgr is responsible.
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/suspend.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/suspend.c b/arch/powerpc/platforms/pseries/suspend.c
index 1d9c580..b87b978 100644
--- a/arch/powerpc/platforms/pseries/suspend.c
+++ b/arch/powerpc/platforms/pseries/suspend.c
@@ -192,7 +192,30 @@ out:
return rc;
}
-static DEVICE_ATTR(hibernate, S_IWUSR, NULL, store_hibernate);
+#define USER_DT_UPDATE 0
+#define KERN_DT_UPDATE 1
+
+/**
+ * show_hibernate - Report device tree update responsibilty
+ * @dev: subsys root device
+ * @attr: device attribute struct
+ * @buf: buffer
+ *
+ * Report whether a device tree update is performed by the kernel after a
+ * resume, or if drmgr must coordinate the update from user space.
+ *
+ * Return value:
+ * 0 if drmgr is to initiate update, and 1 otherwise
+ **/
+static ssize_t show_hibernate(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", KERN_DT_UPDATE);
+}
+
+static DEVICE_ATTR(hibernate, S_IWUSR | S_IRUGO,
+ show_hibernate, store_hibernate);
static struct bus_type suspend_subsys = {
.name = "power",
--
1.7.12.2
^ permalink raw reply related
* [PATCH v4 3/3] powerpc/pseries: Expose in kernel device tree update to drmgr
From: Tyrel Datwyler @ 2014-02-19 20:56 UTC (permalink / raw)
To: benh; +Cc: nfont, linuxppc-dev, Tyrel Datwyler
In-Reply-To: <1392843415-17153-1-git-send-email-tyreld@linux.vnet.ibm.com>
Traditionally it has been drmgr's responsibilty to update the device tree
through the /proc/ppc64/ofdt interface after a suspend/resume operation.
This patchset however has modified suspend/resume ops to preform an update
entirely in the kernel during the resume. Therefore, a mechanism is required
to expose that information to drmgr.
This patch adds a show function to the "hibernate" attribute that returns 1
if the kernel performs a device tree update after the resume and 0 otherwise.
This allows newer versions of drmgr to avoid doing a second unnecessary
device tree update.
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/suspend.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/suspend.c b/arch/powerpc/platforms/pseries/suspend.c
index 1d9c580..b87b978 100644
--- a/arch/powerpc/platforms/pseries/suspend.c
+++ b/arch/powerpc/platforms/pseries/suspend.c
@@ -192,7 +192,30 @@ out:
return rc;
}
-static DEVICE_ATTR(hibernate, S_IWUSR, NULL, store_hibernate);
+#define USER_DT_UPDATE 0
+#define KERN_DT_UPDATE 1
+
+/**
+ * show_hibernate - Report device tree update responsibilty
+ * @dev: subsys root device
+ * @attr: device attribute struct
+ * @buf: buffer
+ *
+ * Report whether a device tree update is performed by the kernel after a
+ * resume, or if drmgr must coordinate the update from user space.
+ *
+ * Return value:
+ * 0 if drmgr is to initiate update, and 1 otherwise
+ **/
+static ssize_t show_hibernate(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", KERN_DT_UPDATE);
+}
+
+static DEVICE_ATTR(hibernate, S_IWUSR | S_IRUGO,
+ show_hibernate, store_hibernate);
static struct bus_type suspend_subsys = {
.name = "power",
--
1.7.12.2
^ permalink raw reply related
* [PATCH v4 2/3] powerpc/pseries: Update dynamic cache nodes for suspend/resume operation
From: Tyrel Datwyler @ 2014-02-19 20:56 UTC (permalink / raw)
To: benh; +Cc: nfont, linuxppc-dev, Tyrel Datwyler
In-Reply-To: <1392843415-17153-1-git-send-email-tyreld@linux.vnet.ibm.com>
From: Haren Myneni <hbabu@us.ibm.com>
pHyp can change cache nodes for suspend/resume operation. Currently the
device tree is updated by drmgr in userspace after all non boot CPUs are
enabled. Hence, we do not modify the cache list based on the latest cache
nodes. Also we do not remove cache entries for the primary CPU.
This patch removes the cache list for the boot CPU, updates the device tree
before enabling nonboot CPUs and adds cache list for the boot cpu.
This patch also has the side effect that older versions of drmgr will
perform a second device tree update from userspace. While this is a
redundant waste of a couple cycles it is harmless since firmware returns the
same data for the subsequent update-nodes/properties rtas calls.
Signed-off-by: Haren Myneni <hbabu@us.ibm.com>
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/rtas.h | 1 +
arch/powerpc/platforms/pseries/suspend.c | 19 +++++++++++++++++++
2 files changed, 20 insertions(+)
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 9bd52c6..a0e1add 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -283,6 +283,7 @@ extern void pSeries_log_error(char *buf, unsigned int err_type, int fatal);
#ifdef CONFIG_PPC_PSERIES
extern int pseries_devicetree_update(s32 scope);
+extern void post_mobility_fixup(void);
#endif
#ifdef CONFIG_PPC_RTAS_DAEMON
diff --git a/arch/powerpc/platforms/pseries/suspend.c b/arch/powerpc/platforms/pseries/suspend.c
index 16a2552..1d9c580 100644
--- a/arch/powerpc/platforms/pseries/suspend.c
+++ b/arch/powerpc/platforms/pseries/suspend.c
@@ -26,6 +26,7 @@
#include <asm/mmu.h>
#include <asm/rtas.h>
#include <asm/topology.h>
+#include "../../kernel/cacheinfo.h"
static u64 stream_id;
static struct device suspend_dev;
@@ -79,6 +80,23 @@ static int pseries_suspend_cpu(void)
}
/**
+ * pseries_suspend_enable_irqs
+ *
+ * Post suspend configuration updates
+ *
+ **/
+static void pseries_suspend_enable_irqs(void)
+{
+ /*
+ * Update configuration which can be modified based on device tree
+ * changes during resume.
+ */
+ cacheinfo_cpu_offline(smp_processor_id());
+ post_mobility_fixup();
+ cacheinfo_cpu_online(smp_processor_id());
+}
+
+/**
* pseries_suspend_enter - Final phase of hibernation
*
* Return value:
@@ -235,6 +253,7 @@ static int __init pseries_suspend_init(void)
return rc;
ppc_md.suspend_disable_cpu = pseries_suspend_cpu;
+ ppc_md.suspend_enable_irqs = pseries_suspend_enable_irqs;
suspend_set_ops(&pseries_suspend_ops);
return 0;
}
--
1.7.12.2
^ permalink raw reply related
* [PATCH v4 1/3] powerpc/pseries: Device tree should only be updated once after suspend/migrate
From: Tyrel Datwyler @ 2014-02-19 20:56 UTC (permalink / raw)
To: benh; +Cc: nfont, linuxppc-dev, Tyrel Datwyler
In-Reply-To: <1392843415-17153-1-git-send-email-tyreld@linux.vnet.ibm.com>
From: Haren Myneni <hbabu@us.ibm.com>
The current code makes rtas calls for update-nodes, activate-firmware and then
update-nodes again. The FW provides the same data for both update-nodes calls.
As a result a proc entry exists error is reported for the second update while
adding device nodes.
This patch makes a single rtas call for update-nodes after activating the FW.
It also add rtas_busy delay for the activate-firmware rtas call.
Signed-off-by: Haren Myneni <hbabu@us.ibm.com>
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/mobility.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index cde4e0a..bde7eba 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -290,13 +290,6 @@ void post_mobility_fixup(void)
int rc;
int activate_fw_token;
- rc = pseries_devicetree_update(MIGRATION_SCOPE);
- if (rc) {
- printk(KERN_ERR "Initial post-mobility device tree update "
- "failed: %d\n", rc);
- return;
- }
-
activate_fw_token = rtas_token("ibm,activate-firmware");
if (activate_fw_token == RTAS_UNKNOWN_SERVICE) {
printk(KERN_ERR "Could not make post-mobility "
@@ -304,16 +297,17 @@ void post_mobility_fixup(void)
return;
}
- rc = rtas_call(activate_fw_token, 0, 1, NULL);
- if (!rc) {
- rc = pseries_devicetree_update(MIGRATION_SCOPE);
- if (rc)
- printk(KERN_ERR "Secondary post-mobility device tree "
- "update failed: %d\n", rc);
- } else {
+ do {
+ rc = rtas_call(activate_fw_token, 0, 1, NULL);
+ } while (rtas_busy_delay(rc));
+
+ if (rc)
printk(KERN_ERR "Post-mobility activate-fw failed: %d\n", rc);
- return;
- }
+
+ rc = pseries_devicetree_update(MIGRATION_SCOPE);
+ if (rc)
+ printk(KERN_ERR "Post-mobility device tree update "
+ "failed: %d\n", rc);
return;
}
--
1.7.12.2
^ permalink raw reply related
* [PATCH v4 0/3] powerpc/pseries: fix issues in suspend/resume code
From: Tyrel Datwyler @ 2014-02-19 20:56 UTC (permalink / raw)
To: benh; +Cc: nfont, linuxppc-dev, Tyrel Datwyler
This patchset fixes a couple of issues encountered in the suspend/resume code
base. First when using the kernel device tree update code update-nodes is
unnecessarily called more than once. Second the cpu cache lists are not
updated after a suspend/resume which under certain conditions may cause a
panic. Finally, since the cache list fix utilzes in kernel device tree update
code a means for telling drmgr that updating the device tree from userspace
is unnecessary.
Changes from v3:
- Updated patch descriptions to better reflect the behavior/interface changes
and why they are acceptable per Ben's concerns.
Changes from v2:
- Moved dynamic configuration update code into pseries specific routine
per Nathan's suggestion.
Changes from v1:
- Fixed several commit message typos
- Fixed authorship of first two patches
Haren Myneni (2):
powerpc/pseries: Device tree should only be updated once after
suspend/migrate
powerpc/pseries: Update dynamic cache nodes for suspend/resume
operation
Tyrel Datwyler (1):
powerpc/pseries: Report in kernel device tree update to drmgr
arch/powerpc/include/asm/rtas.h | 1 +
arch/powerpc/platforms/pseries/mobility.c | 26 +++++++-----------
arch/powerpc/platforms/pseries/suspend.c | 44 ++++++++++++++++++++++++++++++-
3 files changed, 54 insertions(+), 17 deletions(-)
--
1.7.12.2
^ permalink raw reply
* Re: [PATCH] of: give priority to the compatible match in __of_match_node()
From: Grant Likely @ 2014-02-19 20:41 UTC (permalink / raw)
To: Paul Gortmaker, Rob Herring
Cc: devicetree@vger.kernel.org, Kevin Hao, Arnd Bergmann,
Chris Proctor, Stephen N Chivers, Rob Herring, Scott Wood,
linuxppc-dev, Sebastian Hesselbarth
In-Reply-To: <CAP=VYLr7hqnzW2HLS4GCMFeghCgPmrQZHYst+AUfZc_WNT-Wog@mail.gmail.com>
On Wed, 19 Feb 2014 13:25:54 -0500, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@gmail.com> wrote:
> > On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote:
> >> When the device node do have a compatible property, we definitely
> >> prefer the compatible match besides the type and name. Only if
> >> there is no such a match, we then consider the candidate which
> >> doesn't have compatible entry but do match the type or name with
> >> the device node.
> >>
> >> This is based on a patch from Sebastian Hesselbarth.
> >> http://patchwork.ozlabs.org/patch/319434/
> >>
> >> I did some code refactoring and also fixed a bug in the original patch.
> >
> > I'm inclined to just revert this once again and avoid possibly
> > breaking yet another platform.
>
> Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot
> on my sbc8548. It fails with:
I think I've got it fixed now with the latest series. Please try the
devicetree/merge branch on git://git.secretlab.ca/git/linux
g.
> -----------------------------------------------
> libphy: Freescale PowerQUICC MII Bus: probed
> mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520
> PHY address 1312 is too large
> libphy: Freescale PowerQUICC MII Bus: probed
> libphy: Freescale PowerQUICC MII Bus: probed
> mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520
> PHY address 1312 is too large
> libphy: Freescale PowerQUICC MII Bus: probed
> TCP: cubic registered
> Initializing XFRM netlink socket
> NET: Registered protocol family 17
> <fail nfs mount>
> -----------------------------------------------
>
> On a normal boot, we should see this:
> -----------------------------------------------
> libphy: Freescale PowerQUICC MII Bus: probed
> libphy: Freescale PowerQUICC MII Bus: probed
> fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4
> fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd
> fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled
> fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256
> fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256
> fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4
> fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd
> fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled
> fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256
> fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256
> TCP: cubic registered
> Initializing XFRM netlink socket
> NET: Registered protocol family 17
> -----------------------------------------------
>
>
> Git bisect says:
>
> ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit
> commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4
> Author: Kevin Hao <haokexin@gmail.com>
> Date: Tue Feb 18 15:57:30 2014 +0800
>
> of: reimplement the matching method for __of_match_node()
>
> In the current implementation of __of_match_node(), it will compare
> each given match entry against all the node's compatible strings
> with of_device_is_compatible().
>
> To achieve multiple compatible strings per node with ordering from
> specific to generic, this requires given matches to be ordered from
> specific to generic. For most of the drivers this is not true and
> also an alphabetical ordering is more sane there.
>
> Therefore, we define a following priority order for the match, and
> then scan all the entries to find the best match.
> 1. specific compatible && type && name
> 2. specific compatible && type
> 3. specific compatible && name
> 4. specific compatible
> 5. general compatible && type && name
> 6. general compatible && type
> 7. general compatible && name
> 8. general compatible
> 9. type && name
> 10. type
> 11. name
>
> This is based on some pseudo-codes provided by Grant Likely.
>
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> [grant.likely: Changed multiplier to 4 which makes more sense]
> Signed-off-by: Grant Likely <grant.likely@linaro.org>
>
> :040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0
> 8401b0e3903e23e973845ee75b26b04345d803d2 M drivers
>
> As a double check, I checked out the head of linux-next, and did a
> revert of the above commit, and my sbc8548 can then boot properly.
>
> Not doing anything fancy ; using the defconfig exactly as-is, and
> ensuring the dtb is fresh from linux-next HEAD of today.
>
> Thanks,
> Paul.
> --
>
> >
> > However, I think I would like to see this structured differently. We
> > basically have 2 ways of matching: the existing pre-3.14 way and the
> > desired match on best compatible only. All new bindings should match
> > with the new way and the old way needs to be kept for compatibility.
> > So lets structure the code that way. Search the match table first for
> > best compatible with name and type NULL, then search the table the old
> > way. I realize it appears you are doing this, but it is not clear this
> > is the intent of the code. I would like to see this written as a patch
> > with commit 105353145eafb3ea919 reverted first and you add a new match
> > function to call first and then fallback to the existing function.
> >
> > Rob
> >
> >>
> >> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> >> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> >> ---
> >> drivers/of/base.c | 55 +++++++++++++++++++++++++++++++++++++------------------
> >> 1 file changed, 37 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/of/base.c b/drivers/of/base.c
> >> index ff85450d5683..9d655df458bd 100644
> >> --- a/drivers/of/base.c
> >> +++ b/drivers/of/base.c
> >> @@ -730,32 +730,45 @@ out:
> >> }
> >> EXPORT_SYMBOL(of_find_node_with_property);
> >>
> >> +static int of_match_type_or_name(const struct device_node *node,
> >> + const struct of_device_id *m)
> >> +{
> >> + int match = 1;
> >> +
> >> + if (m->name[0])
> >> + match &= node->name && !strcmp(m->name, node->name);
> >> +
> >> + if (m->type[0])
> >> + match &= node->type && !strcmp(m->type, node->type);
> >> +
> >> + return match;
> >> +}
> >> +
> >> static
> >> const struct of_device_id *__of_match_node(const struct of_device_id *matches,
> >> const struct device_node *node)
> >> {
> >> const char *cp;
> >> int cplen, l;
> >> + const struct of_device_id *m;
> >> + int match;
> >>
> >> if (!matches)
> >> return NULL;
> >>
> >> cp = __of_get_property(node, "compatible", &cplen);
> >> - do {
> >> - const struct of_device_id *m = matches;
> >> + while (cp && (cplen > 0)) {
> >> + m = matches;
> >>
> >> /* Check against matches with current compatible string */
> >> while (m->name[0] || m->type[0] || m->compatible[0]) {
> >> - int match = 1;
> >> - if (m->name[0])
> >> - match &= node->name
> >> - && !strcmp(m->name, node->name);
> >> - if (m->type[0])
> >> - match &= node->type
> >> - && !strcmp(m->type, node->type);
> >> - if (m->compatible[0])
> >> - match &= cp
> >> - && !of_compat_cmp(m->compatible, cp,
> >> + if (!m->compatible[0]) {
> >> + m++;
> >> + continue;
> >> + }
> >> +
> >> + match = of_match_type_or_name(node, m);
> >> + match &= cp && !of_compat_cmp(m->compatible, cp,
> >> strlen(m->compatible));
> >> if (match)
> >> return m;
> >> @@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
> >> }
> >>
> >> /* Get node's next compatible string */
> >> - if (cp) {
> >> - l = strlen(cp) + 1;
> >> - cp += l;
> >> - cplen -= l;
> >> - }
> >> - } while (cp && (cplen > 0));
> >> + l = strlen(cp) + 1;
> >> + cp += l;
> >> + cplen -= l;
> >> + }
> >> +
> >> + m = matches;
> >> + /* Check against matches without compatible string */
> >> + while (m->name[0] || m->type[0] || m->compatible[0]) {
> >> + if (!m->compatible[0] && of_match_type_or_name(node, m))
> >> + return m;
> >> + m++;
> >> + }
> >>
> >> return NULL;
> >> }
> >> --
> >> 1.8.5.3
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH] of: give priority to the compatible match in __of_match_node()
From: Scott Wood @ 2014-02-19 18:57 UTC (permalink / raw)
To: Paul Gortmaker
Cc: Chris Proctor, Kevin Hao, Arnd Bergmann,
devicetree@vger.kernel.org, Stephen N Chivers, Rob Herring,
Rob Herring, Grant Likely, linuxppc-dev, Sebastian Hesselbarth
In-Reply-To: <CAP=VYLr7hqnzW2HLS4GCMFeghCgPmrQZHYst+AUfZc_WNT-Wog@mail.gmail.com>
On Wed, 2014-02-19 at 13:25 -0500, Paul Gortmaker wrote:
> On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@gmail.com> wrote:
> > On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote:
> >> When the device node do have a compatible property, we definitely
> >> prefer the compatible match besides the type and name. Only if
> >> there is no such a match, we then consider the candidate which
> >> doesn't have compatible entry but do match the type or name with
> >> the device node.
> >>
> >> This is based on a patch from Sebastian Hesselbarth.
> >> http://patchwork.ozlabs.org/patch/319434/
> >>
> >> I did some code refactoring and also fixed a bug in the original patch.
> >
> > I'm inclined to just revert this once again and avoid possibly
> > breaking yet another platform.
>
> Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot
> on my sbc8548. It fails with:
> -----------------------------------------------
> libphy: Freescale PowerQUICC MII Bus: probed
> mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520
> PHY address 1312 is too large
> libphy: Freescale PowerQUICC MII Bus: probed
> libphy: Freescale PowerQUICC MII Bus: probed
> mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520
> PHY address 1312 is too large
> libphy: Freescale PowerQUICC MII Bus: probed
> TCP: cubic registered
> Initializing XFRM netlink socket
> NET: Registered protocol family 17
> <fail nfs mount>
> -----------------------------------------------
>
> On a normal boot, we should see this:
> -----------------------------------------------
> libphy: Freescale PowerQUICC MII Bus: probed
> libphy: Freescale PowerQUICC MII Bus: probed
> fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4
> fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd
> fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled
> fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256
> fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256
> fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4
> fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd
> fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled
> fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256
> fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256
> TCP: cubic registered
> Initializing XFRM netlink socket
> NET: Registered protocol family 17
> -----------------------------------------------
>
>
> Git bisect says:
>
> ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit
> commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4
> Author: Kevin Hao <haokexin@gmail.com>
> Date: Tue Feb 18 15:57:30 2014 +0800
>
> of: reimplement the matching method for __of_match_node()
>
> In the current implementation of __of_match_node(), it will compare
> each given match entry against all the node's compatible strings
> with of_device_is_compatible().
>
> To achieve multiple compatible strings per node with ordering from
> specific to generic, this requires given matches to be ordered from
> specific to generic. For most of the drivers this is not true and
> also an alphabetical ordering is more sane there.
>
> Therefore, we define a following priority order for the match, and
> then scan all the entries to find the best match.
> 1. specific compatible && type && name
> 2. specific compatible && type
> 3. specific compatible && name
> 4. specific compatible
> 5. general compatible && type && name
> 6. general compatible && type
> 7. general compatible && name
> 8. general compatible
> 9. type && name
> 10. type
> 11. name
>
> This is based on some pseudo-codes provided by Grant Likely.
>
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> [grant.likely: Changed multiplier to 4 which makes more sense]
> Signed-off-by: Grant Likely <grant.likely@linaro.org>
>
> :040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0
> 8401b0e3903e23e973845ee75b26b04345d803d2 M drivers
>
> As a double check, I checked out the head of linux-next, and did a
> revert of the above commit, and my sbc8548 can then boot properly.
>
> Not doing anything fancy ; using the defconfig exactly as-is, and
> ensuring the dtb is fresh from linux-next HEAD of today.
It looks like the new matching function doesn't check the validity of
the entire match (i.e. everything specified must match the node) before
scoring it.
The ethernet driver has this match (among others):
{
.type = "network",
.compatible = "gianfar",
},
...while the mdio driver has this match (among others):
{
.type = "mdio",
.compatible = "gianfar",
.data = &(struct fsl_pq_mdio_data) {
.mii_offset = offsetof(struct fsl_pq_mdio, mii),
.get_tbipa = get_gfar_tbipa,
},
},
(Yes, it's an awful device tree binding.)
It seems that the ethernet device is being bound to the mdio driver due
to the compatible match, even though type differs.
You could also end up with a .type and/or .name based match,
despite .compatible being present an a mismatch.
-Scott
^ permalink raw reply
* Re: Anyone using SysRQ key sequences on console serial port ?
From: Paul Gortmaker @ 2014-02-19 18:42 UTC (permalink / raw)
To: John Donnelly, Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <CAGtOQbQXLpdzAzxgwtzisEJ1MAeND8057103oUO-v9x4Ki8qYQ@mail.gmail.com>
[BTW, your html mail may be ignored by most people ; for example most of
the linux lists on vger.kernel.org actively reject it; top posting isn't
going to help either... ]
On 14-02-18 02:47 PM, John Donnelly wrote:
> I am enable to get one keyboard sequence responded to with the noted change
> in the dts .
>
> for instance:
>
> SysRQ ( Break) c
>
> Panics .. Which is a good response, and since it doesn't require a return
> to user mode ( I suspect ) it appears to work.
>
> Any other requests fail to report any information :
>
> SysRQ (break ) l - list active processes
> m - list memory
>
> Any additional SysRQ are ignored., and the system appears hung.
It must be something specific about your particular platform, or the
custom patches you have applied then. I just tested today's linux-next
tree (i.e. the latest bleeding edge stuff) and it still works for the
sbc8548 (defconfig + enable magic SYSRQ option). I did "s" (sync) multiple
times and "?" then "m" for memory dump (obviously those chars don't get
echo'd to the console....
---------------------------------------
root@sbc8548:~# cat /proc/cpuinfo |grep cpu
cpu : e500v2
root@sbc8548:~# SysRq : Emergency Sync
Emergency Sync complete
SysRq : Emergency Sync
Emergency Sync complete
SysRq : HELP : loglevel(0-9) reboot(b) crash(c) terminate-all-tasks(e) memory-full-oom-kill(f) kill-all-tasks(i) thaw-filesystems(j) show-memory-usage(m) nice-all-RT-tasks(n) poweroff(o) show-registers(p) show-all-timers(q) sync(s) show-task-states(t) unmount(u) show-blocked-tasks(w)
SysRq : Show Memory
Mem-Info:
DMA per-cpu:
CPU 0: hi: 186, btch: 31 usd: 132
active_anon:738 inactive_anon:29 isolated_anon:0
active_file:915 inactive_file:2219 isolated_file:0
unevictable:0 dirty:0 writeback:0 unstable:0
free:188325 slab_reclaimable:488 slab_unreclaimable:778
mapped:1006 shmem:44 pagetables:83 bounce:0
free_cma:0
DMA free:753300kB min:3520kB low:4400kB high:5280kB active_anon:2952kB inactive_anon:116kB active_file:3660kB inactive_file:8876kB unevictable:0kB isolated(anon):0kB isolated(file):0kB
present:786432kB managed:775916kB mlocked:0kB dirty:0kB writeback:0kB mapped:4024kB shmem:176kB slab_reclaimable:1952kB slab_unreclaimable:3112kB kernel_stack:264kB pagetables:332kB uns
table:0kB bounce:0kB free_cma:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
lowmem_reserve[]: 0 0 0
DMA: 403*4kB (UEM) 271*8kB (UEM) 131*16kB (UM) 35*32kB (UM) 15*64kB (UEM) 1*128kB (M) 1*256kB (M) 1*512kB (U) 1*1024kB (U) 3*2048kB (UEM) 180*4096kB (MR) = 753300kB
3178 total pagecache pages
0 pages in swap cache
Swap cache stats: add 0, delete 0, find 0/0
Free swap = 0kB
Total swap = 0kB
196608 pages RAM
0 pages HighMem/MovableOnly
2629 pages reserved
---------------------------------------
Paul.
--
>
> On an reference Intel platform, multiple SyqRQ can be issued and the
> system remains healthy .
>
>
>
>
>
>
>
>
>
> On Mon, Feb 17, 2014 at 1:37 PM, Scott Wood <scottwood@freescale.com> wrote:
>
>> On Sun, 2014-02-16 at 10:56 -0500, Paul Gortmaker wrote:
>>> On Fri, Feb 14, 2014 at 3:42 PM, John Donnelly <john.d@servergy.com>
>> wrote:
>>>> Hi,
>>>>
>>>> I tried using the SysRq hotkey sequence on a serial console -
>>>> 3.11.0-5-powerpc-e500mc system, by issuing a " break " and the system
>>>> immediately wedges after displaying "SysRQ : HELP : " using both
>> "Putty" and
>>>> "Teraterm" terminal emulators. I know the system is dead because my
>> ssh
>>>> sessions stopped too.
>>>
>>> Yes it does work -- or at least it _did_ work. Make sure your dts has
>> an entry
>>>
>>> compatible = "fsl,ns16550", "ns16550";
>>>
>>> since that enables a workaround I'd added for a hardware errata relating
>>> to sending breaks over the serial console. What you describe above
>>> makes me think you aren't getting the workaround enabled.
>>
>> Also make sure CONFIG_SERIAL_8250_FSL is enabled.
>>
>> -Scott
>>
>>
>>
>
>
^ permalink raw reply
* Re: [PATCH] of: give priority to the compatible match in __of_match_node()
From: Paul Gortmaker @ 2014-02-19 18:25 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree@vger.kernel.org, Kevin Hao, Arnd Bergmann,
Chris Proctor, Stephen N Chivers, Scott Wood, Rob Herring,
Grant Likely, linuxppc-dev, Sebastian Hesselbarth
In-Reply-To: <CAL_JsqKMi2H=vwoxrOt8QRA2xJeiLqBKKfLtt4QRCRoFk6JUHg@mail.gmail.com>
On Thu, Feb 13, 2014 at 2:01 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Wed, Feb 12, 2014 at 5:38 AM, Kevin Hao <haokexin@gmail.com> wrote:
>> When the device node do have a compatible property, we definitely
>> prefer the compatible match besides the type and name. Only if
>> there is no such a match, we then consider the candidate which
>> doesn't have compatible entry but do match the type or name with
>> the device node.
>>
>> This is based on a patch from Sebastian Hesselbarth.
>> http://patchwork.ozlabs.org/patch/319434/
>>
>> I did some code refactoring and also fixed a bug in the original patch.
>
> I'm inclined to just revert this once again and avoid possibly
> breaking yet another platform.
Well, for what it is worth, today's (Feb19th) linux-next tree fails to boot
on my sbc8548. It fails with:
-----------------------------------------------
libphy: Freescale PowerQUICC MII Bus: probed
mdio_bus ethernet@e002400: /soc8548@e0000000/ethernet@24000/mdio@520
PHY address 1312 is too large
libphy: Freescale PowerQUICC MII Bus: probed
libphy: Freescale PowerQUICC MII Bus: probed
mdio_bus ethernet@e002500: /soc8548@e0000000/ethernet@25000/mdio@520
PHY address 1312 is too large
libphy: Freescale PowerQUICC MII Bus: probed
TCP: cubic registered
Initializing XFRM netlink socket
NET: Registered protocol family 17
<fail nfs mount>
-----------------------------------------------
On a normal boot, we should see this:
-----------------------------------------------
libphy: Freescale PowerQUICC MII Bus: probed
libphy: Freescale PowerQUICC MII Bus: probed
fsl-gianfar e0024000.ethernet: enabled errata workarounds, flags: 0x4
fsl-gianfar e0024000.ethernet eth0: mac: 02:e0:0c:00:05:fd
fsl-gianfar e0024000.ethernet eth0: Running with NAPI enabled
fsl-gianfar e0024000.ethernet eth0: RX BD ring size for Q[0]: 256
fsl-gianfar e0024000.ethernet eth0: TX BD ring size for Q[0]: 256
fsl-gianfar e0025000.ethernet: enabled errata workarounds, flags: 0x4
fsl-gianfar e0025000.ethernet eth1: mac: 02:e0:0c:00:06:fd
fsl-gianfar e0025000.ethernet eth1: Running with NAPI enabled
fsl-gianfar e0025000.ethernet eth1: RX BD ring size for Q[0]: 256
fsl-gianfar e0025000.ethernet eth1: TX BD ring size for Q[0]: 256
TCP: cubic registered
Initializing XFRM netlink socket
NET: Registered protocol family 17
-----------------------------------------------
Git bisect says:
ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4 is the first bad commit
commit ee8b26ad943aa34acc03ae6cde2b81d8d3d483d4
Author: Kevin Hao <haokexin@gmail.com>
Date: Tue Feb 18 15:57:30 2014 +0800
of: reimplement the matching method for __of_match_node()
In the current implementation of __of_match_node(), it will compare
each given match entry against all the node's compatible strings
with of_device_is_compatible().
To achieve multiple compatible strings per node with ordering from
specific to generic, this requires given matches to be ordered from
specific to generic. For most of the drivers this is not true and
also an alphabetical ordering is more sane there.
Therefore, we define a following priority order for the match, and
then scan all the entries to find the best match.
1. specific compatible && type && name
2. specific compatible && type
3. specific compatible && name
4. specific compatible
5. general compatible && type && name
6. general compatible && type
7. general compatible && name
8. general compatible
9. type && name
10. type
11. name
This is based on some pseudo-codes provided by Grant Likely.
Signed-off-by: Kevin Hao <haokexin@gmail.com>
[grant.likely: Changed multiplier to 4 which makes more sense]
Signed-off-by: Grant Likely <grant.likely@linaro.org>
:040000 040000 8f5dd19174417aece63b308ff299a5dbe2efa5a0
8401b0e3903e23e973845ee75b26b04345d803d2 M drivers
As a double check, I checked out the head of linux-next, and did a
revert of the above commit, and my sbc8548 can then boot properly.
Not doing anything fancy ; using the defconfig exactly as-is, and
ensuring the dtb is fresh from linux-next HEAD of today.
Thanks,
Paul.
--
>
> However, I think I would like to see this structured differently. We
> basically have 2 ways of matching: the existing pre-3.14 way and the
> desired match on best compatible only. All new bindings should match
> with the new way and the old way needs to be kept for compatibility.
> So lets structure the code that way. Search the match table first for
> best compatible with name and type NULL, then search the table the old
> way. I realize it appears you are doing this, but it is not clear this
> is the intent of the code. I would like to see this written as a patch
> with commit 105353145eafb3ea919 reverted first and you add a new match
> function to call first and then fallback to the existing function.
>
> Rob
>
>>
>> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Signed-off-by: Kevin Hao <haokexin@gmail.com>
>> ---
>> drivers/of/base.c | 55 +++++++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 37 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index ff85450d5683..9d655df458bd 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -730,32 +730,45 @@ out:
>> }
>> EXPORT_SYMBOL(of_find_node_with_property);
>>
>> +static int of_match_type_or_name(const struct device_node *node,
>> + const struct of_device_id *m)
>> +{
>> + int match = 1;
>> +
>> + if (m->name[0])
>> + match &= node->name && !strcmp(m->name, node->name);
>> +
>> + if (m->type[0])
>> + match &= node->type && !strcmp(m->type, node->type);
>> +
>> + return match;
>> +}
>> +
>> static
>> const struct of_device_id *__of_match_node(const struct of_device_id *matches,
>> const struct device_node *node)
>> {
>> const char *cp;
>> int cplen, l;
>> + const struct of_device_id *m;
>> + int match;
>>
>> if (!matches)
>> return NULL;
>>
>> cp = __of_get_property(node, "compatible", &cplen);
>> - do {
>> - const struct of_device_id *m = matches;
>> + while (cp && (cplen > 0)) {
>> + m = matches;
>>
>> /* Check against matches with current compatible string */
>> while (m->name[0] || m->type[0] || m->compatible[0]) {
>> - int match = 1;
>> - if (m->name[0])
>> - match &= node->name
>> - && !strcmp(m->name, node->name);
>> - if (m->type[0])
>> - match &= node->type
>> - && !strcmp(m->type, node->type);
>> - if (m->compatible[0])
>> - match &= cp
>> - && !of_compat_cmp(m->compatible, cp,
>> + if (!m->compatible[0]) {
>> + m++;
>> + continue;
>> + }
>> +
>> + match = of_match_type_or_name(node, m);
>> + match &= cp && !of_compat_cmp(m->compatible, cp,
>> strlen(m->compatible));
>> if (match)
>> return m;
>> @@ -763,12 +776,18 @@ const struct of_device_id *__of_match_node(const struct of_device_id *matches,
>> }
>>
>> /* Get node's next compatible string */
>> - if (cp) {
>> - l = strlen(cp) + 1;
>> - cp += l;
>> - cplen -= l;
>> - }
>> - } while (cp && (cplen > 0));
>> + l = strlen(cp) + 1;
>> + cp += l;
>> + cplen -= l;
>> + }
>> +
>> + m = matches;
>> + /* Check against matches without compatible string */
>> + while (m->name[0] || m->type[0] || m->compatible[0]) {
>> + if (!m->compatible[0] && of_match_type_or_name(node, m))
>> + return m;
>> + m++;
>> + }
>>
>> return NULL;
>> }
>> --
>> 1.8.5.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Panic on ppc64 with numa_balancing and !sparsemem_vmemmap
From: Srikar Dronamraju @ 2014-02-19 18:02 UTC (permalink / raw)
To: Aneesh Kumar, riel, mgorman
Cc: Peter Zijlstra, paulus, linuxppc-dev, linux-mm
On a powerpc machine with CONFIG_NUMA_BALANCING=y and CONFIG_SPARSEMEM_VMEMMAP
not enabled, kernel panics.
This is true of kernel versions 3.13 to the latest commit 960dfc4 which is
3.14-rc3+. i.e the recent 3 fixups from Aneesh doesnt seem to help this case.
Sometimes it fails on boot up itself. Otherwise a kernel compile is good enough
to trigger the same. I am seeing this on a Power 7 box.
Kernel 3.14.0-rc3-mainline_v313-00168-g960dfc4 on an ppc64
transam2s-lp1 login: qla2xxx [0003:01:00.1]-8038:2: Cable is unplugged...
Unable to handle kernel paging request for data at address 0x00000457
Faulting instruction address: 0xc0000000000d6004
cpu 0x38: Vector: 300 (Data Access) at [c00000171561f700]
pc: c0000000000d6004: .task_numa_fault+0x604/0xa30
lr: c0000000000d62fc: .task_numa_fault+0x8fc/0xa30
sp: c00000171561f980
msr: 8000000000009032
dar: 457
dsisr: 40000000
current = 0xc0000017155d9b00
paca = 0xc00000000ec1e000 softe: 0 irq_happened: 0x00
pid = 16898, comm = gzip
enter ? for help
[c00000171561fa70] c0000000001b0fb0 .do_numa_page+0x1b0/0x2a0
[c00000171561fb20] c0000000001b2788 .handle_mm_fault+0x538/0xca0
[c00000171561fc00] c00000000082f498 .do_page_fault+0x378/0x880
[c00000171561fe30] c000000000009568 handle_page_fault+0x10/0x30
--- Exception: 301 (Data Access) at 00000000100031d8
SP (3fffd45ea2d0) is in userspace
38:mon>
(gdb) list *(task_numa_fault+0x604)
0xc0000000000d6004 is in task_numa_fault (/home/srikar/work/linux.git/include/linux/mm.h:753).
748 return cpupid_to_cpu(cpupid) == (-1 & LAST__CPU_MASK);
749 }
750
751 static inline bool __cpupid_match_pid(pid_t task_pid, int cpupid)
752 {
753 return (task_pid & LAST__PID_MASK) == cpupid_to_pid(cpupid);
754 }
755
756 #define cpupid_match_pid(task, cpupid) __cpupid_match_pid(task->pid, cpupid)
757 #ifdef LAST_CPUPID_NOT_IN_PAGE_FLAGS
(gdb)
However this doesnt seem to happen if we have CONFIG_SPARSEMEM_VMEMMAP=y set in the config.
--
Thanks nnn Regards
Srikar Dronamraju
^ permalink raw reply
* Re: ppc: RECLAIM_DISTANCE 10?
From: Nishanth Aravamudan @ 2014-02-19 16:26 UTC (permalink / raw)
To: Michal Hocko; +Cc: linux-mm, linuxppc-dev, Anton Blanchard, LKML
In-Reply-To: <20140219082313.GB14783@dhcp22.suse.cz>
On 19.02.2014 [09:23:13 +0100], Michal Hocko wrote:
> On Tue 18-02-14 15:34:05, Nishanth Aravamudan wrote:
> > Hi Michal,
> >
> > On 18.02.2014 [10:06:58 +0100], Michal Hocko wrote:
> > > Hi,
> > > I have just noticed that ppc has RECLAIM_DISTANCE reduced to 10 set by
> > > 56608209d34b (powerpc/numa: Set a smaller value for RECLAIM_DISTANCE to
> > > enable zone reclaim). The commit message suggests that the zone reclaim
> > > is desirable for all NUMA configurations.
> > >
> > > History has shown that the zone reclaim is more often harmful than
> > > helpful and leads to performance problems. The default RECLAIM_DISTANCE
> > > for generic case has been increased from 20 to 30 around 3.0
> > > (32e45ff43eaf mm: increase RECLAIM_DISTANCE to 30).
> >
> > Interesting.
> >
> > > I strongly suspect that the patch is incorrect and it should be
> > > reverted. Before I will send a revert I would like to understand what
> > > led to the patch in the first place. I do not see why would PPC use only
> > > LOCAL_DISTANCE and REMOTE_DISTANCE distances and in fact machines I have
> > > seen use different values.
> > >
> > > Anton, could you comment please?
> >
> > I'll let Anton comment here, but in looking into this issue in working
> > on CONFIG_HAVE_MEMORYLESS_NODE support, I realized that any LPAR with
> > memoryless nodes will set zone_reclaim_mode to 1. I think we want to
> > ignore memoryless nodes when we set up the reclaim mode like the
> > following? I'll send it as a proper patch if you agree?
>
> Funny enough, ppc memoryless node setup is what led me to this code.
> We had a setup like this:
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 0 size: 0 MB
> node 0 free: 0 MB
> node 2 cpus:
> node 2 size: 7168 MB
> node 2 free: 6019 MB
> node distances:
> node 0 2
> 0: 10 40
> 2: 40 10
Yeah, I think this happens fairly often ... and we didn't properly
support it (particularly with SLUB) on powerpc. I'll cc you on my
patchset.
> Which ends up enabling zone_reclaim although there is only a single node
> with memory. Not that RECLAIM_DISTANCE would make any difference here as
> the distance is even above default RECLAIM_DISTANCE.
>
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 5de4337..4f6ff6f 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1853,8 +1853,9 @@ static void __paginginit init_zone_allows_reclaim(int nid)
> > {
> > int i;
> >
> > - for_each_online_node(i)
> > - if (node_distance(nid, i) <= RECLAIM_DISTANCE)
> > + for_each_online_node(i) {
> > + if (node_distance(nid, i) <= RECLAIM_DISTANCE ||
> > + local_memory_node(nid) != nid)
> > node_set(i, NODE_DATA(nid)->reclaim_nodes);
> > else
> > zone_reclaim_mode = 1;
> >
> > Note, this won't actually do anything if CONFIG_HAVE_MEMORYLESS_NODES is
> > not set, but if it is, I think semantically it will indicate that
> > memoryless nodes *have* to reclaim remotely.
> >
> > And actually the above won't work, because the callpath is
> >
> > start_kernel -> setup_arch -> paging_init [-> free_area_init_nodes ->
> > free_area_init_node -> init_zone_allows_reclaim] which is called before
> > build_all_zonelists. This is a similar ordering problem as I'm having
> > with the MEMORYLESS_NODE support, will work on it.
>
> I think you just want for_each_node_state(nid, N_MEMORY) and skip all
> the memory less nodes, no?
Yep, thanks!
-Nish
^ permalink raw reply
* Re: ppc: RECLAIM_DISTANCE 10?
From: Nishanth Aravamudan @ 2014-02-19 16:24 UTC (permalink / raw)
To: David Rientjes
Cc: Michal Hocko, linux-mm, linuxppc-dev, Anton Blanchard, LKML
In-Reply-To: <alpine.DEB.2.02.1402181737530.17521@chino.kir.corp.google.com>
On 18.02.2014 [17:43:38 -0800], David Rientjes wrote:
> On Tue, 18 Feb 2014, Nishanth Aravamudan wrote:
>
> > How about the following?
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 5de4337..1a0eced 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1854,7 +1854,8 @@ static void __paginginit init_zone_allows_reclaim(int nid)
> > int i;
> >
> > for_each_online_node(i)
> > - if (node_distance(nid, i) <= RECLAIM_DISTANCE)
> > + if (node_distance(nid, i) <= RECLAIM_DISTANCE ||
> > + !NODE_DATA(i)->node_present_pages)
> > node_set(i, NODE_DATA(nid)->reclaim_nodes);
> > else
> > zone_reclaim_mode = 1;
>
> [ I changed the above from NODE_DATA(nid) -> NODE_DATA(i) as you caught
> so we're looking at the right code. ]
>
> That can't be right, it would allow reclaiming from a memoryless node. I
> think what you want is
Gah, you're right.
> for_each_online_node(i) {
> if (!node_present_pages(i))
> continue;
> if (node_distance(nid, i) <= RECLAIM_DISTANCE) {
> node_set(i, NODE_DATA(nid)->reclaim_nodes);
> continue;
> }
> /* Always try to reclaim locally */
> zone_reclaim_mode = 1;
> }
>
> but we really should be able to do for_each_node_state(i, N_MEMORY) here
> and memoryless nodes should already be excluded from that mask.
Yep, I found that afterwards, which simplifies the logic. I'll add this
to my series :)
<snip>
> > I think it's safe to move init_zone_allows_reclaim, because I don't
> > think any allocates are occurring here that could cause us to reclaim
> > anyways, right? Moving it allows us to safely reference
> > node_present_pages.
> >
>
> Yeah, this is fine.
Thanks,
Nish
^ permalink raw reply
* Re: ppc: RECLAIM_DISTANCE 10?
From: Nishanth Aravamudan @ 2014-02-19 16:33 UTC (permalink / raw)
To: David Rientjes
Cc: Michal Hocko, linux-mm, linuxppc-dev, Anton Blanchard, LKML
In-Reply-To: <20140219162438.GB27108@linux.vnet.ibm.com>
On 19.02.2014 [08:24:38 -0800], Nishanth Aravamudan wrote:
> On 18.02.2014 [17:43:38 -0800], David Rientjes wrote:
> > On Tue, 18 Feb 2014, Nishanth Aravamudan wrote:
> >
> > > How about the following?
> > >
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 5de4337..1a0eced 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -1854,7 +1854,8 @@ static void __paginginit init_zone_allows_reclaim(int nid)
> > > int i;
> > >
> > > for_each_online_node(i)
> > > - if (node_distance(nid, i) <= RECLAIM_DISTANCE)
> > > + if (node_distance(nid, i) <= RECLAIM_DISTANCE ||
> > > + !NODE_DATA(i)->node_present_pages)
> > > node_set(i, NODE_DATA(nid)->reclaim_nodes);
> > > else
> > > zone_reclaim_mode = 1;
> >
> > [ I changed the above from NODE_DATA(nid) -> NODE_DATA(i) as you caught
> > so we're looking at the right code. ]
> >
> > That can't be right, it would allow reclaiming from a memoryless node. I
> > think what you want is
>
> Gah, you're right.
>
> > for_each_online_node(i) {
> > if (!node_present_pages(i))
> > continue;
> > if (node_distance(nid, i) <= RECLAIM_DISTANCE) {
> > node_set(i, NODE_DATA(nid)->reclaim_nodes);
> > continue;
> > }
> > /* Always try to reclaim locally */
> > zone_reclaim_mode = 1;
> > }
> >
> > but we really should be able to do for_each_node_state(i, N_MEMORY) here
> > and memoryless nodes should already be excluded from that mask.
>
> Yep, I found that afterwards, which simplifies the logic. I'll add this
> to my series :)
In looking at the code, I am wondering about the following:
init_zone_allows_reclaim() is called for each nid from
free_area_init_node(). Which means that calculate_node_totalpages for
other "later" nids and check_for_memory() [which sets up the N_MEMORY
nodemask] hasn't been called yet.
So, would it make sense to pull up the
/* Any memory on that node */
if (pgdat->node_present_pages)
node_set_state(nid, N_MEMORY);
check_for_memory(pgdat, nid);
into free_area_init_node()?
Thanks,
Nish
^ permalink raw reply
* Re: [RFC PATCH 2/3] topology: support node_numa_mem() for determining the fallback node
From: Christoph Lameter @ 2014-02-19 16:11 UTC (permalink / raw)
To: Nishanth Aravamudan
Cc: Han Pingtian, Matt Mackall, Pekka Enberg,
Linux Memory Management List, Paul Mackerras, Anton Blanchard,
David Rientjes, Joonsoo Kim, linuxppc-dev, Wanpeng Li
In-Reply-To: <20140218222242.GA10844@linux.vnet.ibm.com>
On Tue, 18 Feb 2014, Nishanth Aravamudan wrote:
> the performance impact of the underlying NUMA configuration. I guess we
> could special-case memoryless/cpuless configurations somewhat, but I
> don't think there's any reason to do that if we can make memoryless-node
> support work in-kernel?
Well we can make it work in-kernel but it always has been a bit wacky (as
is the idea of numa "memory" nodes without memory).
^ permalink raw reply
* [PATCH RFC/RFT v3 0/9] drivers: cacheinfo support
From: Sudeep Holla @ 2014-02-19 16:06 UTC (permalink / raw)
To: linux-kernel
Cc: linux-s390, linux-ia64, Greg Kroah-Hartman, x86, sudeep.holla,
linux390, linuxppc-dev, linux-arm-kernel
From: Sudeep Holla <sudeep.holla@arm.com>
Hi,
This series adds a generic cacheinfo support similar to topology. The
implementation is based on x86 cacheinfo support. Currently x86, powerpc,
ia64 and s390 have their own implementations. While adding similar support
to ARM and ARM64, here is the attempt to make it generic quite similar to
topology info support. It also adds the missing ABI documentation for
the cacheinfo sysfs which is already being used.
It moves all the existing different implementations on x86, ia64, powerpc
and s390 to use the generic cacheinfo infrastructure introduced here.
These changes on non-ARM platforms are only compile tested and hence
the request for testing too.
This series also adds support for ARM and ARM64 architectures based on
the generic support.
Changes v2[2]->v3:
- Added new class "cpu" to group all cpu devices
- Converted all "raw" kobjects used in cacheinfo to device_attr
by creating cache index devices
- Added back s390 show_cacheinfo for /proc/cpuinfo
- Added disable_sysfs to cache_info for preventing a cache node
to be exposed through sysfs if required(used on s390)
Changes v1[1]->v2[2]:
- Extended the generic cacheinfo support to accomodate all
the existing implementations
- Moved all the existing implementations to use this new
generic infrastructure
- Added missing ABI documentation as suggested by Greg KH
- Added support for unimplemented CTR on pre-ARMv6 implementations
as suggested by Russell. However the ctr_info_list is not yet
populated
- not yet changed to device_attr as suggested by Greg KH,
registering cache as device won't eliminate the need of kobject
unless each index of cache is registered as a device which don't
seem to be good idea, but now it's unified it can be done easily
in one place if needed
[1] https://lkml.org/lkml/2014/1/8/523
[2] https://lkml.org/lkml/2014/2/7/654
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-ia64@vger.kernel.org
Cc: linux390@de.ibm.com
Cc: linux-s390@vger.kernel.org
Cc: x86@kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-arm-kernel@lists.infradead.org
---
Sudeep Holla (9):
drivers: base: add new class "cpu" to group cpu devices
drivers: base: support cpu cache information interface to userspace
via sysfs
ia64: move cacheinfo sysfs to generic cacheinfo infrastructure
s390: move cacheinfo sysfs to generic cacheinfo infrastructure
x86: move cacheinfo sysfs to generic cacheinfo infrastructure
powerpc: move cacheinfo sysfs to generic cacheinfo infrastructure
ARM64: kernel: add support for cpu cache information
ARM: kernel: add support for cpu cache information
ARM: kernel: add outer cache support for cacheinfo implementation
Documentation/ABI/testing/sysfs-devices-system-cpu | 40 +
arch/arm/include/asm/outercache.h | 13 +
arch/arm/kernel/Makefile | 1 +
arch/arm/kernel/cacheinfo.c | 248 ++++++
arch/arm/mm/Kconfig | 13 +
arch/arm/mm/cache-l2x0.c | 14 +
arch/arm/mm/cache-tauros2.c | 35 +
arch/arm/mm/cache-xsc3l2.c | 15 +
arch/arm64/kernel/Makefile | 2 +-
arch/arm64/kernel/cacheinfo.c | 134 ++++
arch/ia64/kernel/topology.c | 399 ++--------
arch/powerpc/kernel/cacheinfo.c | 831 +++------------------
arch/powerpc/kernel/cacheinfo.h | 8 -
arch/powerpc/kernel/sysfs.c | 4 -
arch/s390/kernel/cache.c | 388 +++-------
arch/x86/kernel/cpu/intel_cacheinfo.c | 647 ++++------------
drivers/base/Makefile | 2 +-
drivers/base/cacheinfo.c | 485 ++++++++++++
drivers/base/core.c | 35 +-
drivers/base/cpu.c | 7 +
include/linux/cacheinfo.h | 55 ++
include/linux/cpu.h | 2 +
22 files changed, 1523 insertions(+), 1855 deletions(-)
create mode 100644 arch/arm/kernel/cacheinfo.c
create mode 100644 arch/arm64/kernel/cacheinfo.c
delete mode 100644 arch/powerpc/kernel/cacheinfo.h
create mode 100644 drivers/base/cacheinfo.c
create mode 100644 include/linux/cacheinfo.h
--
1.8.3.2
^ permalink raw reply
* [PATCH RFC/RFT v3 6/9] powerpc: move cacheinfo sysfs to generic cacheinfo infrastructure
From: Sudeep Holla @ 2014-02-19 16:06 UTC (permalink / raw)
To: linux-kernel; +Cc: Paul Mackerras, linuxppc-dev, sudeep.holla
In-Reply-To: <1392825976-17633-1-git-send-email-sudeep.holla@arm.com>
From: Sudeep Holla <sudeep.holla@arm.com>
This patch removes the redundant sysfs cacheinfo code by making use of
the newly introduced generic cacheinfo infrastructure.
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org
---
arch/powerpc/kernel/cacheinfo.c | 831 ++++++----------------------------------
arch/powerpc/kernel/cacheinfo.h | 8 -
arch/powerpc/kernel/sysfs.c | 4 -
3 files changed, 109 insertions(+), 734 deletions(-)
delete mode 100644 arch/powerpc/kernel/cacheinfo.h
diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c
index 2912b87..05b7580 100644
--- a/arch/powerpc/kernel/cacheinfo.c
+++ b/arch/powerpc/kernel/cacheinfo.c
@@ -10,38 +10,10 @@
* 2 as published by the Free Software Foundation.
*/
+#include <linux/cacheinfo.h>
#include <linux/cpu.h>
-#include <linux/cpumask.h>
#include <linux/kernel.h>
-#include <linux/kobject.h>
-#include <linux/list.h>
-#include <linux/notifier.h>
#include <linux/of.h>
-#include <linux/percpu.h>
-#include <linux/slab.h>
-#include <asm/prom.h>
-
-#include "cacheinfo.h"
-
-/* per-cpu object for tracking:
- * - a "cache" kobject for the top-level directory
- * - a list of "index" objects representing the cpu's local cache hierarchy
- */
-struct cache_dir {
- struct kobject *kobj; /* bare (not embedded) kobject for cache
- * directory */
- struct cache_index_dir *index; /* list of index objects */
-};
-
-/* "index" object: each cpu's cache directory has an index
- * subdirectory corresponding to a cache object associated with the
- * cpu. This object's lifetime is managed via the embedded kobject.
- */
-struct cache_index_dir {
- struct kobject kobj;
- struct cache_index_dir *next; /* next index in parent directory */
- struct cache *cache;
-};
/* Template for determining which OF properties to query for a given
* cache type */
@@ -60,11 +32,6 @@ struct cache_type_info {
const char *nr_sets_prop;
};
-/* These are used to index the cache_type_info array. */
-#define CACHE_TYPE_UNIFIED 0
-#define CACHE_TYPE_INSTRUCTION 1
-#define CACHE_TYPE_DATA 2
-
static const struct cache_type_info cache_type_info[] = {
{
/* PowerPC Processor binding says the [di]-cache-*
@@ -77,246 +44,115 @@ static const struct cache_type_info cache_type_info[] = {
.nr_sets_prop = "d-cache-sets",
},
{
- .name = "Instruction",
- .size_prop = "i-cache-size",
- .line_size_props = { "i-cache-line-size",
- "i-cache-block-size", },
- .nr_sets_prop = "i-cache-sets",
- },
- {
.name = "Data",
.size_prop = "d-cache-size",
.line_size_props = { "d-cache-line-size",
"d-cache-block-size", },
.nr_sets_prop = "d-cache-sets",
},
+ {
+ .name = "Instruction",
+ .size_prop = "i-cache-size",
+ .line_size_props = { "i-cache-line-size",
+ "i-cache-block-size", },
+ .nr_sets_prop = "i-cache-sets",
+ },
};
-/* Cache object: each instance of this corresponds to a distinct cache
- * in the system. There are separate objects for Harvard caches: one
- * each for instruction and data, and each refers to the same OF node.
- * The refcount of the OF node is elevated for the lifetime of the
- * cache object. A cache object is released when its shared_cpu_map
- * is cleared (see cache_cpu_clear).
- *
- * A cache object is on two lists: an unsorted global list
- * (cache_list) of cache objects; and a singly-linked list
- * representing the local cache hierarchy, which is ordered by level
- * (e.g. L1d -> L1i -> L2 -> L3).
- */
-struct cache {
- struct device_node *ofnode; /* OF node for this cache, may be cpu */
- struct cpumask shared_cpu_map; /* online CPUs using this cache */
- int type; /* split cache disambiguation */
- int level; /* level not explicit in device tree */
- struct list_head list; /* global list of cache objects */
- struct cache *next_local; /* next cache of >= level */
-};
-
-static DEFINE_PER_CPU(struct cache_dir *, cache_dir_pcpu);
-
-/* traversal/modification of this list occurs only at cpu hotplug time;
- * access is serialized by cpu hotplug locking
- */
-static LIST_HEAD(cache_list);
-
-static struct cache_index_dir *kobj_to_cache_index_dir(struct kobject *k)
-{
- return container_of(k, struct cache_index_dir, kobj);
-}
-
-static const char *cache_type_string(const struct cache *cache)
+static inline int get_cacheinfo_idx(enum cache_type type)
{
- return cache_type_info[cache->type].name;
-}
-
-static void cache_init(struct cache *cache, int type, int level,
- struct device_node *ofnode)
-{
- cache->type = type;
- cache->level = level;
- cache->ofnode = of_node_get(ofnode);
- INIT_LIST_HEAD(&cache->list);
- list_add(&cache->list, &cache_list);
-}
-
-static struct cache *new_cache(int type, int level, struct device_node *ofnode)
-{
- struct cache *cache;
-
- cache = kzalloc(sizeof(*cache), GFP_KERNEL);
- if (cache)
- cache_init(cache, type, level, ofnode);
-
- return cache;
-}
-
-static void release_cache_debugcheck(struct cache *cache)
-{
- struct cache *iter;
-
- list_for_each_entry(iter, &cache_list, list)
- WARN_ONCE(iter->next_local == cache,
- "cache for %s(%s) refers to cache for %s(%s)\n",
- iter->ofnode->full_name,
- cache_type_string(iter),
- cache->ofnode->full_name,
- cache_type_string(cache));
-}
-
-static void release_cache(struct cache *cache)
-{
- if (!cache)
- return;
-
- pr_debug("freeing L%d %s cache for %s\n", cache->level,
- cache_type_string(cache), cache->ofnode->full_name);
-
- release_cache_debugcheck(cache);
- list_del(&cache->list);
- of_node_put(cache->ofnode);
- kfree(cache);
-}
-
-static void cache_cpu_set(struct cache *cache, int cpu)
-{
- struct cache *next = cache;
-
- while (next) {
- WARN_ONCE(cpumask_test_cpu(cpu, &next->shared_cpu_map),
- "CPU %i already accounted in %s(%s)\n",
- cpu, next->ofnode->full_name,
- cache_type_string(next));
- cpumask_set_cpu(cpu, &next->shared_cpu_map);
- next = next->next_local;
- }
+ if (type == CACHE_TYPE_UNIFIED)
+ return 0;
+ else
+ return type;
}
-static int cache_size(const struct cache *cache, unsigned int *ret)
+static int cache_size(struct cache_info *this_leaf)
{
const char *propname;
const __be32 *cache_size;
+ int ct_idx;
- propname = cache_type_info[cache->type].size_prop;
-
- cache_size = of_get_property(cache->ofnode, propname, NULL);
- if (!cache_size)
- return -ENODEV;
-
- *ret = of_read_number(cache_size, 1);
- return 0;
-}
-
-static int cache_size_kb(const struct cache *cache, unsigned int *ret)
-{
- unsigned int size;
+ ct_idx = get_cacheinfo_idx(this_leaf->type);
+ propname = cache_type_info[ct_idx].size_prop;
- if (cache_size(cache, &size))
+ cache_size = of_get_property(this_leaf->of_node, propname, NULL);
+ if (!cache_size) {
+ this_leaf->size = 0;
return -ENODEV;
-
- *ret = size / 1024;
- return 0;
+ } else {
+ this_leaf->size = of_read_number(cache_size, 1);
+ return 0;
+ }
}
/* not cache_line_size() because that's a macro in include/linux/cache.h */
-static int cache_get_line_size(const struct cache *cache, unsigned int *ret)
+static int cache_get_line_size(struct cache_info *this_leaf)
{
const __be32 *line_size;
- int i, lim;
+ int i, lim, ct_idx;
- lim = ARRAY_SIZE(cache_type_info[cache->type].line_size_props);
+ ct_idx = get_cacheinfo_idx(this_leaf->type);
+ lim = ARRAY_SIZE(cache_type_info[ct_idx].line_size_props);
for (i = 0; i < lim; i++) {
const char *propname;
- propname = cache_type_info[cache->type].line_size_props[i];
- line_size = of_get_property(cache->ofnode, propname, NULL);
+ propname = cache_type_info[ct_idx].line_size_props[i];
+ line_size = of_get_property(this_leaf->of_node, propname, NULL);
if (line_size)
break;
}
- if (!line_size)
+ if (!line_size) {
+ this_leaf->coherency_line_size = 0;
return -ENODEV;
-
- *ret = of_read_number(line_size, 1);
- return 0;
+ } else {
+ this_leaf->coherency_line_size = of_read_number(line_size, 1);
+ return 0;
+ }
}
-static int cache_nr_sets(const struct cache *cache, unsigned int *ret)
+static int cache_nr_sets(struct cache_info *this_leaf)
{
const char *propname;
const __be32 *nr_sets;
+ int ct_idx;
- propname = cache_type_info[cache->type].nr_sets_prop;
+ ct_idx = get_cacheinfo_idx(this_leaf->type);
+ propname = cache_type_info[ct_idx].nr_sets_prop;
- nr_sets = of_get_property(cache->ofnode, propname, NULL);
- if (!nr_sets)
+ nr_sets = of_get_property(this_leaf->of_node, propname, NULL);
+ if (!nr_sets) {
+ this_leaf->number_of_sets = 0;
return -ENODEV;
-
- *ret = of_read_number(nr_sets, 1);
- return 0;
+ } else {
+ this_leaf->number_of_sets = of_read_number(nr_sets, 1);
+ return 0;
+ }
}
-static int cache_associativity(const struct cache *cache, unsigned int *ret)
+static int cache_associativity(struct cache_info *this_leaf)
{
- unsigned int line_size;
- unsigned int nr_sets;
- unsigned int size;
-
- if (cache_nr_sets(cache, &nr_sets))
- goto err;
+ unsigned int line_size = this_leaf->coherency_line_size;
+ unsigned int nr_sets = this_leaf->number_of_sets;
+ unsigned int size = this_leaf->size;
/* If the cache is fully associative, there is no need to
* check the other properties.
*/
if (nr_sets == 1) {
- *ret = 0;
+ this_leaf->ways_of_associativity = 0;
return 0;
}
- if (cache_get_line_size(cache, &line_size))
- goto err;
- if (cache_size(cache, &size))
- goto err;
-
- if (!(nr_sets > 0 && size > 0 && line_size > 0))
- goto err;
-
- *ret = (size / nr_sets) / line_size;
- return 0;
-err:
- return -ENODEV;
-}
-
-/* helper for dealing with split caches */
-static struct cache *cache_find_first_sibling(struct cache *cache)
-{
- struct cache *iter;
-
- if (cache->type == CACHE_TYPE_UNIFIED)
- return cache;
-
- list_for_each_entry(iter, &cache_list, list)
- if (iter->ofnode == cache->ofnode && iter->next_local == cache)
- return iter;
-
- return cache;
-}
-
-/* return the first cache on a local list matching node */
-static struct cache *cache_lookup_by_node(const struct device_node *node)
-{
- struct cache *cache = NULL;
- struct cache *iter;
-
- list_for_each_entry(iter, &cache_list, list) {
- if (iter->ofnode != node)
- continue;
- cache = cache_find_first_sibling(iter);
- break;
+ if (!(nr_sets > 0 && size > 0 && line_size > 0)) {
+ this_leaf->ways_of_associativity = 0;
+ return -ENODEV;
+ } else {
+ this_leaf->ways_of_associativity = (size / nr_sets) / line_size;
+ return 0;
}
-
- return cache;
}
static bool cache_node_is_unified(const struct device_node *np)
@@ -324,523 +160,74 @@ static bool cache_node_is_unified(const struct device_node *np)
return of_get_property(np, "cache-unified", NULL);
}
-static struct cache *cache_do_one_devnode_unified(struct device_node *node,
- int level)
-{
- struct cache *cache;
-
- pr_debug("creating L%d ucache for %s\n", level, node->full_name);
-
- cache = new_cache(CACHE_TYPE_UNIFIED, level, node);
-
- return cache;
-}
-
-static struct cache *cache_do_one_devnode_split(struct device_node *node,
- int level)
-{
- struct cache *dcache, *icache;
-
- pr_debug("creating L%d dcache and icache for %s\n", level,
- node->full_name);
-
- dcache = new_cache(CACHE_TYPE_DATA, level, node);
- icache = new_cache(CACHE_TYPE_INSTRUCTION, level, node);
-
- if (!dcache || !icache)
- goto err;
-
- dcache->next_local = icache;
-
- return dcache;
-err:
- release_cache(dcache);
- release_cache(icache);
- return NULL;
-}
-
-static struct cache *cache_do_one_devnode(struct device_node *node, int level)
-{
- struct cache *cache;
-
- if (cache_node_is_unified(node))
- cache = cache_do_one_devnode_unified(node, level);
- else
- cache = cache_do_one_devnode_split(node, level);
-
- return cache;
-}
-
-static struct cache *cache_lookup_or_instantiate(struct device_node *node,
- int level)
-{
- struct cache *cache;
-
- cache = cache_lookup_by_node(node);
-
- WARN_ONCE(cache && cache->level != level,
- "cache level mismatch on lookup (got %d, expected %d)\n",
- cache->level, level);
-
- if (!cache)
- cache = cache_do_one_devnode(node, level);
-
- return cache;
-}
-
-static void link_cache_lists(struct cache *smaller, struct cache *bigger)
-{
- while (smaller->next_local) {
- if (smaller->next_local == bigger)
- return; /* already linked */
- smaller = smaller->next_local;
- }
-
- smaller->next_local = bigger;
-}
-
-static void do_subsidiary_caches_debugcheck(struct cache *cache)
-{
- WARN_ON_ONCE(cache->level != 1);
- WARN_ON_ONCE(strcmp(cache->ofnode->type, "cpu"));
-}
-
-static void do_subsidiary_caches(struct cache *cache)
-{
- struct device_node *subcache_node;
- int level = cache->level;
-
- do_subsidiary_caches_debugcheck(cache);
-
- while ((subcache_node = of_find_next_cache_node(cache->ofnode))) {
- struct cache *subcache;
-
- level++;
- subcache = cache_lookup_or_instantiate(subcache_node, level);
- of_node_put(subcache_node);
- if (!subcache)
- break;
-
- link_cache_lists(cache, subcache);
- cache = subcache;
- }
-}
-
-static struct cache *cache_chain_instantiate(unsigned int cpu_id)
-{
- struct device_node *cpu_node;
- struct cache *cpu_cache = NULL;
-
- pr_debug("creating cache object(s) for CPU %i\n", cpu_id);
-
- cpu_node = of_get_cpu_node(cpu_id, NULL);
- WARN_ONCE(!cpu_node, "no OF node found for CPU %i\n", cpu_id);
- if (!cpu_node)
- goto out;
-
- cpu_cache = cache_lookup_or_instantiate(cpu_node, 1);
- if (!cpu_cache)
- goto out;
-
- do_subsidiary_caches(cpu_cache);
-
- cache_cpu_set(cpu_cache, cpu_id);
-out:
- of_node_put(cpu_node);
-
- return cpu_cache;
-}
-
-static struct cache_dir *cacheinfo_create_cache_dir(unsigned int cpu_id)
-{
- struct cache_dir *cache_dir;
- struct device *dev;
- struct kobject *kobj = NULL;
-
- dev = get_cpu_device(cpu_id);
- WARN_ONCE(!dev, "no dev for CPU %i\n", cpu_id);
- if (!dev)
- goto err;
-
- kobj = kobject_create_and_add("cache", &dev->kobj);
- if (!kobj)
- goto err;
-
- cache_dir = kzalloc(sizeof(*cache_dir), GFP_KERNEL);
- if (!cache_dir)
- goto err;
-
- cache_dir->kobj = kobj;
-
- WARN_ON_ONCE(per_cpu(cache_dir_pcpu, cpu_id) != NULL);
-
- per_cpu(cache_dir_pcpu, cpu_id) = cache_dir;
-
- return cache_dir;
-err:
- kobject_put(kobj);
- return NULL;
-}
-
-static void cache_index_release(struct kobject *kobj)
-{
- struct cache_index_dir *index;
-
- index = kobj_to_cache_index_dir(kobj);
-
- pr_debug("freeing index directory for L%d %s cache\n",
- index->cache->level, cache_type_string(index->cache));
-
- kfree(index);
-}
-
-static ssize_t cache_index_show(struct kobject *k, struct attribute *attr, char *buf)
+static void ci_leaf_init(struct cache_info *this_leaf,
+ enum cache_type type, unsigned int level)
{
- struct kobj_attribute *kobj_attr;
-
- kobj_attr = container_of(attr, struct kobj_attribute, attr);
-
- return kobj_attr->show(k, kobj_attr, buf);
-}
-
-static struct cache *index_kobj_to_cache(struct kobject *k)
-{
- struct cache_index_dir *index;
-
- index = kobj_to_cache_index_dir(k);
-
- return index->cache;
+ this_leaf->level = level;
+ this_leaf->type = type;
+ cache_size(this_leaf);
+ cache_get_line_size(this_leaf);
+ cache_nr_sets(this_leaf);
+ cache_associativity(this_leaf);
}
-static ssize_t size_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
+int init_cache_level(unsigned int cpu)
{
- unsigned int size_kb;
- struct cache *cache;
+ struct device_node *np;
+ struct device *cpu_dev = get_cpu_device(cpu);
+ struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
+ unsigned int level = 0, leaves = 0;
- cache = index_kobj_to_cache(k);
-
- if (cache_size_kb(cache, &size_kb))
+ if (!cpu_dev) {
+ pr_err("No cpu device for CPU %d\n", cpu);
return -ENODEV;
-
- return sprintf(buf, "%uK\n", size_kb);
-}
-
-static struct kobj_attribute cache_size_attr =
- __ATTR(size, 0444, size_show, NULL);
-
-
-static ssize_t line_size_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
-{
- unsigned int line_size;
- struct cache *cache;
-
- cache = index_kobj_to_cache(k);
-
- if (cache_get_line_size(cache, &line_size))
- return -ENODEV;
-
- return sprintf(buf, "%u\n", line_size);
-}
-
-static struct kobj_attribute cache_line_size_attr =
- __ATTR(coherency_line_size, 0444, line_size_show, NULL);
-
-static ssize_t nr_sets_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
-{
- unsigned int nr_sets;
- struct cache *cache;
-
- cache = index_kobj_to_cache(k);
-
- if (cache_nr_sets(cache, &nr_sets))
- return -ENODEV;
-
- return sprintf(buf, "%u\n", nr_sets);
-}
-
-static struct kobj_attribute cache_nr_sets_attr =
- __ATTR(number_of_sets, 0444, nr_sets_show, NULL);
-
-static ssize_t associativity_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
-{
- unsigned int associativity;
- struct cache *cache;
-
- cache = index_kobj_to_cache(k);
-
- if (cache_associativity(cache, &associativity))
- return -ENODEV;
-
- return sprintf(buf, "%u\n", associativity);
-}
-
-static struct kobj_attribute cache_assoc_attr =
- __ATTR(ways_of_associativity, 0444, associativity_show, NULL);
-
-static ssize_t type_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
-{
- struct cache *cache;
-
- cache = index_kobj_to_cache(k);
-
- return sprintf(buf, "%s\n", cache_type_string(cache));
-}
-
-static struct kobj_attribute cache_type_attr =
- __ATTR(type, 0444, type_show, NULL);
-
-static ssize_t level_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
-{
- struct cache_index_dir *index;
- struct cache *cache;
-
- index = kobj_to_cache_index_dir(k);
- cache = index->cache;
-
- return sprintf(buf, "%d\n", cache->level);
-}
-
-static struct kobj_attribute cache_level_attr =
- __ATTR(level, 0444, level_show, NULL);
-
-static ssize_t shared_cpu_map_show(struct kobject *k, struct kobj_attribute *attr, char *buf)
-{
- struct cache_index_dir *index;
- struct cache *cache;
- int len;
- int n = 0;
-
- index = kobj_to_cache_index_dir(k);
- cache = index->cache;
- len = PAGE_SIZE - 2;
-
- if (len > 1) {
- n = cpumask_scnprintf(buf, len, &cache->shared_cpu_map);
- buf[n++] = '\n';
- buf[n] = '\0';
}
- return n;
-}
-
-static struct kobj_attribute cache_shared_cpu_map_attr =
- __ATTR(shared_cpu_map, 0444, shared_cpu_map_show, NULL);
-
-/* Attributes which should always be created -- the kobject/sysfs core
- * does this automatically via kobj_type->default_attrs. This is the
- * minimum data required to uniquely identify a cache.
- */
-static struct attribute *cache_index_default_attrs[] = {
- &cache_type_attr.attr,
- &cache_level_attr.attr,
- &cache_shared_cpu_map_attr.attr,
- NULL,
-};
-
-/* Attributes which should be created if the cache device node has the
- * right properties -- see cacheinfo_create_index_opt_attrs
- */
-static struct kobj_attribute *cache_index_opt_attrs[] = {
- &cache_size_attr,
- &cache_line_size_attr,
- &cache_nr_sets_attr,
- &cache_assoc_attr,
-};
-
-static const struct sysfs_ops cache_index_ops = {
- .show = cache_index_show,
-};
-
-static struct kobj_type cache_index_type = {
- .release = cache_index_release,
- .sysfs_ops = &cache_index_ops,
- .default_attrs = cache_index_default_attrs,
-};
-
-static void cacheinfo_create_index_opt_attrs(struct cache_index_dir *dir)
-{
- const char *cache_name;
- const char *cache_type;
- struct cache *cache;
- char *buf;
- int i;
-
- buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
- if (!buf)
- return;
-
- cache = dir->cache;
- cache_name = cache->ofnode->full_name;
- cache_type = cache_type_string(cache);
-
- /* We don't want to create an attribute that can't provide a
- * meaningful value. Check the return value of each optional
- * attribute's ->show method before registering the
- * attribute.
- */
- for (i = 0; i < ARRAY_SIZE(cache_index_opt_attrs); i++) {
- struct kobj_attribute *attr;
- ssize_t rc;
-
- attr = cache_index_opt_attrs[i];
-
- rc = attr->show(&dir->kobj, attr, buf);
- if (rc <= 0) {
- pr_debug("not creating %s attribute for "
- "%s(%s) (rc = %zd)\n",
- attr->attr.name, cache_name,
- cache_type, rc);
- continue;
- }
- if (sysfs_create_file(&dir->kobj, &attr->attr))
- pr_debug("could not create %s attribute for %s(%s)\n",
- attr->attr.name, cache_name, cache_type);
+ np = cpu_dev->of_node;
+ if (!np) {
+ pr_err("Failed to find cpu%d device node\n", cpu);
+ return -ENOENT;
}
- kfree(buf);
-}
-
-static void cacheinfo_create_index_dir(struct cache *cache, int index,
- struct cache_dir *cache_dir)
-{
- struct cache_index_dir *index_dir;
- int rc;
-
- index_dir = kzalloc(sizeof(*index_dir), GFP_KERNEL);
- if (!index_dir)
- goto err;
-
- index_dir->cache = cache;
-
- rc = kobject_init_and_add(&index_dir->kobj, &cache_index_type,
- cache_dir->kobj, "index%d", index);
- if (rc)
- goto err;
-
- index_dir->next = cache_dir->index;
- cache_dir->index = index_dir;
-
- cacheinfo_create_index_opt_attrs(index_dir);
-
- return;
-err:
- kfree(index_dir);
-}
-
-static void cacheinfo_sysfs_populate(unsigned int cpu_id,
- struct cache *cache_list)
-{
- struct cache_dir *cache_dir;
- struct cache *cache;
- int index = 0;
-
- cache_dir = cacheinfo_create_cache_dir(cpu_id);
- if (!cache_dir)
- return;
-
- cache = cache_list;
- while (cache) {
- cacheinfo_create_index_dir(cache, index, cache_dir);
- index++;
- cache = cache->next_local;
+ while (np) {
+ leaves += cache_node_is_unified(np) ? 1 : 2;
+ level++;
+ of_node_put(np);
+ np = of_find_next_cache_node(np);
}
-}
-
-void cacheinfo_cpu_online(unsigned int cpu_id)
-{
- struct cache *cache;
-
- cache = cache_chain_instantiate(cpu_id);
- if (!cache)
- return;
-
- cacheinfo_sysfs_populate(cpu_id, cache);
-}
-
-#ifdef CONFIG_HOTPLUG_CPU /* functions needed for cpu offline */
-
-static struct cache *cache_lookup_by_cpu(unsigned int cpu_id)
-{
- struct device_node *cpu_node;
- struct cache *cache;
-
- cpu_node = of_get_cpu_node(cpu_id, NULL);
- WARN_ONCE(!cpu_node, "no OF node found for CPU %i\n", cpu_id);
- if (!cpu_node)
- return NULL;
-
- cache = cache_lookup_by_node(cpu_node);
- of_node_put(cpu_node);
+ this_cpu_ci->num_levels = level;
+ this_cpu_ci->num_leaves = leaves;
- return cache;
+ return 0;
}
-static void remove_index_dirs(struct cache_dir *cache_dir)
+int populate_cache_leaves(unsigned int cpu)
{
- struct cache_index_dir *index;
-
- index = cache_dir->index;
+ struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
+ struct cache_info *this_leaf = this_cpu_ci->info_list;
+ struct device *cpu_dev = get_cpu_device(cpu);
+ struct device_node *np;
+ unsigned int level, idx;
- while (index) {
- struct cache_index_dir *next;
-
- next = index->next;
- kobject_put(&index->kobj);
- index = next;
+ np = of_node_get(cpu_dev->of_node);
+ if (!np) {
+ pr_err("Failed to find cpu%d device node\n", cpu);
+ return -ENOENT;
}
-}
-
-static void remove_cache_dir(struct cache_dir *cache_dir)
-{
- remove_index_dirs(cache_dir);
- /* Remove cache dir from sysfs */
- kobject_del(cache_dir->kobj);
-
- kobject_put(cache_dir->kobj);
-
- kfree(cache_dir);
-}
-
-static void cache_cpu_clear(struct cache *cache, int cpu)
-{
- while (cache) {
- struct cache *next = cache->next_local;
-
- WARN_ONCE(!cpumask_test_cpu(cpu, &cache->shared_cpu_map),
- "CPU %i not accounted in %s(%s)\n",
- cpu, cache->ofnode->full_name,
- cache_type_string(cache));
-
- cpumask_clear_cpu(cpu, &cache->shared_cpu_map);
-
- /* Release the cache object if all the cpus using it
- * are offline */
- if (cpumask_empty(&cache->shared_cpu_map))
- release_cache(cache);
-
- cache = next;
+ for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
+ idx < this_cpu_ci->num_leaves; idx++, level++) {
+ if (!this_leaf)
+ return -EINVAL;
+
+ this_leaf->of_node = np;
+ if (cache_node_is_unified(np)) {
+ ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level);
+ } else {
+ ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
+ ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
+ }
+ np = of_find_next_cache_node(np);
}
+ return 0;
}
-void cacheinfo_cpu_offline(unsigned int cpu_id)
-{
- struct cache_dir *cache_dir;
- struct cache *cache;
-
- /* Prevent userspace from seeing inconsistent state - remove
- * the sysfs hierarchy first */
- cache_dir = per_cpu(cache_dir_pcpu, cpu_id);
-
- /* careful, sysfs population may have failed */
- if (cache_dir)
- remove_cache_dir(cache_dir);
-
- per_cpu(cache_dir_pcpu, cpu_id) = NULL;
-
- /* clear the CPU's bit in its cache chain, possibly freeing
- * cache objects */
- cache = cache_lookup_by_cpu(cpu_id);
- if (cache)
- cache_cpu_clear(cache, cpu_id);
-}
-#endif /* CONFIG_HOTPLUG_CPU */
diff --git a/arch/powerpc/kernel/cacheinfo.h b/arch/powerpc/kernel/cacheinfo.h
deleted file mode 100644
index a7b74d3..0000000
--- a/arch/powerpc/kernel/cacheinfo.h
+++ /dev/null
@@ -1,8 +0,0 @@
-#ifndef _PPC_CACHEINFO_H
-#define _PPC_CACHEINFO_H
-
-/* These are just hooks for sysfs.c to use. */
-extern void cacheinfo_cpu_online(unsigned int cpu_id);
-extern void cacheinfo_cpu_offline(unsigned int cpu_id);
-
-#endif /* _PPC_CACHEINFO_H */
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 97e1dc9..2bc61d3 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -19,8 +19,6 @@
#include <asm/pmc.h>
#include <asm/firmware.h>
-#include "cacheinfo.h"
-
#ifdef CONFIG_PPC64
#include <asm/paca.h>
#include <asm/lppaca.h>
@@ -730,7 +728,6 @@ static void register_cpu_online(unsigned int cpu)
device_create_file(s, &dev_attr_altivec_idle_wait_time);
}
#endif
- cacheinfo_cpu_online(cpu);
}
#ifdef CONFIG_HOTPLUG_CPU
@@ -811,7 +808,6 @@ static void unregister_cpu_online(unsigned int cpu)
device_remove_file(s, &dev_attr_altivec_idle_wait_time);
}
#endif
- cacheinfo_cpu_offline(cpu);
}
#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH RFC v7 2/6] dma: mpc512x: add support for peripheral transfers
From: Alexander Popov @ 2014-02-19 14:45 UTC (permalink / raw)
To: Gerhard Sittig
Cc: Lars-Peter Clausen, Arnd Bergmann, Vinod Koul, dmaengine,
Dan Williams, Anatolij Gustschin, linuxppc-dev
In-Reply-To: <20140213000743.GI4524@book.gsilab.sittig.org>
[ adding dmaengine ML to Cc: ]
Thanks for your feedback, Gerhard
2014-02-13 4:07 GMT+04:00 Gerhard Sittig <gsi@denx.de>:
> On Wed, Feb 12, 2014 at 17:25 +0400, Alexander Popov wrote:
>> /*
>> - * This is initial version of MPC5121 DMA driver. Only memory to memory
>> - * transfers are supported (tested using dmatest module).
>> + * MPC512x and MPC8308 DMA driver. It supports
>> + * memory to memory data transfers (tested using dmatest module) and
>> + * data transfers between memory and peripheral I/O memory
>> + * by means of slave s/g with these limitations:
>> + * - chunked transfers (transfers with more than one part) are refused
>> + * as long as proper support for scatter/gather is missing;
>> + * - transfers on MPC8308 always start from software as this SoC appears
>> + * not to have external request lines for peripheral flow control;
>> + * - minimal memory <-> I/O memory transfer size is 4 bytes.
>> */
>
> Often I assume people would notice themselves, and apparently I'm
> wrong. :) Can you adjust the formatting such (here and
> elsewhere) that the bullet list is clearly visible as such?
> Flowing text like above obfuscates the fact that the content may
> have a structure ...
Ok, thanks :)
> There are known limitations which are not listed here, "minimal
> transfer size" is incomplete. It appears that you assume
> constraints on start addresses as well as sizes/lengths. Can you
> update the documentation to match the implementation?
Ok, I see. How about that?
* - minimal memory <-> I/O memory transfer chunk is 4 bytes and consequently
* source and destination addresses must be 4-byte aligned
* and transfer size must be aligned on (4 * maxburst) boundary;
>> + /* Grab either several mem-to-mem transfer descriptors
>> + * or one peripheral transfer descriptor,
>> + * don't mix mem-to-mem and peripheral transfer descriptors
>> + * within the same 'active' list. */
>> + if (mdesc->will_access_peripheral) {
>> + if (list_empty(&mchan->active))
>> + list_move_tail(&mdesc->node, &mchan->active);
>> + break;
>> + } else
>> + list_move_tail(&mdesc->node, &mchan->active);
>> + }
> There are style issues. Both in multi line comments, and in the
> braces of the if/else block.
Ah, thanks! I'll fix that.
>> + struct mpc_dma *mdma = dma_chan_to_mpc_dma(chan);
>> + struct mpc_dma_chan *mchan = dma_chan_to_mpc_dma_chan(chan);
>> + struct mpc_dma_desc *mdesc = NULL;
>> + dma_addr_t per_paddr;
>> + u32 tcd_nunits;
>> + struct mpc_dma_tcd *tcd;
>> + unsigned long iflags;
>> + struct scatterlist *sg;
>> + size_t len;
>> + int iter, i;
> Personally I much dislike this style of mixing declarations and
> instructions. But others may disagree, and strongly so.
Excuse me, I would like to keep it similar to other parts of this driver
and not to change that style.
>> + mdesc = list_first_entry(&mchan->free, struct mpc_dma_desc,
>> + node);
> style (continuation and indentation)
Thanks! I'll fix that.
>
>> + if (!mdesc) {
>> + spin_unlock_irqrestore(&mchan->lock, iflags);
>> + /* try to free completed descriptors */
>> + mpc_dma_process_completed(mdma);
>> + return NULL;
>> + }
>> +
>> + list_del(&mdesc->node);
>> +
>> + per_paddr = mchan->per_paddr;
>> + tcd_nunits = mchan->tcd_nunits;
>> +
>> + spin_unlock_irqrestore(&mchan->lock, iflags);
>> +
>> + if (per_paddr == 0 || tcd_nunits == 0)
>> + goto err_prep;
>> +
>> + mdesc->error = 0;
>> + mdesc->will_access_peripheral = 1;
>> + tcd = mdesc->tcd;
>> +
>> + /* Prepare Transfer Control Descriptor for this transaction */
>> +
>> + memset(tcd, 0, sizeof(struct mpc_dma_tcd));
>> +
>> + if (!IS_ALIGNED(sg_dma_address(sg), 4))
>> + goto err_prep;
>
> You found multiple ways of encoding the "4 byte alignment", using
> both the fixed number as well as (several) symbolic identifiers.
> Can you look into making them use the same condition if the same
> motivation is behind the test?
Gerhard, I don't see checks which can be kind of "merged"
since they have different motivation behind:
1) src_addr_width and dst_addr_width have type "enum dma_slave_buswidth"
and are compared with DMA_SLAVE_BUSWIDTH_4_BYTES which has
the same type;
2) source and destination addresses are checked to be 4-byte aligned;
3) transfer size is checked to be aligned on nbytes boundary
which is (4 * maxburst).
I think the code provides good readability. What do you think?
Thank you!
Best regards,
Alexander
^ 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