Re: [patch 1/5] Staging: VME Framework for the Linux Kernel

Martyn Welch
2009-08-10T15:52:58Z

Emilio G. Cota wrote:
> Martyn Welch wrote:
>
>> Emilio G. Cota wrote:
>>
>>> - semaphores? isn't it screaming for mutexes?
>>>
>> The semaphores are initialized in mutex mode.
>>
>
> I know, but then please use mutexes.
>
>
I'm clearly missing something here - can you provide me with an example?
>>> - some function signatures (get_whatever) pass too many
>>> pointers; it's rather ugly. Passing a pointer to an
>>> appropriate struct seems a better solution.
>>>
>>>
>> When implementing these functions it seemed like a simpler option to
>> pass the parameters rather than defining yet another structure and using
>> it for a single call. If the *_get functions are changed then the *_set
>> functions should also be changed for consistency. I'm not overly
>> bothered either way - if the consensus is that a structure would be
>> better then we can go with a structure.
>>
>
> Yeh, In my opinion a structure there would be prettier. However
> let's put this off a bit, I'm currently re-working this a bit.
>
>
>>> - vme_alloc_consistent looks pretty fishy; why don't you pass
>>> that responsibility to the specific master? There you obviously
>>> know if you're bridging over PCI or whatever. Or even better;
>>> why is this needed at all?
>>>
>>>
>> In the model I have chosen it is up to the VME device driver to create
>> buffers rather than the VME bridge drivers. After all it is the device
>> drivers for the specific hardware found on the VME bus that knows what
>> it is going to do with these buffers. This method is provided as a
>> helper. It ensures that a VME device driver (not to be confused with the
>> VME bridge driver) is able to allocate a buffer suitable for utilizing
>> the DMA engines found in the bridge or indeed to be mapped onto VME
>> though a slave window without explicitly knowing which VME bridge is
>> being used.
>>
>
> hmm I see. The way you provide coherent DMA mappings for VME is through
> providing coherent (consistent) mappings for PCI, and then mapping the
> VME space onto those. I haven't tested if this works, but sounds
> reasonable.
>
>> Master windows don't require a contiguous buffer, are you referring to
>> something else when you say master?
>>
> I was thinking of the hypothetical case where you'd have a bridge not
> over PCI; but anyway since quite a few things seem tied to PCI this
> is fine.
>

That's one reason for the helper function. If VME bridges are added
which sit of on other buses then vme_alloc_consistent can be extended to
support this without requiring VME device drivers to be altered to
reflect the fact that pci_alloc_consistent might not work for all VME
bridges.

>>> - Please explain me what all the DMA functions do; are they
>>> meant to be used by master or slaves?
>>>
>>>
>> The TODO file (drivers/staging/vme/TODO) contains a description of the
>> current API including the DMA functions, does that provide enough of a
>> description?
>>
>
> I've had a closer look; it seems to me that most of it is unnecessary;
> there's no show those lists to a driver. I'd just provide a single
> 'do_dma(attributes)' call that sleeps until it's done (or similar).
>
>
The DMA controller in the tsi-148 can do PCI to PCI; PCI to VME; VME to
PCI; VME to VME; Patterns to VME and Patterns to PCI transfers. How do
you specify all those options without providing a structure where over
50% of the fields aren't used for any given transfer?

Every bridge I have seen is capable of link-list execution. The API
provides the ability to do scatter-gather style DMA transfers, this
could be used as part of a "zero-copy" type driver for high speed VME
capture devices. How would you support the link-list execution with a
single call?

I was also looking at the ability to queue DMA transfers if the
controller was currently busy - if the consensus is that this is
overkill I will happily remove this.

