Re: [PATCH 2/2] sata_rcar: Add R-Car Gen2 SATA support

[Valentine <valentine.barshak@cogentembedded.com>]

On 10/14/2013 10:15 PM, Laurent Pinchart wrote:
> Hi Valentine,
>

Hi Laurent,

> On Monday 14 October 2013 21:58:50 Valentine wrote:
>> On 10/14/2013 08:26 PM, Mark Rutland wrote:
>>> On Mon, Oct 14, 2013 at 04:42:34PM +0100, Valentine Barshak wrote:
>>>> R-Car Gen2 SoCs have a different PHY which is not compatible
>>>> with the older R-Car H1 (R8A7779) version.
>>>> This adds OF/platform device id tables and PHY initialization
>>>>
>>>> callbacks for the following Gen2 SoCs:
>>>>     * R-Car H2: R8A7790;
>>>>     * R-Car M2: R8A7791.
>>>>
>>>> PHY initialization method is chosen, based on the device id.
>>>> Default PHY settings are applied for Gen2 SoCs, which should
>>>> suit the Gen2 boards available.
>>>>
>>>> The R8A7779 platform code is modified to use "sata-r8a7779"
>>>> device id.
>>>>
>>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
>>>> ---
>>>>
>>>>    .../devicetree/bindings/ata/sata_rcar.txt          |   5 +-
>>>>    arch/arm/mach-shmobile/clock-r8a7779.c             |   2 +-
>>>>    arch/arm/mach-shmobile/setup-r8a7779.c             |   2 +-
>>>>    drivers/ata/sata_rcar.c                            | 112 +++++++++++---
>>>>    4 files changed, 102 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt
>>>> b/Documentation/devicetree/bindings/ata/sata_rcar.txt index
>>>> 2465183..b5a41bf 100644
>>>> --- a/Documentation/devicetree/bindings/ata/sata_rcar.txt
>>>> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt
>>>> @@ -1,7 +1,10 @@
>>>>
>>>>    * Renesas R-Car SATA
>>>>
>>>>    Required properties:
>>>> -- compatible		: must be "renesas,sata-r8a7779"
>>>
>>>> +- compatible		: must be one of the following:
>>> s/must be/should contain/
>>>
>>>> +			  - "renesas,sata-r8a7779" for R-Car H1
>>>> +			  - "renesas,sata-r8a7790" for R-Car H2
>>>> +			  - "renesas,sata-r8a7791" for R-Car M2
>>>
>>> How do renesas,sata-r8a7790 and renesas,sata-r8a7791 differ?
>>>
>>>>    - reg			: address range of the SATA registers.
>>>
>>> It's a size too...
>>>
>>>>    - interrupt-parent	: interrupt parent controller phandle
>>>
>>> Not required.
>>>
>>> [...]
>>>
>>>> +static struct of_device_id sata_rcar_match[] = {
>>>> +	{
>>>> +		.compatible = "renesas,sata-r8a7779",
>>>> +		.data = (void *)RCAR_GEN1_SATA,
>>>> +	},
>>>> +	{
>>>> +		.compatible = "renesas,sata-r8a7790",
>>>> +		.data = (void *)RCAR_GEN2_SATA
>>>> +	},
>>>> +	{
>>>> +		.compatible = "renesas,sata-r8a7791",
>>>> +		.data = (void *)RCAR_GEN2_SATA
>>>> +	},
>>>> +	{},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, sata_rcar_match);
>>>
>>> Are the renesas,sata-r8a779x variants identical?
>>
>> Yes.
>>
>>> If so, why the two strings?
>>
>> Just thought the driver should support "renesas,sata-r8a7791"
>> compatibility as well since it should support both SoCs.
>> Didn't want to force sata-r8a7790 compatibility for the r8a7791 SoC DTS.
>
> One reason for two compatibility strings is not to be stuck if we later find
> out the the 7791 SATA controller differs from the 7790.
>

Thanks!
Yeah, anyways, I'd prefer to keep both strings in the device id table.

>>> Could we not require sata-r8a7791 devices to have "sata-r8a7790" in the
>>>
>>> compatible list:
>>> 	compatible = "sata-r8a7791", "sata-r8a7790";
>>>
>>> That way we can match on "sata-r8a7791" if we want to later, but don't
>>> need code for it now.
>>
>> We could. In this case having compatible = "renesas,sata-r8a7791" would not
>> be enough, though it looks like a valid compatible list to me.
>
> I'm fine with both solution, as long as the "sata-r8a7791" compatibility
> string is present and has precedence over "sata-r8a7790".
>

Thanks,
Val.