* Re: [v5 1/2] dt-bindings: gpio: aspeed: Add SGPIO support
From: Linus Walleij @ 2019-07-20 8:12 UTC (permalink / raw)
To: Hongwei Zhang
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-aspeed, Bartosz Golaszewski, Andrew Jeffery,
linux-kernel@vger.kernel.org, open list:GPIO SUBSYSTEM,
Rob Herring, Joel Stanley, Linux ARM
In-Reply-To: <1563564291-9692-2-git-send-email-hongweiz@ami.com>
Hi Hongwei,
after looking close at the driver and bindings I have this feeback:
On Fri, Jul 19, 2019 at 9:25 PM Hongwei Zhang <hongweiz@ami.com> wrote:
+- reg : Address and length of the register set for the device
This 0x100 range may look simple but in the driver it looks like
this:
+static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
+ {
+ .val_regs = 0x0000,
+ .rdata_reg = 0x0070,
+ .irq_regs = 0x0004,
+ .names = { "A", "B", "C", "D" },
+ },
+ {
+ .val_regs = 0x001C,
+ .rdata_reg = 0x0074,
+ .irq_regs = 0x0020,
+ .names = { "E", "F", "G", "H" },
+ },
+ {
+ .val_regs = 0x0038,
+ .rdata_reg = 0x0078,
+ .irq_regs = 0x003C,
+ .names = { "I", "J" },
+ },
+};
So first break this into up to 10 different instances with one device
per bank instead.
For each device:
reg = <0x1e780200 4>, <x1e780204 ..>, <0x1e780270 ..>;
reg-names = "val", "irq", "rdata";
This way you use the device tree features for locating the
register offsets instead of hard-coding it into the driver,
which will make the driver simpler.
> +- ngpios : number of GPIO pins to serialise.
> + (should be multiple of 8, up to 80 pins)
This is wrong. This should be split into 10 different devices providing
8 GPIO lines each. The "ngpios" is not intended to subdivide
banks, but for the case where you use say bits 0..11 of 32 bits in
a register by setting ngpios to 12.
I see that they use the same clock divider and the same interrupt,
but this is better to partition into a separate clock divider driver
and up to 10 instances of GPIO devices with 8 GPIOs each.
I also see that they use the same interrupt. This is fine, in your
driver just grab a shared IRQ and all the IRQ handlers will be
traversed. This is a well known pattern for all operating systems.
> +- clocks : A phandle to the APB clock for SGPM clock division
> +
> +- bus-frequency : SGPM CLK frequency
I see that there is a common clock control register in offset 0x54.
Split this out to a separate clock driver that provides a divided clock
that all GPIO blocks can get, no matter if you use 1, 2 .. 10 of these
blocks.
The fact that these GPIO banks and the clock register is in the same
memory range does not really matter. Split up the memory range in
on reg = per GPIO chip and one reg for the clock.
> + Example:
> + sgpio: sgpio@1e780200 {
> + #gpio-cells = <2>;
> + compatible = "aspeed,ast2500-sgpio";
> + gpio-controller;
> + interrupts = <40>;
> + reg = <0x1e780200 0x0100>;
> + clocks = <&syscon ASPEED_CLK_APB>;
> + interrupt-controller;
> + ngpios = <8>;
> + bus-frequency = <12000000>;
> + };
Splitting this up into a clock controller and 1-10 instances of sgpio
will make things simpler, because it will closer reflect what the hardware
people are doing: they have just created 10 instances of the same
block, and added a clock divider.
You can put the 1-10 instances and the clock divider inside a collected
node "simple-bus" in the device tree:
{
compatible = "simple-bus";
sgpio0: sgpio {
compatible = "aspeed,ast2500-sgpio";
reg = <0x1e780200 ...> ...;
clk = <&sgpioclk>;
};
sgpio1: sgpio {
...
};
...
sgpioclk: clock {
compatible = "aspeed,sgpio-clock";
reg = 0x1e780254 4>;
};
};
This is a better fit with the actual hardware and will make it much
easier to write drivers.
Admittedly DT device partitioning of SoC devices is a grey area,
but here I think the DT infrastructure helps you to break things
down and make it easier, I am thinking in a divide-and-conquer
way about it.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v6 11/22] clk: sunxi-ng: a64: Add minimum rate for PLL_MIPI
From: Jagan Teki @ 2019-07-20 7:34 UTC (permalink / raw)
To: Maxime Ripard, Michael Nazzareno Trimarchi
Cc: David Airlie, Daniel Vetter, Chen-Yu Tsai, Michael Turquette,
Rob Herring, Mark Rutland, linux-arm-kernel, linux-kernel,
linux-clk, dri-devel, devicetree, linux-amarula, linux-sunxi
In-Reply-To: <CAMty3ZDE1xiNgHVLihH378dY5szzkr14V-fwLZdvPs12tY+G1A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Sat, Jul 20, 2019 at 12:46 PM Jagan Teki <jagan-dyjBcgdgk7Pe9wHmmfpqLFaTQe2KTcn/@public.gmane.org> wrote:
>
> On Sat, Jul 20, 2019 at 12:28 PM Maxime Ripard
> <maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> wrote:
> >
> > On Thu, Jul 11, 2019 at 07:43:16PM +0200, Michael Nazzareno Trimarchi wrote:
> > > > > tcon-pixel clock is the rate that you want to achive on display side
> > > > > and if you have 4 lanes 32bit or lanes and different bit number that
> > > > > you need to have a clock that is able to put outside bits and speed
> > > > > equal to pixel-clock * bits / lanes. so If you want a pixel-clock of
> > > > > 40 mhz and you have 32bits and 4 lanes you need to have a clock of
> > > > > 40 * 32 / 4 in no-burst mode. I think that this is done but most of
> > > > > the display.
> > > >
> > > > So this is what the issue is then?
> > > >
> > > > This one does make sense, and you should just change the rate in the
> > > > call to clk_set_rate in sun4i_tcon0_mode_set_cpu.
> > > >
> > > > I'm still wondering why that hasn't been brought up in either the
> > > > discussion or the commit log before though.
> > > >
> > > Something like this?
> > >
> > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +++++++++++---------
> > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 --
> > > 2 files changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > index 64c43ee6bd92..42560d5c327c 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > @@ -263,10 +263,11 @@ static int sun4i_tcon_get_clk_delay(const struct
> > > drm_display_mode *mode,
> > > }
> > >
> > > static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon,
> > > - const struct drm_display_mode *mode)
> > > + const struct drm_display_mode *mode,
> > > + u32 tcon_mul)
> > > {
> > > /* Configure the dot clock */
> > > - clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
> > > + clk_set_rate(tcon->dclk, mode->crtc_clock * tcon_mul * 1000);
> > >
> > > /* Set the resolution */
> > > regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,
> > > @@ -335,12 +336,13 @@ static void sun4i_tcon0_mode_set_cpu(struct
> > > sun4i_tcon *tcon,
> > > u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> > > u8 lanes = device->lanes;
> > > u32 block_space, start_delay;
> > > - u32 tcon_div;
> > > + u32 tcon_div, tcon_mul;
> > >
> > > - tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
> > > - tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
> > > + tcon->dclk_min_div = 4;
> > > + tcon->dclk_max_div = 127;
> > >
> > > - sun4i_tcon0_mode_set_common(tcon, mode);
> > > + tcon_mul = bpp / lanes;
> > > + sun4i_tcon0_mode_set_common(tcon, mode, tcon_mul);
> > >
> > > /* Set dithering if needed */
> > > sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> > > @@ -366,7 +368,7 @@ static void sun4i_tcon0_mode_set_cpu(struct
> > > sun4i_tcon *tcon,
> > > */
> > > regmap_read(tcon->regs, SUN4I_TCON0_DCLK_REG, &tcon_div);
> > > tcon_div &= GENMASK(6, 0);
> > > - block_space = mode->htotal * bpp / (tcon_div * lanes);
> > > + block_space = mode->htotal * tcon_div * tcon_mul;
> > > block_space -= mode->hdisplay + 40;
> > >
> > > regmap_write(tcon->regs, SUN4I_TCON0_CPU_TRI0_REG,
> > > @@ -408,7 +410,7 @@ static void sun4i_tcon0_mode_set_lvds(struct
> > > sun4i_tcon *tcon,
> > >
> > > tcon->dclk_min_div = 7;
> > > tcon->dclk_max_div = 7;
> > > - sun4i_tcon0_mode_set_common(tcon, mode);
> > > + sun4i_tcon0_mode_set_common(tcon, mode, 1);
> > >
> > > /* Set dithering if needed */
> > > sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> > > @@ -487,7 +489,7 @@ static void sun4i_tcon0_mode_set_rgb(struct
> > > sun4i_tcon *tcon,
> > >
> > > tcon->dclk_min_div = 6;
> > > tcon->dclk_max_div = 127;
> > > - sun4i_tcon0_mode_set_common(tcon, mode);
> > > + sun4i_tcon0_mode_set_common(tcon, mode, 1);
> > >
> > > /* Set dithering if needed */
> > > sun4i_tcon0_mode_set_dithering(tcon, connector);
> > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > index 5c3ad5be0690..a07090579f84 100644
> > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > > @@ -13,8 +13,6 @@
> > > #include <drm/drm_encoder.h>
> > > #include <drm/drm_mipi_dsi.h>
> > >
> > > -#define SUN6I_DSI_TCON_DIV 4
> > > -
> > > struct sun6i_dsi {
> > > struct drm_connector connector;
> > > struct drm_encoder encoder;
> >
> > I had more something like this in mind:
> > http://code.bulix.org/nlp5a4-803511
>
> Worth to look at it. was it working on your panel? meanwhile I will check it.
>
> We have updated with below change [1], seems working on but is
> actually checking the each divider as before start with 4... till 127.
>
> This new approach, is start looking the best divider from 4.. based on
> the idea vs rounded it will ended up best divider like [2]
>
> https://gist.github.com/openedev/7e2c33248b372d29be9979e06d483673
> https://gist.github.com/openedev/c72dfffc0ca59e7ec1edcd7ad360cdd1
I made quick check on two possibilities.
1) with Maxime change
https://gist.github.com/openedev/3b0b3d35ced6d89f5be0831f1cc9d840
https://gist.github.com/openedev/dd6a9e528cde80ef0508cb54723f505d
2) with Maxime change along with min 4, max 127 divider values.
The outcome similar like 1)
This look it will depends on divider, need to check further on this page.
Jagan.
^ permalink raw reply
* Re: [PATCH v6 11/22] clk: sunxi-ng: a64: Add minimum rate for PLL_MIPI
From: Michael Nazzareno Trimarchi @ 2019-07-20 7:20 UTC (permalink / raw)
To: Maxime Ripard
Cc: Jagan Teki, David Airlie, Daniel Vetter, Chen-Yu Tsai,
Michael Turquette, Rob Herring, Mark Rutland, linux-arm-kernel,
linux-kernel, linux-clk, dri-devel, devicetree, linux-amarula,
linux-sunxi
In-Reply-To: <20190720065830.zn3txpyduakywcva@flea>
[-- Attachment #1: Type: text/plain, Size: 5551 bytes --]
Hi
On Sat., 20 Jul. 2019, 8:58 am Maxime Ripard, <maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
wrote:
> On Thu, Jul 11, 2019 at 07:43:16PM +0200, Michael Nazzareno Trimarchi
> wrote:
> > > > tcon-pixel clock is the rate that you want to achive on display side
> > > > and if you have 4 lanes 32bit or lanes and different bit number that
> > > > you need to have a clock that is able to put outside bits and speed
> > > > equal to pixel-clock * bits / lanes. so If you want a pixel-clock of
> > > > 40 mhz and you have 32bits and 4 lanes you need to have a clock of
> > > > 40 * 32 / 4 in no-burst mode. I think that this is done but most of
> > > > the display.
> > >
> > > So this is what the issue is then?
> > >
> > > This one does make sense, and you should just change the rate in the
> > > call to clk_set_rate in sun4i_tcon0_mode_set_cpu.
> > >
> > > I'm still wondering why that hasn't been brought up in either the
> > > discussion or the commit log before though.
> > >
> > Something like this?
> >
> > drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +++++++++++---------
> > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 --
> > 2 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index 64c43ee6bd92..42560d5c327c 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -263,10 +263,11 @@ static int sun4i_tcon_get_clk_delay(const struct
> > drm_display_mode *mode,
> > }
> >
> > static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon,
> > - const struct drm_display_mode
> *mode)
> > + const struct drm_display_mode
> *mode,
> > + u32 tcon_mul)
> > {
> > /* Configure the dot clock */
> > - clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
> > + clk_set_rate(tcon->dclk, mode->crtc_clock * tcon_mul * 1000);
> >
> > /* Set the resolution */
> > regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,
> > @@ -335,12 +336,13 @@ static void sun4i_tcon0_mode_set_cpu(struct
> > sun4i_tcon *tcon,
> > u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> > u8 lanes = device->lanes;
> > u32 block_space, start_delay;
> > - u32 tcon_div;
> > + u32 tcon_div, tcon_mul;
> >
> > - tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
> > - tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
> > + tcon->dclk_min_div = 4;
> > + tcon->dclk_max_div = 127;
> >
> > - sun4i_tcon0_mode_set_common(tcon, mode);
> > + tcon_mul = bpp / lanes;
> > + sun4i_tcon0_mode_set_common(tcon, mode, tcon_mul);
> >
> > /* Set dithering if needed */
> > sun4i_tcon0_mode_set_dithering(tcon,
> sun4i_tcon_get_connector(encoder));
> > @@ -366,7 +368,7 @@ static void sun4i_tcon0_mode_set_cpu(struct
> > sun4i_tcon *tcon,
> > */
> > regmap_read(tcon->regs, SUN4I_TCON0_DCLK_REG, &tcon_div);
> > tcon_div &= GENMASK(6, 0);
> > - block_space = mode->htotal * bpp / (tcon_div * lanes);
> > + block_space = mode->htotal * tcon_div * tcon_mul;
> > block_space -= mode->hdisplay + 40;
> >
> > regmap_write(tcon->regs, SUN4I_TCON0_CPU_TRI0_REG,
> > @@ -408,7 +410,7 @@ static void sun4i_tcon0_mode_set_lvds(struct
> > sun4i_tcon *tcon,
> >
> > tcon->dclk_min_div = 7;
> > tcon->dclk_max_div = 7;
> > - sun4i_tcon0_mode_set_common(tcon, mode);
> > + sun4i_tcon0_mode_set_common(tcon, mode, 1);
> >
> > /* Set dithering if needed */
> > sun4i_tcon0_mode_set_dithering(tcon,
> sun4i_tcon_get_connector(encoder));
> > @@ -487,7 +489,7 @@ static void sun4i_tcon0_mode_set_rgb(struct
> > sun4i_tcon *tcon,
> >
> > tcon->dclk_min_div = 6;
> > tcon->dclk_max_div = 127;
> > - sun4i_tcon0_mode_set_common(tcon, mode);
> > + sun4i_tcon0_mode_set_common(tcon, mode, 1);
> >
> > /* Set dithering if needed */
> > sun4i_tcon0_mode_set_dithering(tcon, connector);
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > index 5c3ad5be0690..a07090579f84 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > @@ -13,8 +13,6 @@
> > #include <drm/drm_encoder.h>
> > #include <drm/drm_mipi_dsi.h>
> >
> > -#define SUN6I_DSI_TCON_DIV 4
> > -
> > struct sun6i_dsi {
> > struct drm_connector connector;
> > struct drm_encoder encoder;
>
> I had more something like this in mind:
> http://code.bulix.org/nlp5a4-803511
I sent another patch to jagan and I'm fully agree that the proper change
need to show the reason and not change constraints as we discuss
We are on same line
Thank you
>
>
>
>
>
> You really don't need to change the divider range (or this is another
> issue that the one you mentionned).
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAOf5uwn%3DH3vRQbqMkg7tR4g8BHzdxpXfvjud22F95PQ%2B7CfQhQ%40mail.gmail.com.
[-- Attachment #2: Type: text/html, Size: 7742 bytes --]
^ permalink raw reply
* Re: [PATCH v6 11/22] clk: sunxi-ng: a64: Add minimum rate for PLL_MIPI
From: Jagan Teki @ 2019-07-20 7:16 UTC (permalink / raw)
To: Maxime Ripard
Cc: Michael Nazzareno Trimarchi, David Airlie, Daniel Vetter,
Chen-Yu Tsai, Michael Turquette, Rob Herring, Mark Rutland,
linux-arm-kernel, linux-kernel, linux-clk, dri-devel, devicetree,
linux-amarula, linux-sunxi
In-Reply-To: <20190720065830.zn3txpyduakywcva@flea>
On Sat, Jul 20, 2019 at 12:28 PM Maxime Ripard
<maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> wrote:
>
> On Thu, Jul 11, 2019 at 07:43:16PM +0200, Michael Nazzareno Trimarchi wrote:
> > > > tcon-pixel clock is the rate that you want to achive on display side
> > > > and if you have 4 lanes 32bit or lanes and different bit number that
> > > > you need to have a clock that is able to put outside bits and speed
> > > > equal to pixel-clock * bits / lanes. so If you want a pixel-clock of
> > > > 40 mhz and you have 32bits and 4 lanes you need to have a clock of
> > > > 40 * 32 / 4 in no-burst mode. I think that this is done but most of
> > > > the display.
> > >
> > > So this is what the issue is then?
> > >
> > > This one does make sense, and you should just change the rate in the
> > > call to clk_set_rate in sun4i_tcon0_mode_set_cpu.
> > >
> > > I'm still wondering why that hasn't been brought up in either the
> > > discussion or the commit log before though.
> > >
> > Something like this?
> >
> > drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +++++++++++---------
> > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 --
> > 2 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index 64c43ee6bd92..42560d5c327c 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -263,10 +263,11 @@ static int sun4i_tcon_get_clk_delay(const struct
> > drm_display_mode *mode,
> > }
> >
> > static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon,
> > - const struct drm_display_mode *mode)
> > + const struct drm_display_mode *mode,
> > + u32 tcon_mul)
> > {
> > /* Configure the dot clock */
> > - clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
> > + clk_set_rate(tcon->dclk, mode->crtc_clock * tcon_mul * 1000);
> >
> > /* Set the resolution */
> > regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,
> > @@ -335,12 +336,13 @@ static void sun4i_tcon0_mode_set_cpu(struct
> > sun4i_tcon *tcon,
> > u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> > u8 lanes = device->lanes;
> > u32 block_space, start_delay;
> > - u32 tcon_div;
> > + u32 tcon_div, tcon_mul;
> >
> > - tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
> > - tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
> > + tcon->dclk_min_div = 4;
> > + tcon->dclk_max_div = 127;
> >
> > - sun4i_tcon0_mode_set_common(tcon, mode);
> > + tcon_mul = bpp / lanes;
> > + sun4i_tcon0_mode_set_common(tcon, mode, tcon_mul);
> >
> > /* Set dithering if needed */
> > sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> > @@ -366,7 +368,7 @@ static void sun4i_tcon0_mode_set_cpu(struct
> > sun4i_tcon *tcon,
> > */
> > regmap_read(tcon->regs, SUN4I_TCON0_DCLK_REG, &tcon_div);
> > tcon_div &= GENMASK(6, 0);
> > - block_space = mode->htotal * bpp / (tcon_div * lanes);
> > + block_space = mode->htotal * tcon_div * tcon_mul;
> > block_space -= mode->hdisplay + 40;
> >
> > regmap_write(tcon->regs, SUN4I_TCON0_CPU_TRI0_REG,
> > @@ -408,7 +410,7 @@ static void sun4i_tcon0_mode_set_lvds(struct
> > sun4i_tcon *tcon,
> >
> > tcon->dclk_min_div = 7;
> > tcon->dclk_max_div = 7;
> > - sun4i_tcon0_mode_set_common(tcon, mode);
> > + sun4i_tcon0_mode_set_common(tcon, mode, 1);
> >
> > /* Set dithering if needed */
> > sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> > @@ -487,7 +489,7 @@ static void sun4i_tcon0_mode_set_rgb(struct
> > sun4i_tcon *tcon,
> >
> > tcon->dclk_min_div = 6;
> > tcon->dclk_max_div = 127;
> > - sun4i_tcon0_mode_set_common(tcon, mode);
> > + sun4i_tcon0_mode_set_common(tcon, mode, 1);
> >
> > /* Set dithering if needed */
> > sun4i_tcon0_mode_set_dithering(tcon, connector);
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > index 5c3ad5be0690..a07090579f84 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> > @@ -13,8 +13,6 @@
> > #include <drm/drm_encoder.h>
> > #include <drm/drm_mipi_dsi.h>
> >
> > -#define SUN6I_DSI_TCON_DIV 4
> > -
> > struct sun6i_dsi {
> > struct drm_connector connector;
> > struct drm_encoder encoder;
>
> I had more something like this in mind:
> http://code.bulix.org/nlp5a4-803511
Worth to look at it. was it working on your panel? meanwhile I will check it.
We have updated with below change [1], seems working on but is
actually checking the each divider as before start with 4... till 127.
This new approach, is start looking the best divider from 4.. based on
the idea vs rounded it will ended up best divider like [2]
https://gist.github.com/openedev/7e2c33248b372d29be9979e06d483673
https://gist.github.com/openedev/c72dfffc0ca59e7ec1edcd7ad360cdd1
Jagan.
^ permalink raw reply
* Re: [PATCH v6 11/22] clk: sunxi-ng: a64: Add minimum rate for PLL_MIPI
From: Maxime Ripard @ 2019-07-20 6:58 UTC (permalink / raw)
To: Michael Nazzareno Trimarchi
Cc: Jagan Teki, David Airlie, Daniel Vetter, Chen-Yu Tsai,
Michael Turquette, Rob Herring, Mark Rutland, linux-arm-kernel,
linux-kernel, linux-clk, dri-devel, devicetree, linux-amarula,
linux-sunxi
In-Reply-To: <CAOf5uw=3fiMuhcj3kDtCaGNTsxHKRrYb79MXZ+yUZtmf0jU10A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4589 bytes --]
On Thu, Jul 11, 2019 at 07:43:16PM +0200, Michael Nazzareno Trimarchi wrote:
> > > tcon-pixel clock is the rate that you want to achive on display side
> > > and if you have 4 lanes 32bit or lanes and different bit number that
> > > you need to have a clock that is able to put outside bits and speed
> > > equal to pixel-clock * bits / lanes. so If you want a pixel-clock of
> > > 40 mhz and you have 32bits and 4 lanes you need to have a clock of
> > > 40 * 32 / 4 in no-burst mode. I think that this is done but most of
> > > the display.
> >
> > So this is what the issue is then?
> >
> > This one does make sense, and you should just change the rate in the
> > call to clk_set_rate in sun4i_tcon0_mode_set_cpu.
> >
> > I'm still wondering why that hasn't been brought up in either the
> > discussion or the commit log before though.
> >
> Something like this?
>
> drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +++++++++++---------
> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 --
> 2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 64c43ee6bd92..42560d5c327c 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -263,10 +263,11 @@ static int sun4i_tcon_get_clk_delay(const struct
> drm_display_mode *mode,
> }
>
> static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon,
> - const struct drm_display_mode *mode)
> + const struct drm_display_mode *mode,
> + u32 tcon_mul)
> {
> /* Configure the dot clock */
> - clk_set_rate(tcon->dclk, mode->crtc_clock * 1000);
> + clk_set_rate(tcon->dclk, mode->crtc_clock * tcon_mul * 1000);
>
> /* Set the resolution */
> regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG,
> @@ -335,12 +336,13 @@ static void sun4i_tcon0_mode_set_cpu(struct
> sun4i_tcon *tcon,
> u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> u8 lanes = device->lanes;
> u32 block_space, start_delay;
> - u32 tcon_div;
> + u32 tcon_div, tcon_mul;
>
> - tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
> - tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
> + tcon->dclk_min_div = 4;
> + tcon->dclk_max_div = 127;
>
> - sun4i_tcon0_mode_set_common(tcon, mode);
> + tcon_mul = bpp / lanes;
> + sun4i_tcon0_mode_set_common(tcon, mode, tcon_mul);
>
> /* Set dithering if needed */
> sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> @@ -366,7 +368,7 @@ static void sun4i_tcon0_mode_set_cpu(struct
> sun4i_tcon *tcon,
> */
> regmap_read(tcon->regs, SUN4I_TCON0_DCLK_REG, &tcon_div);
> tcon_div &= GENMASK(6, 0);
> - block_space = mode->htotal * bpp / (tcon_div * lanes);
> + block_space = mode->htotal * tcon_div * tcon_mul;
> block_space -= mode->hdisplay + 40;
>
> regmap_write(tcon->regs, SUN4I_TCON0_CPU_TRI0_REG,
> @@ -408,7 +410,7 @@ static void sun4i_tcon0_mode_set_lvds(struct
> sun4i_tcon *tcon,
>
> tcon->dclk_min_div = 7;
> tcon->dclk_max_div = 7;
> - sun4i_tcon0_mode_set_common(tcon, mode);
> + sun4i_tcon0_mode_set_common(tcon, mode, 1);
>
> /* Set dithering if needed */
> sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder));
> @@ -487,7 +489,7 @@ static void sun4i_tcon0_mode_set_rgb(struct
> sun4i_tcon *tcon,
>
> tcon->dclk_min_div = 6;
> tcon->dclk_max_div = 127;
> - sun4i_tcon0_mode_set_common(tcon, mode);
> + sun4i_tcon0_mode_set_common(tcon, mode, 1);
>
> /* Set dithering if needed */
> sun4i_tcon0_mode_set_dithering(tcon, connector);
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> index 5c3ad5be0690..a07090579f84 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
> @@ -13,8 +13,6 @@
> #include <drm/drm_encoder.h>
> #include <drm/drm_mipi_dsi.h>
>
> -#define SUN6I_DSI_TCON_DIV 4
> -
> struct sun6i_dsi {
> struct drm_connector connector;
> struct drm_encoder encoder;
I had more something like this in mind:
http://code.bulix.org/nlp5a4-803511
You really don't need to change the divider range (or this is another
issue that the one you mentionned).
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* [PATCH 3/3] dt-bindings: dma: Convert Allwinner A31 and A64 DMA to a schema
From: Maxime Ripard @ 2019-07-20 6:41 UTC (permalink / raw)
To: Mark Rutland, Rob Herring, Frank Rowand, Vinod Koul
Cc: devicetree, Rob Herring, Maxime Ripard, Peter Ujfalusi,
Chen-Yu Tsai, dmaengine, linux-arm-kernel
In-Reply-To: <20190720064123.15411-1-maxime.ripard@bootlin.com>
The newer Allwinner SoCs have a DMA controller supported in Linux, with a
matching Device Tree binding.
Now that we have the DT validation in place, let's convert the device tree
bindings for that controller over to a YAML schemas.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
.../dma/allwinner,sun50i-a64-dma.yaml | 88 +++++++++++++++++++
.../bindings/dma/allwinner,sun6i-a31-dma.yaml | 62 +++++++++++++
.../devicetree/bindings/dma/sun6i-dma.txt | 81 -----------------
3 files changed, 150 insertions(+), 81 deletions(-)
create mode 100644 Documentation/devicetree/bindings/dma/allwinner,sun50i-a64-dma.yaml
create mode 100644 Documentation/devicetree/bindings/dma/allwinner,sun6i-a31-dma.yaml
delete mode 100644 Documentation/devicetree/bindings/dma/sun6i-dma.txt
diff --git a/Documentation/devicetree/bindings/dma/allwinner,sun50i-a64-dma.yaml b/Documentation/devicetree/bindings/dma/allwinner,sun50i-a64-dma.yaml
new file mode 100644
index 000000000000..4cb9d6b93138
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/allwinner,sun50i-a64-dma.yaml
@@ -0,0 +1,88 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/allwinner,sun50i-a64-dma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner A64 DMA Controller Device Tree Bindings
+
+maintainers:
+ - Chen-Yu Tsai <wens@csie.org>
+ - Maxime Ripard <maxime.ripard@bootlin.com>
+
+allOf:
+ - $ref: "dma-controller.yaml#"
+
+properties:
+ "#dma-cells":
+ const: 1
+ description: The cell is the request line number.
+
+ compatible:
+ enum:
+ - allwinner,sun50i-a64-dma
+ - allwinner,sun50i-h6-dma
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ minItems: 1
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: bus
+ - const: mbus
+
+ resets:
+ maxItems: 1
+
+required:
+ - "#dma-cells"
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - resets
+ - dma-channels
+
+if:
+ properties:
+ compatible:
+ const: allwinner,sun50i-h6-dma
+
+then:
+ properties:
+ clocks:
+ maxItems: 2
+
+ required:
+ - clock-names
+
+else:
+ properties:
+ clocks:
+ maxItems: 1
+
+# FIXME: We should set it, but it would report all the generic
+# properties as additional properties.
+# additionalProperties: false
+
+examples:
+ - |
+ dma: dma-controller@1c02000 {
+ compatible = "allwinner,sun50i-a64-dma";
+ reg = <0x01c02000 0x1000>;
+ interrupts = <0 50 4>;
+ clocks = <&ccu 30>;
+ dma-channels = <8>;
+ dma-requests = <27>;
+ resets = <&ccu 7>;
+ #dma-cells = <1>;
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/dma/allwinner,sun6i-a31-dma.yaml b/Documentation/devicetree/bindings/dma/allwinner,sun6i-a31-dma.yaml
new file mode 100644
index 000000000000..740b7f9b535b
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/allwinner,sun6i-a31-dma.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/allwinner,sun6i-a31-dma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner A31 DMA Controller Device Tree Bindings
+
+maintainers:
+ - Chen-Yu Tsai <wens@csie.org>
+ - Maxime Ripard <maxime.ripard@bootlin.com>
+
+allOf:
+ - $ref: "dma-controller.yaml#"
+
+properties:
+ "#dma-cells":
+ const: 1
+ description: The cell is the request line number.
+
+ compatible:
+ oneOf:
+ - const: allwinner,sun6i-a31-dma
+ - const: allwinner,sun8i-a23-dma
+ - const: allwinner,sun8i-a83t-dma
+ - const: allwinner,sun8i-h3-dma
+ - const: allwinner,sun8i-v3s-dma
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ resets:
+ maxItems: 1
+
+required:
+ - "#dma-cells"
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - resets
+
+additionalProperties: false
+
+examples:
+ - |
+ dma: dma-controller@1c02000 {
+ compatible = "allwinner,sun6i-a31-dma";
+ reg = <0x01c02000 0x1000>;
+ interrupts = <0 50 4>;
+ clocks = <&ahb1_gates 6>;
+ resets = <&ahb1_rst 6>;
+ #dma-cells = <1>;
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
deleted file mode 100644
index cae31f4e77ba..000000000000
--- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
+++ /dev/null
@@ -1,81 +0,0 @@
-Allwinner A31 DMA Controller
-
-This driver follows the generic DMA bindings defined in dma.txt.
-
-Required properties:
-
-- compatible: Must be one of
- "allwinner,sun6i-a31-dma"
- "allwinner,sun8i-a23-dma"
- "allwinner,sun8i-a83t-dma"
- "allwinner,sun8i-h3-dma"
- "allwinner,sun8i-v3s-dma"
-- reg: Should contain the registers base address and length
-- interrupts: Should contain a reference to the interrupt used by this device
-- clocks: Should contain a reference to the parent AHB clock
-- resets: Should contain a reference to the reset controller asserting
- this device in reset
-- #dma-cells : Should be 1, a single cell holding a line request number
-
-Example:
- dma: dma-controller@1c02000 {
- compatible = "allwinner,sun6i-a31-dma";
- reg = <0x01c02000 0x1000>;
- interrupts = <0 50 4>;
- clocks = <&ahb1_gates 6>;
- resets = <&ahb1_rst 6>;
- #dma-cells = <1>;
- };
-
-------------------------------------------------------------------------------
-For A64 and H6 DMA controller:
-
-Required properties:
-- compatible: Must be one of
- "allwinner,sun50i-a64-dma"
- "allwinner,sun50i-h6-dma"
-- dma-channels: Number of DMA channels supported by the controller.
- Refer to Documentation/devicetree/bindings/dma/dma.txt
-- clocks: In addition to parent AHB clock, it should also contain mbus
- clock (H6 only)
-- clock-names: Should contain "bus" and "mbus" (H6 only)
-- all properties above, i.e. reg, interrupts, clocks, resets and #dma-cells
-
-Optional properties:
-- dma-requests: Number of DMA request signals supported by the controller.
- Refer to Documentation/devicetree/bindings/dma/dma.txt
-
-Example:
- dma: dma-controller@1c02000 {
- compatible = "allwinner,sun50i-a64-dma";
- reg = <0x01c02000 0x1000>;
- interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&ccu CLK_BUS_DMA>;
- dma-channels = <8>;
- dma-requests = <27>;
- resets = <&ccu RST_BUS_DMA>;
- #dma-cells = <1>;
- };
-------------------------------------------------------------------------------
-
-Clients:
-
-DMA clients connected to the A31 DMA controller must use the format
-described in the dma.txt file, using a two-cell specifier for each
-channel: a phandle plus one integer cells.
-The two cells in order are:
-
-1. A phandle pointing to the DMA controller.
-2. The port ID as specified in the datasheet
-
-Example:
-spi2: spi@1c6a000 {
- compatible = "allwinner,sun6i-a31-spi";
- reg = <0x01c6a000 0x1000>;
- interrupts = <0 67 4>;
- clocks = <&ahb1_gates 22>, <&spi2_clk>;
- clock-names = "ahb", "mod";
- dmas = <&dma 25>, <&dma 25>;
- dma-names = "rx", "tx";
- resets = <&ahb1_rst 22>;
-};
--
2.21.0
^ permalink raw reply related
* [PATCH 2/3] dt-bindings: dma: Convert Allwinner A10 DMA to a schema
From: Maxime Ripard @ 2019-07-20 6:41 UTC (permalink / raw)
To: Mark Rutland, Rob Herring, Frank Rowand, Vinod Koul
Cc: devicetree, dmaengine, Chen-Yu Tsai, Maxime Ripard,
linux-arm-kernel, Peter Ujfalusi, Rob Herring
In-Reply-To: <20190720064123.15411-1-maxime.ripard@bootlin.com>
The older Allwinner SoCs have a DMA controller supported in Linux, with a
matching Device Tree binding.
Now that we have the DT validation in place, let's convert the device tree
bindings for that controller over to a YAML schemas.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
.../bindings/dma/allwinner,sun4i-a10-dma.yaml | 55 +++++++++++++++++++
.../devicetree/bindings/dma/sun4i-dma.txt | 45 ---------------
2 files changed, 55 insertions(+), 45 deletions(-)
create mode 100644 Documentation/devicetree/bindings/dma/allwinner,sun4i-a10-dma.yaml
delete mode 100644 Documentation/devicetree/bindings/dma/sun4i-dma.txt
diff --git a/Documentation/devicetree/bindings/dma/allwinner,sun4i-a10-dma.yaml b/Documentation/devicetree/bindings/dma/allwinner,sun4i-a10-dma.yaml
new file mode 100644
index 000000000000..15abc0f9429f
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/allwinner,sun4i-a10-dma.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/allwinner,sun4i-a10-dma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner A10 DMA Controller Device Tree Bindings
+
+maintainers:
+ - Chen-Yu Tsai <wens@csie.org>
+ - Maxime Ripard <maxime.ripard@bootlin.com>
+
+allOf:
+ - $ref: "dma-controller.yaml#"
+
+properties:
+ "#dma-cells":
+ const: 2
+ description:
+ The first cell is either 0 or 1, the former to use the normal
+ DMA, 1 for dedicated DMA. The second cell is the request line
+ number.
+
+ compatible:
+ const: allwinner,sun4i-a10-dma
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+required:
+ - "#dma-cells"
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+
+additionalProperties: false
+
+examples:
+ - |
+ dma: dma-controller@1c02000 {
+ compatible = "allwinner,sun4i-a10-dma";
+ reg = <0x01c02000 0x1000>;
+ interrupts = <27>;
+ clocks = <&ahb_gates 6>;
+ #dma-cells = <2>;
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/dma/sun4i-dma.txt b/Documentation/devicetree/bindings/dma/sun4i-dma.txt
deleted file mode 100644
index 8ad556aca70b..000000000000
--- a/Documentation/devicetree/bindings/dma/sun4i-dma.txt
+++ /dev/null
@@ -1,45 +0,0 @@
-Allwinner A10 DMA Controller
-
-This driver follows the generic DMA bindings defined in dma.txt.
-
-Required properties:
-
-- compatible: Must be "allwinner,sun4i-a10-dma"
-- reg: Should contain the registers base address and length
-- interrupts: Should contain a reference to the interrupt used by this device
-- clocks: Should contain a reference to the parent AHB clock
-- #dma-cells : Should be 2, first cell denoting normal or dedicated dma,
- second cell holding the request line number.
-
-Example:
- dma: dma-controller@1c02000 {
- compatible = "allwinner,sun4i-a10-dma";
- reg = <0x01c02000 0x1000>;
- interrupts = <27>;
- clocks = <&ahb_gates 6>;
- #dma-cells = <2>;
- };
-
-Clients:
-
-DMA clients connected to the Allwinner A10 DMA controller must use the
-format described in the dma.txt file, using a three-cell specifier for
-each channel: a phandle plus two integer cells.
-The three cells in order are:
-
-1. A phandle pointing to the DMA controller.
-2. Whether it is using normal (0) or dedicated (1) channels
-3. The port ID as specified in the datasheet
-
-Example:
- spi2: spi@1c17000 {
- compatible = "allwinner,sun4i-a10-spi";
- reg = <0x01c17000 0x1000>;
- interrupts = <0 12 4>;
- clocks = <&ahb_gates 22>, <&spi2_clk>;
- clock-names = "ahb", "mod";
- dmas = <&dma 1 29>, <&dma 1 28>;
- dma-names = "rx", "tx";
- #address-cells = <1>;
- #size-cells = <0>;
- };
--
2.21.0
^ permalink raw reply related
* [PATCH 1/3] dt-bindings: dma: Add YAML schemas for the generic DMA bindings
From: Maxime Ripard @ 2019-07-20 6:41 UTC (permalink / raw)
To: Mark Rutland, Rob Herring, Frank Rowand, Vinod Koul
Cc: devicetree, Maxime Ripard, Peter Ujfalusi, Chen-Yu Tsai,
dmaengine, linux-arm-kernel
The DMA controllers and consumers have a bunch of generic properties that
are needed in a device tree. Add a YAML schemas for those.
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
Changes from v1:
- Dropped the dma consumer schemas
- Fixed the node name of the examples
- Enhanced a bit the description for dma-requests in case of a router
- Split the bindings in two to handle the router and controller case
separately
- Made #dma-cells required
---
.../devicetree/bindings/dma/dma-common.yaml | 43 +++++++
.../bindings/dma/dma-controller.yaml | 35 ++++++
.../devicetree/bindings/dma/dma-router.yaml | 50 ++++++++
Documentation/devicetree/bindings/dma/dma.txt | 114 +-----------------
4 files changed, 129 insertions(+), 113 deletions(-)
create mode 100644 Documentation/devicetree/bindings/dma/dma-common.yaml
create mode 100644 Documentation/devicetree/bindings/dma/dma-controller.yaml
create mode 100644 Documentation/devicetree/bindings/dma/dma-router.yaml
diff --git a/Documentation/devicetree/bindings/dma/dma-common.yaml b/Documentation/devicetree/bindings/dma/dma-common.yaml
new file mode 100644
index 000000000000..422fd6c8b0ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/dma-common.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/dma-common.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DMA Engine Generic Binding
+
+maintainers:
+ - Vinod Koul <vkoul@kernel.org>
+
+description:
+ Generic binding to provide a way for a driver using DMA Engine to
+ retrieve the DMA request or channel information that goes from a
+ hardware device to a DMA controller.
+
+properties:
+ "#dma-cells":
+ minimum: 1
+ # Should be enough
+ maximum: 255
+ description:
+ Used to provide DMA controller specific information.
+
+ dma-channel-masks:
+ $ref: /schemas/types.yaml#definitions/uint32
+ description:
+ Bitmask of available DMA channels in ascending order that are
+ not reserved by firmware and are available to the
+ kernel. i.e. first channel corresponds to LSB.
+
+ dma-channels:
+ $ref: /schemas/types.yaml#definitions/uint32
+ description:
+ Number of DMA channels supported by the controller.
+
+ dma-requests:
+ $ref: /schemas/types.yaml#definitions/uint32
+ description:
+ Number of DMA request signals supported by the controller.
+
+required:
+ - "#dma-cells"
diff --git a/Documentation/devicetree/bindings/dma/dma-controller.yaml b/Documentation/devicetree/bindings/dma/dma-controller.yaml
new file mode 100644
index 000000000000..c39f6de76670
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/dma-controller.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/dma-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DMA Controller Generic Binding
+
+maintainers:
+ - Vinod Koul <vkoul@kernel.org>
+
+allOf:
+ - $ref: "dma-common.yaml#"
+
+# Everything else is described in the common file
+properties:
+ $nodename:
+ pattern: "^dma-controller(@.*)?$"
+
+examples:
+ - |
+ dma: dma-controller@48000000 {
+ compatible = "ti,omap-sdma";
+ reg = <0x48000000 0x1000>;
+ interrupts = <0 12 0x4
+ 0 13 0x4
+ 0 14 0x4
+ 0 15 0x4>;
+ #dma-cells = <1>;
+ dma-channels = <32>;
+ dma-requests = <127>;
+ dma-channel-mask = <0xfffe>;
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/dma/dma-router.yaml b/Documentation/devicetree/bindings/dma/dma-router.yaml
new file mode 100644
index 000000000000..5b5f07393135
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/dma-router.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/dma-router.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DMA Router Generic Binding
+
+maintainers:
+ - Vinod Koul <vkoul@kernel.org>
+
+allOf:
+ - $ref: "dma-common.yaml#"
+
+description:
+ DMA routers are transparent IP blocks used to route DMA request
+ lines from devices to the DMA controller. Some SoCs (like TI DRA7x)
+ have more peripherals integrated with DMA requests than what the DMA
+ controller can handle directly.
+
+properties:
+ $nodename:
+ pattern: "^dma-router(@.*)?$"
+
+ dma-masters:
+ $ref: /schemas/types.yaml#definitions/phandle-array
+ description:
+ Array of phandles to the DMA controllers the router can direct
+ the signal to.
+
+ dma-requests:
+ description:
+ Number of incoming request lines the router can handle.
+
+required:
+ - "#dma-cells"
+ - dma-masters
+
+examples:
+ - |
+ sdma_xbar: dma-router@4a002b78 {
+ compatible = "ti,dra7-dma-crossbar";
+ reg = <0x4a002b78 0xfc>;
+ #dma-cells = <1>;
+ dma-requests = <205>;
+ ti,dma-safe-map = <0>;
+ dma-masters = <&sdma>;
+ };
+
+...
diff --git a/Documentation/devicetree/bindings/dma/dma.txt b/Documentation/devicetree/bindings/dma/dma.txt
index eeb4e4d1771e..90a67a016a48 100644
--- a/Documentation/devicetree/bindings/dma/dma.txt
+++ b/Documentation/devicetree/bindings/dma/dma.txt
@@ -1,113 +1 @@
-* Generic DMA Controller and DMA request bindings
-
-Generic binding to provide a way for a driver using DMA Engine to retrieve the
-DMA request or channel information that goes from a hardware device to a DMA
-controller.
-
-
-* DMA controller
-
-Required property:
-- #dma-cells: Must be at least 1. Used to provide DMA controller
- specific information. See DMA client binding below for
- more details.
-
-Optional properties:
-- dma-channels: Number of DMA channels supported by the controller.
-- dma-requests: Number of DMA request signals supported by the
- controller.
-- dma-channel-mask: Bitmask of available DMA channels in ascending order
- that are not reserved by firmware and are available to
- the kernel. i.e. first channel corresponds to LSB.
-
-Example:
-
- dma: dma@48000000 {
- compatible = "ti,omap-sdma";
- reg = <0x48000000 0x1000>;
- interrupts = <0 12 0x4
- 0 13 0x4
- 0 14 0x4
- 0 15 0x4>;
- #dma-cells = <1>;
- dma-channels = <32>;
- dma-requests = <127>;
- dma-channel-mask = <0xfffe>
- };
-
-* DMA router
-
-DMA routers are transparent IP blocks used to route DMA request lines from
-devices to the DMA controller. Some SoCs (like TI DRA7x) have more peripherals
-integrated with DMA requests than what the DMA controller can handle directly.
-
-Required property:
-- dma-masters: phandle of the DMA controller or list of phandles for
- the DMA controllers the router can direct the signal to.
-- #dma-cells: Must be at least 1. Used to provide DMA router specific
- information. See DMA client binding below for more
- details.
-
-Optional properties:
-- dma-requests: Number of incoming request lines the router can handle.
-- In the node pointed by the dma-masters:
- - dma-requests: The router driver might need to look for this in order
- to configure the routing.
-
-Example:
- sdma_xbar: dma-router@4a002b78 {
- compatible = "ti,dra7-dma-crossbar";
- reg = <0x4a002b78 0xfc>;
- #dma-cells = <1>;
- dma-requests = <205>;
- ti,dma-safe-map = <0>;
- dma-masters = <&sdma>;
- };
-
-* DMA client
-
-Client drivers should specify the DMA property using a phandle to the controller
-followed by DMA controller specific data.
-
-Required property:
-- dmas: List of one or more DMA specifiers, each consisting of
- - A phandle pointing to DMA controller node
- - A number of integer cells, as determined by the
- #dma-cells property in the node referenced by phandle
- containing DMA controller specific information. This
- typically contains a DMA request line number or a
- channel number, but can contain any data that is
- required for configuring a channel.
-- dma-names: Contains one identifier string for each DMA specifier in
- the dmas property. The specific strings that can be used
- are defined in the binding of the DMA client device.
- Multiple DMA specifiers can be used to represent
- alternatives and in this case the dma-names for those
- DMA specifiers must be identical (see examples).
-
-Examples:
-
-1. A device with one DMA read channel, one DMA write channel:
-
- i2c1: i2c@1 {
- ...
- dmas = <&dma 2 /* read channel */
- &dma 3>; /* write channel */
- dma-names = "rx", "tx";
- ...
- };
-
-2. A single read-write channel with three alternative DMA controllers:
-
- dmas = <&dma1 5
- &dma2 7
- &dma3 2>;
- dma-names = "rx-tx", "rx-tx", "rx-tx";
-
-3. A device with three channels, one of which has two alternatives:
-
- dmas = <&dma1 2 /* read channel */
- &dma1 3 /* write channel */
- &dma2 0 /* error read */
- &dma3 0>; /* alternative error read */
- dma-names = "rx", "tx", "error", "error";
+This file has been moved to dma-controller.yaml.
--
2.21.0
^ permalink raw reply related
* [PATCH v6 7/7] of/platform: Don't create device links for default busses
From: Saravana Kannan @ 2019-07-20 6:16 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190720061647.234852-1-saravanak@google.com>
Default busses also have devices created for them. But there's no point
in creating device links for them. It's especially wasteful as it'll
cause the traversal of the entire device tree and also spend a lot of
time checking and figuring out that creating those links isn't allowed.
So check for default busses and skip trying to create device links for
them.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/of/platform.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index c1a116f7a087..8bf975ee2ff7 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -676,6 +676,8 @@ static int of_link_to_suppliers(struct device *dev)
return 0;
if (unlikely(!dev->of_node))
return 0;
+ if (of_match_node(of_default_bus_match_table, dev->of_node))
+ return 0;
return __of_link_to_suppliers(dev, dev->of_node);
}
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* [PATCH v6 6/7] of/platform: Create device links for all child-supplier depencencies
From: Saravana Kannan @ 2019-07-20 6:16 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190720061647.234852-1-saravanak@google.com>
A parent device can have child devices that it adds when it probes. But
this probing of the parent device can happen way after kernel init is done
-- for example, when the parent device's driver is loaded as a module.
In such cases, if the child devices depend on a supplier in the system, we
need to make sure the supplier gets the sync_state() callback only after
these child devices are added and probed.
To achieve this, when creating device links for a device by looking at its
DT node, don't just look at DT references at the top node level. Look at DT
references in all the descendant nodes too and create device links from the
ancestor device to all these supplier devices.
This way, when the parent device probes and adds child devices, the child
devices can then create their own device links to the suppliers and further
delay the supplier's sync_state() callback to after the child devices are
probed.
Example:
In this illustration, -> denotes DT references and indentation
represents child status.
Device node A
Device node B -> D
Device node C -> B, D
Device node D
Assume all these devices have their drivers loaded as modules.
Without this patch, this is the sequence of events:
1. D is added.
2. A is added.
3. Device D probes.
4. Device D gets its sync_state() callback.
5. Device B and C might malfunction because their resources got
altered/turned off before they can make active requests for them.
With this patch, this is the sequence of events:
1. D is added.
2. A is added and creates device links to D.
3. Device link from A to B is not added because A is a parent of B.
4. Device D probes.
5. Device D does not get it's sync_state() callback because consumer A
hasn't probed yet.
5. Device A probes.
5. a. Devices B and C are added.
5. b. Device links from B and C to D are added.
5. c. Device A's probe completes.
6. Device D does not get it's sync_state() callback because consumer A
has probed but consumers B and C haven't probed yet.
7. Device B and C probe.
8. Device D gets it's sync_state() callback because all its consumers
have probed.
9. None of the devices malfunction.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/of/platform.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index d0c2f6443247..c1a116f7a087 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -649,24 +649,35 @@ static bool of_link_property(struct device *dev, struct device_node *con_np,
return done ? 0 : -ENODEV;
}
+static int __of_link_to_suppliers(struct device *dev,
+ struct device_node *con_np)
+{
+ struct device_node *child;
+ struct property *p;
+ bool done = true;
+
+ for_each_property_of_node(con_np, p)
+ if (of_link_property(dev, con_np, p->name))
+ done = false;
+
+ for_each_child_of_node(con_np, child)
+ if (__of_link_to_suppliers(dev, child))
+ done = false;
+
+ return done ? 0 : -ENODEV;
+}
+
static bool of_devlink;
core_param(of_devlink, of_devlink, bool, 0);
static int of_link_to_suppliers(struct device *dev)
{
- struct property *p;
- bool done = true;
-
if (!of_devlink)
return 0;
if (unlikely(!dev->of_node))
return 0;
- for_each_property_of_node(dev->of_node, p)
- if (of_link_property(dev, dev->of_node, p->name))
- done = false;
-
- return done ? 0 : -ENODEV;
+ return __of_link_to_suppliers(dev, dev->of_node);
}
#ifndef CONFIG_PPC
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* [PATCH v6 5/7] of/platform: Pause/resume sync state during init and of_platform_populate()
From: Saravana Kannan @ 2019-07-20 6:16 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190720061647.234852-1-saravanak@google.com>
When all the top level devices are populated from DT during kernel
init, the supplier devices could be added and probed before the
consumer devices are added and linked to the suppliers. To avoid the
sync_state() callback from being called prematurely, pause the
sync_state() callbacks before populating the devices and resume them
at late_initcall_sync().
Similarly, when children devices are populated after kernel init using
of_platform_populate(), there could be supplier-consumer dependencies
between the children devices that are populated. To avoid the same
problem with sync_state() being called prematurely, pause and resume
sync_state() callbacks across of_platform_populate().
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/of/platform.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 88a2086e26fa..d0c2f6443247 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -486,6 +486,7 @@ int of_platform_populate(struct device_node *root,
pr_debug("%s()\n", __func__);
pr_debug(" starting at: %pOF\n", root);
+ device_links_supplier_sync_state_pause();
for_each_child_of_node(root, child) {
rc = of_platform_bus_create(child, matches, lookup, parent, true);
if (rc) {
@@ -493,6 +494,8 @@ int of_platform_populate(struct device_node *root,
break;
}
}
+ device_links_supplier_sync_state_resume();
+
of_node_set_flag(root, OF_POPULATED_BUS);
of_node_put(root);
@@ -682,6 +685,7 @@ static int __init of_platform_default_populate_init(void)
return -ENODEV;
platform_bus_type.add_links = of_link_to_suppliers;
+ device_links_supplier_sync_state_pause();
/*
* Handle certain compatibles explicitly, since we don't want to create
* platform_devices for every node in /reserved-memory with a
@@ -702,6 +706,13 @@ static int __init of_platform_default_populate_init(void)
return 0;
}
arch_initcall_sync(of_platform_default_populate_init);
+
+static int __init of_platform_sync_state_init(void)
+{
+ device_links_supplier_sync_state_resume();
+ return 0;
+}
+late_initcall_sync(of_platform_sync_state_init);
#endif
int of_platform_device_destroy(struct device *dev, void *data)
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* [PATCH v6 4/7] driver core: Add sync_state driver/bus callback
From: Saravana Kannan @ 2019-07-20 6:16 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190720061647.234852-1-saravanak@google.com>
This sync_state driver/bus callback is called once all the consumers
of a supplier have probed successfully.
This allows the supplier device's driver/bus to sync the supplier
device's state to the software state with the guarantee that all the
consumers are actively managing the resources provided by the supplier
device.
To maintain backwards compatibility and ease transition from existing
frameworks and resource cleanup schemes, late_initcall_sync is the
earliest when the sync_state callback might be called.
There is no upper bound on the time by which the sync_state callback
has to be called. This is because if a consumer device never probes,
the supplier has to maintain its resources in the state left by the
bootloader. For example, if the bootloader leaves the display
backlight at a fixed voltage and the backlight driver is never probed,
you don't want the backlight to ever be turned off after boot up.
Also, when multiple devices are added after kernel init, some
suppliers could be added before their consumer devices get added. In
these instances, the supplier devices could get their sync_state
callback called right after they probe because the consumers devices
haven't had a chance to create device links to the suppliers.
To handle this correctly, this change also provides APIs to
pause/resume sync state callbacks so that when multiple devices are
added, their sync_state callback evaluation can be postponed to happen
after all of them are added.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/base/core.c | 65 ++++++++++++++++++++++++++++++++++++++++++
include/linux/device.h | 21 ++++++++++++++
2 files changed, 86 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6eb14093e905..b03e679faea4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -46,6 +46,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
/* Device links support. */
static LIST_HEAD(wait_for_suppliers);
static DEFINE_MUTEX(wfs_lock);
+static LIST_HEAD(deferred_sync);
+static unsigned int supplier_sync_state_disabled;
#ifdef CONFIG_SRCU
static DEFINE_MUTEX(device_links_lock);
@@ -634,6 +636,62 @@ int device_links_check_suppliers(struct device *dev)
return ret;
}
+static void __device_links_supplier_sync_state(struct device *dev)
+{
+ struct device_link *link;
+
+ if (dev->state_synced)
+ return;
+
+ list_for_each_entry(link, &dev->links.consumers, s_node) {
+ if (link->flags & DL_FLAG_STATELESS)
+ continue;
+ if (link->status != DL_STATE_ACTIVE)
+ return;
+ }
+
+ if (dev->bus->sync_state)
+ dev->bus->sync_state(dev);
+ else if (dev->driver && dev->driver->sync_state)
+ dev->driver->sync_state(dev);
+
+ dev->state_synced = true;
+}
+
+void device_links_supplier_sync_state_pause(void)
+{
+ device_links_write_lock();
+ supplier_sync_state_disabled++;
+ device_links_write_unlock();
+}
+
+void device_links_supplier_sync_state_resume(void)
+{
+ struct device *dev, *tmp;
+
+ device_links_write_lock();
+ if (!supplier_sync_state_disabled) {
+ WARN(true, "Unmatched sync_state pause/resume!");
+ goto out;
+ }
+ supplier_sync_state_disabled--;
+ if (supplier_sync_state_disabled)
+ goto out;
+
+ list_for_each_entry_safe(dev, tmp, &deferred_sync, links.defer_sync) {
+ __device_links_supplier_sync_state(dev);
+ list_del_init(&dev->links.defer_sync);
+ }
+out:
+ device_links_write_unlock();
+}
+
+static void __device_links_supplier_defer_sync(struct device *sup)
+{
+ if (list_empty(&sup->links.defer_sync))
+ list_add_tail(&sup->links.defer_sync, &deferred_sync);
+}
+
/**
* device_links_driver_bound - Update device links after probing its driver.
* @dev: Device to update the links for.
@@ -678,6 +736,11 @@ void device_links_driver_bound(struct device *dev)
WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
WRITE_ONCE(link->status, DL_STATE_ACTIVE);
+
+ if (supplier_sync_state_disabled)
+ __device_links_supplier_defer_sync(link->supplier);
+ else
+ __device_links_supplier_sync_state(link->supplier);
}
dev->links.status = DL_DEV_DRIVER_BOUND;
@@ -787,6 +850,7 @@ void device_links_driver_cleanup(struct device *dev)
WRITE_ONCE(link->status, DL_STATE_DORMANT);
}
+ list_del_init(&dev->links.defer_sync);
__device_links_no_driver(dev);
device_links_write_unlock();
@@ -1758,6 +1822,7 @@ void device_initialize(struct device *dev)
INIT_LIST_HEAD(&dev->links.consumers);
INIT_LIST_HEAD(&dev->links.suppliers);
INIT_LIST_HEAD(&dev->links.needs_suppliers);
+ INIT_LIST_HEAD(&dev->links.defer_sync);
dev->links.status = DL_DEV_NO_DRIVER;
}
EXPORT_SYMBOL_GPL(device_initialize);
diff --git a/include/linux/device.h b/include/linux/device.h
index a2d509a36ebe..489d9f4e3104 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -84,6 +84,13 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
* in the future.
* @probe: Called when a new device or driver add to this bus, and callback
* the specific driver's probe to initial the matched device.
+ * @sync_state: Called to sync device state to software state after all the
+ * state tracking consumers linked to this device (present at
+ * the time of late_initcall) have successfully bound to a
+ * driver. If the device has no consumers, this function will
+ * be called at late_initcall_sync level. If the device has
+ * consumers that are never bound to a driver, this function
+ * will never get called until they do.
* @remove: Called when a device removed from this bus.
* @shutdown: Called at shut-down time to quiesce the device.
*
@@ -128,6 +135,7 @@ struct bus_type {
int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
int (*add_links)(struct device *dev);
int (*probe)(struct device *dev);
+ void (*sync_state)(struct device *dev);
int (*remove)(struct device *dev);
void (*shutdown)(struct device *dev);
@@ -271,6 +279,13 @@ enum probe_type {
* @probe: Called to query the existence of a specific device,
* whether this driver can work with it, and bind the driver
* to a specific device.
+ * @sync_state: Called to sync device state to software state after all the
+ * state tracking consumers linked to this device (present at
+ * the time of late_initcall) have successfully bound to a
+ * driver. If the device has no consumers, this function will
+ * be called at late_initcall_sync level. If the device has
+ * consumers that are never bound to a driver, this function
+ * will never get called until they do.
* @remove: Called when the device is removed from the system to
* unbind a device from this driver.
* @shutdown: Called at shut-down time to quiesce the device.
@@ -309,6 +324,7 @@ struct device_driver {
int (*edit_links)(struct device *dev);
int (*probe) (struct device *dev);
+ void (*sync_state)(struct device *dev);
int (*remove) (struct device *dev);
void (*shutdown) (struct device *dev);
int (*suspend) (struct device *dev, pm_message_t state);
@@ -910,12 +926,14 @@ enum dl_dev_state {
* @suppliers: List of links to supplier devices.
* @consumers: List of links to consumer devices.
* @needs_suppliers: Hook to global list of devices waiting for suppliers.
+ * @defer_sync: Hook to global list of devices that have deferred sync_state.
* @status: Driver status information.
*/
struct dev_links_info {
struct list_head suppliers;
struct list_head consumers;
struct list_head needs_suppliers;
+ struct list_head defer_sync;
enum dl_dev_state status;
};
@@ -1081,6 +1099,7 @@ struct device {
bool offline:1;
bool of_node_reused:1;
bool has_edit_links:1;
+ bool state_synced:1;
#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
@@ -1422,6 +1441,8 @@ struct device_link *device_link_add(struct device *consumer,
void device_link_del(struct device_link *link);
void device_link_remove(void *consumer, struct device *supplier);
void device_link_remove_from_wfs(struct device *consumer);
+void device_links_supplier_sync_state_pause(void);
+void device_links_supplier_sync_state_resume(void);
#ifndef dev_fmt
#define dev_fmt(fmt) fmt
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* [PATCH v6 3/7] of/platform: Add functional dependency link from DT bindings
From: Saravana Kannan @ 2019-07-20 6:16 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand, Jonathan Corbet
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team, linux-doc
In-Reply-To: <20190720061647.234852-1-saravanak@google.com>
Add device-links after the devices are created (but before they are
probed) by looking at common DT bindings like clocks and
interconnects.
Automatically adding device-links for functional dependencies at the
framework level provides the following benefits:
- Optimizes device probe order and avoids the useless work of
attempting probes of devices that will not probe successfully
(because their suppliers aren't present or haven't probed yet).
For example, in a commonly available mobile SoC, registering just
one consumer device's driver at an initcall level earlier than the
supplier device's driver causes 11 failed probe attempts before the
consumer device probes successfully. This was with a kernel with all
the drivers statically compiled in. This problem gets a lot worse if
all the drivers are loaded as modules without direct symbol
dependencies.
- Supplier devices like clock providers, interconnect providers, etc
need to keep the resources they provide active and at a particular
state(s) during boot up even if their current set of consumers don't
request the resource to be active. This is because the rest of the
consumers might not have probed yet and turning off the resource
before all the consumers have probed could lead to a hang or
undesired user experience.
Some frameworks (Eg: regulator) handle this today by turning off
"unused" resources at late_initcall_sync and hoping all the devices
have probed by then. This is not a valid assumption for systems with
loadable modules. Other frameworks (Eg: clock) just don't handle
this due to the lack of a clear signal for when they can turn off
resources. This leads to downstream hacks to handle cases like this
that can easily be solved in the upstream kernel.
By linking devices before they are probed, we give suppliers a clear
count of the number of dependent consumers. Once all of the
consumers are active, the suppliers can turn off the unused
resources without making assumptions about the number of consumers.
By default we just add device-links to track "driver presence" (probe
succeeded) of the supplier device. If any other functionality provided
by device-links are needed, it is left to the consumer/supplier
devices to change the link when they probe.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
.../admin-guide/kernel-parameters.txt | 5 +
drivers/of/platform.c | 158 ++++++++++++++++++
2 files changed, 163 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 138f6664b2e2..109b4310844f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3141,6 +3141,11 @@
This can be set from sysctl after boot.
See Documentation/sysctl/vm.txt for details.
+ of_devlink [KNL] Make device links from common DT bindings. Useful
+ for optimizing probe order and making sure resources
+ aren't turned off before the consumer devices have
+ probed.
+
ohci1394_dma=early [HW] enable debugging via the ohci1394 driver.
See Documentation/debugging-via-ohci1394.txt for more
info.
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 04ad312fd85b..88a2086e26fa 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -509,6 +509,163 @@ int of_platform_default_populate(struct device_node *root,
}
EXPORT_SYMBOL_GPL(of_platform_default_populate);
+bool of_link_is_valid(struct device_node *con, struct device_node *sup)
+{
+ of_node_get(sup);
+ /*
+ * Don't allow linking a device node as a consumer of one of its
+ * descendant nodes. By definition, a child node can't be a functional
+ * dependency for the parent node.
+ */
+ while (sup) {
+ if (sup == con) {
+ of_node_put(sup);
+ return false;
+ }
+ sup = of_get_next_parent(sup);
+ }
+ return true;
+}
+
+static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
+{
+ struct platform_device *sup_dev;
+ u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
+ int ret = 0;
+
+ /*
+ * Since we are trying to create device links, we need to find
+ * the actual device node that owns this supplier phandle.
+ * Often times it's the same node, but sometimes it can be one
+ * of the parents. So walk up the parent till you find a
+ * device.
+ */
+ while (sup_np && !of_find_property(sup_np, "compatible", NULL))
+ sup_np = of_get_next_parent(sup_np);
+ if (!sup_np)
+ return 0;
+
+ if (!of_link_is_valid(dev->of_node, sup_np)) {
+ of_node_put(sup_np);
+ return 0;
+ }
+ sup_dev = of_find_device_by_node(sup_np);
+ of_node_put(sup_np);
+ if (!sup_dev)
+ return -ENODEV;
+ if (!device_link_add(dev, &sup_dev->dev, dl_flags))
+ ret = -ENODEV;
+ put_device(&sup_dev->dev);
+ return ret;
+}
+
+static struct device_node *parse_prop_cells(struct device_node *np,
+ const char *prop, int i,
+ const char *binding,
+ const char *cell)
+{
+ struct of_phandle_args sup_args;
+
+ if (!i && strcmp(prop, binding))
+ return NULL;
+
+ if (of_parse_phandle_with_args(np, binding, cell, i, &sup_args))
+ return NULL;
+
+ return sup_args.np;
+}
+
+static struct device_node *parse_clocks(struct device_node *np,
+ const char *prop, int i)
+{
+ return parse_prop_cells(np, prop, i, "clocks", "#clock-cells");
+}
+
+static struct device_node *parse_interconnects(struct device_node *np,
+ const char *prop, int i)
+{
+ return parse_prop_cells(np, prop, i, "interconnects",
+ "#interconnect-cells");
+}
+
+static int strcmp_suffix(const char *str, const char *suffix)
+{
+ unsigned int len, suffix_len;
+
+ len = strlen(str);
+ suffix_len = strlen(suffix);
+ if (len <= suffix_len)
+ return -1;
+ return strcmp(str + len - suffix_len, suffix);
+}
+
+static struct device_node *parse_regulators(struct device_node *np,
+ const char *prop, int i)
+{
+ if (i || strcmp_suffix(prop, "-supply"))
+ return NULL;
+
+ return of_parse_phandle(np, prop, 0);
+}
+
+/**
+ * struct supplier_bindings - Information for parsing supplier DT binding
+ *
+ * @parse_prop: If the function cannot parse the property, return NULL.
+ * Otherwise, return the phandle listed in the property
+ * that corresponds to index i.
+ */
+struct supplier_bindings {
+ struct device_node *(*parse_prop)(struct device_node *np,
+ const char *name, int i);
+};
+
+struct supplier_bindings bindings[] = {
+ { .parse_prop = parse_clocks, },
+ { .parse_prop = parse_interconnects, },
+ { .parse_prop = parse_regulators, },
+ { },
+};
+
+static bool of_link_property(struct device *dev, struct device_node *con_np,
+ const char *prop)
+{
+ struct device_node *phandle;
+ struct supplier_bindings *s = bindings;
+ unsigned int i = 0;
+ bool done = true;
+
+ while (!i && s->parse_prop) {
+ while ((phandle = s->parse_prop(con_np, prop, i))) {
+ i++;
+ if (of_link_to_phandle(dev, phandle))
+ done = false;
+ }
+ s++;
+ }
+ return done ? 0 : -ENODEV;
+}
+
+static bool of_devlink;
+core_param(of_devlink, of_devlink, bool, 0);
+
+static int of_link_to_suppliers(struct device *dev)
+{
+ struct property *p;
+ bool done = true;
+
+ if (!of_devlink)
+ return 0;
+ if (unlikely(!dev->of_node))
+ return 0;
+
+ for_each_property_of_node(dev->of_node, p)
+ if (of_link_property(dev, dev->of_node, p->name))
+ done = false;
+
+ return done ? 0 : -ENODEV;
+}
+
#ifndef CONFIG_PPC
static const struct of_device_id reserved_mem_matches[] = {
{ .compatible = "qcom,rmtfs-mem" },
@@ -524,6 +681,7 @@ static int __init of_platform_default_populate_init(void)
if (!of_have_populated_dt())
return -ENODEV;
+ platform_bus_type.add_links = of_link_to_suppliers;
/*
* Handle certain compatibles explicitly, since we don't want to create
* platform_devices for every node in /reserved-memory with a
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* [PATCH v6 2/7] driver core: Add edit_links() callback for drivers
From: Saravana Kannan @ 2019-07-20 6:16 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190720061647.234852-1-saravanak@google.com>
The driver core/bus adding supplier-consumer dependencies by default
enables functional dependencies to be tracked correctly even when the
consumer devices haven't had their drivers registered or loaded (if they
are modules).
However, when the bus incorrectly adds dependencies that it shouldn't
have added, the devices might never probe.
For example, if device-C is a consumer of device-S and they have
phandles to each other in DT, the following could happen:
1. Device-S get added first.
2. The bus add_links() callback will (incorrectly) try to link it as
a consumer of device-C.
3. Since device-C isn't present, device-S will be put in
"waiting-for-supplier" list.
4. Device-C gets added next.
5. All devices in "waiting-for-supplier" list are retried for linking.
6. Device-S gets linked as consumer to Device-C.
7. The bus add_links() callback will (correctly) try to link it as
a consumer of device-S.
8. This isn't allowed because it would create a cyclic device links.
Neither devices will get probed since the supplier is marked as
dependent on the consumer. And the consumer will never probe because the
consumer can't get resources from the supplier.
Without this patch, things stay in this broken state. However, with this
patch, the execution will continue like this:
9. Device-C's driver is loaded.
10. Device-C's driver removes Device-S as a consumer of Device-C.
11. Device-C's driver adds Device-C as a consumer of Device-S.
12. Device-S probes.
14. Device-C probes.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/base/core.c | 24 ++++++++++++++++++++++--
drivers/base/dd.c | 29 +++++++++++++++++++++++++++++
include/linux/device.h | 18 ++++++++++++++++++
3 files changed, 69 insertions(+), 2 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0705926d362f..6eb14093e905 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -422,6 +422,19 @@ static void device_link_wait_for_supplier(struct device *consumer)
mutex_unlock(&wfs_lock);
}
+/**
+ * device_link_remove_from_wfs - Unmark device as waiting for supplier
+ * @consumer: Consumer device
+ *
+ * Unmark the consumer device as waiting for suppliers to become available.
+ */
+void device_link_remove_from_wfs(struct device *consumer)
+{
+ mutex_lock(&wfs_lock);
+ list_del_init(&consumer->links.needs_suppliers);
+ mutex_unlock(&wfs_lock);
+}
+
/**
* device_link_check_waiting_consumers - Try to unmark waiting consumers
*
@@ -439,12 +452,19 @@ static void device_link_wait_for_supplier(struct device *consumer)
static void device_link_check_waiting_consumers(void)
{
struct device *dev, *tmp;
+ int ret;
mutex_lock(&wfs_lock);
list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
- links.needs_suppliers)
- if (!dev->bus->add_links(dev))
+ links.needs_suppliers) {
+ ret = 0;
+ if (dev->has_edit_links)
+ ret = driver_edit_links(dev);
+ else if (dev->bus->add_links)
+ ret = dev->bus->add_links(dev);
+ if (!ret)
list_del_init(&dev->links.needs_suppliers);
+ }
mutex_unlock(&wfs_lock);
}
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 0df9b4461766..842fc7b704f9 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -659,6 +659,12 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
drv->bus->name, __func__, dev_name(dev), drv->name);
+ if (drv->edit_links) {
+ if (drv->edit_links(dev))
+ dev->has_edit_links = true;
+ else
+ device_link_remove_from_wfs(dev);
+ }
pm_runtime_get_suppliers(dev);
if (dev->parent)
pm_runtime_get_sync(dev->parent);
@@ -747,6 +753,29 @@ struct device_attach_data {
bool have_async;
};
+static int __driver_edit_links(struct device_driver *drv, void *data)
+{
+ struct device *dev = data;
+
+ if (!drv->edit_links)
+ return 0;
+
+ if (driver_match_device(drv, dev) <= 0)
+ return 0;
+
+ return drv->edit_links(dev);
+}
+
+int driver_edit_links(struct device *dev)
+{
+ int ret;
+
+ device_lock(dev);
+ ret = bus_for_each_drv(dev->bus, NULL, dev, __driver_edit_links);
+ device_unlock(dev);
+ return ret;
+}
+
static int __device_attach_driver(struct device_driver *drv, void *_data)
{
struct device_attach_data *data = _data;
diff --git a/include/linux/device.h b/include/linux/device.h
index 7f8ae7e5fc6b..a2d509a36ebe 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -254,6 +254,20 @@ enum probe_type {
* @probe_type: Type of the probe (synchronous or asynchronous) to use.
* @of_match_table: The open firmware table.
* @acpi_match_table: The ACPI match table.
+ * @edit_links: Called to allow a matched driver to edit the device links the
+ * bus might have added incorrectly. This will be useful to handle
+ * cases where the bus incorrectly adds functional dependencies
+ * that aren't true or tries to create cyclic dependencies. But
+ * doesn't correctly handle functional dependencies that are
+ * missed by the bus as the supplier's sync_state might get to
+ * execute before the driver for a missing consumer is loaded and
+ * gets to edit the device links for the consumer.
+ *
+ * This function might be called multiple times after a new device
+ * is added. The function is expected to create all the device
+ * links for the new device and return 0 if it was completed
+ * successfully or return an error if it needs to be reattempted
+ * in the future.
* @probe: Called to query the existence of a specific device,
* whether this driver can work with it, and bind the driver
* to a specific device.
@@ -293,6 +307,7 @@ struct device_driver {
const struct of_device_id *of_match_table;
const struct acpi_device_id *acpi_match_table;
+ int (*edit_links)(struct device *dev);
int (*probe) (struct device *dev);
int (*remove) (struct device *dev);
void (*shutdown) (struct device *dev);
@@ -1065,6 +1080,7 @@ struct device {
bool offline_disabled:1;
bool offline:1;
bool of_node_reused:1;
+ bool has_edit_links:1;
#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
@@ -1314,6 +1330,7 @@ extern int __must_check device_attach(struct device *dev);
extern int __must_check driver_attach(struct device_driver *drv);
extern void device_initial_probe(struct device *dev);
extern int __must_check device_reprobe(struct device *dev);
+extern int driver_edit_links(struct device *dev);
extern bool device_is_bound(struct device *dev);
@@ -1404,6 +1421,7 @@ struct device_link *device_link_add(struct device *consumer,
struct device *supplier, u32 flags);
void device_link_del(struct device_link *link);
void device_link_remove(void *consumer, struct device *supplier);
+void device_link_remove_from_wfs(struct device *consumer);
#ifndef dev_fmt
#define dev_fmt(fmt) fmt
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* [PATCH v6 1/7] driver core: Add support for linking devices during device addition
From: Saravana Kannan @ 2019-07-20 6:16 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
In-Reply-To: <20190720061647.234852-1-saravanak@google.com>
When devices are added, the bus might want to create device links to track
functional dependencies between supplier and consumer devices. This
tracking of supplier-consumer relationship allows optimizing device probe
order and tracking whether all consumers of a supplier are active. The
add_links bus callback is added to support this.
However, when consumer devices are added, they might not have a supplier
device to link to despite needing mandatory resources/functionality from
one or more suppliers. A waiting_for_suppliers list is created to track
such consumers and retry linking them when new devices get added.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/base/core.c | 83 ++++++++++++++++++++++++++++++++++++++++++
include/linux/device.h | 8 ++++
2 files changed, 91 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index fd7511e04e62..0705926d362f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -44,6 +44,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
#endif
/* Device links support. */
+static LIST_HEAD(wait_for_suppliers);
+static DEFINE_MUTEX(wfs_lock);
#ifdef CONFIG_SRCU
static DEFINE_MUTEX(device_links_lock);
@@ -401,6 +403,51 @@ struct device_link *device_link_add(struct device *consumer,
}
EXPORT_SYMBOL_GPL(device_link_add);
+/**
+ * device_link_wait_for_supplier - Mark device as waiting for supplier
+ * @consumer: Consumer device
+ *
+ * Marks the consumer device as waiting for suppliers to become available. The
+ * consumer device will never be probed until it's unmarked as waiting for
+ * suppliers. The caller is responsible for adding the link to the supplier
+ * once the supplier device is present.
+ *
+ * This function is NOT meant to be called from the probe function of the
+ * consumer but rather from code that creates/adds the consumer device.
+ */
+static void device_link_wait_for_supplier(struct device *consumer)
+{
+ mutex_lock(&wfs_lock);
+ list_add_tail(&consumer->links.needs_suppliers, &wait_for_suppliers);
+ mutex_unlock(&wfs_lock);
+}
+
+/**
+ * device_link_check_waiting_consumers - Try to unmark waiting consumers
+ *
+ * Loops through all consumers waiting on suppliers and tries to add all their
+ * supplier links. If that succeeds, the consumer device is unmarked as waiting
+ * for suppliers. Otherwise, they are left marked as waiting on suppliers,
+ *
+ * The add_links bus callback is expected to return 0 if it has found and added
+ * all the supplier links for the consumer device. It should return an error if
+ * it isn't able to do so.
+ *
+ * The caller of device_link_wait_for_supplier() is expected to call this once
+ * it's aware of potential suppliers becoming available.
+ */
+static void device_link_check_waiting_consumers(void)
+{
+ struct device *dev, *tmp;
+
+ mutex_lock(&wfs_lock);
+ list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
+ links.needs_suppliers)
+ if (!dev->bus->add_links(dev))
+ list_del_init(&dev->links.needs_suppliers);
+ mutex_unlock(&wfs_lock);
+}
+
static void device_link_free(struct device_link *link)
{
while (refcount_dec_not_one(&link->rpm_active))
@@ -535,6 +582,19 @@ int device_links_check_suppliers(struct device *dev)
struct device_link *link;
int ret = 0;
+ /*
+ * If a device is waiting for one or more suppliers (in
+ * wait_for_suppliers list), it is not ready to probe yet. So just
+ * return -EPROBE_DEFER without having to check the links with existing
+ * suppliers.
+ */
+ mutex_lock(&wfs_lock);
+ if (!list_empty(&dev->links.needs_suppliers)) {
+ mutex_unlock(&wfs_lock);
+ return -EPROBE_DEFER;
+ }
+ mutex_unlock(&wfs_lock);
+
device_links_write_lock();
list_for_each_entry(link, &dev->links.suppliers, c_node) {
@@ -812,6 +872,10 @@ static void device_links_purge(struct device *dev)
{
struct device_link *link, *ln;
+ mutex_lock(&wfs_lock);
+ list_del(&dev->links.needs_suppliers);
+ mutex_unlock(&wfs_lock);
+
/*
* Delete all of the remaining links from this device to any other
* devices (either consumers or suppliers).
@@ -1673,6 +1737,7 @@ void device_initialize(struct device *dev)
#endif
INIT_LIST_HEAD(&dev->links.consumers);
INIT_LIST_HEAD(&dev->links.suppliers);
+ INIT_LIST_HEAD(&dev->links.needs_suppliers);
dev->links.status = DL_DEV_NO_DRIVER;
}
EXPORT_SYMBOL_GPL(device_initialize);
@@ -2108,6 +2173,24 @@ int device_add(struct device *dev)
BUS_NOTIFY_ADD_DEVICE, dev);
kobject_uevent(&dev->kobj, KOBJ_ADD);
+
+ /*
+ * Check if any of the other devices (consumers) have been waiting for
+ * this device (supplier) to be added so that they can create a device
+ * link to it.
+ *
+ * This needs to happen after device_pm_add() because device_link_add()
+ * requires the supplier be registered before it's called.
+ *
+ * But this also needs to happe before bus_probe_device() to make sure
+ * waiting consumers can link to it before the driver is bound to the
+ * device and the driver sync_state callback is called for this device.
+ */
+ device_link_check_waiting_consumers();
+
+ if (dev->bus && dev->bus->add_links && dev->bus->add_links(dev))
+ device_link_wait_for_supplier(dev);
+
bus_probe_device(dev);
if (parent)
klist_add_tail(&dev->p->knode_parent,
diff --git a/include/linux/device.h b/include/linux/device.h
index 848fc71c6ba6..7f8ae7e5fc6b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -77,6 +77,11 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
* -EPROBE_DEFER it will queue the device for deferred probing.
* @uevent: Called when a device is added, removed, or a few other things
* that generate uevents to add the environment variables.
+ * @add_links: Called, perhaps multiple times, when a new device is added to
+ * this bus. The function is expected to create all the device
+ * links for the new device and return 0 if it was completed
+ * successfully or return an error if it needs to be reattempted
+ * in the future.
* @probe: Called when a new device or driver add to this bus, and callback
* the specific driver's probe to initial the matched device.
* @remove: Called when a device removed from this bus.
@@ -121,6 +126,7 @@ struct bus_type {
int (*match)(struct device *dev, struct device_driver *drv);
int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
+ int (*add_links)(struct device *dev);
int (*probe)(struct device *dev);
int (*remove)(struct device *dev);
void (*shutdown)(struct device *dev);
@@ -888,11 +894,13 @@ enum dl_dev_state {
* struct dev_links_info - Device data related to device links.
* @suppliers: List of links to supplier devices.
* @consumers: List of links to consumer devices.
+ * @needs_suppliers: Hook to global list of devices waiting for suppliers.
* @status: Driver status information.
*/
struct dev_links_info {
struct list_head suppliers;
struct list_head consumers;
+ struct list_head needs_suppliers;
enum dl_dev_state status;
};
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply related
* [PATCH v6 0/7] Solve postboot supplier cleanup and optimize probe ordering
From: Saravana Kannan @ 2019-07-20 6:16 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand
Cc: Saravana Kannan, devicetree, linux-kernel, David Collins,
kernel-team
Add device-links to track functional dependencies between devices
after they are created (but before they are probed) by looking at
their common DT bindings like clocks, interconnects, etc.
Having functional dependencies automatically added before the devices
are probed, provides the following benefits:
- Optimizes device probe order and avoids the useless work of
attempting probes of devices that will not probe successfully
(because their suppliers aren't present or haven't probed yet).
For example, in a commonly available mobile SoC, registering just
one consumer device's driver at an initcall level earlier than the
supplier device's driver causes 11 failed probe attempts before the
consumer device probes successfully. This was with a kernel with all
the drivers statically compiled in. This problem gets a lot worse if
all the drivers are loaded as modules without direct symbol
dependencies.
- Supplier devices like clock providers, interconnect providers, etc
need to keep the resources they provide active and at a particular
state(s) during boot up even if their current set of consumers don't
request the resource to be active. This is because the rest of the
consumers might not have probed yet and turning off the resource
before all the consumers have probed could lead to a hang or
undesired user experience.
Some frameworks (Eg: regulator) handle this today by turning off
"unused" resources at late_initcall_sync and hoping all the devices
have probed by then. This is not a valid assumption for systems with
loadable modules. Other frameworks (Eg: clock) just don't handle
this due to the lack of a clear signal for when they can turn off
resources. This leads to downstream hacks to handle cases like this
that can easily be solved in the upstream kernel.
By linking devices before they are probed, we give suppliers a clear
count of the number of dependent consumers. Once all of the
consumers are active, the suppliers can turn off the unused
resources without making assumptions about the number of consumers.
By default we just add device-links to track "driver presence" (probe
succeeded) of the supplier device. If any other functionality provided
by device-links are needed, it is left to the consumer/supplier
devices to change the link when they probe.
v1 -> v2:
- Drop patch to speed up of_find_device_by_node()
- Drop depends-on property and use existing bindings
v2 -> v3:
- Refactor the code to have driver core initiate the linking of devs
- Have driver core link consumers to supplier before it's probed
- Add support for drivers to edit the device links before probing
v3 -> v4:
- Tested edit_links() on system with cyclic dependency. Works.
- Added some checks to make sure device link isn't attempted from
parent device node to child device node.
- Added way to pause/resume sync_state callbacks across
of_platform_populate().
- Recursively parse DT node to create device links from parent to
suppliers of parent and all child nodes.
v4 -> v5:
- Fixed copy-pasta bugs with linked list handling
- Walk up the phandle reference till I find an actual device (needed
for regulators to work)
- Added support for linking devices from regulator DT bindings
- Tested the whole series again to make sure cyclic dependencies are
broken with edit_links() and regulator links are created properly.
v5 -> v6:
- Split, squashed and reordered some of the patches.
- Refactored the device linking code to follow the same code pattern for
any property.
I've also not updated this patch series to handle the new patch [1] from
Rafael. Will do that once this patch series is close to being Acked.
[1] - https://lore.kernel.org/lkml/3121545.4lOhFoIcdQ@kreacher/
-Saravana
Saravana Kannan (7):
driver core: Add support for linking devices during device addition
driver core: Add edit_links() callback for drivers
of/platform: Add functional dependency link from DT bindings
driver core: Add sync_state driver/bus callback
of/platform: Pause/resume sync state during init and
of_platform_populate()
of/platform: Create device links for all child-supplier depencencies
of/platform: Don't create device links for default busses
.../admin-guide/kernel-parameters.txt | 5 +
drivers/base/core.c | 168 ++++++++++++++++
drivers/base/dd.c | 29 +++
drivers/of/platform.c | 182 ++++++++++++++++++
include/linux/device.h | 47 +++++
5 files changed, 431 insertions(+)
--
2.22.0.657.g960e92d24f-goog
^ permalink raw reply
* [Question] orphan platform data header
From: Masahiro Yamada @ 2019-07-20 3:25 UTC (permalink / raw)
To: Linux Kernel Mailing List
Cc: DTML, Arnd Bergmann, Greg Kroah-Hartman, masahiroy,
Linus Torvalds, linux-arm-kernel
Hi.
I see several platform-data headers
that are not used in upstream.
For instance, please look at this driver:
drivers/leds/leds-netxbig.c
If I understood it correctly, this driver
supports both device tree and legacy board-file.
I grepped 'netxbig_led_platform_data', but
I only found the driver and platform_data header.
No board-file in upstream.
masahiro@grover:~/ref/linux$ git grep netxbig_led_platform_data
drivers/leds/leds-netxbig.c: struct
netxbig_led_platform_data *pdata,
drivers/leds/leds-netxbig.c: struct
netxbig_led_platform_data *pdata)
drivers/leds/leds-netxbig.c: struct
netxbig_led_platform_data *pdata)
drivers/leds/leds-netxbig.c: struct netxbig_led_platform_data
*pdata = dev_get_platdata(&pdev->dev);
include/linux/platform_data/leds-kirkwood-netxbig.h:struct
netxbig_led_platform_data {
So, what shall we do?
Drop the board-file support? Or, keep it
in case somebody is still using their board-files
in downstream?
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [TRIVIAL PATCH] of: per-file dtc preprocessor flags
From: Masahiro Yamada @ 2019-07-20 2:40 UTC (permalink / raw)
To: Eugeniy Paltsev
Cc: Linux Kbuild mailing list, Michal Marek,
Linux Kernel Mailing List, DTML, arcml, Vineet Gupta,
Alexey Brodkin
In-Reply-To: <20190719192824.24639-1-Eugeniy.Paltsev@synopsys.com>
On Sat, Jul 20, 2019 at 4:28 AM Eugeniy Paltsev
<Eugeniy.Paltsev@synopsys.com> wrote:
>
> As of today we are able to pass custom flags to dtc compiler but not
> to dtc preprocessor.
> This ability is required for including some board-specific header files.
> It allows us to pass defined constants to dts without their duplication
> in several places.
How to use this option in a useful way?
I see a bunch of defined constants under include/dt-bindings/.
If you are talking about code duplication across architectures,
you can include arm dtsi from arm64 dts, or vice versa.
This was made possible by the symbolic links
in scripts/dtc/include-prefixes/.
Could you please elaborate your issues if you cannot solve them
by the current strategy?
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
> scripts/Makefile.lib | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index f1f38c8cdc74..f2595a608dce 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -166,6 +166,8 @@ dtc_cpp_flags = -Wp,-MD,$(depfile).pre.tmp -nostdinc \
> $(addprefix -I,$(DTC_INCLUDE)) \
> -undef -D__DTS__
>
> +dtc_cpp_flags += $(DTC_CPP_FLAGS_$(basetarget))
> +
> # Useful for describing the dependency of composite objects
> # Usage:
> # $(call multi_depend, multi_used_targets, suffix_to_remove, suffix_to_add)
> --
> 2.21.0
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH 4/4][V2] dt-bindings: iio: imu: add bindings for ADIS16460
From: Rob Herring @ 2019-07-19 22:49 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: open list:IIO SUBSYSTEM AND DRIVERS, linux-spi, devicetree,
linux-kernel@vger.kernel.org, Jonathan Cameron, Mark Rutland,
Mark Brown
In-Reply-To: <20190717115109.15168-5-alexandru.ardelean@analog.com>
On Wed, Jul 17, 2019 at 5:51 AM Alexandru Ardelean
<alexandru.ardelean@analog.com> wrote:
>
> This change adds device-tree bindings for the ADIS16460.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> .../bindings/iio/imu/adi,adis16460.yaml | 53 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 54 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* dt-bindings: gpio: aspeed: Add SGPIO support
From: Hongwei Zhang @ 2019-07-19 20:37 UTC (permalink / raw)
To: hongweiz, Andrew Jeffery, Linus Walleij, linux-gpio
Cc: Joel Stanley, devicetree, linux-aspeed, Bartosz Golaszewski,
Rob Herring, Mark Rutland, linux-kernel, linux-arm-kernel
In-Reply-To: <1563394325-15941-1-git-send-email-hongweiz@ami.com>
Hello Andrew,
Thanks for reviewing and please see my inline comments.
--Hongwei
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Wednesday, July 17, 2019 9:48 PM
> To: Hongwei Zhang; Joel Stanley; Linus Walleij; devicetree@vger.kernel.org
> Cc: Rob Herring; Mark Rutland; Bartosz Golaszewski; linux-aspeed@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-gpio@vger.kernel.org
> Subject: Re: [PATCH 2/3 v4] dt-bindings: gpio: aspeed: Add SGPIO support
>
> The subject is largely correct, but please see the discussion on the driver patch about how to clean up
> the [PATCH ...] prefix.
>
> On Thu, 18 Jul 2019, at 05:42, Hongwei Zhang wrote:
> > Add bindings to support SGPIO on AST2400 or AST2500.
> >
> > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > ---
> > .../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
> > 1 file changed, 55 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > new file mode 100644
> > index 0000000..2d6305e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > @@ -0,0 +1,55 @@
> > +Aspeed SGPIO controller Device Tree Bindings
> > +-------------------------------------------
> > +
> > +This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80
> > full
> > +featured Serial GPIOs. Each of the Serial GPIO pins can be programmed
> > to
> > +support the following options:
> > +- Support interrupt option for each input port and various interrupt
> > + sensitivity option (level-high, level-low, edge-high, edge-low)
> > +- Support reset tolerance option for each output port
> > +- Directly connected to APB bus and its shift clock is from APB bus
> > clock
> > + divided by a programmable value.
> > +- Co-work with external signal-chained TTL components
> > +(74LV165/74LV595)
> > +
> > +
> > +Required properties:
> > +
> > +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> > +
> > +- #gpio-cells : Should be two
> > + - First cell is the GPIO line number
> > + - Second cell is used to specify optional
> > + parameters (unused)
> > +
> > +- reg : Address and length of the register set for the device
> > +- gpio-controller : Marks the device node as a GPIO controller
> > +- interrupts : Interrupt specifier (see interrupt bindings for
> > + details)
> > +
> > +- interrupt-controller : Mark the GPIO controller as an
> > interrupt-controller
> > +
> > +- nr-gpios : number of GPIO pins to serialise.
> > + (should be multiple of 8, up to 80 pins)
>
> Please change the property name to "ngpios", as per the generic GPIO bindings[1].
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindi
> ngs/gpio/gpio.txt?h=v5.2#n141
done
>
> Cheers,
>
> Andrew
>
> > +
> > +- clocks : A phandle to the APB clock for SGPM clock
> > division
> > +
> > +- bus-frequency : SGPM CLK frequency
> > +
> > +
> > +The sgpio and interrupt properties are further described in their
> > respective bindings documentation:
> > +
> > +- Documentation/devicetree/bindings/sgpio/gpio.txt
> > +-
> > +Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> > +
> > + Example:
> > + sgpio: sgpio@1e780200 {
> > + #gpio-cells = <2>;
> > + compatible = "aspeed,ast2500-sgpio";
> > + gpio-controller;
> > + interrupts = <40>;
> > + reg = <0x1e780200 0x0100>;
> > + clocks = <&syscon ASPEED_CLK_APB>;
> > + interrupt-controller;
> > + nr-gpios = <8>;
> > + bus-frequency = <12000000>;
> > + };
> > --
> > 2.7.4
> >
> >
^ permalink raw reply
* [PATCH 2/2] arm64: dts: sdm845: Redefine interconnect provider DT nodes
From: David Dai @ 2019-07-19 20:32 UTC (permalink / raw)
To: georgi.djakov, bjorn.andersson, robh+dt
Cc: David Dai, evgreen, ilina, seansw, elder, linux-kernel,
devicetree, linux-arm-msm, linux-pm
In-Reply-To: <1563568344-1274-1-git-send-email-daidavid1@codeaurora.org>
Add the DT nodes for each of the Network-On-Chip interconnect
buses found on SDM845 based platform and redefine the rsc_hlos
child node as a bcm-voter device to better represent the hardware.
Signed-off-by: David Dai <daidavid1@codeaurora.org>
---
arch/arm64/boot/dts/qcom/sdm845.dtsi | 61 ++++++++++++++++++++++++++++++++++--
1 file changed, 58 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index e7d78bc..204222e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -610,6 +610,62 @@
#power-domain-cells = <1>;
};
+ aggre1_noc: interconnect@16e0000 {
+ compatible = "qcom,sdm845-aggre1_noc";
+ reg = <0 0x16e0000 0 0xd080>;
+ #interconnect-cells = <1>;
+ qcom,bcm-voter = <&apps_bcm_voter>;
+ };
+
+ aggre2_noc: interconnect@1700000 {
+ compatible = "qcom,sdm845-aggre2_noc";
+ reg = <0 0x1700000 0 0x3b100>;
+ #interconnect-cells = <1>;
+ qcom,bcm-voter = <&apps_bcm_voter>;
+ };
+
+ config_noc: interconnect@1500000 {
+ compatible = "qcom,sdm845-config_noc";
+ reg = <0 0x1500000 0 0x5080>;
+ #interconnect-cells = <1>;
+ qcom,bcm-voter = <&apps_bcm_voter>;
+ };
+
+ dc_noc: interconnect@14e0000 {
+ compatible = "qcom,sdm845-dc_noc";
+ reg = <0 0x14e0000 0 0x400>;
+ #interconnect-cells = <1>;
+ qcom,bcm-voter = <&apps_bcm_voter>;
+ };
+
+ gladiator_noc: interconnect@17900000 {
+ compatible = "qcom,sdm845-gladiator_noc";
+ reg = <0 0x17900000 0 0xd080>;
+ #interconnect-cells = <1>;
+ qcom,bcm-voter = <&apps_bcm_voter>;
+ };
+
+ mem_noc: interconnect@1380000 {
+ compatible = "qcom,sdm845-mem_noc";
+ reg = <0 0x1380000 0 0x27200>;
+ #interconnect-cells = <1>;
+ qcom,bcm-voter = <&apps_bcm_voter>;
+ };
+
+ mmss_noc: interconnect@1740000 {
+ compatible = "qcom,sdm845-mmss_noc";
+ reg = <0 0x1740000 0 0x1c1000>;
+ #interconnect-cells = <1>;
+ qcom,bcm-voter = <&apps_bcm_voter>;
+ };
+
+ system_noc: interconnect@1620000 {
+ compatible = "qcom,sdm845-system_noc";
+ reg = <0 0x1620000 0 0x18080>;
+ #interconnect-cells = <1>;
+ qcom,bcm-voter = <&apps_bcm_voter>;
+ };
+
qfprom@784000 {
compatible = "qcom,qfprom";
reg = <0 0x00784000 0 0x8ff>;
@@ -2801,9 +2857,8 @@
};
};
- rsc_hlos: interconnect {
- compatible = "qcom,sdm845-rsc-hlos";
- #interconnect-cells = <1>;
+ apps_bcm_voter: bcm_voter {
+ compatible = "qcom,sdm845-bcm-voter";
};
};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* [PATCH 1/2] dt-bindings: interconnect: Update Qualcomm SDM845 DT bindings
From: David Dai @ 2019-07-19 20:32 UTC (permalink / raw)
To: georgi.djakov, bjorn.andersson, robh+dt
Cc: David Dai, evgreen, ilina, seansw, elder, linux-kernel,
devicetree, linux-arm-msm, linux-pm
In-Reply-To: <1563568344-1274-1-git-send-email-daidavid1@codeaurora.org>
Redefine the Network-on-Chip devices to more accurately describe
the interconnect topology on Qualcomm's SDM845 platform. Each
interconnect device can communicate with different instances of the
RPMh hardware which are described as RSCs(Resource State Coordinators).
Signed-off-by: David Dai <daidavid1@codeaurora.org>
---
.../bindings/interconnect/qcom,bcm-voter.txt | 32 +++++++++++++++++
.../bindings/interconnect/qcom,sdm845.txt | 40 +++++++++++++++++-----
2 files changed, 63 insertions(+), 9 deletions(-)
create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,bcm-voter.txt
diff --git a/Documentation/devicetree/bindings/interconnect/qcom,bcm-voter.txt b/Documentation/devicetree/bindings/interconnect/qcom,bcm-voter.txt
new file mode 100644
index 0000000..2cf7da2
--- /dev/null
+++ b/Documentation/devicetree/bindings/interconnect/qcom,bcm-voter.txt
@@ -0,0 +1,32 @@
+Qualcomm BCM-Voter interconnect driver binding
+-----------------------------------------------------------
+
+The Bus Clock Manager (BCM) is a dedicated hardware accelerator
+that manages shared system resources by aggregating requests
+from multiple Resource State Coordinators (RSC). Interconnect
+providers are able to vote for aggregated thresholds values from
+consumers by communicating through their respective RSCs.
+
+Required properties :
+- compatible : shall contain only one of the following:
+ "qcom,sdm845-bcm-voter",
+
+Examples:
+
+apps_rsc: rsc@179c0000 {
+ label = "apps_rsc";
+ compatible = "qcom,rpmh-rsc";
+
+ apps_bcm_voter: bcm_voter {
+ compatible = "qcom,sdm845-bcm-voter";
+ };
+}
+
+disp_rsc: rsc@179d0000 {
+ label = "disp_rsc";
+ compatible = "qcom,rpmh-rsc";
+
+ disp_bcm_voter: bcm_voter {
+ compatible = "qcom,sdm845-bcm-voter";
+ };
+}
diff --git a/Documentation/devicetree/bindings/interconnect/qcom,sdm845.txt b/Documentation/devicetree/bindings/interconnect/qcom,sdm845.txt
index 5c4f1d9..27f9ed9 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,sdm845.txt
+++ b/Documentation/devicetree/bindings/interconnect/qcom,sdm845.txt
@@ -4,21 +4,43 @@ Qualcomm SDM845 Network-On-Chip interconnect driver binding
SDM845 interconnect providers support system bandwidth requirements through
RPMh hardware accelerators known as Bus Clock Manager (BCM). The provider is
able to communicate with the BCM through the Resource State Coordinator (RSC)
-associated with each execution environment. Provider nodes must reside within
-an RPMh device node pertaining to their RSC and each provider maps to a single
-RPMh resource.
+associated with each execution environment. Provider nodes must point to at
+least one RPMh device child node pertaining to their RSC and each provider
+can map to multiple RPMh resources.
Required properties :
- compatible : shall contain only one of the following:
- "qcom,sdm845-rsc-hlos"
+ "qcom,sdm845-aggre1_noc",
+ "qcom,sdm845-aggre2_noc",
+ "qcom,sdm845-config_noc",
+ "qcom,sdm845-dc_noc",
+ "qcom,sdm845-gladiator_noc",
+ "qcom,sdm845-mem_noc",
+ "qcom,sdm845-mmss_noc",
+ "qcom,sdm845-system_noc",
- #interconnect-cells : should contain 1
+- reg : shall contain base register location and length
+- qcom,bcm-voter : shall contain phandles to bcm voters
Examples:
-apps_rsc: rsc {
- rsc_hlos: interconnect {
- compatible = "qcom,sdm845-rsc-hlos";
- #interconnect-cells = <1>;
- };
+aggre1_noc: interconnect@16e0000 {
+ compatible = "qcom,sdm845-aggre1_noc";
+ reg = <0x16e0000 0xd080>;
+ interconnect-cells = <1>;
+ qcom,bcm-voter = <&apps_bcm_voter>;
};
+mmss_noc: interconnect@1740000 {
+ compatible = "qcom,sdm845-mmss_noc";
+ reg = <0x1740000 0x1c1000>;
+ interconnect-cells = <1>;
+ qcom,bcm-voter = <&apps_bcm_voter>, <&disp_bcm_voter>;
+};
+
+mem_noc: interconnect@1380000 {
+ compatible = "qcom,sdm845-mem_noc";
+ reg = <0 0x1380000 0 0x27200>;
+ #interconnect-cells = <1>;
+ qcom,bcm-voter = <&apps_bcm_voter>, <&disp_bcm_voter>;
+};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* [PATCH 0/2] Redefine interconnect provider DT nodes for SDM845
From: David Dai @ 2019-07-19 20:32 UTC (permalink / raw)
To: georgi.djakov, bjorn.andersson, robh+dt
Cc: David Dai, evgreen, ilina, seansw, elder, linux-kernel,
devicetree, linux-arm-msm, linux-pm
Redefine the SDM845 interconnect device nodes as the previous definitions
of using a single child node under the apps_rsc device did not accurately
capture the description of the hardware. The Network-On-Chip (NoC) interconnect
devices should be represented in a manner akin to QCS404 platforms[1]
where there is a separation of NoC devices and its RPM/RPMh counterparts.
The bcm-voter devices are representing the RPMh devices that the interconnect
providers need to communicate with and there can be more than one instance of
the Bus Clock Manager (BCM) which can live under different instances of Resource
State Coordinators (RSC). There are display use cases where consumers may need
to target a different bcm-voter (Some display specific RSC) than the default,
and there needs to be a way to represent this connection in devicetree.
[1]: https://lkml.org/lkml/2019/6/13/143
David Dai (2):
dt-bindings: interconnect: Update Qualcomm SDM845 DT bindings
arm64: dts: sdm845: Redefine interconnect provider DT nodes
.../bindings/interconnect/qcom,bcm-voter.txt | 32 ++++++++++++
.../bindings/interconnect/qcom,sdm845.txt | 40 ++++++++++----
arch/arm64/boot/dts/qcom/sdm845.dtsi | 61 ++++++++++++++++++++--
3 files changed, 121 insertions(+), 12 deletions(-)
create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,bcm-voter.txt
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v4 2/2] EDAC: add EDAC driver for DMC520
From: Lei Wang @ 2019-07-19 19:49 UTC (permalink / raw)
To: James Morse
Cc: mark.rutland@arm.com, bp@alien8.de, robh+dt@kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
mchehab@kernel.org, linux-edac@vger.kernel.org, sashal@kernel.org,
hangl@microsoft.com, lewan@microsoft.com, ruizhao@microsoft.com
In-Reply-To: <d82e6187-4a3d-1dc2-298a-0748ef0a7136@arm.com>
Hi James,
> Bingo: 'format_control' can be read in all states and has a 'memory_width' field that
> indicates if the PHY is connected to a 'x32 DDR device' or 'x64 DDR device'. (I'm guessing
> those double bit values are for address + data)
>
> This probably affects the 'grain' too.
>
Thanks a lot for this finding! I double-checked with brcm and yes this
can be used to decide the memory data width.
>>>> +static void dmc520_handle_dram_ecc_errors(struct mem_ctl_info *mci,
>>>> + bool is_ce)
>>>> +{
>>>> + struct ecc_error_info info;
>>>> + struct dmc520_edac *edac;
>>>> + u32 cnt;
>>>> + char message[EDAC_MSG_BUF_SIZE];
>>>> + unsigned long flags;
>>>> +
>>>> + edac = mci->pvt_info;
>>>> + dmc520_get_dram_ecc_error_info(edac, is_ce, &info);
>>>> +
>>>> + cnt = dmc520_get_dram_ecc_error_count(edac, is_ce);
>>>> +
>>>> + if (cnt > 0) {
>>>> + snprintf(message, ARRAY_SIZE(message),
>>>> + "rank:%d bank:%d row:%d col:%d",
>>>> + info.rank, info.bank,
>>>> + info.row, info.col);
>>>> +
>>>> + spin_lock_irqsave(&edac->ecc_lock, flags);
>>> irqsave/irqrestore is overkill as this function is only called from an interrupt handler.
>>> There is no way for this to be called with interrupts unmasked.
>
>> Still feel spin_lock_irqsave and spin_unlock_irqstore are the safest. If
>> a processor is on isr for interrupt line 1 and calls
>> dmc520_handle_dram_ecc_errors, only line 1 is disabled and and other
>> lines can still interrupt calling dmc520_handle_dram_ecc_errors again.
>
> But not on the same CPU, (which is the problem the irqsave helpers solve).
>
> When the CPU takes an interrupt the hardware sets a status bit to prevent this CPU taking
> any other interrupt. On armv8, this is PSTATE.I, its set automatically by the hardware. If
> CPUs didn't do this, you could never guarantee that any interrupt would ever be handled.
>
> You are right a second interrupt may occur, but it can't interrupt this CPU until it
> returns from the Interrupt, or clears the status bit. If the second interrupt is taken at
> the same time its because it was routed to a different CPU. A regular spin_lock() stops
> any problems here, the second CPU has to wait for the first CPU to release the lock.
>
> spin_lock_irqsave() is for a more complicated problem. If you used the spinlock in process
> context, (e.g. your probe function), as well as interrupt context, then its possible the
> interrupt is taken while the lock is held in process context on the same CPU. This causes
> your interrupt handler to wait forever for the lock. The irqsave helpers stop this by
> masking interrupts when taking the lock, so that this 'same CPU' sequence can't happen.
>
> Because you don't take the lock in process context, you don't need the irqsave variants.
>
Thanks! I overlooked the fact that aarch64's default behavior for
interrupt handling is that further interrupts are disabled on the same
CPU in interrupt context. Found the arm doc for it. :)
https://developer.arm.com/docs/100933/latest/interrupt-handling. With
this, yes I'll switch to normal spin_lock instead of irq version's
spin_lock API's.
Just for my education: if in a more general condition when the same CPU
supports embedded interrupts, are the spin_lock_irqsave and restore
always the recommended if used in an interrupt context?
>
> (N.B: your mail is still coming base64 encoded, so its very unlikely the maintainer can
> pick it up.)
>
Ah I found if without explicit "--transfer-encoding=7bit" when do "git
send-mail", my ubuntu box sent out base64 by default. I'll make sure to
explicitly setting 7bit sending new patches.
Thanks,
-Lei
^ permalink raw reply
* [TRIVIAL PATCH] of: per-file dtc preprocessor flags
From: Eugeniy Paltsev @ 2019-07-19 19:28 UTC (permalink / raw)
To: linux-kbuild, Masahiro Yamada, Michal Marek
Cc: linux-kernel, devicetree, linux-snps-arc, Vineet Gupta,
Alexey Brodkin, Eugeniy Paltsev
As of today we are able to pass custom flags to dtc compiler but not
to dtc preprocessor.
This ability is required for including some board-specific header files.
It allows us to pass defined constants to dts without their duplication
in several places.
Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
scripts/Makefile.lib | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index f1f38c8cdc74..f2595a608dce 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -166,6 +166,8 @@ dtc_cpp_flags = -Wp,-MD,$(depfile).pre.tmp -nostdinc \
$(addprefix -I,$(DTC_INCLUDE)) \
-undef -D__DTS__
+dtc_cpp_flags += $(DTC_CPP_FLAGS_$(basetarget))
+
# Useful for describing the dependency of composite objects
# Usage:
# $(call multi_depend, multi_used_targets, suffix_to_remove, suffix_to_add)
--
2.21.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox