Re: [RFC][PATCH] at91_ide driver

[Stanislaw Gruszka <stf_xl@wp.pl>]

Thursday 22 January 2009 13:06:16 Sergei Shtylyov napisał(a):
> >>> +/* Proper CS address space will be added */
> >>> +#define AT91_IDE_TASK_FILE	0x00c00000
> >>> +#define AT91_IDE_CTRL_REG	0x00e00000
> 
>    Besides, I'm not sure: these 2 address range are decoded via 1 SMC 
> chip select or 2?

These are in one chip select address space.

> > Well, we could add #ifdef with diffrent implementation of init_smc_mode(), 
> > set_8bit_mode(), etc... 
> >   
> 
>    No, #ifdef'ery is certainly not an option.

Why? Other option is create some header files and implement and exporting these
functions from processor specific code. This add files dependencies and spread
things across sources, FWIW.

> >>> +	/* values are rounded up so we need to assure cycle is larger than pulse */
> >>> +	if (t0 < t1 + t2 + t9)
> >>> +		t0 = t1 + t2 + t9;
> >>> +
> >>> +	/* setup calculated timings */
> >>> +	at91_sys_write(AT91_SMC_SETUP(chipselect), AT91_SMC_NWESETUP_(t1) |
> >>> +						   AT91_SMC_NCS_WRSETUP_(0) |
> >>> +						   AT91_SMC_NRDSETUP_(t1) |
> >>> +						   AT91_SMC_NCS_RDSETUP_(0));
> >>> +	at91_sys_write(AT91_SMC_PULSE(chipselect), AT91_SMC_NWEPULSE_(t2) |
> >>> +						   AT91_SMC_NCS_WRPULSE_(t1 + t2 + t9) |
> >>>   
> >>>       
> >>   With zero address to CS setup time it's the same as t0.
> >>     
> > Not true for slower PIO modes.
> 
>    Well, you're right probably...
> 
> > But CS pulse can be t0.
> 
>    Yes, it must be no less than t0.

Pulse time can be less than t0, cycle time no.
 
> > Write data is driver on
> > NWR signal (WRITE_MODE = 1) in at91_ide,  in opposite to pata_at32 where 
> > there is need to non zero CS setup or recovery time.
> >   
> 
>    You're always setting CS setup to 0. Recovery time can't be 0, you 
> probably mean nCS hold time?

Yes, NCS hold time of course.

 
> > Maybe this is simpler, host have to react on IORDY signal but device
> > just not assert it. 
> >
> > I would like to remove this code and have allways NWAIT
> > READY mode to make flipping 8/16 simpler.
>
>    I don't understand how these are connected.

Both set SMC MODE register. If I will not change NWAIT in MODE register,
I can write static values in set_8/16bit_mode and stop doing logical operation on it.

> >>    Why not pass it normally, via the platform device's resource?
> >>     
> > Irq pin is board specific. When use resource I will need in processor 
> > code modify resource.
> 
>    So what? That's pretty normal and won't take much code.

I see only more memory and code usage. Let's reverse question:
Why use RESOURCE_IRQ when we have irq in board data?


Cheers
Stanislaw Gruszka