diff mbox series

[v1,15/24] ata: ahci: Add BAR index quirk for Cavium PCI SATA device

Message ID 20200724100856.1482324-16-sr@denx.de
State Superseded
Delegated to: Stefan Roese
Headers show
Series arm: Introduce Marvell/Cavium OcteonTX/TX2 | expand

Commit Message

Stefan Roese July 24, 2020, 10:08 a.m. UTC 
From: Suneel Garapati <sgarapati@marvell.com>

For SATA controller found on OcteonTX SoC's, use non-standard PCI BAR0
instead of BAR5.

Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
Cc: Simon Glass <sjg@chromium.org>

Signed-off-by: Stefan Roese <sr@denx.de>
---

Changes in v1:
- Change patch subject
- Use constants from pci_ids.h instead of hardcoded values

 drivers/ata/ahci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Simon Glass July 28, 2020, 7:01 p.m. UTC | #1 
Hi Stefan,

On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr@denx.de> wrote:
>
> From: Suneel Garapati <sgarapati@marvell.com>
>
> For SATA controller found on OcteonTX SoC's, use non-standard PCI BAR0
> instead of BAR5.
>
> Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
> Cc: Simon Glass <sjg@chromium.org>
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
>
> Changes in v1:
> - Change patch subject
> - Use constants from pci_ids.h instead of hardcoded values
>
>  drivers/ata/ahci.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 47cdea1f58..28161b5e62 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1198,10 +1198,18 @@ int ahci_probe_scsi(struct udevice *ahci_dev, ulong base)
>  int ahci_probe_scsi_pci(struct udevice *ahci_dev)
>  {
>         ulong base;
> +       u16 vendor, device;
>
>         base = (ulong)dm_pci_map_bar(ahci_dev, PCI_BASE_ADDRESS_5,
>                                      PCI_REGION_MEM);
>
> +       dm_pci_read_config16(ahci_dev, PCI_VENDOR_ID, &vendor);
> +       dm_pci_read_config16(ahci_dev, PCI_DEVICE_ID, &device);
> +
> +       if (vendor == PCI_VENDOR_ID_CAVIUM &&
> +           device == PCI_DEVICE_ID_CAVIUM_SATA)
> +               base = (uintptr_t)dm_pci_map_bar(ahci_dev, PCI_BASE_ADDRESS_0,
> +                                                PCI_REGION_MEM);

How should we handle this in general? Should we have a Kconfig to
enable quirks in ahci?

>         return ahci_probe_scsi(ahci_dev, base);
>  }
>  #endif
> --
> 2.27.0
>

Regards,
Simon
Stefan Roese July 30, 2020, 3:41 p.m. UTC | #2 
Hi Simon,

