* Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
From: Scott Wood @ 2013-10-11 19:07 UTC (permalink / raw)
To: Mark Rutland
Cc: Tang Yuantian-B29983, devicetree@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, Li Yang-Leo-R58472
In-Reply-To: <20131011092526.GE3910@e106331-lin.cambridge.arm.com>
On Fri, 2013-10-11 at 10:25 +0100, Mark Rutland wrote:
> On Fri, Oct 11, 2013 at 04:18:18AM +0100, Tang Yuantian-B29983 wrote:
> > Thanks for your review.
> > See my reply inline
> >
> > > -----Original Message-----
> > > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > > Sent: 2013=E5=B9=B410=E6=9C=8810=E6=97=A5 =E6=98=9F=E6=9C=9F=E5=9B=9B=
18:04
> > > To: Tang Yuantian-B29983
> > > Cc: galak@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org;
> > > devicetree@vger.kernel.org; Li Yang-Leo-R58472
> > > Subject: Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in =
device
> > > tree
> > >
> > > On Wed, Oct 09, 2013 at 07:38:24AM +0100, Yuantian.Tang@freescale.c=
om
> > > wrote:
> > > > From: Tang Yuantian <yuantian.tang@freescale.com>
> > > >
> > > > The following SoCs will be affected: p2041, p3041, p4080, p5020,
> > > > p5040, b4420, b4860, t4240
> > > >
> > > > Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
> > > > Signed-off-by: Li Yang <leoli@freescale.com>
> > > > ---
> > > > v5:
> > > > - refine the binding document
> > > > - update the compatible string
> > > > v4:
> > > > - add binding document
> > > > - update compatible string
> > > > - update the reg property
> > > > v3:
> > > > - fix typo
> > > > v2:
> > > > - add t4240, b4420, b4860 support
> > > > - remove pll/4 clock from p2041, p3041 and p5020 board
> > > >
> > > > .../devicetree/bindings/clock/corenet-clock.txt | 111
> > > ++++++++++++++++++++
> > > > arch/powerpc/boot/dts/fsl/b4420si-post.dtsi | 35 +++++++
> > > > arch/powerpc/boot/dts/fsl/b4420si-pre.dtsi | 2 +
> > > > arch/powerpc/boot/dts/fsl/b4860si-post.dtsi | 35 +++++++
> > > > arch/powerpc/boot/dts/fsl/b4860si-pre.dtsi | 4 +
> > > > arch/powerpc/boot/dts/fsl/p2041si-post.dtsi | 60 +++++++=
++++
> > > > arch/powerpc/boot/dts/fsl/p2041si-pre.dtsi | 4 +
> > > > arch/powerpc/boot/dts/fsl/p3041si-post.dtsi | 60 +++++++=
++++
> > > > arch/powerpc/boot/dts/fsl/p3041si-pre.dtsi | 4 +
> > > > arch/powerpc/boot/dts/fsl/p4080si-post.dtsi | 112
> > > +++++++++++++++++++++
> > > > arch/powerpc/boot/dts/fsl/p4080si-pre.dtsi | 8 ++
> > > > arch/powerpc/boot/dts/fsl/p5020si-post.dtsi | 42 +++++++=
+
> > > > arch/powerpc/boot/dts/fsl/p5020si-pre.dtsi | 2 +
> > > > arch/powerpc/boot/dts/fsl/p5040si-post.dtsi | 60 +++++++=
++++
> > > > arch/powerpc/boot/dts/fsl/p5040si-pre.dtsi | 4 +
> > > > arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 85
> > > ++++++++++++++++
> > > > arch/powerpc/boot/dts/fsl/t4240si-pre.dtsi | 12 +++
> > > > 17 files changed, 640 insertions(+)
> > > > create mode 100644
> > > > Documentation/devicetree/bindings/clock/corenet-clock.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/clock/corenet-cloc=
k.txt
> > > > b/Documentation/devicetree/bindings/clock/corenet-clock.txt
> > > > new file mode 100644
> > > > index 0000000..8efc62d
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/corenet-clock.txt
> > > > @@ -0,0 +1,111 @@
> > > > +* Clock Block on Freescale CoreNet Platforms
> > > > +
> > > > +Freescale CoreNet chips take primary clocking input from the ext=
ernal
> > > > +SYSCLK signal. The SYSCLK input (frequency) is multiplied using
> > > > +multiple phase locked loops (PLL) to create a variety of frequen=
cies
> > > > +which can then be passed to a variety of internal logic, includi=
ng
> > > > +cores and peripheral IP blocks.
> > > > +Please refer to the Reference Manual for details.
> > > > +
> > > > +1. Clock Block Binding
> > > > +
> > > > +Required properties:
> > > > +- compatible: Should include one or more of the following:
> > > > + - "fsl,<chip>-clockgen": for chip specific clock block
> > > > + - "fsl,qoriq-clockgen-[1,2].x": for chassis 1.x and 2.x c=
lock
> > >
> > > While I can see that "fsl,<chip>-clockgen" might have a large set o=
f
> > > strings that we may never deal with in th kernel, I'd prefer that t=
he
> > > basic strings (i.e. all the "fsl,qoriq-clockgen-[1,2].x" variants) =
were
> > > listed explicitly here.
> > >
> > > Given they only seem to be "fsl,qoriq-clockgen-1.0" and "fsl,qoriq-
> > > clockgen-2.0" this shouldn't be too difficult to list and describe.
> > >
> > OK, I will list them all.
>=20
> Thanks.
>=20
> >
> > > > +- reg: Offset and length of the clock register set
> > > > +- clock-frequency: Indicates input clock frequency of clock bloc=
k.
> > > > + Will be set by u-boot
> > >
> > > Why does the fact this is set by u-boot matter to the binding?
> > >
> > OK, I will remove it.
> >
> > > > +
> > > > +Recommended properties:
> > > > +- #ddress-cells: Specifies the number of cells used to represent
> > > > + physical base addresses. Must be present if the device h=
as
> > > > + sub-nodes and set to 1 if present
> > >
> > > Typo: #address-cells
> > >
> > > In the example it looks like the address cells of child nodes are o=
ffsets
> > > within the unit, rather than absolute physical addresses. Could the=
code
> > > not treat them as absolute addresses? Then we'd only need to docume=
nt
> > > that #address-cells, #size-cells and ranges must be present and hav=
e
> > > values suitable for mapping child nodes into the address space of t=
he
> > > parent.
> > >
> > OK, thanks.
> >
> > > > +- #size-cells: Specifies the number of cells used to represent
> > > > + the size of an address. Must be present if the device has
> > > > + sub-nodes and set to 1 if present
> > >
> > > It's not really the size of an address, it's the size of a region
> > > identified by a reg entry.
> > >
> > > I think this can be simplified by my suggestion above.
> > >
> > Yes
>=20
> Aah, I see that this is already in use, so it's a bit late to change
> this...
>=20
> >
> > > > +
> > > > +2. Clock Provider/Consumer Binding
> > > > +
> > > > +Most of the binding are from the common clock binding[1].
> > > > + [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > +
> > > > +Required properties:
> > > > +- compatible : Should include one or more of the following:
>=20
> I didn't spot this earlier, but why "one or more"? are the 2.0 variants
> backwards compatible with the 1.0 variants.
>=20
> > > > + - "fsl,qoriq-core-pll-[1,2].x": Indicates a core PLL cloc=
k
> > > device
> > > > + - "fsl,qoriq-core-mux-[1,2].x": Indicates a core multiple=
xer
> > > clock
> > > > + device; divided from the core PLL clock
> > >
> > > As above, I'd prefer a complete list of the basic strings we expect=
.
> > >
> > There is no list here, just *-mux-1.x and *-mux-2.x
> > What kind of list do you prefer?
>=20
> Something like:
>=20
> - compatible: Should include at least one of:
> * "fsl,qoriq-core-pll-1.0" for core PLL clocks (v1.0)
> * "fsl,qoriq-core-pll-2.0" for core PLL clocks (v2.0)
> * "fsl,qoriq-core-mux-1.0" for core mux clocks (v1.0)
> * "fsl,qoriq-core-mux-2.0" for core mux clocks (v2.0)
> The *-2.0 variants are backwards compatible with the *-1.0 variants,
> so for compatiblity a *-1.0 variant string should be in the list.
I'm not sure that they're 100% compatible. 1.0 seems to have a "KILL"
bit in the PLL register that 2.0 doesn't have (unless it's a
documentation glitch).
> > > > +- clock-output-names: From common clock binding, indicates the n=
ames
> > > of
> > > > + output clocks
> > > > +- reg: Should be the offset and length of clock block base addre=
ss.
> > > > + The length should be 4.
> > > > +
> > > > +Example for clock block and clock provider:
> > > > +/ {
> > > > + clockgen: global-utilities@e1000 {
> > > > + compatible =3D "fsl,p5020-clockgen", "fsl,qoriq-c=
lockgen-
> > > 1.0";
> > > > + reg =3D <0xe1000 0x1000>;
> > > > + clock-frequency =3D <0>;
> > >
> > > That looks odd.
> > >
> > Yes, but it has already existed here before this patch.
> > Can I move it to sysclk clock node since it is not used yet?
>=20
> I hadn't realised there were DTS with this already. Why is there a cloc=
k
> with clock-frequency =3D <0> at all? Surely that isn't useful...
The actual frequency is patched in by U-Boot -- and moving it to a
different node would break this.
> > > > + #address-cells =3D <1>;
> > > > + #size-cells =3D <1>;
> > > > +
> > > > + sysclk: sysclk {
> > > > + #clock-cells =3D <0>;
> > > > + compatible =3D "fsl,qoriq-sysclk-1.0",
> > > > + "fixed-clock";
> > >
> > > We didn't mention in the binding that "fsl,qoriq-sysclk-1.0" was
> > > compatible with "fixed-clock" and should have "fixed-clock" in the
> > > compatible list...
> > >
> > OK, will fix it.
>=20
> Cheers. Also, doesn't a fixed-clock require a clock-frequency?
Why isn't the global-utilities node, that has the clock-frequency,
acting as the fixed-clock? I thought that's what was in earlier
revisions...
If it absolutely must be a separate node for some reason, I suppose you
could remove the "fixed-clock" and have a tiny "driver" that looks up
the frequency in the parent node. This would be an instance of a
non-"fixed-clock" that doesn't have a parent clock described in the
device tree, because the information comes from some other mechanism.
> > > > + mux1: mux1@20 {
> > > > + #clock-cells =3D <0>;
> > > > + reg =3D <0x20 0x4>;
> > > > + compatible =3D "fsl,qoriq-core-mux-1.0";
> > > > + clocks =3D <&pll0 0>, <&pll0 1>, <&pll1 0=
>,
> > > <&pll1 1>;
> > > > + clock-names =3D "pll0_0", "pll0_1", "pll1=
_0",
> > > "pll1_1";
>=20
> I didn't spot this last time, but the clock-names here seem to be the
> names of the outputs from the provider, rather than the input names of
> the consumer. This goes against the intended purpose of clock-names.
As far as "pll0", "pll1", etc. goes, that appears to be the input name.
It's all on one chip, so the virtual pins are documented based on what
they're connected to.
I'm not sure I understand the "_0"/"_1" part, though. Doesn't each PLL
just have one output, which the consumer may choose to divide by 2 (or
in some cases 4)? Why does that division need to be exposed in the
device tree as separate connections to the parent clock?
-Scott
^ permalink raw reply
* Re: Gianfar driver crashes in Kernel v3.10
From: Scott Wood @ 2013-10-11 17:49 UTC (permalink / raw)
To: Thomas Hühn
Cc: linuxppc-dev@lists.ozlabs.org, claudiu.manoil@freescale.com
In-Reply-To: <1381513748.7979.503.camel@snotra.buserror.net>
On Fri, 2013-10-11 at 12:49 -0500, Scott Wood wrote:
> On Fri, 2013-10-11 at 11:17 +0200, Thomas H=C3=BChn wrote:
> > Hi Scott,
> >=20
> > On 09.10.2013, at 00:09, Scott Wood <scottwood@freescale.com> wrote:
> >=20
> > > On Fri, 2013-10-04 at 12:03 +0000, Thomas H=C3=BChn wrote:
> > >> [code]
> > >> [ 2671.841927] Oops: Exception in kernel mode, sig: 5 [#1]
> > >> [ 2671.847141] Freescale P1014
> > >> [ 2671.849925] Modules linked in: ath9k pppoe ppp_async iptable_na=
t ath9k_common pppox p
> > >> e xt_tcpudp xt_tcpmss xt_string xt_statistic xt_state xt_recent xt=
_quota xt_pkttype xt_o
> > >> mark xt_connbytes xt_comment xt_addrtype xt_TCPMSS xt_REDIRECT xt_=
NETMAP xt_LOG xt_IPMAR
> > >> ms_datafab ums_cypress ums_alauda slhc nf_nat_tftp nf_nat_snmp_bas=
ic nf_nat_sip nf_nat_r
> > >> ntrack_sip nf_conntrack_rtsp nf_conntrack_proto_gre nf_conntrack_i=
rc nf_conntrack_h323 n
> > >> compat_xtables compat ath sch_teql sch_tbf sch_sfq sch_red sch_pri=
o sch_htb sch_gred sc
> > >> skbedit act_mirred em_u32 cls_u32 cls_tcindex cls_flow cls_route c=
ls_fw sch_hfsc sch_ing
> > >> r usb_storage leds_gpio ohci_hcd ehci_platform ehci_hcd sd_mod scs=
i_mod fsl_mph_dr_of gp
> > >> [ 2671.988946] CPU: 0 PID: 5209 Comm: iftop Not tainted 3.10.13 #2
> > >> [ 2671.994859] task: c4b22220 ti: c7ff8000 task.ti: c477e000
> > >> [ 2672.000250] NIP: c018c7a0 LR: c018c794 CTR: c000b070
> > >> [ 2672.005206] REGS: c7ff9f10 TRAP: 3202 Not tainted (3.10.13)
> > >=20
> > > Trap 0x3202 is a watchdog timer.
> > >=20
> > > Did you get a "Bad trap at=E2=80=A6" line before the above dump? =20
> >=20
> > I need to setup my test scenario again as I just copied the crash lin=
es out of it without saving the full file with all lines.
> >=20
> > > Do you have
> > > any idea why the watchdog would have been armed without CONFIG_BOOK=
E_WDT
> > > being set? =20
> >=20
> > > Is CONFIG_BOOKE_WDT set?
> > >=20
> > This config option is not set. Should I try with the CONFIG_BOOK_WDT =
enabled ?
>=20
> Instead, could you try to track down where TCR[WE] is getting set, and
> dump TCR in unknown_exception()?
Sorry, that should be TCR[WIE].
-Scott
^ permalink raw reply
* Re: Gianfar driver crashes in Kernel v3.10
From: Scott Wood @ 2013-10-11 17:49 UTC (permalink / raw)
To: Thomas Hühn
Cc: linuxppc-dev@lists.ozlabs.org, claudiu.manoil@freescale.com
In-Reply-To: <B1062BEC-9A3C-49DC-B97A-9E0A6EE9CD49@net.t-labs.tu-berlin.de>
On Fri, 2013-10-11 at 11:17 +0200, Thomas H=C3=BChn wrote:
> Hi Scott,
>=20
> On 09.10.2013, at 00:09, Scott Wood <scottwood@freescale.com> wrote:
>=20
> > On Fri, 2013-10-04 at 12:03 +0000, Thomas H=C3=BChn wrote:
> >> [code]
> >> [ 2671.841927] Oops: Exception in kernel mode, sig: 5 [#1]
> >> [ 2671.847141] Freescale P1014
> >> [ 2671.849925] Modules linked in: ath9k pppoe ppp_async iptable_nat =
ath9k_common pppox p
> >> e xt_tcpudp xt_tcpmss xt_string xt_statistic xt_state xt_recent xt_q=
uota xt_pkttype xt_o
> >> mark xt_connbytes xt_comment xt_addrtype xt_TCPMSS xt_REDIRECT xt_NE=
TMAP xt_LOG xt_IPMAR
> >> ms_datafab ums_cypress ums_alauda slhc nf_nat_tftp nf_nat_snmp_basic=
nf_nat_sip nf_nat_r
> >> ntrack_sip nf_conntrack_rtsp nf_conntrack_proto_gre nf_conntrack_irc=
nf_conntrack_h323 n
> >> compat_xtables compat ath sch_teql sch_tbf sch_sfq sch_red sch_prio =
sch_htb sch_gred sc
> >> skbedit act_mirred em_u32 cls_u32 cls_tcindex cls_flow cls_route cls=
_fw sch_hfsc sch_ing
> >> r usb_storage leds_gpio ohci_hcd ehci_platform ehci_hcd sd_mod scsi_=
mod fsl_mph_dr_of gp
> >> [ 2671.988946] CPU: 0 PID: 5209 Comm: iftop Not tainted 3.10.13 #2
> >> [ 2671.994859] task: c4b22220 ti: c7ff8000 task.ti: c477e000
> >> [ 2672.000250] NIP: c018c7a0 LR: c018c794 CTR: c000b070
> >> [ 2672.005206] REGS: c7ff9f10 TRAP: 3202 Not tainted (3.10.13)
> >=20
> > Trap 0x3202 is a watchdog timer.
> >=20
> > Did you get a "Bad trap at=E2=80=A6" line before the above dump? =20
>=20
> I need to setup my test scenario again as I just copied the crash lines=
out of it without saving the full file with all lines.
>=20
> > Do you have
> > any idea why the watchdog would have been armed without CONFIG_BOOKE_=
WDT
> > being set? =20
>=20
> > Is CONFIG_BOOKE_WDT set?
> >=20
> This config option is not set. Should I try with the CONFIG_BOOK_WDT en=
abled ?
Instead, could you try to track down where TCR[WE] is getting set, and
dump TCR in unknown_exception()?
-Scott
^ permalink raw reply
* [PATCH v2] powerpc/mpc512x: remove unnecessary #if
From: Brian Norris @ 2013-10-11 17:37 UTC (permalink / raw)
To: Anatolij Gustschin; +Cc: Gerhard Sittig, Brian Norris, linuxppc-dev
In-Reply-To: <1381512777-13966-1-git-send-email-computersforpeace@gmail.com>
Several functions are only ever referenced locally, so make them static.
Of those functions, many of them are protected by an #if. However, the
code which can compile fine in either case.
Now that (1) the unneeded code is marked 'static' and (2) the code is
only used under a C 'if (IS_ENABLED(CONFIG_FB_FSL_DIU))', the compiler
can automatically remove the unneeded code, and we don't need the #if or
the empty stub functions.
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v2: left out a diff I was holding locally (to remove an #if/#endif
completely). Sorry for the noise.
Based off of Gerhard Sittig's patch:
powerpc/mpc512x: silence build warning upon disabled DIU
Compile-tested with CONFIG_FB_FSL_DIU=n
arch/powerpc/platforms/512x/mpc512x_shared.c | 21 +++++++--------------
arch/powerpc/sysdev/fsl_soc.h | 3 ---
2 files changed, 7 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c
index 1a7b1d0..36b5652 100644
--- a/arch/powerpc/platforms/512x/mpc512x_shared.c
+++ b/arch/powerpc/platforms/512x/mpc512x_shared.c
@@ -60,8 +60,6 @@ void mpc512x_restart(char *cmd)
;
}
-#if IS_ENABLED(CONFIG_FB_FSL_DIU)
-
struct fsl_diu_shared_fb {
u8 gamma[0x300]; /* 32-bit aligned! */
struct diu_ad ad0; /* 32-bit aligned! */
@@ -71,7 +69,7 @@ struct fsl_diu_shared_fb {
};
#define DIU_DIV_MASK 0x000000ff
-void mpc512x_set_pixel_clock(unsigned int pixclock)
+static void mpc512x_set_pixel_clock(unsigned int pixclock)
{
unsigned long bestval, bestfreq, speed, busfreq;
unsigned long minpixclock, maxpixclock, pixval;
@@ -164,7 +162,7 @@ void mpc512x_set_pixel_clock(unsigned int pixclock)
iounmap(ccm);
}
-enum fsl_diu_monitor_port
+static enum fsl_diu_monitor_port
mpc512x_valid_monitor_port(enum fsl_diu_monitor_port port)
{
return FSL_DIU_PORT_DVI;
@@ -179,7 +177,7 @@ static inline void mpc512x_free_bootmem(struct page *page)
free_reserved_page(page);
}
-void mpc512x_release_bootmem(void)
+static void mpc512x_release_bootmem(void)
{
unsigned long addr = diu_shared_fb.fb_phys & PAGE_MASK;
unsigned long size = diu_shared_fb.fb_len;
@@ -205,7 +203,7 @@ void mpc512x_release_bootmem(void)
* address range will be reserved in setup_arch() after bootmem
* allocator is up.
*/
-void __init mpc512x_init_diu(void)
+static void __init mpc512x_init_diu(void)
{
struct device_node *np;
struct diu __iomem *diu_reg;
@@ -274,7 +272,7 @@ out:
iounmap(diu_reg);
}
-void __init mpc512x_setup_diu(void)
+static void __init mpc512x_setup_diu(void)
{
int ret;
@@ -303,11 +301,6 @@ void __init mpc512x_setup_diu(void)
diu_ops.release_bootmem = mpc512x_release_bootmem;
}
-#else
-void __init mpc512x_setup_diu(void) { /* EMPTY */ }
-void __init mpc512x_init_diu(void) { /* EMPTY */ }
-#endif
-
void __init mpc512x_init_IRQ(void)
{
struct device_node *np;
@@ -340,7 +333,7 @@ static struct of_device_id __initdata of_bus_ids[] = {
{},
};
-void __init mpc512x_declare_of_platform_devices(void)
+static void __init mpc512x_declare_of_platform_devices(void)
{
if (of_platform_bus_probe(NULL, of_bus_ids, NULL))
printk(KERN_ERR __FILE__ ": "
@@ -390,7 +383,7 @@ static unsigned int __init get_fifo_size(struct device_node *np,
((u32)(_base) + sizeof(struct mpc52xx_psc)))
/* Init PSC FIFO space for TX and RX slices */
-void __init mpc512x_psc_fifo_init(void)
+static void __init mpc512x_psc_fifo_init(void)
{
struct device_node *np;
void __iomem *psc;
diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
index c6d0073..4c5a19e 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h
@@ -21,8 +21,6 @@ struct device_node;
extern void fsl_rstcr_restart(char *cmd);
-#if defined(CONFIG_FB_FSL_DIU) || defined(CONFIG_FB_FSL_DIU_MODULE)
-
/* The different ports that the DIU can be connected to */
enum fsl_diu_monitor_port {
FSL_DIU_PORT_DVI, /* DVI */
@@ -43,7 +41,6 @@ struct platform_diu_data_ops {
};
extern struct platform_diu_data_ops diu_ops;
-#endif
void fsl_hv_restart(char *cmd);
void fsl_hv_halt(void);
--
1.8.4
^ permalink raw reply related
* Re: [PATCH] powerpc/mpc512x: remove unnecessary #if
From: Brian Norris @ 2013-10-11 17:37 UTC (permalink / raw)
To: Anatolij Gustschin; +Cc: Gerhard Sittig, Brian Norris, linuxppc-dev
In-Reply-To: <1381512777-13966-1-git-send-email-computersforpeace@gmail.com>
Hi,
I'm sorry, ignore this version. I left out a diff I meant to include.
On Fri, Oct 11, 2013 at 10:32 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Several functions are only ever referenced locally, so make them static.
> Of those functions, many of them are protected by an #if. However, the
> code which can compile fine in either case.
>
> Now that (1) the unneeded code is marked 'static' and (2) the code is
> only used under a C 'if (IS_ENABLED(CONFIG_FB_FSL_DIU))', the compiler
> can automatically remove the unneeded code, and we don't need the #if or
> the empty stub functions.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> Based off of Gerhard Sittig's patch:
> powerpc/mpc512x: silence build warning upon disabled DIU
>
> Compile-tested with CONFIG_FB_FSL_DIU=n
...
> @@ -42,6 +40,7 @@ struct platform_diu_data_ops {
> void (*release_bootmem)(void);
> };
>
> +#if defined(CONFIG_FB_FSL_DIU) || defined(CONFIG_FB_FSL_DIU_MODULE)
> extern struct platform_diu_data_ops diu_ops;
> #endif
The above hunk is incorrect. I will send v2.
Brian
^ permalink raw reply
* Re: Elbc device driver
From: Scott Wood @ 2013-10-11 17:35 UTC (permalink / raw)
To: Mercier Ivan; +Cc: linuxppc-dev
In-Reply-To: <CAMc2ieqO5eKiXdGh0qraJy3FKPMbzkpTGgWrSwwGHh7sn2iCFw@mail.gmail.com>
On Fri, 2013-10-11 at 17:03 +0200, Mercier Ivan wrote:
> Hi,
> this should be correct (I'm using chip select 3 for this device)
> lbc: localbus@ffe124000 {
> reg = <0xf 0xfe124000 0 0x1000>;
> ranges = <3 0 0xf 0xe0000000 0x08000000>;
>
> a3p400{
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "my_a3p_driver";
> reg = <0x0 0x0 0x800000>;
> };
> };
Compatible describes the device, not the driver. It takes the format
"vendor,device". The node name, OTOH, is normally a generic description
of the device's functionality ("flash", "ethernet", "board-control",
etc).
You don't need #address-cells/#size-cells on the a3p400 node unless it
has child nodes with reg or ranges.
-Scott
^ permalink raw reply
* [PATCH] powerpc/mpc512x: remove unnecessary #if
From: Brian Norris @ 2013-10-11 17:32 UTC (permalink / raw)
To: Anatolij Gustschin; +Cc: Gerhard Sittig, Brian Norris, linuxppc-dev
Several functions are only ever referenced locally, so make them static.
Of those functions, many of them are protected by an #if. However, the
code which can compile fine in either case.
Now that (1) the unneeded code is marked 'static' and (2) the code is
only used under a C 'if (IS_ENABLED(CONFIG_FB_FSL_DIU))', the compiler
can automatically remove the unneeded code, and we don't need the #if or
the empty stub functions.
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Based off of Gerhard Sittig's patch:
powerpc/mpc512x: silence build warning upon disabled DIU
Compile-tested with CONFIG_FB_FSL_DIU=n
arch/powerpc/platforms/512x/mpc512x_shared.c | 19 ++++++-------------
arch/powerpc/sysdev/fsl_soc.h | 3 +--
2 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c
index 1a7b1d0..48e0718 100644
--- a/arch/powerpc/platforms/512x/mpc512x_shared.c
+++ b/arch/powerpc/platforms/512x/mpc512x_shared.c
@@ -60,8 +60,6 @@ void mpc512x_restart(char *cmd)
;
}
-#if IS_ENABLED(CONFIG_FB_FSL_DIU)
-
struct fsl_diu_shared_fb {
u8 gamma[0x300]; /* 32-bit aligned! */
struct diu_ad ad0; /* 32-bit aligned! */
@@ -71,7 +69,7 @@ struct fsl_diu_shared_fb {
};
#define DIU_DIV_MASK 0x000000ff
-void mpc512x_set_pixel_clock(unsigned int pixclock)
+static void mpc512x_set_pixel_clock(unsigned int pixclock)
{
unsigned long bestval, bestfreq, speed, busfreq;
unsigned long minpixclock, maxpixclock, pixval;
@@ -164,7 +162,7 @@ void mpc512x_set_pixel_clock(unsigned int pixclock)
iounmap(ccm);
}
-enum fsl_diu_monitor_port
+static enum fsl_diu_monitor_port
mpc512x_valid_monitor_port(enum fsl_diu_monitor_port port)
{
return FSL_DIU_PORT_DVI;
@@ -179,7 +177,7 @@ static inline void mpc512x_free_bootmem(struct page *page)
free_reserved_page(page);
}
-void mpc512x_release_bootmem(void)
+static void mpc512x_release_bootmem(void)
{
unsigned long addr = diu_shared_fb.fb_phys & PAGE_MASK;
unsigned long size = diu_shared_fb.fb_len;
@@ -205,7 +203,7 @@ void mpc512x_release_bootmem(void)
* address range will be reserved in setup_arch() after bootmem
* allocator is up.
*/
-void __init mpc512x_init_diu(void)
+static void __init mpc512x_init_diu(void)
{
struct device_node *np;
struct diu __iomem *diu_reg;
@@ -274,7 +272,7 @@ out:
iounmap(diu_reg);
}
-void __init mpc512x_setup_diu(void)
+static void __init mpc512x_setup_diu(void)
{
int ret;
@@ -303,11 +301,6 @@ void __init mpc512x_setup_diu(void)
diu_ops.release_bootmem = mpc512x_release_bootmem;
}
-#else
-void __init mpc512x_setup_diu(void) { /* EMPTY */ }
-void __init mpc512x_init_diu(void) { /* EMPTY */ }
-#endif
-
void __init mpc512x_init_IRQ(void)
{
struct device_node *np;
@@ -340,7 +333,7 @@ static struct of_device_id __initdata of_bus_ids[] = {
{},
};
-void __init mpc512x_declare_of_platform_devices(void)
+static void __init mpc512x_declare_of_platform_devices(void)
{
if (of_platform_bus_probe(NULL, of_bus_ids, NULL))
printk(KERN_ERR __FILE__ ": "
diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
index c6d0073..f845ffd 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h
@@ -21,8 +21,6 @@ struct device_node;
extern void fsl_rstcr_restart(char *cmd);
-#if defined(CONFIG_FB_FSL_DIU) || defined(CONFIG_FB_FSL_DIU_MODULE)
-
/* The different ports that the DIU can be connected to */
enum fsl_diu_monitor_port {
FSL_DIU_PORT_DVI, /* DVI */
@@ -42,6 +40,7 @@ struct platform_diu_data_ops {
void (*release_bootmem)(void);
};
+#if defined(CONFIG_FB_FSL_DIU) || defined(CONFIG_FB_FSL_DIU_MODULE)
extern struct platform_diu_data_ops diu_ops;
#endif
--
1.8.4
^ permalink raw reply related
* Re: [v1] powerpc/mpc512x: silence build warning upon disabled DIU
From: Brian Norris @ 2013-10-11 17:33 UTC (permalink / raw)
To: Anatolij Gustschin; +Cc: Gerhard Sittig, linuxppc-dev
In-Reply-To: <20131010213747.4e56ca1f@crub>
Hi,
On Thu, Oct 10, 2013 at 12:37 PM, Anatolij Gustschin <agust@denx.de> wrote:
> On Thu, 10 Oct 2013 11:23:55 -0700
> Brian Norris <computersforpeace@gmail.com> wrote:
>> > Yes, we also have to remove CONFIG_FB_FSL_DIU ifdef in
>> > arch/powerpc/sysdev/fsl_soc.h and building should work then.
>>
>> Still want it around this line, probably, so we'll get compiler errors
>> and not linker errors if someone tries to use it?
>>
>> extern struct platform_diu_data_ops diu_ops;
I was wrong about this: we need to remove the #ifdef for this extern
as well, because the 'diu_ops' symbol is used in either case with my
patch. Sending out my patch now.
Brian
^ permalink raw reply
* Re: [PATCH] powerpc 8xx: Fixing memory init issue with CONFIG_PIN_TLB
From: Joakim Tjernlund @ 2013-10-11 15:13 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, linux-kernel, Paul Mackerras
In-Reply-To: <201310111256.r9BCuepM006243@localhost.localdomain>
"Linuxppc-dev"
<linuxppc-dev-bounces+joakim.tjernlund=transmode.se@lists.ozlabs.org>
wrote on 2013/10/11 14:56:40:
>
> Activating CONFIG_PIN_TLB allows access to the 24 first Mbytes of memory
at
> bootup instead of 8. It is needed for "big" kernels for instance when
activating
> CONFIG_LOCKDEP_SUPPORT. This needs to be taken into account in init_32
too,
> otherwise memory allocation soon fails after startup.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>
> diff -ur linux-3.11.org/arch/powerpc/kernel/head_8xx.S
linux-3.11/arch/powerpc/kernel/head_8xx.S
> --- linux-3.11.org/arch/powerpc/mm/init_32.c 2013-09-02
22:46:10.000000000 +0200
> +++ linux-3.11/arch/powerpc/mm/init_32.c 2013-09-09 11:28:54.000000000
+0200
> @@ -213,7 +213,12 @@
> */
> BUG_ON(first_memblock_base != 0);
>
> +#ifdef CONFIG_PIN_TLB
> + /* 8xx can only access 24MB at the moment */
> + memblock_set_current_limit(min_t(u64, first_memblock_size,
0x01800000));
> +#else
> /* 8xx can only access 8MB at the moment */
> memblock_set_current_limit(min_t(u64, first_memblock_size,
0x00800000));
> +#endif
> }
> #endif /* CONFIG_8xx */
hmm, I think you should always map 24 MB (or less if RAM < 24 MB) and do
the same
in head_8xx.S.
Or to keep it simple, just always map at least 16 MB here and in
head_8xx.S, assuming
that 16 MB is min RAM for any 8xx system running 3.x kernels.
Much of the need for pinning would go away if you adopted my 8MB pages
from 2.4 to 3.x
Jocke
^ permalink raw reply
* Re: Elbc device driver
From: Mercier Ivan @ 2013-10-11 15:03 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <1381418079.7979.396.camel@snotra.buserror.net>
Hi,
this should be correct (I'm using chip select 3 for this device)
lbc: localbus@ffe124000 {
reg = <0xf 0xfe124000 0 0x1000>;
ranges = <3 0 0xf 0xe0000000 0x08000000>;
a3p400{
#address-cells = <1>;
#size-cells = <1>;
compatible = "my_a3p_driver";
reg = <0x0 0x0 0x800000>;
};
};
yeah, I'm including p3041-post.dtsi.
so it I get it, I should code my driver doing basically:
{
static u8 __iomem * a3p
struct device_node *np;
np = of_find_compatible_node(NULL, NULL, "my_a3p_driver");
if (!np) {
printk(KERN_CRIT "Could not find my_a3p_driver node\n");
return;
}
a3p_bcsr = of_iomap(np, 0);
of_node_put(np);
if (!a3p_bcsr) {
printk(KERN_CRIT "Could not remap BCSR\n");
return;
}
}
Thanks for your help Scott
2013/10/10 Scott Wood <scottwood@freescale.com>:
> On Thu, 2013-10-10 at 16:54 +0200, Mercier Ivan wrote:
>> Hi,
>> I've got a KERN_INFO msg: "fsl-lbc a3p400.11: failed to get memory
>> region" corresponding to my elbc device that I try to map.
>> a3p400@3,0 {
>> compatible = "fsl,elbc";
>> reg = <0 0xe0000000 0x1000000>;
>> };
>
> The node name and unit address do not match the contents of the node.
> The former describes a device that sits on the localbus, while the
> latter has a compatible describes the localbus itself. I don't know
> what that reg is, but it doesn't match the unit address.
>
>> The corresponding law on ELBC is 0xe0000000 ->0xf0000000.
>> I want to map my device on 0xe0000000 ->0xe8000000 as it has 16 adress bits.
>>
>> Now the controler registers are properly set,but how do I map the device?
>
> Your eLBC node should look something like this (I'm assuming CCSR is at
> 0xffe000000, and that you actually meant 0x0e0000000 and not
> 0xfe0000000, though probably one of those assumptions is wrong):
>
> localbus@ffe124000 {
> compatible = "fsl,elbc", "simple-bus";
> reg = <0xf 0xfe124000 0 0x1000>;
> interrupts = <25 2 0 0>;
> #address-cells = <2>;
> #size-cells = <1>;
>
> ranges = <3 0 0 0xe0000000 0x08000000>;
>
> /* If at all possible, replace "a3p400" in the node name with
> something generic */
> a3p400@3,0 {
> compatible = "something appropriate here";
> reg = <3 0 0x08000000>;
> };
> };
>
> Note that at the dts level, you should be including
> arch/powerpc/boot/dts/fsl/p3041-post.dtsi and thus your board dts would
> only need to supply reg, ranges, and the a3p400 node (see
> arch/powerpc/boot/dts/p3041ds.dts for an example).
>
> -Scott
>
>
>
^ permalink raw reply
* [PATCH] powerpc 8xx: Fixing memory init issue with CONFIG_PIN_TLB
From: Christophe Leroy @ 2013-10-11 12:56 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras; +Cc: linuxppc-dev, linux-kernel
Activating CONFIG_PIN_TLB allows access to the 24 first Mbytes of memory at
bootup instead of 8. It is needed for "big" kernels for instance when activating
CONFIG_LOCKDEP_SUPPORT. This needs to be taken into account in init_32 too,
otherwise memory allocation soon fails after startup.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
diff -ur linux-3.11.org/arch/powerpc/kernel/head_8xx.S linux-3.11/arch/powerpc/kernel/head_8xx.S
--- linux-3.11.org/arch/powerpc/mm/init_32.c 2013-09-02 22:46:10.000000000 +0200
+++ linux-3.11/arch/powerpc/mm/init_32.c 2013-09-09 11:28:54.000000000 +0200
@@ -213,7 +213,12 @@
*/
BUG_ON(first_memblock_base != 0);
+#ifdef CONFIG_PIN_TLB
+ /* 8xx can only access 24MB at the moment */
+ memblock_set_current_limit(min_t(u64, first_memblock_size, 0x01800000));
+#else
/* 8xx can only access 8MB at the moment */
memblock_set_current_limit(min_t(u64, first_memblock_size, 0x00800000));
+#endif
}
#endif /* CONFIG_8xx */
^ permalink raw reply
* Re: [PATCH 1/2] tty/hvc_console: Add DTR/RTS callback to handle HUPCL control
From: Hendrik Brueckner @ 2013-10-11 12:47 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linux-s390, brueckner, gregkh, heiko.carstens, linuxppc-dev,
linux-kernel, Hendrik Brueckner, schwidefsky, jslaby
In-Reply-To: <1381475711.5630.67.camel@pasglop>
Hi Benjamin,
On Fri, Oct 11, 2013 at 06:15:11PM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2013-07-02 at 17:07 +0200, Hendrik Brueckner wrote:
> > Introduce a new callback to explicitly handle the HUPCL termios control flag.
> > This prepares for a follow-up commit for the hvc_iucv device driver to
> > improve handling when to drop an established network connection.
> >
> > The callback naming is based on the recently added tty_port interface to
> > facilitate a potential refactoring of the hvc_console to use tty_port
> > functions.
>
> I only just noticed that ... oops. Why add those dtr_rts() calls ? We
> already have tiocmset in there which is used to set DTR on HVSI consoles
> such as hvc_opal when using hvsi_lib...
>
> Any reason why a separate callback was needed ?
The tiocmget/tiocmset callbacks are used to set and get modem status and
triggered through an tty ioctl.
The dtr_rts() callback is different and it is used for DTS/RTS handshaking
between the hvc_console (or any other tty_port) and the tty layer. The tty
port layer uses this callback to signal the hvc_console whether to raise or
lower the DTR/RTS lines. This is different than the ioctl interface to
controls the modem status.
Thanks and kind regards,
Hendrik
^ permalink raw reply
* Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device tree
From: Mark Rutland @ 2013-10-11 9:25 UTC (permalink / raw)
To: Tang Yuantian-B29983
Cc: devicetree@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
Li Yang-Leo-R58472
In-Reply-To: <D07C73A334FF604B95B3CBD2A545D07B150BD137@039-SN2MPN1-013.039d.mgd.msft.net>
On Fri, Oct 11, 2013 at 04:18:18AM +0100, Tang Yuantian-B29983 wrote:
> Thanks for your review.
> See my reply inline
>
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > Sent: 2013年10月10日 星期四 18:04
> > To: Tang Yuantian-B29983
> > Cc: galak@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org;
> > devicetree@vger.kernel.org; Li Yang-Leo-R58472
> > Subject: Re: [PATCH v5] powerpc/mpc85xx: Update the clock nodes in device
> > tree
> >
> > On Wed, Oct 09, 2013 at 07:38:24AM +0100, Yuantian.Tang@freescale.com
> > wrote:
> > > From: Tang Yuantian <yuantian.tang@freescale.com>
> > >
> > > The following SoCs will be affected: p2041, p3041, p4080, p5020,
> > > p5040, b4420, b4860, t4240
> > >
> > > Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
> > > Signed-off-by: Li Yang <leoli@freescale.com>
> > > ---
> > > v5:
> > > - refine the binding document
> > > - update the compatible string
> > > v4:
> > > - add binding document
> > > - update compatible string
> > > - update the reg property
> > > v3:
> > > - fix typo
> > > v2:
> > > - add t4240, b4420, b4860 support
> > > - remove pll/4 clock from p2041, p3041 and p5020 board
> > >
> > > .../devicetree/bindings/clock/corenet-clock.txt | 111
> > ++++++++++++++++++++
> > > arch/powerpc/boot/dts/fsl/b4420si-post.dtsi | 35 +++++++
> > > arch/powerpc/boot/dts/fsl/b4420si-pre.dtsi | 2 +
> > > arch/powerpc/boot/dts/fsl/b4860si-post.dtsi | 35 +++++++
> > > arch/powerpc/boot/dts/fsl/b4860si-pre.dtsi | 4 +
> > > arch/powerpc/boot/dts/fsl/p2041si-post.dtsi | 60 +++++++++++
> > > arch/powerpc/boot/dts/fsl/p2041si-pre.dtsi | 4 +
> > > arch/powerpc/boot/dts/fsl/p3041si-post.dtsi | 60 +++++++++++
> > > arch/powerpc/boot/dts/fsl/p3041si-pre.dtsi | 4 +
> > > arch/powerpc/boot/dts/fsl/p4080si-post.dtsi | 112
> > +++++++++++++++++++++
> > > arch/powerpc/boot/dts/fsl/p4080si-pre.dtsi | 8 ++
> > > arch/powerpc/boot/dts/fsl/p5020si-post.dtsi | 42 ++++++++
> > > arch/powerpc/boot/dts/fsl/p5020si-pre.dtsi | 2 +
> > > arch/powerpc/boot/dts/fsl/p5040si-post.dtsi | 60 +++++++++++
> > > arch/powerpc/boot/dts/fsl/p5040si-pre.dtsi | 4 +
> > > arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 85
> > ++++++++++++++++
> > > arch/powerpc/boot/dts/fsl/t4240si-pre.dtsi | 12 +++
> > > 17 files changed, 640 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/clock/corenet-clock.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/clock/corenet-clock.txt
> > > b/Documentation/devicetree/bindings/clock/corenet-clock.txt
> > > new file mode 100644
> > > index 0000000..8efc62d
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/corenet-clock.txt
> > > @@ -0,0 +1,111 @@
> > > +* Clock Block on Freescale CoreNet Platforms
> > > +
> > > +Freescale CoreNet chips take primary clocking input from the external
> > > +SYSCLK signal. The SYSCLK input (frequency) is multiplied using
> > > +multiple phase locked loops (PLL) to create a variety of frequencies
> > > +which can then be passed to a variety of internal logic, including
> > > +cores and peripheral IP blocks.
> > > +Please refer to the Reference Manual for details.
> > > +
> > > +1. Clock Block Binding
> > > +
> > > +Required properties:
> > > +- compatible: Should include one or more of the following:
> > > + - "fsl,<chip>-clockgen": for chip specific clock block
> > > + - "fsl,qoriq-clockgen-[1,2].x": for chassis 1.x and 2.x clock
> >
> > While I can see that "fsl,<chip>-clockgen" might have a large set of
> > strings that we may never deal with in th kernel, I'd prefer that the
> > basic strings (i.e. all the "fsl,qoriq-clockgen-[1,2].x" variants) were
> > listed explicitly here.
> >
> > Given they only seem to be "fsl,qoriq-clockgen-1.0" and "fsl,qoriq-
> > clockgen-2.0" this shouldn't be too difficult to list and describe.
> >
> OK, I will list them all.
Thanks.
>
> > > +- reg: Offset and length of the clock register set
> > > +- clock-frequency: Indicates input clock frequency of clock block.
> > > + Will be set by u-boot
> >
> > Why does the fact this is set by u-boot matter to the binding?
> >
> OK, I will remove it.
>
> > > +
> > > +Recommended properties:
> > > +- #ddress-cells: Specifies the number of cells used to represent
> > > + physical base addresses. Must be present if the device has
> > > + sub-nodes and set to 1 if present
> >
> > Typo: #address-cells
> >
> > In the example it looks like the address cells of child nodes are offsets
> > within the unit, rather than absolute physical addresses. Could the code
> > not treat them as absolute addresses? Then we'd only need to document
> > that #address-cells, #size-cells and ranges must be present and have
> > values suitable for mapping child nodes into the address space of the
> > parent.
> >
> OK, thanks.
>
> > > +- #size-cells: Specifies the number of cells used to represent
> > > + the size of an address. Must be present if the device has
> > > + sub-nodes and set to 1 if present
> >
> > It's not really the size of an address, it's the size of a region
> > identified by a reg entry.
> >
> > I think this can be simplified by my suggestion above.
> >
> Yes
Aah, I see that this is already in use, so it's a bit late to change
this...
>
> > > +
> > > +2. Clock Provider/Consumer Binding
> > > +
> > > +Most of the binding are from the common clock binding[1].
> > > + [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > +
> > > +Required properties:
> > > +- compatible : Should include one or more of the following:
I didn't spot this earlier, but why "one or more"? are the 2.0 variants
backwards compatible with the 1.0 variants.
> > > + - "fsl,qoriq-core-pll-[1,2].x": Indicates a core PLL clock
> > device
> > > + - "fsl,qoriq-core-mux-[1,2].x": Indicates a core multiplexer
> > clock
> > > + device; divided from the core PLL clock
> >
> > As above, I'd prefer a complete list of the basic strings we expect.
> >
> There is no list here, just *-mux-1.x and *-mux-2.x
> What kind of list do you prefer?
Something like:
- compatible: Should include at least one of:
* "fsl,qoriq-core-pll-1.0" for core PLL clocks (v1.0)
* "fsl,qoriq-core-pll-2.0" for core PLL clocks (v2.0)
* "fsl,qoriq-core-mux-1.0" for core mux clocks (v1.0)
* "fsl,qoriq-core-mux-2.0" for core mux clocks (v2.0)
The *-2.0 variants are backwards compatible with the *-1.0 variants,
so for compatiblity a *-1.0 variant string should be in the list.
>
> > > + - "fixed-clock": From common clock binding; indicates output
> > clock
> > > + of oscillator
> > > + - "fsl,qoriq-sysclk-[1,2].x": Indicates input system clock
> >
> > Here too.
> >
> > > +- #clock-cells: From common clock binding; indicates the number of
> > > + output clock. 0 is for one output clock; 1 for more than one
> > > +clock
> >
> > If a clock source has multiple outputs, what those outputs are and what
> > values in clock-cells they correspond to should be described here.
> >
> There is no way to describe the values of multiple outputs here.
> This property is the type of BOOL. See the clock-bindings.txt.
Sorry? #clock-cells is most definitely _not_ a bool:
17: #clock-cells: Number of cells in a clock specifier; Typically 0 for nodes
18: with a single clock output and 1 for nodes with multiple
19: clock outputs.
And neither are the clock-specifiers encoded with those cells. Consider:
pll0: pll0@800 {
#clock-cells = <1>;
reg = <0x800 0x4>;
compatible = "fsl,qoriq-core-pll-1.0";
clocks = <&sysclk>;
clock-output-names = "pll0", "pll0-div2";
};
Here the value of the cells in a clock-specifier seem to be:
0: pll0
1: pll0-div2
So in a consumer, the valid values of the cells in a clock-specifier
are:
consumer: device {
compatible = "vendor,some-device";
clocks = <&pll0 0>, /* pll0 */
<&pll0 1>; /* pll0-div2 */
};
There must be some meaning assigned to the values of the cells in the
clock-specifier (e.g. linear index of the clock output in the hardware).
It would be good to describe this, other clock bindings do.
>
> > > +
> > > +Recommended properties:
> > > +- clocks: Should be the phandle of input parent clock
> > > +- clock-names: From common clock binding, indicates the clock name
> >
> > That description's a bit opaque.
> >
> > What's the name of the clock input on these units? That's what clock-
> > names should contain, and that should be documented.
> >
> Is it necessary to document these names since they are totally used
> by clock provider and clock consumer has no idea about them?
I'm not sure I follow -- clocks and clock-names are used by consumers.
They define which clocks are inputs to a consumer, and the names of the
clock inputs on the consumer:
consumer@0xffff0000 {
reg = <0xffff0000 0x1000>;
compatible = "vendor,some-consumer";
clocks = <&pl011 0>,
<&otherclock 43 22>,
<&pl011 1>;
clock-names = "apb_pclk",
"pixel_clk",
"scanout_clk";
};
Here the set of clock-names would be defined in the binding of the
consumer, based on the clock input names in the IP documentation -- they
tell the consumer which clock inputs on the consumer the clocks
described in clocks are wired in to, and describe nothing about output
of the provider. Using clock-names allows the set of clocks on an IP to
change over revisions and for some clock inputs to be optional.
>
> > Do we not _always_ need the parent clock?
> >
> Not for fixed-clock that its parent clock is oscillator :)
Certainly fixed-clock doesn't need any parent clock, but I guess PLLs
and muxes always do.
>
> > If we have a clock do we need a clock-names entry for it?
> >
> Technically yes, but I don't bother to add it if the clock node has
> only one clocks.
Ok. Where we only have one input clock, this doesn't need to be
described.
Do the mux clocks always take 3 input clocks (judging by the examples
they do)?
>
> > > +- clock-output-names: From common clock binding, indicates the names
> > of
> > > + output clocks
> > > +- reg: Should be the offset and length of clock block base address.
> > > + The length should be 4.
> > > +
> > > +Example for clock block and clock provider:
> > > +/ {
> > > + clockgen: global-utilities@e1000 {
> > > + compatible = "fsl,p5020-clockgen", "fsl,qoriq-clockgen-
> > 1.0";
> > > + reg = <0xe1000 0x1000>;
> > > + clock-frequency = <0>;
> >
> > That looks odd.
> >
> Yes, but it has already existed here before this patch.
> Can I move it to sysclk clock node since it is not used yet?
I hadn't realised there were DTS with this already. Why is there a clock
with clock-frequency = <0> at all? Surely that isn't useful...
>
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> > > +
> > > + sysclk: sysclk {
> > > + #clock-cells = <0>;
> > > + compatible = "fsl,qoriq-sysclk-1.0",
> > > + "fixed-clock";
> >
> > We didn't mention in the binding that "fsl,qoriq-sysclk-1.0" was
> > compatible with "fixed-clock" and should have "fixed-clock" in the
> > compatible list...
> >
> OK, will fix it.
Cheers. Also, doesn't a fixed-clock require a clock-frequency?
>
> > > + clock-output-names = "sysclk";
> > > + }
> > > +
> > > + pll0: pll0@800 {
> > > + #clock-cells = <1>;
> > > + reg = <0x800 0x4>;
> > > + compatible = "fsl,qoriq-core-pll-1.0";
> > > + clocks = <&sysclk>;
> > > + clock-output-names = "pll0", "pll0-div2";
> > > + };
> > > +
> > > + pll1: pll1@820 {
> > > + #clock-cells = <1>;
> > > + reg = <0x820 0x4>;
> > > + compatible = "fsl,qoriq-core-pll-1.0";
> > > + clocks = <&sysclk>;
> > > + clock-output-names = "pll1", "pll1-div2";
> > > + };
> > > +
> > > + mux0: mux0@0 {
> > > + #clock-cells = <0>;
> > > + reg = <0x0 0x4>;
> > > + compatible = "fsl,qoriq-core-mux-1.0";
> > > + clocks = <&pll0 0>, <&pll0 1>, <&pll1 0>,
> > <&pll1 1>;
> > > + clock-names = "pll0_0", "pll0_1", "pll1_0",
> > "pll1_1";
> > > + clock-output-names = "cmux0";
> > > + };
> > > +
> > > + mux1: mux1@20 {
> > > + #clock-cells = <0>;
> > > + reg = <0x20 0x4>;
> > > + compatible = "fsl,qoriq-core-mux-1.0";
> > > + clocks = <&pll0 0>, <&pll0 1>, <&pll1 0>,
> > <&pll1 1>;
> > > + clock-names = "pll0_0", "pll0_1", "pll1_0",
> > "pll1_1";
I didn't spot this last time, but the clock-names here seem to be the
names of the outputs from the provider, rather than the input names of
the consumer. This goes against the intended purpose of clock-names.
> > > + clock-output-names = "cmux1";
> >
> > How does the mux choose which input clock to use at a point in time?
> >
> That is decided at runtime. Different input clock will lead to the different
> Clock frequency that the CPUs work on.
Ok.
Cheers,
Mark.
^ permalink raw reply
* Re: Gianfar driver crashes in Kernel v3.10
From: Thomas Hühn @ 2013-10-11 9:17 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev@lists.ozlabs.org, claudiu.manoil@freescale.com
In-Reply-To: <1381270140.7979.287.camel@snotra.buserror.net>
Hi Scott,
On 09.10.2013, at 00:09, Scott Wood <scottwood@freescale.com> wrote:
> On Fri, 2013-10-04 at 12:03 +0000, Thomas H=FChn wrote:
>> [code]
>> [ 2671.841927] Oops: Exception in kernel mode, sig: 5 [#1]
>> [ 2671.847141] Freescale P1014
>> [ 2671.849925] Modules linked in: ath9k pppoe ppp_async iptable_nat =
ath9k_common pppox p
>> e xt_tcpudp xt_tcpmss xt_string xt_statistic xt_state xt_recent =
xt_quota xt_pkttype xt_o
>> mark xt_connbytes xt_comment xt_addrtype xt_TCPMSS xt_REDIRECT =
xt_NETMAP xt_LOG xt_IPMAR
>> ms_datafab ums_cypress ums_alauda slhc nf_nat_tftp nf_nat_snmp_basic =
nf_nat_sip nf_nat_r
>> ntrack_sip nf_conntrack_rtsp nf_conntrack_proto_gre nf_conntrack_irc =
nf_conntrack_h323 n
>> compat_xtables compat ath sch_teql sch_tbf sch_sfq sch_red sch_prio =
sch_htb sch_gred sc
>> skbedit act_mirred em_u32 cls_u32 cls_tcindex cls_flow cls_route =
cls_fw sch_hfsc sch_ing
>> r usb_storage leds_gpio ohci_hcd ehci_platform ehci_hcd sd_mod =
scsi_mod fsl_mph_dr_of gp
>> [ 2671.988946] CPU: 0 PID: 5209 Comm: iftop Not tainted 3.10.13 #2
>> [ 2671.994859] task: c4b22220 ti: c7ff8000 task.ti: c477e000
>> [ 2672.000250] NIP: c018c7a0 LR: c018c794 CTR: c000b070
>> [ 2672.005206] REGS: c7ff9f10 TRAP: 3202 Not tainted (3.10.13)
>=20
> Trap 0x3202 is a watchdog timer.
>=20
> Did you get a "Bad trap at=85" line before the above dump? =20
I need to setup my test scenario again as I just copied the crash lines =
out of it without saving the full file with all lines.
> Do you have
> any idea why the watchdog would have been armed without =
CONFIG_BOOKE_WDT
> being set? =20
> Is CONFIG_BOOKE_WDT set?
>=20
This config option is not set. Should I try with the CONFIG_BOOK_WDT =
enabled ?
Greetings Thomas
> -Scott
>=20
>=20
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Fwd: Gianfar driver crashes in Kernel v3.10
From: Thomas Hühn @ 2013-10-11 8:59 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <90BE8C5D-E23B-41B5-BB52-8A0C0758931D@net.t-labs.tu-berlin.de>
Hi Claudiu,
> Does this show up on a half duplex (100Mb/s) link?
In my testsetup I always used 1GBit Ethernet connections, so no 100MBit =
tested yet.
Should I do so ?
> Could you provide following for the gianfar interface, on your setup:
sure:=20
> # ethtool ethX
root@Bluse-home:~# ethtool eth0
Settings for eth0:
Supported ports: [ ]
Supported link modes: 1000baseT/Full=20
Supported pause frame use: No
Supports auto-negotiation: No
Advertised link modes: 1000baseT/Full=20
Advertised pause frame use: No
Advertised auto-negotiation: No
Speed: 1000Mb/s
Duplex: Full
Port: MII
PHYAD: 0
Transceiver: external
Auto-negotiation: on
Current message level: 0x0000003f (63)
drv probe link timer ifdown ifup
Link detected: yes
> and
> # ethtool -d ethX | grep 500
>=20
root@Bluse-home:~# ethtool -d eth0 | grep 500
0x0500: 00 00 00 3f 00 00 72 05 40 60 50 60 00 a1 f0 37=20
> Is there any other indication before this Oops? Like a tx timeout =
WARN?
>=20
Nothing there about any timeout.
Greetings Thomas
> Thanks,
> Claudiu
>=20
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH RFC 00/77] Re-design MSI/MSI-X interrupts enablement pattern
From: Alexander Gordeev @ 2013-10-11 8:41 UTC (permalink / raw)
To: Mark Lord
Cc: linux-mips, VMware, Inc., linux-nvme, linux-ide, H. Peter Anvin,
linux-s390, Andy King, linux-scsi, linux-rdma, x86, Ingo Molnar,
linux-pci, iss_storagedev, linux-driver, Tejun Heo, Bjorn Helgaas,
Dan Williams, Jon Mason, Solarflare linux maintainers, netdev,
linux-kernel, Ralf Baechle, e1000-devel, Martin Schwidefsky,
linux390, linuxppc-dev
In-Reply-To: <5257357E.8080506@start.ca>
On Thu, Oct 10, 2013 at 07:17:18PM -0400, Mark Lord wrote:
> Just to help us all understand "the loop" issue..
>
> Here's an example of driver code which uses the existing MSI-X interfaces,
> for a device which can work with either 16, 8, 4, 2, or 1 MSI-X interrupt.
> This is from a new driver I'm working on right now:
>
>
> static int xx_alloc_msix_irqs (struct xx_dev *dev, int nvec)
> {
> xx_disable_all_irqs(dev);
> do {
> if (nvec < 2)
> xx_prep_for_1_msix_vector(dev);
> else if (nvec < 4)
> xx_prep_for_2_msix_vectors(dev);
> else if (nvec < 8)
> xx_prep_for_4_msix_vectors(dev);
> else if (nvec < 16)
> xx_prep_for_8_msix_vectors(dev);
> else
> xx_prep_for_16_msix_vectors(dev);
> nvec = pci_enable_msix(dev->pdev, dev->irqs, dev->num_vectors);
> } while (nvec > 0);
>
> if (nvec) {
> kerr(dev->name, "pci_enable_msix() failed, err=%d", nvec);
> dev->num_vectors = 0;
> return nvec;
> }
> return 0; /* success */
> }
Yeah, that is a very good example.
I would move all xx_prep_for_<pow2>_msix_vector() functions to a single
helper i.e. xx_prep_msix_vectors(dev, ndev).
Considering also (a) we do not want to waste unused platform resources
associated with MSI-Xs and pull more quota than needed and (b) fixing
couple of bugs, this function could look like this:
static int xx_alloc_msix_irqs(struct xx_dev *dev, int nvec_in)
{
int nvec = roundup_pow_of_two(nvec_in); /* assume 0 > nvec_in <= 16 */
int rc;
xx_disable_all_irqs(dev);
retry:
xx_prep_for_msix_vectors(dev, nvec);
rc = pci_enable_msix(dev->pdev, dev->irqs, nvec); /* (b) */
if (rc > 0) {
nvec = rounddown_pow_of_two(nvec); /* (a) */
goto retry;
}
if (rc) {
kerr(dev->name, "pci_enable_msix() failed, err=%d", rc);
dev->num_vectors = 0;
return rc;
}
dev->num_vectors = nvec; /* (b) */
return 0; /* success */
}
Now, this is a loop-free alternative:
static int xx_alloc_msix_irqs(struct xx_dev *dev, int nvec)
{
nvec = roundup_pow_of_two(nvec); /* assume 0 > nvec <= 16 */
xx_disable_all_irqs(dev);
pci_lock_msi(dev->pdev);
rc = pci_get_msix_limit(dev->pdev, nvec);
if (rc < 0)
goto err;
nvec = min(nvec, rc); /* if limit is more than requested */
nvec = rounddown_pow_of_two(nvec); /* (a) */
xx_prep_for_msix_vectors(dev, nvec);
rc = pci_enable_msix(dev->pdev, dev->irqs, nvec); /* (b) */
if (rc < 0)
goto err;
pci_unlock_msi(dev->pdev);
dev->num_vectors = nvec; /* (b) */
return 0;
err:
pci_unlock_msi(dev->pdev);
kerr(dev->name, "pci_enable_msix() failed, err=%d", rc);
dev->num_vectors = 0;
return rc;
}
--
Regards,
Alexander Gordeev
agordeev@redhat.com
^ permalink raw reply
* Re: [PATCH v2 3/6] powerpc/pci: use pci_is_pcie() to simplify code
From: Yijing Wang @ 2013-10-11 8:22 UTC (permalink / raw)
To: Gavin Shan
Cc: linux-pci, linux-kernel, James E.J. Bottomley, Paul Mackerras,
Hanjun Guo, Bjorn Helgaas, linuxppc-dev
In-Reply-To: <20131011075642.GA20443@shangw.(null)>
>> In my idea, dev->pcie_cap(here is pci_dev->pcie_cap) will update in set_pcie_port_type() function,
>> and this function always be called after allocate pci device. We get pci_dev by eeh_dev_to_pci_dev(),
>> I think pci_dev has been initialized completely.
>>
>>> This function has possibility to be invoked before that. However,
>>> we don't have the binding (eeh device <-> PCI device) for the case.
>>> So the piece of code shouldn't be running
>>
>> In PCI core, I knew
>>
>> pci_scan_device()
>> pci_setup_device()
>> set_pcie_port_type()
>> pci_dev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>>
>> In powerpc, I also found
>>
>> of_scan_pci_dev()
>> of_create_pci_dev()
>> set_pcie_port_type()
>> pci_dev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>>>
>>> However, it's a bit safer to have pci_find_capability(dev, PCI_CAP_ID_EXP)
>>> as well even though we needn't it for 99.9% cases if you agree :-)
>>
>> I agree, this function is not the performance bottleneck,
>> safety is more important. :)
>> So if Bjorn and Benjamin think it's not safe, it's ok to drop it. :)
>>
>
> No, it's not what I mean. Anyway, "v3" looks good to me.
> At least, it can save PCI-CFG access cycles find locate
> the PCIe capability position :-)
Thanks! :)
>
> Thanks,
> Gavin
>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 3/6] powerpc/pci: use pci_is_pcie() to simplify code
From: Gavin Shan @ 2013-10-11 7:56 UTC (permalink / raw)
To: Yijing Wang
Cc: Gavin Shan, Hanjun Guo, linux-kernel, James E.J. Bottomley,
Paul Mackerras, linux-pci, Bjorn Helgaas, linuxppc-dev
In-Reply-To: <5257A882.1070809@huawei.com>
On Fri, Oct 11, 2013 at 03:28:02PM +0800, Yijing Wang wrote:
>On 2013/10/11 14:53, Gavin Shan wrote:
>> On Fri, Oct 11, 2013 at 02:33:58PM +0800, Yijing Wang wrote:
>>> On 2013/10/11 14:16, Gavin Shan wrote:
>>>> On Fri, Oct 11, 2013 at 04:49:56PM +1100, Benjamin Herrenschmidt wrote:
>>>>> On Fri, 2013-09-06 at 14:30 -0600, Bjorn Helgaas wrote:
>>>>>> On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote:
>>
>> .../...
>>
>>>>>>> Use pci_is_pcie() to simplify code.
>>>>>>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>>>>>> index 55593ee..6ebbe54 100644
>>>>>>> --- a/arch/powerpc/kernel/eeh.c
>>>>>>> +++ b/arch/powerpc/kernel/eeh.c
>>>>>>> @@ -189,8 +189,7 @@ static size_t eeh_gather_pci_data(struct eeh_dev *edev, char * buf, size_t len)
>>>>>>> }
>>>>>>>
>>>>>>> /* If PCI-E capable, dump PCI-E cap 10, and the AER */
>>>>>>> - cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
>>>>>>> - if (cap) {
>>>>>>> + if (pci_is_pcie(dev)) {
>>>>>>> n += scnprintf(buf+n, len-n, "pci-e cap10:\n");
>>>>>>> printk(KERN_WARNING
>>>>>>> "EEH: PCI-E capabilities and status follow:\n");
>>>>>
>>>>> So we remove reading of "cap", but slightly further down the code does:
>>>>>
>>>>> for (i=0; i<=8; i++) {
>>>>> eeh_ops->read_config(dn, cap+4*i, 4, &cfg);
>>>>> n += scnprintf(buf+n, len-n, "%02x:%x\n", 4*i, cfg);
>>>>> printk(KERN_WARNING "EEH: PCI-E %02x: %08x\n", i, cfg);
>>>>> }
>>>>>
>>>>> Which actually *uses* the value of "cap" ... oops :-)
>>>>>
>>>>
>>>> It's my fault and I should have looked into the changes more closely.
>>>> How about changing it like this:
>>>>
>>>> cap = pci_is_pcie(dev) ? pci_pcie_cap(dev) :
>>>> pci_find_capability(dev, PCI_CAP_ID_EXP);
>>>> if (cap) {
>>>> ...
>>>> }
>>>>
>>>> It would save some PCI-CFG access cycles for most cases :-)
>>>
>>> Hi Gavin, it's not your fault, it's my fault. :)
>>>
>>> Because pci_pcie_cap(dev) == dev->pcie_cap == pci_find_capability(dev, PCI_CAP_ID_EXP);
>>>
>>> so I think it's ok to use dev->pcie_cap instead of stale "cap".
>>>
>>
>> Yijing, There has one exception: dev->pcie_cap isn't updated yet.
>
>In my idea, dev->pcie_cap(here is pci_dev->pcie_cap) will update in set_pcie_port_type() function,
>and this function always be called after allocate pci device. We get pci_dev by eeh_dev_to_pci_dev(),
>I think pci_dev has been initialized completely.
>
>> This function has possibility to be invoked before that. However,
>> we don't have the binding (eeh device <-> PCI device) for the case.
>> So the piece of code shouldn't be running
>
>In PCI core, I knew
>
>pci_scan_device()
> pci_setup_device()
> set_pcie_port_type()
> pci_dev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>
>In powerpc, I also found
>
>of_scan_pci_dev()
> of_create_pci_dev()
> set_pcie_port_type()
> pci_dev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>>
>> However, it's a bit safer to have pci_find_capability(dev, PCI_CAP_ID_EXP)
>> as well even though we needn't it for 99.9% cases if you agree :-)
>
>I agree, this function is not the performance bottleneck,
>safety is more important. :)
>So if Bjorn and Benjamin think it's not safe, it's ok to drop it. :)
>
No, it's not what I mean. Anyway, "v3" looks good to me.
At least, it can save PCI-CFG access cycles find locate
the PCIe capability position :-)
Thanks,
Gavin
^ permalink raw reply
* Re: Gianfar driver crashes in Kernel v3.10
From: Claudiu Manoil @ 2013-10-11 7:49 UTC (permalink / raw)
To: Scott Wood; +Cc: Thomas Hühn, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1381441287.7979.455.camel@snotra.buserror.net>
On 10/11/2013 12:41 AM, Scott Wood wrote:
> On Thu, 2013-10-10 at 14:07 +0300, Claudiu Manoil wrote:
>> On 10/4/2013 3:28 PM, Thomas H=C3=BChn wrote:
>>>
>>> [code]
>>> [ 2671.841927] Oops: Exception in kernel mode, sig: 5 [#1]
>>> [ 2671.847141] Freescale P1014
>>> [ 2671.849925] Modules linked in: ath9k pppoe ppp_async iptable_nat
>>> ath9k_common pppox p
>>> e xt_tcpudp xt_tcpmss xt_string xt_statistic xt_state xt_recent xt_qu=
ota
>>> xt_pkttype xt_o
>>> mark xt_connbytes xt_comment xt_addrtype xt_TCPMSS xt_REDIRECT xt_NET=
MAP
>>> xt_LOG xt_IPMAR
>>> ms_datafab ums_cypress ums_alauda slhc nf_nat_tftp nf_nat_snmp_basic
>>> nf_nat_sip nf_nat_r
>>> ntrack_sip nf_conntrack_rtsp nf_conntrack_proto_gre nf_conntrack_irc
>>> nf_conntrack_h323 n
>>> compat_xtables compat ath sch_teql sch_tbf sch_sfq sch_red sch_prio
>>> sch_htb sch_gred sc
>>> skbedit act_mirred em_u32 cls_u32 cls_tcindex cls_flow cls_route cls_=
fw
>>> sch_hfsc sch_ing
>>> r usb_storage leds_gpio ohci_hcd ehci_platform ehci_hcd sd_mod scsi_m=
od
>>> fsl_mph_dr_of gp
>>> [ 2671.988946] CPU: 0 PID: 5209 Comm: iftop Not tainted 3.10.13 #2
>>> [ 2671.994859] task: c4b22220 ti: c7ff8000 task.ti: c477e000
>>> [ 2672.000250] NIP: c018c7a0 LR: c018c794 CTR: c000b070
>>> [ 2672.005206] REGS: c7ff9f10 TRAP: 3202 Not tainted (3.10.13)
>>> [ 2672.011028] MSR: 00029000 <CE,EE,ME> CR: 48000024 XER: 20000000
>>> [ 2672.017125]
>>> GPR00: 000000ff c477fde0 c4b22220 00000000 00000000 000000ff 00000000
>>> 70000000
>>> GPR08: ffffffff 00000008 00000000 ffffffff 00000046 10022248 00000000
>>> 00000008
>>> GPR16: c781b3c0 c781b3c0 000000ff 00000000 00000001 0000021c 00000086
>>> fffff800
>>> GPR24: c7980300 00000000 00000001 00000040 00000003 c4b33000 00000000
>>> 00000001
>>> [ 2672.046832] NIP [c018c7a0] gfar_poll+0x424/0x520
>>> [ 2672.051442] LR [c018c794] gfar_poll+0x418/0x520
>>> [ 2672.055962] Call Trace:
>>> [ 2672.058402] [c477fde0] [c018c674] gfar_poll+0x2f8/0x520 (unreliabl=
e)
>>> [ 2672.064762] [c477fe80] [c01b0ce8] net_rx_action+0x6c/0x158
>>> [ 2672.070249] [c477feb0] [c0027dc4] __do_softirq+0xbc/0x16c
>>> [ 2672.075642] [c477ff00] [c0027f7c] irq_exit+0x4c/0x68
>>> [ 2672.080604] [c477ff10] [c00041f8] do_IRQ+0xf4/0x10c
>>> [ 2672.085478] [c477ff40] [c000ca3c] ret_from_except+0x0/0x18
>>> [ 2672.090991] --- Exception: 501 at 0x48083c28
>>> [ 2672.090991] LR =3D 0x48083bf8
>>> [ 2672.098378] Instruction dump:
>>> [ 2672.101338] 7f8f2040 419cfcc4 80900000 38a00000 8061004c 7e118378
>>> 81c10050 7ffafb78
>>> [ 2672.109092] 4bf9eaa1 83810034 7c7e1b78 8361003c <83210038> 83a1004=
c
>>> 48000060 41a2004c
>>> [ 2672.117021] ---[ end trace 565fb54528d305fa ]---
>>> [ 2672.121628]
>>> [ 2673.103130] Kernel panic - not syncing: Fatal exception in interru=
pt
>>> [ 2673.109474] Rebooting in 3 seconds..
>>>
>>> U-Boot 2010.12-svn15934 (Dec 11 2012 - 16:23:49)
>>> [/code]
>>>
>>
>> Hi,
>>
>> Does this show up on a half duplex (100Mb/s) link?
>> Could you provide following for the gianfar interface, on your setup:
>> # ethtool ethX
>> and
>> # ethtool -d ethX | grep 500
>>
>> Is there any other indication before this Oops? Like a tx timeout WARN=
?
>
> It's a watchdog interrupt (CPU watchdog, not netdev). I think it's onl=
y
> showing up in the gianfar code because that's what's running (unless th=
e
> gianfar code is causing the watchdog daemon to not run).
>
Hi Scott,
Good to know that the exception is triggered by the watchdog, and at
this point I assume they simply enabled the watchdog support in kernel
(as you know, it's not enabled by the default config) and that the
exception triggered as the system froze. Since this reportedly happens
under certain traffic conditions (not "high network load, or routing
traffic") I think that information about the link state (whether
it's 100 Mb/s half duplex or not) is relevant here. Any other
indication on top of that (if there is any) is also useful.
Thanks.
Claudiu
^ permalink raw reply
* RE: [PATCH v4 4/4] powerpc/85xx: add sysfs for pw20 state and altivec idle
From: Wang Dongsheng-B40534 @ 2013-10-11 7:43 UTC (permalink / raw)
To: Wood Scott-B07421; +Cc: linuxppc-dev@lists.ozlabs.org, Bhushan Bharat-R65777
In-Reply-To: <1381243807.7979.198.camel@snotra.buserror.net>
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogVHVlc2RheSwgT2N0b2JlciAwOCwgMjAxMyAxMDo1MCBQTQ0KPiBUbzogV2Fu
ZyBEb25nc2hlbmctQjQwNTM0DQo+IENjOiBXb29kIFNjb3R0LUIwNzQyMTsgQmh1c2hhbiBCaGFy
YXQtUjY1Nzc3OyBsaW51eHBwYy0NCj4gZGV2QGxpc3RzLm96bGFicy5vcmcNCj4gU3ViamVjdDog
UmU6IFtQQVRDSCB2NCA0LzRdIHBvd2VycGMvODV4eDogYWRkIHN5c2ZzIGZvciBwdzIwIHN0YXRl
IGFuZA0KPiBhbHRpdmVjIGlkbGUNCj4gDQo+IE9uIE1vbiwgMjAxMy0xMC0wNyBhdCAyMjo1OCAt
MDUwMCwgV2FuZyBEb25nc2hlbmctQjQwNTM0IHdyb3RlOg0KPiA+DQo+ID4gPiAtLS0tLU9yaWdp
bmFsIE1lc3NhZ2UtLS0tLQ0KPiA+ID4gRnJvbTogV29vZCBTY290dC1CMDc0MjENCj4gPiA+IFNl
bnQ6IFR1ZXNkYXksIE9jdG9iZXIgMDEsIDIwMTMgNzowNiBBTQ0KPiA+ID4gVG86IFdhbmcgRG9u
Z3NoZW5nLUI0MDUzNA0KPiA+ID4gQ2M6IFdvb2QgU2NvdHQtQjA3NDIxOyBCaHVzaGFuIEJoYXJh
dC1SNjU3Nzc7IGxpbnV4cHBjLQ0KPiA+ID4gZGV2QGxpc3RzLm96bGFicy5vcmcNCj4gPiA+IFN1
YmplY3Q6IFJlOiBbUEFUQ0ggdjQgNC80XSBwb3dlcnBjLzg1eHg6IGFkZCBzeXNmcyBmb3IgcHcy
MCBzdGF0ZQ0KPiBhbmQNCj4gPiA+IGFsdGl2ZWMgaWRsZQ0KPiA+ID4NCj4gPiA+IE9uIFN1biwg
MjAxMy0wOS0yOSBhdCAwMTo1NyAtMDUwMCwgV2FuZyBEb25nc2hlbmctQjQwNTM0IHdyb3RlOg0K
PiA+ID4gPiBJIHRoaW5rIHdlIG5lZWQgdG8gZG8gdGhpczoNCj4gPiA+ID4NCj4gPiA+ID4gI2Rl
ZmluZSBVNjRfTE9XX01BU0sgICAgICAgICAgICAweGZmZmZmZmZmVUxMDQo+ID4gPiA+ICNkZWZp
bmUgVTY0X01BU0sgICAgICAgICAgICAgICAgMHhmZmZmZmZmZmZmZmZmZmZmVUxMDQo+ID4gPiA+
DQo+ID4gPiA+ICAgICAgICAgdTMyIHRtcF9yZW07DQo+ID4gPiA+ICAgICAgICAgdTY0IG5zX3Vf
cmVtLCBuc191LCBuc19sLCBuc19sX2NhcnJ5Ow0KPiA+ID4gPiAgICAgICAgIHU2NCBjeWNsZTsN
Cj4gPiA+ID4NCj4gPiA+ID4gICAgICAgICBuc191ID0gbnMgPj4gMzI7DQo+ID4gPiA+ICAgICAg
ICAgbnNfbCA9IG5zICYgVTY0X0xPV19NQVNLOw0KPiA+ID4gPg0KPiA+ID4gPiAgICAgICAgIG5z
X2wgKj0gdGJfdGlja3NfcGVyX3VzZWM7DQo+ID4gPiA+ICAgICAgICAgbnNfbF9jYXJyeSA9IG5z
X2wgPj4gMzI7DQo+ID4gPiA+ICAgICAgICAgbnNfdSAqPSB0Yl90aWNrc19wZXJfdXNlYzsNCj4g
PiA+ID4gICAgICAgICBuc191ICs9IG5zX2xfY2Fycnk7DQo+ID4gPiA+DQo+ID4gPiA+ICAgICAg
ICAgbnNfdSA9IGRpdl91NjRfcmVtKG5zX3UsIDEwMDAsICZ0bXBfcmVtKTsNCj4gPiA+ID4gICAg
ICAgICBuc191X3JlbSA9IHRtcF9yZW07DQo+ID4gPiA+ICAgICAgICAgbnNfbCA9IChuc19sICYg
VTY0X0xPV19NQVNLKSB8ICgobnNfdV9yZW0pIDw8IDMyKTsNCj4gPiA+ID4gICAgICAgICBuc19s
ID0gZGl2X3U2NChuc19sLCAxMDAwKTsNCj4gPiA+ID4NCj4gPiA+ID4gICAgICAgICBpZiAobnNf
dSA+PiAzMikNCj4gPiA+ID4gICAgICAgICAgICAgICAgIGN5Y2xlID0gVTY0X01BU0s7DQo+ID4g
PiA+ICAgICAgICAgZWxzZQ0KPiA+ID4gPiAgICAgICAgICAgICAgICAgY3ljbGUgPSAobnNfdSA8
PCAzMikgfCAobnNfbCAmIFU2NF9MT1dfTUFTSyk7DQo+ID4gPiA+DQo+ID4gPiA+IEkgaGFzIGFs
cmVhZHkgdGVzdGVkIHRoaXMgY29kZSwgYW5kIHdvcmtzIGdvb2QuIDopDQo+ID4gPg0KPiA+ID4g
VWdoLiAgSSBkb24ndCB0aGluayB3ZSBuZWVkIHRvIGdldCB0aGlzIGNvbXBsaWNhdGVkIChhbmQg
SSdkIHJhdGhlcg0KPiBub3QNCj4gPiA+IHNwZW5kIHRoZSB0aW1lIHZlcmlmeWluZyB0aGUgY29y
cmVjdG5lc3Mgb2YgdGhpcykuDQo+ID4gPg0KPiA+ID4gSWYgZm9yIHNvbWUgcmVhc29uIHdlIGRp
ZCBuZWVkIHNvbWV0aGluZyBsaWtlIHRoaXMgaW4gc29tZSBvdGhlcg0KPiBjb250ZXh0DQo+ID4g
PiAoSSBkb24ndCB3YW50IHRvIHNlZSBpdCBqdXN0IGZvciBwdzIwKSwgSSdkIGJlIG1vcmUgaW5j
bGluZWQgdG8gc2VlDQo+ID4gPiBnZW5lcmFsIDEyOC1iaXQgbXVsdC9kaXZpZGUgc3VwcG9ydC4N
Cj4gPiA+DQo+ID4gSSB3b3VsZCBsaWtlIHRvIHVzZSBteSB2ZXJzaW9uLDopLCBiZWNhdXNlIGl0
IGNhbiBoYW5kbGUgYW55IHNpdHVhdGlvbg0KPiBhbmQgd2UgZG8gbm90IG5lZWQgdG8gcmVzdHJp
Y3QgdXNlcnMuDQo+IA0KPiBJdCBhbHNvIHdvdWxkIHRha2UgbW9yZSB0aW1lIHRvIHJldmlldyB0
aGFuIEkgaGF2ZSB0byBzcGVuZCBvbiBpdCwgbm90DQo+IHRvIG1lbnRpb24gdGhlIGltcGFjdCBv
biBhbnlvbmUgaW4gdGhlIGZ1dHVyZSB0aGF0IHdhbnRzIHRvIHVuZGVyc3RhbmQNCj4gb3IgbWFp
bnRhaW4gdGhpcyBjb2RlIC0tIGFsbCBmb3IgdmVyeSB1bmxpa2VseSBzaXR1YXRpb25zIChhbmQg
dGhlDQo+ICJmYWlsdXJlIiBpbiB0aG9zZSB2ZXJ5IHVubGlrZWx5IHNpdHVhdGlvbnMgaXMganVz
dCB0aGF0IHdlIGdvIGludG8gUFcyMA0KPiBtb3JlIG9mdGVuIHRoYW4gaW50ZW5kZWQpLg0KPiAN
Ck9LLCBJIHdpbGwgdXNlIHlvdXIgdmVyaXNvbiwgOikNCg0KICAgICAgICBpZiAobnMgPj0gMTAw
MDApDQogICAgICAgICAgICAgICAgY3ljbGUgPSAoKG5zICsgNTAwKSAvIDEwMDApICogdGJfdGlj
a3NfcGVyX3VzZWM7DQogICAgICAgIGVsc2UNCiAgICAgICAgICAgICAgICBjeWNsZSA9IGRpdl91
NjQoKHU2NClucyAqIHRiX3RpY2tzX3Blcl91c2VjLCAxMDAwKTsNCg0KLWRvbmdzaGVuZw0KDQo+
IC1TY290dA0KPiANCg0K
^ permalink raw reply
* Re: [PATCH v2 3/6] powerpc/pci: use pci_is_pcie() to simplify code
From: Yijing Wang @ 2013-10-11 7:28 UTC (permalink / raw)
To: Gavin Shan
Cc: linux-pci, linux-kernel, James E.J. Bottomley, Paul Mackerras,
Hanjun Guo, Bjorn Helgaas, linuxppc-dev
In-Reply-To: <20131011065329.GA5013@shangw.(null)>
On 2013/10/11 14:53, Gavin Shan wrote:
> On Fri, Oct 11, 2013 at 02:33:58PM +0800, Yijing Wang wrote:
>> On 2013/10/11 14:16, Gavin Shan wrote:
>>> On Fri, Oct 11, 2013 at 04:49:56PM +1100, Benjamin Herrenschmidt wrote:
>>>> On Fri, 2013-09-06 at 14:30 -0600, Bjorn Helgaas wrote:
>>>>> On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote:
>
> .../...
>
>>>>>> Use pci_is_pcie() to simplify code.
>>>>>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>>>>> index 55593ee..6ebbe54 100644
>>>>>> --- a/arch/powerpc/kernel/eeh.c
>>>>>> +++ b/arch/powerpc/kernel/eeh.c
>>>>>> @@ -189,8 +189,7 @@ static size_t eeh_gather_pci_data(struct eeh_dev *edev, char * buf, size_t len)
>>>>>> }
>>>>>>
>>>>>> /* If PCI-E capable, dump PCI-E cap 10, and the AER */
>>>>>> - cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
>>>>>> - if (cap) {
>>>>>> + if (pci_is_pcie(dev)) {
>>>>>> n += scnprintf(buf+n, len-n, "pci-e cap10:\n");
>>>>>> printk(KERN_WARNING
>>>>>> "EEH: PCI-E capabilities and status follow:\n");
>>>>
>>>> So we remove reading of "cap", but slightly further down the code does:
>>>>
>>>> for (i=0; i<=8; i++) {
>>>> eeh_ops->read_config(dn, cap+4*i, 4, &cfg);
>>>> n += scnprintf(buf+n, len-n, "%02x:%x\n", 4*i, cfg);
>>>> printk(KERN_WARNING "EEH: PCI-E %02x: %08x\n", i, cfg);
>>>> }
>>>>
>>>> Which actually *uses* the value of "cap" ... oops :-)
>>>>
>>>
>>> It's my fault and I should have looked into the changes more closely.
>>> How about changing it like this:
>>>
>>> cap = pci_is_pcie(dev) ? pci_pcie_cap(dev) :
>>> pci_find_capability(dev, PCI_CAP_ID_EXP);
>>> if (cap) {
>>> ...
>>> }
>>>
>>> It would save some PCI-CFG access cycles for most cases :-)
>>
>> Hi Gavin, it's not your fault, it's my fault. :)
>>
>> Because pci_pcie_cap(dev) == dev->pcie_cap == pci_find_capability(dev, PCI_CAP_ID_EXP);
>>
>> so I think it's ok to use dev->pcie_cap instead of stale "cap".
>>
>
> Yijing, There has one exception: dev->pcie_cap isn't updated yet.
In my idea, dev->pcie_cap(here is pci_dev->pcie_cap) will update in set_pcie_port_type() function,
and this function always be called after allocate pci device. We get pci_dev by eeh_dev_to_pci_dev(),
I think pci_dev has been initialized completely.
> This function has possibility to be invoked before that. However,
> we don't have the binding (eeh device <-> PCI device) for the case.
> So the piece of code shouldn't be running
In PCI core, I knew
pci_scan_device()
pci_setup_device()
set_pcie_port_type()
pci_dev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
In powerpc, I also found
of_scan_pci_dev()
of_create_pci_dev()
set_pcie_port_type()
pci_dev->pcie_cap = pci_find_capability(pdev, PCI_CAP_ID_EXP);
>
> However, it's a bit safer to have pci_find_capability(dev, PCI_CAP_ID_EXP)
> as well even though we needn't it for 99.9% cases if you agree :-)
I agree, this function is not the performance bottleneck,
safety is more important. :)
So if Bjorn and Benjamin think it's not safe, it's ok to drop it. :)
Thanks!
Yijing.
>
> Thanks,
> Gavin
>
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH 1/2] tty/hvc_console: Add DTR/RTS callback to handle HUPCL control
From: Benjamin Herrenschmidt @ 2013-10-11 7:15 UTC (permalink / raw)
To: Hendrik Brueckner
Cc: linux-s390, gregkh, heiko.carstens, linuxppc-dev, linux-kernel,
brueckner, schwidefsky, jslaby
In-Reply-To: <1372777635-10423-2-git-send-email-brueckner@linux.vnet.ibm.com>
On Tue, 2013-07-02 at 17:07 +0200, Hendrik Brueckner wrote:
> Introduce a new callback to explicitly handle the HUPCL termios control flag.
> This prepares for a follow-up commit for the hvc_iucv device driver to
> improve handling when to drop an established network connection.
>
> The callback naming is based on the recently added tty_port interface to
> facilitate a potential refactoring of the hvc_console to use tty_port
> functions.
I only just noticed that ... oops. Why add those dtr_rts() calls ? We
already have tiocmset in there which is used to set DTR on HVSI consoles
such as hvc_opal when using hvsi_lib...
Any reason why a separate callback was needed ?
Cheers,
Ben.
> Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> ---
> drivers/tty/hvc/hvc_console.c | 11 ++++++++++-
> drivers/tty/hvc/hvc_console.h | 3 +++
> 2 files changed, 13 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index eb255e8..9eba119 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -361,7 +361,12 @@ static int hvc_open(struct tty_struct *tty, struct file * filp)
> tty->driver_data = NULL;
> tty_port_put(&hp->port);
> printk(KERN_ERR "hvc_open: request_irq failed with rc %d.\n", rc);
> - }
> + } else
> + /* We are ready... raise DTR/RTS */
> + if (C_BAUD(tty))
> + if (hp->ops->dtr_rts)
> + hp->ops->dtr_rts(hp, 1);
> +
> /* Force wakeup of the polling thread */
> hvc_kick();
>
> @@ -393,6 +398,10 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
> /* We are done with the tty pointer now. */
> tty_port_tty_set(&hp->port, NULL);
>
> + if (C_HUPCL(tty))
> + if (hp->ops->dtr_rts)
> + hp->ops->dtr_rts(hp, 0);
> +
> if (hp->ops->notifier_del)
> hp->ops->notifier_del(hp, hp->data);
>
> diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
> index 674d23c..9131019 100644
> --- a/drivers/tty/hvc/hvc_console.h
> +++ b/drivers/tty/hvc/hvc_console.h
> @@ -75,6 +75,9 @@ struct hv_ops {
> /* tiocmget/set implementation */
> int (*tiocmget)(struct hvc_struct *hp);
> int (*tiocmset)(struct hvc_struct *hp, unsigned int set, unsigned int clear);
> +
> + /* Callbacks to handle tty ports */
> + void (*dtr_rts)(struct hvc_struct *hp, int raise);
> };
>
> /* Register a vterm and a slot index for use as a console (console_init) */
^ permalink raw reply
* Re: [PATCH v2 3/6] powerpc/pci: use pci_is_pcie() to simplify code
From: Gavin Shan @ 2013-10-11 6:53 UTC (permalink / raw)
To: Yijing Wang
Cc: Gavin Shan, Hanjun Guo, linux-kernel, James E.J. Bottomley,
Paul Mackerras, linux-pci, Bjorn Helgaas, linuxppc-dev
In-Reply-To: <52579BD6.40802@huawei.com>
On Fri, Oct 11, 2013 at 02:33:58PM +0800, Yijing Wang wrote:
>On 2013/10/11 14:16, Gavin Shan wrote:
>> On Fri, Oct 11, 2013 at 04:49:56PM +1100, Benjamin Herrenschmidt wrote:
>>> On Fri, 2013-09-06 at 14:30 -0600, Bjorn Helgaas wrote:
>>>> On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote:
.../...
>>>>> Use pci_is_pcie() to simplify code.
>>>>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>>>> index 55593ee..6ebbe54 100644
>>>>> --- a/arch/powerpc/kernel/eeh.c
>>>>> +++ b/arch/powerpc/kernel/eeh.c
>>>>> @@ -189,8 +189,7 @@ static size_t eeh_gather_pci_data(struct eeh_dev *edev, char * buf, size_t len)
>>>>> }
>>>>>
>>>>> /* If PCI-E capable, dump PCI-E cap 10, and the AER */
>>>>> - cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
>>>>> - if (cap) {
>>>>> + if (pci_is_pcie(dev)) {
>>>>> n += scnprintf(buf+n, len-n, "pci-e cap10:\n");
>>>>> printk(KERN_WARNING
>>>>> "EEH: PCI-E capabilities and status follow:\n");
>>>
>>> So we remove reading of "cap", but slightly further down the code does:
>>>
>>> for (i=0; i<=8; i++) {
>>> eeh_ops->read_config(dn, cap+4*i, 4, &cfg);
>>> n += scnprintf(buf+n, len-n, "%02x:%x\n", 4*i, cfg);
>>> printk(KERN_WARNING "EEH: PCI-E %02x: %08x\n", i, cfg);
>>> }
>>>
>>> Which actually *uses* the value of "cap" ... oops :-)
>>>
>>
>> It's my fault and I should have looked into the changes more closely.
>> How about changing it like this:
>>
>> cap = pci_is_pcie(dev) ? pci_pcie_cap(dev) :
>> pci_find_capability(dev, PCI_CAP_ID_EXP);
>> if (cap) {
>> ...
>> }
>>
>> It would save some PCI-CFG access cycles for most cases :-)
>
>Hi Gavin, it's not your fault, it's my fault. :)
>
>Because pci_pcie_cap(dev) == dev->pcie_cap == pci_find_capability(dev, PCI_CAP_ID_EXP);
>
>so I think it's ok to use dev->pcie_cap instead of stale "cap".
>
Yijing, There has one exception: dev->pcie_cap isn't updated yet.
This function has possibility to be invoked before that. However,
we don't have the binding (eeh device <-> PCI device) for the case.
So the piece of code shouldn't be running
However, it's a bit safer to have pci_find_capability(dev, PCI_CAP_ID_EXP)
as well even though we needn't it for 99.9% cases if you agree :-)
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH v2 3/6] powerpc/pci: use pci_is_pcie() to simplify code
From: Yijing Wang @ 2013-10-11 6:28 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Gavin Shan, linux-pci, linux-kernel, James E.J. Bottomley,
Paul Mackerras, Hanjun Guo, Bjorn Helgaas, linuxppc-dev
In-Reply-To: <1381470596.5630.61.camel@pasglop>
On 2013/10/11 13:49, Benjamin Herrenschmidt wrote:
> On Fri, 2013-09-06 at 14:30 -0600, Bjorn Helgaas wrote:
>> On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote:
>>> Use pci_is_pcie() to simplify code.
>>>
>>> Acked-by: Kumar Gala <galak@kernel.crashing.org>
>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>>> Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Cc: Paul Mackerras <paulus@samba.org>
>>> Cc: linuxppc-dev@lists.ozlabs.org
>>> Cc: linux-kernel@vger.kernel.org
>>> ---
>>> arch/powerpc/kernel/eeh.c | 3 +--
>>> arch/powerpc/sysdev/fsl_pci.c | 2 +-
>>> 2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> Ben, Paul, this has no dependencies on anything new to PCI or any
>> other patches in this series, so you can take it through the POWERPC
>> tree. If you don't want to do that, let me know and I can take it.
>>
>> If you want it:
>>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> It's also quite broken :-)
>
> See below:
>
>>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>> index 55593ee..6ebbe54 100644
>>> --- a/arch/powerpc/kernel/eeh.c
>>> +++ b/arch/powerpc/kernel/eeh.c
>>> @@ -189,8 +189,7 @@ static size_t eeh_gather_pci_data(struct eeh_dev *edev, char * buf, size_t len)
>>> }
>>>
>>> /* If PCI-E capable, dump PCI-E cap 10, and the AER */
>>> - cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
>>> - if (cap) {
>>> + if (pci_is_pcie(dev)) {
>>> n += scnprintf(buf+n, len-n, "pci-e cap10:\n");
>>> printk(KERN_WARNING
>>> "EEH: PCI-E capabilities and status follow:\n");
>
> So we remove reading of "cap", but slightly further down the code does:
>
> for (i=0; i<=8; i++) {
> eeh_ops->read_config(dn, cap+4*i, 4, &cfg);
> n += scnprintf(buf+n, len-n, "%02x:%x\n", 4*i, cfg);
> printk(KERN_WARNING "EEH: PCI-E %02x: %08x\n", i, cfg);
> }
>
> Which actually *uses* the value of "cap" ... oops :-)
Hi Benjamin,
Thanks for your review and comments! I will update it at once.
Thanks!
Yijing.
>
>>> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
>>> index 46ac1dd..5402a1d 100644
>>> --- a/arch/powerpc/sysdev/fsl_pci.c
>>> +++ b/arch/powerpc/sysdev/fsl_pci.c
>>> @@ -41,7 +41,7 @@ static void quirk_fsl_pcie_header(struct pci_dev *dev)
>>> u8 hdr_type;
>>>
>>> /* if we aren't a PCIe don't bother */
>>> - if (!pci_find_capability(dev, PCI_CAP_ID_EXP))
>>> + if (!pci_is_pcie(dev))
>>> return;
>>>
>>> /* if we aren't in host mode don't bother */
>>> --
>>> 1.7.1
>>>
>>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
> .
>
--
Thanks!
Yijing
^ permalink raw reply
* Re: [PATCH v2 3/6] powerpc/pci: use pci_is_pcie() to simplify code
From: Yijing Wang @ 2013-10-11 6:33 UTC (permalink / raw)
To: Gavin Shan
Cc: linux-pci, linux-kernel, James E.J. Bottomley, Paul Mackerras,
Hanjun Guo, Bjorn Helgaas, linuxppc-dev
In-Reply-To: <20131011061654.GA561@shangw.(null)>
On 2013/10/11 14:16, Gavin Shan wrote:
> On Fri, Oct 11, 2013 at 04:49:56PM +1100, Benjamin Herrenschmidt wrote:
>> On Fri, 2013-09-06 at 14:30 -0600, Bjorn Helgaas wrote:
>>> On Thu, Sep 05, 2013 at 03:55:27PM +0800, Yijing Wang wrote:
>>>> Use pci_is_pcie() to simplify code.
>>>>
>>>> Acked-by: Kumar Gala <galak@kernel.crashing.org>
>>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>>>> Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
>>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>> Cc: Paul Mackerras <paulus@samba.org>
>>>> Cc: linuxppc-dev@lists.ozlabs.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> ---
>>>> arch/powerpc/kernel/eeh.c | 3 +--
>>>> arch/powerpc/sysdev/fsl_pci.c | 2 +-
>>>> 2 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> Ben, Paul, this has no dependencies on anything new to PCI or any
>>> other patches in this series, so you can take it through the POWERPC
>>> tree. If you don't want to do that, let me know and I can take it.
>>>
>>> If you want it:
>>>
>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> It's also quite broken :-)
>>
>> See below:
>>
>>>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>>> index 55593ee..6ebbe54 100644
>>>> --- a/arch/powerpc/kernel/eeh.c
>>>> +++ b/arch/powerpc/kernel/eeh.c
>>>> @@ -189,8 +189,7 @@ static size_t eeh_gather_pci_data(struct eeh_dev *edev, char * buf, size_t len)
>>>> }
>>>>
>>>> /* If PCI-E capable, dump PCI-E cap 10, and the AER */
>>>> - cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
>>>> - if (cap) {
>>>> + if (pci_is_pcie(dev)) {
>>>> n += scnprintf(buf+n, len-n, "pci-e cap10:\n");
>>>> printk(KERN_WARNING
>>>> "EEH: PCI-E capabilities and status follow:\n");
>>
>> So we remove reading of "cap", but slightly further down the code does:
>>
>> for (i=0; i<=8; i++) {
>> eeh_ops->read_config(dn, cap+4*i, 4, &cfg);
>> n += scnprintf(buf+n, len-n, "%02x:%x\n", 4*i, cfg);
>> printk(KERN_WARNING "EEH: PCI-E %02x: %08x\n", i, cfg);
>> }
>>
>> Which actually *uses* the value of "cap" ... oops :-)
>>
>
> It's my fault and I should have looked into the changes more closely.
> How about changing it like this:
>
> cap = pci_is_pcie(dev) ? pci_pcie_cap(dev) :
> pci_find_capability(dev, PCI_CAP_ID_EXP);
> if (cap) {
> ...
> }
>
> It would save some PCI-CFG access cycles for most cases :-)
Hi Gavin, it's not your fault, it's my fault. :)
Because pci_pcie_cap(dev) == dev->pcie_cap == pci_find_capability(dev, PCI_CAP_ID_EXP);
so I think it's ok to use dev->pcie_cap instead of stale "cap".
I have updated this patch.
Thanks for your review and comments!
Thanks!
Yijing.
>
>>>> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
>>>> index 46ac1dd..5402a1d 100644
>>>> --- a/arch/powerpc/sysdev/fsl_pci.c
>>>> +++ b/arch/powerpc/sysdev/fsl_pci.c
>>>> @@ -41,7 +41,7 @@ static void quirk_fsl_pcie_header(struct pci_dev *dev)
>>>> u8 hdr_type;
>>>>
>>>> /* if we aren't a PCIe don't bother */
>>>> - if (!pci_find_capability(dev, PCI_CAP_ID_EXP))
>>>> + if (!pci_is_pcie(dev))
>>>> return;
>>>>
>>>> /* if we aren't in host mode don't bother */
>
> Thanks,
> Gavin
>
>
> .
>
--
Thanks!
Yijing
^ 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