Prev: Patch to gianfar.c for 2.6.33.2-rt13
Next: [GIT PULL] regulator fixes for 2.6.34-rc6
From: Jonathan Corbet on 23 Apr 2010 18:00
On Fri, 23 Apr 2010 23:12:54 +0200
Florian Tobias Schandinat <FlorianSchandinat(a)gmx.de> wrote:

> 1. while the module loads the screen shows some strange colours. The
> colours itself do not worry me but it probably at least means that the
> module load time was increased significantly (2-3 seconds, I guess)
>
> 2. most annoyingly after this patch I can no longer get a working VGA
> console after unbinding the module. Normally I just do:

Hmm...compiling the driver as a module is not really something that
makes sense on the OLPC platform, so I've never done it.

#1 is weird, there should be nothing there that slows initialization
down. Putting in some prints and looking at the timing would be most
helpful here.

As for #2, I can certainly say that I've never unloaded this code, so
that's an untested path. I'll have a look and see if I can see
anything obvious.

jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jonathan Corbet on 23 Apr 2010 19:00
On Sat, 24 Apr 2010 00:40:39 +0200
Florian Tobias Schandinat <FlorianSchandinat(a)gmx.de> wrote:

> Actually that is probably a mistake on my side. I had the impression
> that it was much longer but didn't take into account that the old
> behaviour allowed the VGA console to work until viafb was completly
> loaded and fbcon took over while the new one immediately destroys the
> screen and shows random things until it is completely loaded.

"New one" being new relative to what? Is that change the result of the
patches I've posted, or something else?

> > As for #2, I can certainly say that I've never unloaded this code, so
> > that's an untested path. I'll have a look and see if I can see
> > anything obvious.
>
> Well as for the behaviour change described above I think the problem
> might be in the load path. At least when I faked an exit as when memory
> size detection or ioremapping fails (which is a very common issue that
> really needs a workaround) I get the same unusable VGA console. This
> needs to be fixed.

Interesting. In the environment I've been working in the whole box is
a brick if the framebuffer doesn't come up right. But things are
pretty solid on that front here.

> This whole I2C stuff seems incredibly unstable I even have indicators
> that the current code might be to blame for freezing the machine on some
> configurations with P4M900 IGP. I just try to prevent it to get even
> worse...

I have to say that i2c has often been the bane of my existence. It
seems like something that just barely works most of the time.

That said, it's been a long time since I've seen any trouble I could
blame on i2c in the viafb driver. It's *really* hard to imagine how
it could free machines. Unless, maybe, you're hitting some sort of
race condition in all of those indexed I/O port operations. But that
looks like something which would be hard to do on most machines that
would have these chipsets in them.

The second series adds some locking around i2c port operations, but has
not yet pushed that locking into the framebuffer side of the driver;
that would be a good thing to do.

Meanwhile, I'm a little unsure now...is there an action item for me
with regard to the i2c code? I've been staring at it since your last
note, but I couldn't find any obvious problems. I do have to say that
Harald's rework is far cleaner than what came before...

Thanks,

jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jonathan Corbet on 24 Apr 2010 09:40
On Sat, 24 Apr 2010 12:47:34 +0200
Florian Tobias Schandinat <FlorianSchandinat(a)gmx.de> wrote:

> I was able to narrow this issue down to the fourth bus. So with
> if (i == 4)
> continue;
> in the bus creation loop I'm able to get a working framebuffer which
> does not have the issues mentioned. Any ideas what should be done now?

That is useful information indeed, thanks.

This patch creates an i2c bus on every port which could conceivably
have one. That's rather different from the original code, which
created a single bus (in the kernel) then swapped it between ports 31
and 2c in weird ways. In particular, it takes over ports which,
conceivably, somebody else could be using for GPIO. So, perhaps, there
is some contention going on there.

If my hypothesis is correct, this problem will go away with the
application of the second series, which doesn't create i2c buses on
ports used for GPIO. But the problem should be fixed here so we don't
have things broken in the middle.

Harald, does this reasoning make sense to you? If so, what I should do
is bring forward just enough of the via-core code to be able to
configure which ports actually get i2c buses created on them. Easily
done.

Thanks,

jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Harald Welte on 24 Apr 2010 11:10
Hi Jon,

On Sat, Apr 24, 2010 at 07:33:59AM -0600, Jonathan Corbet wrote:

