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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Valentine,

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.

> > 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".

-- 
Regards,

Laurent Pinchart