RE: [v2,2/3] ata: ahci_tegra: Add support to disable features

[Preetham Chandru <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

>-----Original Message-----
>From: Mikko Perttunen [mailto:cyndis@kapsi.fi]
>Sent: Wednesday, December 21, 2016 3:01 PM
>To: Preetham Chandru; Mikko Perttunen; tj@kernel.org;
>swarren@wwwdotorg.org; thierry.reding@gmail.com;
>preetham260@gmail.com
>Cc: Laxman Dewangan; linux-ide@vger.kernel.org; Venu Byravarasu; Pavan
>Kunapuli; linux-tegra@vger.kernel.org
>Subject: Re: [v2,2/3] ata: ahci_tegra: Add support to disable features
>
>On 21.12.2016 11:12, Preetham Chandru wrote:
>>
>>
>>> -----Original Message-----
>>> From: Mikko Perttunen
>>> Sent: Monday, November 28, 2016 6:09 PM
>>> To: Preetham Chandru; tj@kernel.org; swarren@wwwdotorg.org;
>>> thierry.reding@gmail.com; preetham260@gmail.com
>>> Cc: Laxman Dewangan; linux-ide@vger.kernel.org; Venu Byravarasu;
>>> Pavan Kunapuli; linux-tegra@vger.kernel.org
>>> Subject: Re: [v2,2/3] ata: ahci_tegra: Add support to disable
>>> features
>>>
>>> On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>>>> From: Preetham Chandru R <pchandru@nvidia.com>
>>>>
>>>> Add support to disable DIPM, HIPM, DevSlp, partial, slumber and NCQ
>>>> features from DT. By default these features are enabled.
>>>>
>>>
>>> Why are these features disabled? Are they broken on all Tegra210
>>> chips, broken on specific boards or do they require some support from
>>> the driver that is not there?
>>>
>>> Most likely we should hardcode the disabled features into the driver
>>> instead of reading them from the device tree.
>>>
>>
>> Yes, currently DevSlp and DIPM features are broken for t210 and t124 but
>devslp is fixed for tegra186.
>>  Also I thought it would be nice to provide similar options for other
>features as well.
>> Since we do not support hotplug this would help us in debugging if we face
>issues with certain drives during kernel boot.
>> We can disable features we think are causing issues and retry.
>
>We should put this data into the soc_data struct in the driver, since it is
>easily determined from just the compatibility string; for example
>
>struct tegra_ahci_soc {
>	...
>	uint32_t quirks;
>};
>
>then
>
>.quirks = NO_DEVSLP | NO_DIPM
>
>or the other way around and list the features that we want to enable.
>This is also quite easy to change to test.
>
>The device tree should also only contain information that describes the
>hardware itself, and nothing related to any specific driver's operation; so if
>there's a possibility that some feature is not working due to a driver issue,
>then the device tree definitely should not contain any entry disabling that
>feature.
>
>(The downstream kernel can of course carry such features as patches on top,
>if required for some reason - but naturally it increases maintenance effort)
>

Ok, sounds good to me. Will move it to soc_data structure.

>Thanks,
>Mikko.