Re: [PATCH] EHCI driver - USB 2.0 support

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4784 bytes --]


> To Vladimir:
>
> Ufff, You sent a lot of comments... :-)
Don't take it as not appreciation of your work. I appreciate it and if
you have no time to do the requested fixes I can help you. It's very
important patch.
> Note, it is first working/tested version... My main problem is the time,
> so I wrote it piece by piece through approx. half of year, often I
> forgot what I did before... - so the code looks like it looks...
I understand.
> I will rewrite some part according to Your comments but don't expect it
> too early...
>
>
> Answers/questions to most of Your comments:
>
> ... macro->inline func. - I like macros... :-) But You are right.
>
Macros are a good way to hit quirks and unless really necessary are to
be avoided.
> ... run indent - Why not. But what options of indent are preferred for
> GRUB source ? I can set in the same way also my favorite editor (Geany)
> to prevent necessity of such source code additional processing.
>
No options, just defaults.
> ... __attribute__((packed)) - I remember, we probably discussed the same
> thing on OHCI/UHCI in past. In these drivers are such attributes still
> used for HW structures. I think it is more safe - we probably cannot
> assume that compiler will align structure members to 32 bits.
You already ensure that the structures are aligned at 16 bytes which is
more than 4 bytes.
> ... only 32 bits - The driver is very simple, similarly as UHCI driver.
> EHCI can support memory higher than 4G but I currently don't care about
> it, I fill all related registers with zero... I cannot test it (my
> computer is little bit older and cannot use more than 4GB RAM...) and,
> generally, I don't have experiences with 64 bits...
Even on 64-bit machines it's fine and recommended to keep all structures
in first 4G. We currently do so for AHCI
> Similarly, to be simple, driver currently does not use DMA allocations
> (as UHCI driver) - there is too much work with it... :-) - some thing
> should be done little bit different because in GRUB is missing some
> function which can convert DMA address to virtual address and vice
> versa.
P2V wouldn't be single-valued. We have grub_vtop but the DMA address to
write into registers is different from the physical address on Yeeloong
so DMA needs separate function to retrieve this value.
>  I made some helping functions in OHCI driver but I am not sure if
> these functions are safe because of possible not continuous DMA memory -
> or does the function grub_memalign_dma32 ensure that whole allocated
> memory is continuous in both DMA and virtual spaces in every case ?
It does.
> Under term "full 64-bit compatible" I understand that any register,
> QH/TD structure and any buffer data can be above 4G - maybe it is wrong
> from my side.
There is no need. We can just stick to use first 4G of memory to stay on
the safe side. For GRUB 3G of RAM is much more than we need.
> ... "No need to specifically exclude those. Just zero-pad address." -
> Hm, I think it could be not so easy because (base_h !=0) means that EHCI
> I/O memory registers are mapped above 4G... Maybe stupid question - does
> it work access to mapped I/O in this case properly in GRUB ?
>
It doesn't. on i386-pc GRUB is limited to first 4G
> ... "e->iobase should have volatile attribute" - I have related but
> probably stupid question - why does work evaluation of QH/TD values in
> runtime properly even if related variables are not declared as
> "volatile" ? (It is the same in UHCI/OHCI drivers.)
> AFAIK "volatile" should be used for variables which can be modified
> "outside" of compiler code, i.e. by HW/DMA. So, I cannot understand why
> for example e->iobase should be "volatile" (even if this variable cannot
> be changed by HW/DMA - but, of course, values of memory area related to
> this pointer can be changed by HW - maybe this is the reason (?)) and
> structures QH/TD need not to be "volatile" even if their values can be
> changed by HW/DMA... ???
But its a pointer to an area which has this property, hence it needs
volatile attribute.
> ... "Arithmetics with PCI address aren't guaranteed to be available or
> to behave in a sane way." - Hm, yes, I change it to usual construction -
> read whole DWORD, change only one bit and write it back.
Read bytes is both correct and fine. Trouble is incrementing an address
like e.g. addr = ...; addr++;
> I will also add the "hard way" of ownership as You propose.
>
I have a laptop we can test it on.
> Best regards
> Ales
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]