pwm: rcar: improve calculation of divider
Hello,
while looking at the driver I noticed another patch opportunity: In
rcar_pwm_get_clock_division() there is a loop:
for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
(1 << div);
do_div(max, clk_rate);
if (period_ns <= max)
break;
}
The value of div should be calculatable without a loop. Something like:
divider = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
tmp = (unsigned long long)period_ns * clk_rate + (divider - 1);
do_div(tmp, divider);
div = ilog2(tmp - 1) + 1;
You might want to check if my maths are right, I didn't test.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH 2/5] pwm: rcar: Add support "atomic" API
Hello, On Fri, Dec 07, 2018 at 10:57:48AM +0100, Geert Uytterhoeven wrote: > > Is the documentation for this hardware publically available? > > Please check Section 59 of the "User's Manual: Hardware" at > https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html Thanks. So the ugly things here are: a) if the pwm is stopped (PWMCR.ENO = 0) the output is set to high after completion of the currently running period. b) setting a duty_cycle of 0 is forbidden both are bad. A workaround for both is implementation of something similar to the switch to a gpio suggested by Michal for imx. But this cannot be done reliably because the current period's end isn't observable. Alternatively a confirmation from the Renesas engineers that PWMCNT.PHO can be set to 0 with the intended effect despite being forbidden in the reference manual would be great. Did someone with access to such hardware test what happens if the PHO field is set to 0? Maybe the forbidden value is just a wrong copy from the CYCO field? I think it would be a good idea to add the link to the documentation into a comment at the top of the driver. @Thierry: Given that nobody seems to have an overview about the features and ugly implementation details of all the PWMs, what about documenting them in the driver files in a greppable way. For the rcar driver something like: - duty-counter-bits: 10 - period-counter-bits: 10 - hardware-polarity-support: false - uglyness: - OUTPUT-ACTIVE-ON-DISABLE - NO-ZERO-DUTY-CYCLE Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH 5/5] pwm: rcar: add workaround to output "pseudo" low level
On Fri, Dec 07, 2018 at 05:29:33PM +0900, Yoshihiro Shimoda wrote:
> This PWM Timer cannot output low because setting 0x000 is prohibited
> on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> the prohibited, this patch adds a workaround function to change
> the value from 0 to 1 as pseudo low level.
>
> Signed-off-by: Yoshihiro Shimoda
> ---
> drivers/pwm/pwm-rcar.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index e479b6a..888cb37 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -166,6 +166,20 @@ static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
> rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
> }
>
> +static void rcar_pwm_workaround_output_low(struct rcar_pwm_chip *rp)
> +{
> + /*
> + * This PWM Timer cannot output low because setting 0x000 is
> + * prohibited on PWMCNT.PH0 (High-Level Period) bitfields. So, avoiding
> + * the prohibited, this function changes the value from 0 to 1 as
> + * pseudo low level.
> + *
> + * TODO: Add GPIO handling to output low level.
> + */
> + if ((rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> + rp->pwmcnt |= 1;
In my eyes this is too broken to do. Not sure I have the complete
picture, but given a small period (say 2) this 1 cycle might result in
50 % duty cycle. Depending on how the hardware behaves if you disable
it, better do this instead.
Are you aware of the series adding such gpio support to the imx driver?
@Thierry: So there are three drivers now that could benefit for a
generic approach.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH 2/5] pwm: rcar: Add support "atomic" API
Hello,
On Fri, Dec 07, 2018 at 05:29:30PM +0900, Yoshihiro Shimoda wrote:
> This patch adds support for "atomic" API. Behavior is the same as
> when using legacy APIs.
>
> Signed-off-by: Yoshihiro Shimoda
> ---
> drivers/pwm/pwm-rcar.c | 32
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index 9cf4567..a5ea0f3 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -200,12 +200,44 @@ static void rcar_pwm_disable(struct pwm_chip *chip,
> struct pwm_device *pwm)
> rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
> }
>
> +static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> + int div, ret;
> +
> + div = rcar_pwm_get_clock_division(rp, state->period);
> + if (div < 0)
> + return div;
> +
> + rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
> +
> + rcar_pwm_calc_counter(rp, div, state->duty_cycle, state->period);
> + ret = rcar_pwm_set_counter(rp);
> + if (!ret)
> + rcar_pwm_set_clock_control(rp, div);
> +
> + /* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */
> + rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
> +
> + if (!ret && state->enabled)
> + ret = rcar_pwm_enable(chip, pwm);
> +
> + if (!state->enabled) {
> + rcar_pwm_disable(chip, pwm);
> + ret = 0;
> + }
> +
> + return ret;
state->polarity isn't used here which is a bug I think.
Is the documentation for this hardware publically available?
If the pwm runs at say 30% duty cycle and then pwm_apply is called with
.period = 1000,
.duty_cycle = 600,
.enabled = false,
can it happen that for some time a duty cycle of 60% is provided? If so,
that's a bug.
Out of interest: How does the output behave if you disable the hardware?
Does it give a 0, or high-z?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH 1/5] pwm: rcar: add rcar_pwm_calc_counter() to calculate PWMCNT value only
Hello,
On Fri, Dec 07, 2018 at 05:29:29PM +0900, Yoshihiro Shimoda wrote:
> -static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int
> duty_ns,
> - int period_ns)
> +static void rcar_pwm_calc_counter(struct rcar_pwm_chip *rp, int div,
> + int duty_ns, int period_ns)
> {
> unsigned long long one_cycle, tmp; /* 0.01 nanoseconds */
> unsigned long clk_rate = clk_get_rate(rp->clk);
> @@ -120,11 +121,17 @@ static int rcar_pwm_set_counter(struct rcar_pwm_chip
> *rp, int div, int duty_ns,
> do_div(tmp, one_cycle);
> ph = tmp & RCAR_PWMCNT_PH0_MASK;
>
> + rp->pwmcnt = cyc | ph;
Wouldn't it be more natural to let rcar_pwm_calc_counter return cyc | ph
and then ...
> +}
> +
> +static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp)
> +{
> /* Avoid prohibited setting */
> - if (cyc == 0 || ph == 0)
> + if ((rp->pwmcnt & RCAR_PWMCNT_CYC0_MASK) == 0 ||
> + (rp->pwmcnt & RCAR_PWMCNT_PH0_MASK) == 0)
> return -EINVAL;
>
> - rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
> + rcar_pwm_write(rp, rp->pwmcnt, RCAR_PWMCNT);
>
> return 0;
> }
> @@ -159,7 +166,8 @@ static int rcar_pwm_config(struct pwm_chip *chip, struct
> pwm_device *pwm,
>
> rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
>
> - ret = rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
> + rcar_pwm_calc_counter(rp, div, duty_ns, period_ns);
> + ret = rcar_pwm_set_counter(rp);
> if (!ret)
> rcar_pwm_set_clock_control(rp, div);
... pass this value to rcar_pwm_set_counter as u32. (Or pass div,
duty_ns and period_ns to rcar_pwm_set_counter and let this then call
rcar_pwm_calc_counter. Then you don't need a new member in the struct
rcar_pwm_chip.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH v6 1/6] clk: Add of_clk_get_by_name_optional() function
Hello Phil, On Mon, Nov 19, 2018 at 12:53:46PM +, Phil Edworthy wrote: > On 19 November 2018 10:46 Uwe Kleine-König wrote: > > On Mon, Nov 19, 2018 at 10:41:42AM +, Phil Edworthy wrote: > > > btw, do we need to add of_clk_get_by_name_optional()? I only added it > > > as a counterpart to of_clk_get_by_name(), but it may not be needed. > > > > I don't need it. Given that it is easy to add when someone has a need, I'd > > say, > > skip it for now. > > I'm wondering if we actually need clk_get_optional(). For me at least, I just > want devm_clk_get_optional(). That would get rid of the arch patches. Given that clk_get_optional will be that simple, it can live in linux/clk.h for all implementors of the clk API, then you don't have to care about different archs. (Unless I'm missing something.) I don't think it's a good idea to drop clk_get_optional even if you'd have to provide arch-specific stuff. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH v6 1/6] clk: Add of_clk_get_by_name_optional() function
Hello Phil,
On Mon, Nov 19, 2018 at 10:41:42AM +, Phil Edworthy wrote:
> On 16 November 2018 16:11 Uwe Kleine-König wrote:
> > On Fri, Nov 16, 2018 at 05:01:28PM +0100, Uwe Kleine-König wrote:
> > > Other than that I think the patch is fine
> >
> > Thinking again, I wonder why not just do:
> >
> > static inline struct clk *clk_get_optional(struct device *dev, const char
> > *id) {
> > struct clk *c = clk_get(dev, id);
> >
> > if (c == ERR_PTR(-ENOENT))
> > return NULL;
> > else
> > return c;
> > }
>
> Unfortunately, underneath this __of_clk_get_by_name() returns -EINVAL
> when looking for a named clock, and the "clock-names" OF property can't
> be found or the name is not in that prop. This is because the index
> returned by of_property_match_string() will be an error code and is then
> currently always passed to __of_clk_get().
>
> If, as you said, I split the patches into one that fixes the error code, and
> then
> adds clk_get_optional() like above, it will make more sense.
Sounds like a good plan.
> btw, do we need to add of_clk_get_by_name_optional()? I only added it as a
> counterpart to of_clk_get_by_name(), but it may not be needed.
I don't need it. Given that it is easy to add when someone has a need,
I'd say, skip it for now.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH 53/61] tty: serial: simplify getting .drvdata
Hello Wolfram, On Thu, Apr 19, 2018 at 04:06:23PM +0200, Wolfram Sang wrote: > We should get drvdata from struct device directly. Going via > platform_device is an unneeded step back and forth. > > Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com> > --- > > Build tested only. buildbot is happy. Please apply individually. > > drivers/tty/serial/imx.c | 18 ++---- for serial/imx.c: Acked-by: Uwe Kleine-König <u.kleine-koe...@pengutronix.de> Thanks Wolfram for going through this Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH v2 3/9] serial: imx: Fix out-of-bounds access through serial port index
On Fri, Feb 23, 2018 at 02:38:31PM +0100, Geert Uytterhoeven wrote:
> The imx_ports[] array is indexed using a value derived from the
> "serialN" alias in DT, or from platform data, which may lead to an
> out-of-bounds access.
>
> Fix this by adding a range check.
>
> Fixes: ff05967a07225ab6 ("serial/imx: add of_alias_get_id() reference back")
> Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>
Reviewed-by: Uwe Kleine-König <u.kleine-koe...@pengutronix.de>
Thanks for your time
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH 3/9] serial: imx: Fix out-of-bounds access through DT alias
Hello Geert,
On Tue, Feb 20, 2018 at 10:40:18AM +0100, Geert Uytterhoeven wrote:
> The imx_ports[] array is indexed using a value derived from the
> "serialN" alias in DT, which may lead to an out-of-bounds access.
>
> Fix this by adding a range check.
>
> Fixes: 9206ab8a0350c3da ("serial: imx: Fix out-of-bounds access through DT
> alias")
huh, this patch fixes itself?
> Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>
> ---
> drivers/tty/serial/imx.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 1d7ca382bc12b238..e89e90ad87d8245c 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -2041,6 +2041,11 @@ static int serial_imx_probe(struct platform_device
> *pdev)
> serial_imx_probe_pdata(sport, pdev);
> else if (ret < 0)
> return ret;
I'd prefer an empty line here.
> + if (sport->port.line >= UART_NR) {
I would have used:
if (sport->port.line >= ARRAY_SIZE(imx_ports))
which IMHO is better understandable
> + dev_err(>dev, "serial%d out of range\n",
> + sport->port.line);
Note that the same overflow can happen when a device is probed using
platform data (and your commit fixes that, too). Maybe worth to point
out in the commit log?
Other than that: Good catch, thanks for your patch.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH] ARM: Convert to using %pOF instead of full_name
On Tue, Jul 18, 2017 at 04:42:41PM -0500, Rob Herring wrote:
> Now that we have a custom printf format specifier, convert users of
> full_name to use %pOF instead. This is preparation to remove storing
Oh nice. If the commit adding %pOF is already set in stone, i'd suggest
to mention the commit id here in the commit log.
> of the full path string for each node.
>
> Signed-off-by: Rob Herring <r...@kernel.org>
> Cc: Russell King <li...@armlinux.org.uk>
> Cc: Kukjin Kim <kg...@kernel.org>
> Cc: Krzysztof Kozlowski <k...@kernel.org>
> Cc: Javier Martinez Canillas <jav...@osg.samsung.com>
> Cc: Shawn Guo <shawn...@kernel.org>
> Cc: Sascha Hauer <ker...@pengutronix.de>
> Cc: Fabio Estevam <fabio.este...@nxp.com>
> Cc: Jason Cooper <ja...@lakedaemon.net>
> Cc: Andrew Lunn <and...@lunn.ch>
> Cc: Gregory Clement <gregory.clem...@free-electrons.com>
> Cc: Sebastian Hesselbarth <sebastian.hesselba...@gmail.com>
> Cc: Tony Lindgren <t...@atomide.com>
> Cc: "Benoît Cousson" <bcous...@baylibre.com>
> Cc: Paul Walmsley <p...@pwsan.com>
> Cc: Heiko Stuebner <he...@sntech.de>
> Cc: Simon Horman <ho...@verge.net.au>
> Cc: Magnus Damm <magnus.d...@gmail.com>
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: linux-o...@vger.kernel.org
> Cc: linux-rockc...@lists.infradead.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> arch/arm/kernel/cpuidle.c| 4 ++--
> arch/arm/kernel/devtree.c| 6 +++---
> arch/arm/kernel/topology.c | 4 ++--
> arch/arm/mach-exynos/suspend.c | 8
> arch/arm/mach-imx/gpc.c | 4 ++--
> arch/arm/mach-mvebu/kirkwood.c | 4 ++--
> arch/arm/mach-omap2/omap-wakeupgen.c | 4 ++--
> arch/arm/mach-omap2/omap_hwmod.c | 4 ++--
> arch/arm/mach-rockchip/platsmp.c | 4 ++--
> arch/arm/mach-shmobile/pm-rmobile.c | 8
> 10 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> index a3308ad1a024..fda5579123a8 100644
> --- a/arch/arm/kernel/cpuidle.c
> +++ b/arch/arm/kernel/cpuidle.c
> @@ -101,8 +101,8 @@ static int __init arm_cpuidle_read_ops(struct device_node
> *dn, int cpu)
>
> ops = arm_cpuidle_get_ops(enable_method);
> if (!ops) {
> - pr_warn("%s: unsupported enable-method property: %s\n",
> - dn->full_name, enable_method);
> + pr_warn("%pOF: unsupported enable-method property: %s\n",
> + dn, enable_method);
> return -EOPNOTSUPP;
> }
>
> diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
> index f676febbb270..28174c9a94ac 100644
> --- a/arch/arm/kernel/devtree.c
> +++ b/arch/arm/kernel/devtree.c
> @@ -95,7 +95,7 @@ void __init arm_dt_init_cpu_maps(void)
> if (of_node_cmp(cpu->type, "cpu"))
> continue;
>
> - pr_debug(" * %s...\n", cpu->full_name);
> + pr_debug(" * %pOF...\n", cpu);
> /*
>* A device tree containing CPU nodes with missing "reg"
>* properties is considered invalid to build the
> @@ -103,8 +103,8 @@ void __init arm_dt_init_cpu_maps(void)
>*/
> cell = of_get_property(cpu, "reg", _bytes);
> if (!cell || prop_bytes < sizeof(*cell)) {
> - pr_debug(" * %s missing reg property\n",
> - cpu->full_name);
> + pr_debug(" * %pOF missing reg property\n",
> + cpu);
You could join lines here and in the next hunk. And there are two more
further down.
> of_node_put(cpu);
> return;
> }
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index bf949a763dbe..e596c5b8f931 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -127,8 +127,8 @@ static void __init parse_dt_topology(void)
>
> rate = of_get_property(cn, "clock-frequency", );
> if (!rate || len != 4) {
> - pr_err("%s missing clock-frequency property\n",
> - cn->full_name);
> + pr_err("%pOF missing clock-frequency property\n",
> + cn);
> continue;
> }
>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: Clocks used by another OS/CPU (was: Re: [RFC PATCH] clk: renesas: cpg-mssr: Add interface for critical core clocks)
Hello Dirk, On Sun, Jul 02, 2017 at 07:48:41AM +0200, Dirk Behme wrote: > > On my machine (Udoo Neo, A9 + M4) the A9 is the primary CPU that is > > started by the bootrom. If I want the M4 being the primary device I'd > > need support in the bootloader to wait long enough (i.e. until the M4 is > > up) before letting the A9 jump into Linux. Managable I'd say. This way > > would even make sense if the M4 runs a rt critical OS that shouldn't be > > forced to wait on the non-rt A9 to enable a clk. > > > Overall, assuming that the issue we are discussing here can be solved quite > easily in hardware (a set of clock registers for each CPU/OS domain, > connected cleverly to effectively control each clock, with access protection > for each set of registers) I tend to think that for a SoC supposed to run > different OS on different cores this is a missing hardware feature (bug?). So you want to enable bits for your CAN clock, one in each cpu's domain. I'd say that is a nice idea that a hardware engineer might be proud to pick up but that results in more headache than fun for the software colleague. There are several problems that come immediately to mind: - You can switch of a clk because you don't need it on, or because you need it off. I guess you want to have the clock on if at least one cpu wants it on. So you take away the freedom from the other cpu to force the clock off. (Yeah, the currently available clk framework doesn't allow that either.) - What if cpu 0 sets the parent of the can clk to pll2 but cpu 1 wants it set to pll1? How does cpu 1 notice the change? - On off might be relatively easy, what about clk dividers? cpu 0 sets 2 which cpu 1 sets 6. That convinces me that the disadvantages of having two views on the clk core have more weight and you really want a single view and share that by software. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: Clocks used by another OS/CPU (was: Re: [RFC PATCH] clk: renesas: cpg-mssr: Add interface for critical core clocks)
Hello,
On Sat, Jul 01, 2017 at 07:02:48AM +0200, Dirk Behme wrote:
> On 30.06.2017 22:24, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Fri, Jun 30, 2017 at 10:58:26AM -0500, Rob Herring wrote:
> > > > TL;DR: Clocks may be in use by another CPU not running Linux, while
> > > > Linux
> > > > disables them as being unused.
> >
> > not long ago I thought with a few colleagues about this. The scenario is
> > to start a Linux kernel on a Cortex-M companion to a Cortex-A.
> >
> > > > On Mon, Jun 26, 2017 at 1:30 PM, Dirk Behme <dirk.be...@de.bosch.com>
> > > > wrote:
> > > > > With commit 72f5df2c2bbb6 ("clk: renesas: cpg-mssr: Migrate to
> > > > > CLK_IS_CRITICAL") we are able to handle critical module clocks.
> > > > > Introduce the same logic for critical core clocks.
> > > > >
> > > > > Signed-off-by: Dirk Behme <dirk.be...@de.bosch.com>
> > > > > ---
> > > > > Commit
> > > > >
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/renesas?id=72f5df2c2bbb66d4a555cb51eb9f412abf1af77f
> > > > >
> > > > > is quite nice to avoid *module* clocks being disabled. Unfortunately,
> > > > > there are *core* clocks, too. E.g. using an other OS on the Cortex R7
> > > > > core of the r8a7795, the 'canfd' is a quite popular core clock which
> > > > > shouldn't be disabled by Linux.
> > > > >
> > > > > Therefore, this patch is a proposal to use the same 'mark clocks as
> > > > > critical' logic implemented for the module clocks for the core
> > > > > clocks, too.
> > > > >
> > > > > Opinions?
> > > >
> > > > On r8a7795, there are several Cortex A cores running Linux, and a
> > > > Cortex R7
> > > > core which may run another OS.
> > > > This is an interesting issue, and relevant to other SoCs, too.
> > > >
> > > > In this particular case, the "canfd" clock is a core clock used as an
> > > > auxiliary clock for the CAN0, CAN1, and CANFD interfaces. This can lead
> > > > to three scenarios:
> > > >1. Linux controls all CAN interfaces
> > > > => no issue,
> > > >2. The OS on the RT CPU controls all CAN interfaces
> > > > => issue, Linux disables the clock
> > > >3. Mix of 1 and 2
> > > > => More issues.
> > > > Of course this is not limited to clocks, but also to e.g. PM domains.
> > > >
> > > > How can this be handled?
> > > > I believe just marking the "canfd" clock critical is not the right
> > > > solution,
> > > > as about any clock could be used by the RT CPU.
> > > >
> > > > Still, Linux needs to be made aware that devices (clocks and PM
> > > > domains) are
> > > > controlled by another CPU/OS.
> > > >
> > > > Should this be described in DT? It feels like software policy to me.
> > >
> > > No, it shouldn't. It is Linux policy to disable all unused clocks, so
> > > Linux gets to deal with the consequences.
> >
> > The ideal solution I imagine is to make the other CPU's OS a consumer of
> > the Linux clock driver. This would require a generic device driver on the
> > companion CPU that forwards clk requests via inter-cpu communication to
> > the Linux clk driver. It could be feed with the necessary information by
> > the rproc glue. So when the companion cpu is supposed to care for the
> > can0 device, the steps that should happen are:
> >
> > - make sure can0 isn't occupied by the Linux Host
> > - reroute the can irq to the companion cpu (if necessary)
> > - create a dtb containing something like this for the companion CPU:
> >
> > clks: virtclk {
> > compatible = ???
> > #clock-cells = <1>;
> > ...
> > };
> >
> > can@$address {
> > compatible = ...
> > regs = ...
> > clocks = < 3>;
> > clock-names = ...
> > ...
> > };
> >
> > where the driver binding to the virtclk device just forwards clk
> > requests to the Linux host side which then knows that clk 3 is the
> > can clock and does the necessary stuff.
> >
> &g
Re: Clocks used by another OS/CPU (was: Re: [RFC PATCH] clk: renesas: cpg-mssr: Add interface for critical core clocks)
Hello,
On Fri, Jun 30, 2017 at 10:58:26AM -0500, Rob Herring wrote:
> > TL;DR: Clocks may be in use by another CPU not running Linux, while Linux
> > disables them as being unused.
not long ago I thought with a few colleagues about this. The scenario is
to start a Linux kernel on a Cortex-M companion to a Cortex-A.
> > On Mon, Jun 26, 2017 at 1:30 PM, Dirk Behme <dirk.be...@de.bosch.com> wrote:
> >> With commit 72f5df2c2bbb6 ("clk: renesas: cpg-mssr: Migrate to
> >> CLK_IS_CRITICAL") we are able to handle critical module clocks.
> >> Introduce the same logic for critical core clocks.
> >>
> >> Signed-off-by: Dirk Behme <dirk.be...@de.bosch.com>
> >> ---
> >> Commit
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/renesas?id=72f5df2c2bbb66d4a555cb51eb9f412abf1af77f
> >>
> >> is quite nice to avoid *module* clocks being disabled. Unfortunately,
> >> there are *core* clocks, too. E.g. using an other OS on the Cortex R7
> >> core of the r8a7795, the 'canfd' is a quite popular core clock which
> >> shouldn't be disabled by Linux.
> >>
> >> Therefore, this patch is a proposal to use the same 'mark clocks as
> >> critical' logic implemented for the module clocks for the core
> >> clocks, too.
> >>
> >> Opinions?
> >
> > On r8a7795, there are several Cortex A cores running Linux, and a Cortex R7
> > core which may run another OS.
> > This is an interesting issue, and relevant to other SoCs, too.
> >
> > In this particular case, the "canfd" clock is a core clock used as an
> > auxiliary clock for the CAN0, CAN1, and CANFD interfaces. This can lead
> > to three scenarios:
> > 1. Linux controls all CAN interfaces
> > => no issue,
> > 2. The OS on the RT CPU controls all CAN interfaces
> > => issue, Linux disables the clock
> > 3. Mix of 1 and 2
> > => More issues.
> > Of course this is not limited to clocks, but also to e.g. PM domains.
> >
> > How can this be handled?
> > I believe just marking the "canfd" clock critical is not the right solution,
> > as about any clock could be used by the RT CPU.
> >
> > Still, Linux needs to be made aware that devices (clocks and PM domains) are
> > controlled by another CPU/OS.
> >
> > Should this be described in DT? It feels like software policy to me.
>
> No, it shouldn't. It is Linux policy to disable all unused clocks, so
> Linux gets to deal with the consequences.
The ideal solution I imagine is to make the other CPU's OS a consumer of
the Linux clock driver. This would require a generic device driver on the
companion CPU that forwards clk requests via inter-cpu communication to
the Linux clk driver. It could be feed with the necessary information by
the rproc glue. So when the companion cpu is supposed to care for the
can0 device, the steps that should happen are:
- make sure can0 isn't occupied by the Linux Host
- reroute the can irq to the companion cpu (if necessary)
- create a dtb containing something like this for the companion CPU:
clks: virtclk {
compatible = ???
#clock-cells = <1>;
...
};
can@$address {
compatible = ...
regs = ...
clocks = < 3>;
clock-names = ...
...
};
where the driver binding to the virtclk device just forwards clk
requests to the Linux host side which then knows that clk 3 is the
can clock and does the necessary stuff.
This way the can clock doesn't need special handling in the host's dtb
and no clock necessary for the companion is disabled as unused because
it is requested and enabled.
The only problem I see is that implementing such a driver/protocol
probably is time consuming.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [i2c-tools PATCH v2] i2ctransfer: add new tool
Hello, On Mon, Mar 13, 2017 at 12:01:40PM +0100, Wolfram Sang wrote: > Hi Uwe, > > thanks for the review! > > > > +.RI [ data ] > > > +.RI [ desc > > > +.RI [ data ]] > > > > You could join the previous two lines. > > Try it. You will miss some spaces, then. It works fine with quoting: .RI [ "desc " [ data "]] ..." . But ok, it's at least arguable if this is better than your solution. > > > +Also, you cannot be interrupted by another I2C master during one > > > transfer, but it might happen between multiple transfers. > > > > Well, unless you loose arbitration. Maybe this is too picky to be > > relevant here? > > I wonder: will another I2C master start a transfer on a repeated start? > Need to investigate. I'd say it must not. It should have logic to detect bus busy and delay any transfers until the bus becomes idle. With a repeated start the bus doesn't become idle between two transfers. > > > + for (i = 0; i <= nmsgs; i++) > > > + free(msgs[i].buf); > > > + > > > + exit(1); > > > > return EXIT_FAILURE; ? > > > > Apart from the exit code this is exactly the trailer of the good path, > > so these could share code. > > No! One has '< nmsgs', the other one '<= nmsgs'. Friendly rant: It was > all easier and less subtle before Jean wanted the 'don't rely on the OS > for cleanup' additions ;) ah, ok. BTW, I like tools that clean up after themselves. This way debugging is much easier if you look for lost memory. And yes, this doesn't matter much for short-living programs like i2c-tools, but I like being consistent here and also do the cleanup for this this type of program. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [i2c-tools PATCH v2] i2ctransfer: add new tool
print_msgs(msgs, nmsgs, PRINT_STDERR | PRINT_HEADER | PRINT_WRITE_BUF);
> +
> + fprintf(stderr, "Continue? [y/N] ");
> + fflush(stderr);
> + if (!user_ack(0)) {
> + fprintf(stderr, "Aborting on user request.\n");
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + char filename[20];
> + int i2cbus, address = -1, file, arg_idx = 1, nmsgs = 0, nmsgs_sent, i;
> + int force = 0, yes = 0, version = 0, verbose = 0;
> + struct i2c_msg msgs[I2C_RDRW_IOCTL_MAX_MSGS];
Should this limit be described in the man page?
> + switch (state) {
> + case PARSE_GET_DESC:
> + flags = 0;
> +
> + switch (*arg_ptr++) {
> + case 'r': flags |= I2C_M_RD; break;
This doesn't match kernel coding style and I'd put it on separate lines.
> + case 'w': break;
> +[...]
> + close(file);
> +
> + for (i = 0; i < nmsgs; i++)
> + free(msgs[i].buf);
> +
> + exit(0);
return EXIT_SUCCESS; ?
> +
> + err_out_with_arg:
> + fprintf(stderr, "Error: faulty argument is '%s'\n", argv[arg_idx]);
> + err_out:
> + close(file);
> +
> + for (i = 0; i <= nmsgs; i++)
> + free(msgs[i].buf);
> +
> + exit(1);
return EXIT_FAILURE; ?
Apart from the exit code this is exactly the trailer of the good path,
so these could share code.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: Regression introduced by "net: ipconfig: Support using "delayed" DHCP replies"
Hello Geert,
On Tue, Aug 09, 2016 at 12:02:44PM +0200, Geert Uytterhoeven wrote:
> Hi Uwe, David,
>
> On current net-next, I see the following corruption during DHCP on
> r8a7791/koelsch, which uses the sh_eth driver:
>
> Sending DHCP requests ., OK
> IP-Config: Got DHCP answer from 192.168.97.254, my address is
> 192.168.97.28
> IP-Config: Complete:
> - device=eth0, hwaddr=2e:09:0a:00:6d:85, ipaddr=192.168.97.28,
> mask=255.255.255.0, gw=192.168.97.254
> + device=^M\xffc0\xffa0\xffe1,
> hwaddr=0e:9e:45:ac:dd:b4:3a:88:95:e1:50:af:0c:44:92:c0:6c:46:9f:02:34:1e:58:21:cd:c3:40:0c:ed:80:b1:74:10:0c:04:31:06:9f:04:01:04:93:5c:51:83:18:46:09,
> ipaddr=192.168.97.28, mask=255.255.255.0, gw=192.168.97.254
I fail to see the reason from looking at the patch.
Is this reproducible? Can you please enable pr_debug output and provide
the corresponding output? What is your kernel command line? Are there >1
eth devices?
I will try to reproduce and fix later today.
> It may also crash in ip_auto_config() with e.g. "Unable to handle kernel NULL
> pointer dereference at virtual address 017d".
>
> I've bisected this to commit 2647cffb2bc6fbed163d377390eb7ca552c7c1cb
> ('net: ipconfig: Support using "delayed" DHCP replies').
> Unfortunately just reverting that commit on current net-next is not a
> solution, as
> that may lead to DHCP failures:
>
> DHCP/BOOTP: Ignoring delayed packet
> timed out!
> [...]
> IP-Config: Retrying forever (NFS root)...
>
> Reverting also commit e068853409aa1720 ("net: ipconfig: drop inter-device
> timeout") fixes that, though.
That's not surprising. e068853409aa1720 was only possible because of
2647cffb2bc6fbed163d377390eb7ca552c7c1cb.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