> This patch creates an i2c bus on every port which could conceivably
> have one. That's rather different from the original code, which
> created a single bus (in the kernel) then swapped it between ports 31
> and 2c in weird ways. In particular, it takes over ports which,
> conceivably, somebody else could be using for GPIO. So, perhaps, there
> is some contention going on there.
>
> If my hypothesis is correct, this problem will go away with the
> application of the second series, which doesn't create i2c buses on
> ports used for GPIO. But the problem should be fixed here so we don't
> have things broken in the middle.
>
> Harald, does this reasoning make sense to you? If so, what I should do
> is bring forward just enough of the via-core code to be able to
> configure which ports actually get i2c buses created on them. Easily
> done.

It makes a lot of sense. When I added all possible I2C busses, this was merely
the result of "let's try to make sure every possible configuration is
supported" rather than some kind of neccessity.

Simply converting the original two busses to the new code is definitely
the way to go. Maybe we'll keep the code for the other two around, but
somehow keep them inactive and don't advertise them unless somebody explicitly
does so?

--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jonathan Corbet on 25 Apr 2010 10:40
On Sat, 24 Apr 2010 15:53:16 +0200
Harald Welte <laforge(a)gnumonks.org> wrote:

> Simply converting the original two busses to the new code is definitely
> the way to go. Maybe we'll keep the code for the other two around, but
> somehow keep them inactive and don't advertise them unless somebody explicitly
> does so?

OK, my proposal would be to add the following patch into the early part
of the series; that will help to avoid the creation of confusion in the
middle until the full i2c/gpio configuration code is in.

Look good?

Thanks,

jon

viafb: Only establish i2c busses on ports that always had them

....otherwise it seems we run into conflicts with shadowy other users which
don't expect to see i2c taking control of ports it never used to do
anything with.

Reported-by: Florian Tobias Schandinat <FlorianSchandinat(a)gmx.de>
Signed-off-by: Jonathan Corbet <corbet(a)lwn.net>
---
drivers/video/via/via_i2c.c | 19 +++++++++++++------
drivers/video/via/via_i2c.h | 1 +
2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/video/via/via_i2c.c b/drivers/video/via/via_i2c.c
index fe5535c..b5253e3 100644
--- a/drivers/video/via/via_i2c.c
+++ b/drivers/video/via/via_i2c.c
@@ -170,13 +170,18 @@ static int create_i2c_bus(struct i2c_adapter *adapter,
return i2c_bit_add_bus(adapter);
}

+/*
+ * By default, we only activate busses on ports 2c and 31 to avoid
+ * conflicts with other possible users; that default can be changed
+ * below.
+ */
static struct via_i2c_adap_cfg adap_configs[] = {
- [VIA_I2C_ADAP_26] = { VIA_I2C_I2C, VIASR, 0x26 },
- [VIA_I2C_ADAP_31] = { VIA_I2C_I2C, VIASR, 0x31 },
- [VIA_I2C_ADAP_25] = { VIA_I2C_GPIO, VIASR, 0x25 },
- [VIA_I2C_ADAP_2C] = { VIA_I2C_GPIO, VIASR, 0x2c },
- [VIA_I2C_ADAP_3D] = { VIA_I2C_GPIO, VIASR, 0x3d },
- { 0, 0, 0 }
+ [VIA_I2C_ADAP_26] = { VIA_I2C_I2C, VIASR, 0x26, 0 },
+ [VIA_I2C_ADAP_31] = { VIA_I2C_I2C, VIASR, 0x31, 1 },
+ [VIA_I2C_ADAP_25] = { VIA_I2C_GPIO, VIASR, 0x25, 0 },
+ [VIA_I2C_ADAP_2C] = { VIA_I2C_GPIO, VIASR, 0x2c, 1 },
+ [VIA_I2C_ADAP_3D] = { VIA_I2C_GPIO, VIASR, 0x3d, 0 },
+ { 0, 0, 0, 0 }
};

int viafb_create_i2c_busses(struct viafb_par *viapar)
@@ -189,6 +194,8 @@ int viafb_create_i2c_busses(struct viafb_par *viapar)

if (adap_cfg->type == 0)
break;
+ if (!adap_cfg->is_active)
+ continue;

ret = create_i2c_bus(&i2c_stuff->adapter,
&i2c_stuff->algo, adap_cfg,
diff --git a/drivers/video/via/via_i2c.h b/drivers/video/via/via_i2c.h
index 00ed978..73d682f 100644
--- a/drivers/video/via/via_i2c.h
+++ b/drivers/video/via/via_i2c.h
@@ -35,6 +35,7 @@ struct via_i2c_adap_cfg {
enum via_i2c_type type;
u_int16_t io_port;
u_int8_t ioport_index;
+ u8 is_active;
};

struct via_i2c_stuff {
--
1.7.0.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: Patch to gianfar.c for 2.6.33.2-rt13
Next: [GIT PULL] regulator fixes for 2.6.34-rc6