[LinuxBIOS] [PATCH] More vt8237r fixes

[Corey Osgood]

See patch. Rudolf, can you test this one to make sure it works correctly
on your board?

Thanks,
Corey

[Rudolf Marek]

Corey Osgood wrote:
> See patch. Rudolf, can you test this one to make sure it works correctly
> on your board?

Hi,

Your patch seems not so solve KBD and RTC correct? This could be implemented in 
superio on other board for example. Also the clock gating for Ethernet should be 
made programmable.

index = PCI_FUNC(dev->path.u.pci.devfn);

This is not very user friendly, index is quite confusing, in fact I was staring 
to the code quite long time to get how it works. Perhaps two variables would be 
more handy???

I would suggest some lookup table to make it more elegant???? Plus we will need 
at least for RTC and KBD some? variables, just like enable_internal_RTC....
or something shorter.

More over you cant enable just UHCI function 2 and not enable function zero and 
one. Plus you cant disable all UHCIs when EHCI is enabled. (you would violate 
PCI specs not having fn0)

The lookup table could be:

u8/u16 enable_bits[dev-15][fn=6] = { { 0xb , 0x0}, { c, d, a, 8, 9} ....

Or alternatively we could store the bitmask which would allow to enable usb fn0 
when someone wants fn1 for example...

The table would be 4*6*2 bytes so 48B, this is not so much contrary to the big 
if else if code...

Rudolf

[Corey Osgood]

Rudolf Marek wrote:
> Corey Osgood wrote:
> > See patch. Rudolf, can you test this one to make sure it works correctly
> > on your board?
>
> Hi,
>
> Your patch seems not so solve KBD and RTC correct?

Correct, kbc/rtc init should handle things their own way. I thought
about doing that when we come to the lpc controller, but IIRC that's
already taken care of in vt8237r_lpc, so I figured I'd leave it alone.
> This could be implemented in superio on other board for example. Also
> the clock gating for Ethernet should be made programmable.

Yep, it should.
> index = PCI_FUNC(dev->path.u.pci.devfn);
>
> This is not very user friendly, index is quite confusing, in fact I
> was staring to the code quite long time to get how it works. Perhaps
> two variables would be more handy???
>
> I would suggest some lookup table to make it more elegant???? Plus we
> will need at least for RTC and KBD some? variables, just like
> enable_internal_RTC....
> or something shorter.

Uwe and I discussed this on IRC. I'm working on it (among other things) now.
> More over you cant enable just UHCI function 2 and not enable function
> zero and one. Plus you cant disable all UHCIs when EHCI is enabled.
> (you would violate PCI specs not having fn0)

Very good point! I'll keep it in mind. And you need at least one UHCI
controller for EHCI to work.
> The lookup table could be:
>
> u8/u16 enable_bits[dev-15][fn=6] = { { 0xb , 0x0}, { c, d, a, 8, 9} ....
>
> Or alternatively we could store the bitmask which would allow to
> enable usb fn0 when someone wants fn1 for example...
>
> The table would be 4*6*2 bytes so 48B, this is not so much contrary to
> the big if else if code...
>
> Rudolf

-Corey

[Rudolf Marek]

> Very good point! I'll keep it in mind. And you need at least one UHCI
> controller for EHCI to work.

And if you have UHCI fn2 enabled you need to have fn0 and fn1 too... (check VIA 
docs)

Rudolf

[LinuxBIOS] [PATCH] More vt8237r fixes (new patch)

[Corey Osgood]

Updated patch attached, thanks for the suggestions. I think I've done
this right, can you have a look at it/test it? I can't test on hardware
ATM because of some work I'm doing on ram init.

Rudolf Marek wrote:
> > Very good point! I'll keep it in mind. And you need at least one UHCI
> > controller for EHCI to work.
>
> And if you have UHCI fn2 enabled you need to have fn0 and fn1 too...
> (check VIA docs)
>
> Rudolf

I'm not seeing that anywhere, is it in the porting guide? All I have is
the datasheet.

Thanks,
Corey

[LinuxBIOS] [PATCH] More vt8237r fixes (new patch)

[Corey Osgood]

Corey Osgood wrote:
> Updated patch attached, thanks for the suggestions. I think I've done
> this right, can you have a look at it/test it? I can't test on hardware
> ATM because of some work I'm doing on ram init.

Oops, little mistake. This line:
> if (disable_bit == 0x4) enabled = 0;

should have been

+	if (disable_bit == 0x4) enabled = !enabled;

-Corey

[LinuxBIOS] [PATCH] More vt8237r fixes (new patch)

[Rudolf Marek]

> I'm not seeing that anywhere, is it in the porting guide? All I have is
> the datasheet.

Yes in PG.

Rudolf

[LinuxBIOS] [PATCH] More vt8237r fixes (new patch)

[Rudolf Marek]

Hi all,

Corey, that patch does not work for me.

PCI: 00:03.3 [1106/f238] enabled
PCI: 00:0c.0 [10ec/8139] enable
<null> Cannot find pci bus operations
...freeze...

If I remove your patch again after 00:0c.0 it will continue with:
PCI: 00:0f.0 enabled
...

Rudolf