Re: EP93xx PIO IDE driver proposal

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Friday 15 May 2009 19:01:11 João Ramos wrote:

[...]

> > +/*
> > + * readl/writel helpers to access internal registers using
> > + * an ioremapped cookie and the specified IDE register offset
> > + */
> > +
> > +static inline u32 ep93xx_readl(unsigned long base, u8 offset)
> > +{
> > +	return readl((void __iomem *)(base + offset));
> > +}
> > +
> > +static inline void ep93xx_writel(u32 value, unsigned long base, u8 offset)
> > +{
> > +	writel(value, (void __iomem *)(base + offset));
> > +}
> >
> > Hmm, what do we need these wrappers for exactly?
> >
> > Please remove them.
> >   
> I just added those to increase code readability, so that I wouldn't have 
> to do a '(void __iomem *)(base + offset)' in every readl/writel call.
> But I can remove it, no problem.

If readability is harmed by such casts you can always add a local variable
to alleviate for it, i.e.:

	void __iomem *base = (unsigned long)__base;

However I don't think a lot of such tricks would be needed after fixing function
arguments to pass 'void __iomem *base' around and not 'unsigned long base'

> > +/*
> > + * Check whether IORDY is asserted or not.
> > + */
> > +static inline int ep93xx_ide_check_iordy(unsigned long base)

like here

> > +{
> > +	u32	reg	= ep93xx_readl(base, IDECTRL);
> > +
> > +	return (reg & IDECTRL_IORDY) ? 1 : 0;
> > +}

[...]

> > +	/*
> > +	 * fill in ide_io_ports structure.
> > +	 * we use magic numbers 0x10 for io_addr and 0x0E for ctl_addr,
> > +	 * hence the lowest 3 bits will give us the real address (ranging from
> > +	 * 0 to 7) and the subsequent 2 bits will give us the CS1n and CS0n
> > +	 * values:
> > +	 *   CS1n  CS0n   A2   A1   A0
> > +	 *     1    0      x    x    x   -> IO_ADDR (base 0x10)
> > +	 *     0    1      x    x    x   -> CTL_ADDR (base 0x0E)
> > +	 */
> > +	ide_std_init_ports(&hw, 0x10, 0x0E);
> >
> > Why not just setup the real addresses here instead of using
> > ide_std_init_ports()?
> >   
> 
> The IDE register's address are specified through bitfields in the EP93xx 
> IDECTRL register, as described in the above comment.
> The 'workaround' in the addresses is just to ease manipulation of those 
> bitfields while writing the register.

Ok, so there is really no reason to use ide_std_init_ports().

> I suppose I could write the register's address in the hw->io_ports so 
> that the value would be directly ORed to the IDECTRL register, if that 
> is what you suggest.

Please try it.

> > It seems that it would make driver simpler and get rid of >config_data use.
> >   
> 
> No. The config_data is used to store the ioremapped cookie of the IDE 
> registers base address. I need that address to access the CPUs IDE 
> registers.

Please note that ep93xx_ide_{read,write}() are always passed
hwif->config_data as 'unsigned long base' argument so the first
argument should become 'ide_hwif_t *hwif' with:

	unsigned long base = hwif->config_data

inside ep93xx_ide_{read,write}().

Also how's about using host->priv_data instead of hwif->config_data?

[ By using ide_host_alloc()+ide_host_register() instead of ide_host_add()
  and later obtaining host->priv_data through hwif->host->priv_data. ]

> There's no connection between this address (which maps to CPU's internal 
> memory, allowing to acess the IDE registers) and the actual IDE 
> addresses, which are defined through bitfields in the IDE control register.
> Yeah, I know, weird IDE host controller... :-)

Indeed. :)