Re: [PATCH 23/29] atari_scsi: Convert to platform device

Michael Schmitz
2014-10-04T23:43:51Z

Finn,
> On Fri, 3 Oct 2014, Geert Uytterhoeven wrote:
>
>
>>> + if (ATARIHW_PRESENT(TT_SCSI)) {
>>> + atari_scsi_reg_read = atari_scsi_tt_reg_read;
>>> + atari_scsi_reg_write = atari_scsi_tt_reg_write;
>>> + } else if (ATARIHW_PRESENT(ST_SCSI)) {
>>> + atari_scsi_reg_read = atari_scsi_falcon_reg_read;
>>> + atari_scsi_reg_write = atari_scsi_falcon_reg_write;
>>>
>> Can these be handled through the platform_device's resources?
>>
>>
>
> I don't know.
>

Should be possible - I've seen that used in the ISP116x HCD driver
(arch-dependent post-register access delay function passed via platform
data).

>>
>>> + /* Leave sg_tablesize at 0 on a Falcon! */
>>> + if (IS_A_TT() && setup_sg_tablesize >= 0)
>>> + atari_scsi_template.sg_tablesize = setup_sg_tablesize;
>>>
>> I think the IS_A_TT() check can just be removed.
>>
>
> Well, ST DMA will break if scatter/gather is enabled by the user.
>

Concur - we'd have to make certain the SG buffers are contiguous in
physical memory before allowing SG on Falcon.

>
>>> +#ifdef REAL_DMA
>>> + /* If running on a Falcon and if there's TT-Ram (i.e., more than one
>>> + * memory block, since there's always ST-Ram in a Falcon), then
>>> + * allocate a STRAM_BUFFER_SIZE byte dribble buffer for transfers
>>> + * from/to alternative Ram.
>>> + */
>>> + if (ATARIHW_PRESENT(ST_SCSI) && !ATARIHW_PRESENT(EXTD_DMA) &&
>>> + m68k_num_memory > 1) {
>>> + atari_dma_buffer = atari_stram_alloc(STRAM_BUFFER_SIZE, "SCSI");
>>> + if (!atari_dma_buffer) {
>>> + pr_err(PFX "can't allocate ST-RAM double buffer\n");
>>> + return -ENOMEM;
>>> + }
>>> + atari_dma_phys_buffer = atari_stram_to_phys(atari_dma_buffer);
>>> + atari_dma_orig_addr = 0;
>>> + }
>>> +#endif
>>>
>> More platform data?
>>

Perhaps.

>>
>>> + if (IS_A_TT())
>>> + instance->irq = IRQ_TT_MFP_SCSI;
>>> + else
>>> + instance->irq = IRQ_NONE;
>>>
>> platform_device resource?
>>
>
> If I thought it possible to parameterize the driver such that it never had
> to test IS_A_TT(), I'd probably agree that this would be more elegant.
>
> Otherwise I'd prefer not to have parts of the logic separated off into the
> platform resources while the remaining logic remains in the driver itself.
>
> While I don't think platform resources are desirable in this driver, I
> would like to hear Michael's views.
>

The IRQ is a good candidate to be passed via platform data. Register
access primitives can be done via platform data as well. Likewise, the
ST-DMA locking primitives. That still leaves the DMA setup and
completion code - Falcon and TT differ here in that they require a
different order of DMA setup and NCR setup (the Falcon SCSI chip is
hooked up via the ST-DMA, the TT one memory mapped so DMA setup must
come last on Falcon, and DMA completion check first. TT has that
reversed. That's a bit more hassle and might require lib_NCR5380
approach similar to the ESP SCSI driver.

> Aside from TT and ST, is there a third configuration that might benefit
> from a more data driven configuration?
>

TT and Falcon, not sure any of the ST/STE series ever had a SCSI
adapter. Medusa is TT compatible

>> (and IRQ_NONE is wrong, you should use 0)
>>
>>
>>> + if (IS_A_TT()) {
>>>
>> Check for instance->irq instead?
>>
>
> Yes, you'd think so, but a later patch (not in this set) would have
> to change it back to IS_A_TT().
>
> Further patches align atari_NCR5380.c with NCR5380.c, such that the core
> driver then checks host->irq to find out whether an IRQ is in use. For
> Atari ST, request_irq() is not called even though the ST DMA irq is in
> use.
>
That's why the IRQ is set to 0, I guess? Works for me. The old code
states that setting instance->irq = 0 keeps the midlevel from tampering
with the SCSI IRQ during queuecmd which would interfere with IDE and
floppy. I guess this is still relevant - I would not have seen the
ST-DMA locked by IDE during SCSI queuecmd otherwise.

Won't be able to test any of this for a while, sorry.

Cheers,

Michael