On 28.07.20 21:01, Simon Glass wrote:
> Hi Stefan,
> 
> On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr@denx.de> wrote:
>>
>> From: Suneel Garapati <sgarapati@marvell.com>
>>
>> For SATA controller found on OcteonTX SoC's, use non-standard PCI BAR0
>> instead of BAR5.
>>
>> Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
>> Cc: Simon Glass <sjg@chromium.org>
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> ---
>>
>> Changes in v1:
>> - Change patch subject
>> - Use constants from pci_ids.h instead of hardcoded values
>>
>>   drivers/ata/ahci.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 47cdea1f58..28161b5e62 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1198,10 +1198,18 @@ int ahci_probe_scsi(struct udevice *ahci_dev, ulong base)
>>   int ahci_probe_scsi_pci(struct udevice *ahci_dev)
>>   {
>>          ulong base;
>> +       u16 vendor, device;
>>
>>          base = (ulong)dm_pci_map_bar(ahci_dev, PCI_BASE_ADDRESS_5,
>>                                       PCI_REGION_MEM);
>>
>> +       dm_pci_read_config16(ahci_dev, PCI_VENDOR_ID, &vendor);
>> +       dm_pci_read_config16(ahci_dev, PCI_DEVICE_ID, &device);
>> +
>> +       if (vendor == PCI_VENDOR_ID_CAVIUM &&
>> +           device == PCI_DEVICE_ID_CAVIUM_SATA)
>> +               base = (uintptr_t)dm_pci_map_bar(ahci_dev, PCI_BASE_ADDRESS_0,
>> +                                                PCI_REGION_MEM);
> 
> How should we handle this in general? Should we have a Kconfig to
> enable quirks in ahci?

Perhaps we should wait a bit until, other "quirks" for AHCI get posted.
Only then we can see, where and how these can be grouped and can 
"extract" them into Kconfig option(s). One quirk is the overhead of
the additional Kconfig option / handling not worth, IMHO.

What do you think?

Thanks,
Stefan
Simon Glass July 31, 2020, 6:35 p.m. UTC | #3 
Hi Stefan,

On Thu, 30 Jul 2020 at 09:41, Stefan Roese <sr@denx.de> wrote:
>
> Hi Simon,
>
> On 28.07.20 21:01, Simon Glass wrote:
> > Hi Stefan,
> >
> > On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr@denx.de> wrote:
> >>
> >> From: Suneel Garapati <sgarapati@marvell.com>
> >>
> >> For SATA controller found on OcteonTX SoC's, use non-standard PCI BAR0
> >> instead of BAR5.
> >>
> >> Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
> >> Cc: Simon Glass <sjg@chromium.org>
> >>
> >> Signed-off-by: Stefan Roese <sr@denx.de>
> >> ---
> >>
> >> Changes in v1:
> >> - Change patch subject
> >> - Use constants from pci_ids.h instead of hardcoded values
> >>
> >>   drivers/ata/ahci.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> >>
> >> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> >> index 47cdea1f58..28161b5e62 100644
> >> --- a/drivers/ata/ahci.c
> >> +++ b/drivers/ata/ahci.c
> >> @@ -1198,10 +1198,18 @@ int ahci_probe_scsi(struct udevice *ahci_dev, ulong base)
> >>   int ahci_probe_scsi_pci(struct udevice *ahci_dev)
> >>   {
> >>          ulong base;
> >> +       u16 vendor, device;
> >>
> >>          base = (ulong)dm_pci_map_bar(ahci_dev, PCI_BASE_ADDRESS_5,
> >>                                       PCI_REGION_MEM);
> >>
> >> +       dm_pci_read_config16(ahci_dev, PCI_VENDOR_ID, &vendor);
> >> +       dm_pci_read_config16(ahci_dev, PCI_DEVICE_ID, &device);
> >> +
> >> +       if (vendor == PCI_VENDOR_ID_CAVIUM &&
> >> +           device == PCI_DEVICE_ID_CAVIUM_SATA)
> >> +               base = (uintptr_t)dm_pci_map_bar(ahci_dev, PCI_BASE_ADDRESS_0,
> >> +                                                PCI_REGION_MEM);
> >
> > How should we handle this in general? Should we have a Kconfig to
> > enable quirks in ahci?
>
> Perhaps we should wait a bit until, other "quirks" for AHCI get posted.
> Only then we can see, where and how these can be grouped and can
> "extract" them into Kconfig option(s). One quirk is the overhead of
> the additional Kconfig option / handling not worth, IMHO.
>
> What do you think?

Sounds OK to me. You could add a note about it perhaps.

Regards,
SImon
Stefan Roese Aug. 4, 2020, 1:37 p.m. UTC | #4 
Hi Simon,

On 31.07.20 20:35, Simon Glass wrote:
> Hi Stefan,
> 
> On Thu, 30 Jul 2020 at 09:41, Stefan Roese <sr@denx.de> wrote:
>>
>> Hi Simon,
>>
>> On 28.07.20 21:01, Simon Glass wrote:
>>> Hi Stefan,
>>>
>>> On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> From: Suneel Garapati <sgarapati@marvell.com>
>>>>
>>>> For SATA controller found on OcteonTX SoC's, use non-standard PCI BAR0
>>>> instead of BAR5.
>>>>
>>>> Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> ---
>>>>
>>>> Changes in v1:
>>>> - Change patch subject
>>>> - Use constants from pci_ids.h instead of hardcoded values
>>>>
>>>>    drivers/ata/ahci.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>>>
>>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>>> index 47cdea1f58..28161b5e62 100644
>>>> --- a/drivers/ata/ahci.c
>>>> +++ b/drivers/ata/ahci.c
>>>> @@ -1198,10 +1198,18 @@ int ahci_probe_scsi(struct udevice *ahci_dev, ulong base)
>>>>    int ahci_probe_scsi_pci(struct udevice *ahci_dev)
>>>>    {
>>>>           ulong base;
>>>> +       u16 vendor, device;
>>>>
>>>>           base = (ulong)dm_pci_map_bar(ahci_dev, PCI_BASE_ADDRESS_5,
>>>>                                        PCI_REGION_MEM);
>>>>
>>>> +       dm_pci_read_config16(ahci_dev, PCI_VENDOR_ID, &vendor);
>>>> +       dm_pci_read_config16(ahci_dev, PCI_DEVICE_ID, &device);
>>>> +
>>>> +       if (vendor == PCI_VENDOR_ID_CAVIUM &&
>>>> +           device == PCI_DEVICE_ID_CAVIUM_SATA)
>>>> +               base = (uintptr_t)dm_pci_map_bar(ahci_dev, PCI_BASE_ADDRESS_0,
>>>> +                                                PCI_REGION_MEM);
>>>
>>> How should we handle this in general? Should we have a Kconfig to
>>> enable quirks in ahci?
>>
>> Perhaps we should wait a bit until, other "quirks" for AHCI get posted.
>> Only then we can see, where and how these can be grouped and can
>> "extract" them into Kconfig option(s). One quirk is the overhead of
>> the additional Kconfig option / handling not worth, IMHO.
>>
>> What do you think?
> 
> Sounds OK to me. You could add a note about it perhaps.

Okay, will do.

Thanks,
Stefan
diff mbox series

Patch

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 47cdea1f58..28161b5e62 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1198,10 +1198,18 @@  int ahci_probe_scsi(struct udevice *ahci_dev, ulong base)
 int ahci_probe_scsi_pci(struct udevice *ahci_dev)
 {
 	ulong base;
+	u16 vendor, device;
 
 	base = (ulong)dm_pci_map_bar(ahci_dev, PCI_BASE_ADDRESS_5,
 				     PCI_REGION_MEM);
 
+	dm_pci_read_config16(ahci_dev, PCI_VENDOR_ID, &vendor);
+	dm_pci_read_config16(ahci_dev, PCI_DEVICE_ID, &device);
+
+	if (vendor == PCI_VENDOR_ID_CAVIUM &&
+	    device == PCI_DEVICE_ID_CAVIUM_SATA)
+		base = (uintptr_t)dm_pci_map_bar(ahci_dev, PCI_BASE_ADDRESS_0,
+						 PCI_REGION_MEM);
 	return ahci_probe_scsi(ahci_dev, base);
 }
 #endif