>>> Have a look at the interface for slaves we've got for our tsi148
>>> driver, available at:
>>> http://repo.or.cz/w/tsi148vmebridge.git
>>>
>>> /* API for new drivers */
>>> extern int vme_request_irq(unsigned int, int (*)(void *),
>>> void *, const char *);
>>>
> [snip]
>
>>> extern int vme_bus_error_check(int);
>>>
>>> That's pretty thin and it covers our slaves' needs. Do you see
>>> anything missing there?
>>>
>>>
>> This interface, especially struct vme_mapping seems to have been written
>> solely for the tsi-148 bridge. The API I have defined aims to provide a
>> consistent API across multiple VME bridges. Whilst this API clearly
>> works for you with the tsi-148 I am unsure how suitable it will be for
>> other bridge chips.
>>
>> The API I have proposed is designed to support more than just the tsi148
>> chipset. Have you thought about how the above API will be supported on
>> other VME bridges?
>>
>
> We only have access to tsi148 chips. Since apparently there aren't
> many bridges around, I think is saner to just provide an interface
> that works well with the devices we have access to (i.e. tsi148 and
> Tundra Universe in your case), and when new chips come along, we simply
> modify the interface as needed. Of course this doesn't mean we shouldn't
> try to make it generic, but within reason.
>
>
Agreed - which I believe is the approach I've taken.
>>> For masters there's no interface there because it was the
>>> master's driver who directly provided these calls to slaves.
>>> I had it in my to-do list to split that from the tsi148, in
>>> the same fashion as you've done with this work.
>>>
>>> - Note above the interrupt handler; simply needs the cookie. Also,
>>> shouldn't your vme_request_irq() just require the IRQ vector?
>>>
>>>
>> No. I believe that you are using the term IRQ vector where we would use
>> the term status ID, the value which is returned from an IACK cycle. Your
>> interrupt handling code assigns a single interrupt handler to all
>> interrupt levels, purely using the interrupt vector/status ID to
>> determine which interrupt handler will be used. This adds an artificial
>> limitation and would not work in some instances that we have seen. Our
>> framework provides the ability to attach an interrupt handler to each
>> combination of IRQ level and Status ID/vector.
>>
> Fair enough. For sanity we tend not to share IRQ vectors among
> different modules, but yes, the interface should know about this.
>
>
>>> - I'd like to see the whole picture, or 'vertical slice', i.e.
>>> the bus interface + a master + a slave driver. How would
>>> the slave's driver get the addresses and sizes of the mappings,
>>> interrupt lines, etc. for each of the devices it controls?
>>> For the time being we quickly hacked an xml-based scheme to get
>>> this info upon installation, but it's clearly not suitable
>>> for mainline.
>>>
>>>
>> I have written a test driver for a very old slave card we had lying
>> around. In that case we used module parameters to determine the location
>> of the device on the bus (it used a fixed VME address space). In this
>> instance the interrupt level and ID could be configured in the registers
>> available in the VME address space, hence I added module parameters to
>> allow these to be configured. In this respect configuration of VME
>> devices is very similar to ISA devices - neither of the buses has
>> supported discovery mechanism from the outset and thus old cards. I
>> therefore implemented a mechanism similar to how I believe ISA
>> approaches this.
>>
>> The framework that I have proposed aims to provide a consistent API to
>> manage allowing the resources provided by the VME bridges to be managed
>> in as a consistent a manner as possible. I believe it is up to the
>> device drivers for the devices found on the VME bus to determine the
>> best way to configure this as it is not provided by the VME
>> specifications.
>>
>
> The problem is how to manage several (say 10) devices that have to be
> controlled with the same driver; passing parameters upon loading the
> driver's module doesn't seem to scale beyond one device..
I implemented it using param array, I agree that the arguments might get quite long with 10 devices, especially if multiple parameters are required, but I don't see why it shouldn't work.

> At the moment I see no clean way of doing this; right now we're doing
> it through an IOCTL, copying from user-space a tree that describes the
> devices to install, namely mappings and IRQs for each of them.
>
> Greg, any ideas on how to deal with this?
>
>
>> For using the VME bus for communication between two SBCs, I think we
>> could use the CRCSR space to provide some information about the
>> resources used on each board for say, a virtual network driver like the
>> rapidIO subsystem provides. Possibly using the "device tree" stuff used
>> on PowerPC, Blackfin and Sparc archs (I believe) for passing device
>> layout between the firmware and kernel at boot.
>>
>
> hmm I don't see how we could implement this common consistent view of
> the bus in a way that could work with every bridge. For the time
> being I'd defer this and simply pass that responsibility to user-space.
>
>
That's the conclusion I sort of came to. I guess how the VME address
space(s) are used is a system integration issue.
>> Sure, np,
>>
>> Martyn
>>
>
> Thanks,
> E.
>

--
Martyn Welch MEng MPhil MIET (Principal Software Engineer) T:+44(0)1327322748
GE Fanuc Intelligent Platforms Ltd, |Registered in England and Wales
Tove Valley Business Park, Towcester, |(3828642) at 100 Barbirolli Square,
Northants, NN12 6PF, UK T:+44(0)1327359444 |Manchester,M2 3AB VAT:GB 927559189