Re: [PATCH v9 04/10] clk: Add Lynx 10G SerDes PLL driver
Quoting Sean Anderson (2022-12-29 16:01:33)
> This adds support for the PLLs found in Lynx 10G "SerDes" devices found on
> various NXP QorIQ SoCs. There are two PLLs in each SerDes. This driver has
> been split from the main PHY driver to allow for better review, even though
> these PLLs are not present anywhere else besides the SerDes. An auxiliary
> device is not used as it offers no benefits over a function call (and there
> is no need to have a separate device).
>
> The PLLs are modeled as clocks proper to let us take advantage of the
> existing clock infrastructure.
What advantage do we gain?
> I have not given the same treatment to the
> per-lane clocks because they need to be programmed in-concert with the rest
> of the lane settings. One tricky thing is that the VCO (PLL) rate exceeds
> 2^32 (maxing out at around 5GHz). This will be a problem on 32-bit
> platforms, since clock rates are stored as unsigned longs. To work around
> this, the pll clock rate is generally treated in units of kHz.
This looks like a disadvantage. Are we reporting the frequency in kHz to
the clk framework?
>
> The PLLs are configured rather interestingly. Instead of the usual direct
> programming of the appropriate divisors, the input and output clock rates
> are selected directly. Generally, the only restriction is that the input
> and output must be integer multiples of each other. This suggests some kind
> of internal look-up table. The datasheets generally list out the supported
> combinations explicitly, and not all input/output combinations are
> documented. I'm not sure if this is due to lack of support, or due to an
> oversight. If this becomes an issue, then some combinations can be
> blacklisted (or whitelisted). This may also be necessary for other SoCs
> which have more stringent clock requirements.
I'm wondering if a clk provider should be created at all here. Who is
the consumer of the clk? The phy driver itself? Does the clk provided
need to interact with other clks in the system? Or is the clk tree
wholly self-contained?
Can the phy consumer configure the output frequency directly via
phy_configure() or when the phy is enabled? I'm thinking the phy driver
can call clk_set_rate() on the parent 'rfclk' before or after setting
the bits to control the output rate, and use clk_round_rate() to figure
out what input frequencies are supported for the output frequency
desired. This would avoid kHz overflowing 32-bits, and the big clk lock
getting blocked on some other clk in the system changing rates.
BTW, what sort of phy is this? Some networking device?
>
> diff --git a/drivers/clk/clk-fsl-lynx-10g.c b/drivers/clk/clk-fsl-lynx-10g.c
> new file mode 100644
> index ..61f68b5ae675
> --- /dev/null
> +++ b/drivers/clk/clk-fsl-lynx-10g.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Sean Anderson
> + *
> + * This file contains the implementation for the PLLs found on Lynx 10G phys.
> + *
> + * XXX: The VCO rate of the PLLs can exceed ~4GHz, which is the maximum rate
> + * expressable in an unsigned long. To work around this, rates are specified
> in
> + * kHz. This is as if there was a division by 1000 in the PLL.
> + */
> +
> +#include
Is this include used? If not, please remove.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define PLL_STRIDE 0x20
> +#define PLLa(a, off) ((a) * PLL_STRIDE + (off))
> +#define PLLaRSTCTL(a) PLLa(a, 0x00)
> +#define PLLaCR0(a) PLLa(a, 0x04)
> +
> +#define PLLaRSTCTL_RSTREQ BIT(31)
> +#define PLLaRSTCTL_RST_DONEBIT(30)
> +#define PLLaRSTCTL_RST_ERR BIT(29)
[...]
> +
> +static int lynx_clk_init(struct clk_hw_onecell_data *hw_data,
> +struct device *dev, struct regmap *regmap,
> +unsigned int index)
> +{
> + const struct clk_hw *ex_dly_parents;
> + struct clk_parent_data pll_parents[1] = { };
> + struct clk_init_data pll_init = {
> + .ops = _pll_clk_ops,
> + .parent_data = pll_parents,
> + .num_parents = 1,
> + .flags = CLK_GET_RATE_NOCACHE | CLK_SET_RATE_PARENT |
Why is the nocache flag used?
> +CLK_OPS_PARENT_ENABLE,
> + };
> + struct clk_init_data ex_dly_init = {
> + .ops = _ex_dly_clk_ops,
> + .parent_hws = _dly_parents,
> + .num_parents = 1,
> + };
> + struct lynx_clk *clk;
> + int ret;
Re: [PATCH v3 41/51] cpuidle,clk: Remove trace_.*_rcuidle()
Quoting Peter Zijlstra (2023-01-12 11:43:55) > OMAP was the one and only user. > > Signed-off-by: Peter Zijlstra (Intel) > Reviewed-by: Ulf Hansson > Acked-by: Rafael J. Wysocki > Acked-by: Frederic Weisbecker > Tested-by: Tony Lindgren > Tested-by: Ulf Hansson > --- Acked-by: Stephen Boyd
Re: [PATCH v8 4/9] phy: fsl: Add Lynx 10G SerDes driver
Quoting Sean Anderson (2022-12-08 07:36:45) > On 12/6/22 21:17, Stephen Boyd wrote: > > Quoting Sean Anderson (2022-11-01 16:27:21) > >> On 11/1/22 16:10, Stephen Boyd wrote: > >> >> > >> >> Oh, I remember why I did this. I need the reference clock for > >> >> clk_hw_round_rate, > >> >> which is AFAICT the only correct way to implement round_rate. > >> >> > >> > > >> > Is the reference clk the parent of the clk implementing > >> > clk_ops::round_rate()? > >> > >> Yes. We may be able to produce a given output with multiple reference > >> rates. However, the clock API provides no mechanism to say "Don't ask > >> for the parent clock to be rate X, you just tried it and the parent > >> clock can't support it." So instead, we loop over the possible reference > >> rates and pick the first one which the parent says it can round to. > >> > > > > Sorry, I'm lost. Why can't you loop over possible reference rates in > > determine_rate/round_rate clk op here? > > This is what I do currently, but you need to have the parent clock to do > so. With your suggested method, we never actually get a struct clk(_hw) > which we can query for rate support. The clk_hw for the parent is given to the determine_rate clk_op in the clk_rate_request structure. It's stored in the best_parent_hw pointer when the determine_rate function is called. Does that work for you?
Re: [PATCH v8 4/9] phy: fsl: Add Lynx 10G SerDes driver
Quoting Sean Anderson (2022-11-01 16:27:21) > On 11/1/22 16:10, Stephen Boyd wrote: > >> > >> Oh, I remember why I did this. I need the reference clock for > >> clk_hw_round_rate, > >> which is AFAICT the only correct way to implement round_rate. > >> > > > > Is the reference clk the parent of the clk implementing > > clk_ops::round_rate()? > > Yes. We may be able to produce a given output with multiple reference > rates. However, the clock API provides no mechanism to say "Don't ask > for the parent clock to be rate X, you just tried it and the parent > clock can't support it." So instead, we loop over the possible reference > rates and pick the first one which the parent says it can round to. > Sorry, I'm lost. Why can't you loop over possible reference rates in determine_rate/round_rate clk op here?
Re: [PATCH v8 4/9] phy: fsl: Add Lynx 10G SerDes driver
Quoting Sean Anderson (2022-10-28 09:33:59)
> On 10/28/22 12:13, Sean Anderson wrote:
> > On 10/27/22 19:03, Stephen Boyd wrote:
> >>> + ref = devm_clk_get(dev, ref_name);
> >>> + if (IS_ERR(clk->ref)) {
> >>> + ret = PTR_ERR(clk->ref);
> >>> + dev_err_probe(dev, ret, "could not get %s\n", ref_name);
> >>> + goto out;
> >>> + }
> >>> +
> >>> + clk->ref = __clk_get_hw(ref);
> >>
> >> Please don't use __clk_get_hw() for this. Instead use struct
> >> clk_parent_data and set a DT index in the index member to map to this
> >> clk.
> >
> > OK
>
> Oh, I remember why I did this. I need the reference clock for
> clk_hw_round_rate,
> which is AFAICT the only correct way to implement round_rate.
>
Is the reference clk the parent of the clk implementing
clk_ops::round_rate()?
Re: [PATCH v8 4/9] phy: fsl: Add Lynx 10G SerDes driver
Quoting Sean Anderson (2022-10-28 09:13:57)
> On 10/27/22 19:03, Stephen Boyd wrote:
> > Quoting Sean Anderson (2022-10-27 12:11:08)
> >> diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
> >> index 853958fb2c06..a6f9e39b 100644
> >> --- a/drivers/phy/freescale/Kconfig
> >> +++ b/drivers/phy/freescale/Kconfig
> >> @@ -47,3 +47,25 @@ config PHY_FSL_LYNX_28G
> >>found on NXP's Layerscape platforms such as LX2160A.
> >>Used to change the protocol running on SerDes lanes at runtime.
> >>Only useful for a restricted set of Ethernet protocols.
> >> +
> >> +config PHY_FSL_LYNX_10G
> >> + tristate "Freescale QorIQ Lynx 10G SerDes support"
> >> + depends on COMMON_CLK
> >
> > Does something not compile if COMMON_CLK is disabled?
>
> ld: drivers/phy/freescale/phy-fsl-lynx-10g-clk.o: in function
> `lynx_pll_round_rate':
> phy-fsl-lynx-10g-clk.c:(.text+0x444): undefined reference to
> `clk_hw_round_rate'
> ld: drivers/phy/freescale/phy-fsl-lynx-10g-clk.o: in function
> `lynx_clks_init':
> phy-fsl-lynx-10g-clk.c:(.text+0x5eb): undefined reference to
> `devm_clk_hw_register'
> ld: phy-fsl-lynx-10g-clk.c:(.text+0x625): undefined reference to
> `devm_clk_hw_register'
Cool thanks!
>
> >> diff --git a/drivers/phy/freescale/lynx-10g.h
> >> b/drivers/phy/freescale/lynx-10g.h
> >> new file mode 100644
> >> index ..75d9353a867b
> >> --- /dev/null
> >> +++ b/drivers/phy/freescale/lynx-10g.h
> >> @@ -0,0 +1,16 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * Copyright (C) 2022 Sean Anderson
> >> + */
> >> +
> >> +#ifndef LYNX_10G
> >> +#define LYNX_10G
> >> +
> >> +struct clk;
> >> +struct device;
> >> +struct regmap;
> >> +
> >> +int lynx_clks_init(struct device *dev, struct regmap *regmap,
> >
> > Can you use auxiliary bus to register this clk controller instead and
> > then move the clk file to drivers/clk/?
>
> I don't want to have to deal with my clock driver getting unbound (aka
> the user has come and decided to make my life harder). Dynamic binding
> will only add complexity in this situation.
We have 'suppress_bind_attrs' for that. What dynamic binding are you
thinking about?
>
> I don't know how much context you've picked up, but this driver
>
> - Has one consumer, and is is the serdes.
> - Is not accessible from outside the serdes.
> - Does not share any code with other drivers.
> - Has bits in its registers which can control the reset process of lanes
>using the PLLs.
>
> These drivers are tightly coupled to each other. It is very likely IMO
> that changes to one (bugs, features, etc) will affect the other. For
> this reason, I think it makes sense to keep them in the same source
> directory. I actually would have preferred to keep them in the same
> file.
>
Using the auxiliary bus is about getting better code review on the
subsystem specific parts of a device. I'm not going to be paying a lot
of attention to the clk parts of this driver if it is outside
drivers/clk. Making this change helps with better code review.
The Kconfig symbol could be the same for the clk part and the phy part,
and this is already split to a different file. It seems that your
argument for keeping the clk file in the phy directory is because
they're part of the same phy device. Do you expect to get clk driver
review on the clk parts with the clk implementation in a different
directory?
> >> + for (i = 0; i < NUM_PLLS; i++) {
> >> + ret = lynx_clk_init(hw_data, dev, regmap, i);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + plls[i] = hw_data->hws[LYNX10G_PLLa(i)]->clk;
> >> + ex_dlys[i] = hw_data->hws[LYNX10G_PLLa_EX_DLY(i)]->clk;
> >
> > Use clk_hw_get_clk() please.
>
> I don't want to do that, because then I'd have to generate the clock ID
> again. And why do we even need a new clock consumer in the first place?
> This is only for internal use by the driver; the consumer is the same as
> the producer.
>
We want to get rid of the clk pointer stashed in clk_hw. Using
clk_hw_get_clk() lets us easily change that and not need to change this
driver. I don't quite understand why you need to generate a clock ID.
Are you concerned about passing in the 'con_id' argument? That string
can be anything.
Re: [PATCH v8 4/9] phy: fsl: Add Lynx 10G SerDes driver
Quoting Sean Anderson (2022-10-27 12:11:08) > diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig > index 853958fb2c06..a6f9e39b 100644 > --- a/drivers/phy/freescale/Kconfig > +++ b/drivers/phy/freescale/Kconfig > @@ -47,3 +47,25 @@ config PHY_FSL_LYNX_28G > found on NXP's Layerscape platforms such as LX2160A. > Used to change the protocol running on SerDes lanes at runtime. > Only useful for a restricted set of Ethernet protocols. > + > +config PHY_FSL_LYNX_10G > + tristate "Freescale QorIQ Lynx 10G SerDes support" > + depends on COMMON_CLK Does something not compile if COMMON_CLK is disabled? > + depends on ARCH_LAYERSCAPE || PPC || COMPILE_TEST > + select GENERIC_PHY > + select REGMAP_MMIO > + help > + This adds support for the Lynx "SerDes" devices found on various > QorIQ > + SoCs. There may be up to four SerDes devices on each SoC, and each > + device supports up to eight lanes. The SerDes is configured by > + default by the RCW, but this module is necessary in order to support > + some modes (such as 2.5G SGMII or 1000BASE-KX), or clock setups (as > + only as subset of clock configurations are supported by the RCW). > + The hardware supports a variety of protocols, including Ethernet, > + SATA, PCIe, and more exotic links such as Interlaken and Aurora. > This > + driver only supports Ethernet, but it will try not to touch lanes > + configured for other protocols. > + > + If you have a QorIQ processor and want to dynamically reconfigure > your > + SerDes, say Y. If this driver is compiled as a module, it will be > + named phy-fsl-lynx-10g-drv. > diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile > index cedb328bc4d2..1f18936507e0 100644 > --- a/drivers/phy/freescale/Makefile > +++ b/drivers/phy/freescale/Makefile > @@ -3,4 +3,7 @@ obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)+= > phy-fsl-imx8mq-usb.o > obj-$(CONFIG_PHY_MIXEL_LVDS_PHY) += phy-fsl-imx8qm-lvds-phy.o > obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY) += phy-fsl-imx8-mipi-dphy.o > obj-$(CONFIG_PHY_FSL_IMX8M_PCIE) += phy-fsl-imx8m-pcie.o > +phy-fsl-lynx-10g-drv-y += phy-fsl-lynx-10g.o > +phy-fsl-lynx-10g-drv-y += phy-fsl-lynx-10g-clk.o > +obj-$(CONFIG_PHY_FSL_LYNX_10G) += phy-fsl-lynx-10g-drv.o > obj-$(CONFIG_PHY_FSL_LYNX_28G) += phy-fsl-lynx-28g.o > diff --git a/drivers/phy/freescale/lynx-10g.h > b/drivers/phy/freescale/lynx-10g.h > new file mode 100644 > index ..75d9353a867b > --- /dev/null > +++ b/drivers/phy/freescale/lynx-10g.h > @@ -0,0 +1,16 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2022 Sean Anderson > + */ > + > +#ifndef LYNX_10G > +#define LYNX_10G > + > +struct clk; > +struct device; > +struct regmap; > + > +int lynx_clks_init(struct device *dev, struct regmap *regmap, Can you use auxiliary bus to register this clk controller instead and then move the clk file to drivers/clk/? > + struct clk *plls[2], struct clk *ex_dlys[2]); > + > +#endif /* LYNX 10G */ > diff --git a/drivers/phy/freescale/phy-fsl-lynx-10g-clk.c > b/drivers/phy/freescale/phy-fsl-lynx-10g-clk.c > new file mode 100644 > index ..6ec32bdfb9dd > --- /dev/null > +++ b/drivers/phy/freescale/phy-fsl-lynx-10g-clk.c > @@ -0,0 +1,503 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2022 Sean Anderson > + * > + * This file contains the implementation for the PLLs found on Lynx 10G phys. > + * > + * XXX: The VCO rate of the PLLs can exceed ~4GHz, which is the maximum rate > + * expressable in an unsigned long. To work around this, rates are specified > in > + * kHz. This is as if there was a division by 1000 in the PLL. > + */ > + > +#include Ideally clk.h isn't included in a clk provider. This allows us to easily identify drivers that are both a consumer (clk.h) and a provider (clk-provider.h). A provider/consumer is rare. > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "lynx-10g.h" > + > +#define PLL_STRIDE 0x20 > +#define PLLa(a, off) ((a) * PLL_STRIDE + (off)) > +#define PLLaRSTCTL(a) PLLa(a, 0x00) > +#define PLLaCR0(a) PLLa(a, 0x04) > + > +#define PLLaRSTCTL_RSTREQ BIT(31) > +#define PLLaRSTCTL_RST_DONEBIT(30) > +#define PLLaRSTCTL_RST_ERR BIT(29) > +#define PLLaRSTCTL_PLLRST_BBIT(7) > +#define PLLaRSTCTL_SDRST_B BIT(6) > +#define PLLaRSTCTL_SDENBIT(5) > + > +#define PLLaRSTCTL_ENABLE_SET (PLLaRSTCTL_RST_DONE | PLLaRSTCTL_PLLRST_B | \ > +PLLaRSTCTL_SDRST_B | PLLaRSTCTL_SDEN) > +#define PLLaRSTCTL_ENABLE_MASK (PLLaRSTCTL_ENABLE_SET | PLLaRSTCTL_RST_ERR) > + > +#define PLLaCR0_POFF BIT(31) > +#define PLLaCR0_RFCLK_SEL GENMASK(30, 28) >
Re: [PATCH v8 3/9] dt-bindings: clock: Add ids for Lynx 10g PLLs
Quoting Sean Anderson (2022-10-27 12:11:07) > This adds ids for the Lynx 10g SerDes's internal PLLs. These may be used > with assigned-clock* to specify a particular frequency to use. For > example, to set the second PLL (at offset 0x20)'s frequency, use > LYNX10G_PLLa(1). These are for use only in the device tree, and are not > otherwise used by the driver. > > Signed-off-by: Sean Anderson > Acked-by: Rob Herring > --- Acked-by: Stephen Boyd
Re: [PATCH v7 3/8] dt-bindings: clock: Add ids for Lynx 10g PLLs
Quoting Sean Anderson (2022-10-18 16:11:07) > This adds ids for the Lynx 10g SerDes's internal PLLs. These may be used > with assigned-clock* to specify a particular frequency to use. For > example, to set the second PLL (at offset 0x20)'s frequency, use > LYNX10G_PLLa(1). These are for use only in the device tree, and are not > otherwise used by the driver. > > Signed-off-by: Sean Anderson > Acked-by: Rob Herring > --- Acked-by: Stephen Boyd
Re: [PATCH v2 39/44] cpuidle,clk: Remove trace_.*_rcuidle()
Quoting Peter Zijlstra (2022-09-19 03:00:18) > OMAP was the one and only user. > > Signed-off-by: Peter Zijlstra (Intel) > --- Acked-by: Stephen Boyd
Re: [PATCH v6 3/8] dt-bindings: clock: Add ids for Lynx 10g PLLs
Quoting Sean Anderson (2022-09-20 13:23:51) > This adds ids for the Lynx 10g SerDes's internal PLLs. These may be used > with assigned-clock* to specify a particular frequency to use. For > example, to set the second PLL (at offset 0x20)'s frequency, use > LYNX10G_PLLa(1). These are for use only in the device tree, and are not > otherwise used by the driver. > > Signed-off-by: Sean Anderson > Acked-by: Rob Herring > --- Acked-by: Stephen Boyd
Re: [PATCH 36/36] cpuidle,clk: Remove trace_.*_rcuidle()
Quoting Peter Zijlstra (2022-06-08 07:27:59) > OMAP was the one and only user. > > Signed-off-by: Peter Zijlstra (Intel) > --- Acked-by: Stephen Boyd
Re: [PATCH v2 1/1] kernel.h: Split out panic and oops helpers
Quoting Andy Shevchenko (2021-04-09 03:02:50) > kernel.h is being used as a dump for all kinds of stuff for a long time. > Here is the attempt to start cleaning it up by splitting out panic and > oops helpers. > > There are several purposes of doing this: > - dropping dependency in bug.h > - dropping a loop by moving out panic_notifier.h > - unload kernel.h from something which has its own domain > > At the same time convert users tree-wide to use new headers, although > for the time being include new header back to kernel.h to avoid twisted > indirected includes for existing users. > > Signed-off-by: Andy Shevchenko > Reviewed-by: Bjorn Andersson > Acked-by: Mike Rapoport > Acked-by: Corey Minyard > Acked-by: Christian Brauner > Acked-by: Arnd Bergmann > Acked-by: Kees Cook > Acked-by: Wei Liu > Acked-by: Rasmus Villemoes > Signed-off-by: Andrew Morton > --- > drivers/clk/analogbits/wrpll-cln28hpc.c | 4 + Acked-by: Stephen Boyd
Re: [PATCH v2] clk: renesas: r9a06g032: Drop __packed for portability
Quoting Geert Uytterhoeven (2020-11-30 00:57:43)
> The R9A06G032 clock driver uses an array of packed structures to reduce
> kernel size. However, this array contains pointers, which are no longer
> aligned naturally, and cannot be relocated on PPC64. Hence when
> compile-testing this driver on PPC64 with CONFIG_RELOCATABLE=y (e.g.
> PowerPC allyesconfig), the following warnings are produced:
>
> WARNING: 136 bad relocations
> c0616be3 R_PPC64_UADDR64 .rodata+0x000cf338
> c0616bfe R_PPC64_UADDR64 .rodata+0x000cf370
> ...
>
> Fix this by dropping the __packed attribute from the r9a06g032_clkdesc
> definition, trading a small size increase for portability.
>
> This increases the 156-entry clock table by 1 byte per entry, but due to
> the compiler generating more efficient code for unpacked accesses, the
> net size increase is only 76 bytes (gcc 9.3.0 on arm32).
>
> Reported-by: Stephen Rothwell
> Fixes: 4c3d88526eba2143 ("clk: renesas: Renesas R9A06G032 clock driver")
> Signed-off-by: Geert Uytterhoeven
> ---
Applied to clk-fixes
Re: [PATCH v2] clk: renesas: r9a06g032: Drop __packed for portability
Quoting Geert Uytterhoeven (2020-11-30 00:57:43)
> The R9A06G032 clock driver uses an array of packed structures to reduce
> kernel size. However, this array contains pointers, which are no longer
> aligned naturally, and cannot be relocated on PPC64. Hence when
> compile-testing this driver on PPC64 with CONFIG_RELOCATABLE=y (e.g.
> PowerPC allyesconfig), the following warnings are produced:
>
> WARNING: 136 bad relocations
> c0616be3 R_PPC64_UADDR64 .rodata+0x000cf338
> c0616bfe R_PPC64_UADDR64 .rodata+0x000cf370
> ...
>
> Fix this by dropping the __packed attribute from the r9a06g032_clkdesc
> definition, trading a small size increase for portability.
>
> This increases the 156-entry clock table by 1 byte per entry, but due to
> the compiler generating more efficient code for unpacked accesses, the
> net size increase is only 76 bytes (gcc 9.3.0 on arm32).
>
> Reported-by: Stephen Rothwell
> Fixes: 4c3d88526eba2143 ("clk: renesas: Renesas R9A06G032 clock driver")
> Signed-off-by: Geert Uytterhoeven
> ---
Acked-by: Stephen Boyd
Unless you want me to pick this up for clk-fixes?
Re: [PATCH v2 2/2] clk: qoriq: add cpufreq platform device
Quoting Mian Yousaf Kaukab (2020-04-21 01:30:00) > Add a platform device for qoirq-cpufreq driver for the compatible > clockgen blocks. > > Reviewed-by: Yuantian Tang > Acked-by: Viresh Kumar > Signed-off-by: Mian Yousaf Kaukab > --- Acked-by: Stephen Boyd
Re: [PATCH 3/7] docs: fix broken references to text files
Quoting Mauro Carvalho Chehab (2020-02-22 01:00:03) > Several references got broken due to txt to ReST conversion. > > Several of them can be automatically fixed with: > > scripts/documentation-file-ref-check --fix > > Signed-off-by: Mauro Carvalho Chehab > --- > drivers/hwtracing/coresight/Kconfig | 2 +- > > diff --git a/drivers/hwtracing/coresight/Kconfig > b/drivers/hwtracing/coresight/Kconfig > index 6ff30e25af55..6d42a6d3766f 100644 > --- a/drivers/hwtracing/coresight/Kconfig > +++ b/drivers/hwtracing/coresight/Kconfig > @@ -107,7 +107,7 @@ config CORESIGHT_CPU_DEBUG > can quickly get to know program counter (PC), secure state, > exception level, etc. Before use debugging functionality, platform > needs to ensure the clock domain and power domain are enabled > - properly, please refer Documentation/trace/coresight-cpu-debug.rst > + properly, please refer > Documentation/trace/coresight/coresight-cpu-debug.rst > for detailed description and the example for usage. > > endif I ran into this today and almost sent a patch. Can you split this patch up into more pieces and send it off to the respective subsystem maintainers?
Re: [PATCH] powerpc/time: Replace by
Quoting Geert Uytterhoeven (2020-02-12 02:17:36) > The PowerPC time code is not a clock provider, and just needs to call > of_clk_init(). > > Hence it can include instead of . > > Signed-off-by: Geert Uytterhoeven > --- Reviewed-by: Stephen Boyd This has an ifdef around the of_clk_init() call. Can you remove that too given that we stub it out in the header?
Re: [PATCH RFT V3 1/8] clk: divider: add explicit big endian support
Quoting Jonas Gorski (2019-04-23 03:39:59) > No purpose at all, it's an uncaught artifact from rebasing ._. > > Stephen, which one is your preferred way of fixing that? > > a) a new V4 patchset without this line > b) a follow up patch that removes it > c) just removing the line yourself I'll go for c and fix up things. Thanks for the catch Geert!
Re: [PATCH RFT V3 8/8] clk: core: replace clk_{readl, writel} with {readl, writel}
Quoting Jonas Gorski (2019-04-18 04:12:11)
> Now that clk_{readl,writel} is just an alias for {readl,writel}, we can
> switch all users of clk_* to use the accessors directly and remove the
> helpers.
>
> Signed-off-by: Jonas Gorski
> ---
Applied to clk-next
Re: [PATCH RFT V3 7/8] clk: core: remove powerpc special handling
Quoting Jonas Gorski (2019-04-18 04:12:10) > Now that the powerpc clocks are properly marked as big endian, we can > remove the special handling for PowerPC. > > Signed-off-by: Jonas Gorski > --- Applied to clk-next
Re: [PATCH RFT V3 6/8] powerpc/512x: mark clocks as big endian
Quoting Jonas Gorski (2019-04-18 04:12:09) > These clocks' registers are accessed as big endian, so mark them as > such. > > Signed-off-by: Jonas Gorski > --- Applied to clk-next
Re: [PATCH RFT V3 5/8] clk: mux: add explicit big endian support
Quoting Jonas Gorski (2019-04-18 04:12:08) > Add a clock specific flag to switch register accesses to big endian, to > allow runtime configuration of big endian mux clocks. > > Signed-off-by: Jonas Gorski > --- Applied to clk-next
Re: [PATCH RFT V3 4/8] clk: multiplier: add explicit big endian support
Quoting Jonas Gorski (2019-04-18 04:12:07) > Add a clock specific flag to switch register accesses to big endian, to > allow runtime configuration of big endian multiplier clocks. > > Signed-off-by: Jonas Gorski > --- Applied to clk-next
Re: [PATCH RFT V3 3/8] clk: gate: add explicit big endian support
Quoting Jonas Gorski (2019-04-18 04:12:06) > Add a clock specific flag to switch register accesses to big endian, to > allow runtime configuration of big endian gated clocks. > > Signed-off-by: Jonas Gorski > --- Applied to clk-next
Re: [PATCH RFT V3 2/8] clk: fractional-divider: add explicit big endian support
Quoting Jonas Gorski (2019-04-18 04:12:05) > Add a clock specific flag to switch register accesses to big endian, to > allow runtime configuration of big endian fractional divider clocks. > > Signed-off-by: Jonas Gorski > --- Applied to clk-next
Re: [PATCH RFT V3 1/8] clk: divider: add explicit big endian support
Quoting Jonas Gorski (2019-04-18 04:12:04) > Add a clock specific flag to switch register accesses to big endian, to > allow runtime configuration of big endian divider clocks. > > Signed-off-by: Jonas Gorski > --- Applied to clk-next
Re: [PATCH RFT V2 1/8] clk: divider: add explicit big endian support
Quoting Jonas Gorski (2019-04-15 03:10:39)
> @@ -370,7 +388,7 @@ static long clk_divider_round_rate(struct clk_hw *hw,
> unsigned long rate,
> if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> u32 val;
>
> - val = clk_readl(divider->reg) >> divider->shift;
> + val = clk_div_readl(divider->reg) >> divider->shift;
Good deal that kbuild running sparse found that this was supposed to be
divider and not divider->reg. If you can fix that and remove the else in
all the basic type readl wrappers then this series looks good to merge
for me.
Re: [PATCH RFT V2 2/8] clk: fractional-divider: add explicit big endian support
Quoting Jonas Gorski (2019-04-15 03:10:40)
> diff --git a/drivers/clk/clk-fractional-divider.c
> b/drivers/clk/clk-fractional-divider.c
> index fdfe2e423d15..b9988d3b3828 100644
> --- a/drivers/clk/clk-fractional-divider.c
> +++ b/drivers/clk/clk-fractional-divider.c
> @@ -13,6 +13,22 @@
> #include
> #include
>
> +static inline u32 clk_fd_readl(struct clk_fractional_divider *fd)
> +{
> + if (fd->flags & CLK_FRAC_DIVIDER_BIG_ENDIAN)
> + return ioread32be(fd->reg);
> + else
> + return clk_readl(fd->reg);
Can you write it without the else?
if (fd->flags & CLK_FRAC_DIVIDER_BIG_ENDIAN)
return ioread32be(fd->reg);
return clk_readl(fd->reg);
This same comment applies to all the basic clk types.
Re: [PATCH RFC/RFT 1/6] clk: core: add support for generic big endian accesses
Quoting Jonas Gorski (2019-04-08 03:20:34)
> Add a generic flag to mark a clock as big endian register based, and add
> accessors following these.
I like the idea of getting rid of clk_readl() and clk_writel(), but I'd
rather see that this flag is per-basic clk type instead of global to all
clks. Mostly because I don't see it as a clk property that's applicable
in general. So we would either have a flag for dividers and gates that
goes into the respective CLK_{GATE,DIVIDER}_* flag space, or some
wrapper functions like:
clk_gate_register_be()
clk_divider_register_be()
that passes this information through to the basic clk types somehow.
Then if there's no other user left of clk_readl() and clk_writel() I'd
just slam in a patch to all the files that use it to convert them to
readl() and writel(). After that, it would be great to drop the io.h
include from clk-provider.h and push that include out to any code that's
relying on it implicitly.
Re: [PATCH v1 0/4] arm64: dts: NXP: add basic dts file for LX2160A SoC
Quoting Vabhav Sharma (2019-02-26 02:10:46) > These patches were reviewed and acked but dropped during merge window. > Patchwork link was https://lore.kernel.org/patchwork/cover/1004155/ > > Changes for v1: > - Updated lx2160a clockgen in alphabetical order > > Vabhav Sharma (2): > dt-bindings: arm64: add compatible for LX2160A > soc/fsl/guts: Add definition for LX2160A > > Yogesh Gaur (2): > clk: qoriq: increase array size of cmux_to_group > clk: qoriq: Add clockgen support for lx2160a Who do you want to apply these patches?
Re: [PATCH 2/2 v3] clk: qoriq: add more compatibles strings
Quoting Yuantian Tang (2018-10-30 23:57:36) > Add more SoC compatible strings to support more chips. > > Signed-off-by: Yuantian Tang > --- Acked-by: Stephen Boyd
Re: [PATCH v5 3/6] clk: qoriq: increase array size of cmux_to_group
Quoting Vabhav Sharma (2018-10-14 10:08:00) > From: Yogesh Gaur > > Increase size of cmux_to_group array, to accomdate entry of > -1 termination. > > Added -1, terminated, entry for 4080_cmux_grpX. > > Signed-off-by: Yogesh Gaur > Signed-off-by: Vabhav Sharma > --- Acked-by: Stephen Boyd
RE: [PATCH v4 5/6] arm64: dts: add QorIQ LX2160A SoC support
Quoting Vabhav Sharma (2018-10-14 19:58:15)
> > > +
> > > + pmu {
> > > + compatible = "arm,cortex-a72-pmu";
> > > + interrupts = ;
> > > + };
> > > +
> > > + psci {
> > > + compatible = "arm,psci-0.2";
> > > + method = "smc";
> > > + };
> > > +
> > > + memory@8000 {
> > > + // DRAM space - 1, size : 2 GB DRAM
> > > + device_type = "memory";
> > > + reg = <0x 0x8000 0 0x8000>;
> > > + };
> > > +
> > > + ddr1: memory-controller@108 {
> > > + compatible = "fsl,qoriq-memory-controller";
> > > + reg = <0x0 0x108 0x0 0x1000>;
> > > + interrupts = ;
> > > + little-endian;
> > > + };
> > > +
> > > + ddr2: memory-controller@109 {
> > > + compatible = "fsl,qoriq-memory-controller";
> > > + reg = <0x0 0x109 0x0 0x1000>;
> > > + interrupts = ;
> > > + little-endian;
> > > + };
> > > +
> > > + sysclk: sysclk {
> >
> > Name the node a bit generic like clock-xxx.
> There is only one clock-unit, Bootloader(U-boot) require sysclk node during
> device tree fix-up as different platform support varied platform frequency as
> per reset configuration word used.
> Referred other ARM based platform with one clock using name as x: x
Please add a comment above this node with this information. Newcomers
reading this DTS file won't have any idea why this node is specially
named and a comment will help tremendously here.
Re: [PATCH v3 4/6] drivers: clk-qoriq: Add clockgen support for lx2160a
Same subject comment. Quoting Vabhav Sharma (2018-09-23 17:08:59) > From: Yogesh Gaur > > Add clockgen support for lx2160a.
Re: [PATCH v3 3/6] drivers: clk-qoriq: increase array size of cmux_to_group
Subject should be "clk: qoriq: increase array size ..."
Re: [PATCH 3/5] drivers: clk-qoriq: Add clockgen support for lx2160a
Quoting Vabhav Sharma (2018-08-19 23:47:14) > From: Yogesh Gaur > > Add clockgen support for lx2160a. > Added entry for compat 'fsl,lx2160a-clockgen'. > As LX2160A is 16 core, so modified value for NUM_CMUX > > Signed-off-by: Tang Yuantian > Signed-off-by: Yogesh Gaur > Signed-off-by: Vabhav Sharma > --- Acked-by: Stephen Boyd
Re: [PATCH v6 2/4] of: make __of_attach_node() static
On 06/20, frowand.l...@gmail.com wrote: > From: Frank Rowand <frank.row...@sony.com> > > __of_attach_node() is not used outside of drivers/of/dynamic.c. Make > it static and remove it from drivers/of/of_private.h. > > Signed-off-by: Frank Rowand <frank.row...@sony.com> > --- Reviewed-by: Stephen Boyd <sb...@codeaurora.org> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH] powerpc/512x: clk: Remove CLK_IS_ROOT
On 04/19, Stephen Boyd wrote: > This flag is a no-op now (see commit 47b0eeb3dc8a "clk: Deprecate > CLK_IS_ROOT", 2016-02-02) so remove it. > > Cc: Gerhard Sittig <g...@denx.de> > Signed-off-by: Stephen Boyd <sb...@codeaurora.org> > --- Applied to clk-fixes -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc/512x: clk: Remove CLK_IS_ROOT
This flag is a no-op now (see commit 47b0eeb3dc8a "clk: Deprecate
CLK_IS_ROOT", 2016-02-02) so remove it.
Cc: Gerhard Sittig <g...@denx.de>
Signed-off-by: Stephen Boyd <sb...@codeaurora.org>
---
arch/powerpc/platforms/512x/clock-commonclk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/512x/clock-commonclk.c
b/arch/powerpc/platforms/512x/clock-commonclk.c
index c50ea76ba66c..6081fbd75330 100644
--- a/arch/powerpc/platforms/512x/clock-commonclk.c
+++ b/arch/powerpc/platforms/512x/clock-commonclk.c
@@ -221,7 +221,7 @@ static bool soc_has_mclk_mux0_canin(void)
/* convenience wrappers around the common clk API */
static inline struct clk *mpc512x_clk_fixed(const char *name, int rate)
{
- return clk_register_fixed_rate(NULL, name, NULL, CLK_IS_ROOT, rate);
+ return clk_register_fixed_rate(NULL, name, NULL, 0, rate);
}
static inline struct clk *mpc512x_clk_factor(
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [v7, 4/5] powerpc/fsl: move mpc85xx.h to include/linux/fsl
On 03/31/2016 08:07 PM, Yangbo Lu wrote: > drivers/clk/clk-qoriq.c | 3 +-- > For clk part: Acked-by: Stephen Boyd <sb...@codeaurora.org> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 0/2] clk: imx6: add kpp clock for i.MX6UL
On 01/12, Lothar Waßmann wrote: > This patchset adds the clock which is necessary to operate the KPP > unit on i.MX6UL. > The first patch removes bogus whitespace before TABs in indentation. > The second patch adds the clock definition. > Both look fine. Shawn? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 3/5] clk: qoriq: Add ls2080a support.
On 09/19, Scott Wood wrote: > LS2080A is the first implementation of the chassis 3 clockgen, which > has a different register layout than previous chips. It is also little > endian, unlike previous chips. > > Signed-off-by: Scott Wood <scottw...@freescale.com> > --- Acked-by: Stephen Boyd <sb...@codeaurora.org> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 2/5] clk: qoriq: Move chip-specific knowledge into driver
On 09/19, Scott Wood wrote: > The device tree should describe the chips (or chip-like subblocks) in > the system, but it generally does not describe individual registers -- > it should identify, rather than describe, a programming interface. > > This has not been the case with the QorIQ clockgen nodes. The > knowledge of what each bit setting of CLKCnCSR means is encoded in > three places (binding, pll node, and mux node), and the last also needs > to know which options are valid on a particular chip. All three of > these locations are considered stable ABI, making it difficult to fix > mistakes (of which I have found several), much less refactor the > abstraction to be able to address problems, limitations, or new chips. > > Under the current binding, a pll clock specifier of 2 means that the > PLL is divided by 4 -- and the driver implements this, unless there > happen to be four clock-output-names rather than 3, in which case it > interprets it as PLL divided by 3. This does not appear in the binding > documentation at all. That hack is now considered stable ABI. > > The current device tree nodes contain errors, such as saying that > T1040 can set a core clock to PLL/4 when only PLL and PLL/2 are options. > The current binding also ignores some restrictions on clock selection, > such as p5020's requirement that if a core uses the "wrong" PLL, that > PLL must be clocked lower than the "correct" PLL and be at most 80% of > the rated CPU frequency. > > Possibly because of the lack of the ability to express such nuance in > the binding, some valid options are omitted from the device trees, such > as the ability on p4080 to run cores 0-3 from PLL3 and cores 4-7 from > PLL1 (again, only if they are at most 80% of rated CPU frequency). > This omission, combined with excessive caution in the cpufreq driver > (addressed in a subsequent patch), means that currently on a 1500 MHz > p4080 with typical PLL configuration, cpufreq can lower the frequency > to 1200 MHz on half the CPUs and do nothing on the others. With this > patchset, all CPUs can be lowered to 1200 MHz on a rev2 p4080, and on a > rev3 p4080 half can be lowered to 750 MHz and the other half to 600 > MHz. > > The current binding only deals with CPU clocks. To describe FMan in > the device tree, we need to describe its clock. Some chips have > additional muxes that work like the CPU muxes, but are not described in > the device tree. Others require inspecting the Reset Control Word to > determine which PLL is used. Rather than continue to extend this mess, > replace it. Have the driver bind to the chip-specific clockgen > compatible, and keep the detailed description of quirky chip variations > in the driver, where it can be easily fixed, refactored, and extended. > > Older device trees will continue to work (including a workaround for > old ls1021a device trees that are missing compatible and reg in the > clockgen node, which even the old binding required). The pll/mux > details in old device trees will be ignored, but "clocks" properties > pointing at the old nodes will still work, and be directed at the > corresponding new clock. > > Signed-off-by: Scott Wood <scottw...@freescale.com> > --- Acked-by: Stephen Boyd <sb...@codeaurora.org> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 01/44] kernel: Add support for poweroff handler call chain
On 06/18/2015 08:30 AM, Guenter Roeck wrote: On Wed, Jun 17, 2015 at 06:04:54PM -0700, Stephen Boyd wrote: [ ... ] What happened to this series? I want to add shutdown support to my platform and I need to write a register on the PMIC in one driver to configure it for shutdown instead of restart and then write an MMIO register to tell the PMIC to actually do the shutdown in another driver. It seems that the notifier solves this case for me, albeit with the slight complication that I need to order the two with some priority. Can you use the .shutdown driver callback instead ? I see other drivers use that, and check for system_state == SYSTEM_POWER_OFF to power off the hardware. Yes I think that will work. I'll still have to hook pm_power_off() for the mmio register, but I guess that's ok and I don't need to worry about this series then. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 01/44] kernel: Add support for poweroff handler call chain
On 10/06/2014 10:28 PM, Guenter Roeck wrote: Various drivers implement architecture and/or device specific means to remove power from the system. For the most part, those drivers set the global variable pm_power_off to point to a function within the driver. This mechanism has a number of drawbacks. Typically only one scheme to remove power is supported (at least if pm_power_off is used). At least in theory there can be multiple means remove power, some of which may be less desirable. For example, some mechanisms may only power off the CPU or the CPU card, while another may power off the entire system. Others may really just execute a restart sequence or drop into the ROM monitor. Using pm_power_off can also be racy if the function pointer is set from a driver built as module, as the driver may be in the process of being unloaded when pm_power_off is called. If there are multiple poweroff handlers in the system, removing a module with such a handler may inadvertently reset the pointer to pm_power_off to NULL, leaving the system with no means to remove power. Introduce a system poweroff handler call chain to solve the described problems. This call chain is expected to be executed from the architecture specific machine_power_off() function. Drivers providing system poweroff functionality are expected to register with this call chain. By using the priority field in the notifier block, callers can control poweroff handler execution sequence and thus ensure that the poweroff handler with the optimal capabilities to remove power for a given system is called first. What happened to this series? I want to add shutdown support to my platform and I need to write a register on the PMIC in one driver to configure it for shutdown instead of restart and then write an MMIO register to tell the PMIC to actually do the shutdown in another driver. It seems that the notifier solves this case for me, albeit with the slight complication that I need to order the two with some priority. I'm also considering putting the PMIC configuration part into the reboot notifier chain, because it only does things to change the configuration and not actually any shutdown/restart itself. That removes any requirement to get the priority of notifiers right. This series will still be useful for the MMIO register that needs to be toggled though. Right now I have to assign pm_power_off or hook the reboot notifier with a different priority to make this work. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [v4] clk: qoriq: Add support for the FMan clock
On 04/16, Igal.Liberman wrote:
From: Igal Liberman igal.liber...@freescale.com
This patch depends on the following patches:
https://patchwork.ozlabs.org/patch/461151/
https://patchwork.ozlabs.org/patch/461155/
This patche is described by the following binding document update:
https://patchwork.ozlabs.org/patch/461166/
v4: - Replaced fsl,b4-device-config with
fsl,b4860/b4420-device-config
- Updated error messages
v3: Updated commit message
v2: - Added clock maintainers
- Cached FMan clock parent during initialization
- Register the clock after checking if the hardware exists
- updated error messages
Signed-off-by: Igal Liberman igal.liber...@freescale.com
---
drivers/clk/clk-qoriq.c | 213
+++
If I try to compile this on ARM (the Kconfig for this file shows
that ARM is possible) then it fails with this error message:
CC drivers/clk/clk-qoriq.o
drivers/clk/clk-qoriq.c:22:26:
fatal error: asm/fsl_guts.h: No such file or directory
compilation terminated.
1 file changed, 213 insertions(+)
diff --git a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c
index cda90a9..871c6df 100644
--- a/drivers/clk/clk-qoriq.c
+++ b/drivers/clk/clk-qoriq.c
+
+static u8 get_fm_clk_parent(struct clk_hw *hw)
+{
+ return hw-init-flags;
+}
This is very confusing. How is flags the parent index? Please
don't abuse framework data structures. I'm actually thinking we
should replace hw-init with NULL during clk_register() to avoid
this kind of abuse...
+
+static const struct clk_ops fm_clk_ops = {
+ .get_parent = get_fm_clk_parent,
+};
+
+static int get_fm_clk_idx(int fm_id, int *fm_clk_idx)
+{
+ struct ccsr_guts __iomem *guts_regs = NULL;
Unnecessary initialization to NULL. Also, marking a structure as
__iomem is odd. Why do we need to use a struct to figure out
offsets for registers? Why not just use #defines? That would
probably also make it easy to avoid the asm include here.
+ struct device_node *guts;
+ uint32_t reg = 0;
s/uint32_t/u32/
Also unnecessary initialization.
+ int clk_src = 0;
+
+ guts = of_find_matching_node(NULL, guts_device_ids);
+ if (!guts) {
+ pr_err(%s(): could not find GUTS node\n, __func__);
+ return -ENODEV;
+ }
+
+ guts_regs = of_iomap(guts, 0);
+ of_node_put(guts);
+ if (!guts_regs) {
+ pr_err(%s(): ioremap of GUTS node failed\n, __func__);
+ return -ENODEV;
+ }
[...]
+
+static void __init fm_mux_init(struct device_node *np)
+{
+ struct clk_init_data *init;
+ struct clk_hw *hw;
+ int count, i, ret, fm_id = 0, fm_clk_idx;
+ struct clk *clk;
+
+ init = kmalloc((sizeof(struct clk_init_data)), GFP_KERNEL);
Please remove extra parens and do sizeof(*init) so that we don't
have to care about the type matching.
+ if (!init)
+ return;
+
+ /* get the input clock source count */
+ count = of_property_count_strings(np, clock-names);
+ if (count 0) {
+ pr_err(%s(): %s: get clock count error\n,
+__func__, np-name);
+ goto err_init;
+ }
+
+ init-parent_names = kmalloc((sizeof(char *) * count), GFP_KERNEL);
Use kcalloc please
+ if (!init-parent_names)
+ goto err_init;
+
+ for (i = 0; i count; i++)
+ init-parent_names[i] = of_clk_get_parent_name(np, i);
+
+ hw = kzalloc(sizeof(*hw), GFP_KERNEL);
+ if (!hw)
+ goto err_name;
+
+ ret = of_property_read_string_index(np, clock-output-names, 0,
+ init-name);
+ if (ret) {
+ pr_err(%s(): %s: read clock names error\n,
+__func__, np-name);
+ goto err_clk_hw;
+ }
+
+ if (!strcmp(np-name, fm1-clk-mux))
+ fm_id = 1;
+
+ ret = get_fm_clk_idx(fm_id, fm_clk_idx);
+ if (ret)
+ goto err_clk_hw;
+
+ init-ops = fm_clk_ops;
+ init-num_parents = count;
+ /* Save clock source index */
+ init-flags = fm_clk_idx;
Don't do this.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [v4] clk: qoriq: Add support for the FMan clock
On 05/06, Scott Wood wrote:
On Wed, 2015-05-06 at 00:02 -0700, Stephen Boyd wrote:
On 04/16, Igal.Liberman wrote:
+static int get_fm_clk_idx(int fm_id, int *fm_clk_idx)
+{
+ struct ccsr_guts __iomem *guts_regs = NULL;
Unnecessary initialization to NULL. Also, marking a structure as
__iomem is odd. Why do we need to use a struct to figure out
offsets for registers? Why not just use #defines? That would
probably also make it easy to avoid the asm include here.
Using a struct for registers is quite common:
scott@snotra:~/fsl/git/linux/upstream$ git grep struct|grep __iomem|wc -l
3005
$ git grep -E 'struct \w+ __iomem' | wc -l
2212
That's slightly inflated, but ok.
Within drivers/clk there aren't any though, hence my apprehension
$ git grep -E 'struct \w+ __iomem' -- drivers/clk/ | wc -l
0
It provides type-safety, and makes accessing the registers more natural.
Sure, we can leave the struct as is, but to make this compile on
ARM we need to figure something out. Move the struct definition
into include/linux/platform_data/ perhaps?
+ struct device_node *guts;
+ uint32_t reg = 0;
s/uint32_t/u32/
Why?
This matches the rest of the file except for one instance of
uint32_t. I googled it and found [1], perhaps that will help.
Also unnecessary initialization.
Given the if/else if/else if/... nature of how reg is initialized, this
seems like a useful and harmless way of making behavior predictable if
there is a bug.
If there's a possibility of a bug due to missed initialization
perhaps it's a sign the code is too complicated and should be
broken down into smaller functions. For example, this function
could be rewritten to have a match table with function pointers
that return the fm_clk_idx.
[1] http://lkml.iu.edu/hypermail/linux/kernel/1101.3/02176.html
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 04/11] drivers: base: support cpu cache information interface to userspace via sysfs
On 09/03/14 10:00, Sudeep Holla wrote: From: Sudeep Holla sudeep.ho...@arm.com This patch adds initial support for providing processor cache information to userspace through sysfs interface. This is based on already existing implementations(x86, ia64, s390 and powerpc) and hence the interface is intended to be fully compatible. The main purpose of this generic support is to avoid further code duplication to support new architectures and also to unify all the existing different implementations. This implementation maintains the hierarchy of cache objects which reflects the system's cache topology. Cache devices are instantiated as needed as CPUs come online. The cache information is replicated per-cpu even if they are shared. A per-cpu array of cache information maintained is used mainly for sysfs-related book keeping. It also implements the shared_cpu_map attribute, which is essential for enabling both kernel and user-space to discover the system's overall cache topology. This patch also add the missing ABI documentation for the cacheinfo sysfs interface already, which is well defined and widely used. Signed-off-by: Sudeep Holla sudeep.ho...@arm.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Stephen Boyd sb...@codeaurora.org Cc: linux-...@vger.kernel.org Cc: linux...@de.ibm.com Cc: linux-arm-ker...@lists.infradead.org Cc: linux-i...@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-s...@vger.kernel.org Cc: x...@kernel.org Reviewed-by: Stephen Boyd sb...@codeaurora.org Tested-by: Stephen Boyd sb...@codeaurora.org -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] lib: Consolidate DEBUG_STACK_USAGE option
Most arches define CONFIG_DEBUG_STACK_USAGE exactly the same way. Move it to lib/Kconfig.debug so each arch doesn't have to define it. This obviously makes the option generic, but that's fine because the config is already used in generic code. It's not obvious to me that sysrq-P actually does anything different with this option enabled, but I erred on the side of caution by keeping the most inclusive wording. Cc: linux-a...@vger.kernel.org Cc: linux-arm-ker...@lists.infradead.org Cc: uclinux-dist-de...@blackfin.uclinux.org Cc: linux-m...@ml.linux-m32r.org Cc: linux-m...@linux-mips.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux...@vger.kernel.org Cc: sparcli...@vger.kernel.org Cc: Chris Metcalf cmetc...@tilera.com Cc: user-mode-linux-de...@lists.sourceforge.net Cc: x...@kernel.org Signed-off-by: Stephen Boyd sb...@codeaurora.org --- This is on top of mmotm's lib-conslidate-debug_per_cpu_maps patch. arch/arm/Kconfig.debug |7 --- arch/blackfin/Kconfig.debug |9 - arch/m32r/Kconfig.debug |9 - arch/mips/Kconfig.debug |9 - arch/powerpc/Kconfig.debug |9 - arch/score/Kconfig.debug |9 - arch/sh/Kconfig.debug|9 - arch/sparc/Kconfig.debug |9 - arch/tile/Kconfig.debug |9 - arch/um/Kconfig.debug|9 - arch/unicore32/Kconfig.debug |7 --- arch/x86/Kconfig.debug |9 - lib/Kconfig.debug|9 + 13 files changed, 9 insertions(+), 104 deletions(-) diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug index 03d01d7..81cbe40 100644 --- a/arch/arm/Kconfig.debug +++ b/arch/arm/Kconfig.debug @@ -63,13 +63,6 @@ config DEBUG_USER 8 - SIGSEGV faults 16 - SIGBUS faults -config DEBUG_STACK_USAGE - bool Enable stack utilization instrumentation - depends on DEBUG_KERNEL - help - Enables the display of the minimum amount of free stack which each - task has ever had available in the sysrq-T output. - # These options are only for real kernel hackers who want to get their hands dirty. config DEBUG_LL bool Kernel low-level debugging functions diff --git a/arch/blackfin/Kconfig.debug b/arch/blackfin/Kconfig.debug index 2641731..19ccfb3 100644 --- a/arch/blackfin/Kconfig.debug +++ b/arch/blackfin/Kconfig.debug @@ -9,15 +9,6 @@ config DEBUG_STACKOVERFLOW This option will cause messages to be printed if free stack space drops below a certain limit. -config DEBUG_STACK_USAGE - bool Enable stack utilization instrumentation - depends on DEBUG_KERNEL - help - Enables the display of the minimum amount of free stack which each - task has ever had available in the sysrq-T output. - - This option will slow down process creation somewhat. - config DEBUG_VERBOSE bool Verbose fault messages default y diff --git a/arch/m32r/Kconfig.debug b/arch/m32r/Kconfig.debug index 2e1019d..bb1afc1 100644 --- a/arch/m32r/Kconfig.debug +++ b/arch/m32r/Kconfig.debug @@ -9,15 +9,6 @@ config DEBUG_STACKOVERFLOW This option will cause messages to be printed if free stack space drops below a certain limit. -config DEBUG_STACK_USAGE - bool Stack utilization instrumentation - depends on DEBUG_KERNEL - help - Enables the display of the minimum amount of free stack which each - task has ever had available in the sysrq-T and sysrq-P debug output. - - This option will slow down process creation somewhat. - config DEBUG_PAGEALLOC bool Debug page memory allocations depends on DEBUG_KERNEL BROKEN diff --git a/arch/mips/Kconfig.debug b/arch/mips/Kconfig.debug index 5358f90..83ed00a 100644 --- a/arch/mips/Kconfig.debug +++ b/arch/mips/Kconfig.debug @@ -76,15 +76,6 @@ config DEBUG_STACKOVERFLOW provides another way to check stack overflow happened on kernel mode stack usually caused by nested interruption. -config DEBUG_STACK_USAGE - bool Enable stack utilization instrumentation - depends on DEBUG_KERNEL - help - Enables the display of the minimum amount of free stack which each - task has ever had available in the sysrq-T and sysrq-P debug output. - - This option will slow down process creation somewhat. - config SMTC_IDLE_HOOK_DEBUG bool Enable additional debug checks before going into CPU idle loop depends on DEBUG_KERNEL MIPS_MT_SMTC diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug index 12a8d18..f862fc0 100644 --- a/arch/powerpc/Kconfig.debug +++ b/arch/powerpc/Kconfig.debug @@ -35,15 +35,6 @@ config DEBUG_STACKOVERFLOW This option will cause messages to be printed if free stack space drops below a certain limit. -config DEBUG_STACK_USAGE - bool Stack utilization instrumentation
[PATCH] lib: Consolidate DEBUG_PER_CPU_MAPS
DEBUG_PER_CPU_MAPS is used in lib/cpumask.c as well as in inlcude/linux/cpumask.h and thus it has outgrown its use within x86 and powerpc alone. Any arch with SMP support may want to get some more debugging, so make this option generic. Cc: linux-a...@vger.kernel.org Cc: x...@kernel.org Cc: linuxppc-dev@lists.ozlabs.org Signed-off-by: Stephen Boyd sb...@codeaurora.org --- I don't know what tree to send this through, so I'm sending it to Andrew. I suppose mm is as good as anything. arch/powerpc/Kconfig.debug | 12 arch/x86/Kconfig.debug | 11 --- lib/Kconfig.debug | 11 +++ 3 files changed, 11 insertions(+), 23 deletions(-) diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug index 2d38a50..12a8d18 100644 --- a/arch/powerpc/Kconfig.debug +++ b/arch/powerpc/Kconfig.debug @@ -44,18 +44,6 @@ config DEBUG_STACK_USAGE This option will slow down process creation somewhat. -config DEBUG_PER_CPU_MAPS - bool Debug access to per_cpu maps - depends on DEBUG_KERNEL - depends on SMP - default n - ---help--- - Say Y to verify that the per_cpu map being accessed has - been setup. Adds a fair amount of code to kernel memory - and decreases performance. - - Say N if unsure. - config HCALL_STATS bool Hypervisor call instrumentation depends on PPC_PSERIES DEBUG_FS TRACEPOINTS diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug index 615e188..1bf8839 100644 --- a/arch/x86/Kconfig.debug +++ b/arch/x86/Kconfig.debug @@ -75,17 +75,6 @@ config DEBUG_STACK_USAGE This option will slow down process creation somewhat. -config DEBUG_PER_CPU_MAPS - bool Debug access to per_cpu maps - depends on DEBUG_KERNEL - depends on SMP - ---help--- - Say Y to verify that the per_cpu map being accessed has - been setup. Adds a fair amount of code to kernel memory - and decreases performance. - - Say N if unsure. - config X86_PTDUMP bool Export kernel pagetable layout to userspace via debugfs depends on DEBUG_KERNEL diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index c768bcd..c792431 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -993,6 +993,17 @@ config DEBUG_FORCE_WEAK_PER_CPU To ensure that generic code follows the above rules, this option forces all percpu variables to be defined as weak. +config DEBUG_PER_CPU_MAPS + bool Debug access to per_cpu maps + depends on DEBUG_KERNEL + depends on SMP + help + Say Y to verify that the per_cpu map being accessed has + been set up. This adds a fair amount of code to kernel memory + and decreases performance. + + Say N if unsure. + config LKDTM tristate Linux Kernel Dump Test Tool Module depends on DEBUG_FS -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support
On 01/14/2011 11:19 AM, Tony Lindgren wrote: * Stephen Boyd sb...@codeaurora.org [101207 11:00]: On 12/01/2010 12:20 PM, Stephen Boyd wrote: Definitely for TX since it seems like a redundant loop, but I agree RX code has changed. Instead of If RX buffer full Poll for RX buffer full Read character from RX buffer we would have If RX buffer full Read character from RX buffer which doesn't seem all that different assuming the RX buffer doesn't go from full to empty between the If and Poll steps. Hopefully Tony knows more. Tony, any thoughts? Sorry for the delay, looks like I'm only 1 month behind with email.. Sounds like it should work to me. I can try it out if you point me to a patch. I think you acked the patches to make this change. You can test them out by applying the hvc_dcc cleanups and fixes patches (Message-Id: 1292875718-7980-1-git-send-email-sb...@codeaurora.org) on top of this patch. They were sent as a reply to this thread. Stephen -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support
On 12/01/2010 12:20 PM, Stephen Boyd wrote: Definitely for TX since it seems like a redundant loop, but I agree RX code has changed. Instead of If RX buffer full Poll for RX buffer full Read character from RX buffer we would have If RX buffer full Read character from RX buffer which doesn't seem all that different assuming the RX buffer doesn't go from full to empty between the If and Poll steps. Hopefully Tony knows more. Tony, any thoughts? -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support
On 12/01/2010 10:54 AM, Daniel Walker wrote:
Are you talking about __dcc_getstatus only? I don't think adding
volatile is going to hurt anything, if not having it causes problems.
Just marking __dcc_getstatus volatile gives me
0038 hvc_dcc_get_chars:
38: ee10fe11mrc 14, 0, pc, cr0, cr1, {0}
3c: 1afdbne 38 hvc_dcc_get_chars
40: ee103e15mrc 14, 0, r3, cr0, cr5, {0}
44: e3a0mov r0, #0 ; 0x0
48: e6ef3073uxtbr3, r3
4c: ea04b 64 hvc_dcc_get_chars+0x2c
50: ee10ce11mrc 14, 0, ip, cr0, cr1, {0}
54: e31c0101tst ip, #1073741824 ; 0x4000
58: 012fff1ebxeqlr
5c: e7c13000strbr3, [r1, r0]
60: e281add r0, r0, #1 ; 0x1
64: e152cmp r0, r2
68: baf8blt 50 hvc_dcc_get_chars+0x18
6c: e12fff1ebx lr
Seems the compiler keeps the value of __dcc_getchar() in r3 for the
duration of the loop. So we need to mark that one volatile too. I don't
think __dcc_putchar() needs to be marked volatile but it probably
doesn't hurt.
We could maybe drop the looping for TX, but RX has no C based looping
even tho for v7 it's recommended that we loop (presumably for v6 it's
not recommended).
Definitely for TX since it seems like a redundant loop, but I agree RX
code has changed. Instead of
If RX buffer full
Poll for RX buffer full
Read character from RX buffer
we would have
If RX buffer full
Read character from RX buffer
which doesn't seem all that different assuming the RX buffer doesn't go
from full to empty between the If and Poll steps. Hopefully Tony knows more.
Like this?
for (i = 0; i count; ++i) {
if (__dcc_getstatus() DCC_STATUS_RX)
buf[i] = __dcc_getchar();
else
break;
}
It's a micro clean up I guess ..
Yes, it's much clearer that way.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] drivers: char: hvc: add arm JTAG DCC console support
On 11/30/2010 11:25 AM, Daniel Walker wrote:
@@ -682,6 +682,15 @@ config HVC_UDBG
select HVC_DRIVER
default n
+config HVC_DCC
+ bool ARM JTAG DCC console
+ depends on ARM
+ select HVC_DRIVER
+ help
+ This console uses the JTAG DCC on ARM to create a console under the
HVC
Looks like you added one too many spaces for indent here.
diff --git a/drivers/char/hvc_dcc.c b/drivers/char/hvc_dcc.c
new file mode 100644
index 000..6470f63
--- /dev/null
+++ b/drivers/char/hvc_dcc.c
+static inline u32 __dcc_getstatus(void)
+{
+ u32 __ret;
+
+ asm(mrc p14, 0, %0, c0, c1, 0 @ read comms ctrl reg
+ : =r (__ret) : : cc);
+
+ return __ret;
+}
Without marking this asm volatile my compiler decides it can cache the
value of __ret in a register and then check the value of it continually
in hvc_dcc_put_chars() (I had to replace get_wait/put_wait with 1 and
fixup the branch otherwise my disassembler barfed on __dcc_(get|put)char).
hvc_dcc_put_chars:
0: ee103e11mrc 14, 0, r3, cr0, cr1, {0}
4: e3a0c000mov ip, #0 ; 0x0
8: e2033202and r3, r3, #536870912 ; 0x2000
c: ea06b 2c hvc_dcc_put_chars+0x2c
10: e353cmp r3, #0 ; 0x0
14: 1afdbne 10 hvc_dcc_put_chars+0x10
18: e7d1000cldrbr0, [r1, ip]
1c: ee10fe11mrc 14, 0, pc, cr0, cr1, {0}
20: 2afdbcs 1c hvc_dcc_put_chars+0x1c
24: ee000e15mcr 14, 0, r0, cr0, cr5, {0}
28: e28cc001add ip, ip, #1 ; 0x1
2c: e15c0002cmp ip, r2
30: baf6blt 10 hvc_dcc_put_chars+0x10
34: e1a2mov r0, r2
38: e12fff1ebx lr
As you can see, the value of the mrc is checked against DCC_STATUS_TX
(bit 29) and then stored in r3 for later use. Marking this volatile
produces the following:
hvc_dcc_put_chars:
0: e3a03000mov r3, #0 ; 0x0
4: ea07b 28 hvc_dcc_put_chars+0x28
8: ee100e11mrc 14, 0, r0, cr0, cr1, {0}
c: e3100202tst r0, #536870912 ; 0x2000
10: 1afcbne 8 hvc_dcc_put_chars+0x8
14: e7d10003ldrbr0, [r1, r3]
18: ee10fe11mrc 14, 0, pc, cr0, cr1, {0}
1c: 2afdbcs 18 hvc_dcc_put_chars+0x18
20: ee000e15mcr 14, 0, r0, cr0, cr5, {0}
24: e2833001add r3, r3, #1 ; 0x1
28: e1530002cmp r3, r2
2c: baf5blt 8 hvc_dcc_put_chars+0x8
30: e1a2mov r0, r2
34: e12fff1ebx lr
which looks better.
I marked all the asm in this driver as volatile. Is that correct?
+#if defined(CONFIG_CPU_V7)
+static inline char __dcc_getchar(void)
+{
+ char __c;
+
+ asm(get_wait: mrc p14, 0, pc, c0, c1, 0 \n\
+ bne get_wait \n\
+ mrc p14, 0, %0, c0, c5, 0 @ read comms data reg
+ : =r (__c) : : cc);
+
+ return __c;
+}
+#else
+static inline char __dcc_getchar(void)
+{
+ char __c;
+
+ asm(mrc p14, 0, %0, c0, c5, 0 @ read comms data reg
+ : =r (__c));
+
+ return __c;
+}
+#endif
+
+#if defined(CONFIG_CPU_V7)
+static inline void __dcc_putchar(char c)
+{
+ asm(put_wait: mrc p14, 0, pc, c0, c1, 0 \n\
+ bcs put_wait \n\
+ mcr p14, 0, %0, c0, c5, 0
+ : : r (c) : cc);
+}
+#else
+static inline void __dcc_putchar(char c)
+{
+ asm(mcr p14, 0, %0, c0, c5, 0 @ write a char
+ : /* no output register */
+ : r (c));
+}
+#endif
+
I don't think both the v7 and v6 functions are necessary. It seems I can
get away with just the second version of __dcc_(get|put)char() on a v7.
The mrc p14, 0, pc, c0, c1, 0 will assign the top 4 bits (31-28) to the
condition codes NZCV on v7. It also looks like on an ARM11 (a v6) will
also do the same thing if I read the manuals right. The test in the
inline assembly is saying, wait for a character to be ready or wait for
a character to be read then actually write a character or read one. The
code in hvc_dcc_put_chars() is already doing the same thing, albeit in a
slightly different form. Instead of getting the status bits put into the
condition codes and looping with bne or bcs it will read the register,
and it with bit 29 or bit 28 to see if it should wait and then continue
with the writing/reading. I think you can just drop the looping for the
v7 version of the functions and have this driver work on v6 and v7.
Alternatively, you can make some function that says tx buffer is empty,
rx buffer is full or something but I don't see how saving a couple
