Question about PATA Sil680 Bus Reset Code

Question about PATA Sil680 Bus Reset Code

[Fajun Chen]

Hi,

Could someone help me interpret the code snippet below:
static int sil680_bus_reset(struct ata_port *ap,unsigned int *classes)
{
        struct pci_dev *pdev = to_pci_dev(ap->host->dev);
        unsigned long addr = sil680_selreg(ap, 0);
        u8 reset;

        pci_read_config_byte(pdev, addr, &reset);
        pci_write_config_byte(pdev, addr, reset | 0x03);   // ?
        udelay(25);
        pci_write_config_byte(pdev, addr, reset);
        return ata_std_softreset(ap, classes);
}

Based on Sil680 data sheet, channel reset bit is bit 2,  why the reset
code above is not "pci_write_config_byte(pdev, addr, reset | 0x04);"?

Thanks in advance!

Fajun

Re: Question about PATA Sil680 Bus Reset Code

[Jeff Garzik]

Fajun Chen wrote:
> Hi,
> 
> Could someone help me interpret the code snippet below:
> static int sil680_bus_reset(struct ata_port *ap,unsigned int *classes)
> {
>        struct pci_dev *pdev = to_pci_dev(ap->host->dev);
>        unsigned long addr = sil680_selreg(ap, 0);
>        u8 reset;
> 
>        pci_read_config_byte(pdev, addr, &reset);
>        pci_write_config_byte(pdev, addr, reset | 0x03);   // ?
>        udelay(25);
>        pci_write_config_byte(pdev, addr, reset);
>        return ata_std_softreset(ap, classes);
> }
> 
> Based on Sil680 data sheet, channel reset bit is bit 2,  why the reset
> code above is not "pci_write_config_byte(pdev, addr, reset | 0x04);"?

Definitely looks like a bug to me.

Channel reset is bit 2 in SiI 311x SATA family as well.

	Jeff

Re: Question about PATA Sil680 Bus Reset Code

[Sergei Shtylyov]

Hello.

Fajun Chen wrote:

> Could someone help me interpret the code snippet below:
> static int sil680_bus_reset(struct ata_port *ap,unsigned int *classes)
> {
>        struct pci_dev *pdev = to_pci_dev(ap->host->dev);
>        unsigned long addr = sil680_selreg(ap, 0);
>        u8 reset;
> 
>        pci_read_config_byte(pdev, addr, &reset);
>        pci_write_config_byte(pdev, addr, reset | 0x03);   // ?

>        udelay(25);
>        pci_write_config_byte(pdev, addr, reset);
>        return ata_std_softreset(ap, classes);
> }
> 
> Based on Sil680 data sheet, channel reset bit is bit 2,  why the reset
> code above is not "pci_write_config_byte(pdev, addr, reset | 0x04);"?

    I guess it's been blindly copied over form drivers/ide/pci/siimage.c...
The code indeed does seem meaningless. For the libata it could make sense to 
set bit 2 for the hardreset -- but then sil680_error_handler() needs to be 
turn into ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset, 
sil680_bus_reset, ata_std_postreset)...
    For the legacy driver, this function needs to be converted to something 
sane too...

> Thanks in advance!

MBR, Sergei

Re: Question about PATA Sil680 Bus Reset Code

[Jeff Garzik]

Sergei Shtylyov wrote:
> The code indeed does seem meaningless. For the libata it could make 
> sense to set bit 2 for the hardreset -- but then sil680_error_handler() 
> needs to be turn into ata_bmdma_drive_eh(ap, ata_std_prereset, 
> ata_std_softreset, sil680_bus_reset, ata_std_postreset)...

Presuming bit 2 == hardreset... correct.

In some datasheets it is not clear whether this means resetting the 
silicon's ATA state machine, or truly hard-resetting the port.  There is 
occasionally a difference.  I do not remember if Sil680 is one of those.

	Jeff

Re: Question about PATA Sil680 Bus Reset Code

[Sergei Shtylyov]

Hello.

Jeff Garzik wrote:

>> The code indeed does seem meaningless. For the libata it could make 
>> sense to set bit 2 for the hardreset -- but then 
>> sil680_error_handler() needs to be turn into ata_bmdma_drive_eh(ap, 
>> ata_std_prereset, ata_std_softreset, sil680_bus_reset, 
>> ata_std_postreset)...

> Presuming bit 2 == hardreset... correct.

    It's said to control the RST pin.

>     Jeff

MBR, Sergei

Re: Question about PATA Sil680 Bus Reset Code

[Alan Cox]

>     I guess it's been blindly copied over form drivers/ide/pci/siimage.c...
> The code indeed does seem meaningless. For the libata it could make sense to 
> set bit 2 for the hardreset -- but then sil680_error_handler() needs to be 
> turn into ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset, 
> sil680_bus_reset, ata_std_postreset)...
>     For the legacy driver, this function needs to be converted to something 
> sane too...

I think the evidence based upon years of highly reliable siimage usage is
that its simply not needed 8)

Re: Question about PATA Sil680 Bus Reset Code

[Sergei Shtylyov]

Alan Cox wrote:

>>    I guess it's been blindly copied over form drivers/ide/pci/siimage.c...

    Yet the order of events between IDE and libata drivers is different:
the old driver's resetproc() method is called just after the twiddling the bit 
on/off,  the new driver calls ata_std_softreset() after the PCI config. 
register manipulation.  However, since all it does is set 2 read-only bits, it 
should make no difference...

>>The code indeed does seem meaningless. For the libata it could make sense to 
>>set bit 2 for the hardreset -- but then sil680_error_handler() needs to be 
>>turn into ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset, 
>>sil680_bus_reset, ata_std_postreset)...
>>    For the legacy driver, this function needs to be converted to something 
>>sane too...

> I think the evidence based upon years of highly reliable siimage usage is
> that its simply not needed 8)

    I would think so as well but was not sure about the SStatus reg. read at 
the end of it...

MBR, Sergei

Re: Question about PATA Sil680 Bus Reset Code

[Bartlomiej Zolnierkiewicz]

On Tuesday 10 July 2007, Sergei Shtylyov wrote:
> Alan Cox wrote:
> 
> >>    I guess it's been blindly copied over form drivers/ide/pci/siimage.c...
> 
>     Yet the order of events between IDE and libata drivers is different:
> the old driver's resetproc() method is called just after the twiddling the bit 
> on/off,  the new driver calls ata_std_softreset() after the PCI config. 
> register manipulation.  However, since all it does is set 2 read-only bits, it 
> should make no difference...
> 
> >>The code indeed does seem meaningless. For the libata it could make sense to 
> >>set bit 2 for the hardreset -- but then sil680_error_handler() needs to be 
> >>turn into ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset, 
> >>sil680_bus_reset, ata_std_postreset)...
> >>    For the legacy driver, this function needs to be converted to something 
> >>sane too...
> 
> > I think the evidence based upon years of highly reliable siimage usage is
> > that its simply not needed 8)
> 
>     I would think so as well but was not sure about the SStatus reg. read at 
> the end of it...

Could you send a patch removing a said code?

Thanks,
Bart