Re: [PATCH] ata: sata_rcar: Disable DIPM mode for r8a7790 ES1

[Simon Horman <horms@verge.net.au>]

On Fri, Oct 17, 2014 at 07:25:34AM +0200, Magnus Damm wrote:
> Hi Simon,
> 
> On Thu, Oct 16, 2014 at 7:50 AM, Simon Horman
> <horms+renesas@verge.net.au> wrote:
> > Unlike other SATA R-Car r8a7790 controllers the r8a7790 ES1 SATA R-Car
> > controller needs to be run with DIPM disabled.
> >
> > Loosely based on work by Koji Matsuoka.
> >
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > ---
> > This patch is against for-next branch of Tejun's libata tree.
> 
> Thanks for cooking up this improved version of the ES1 workaround.
> 
> >  Documentation/devicetree/bindings/ata/sata_rcar.txt |  3 ++-
> >  drivers/ata/sata_rcar.c                             | 10 ++++++++++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt b/Documentation/devicetree/bindings/ata/sata_rcar.txt
> > index 1e61113..7dd32d3 100644
> > --- a/Documentation/devicetree/bindings/ata/sata_rcar.txt
> > +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt
> > @@ -3,7 +3,8 @@
> >  Required properties:
> >  - compatible           : should contain one of the following:
> >                           - "renesas,sata-r8a7779" for R-Car H1
> > -                         - "renesas,sata-r8a7790" for R-Car H2
> > +                         - "renesas,sata-r8a7790-es1" for R-Car H2 ES1
> > +                         - "renesas,sata-r8a7790" for R-Car H2 other than ES1
> >                           - "renesas,sata-r8a7791" for R-Car M2
> >  - reg                  : address and length of the SATA registers;
> >  - interrupts           : must consist of one interrupt specifier.
> 
> This portion looks fine to me.
> 
> > diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
> > index 61eb6d7..d5cacb5 100644
> > --- a/drivers/ata/sata_rcar.c
> > +++ b/drivers/ata/sata_rcar.c
> > @@ -146,6 +146,7 @@
> >  enum sata_rcar_type {
> >         RCAR_GEN1_SATA,
> >         RCAR_GEN2_SATA,
> > +       RCAR_R8A7790_ES1_SATA,
> >  };
> >
> >  struct sata_rcar_priv {
> > @@ -763,6 +764,10 @@ static void sata_rcar_setup_port(struct ata_host *host)
> >         ap->udma_mask   = ATA_UDMA6;
> >         ap->flags       |= ATA_FLAG_SATA;
> >
> > +       if (priv->type = RCAR_R8A7790_ES1_SATA) {
> > +               ap->flags       |= ATA_FLAG_NO_DIPM;
> > +       }
> > +
> >         ioaddr->cmd_addr = base + SDATA_REG;
> >         ioaddr->ctl_addr = base + SSDEVCON_REG;
> >         ioaddr->scr_addr = base + SCRSSTS_REG;
> > @@ -838,6 +843,10 @@ static struct of_device_id sata_rcar_match[] = {
> >                 .data = (void *)RCAR_GEN2_SATA
> >         },
> >         {
> > +               .compatible = "renesas,sata-r8a7790-es1",
> > +               .data = (void *)RCAR_R8A7790_ES1_SATA
> > +       },
> > +       {
> >                 .compatible = "renesas,sata-r8a7791",
> >                 .data = (void *)RCAR_GEN2_SATA
> >         },
> > @@ -849,6 +858,7 @@ static const struct platform_device_id sata_rcar_id_table[] = {
> >         { "sata_rcar", RCAR_GEN1_SATA }, /* Deprecated by "sata-r8a7779" */
> >         { "sata-r8a7779", RCAR_GEN1_SATA },
> >         { "sata-r8a7790", RCAR_GEN2_SATA },
> > +       { "sata-r8a7790-es1", RCAR_R8A7790_ES1_SATA },
> >         { "sata-r8a7791", RCAR_GEN2_SATA },
> >         { },
> >  };
> > --
> > 2.1.1
> >
> 
> >From my side I'm not sure if the platform device interface at this
> point really needs to cover the ES portion or not. It may not hurt to
> be consistent though so no need to change from my side.
> 
> Is this patch all needed in the driver to handle the ES1 case?
> 
> I'm asking because it looks like the switch() statement in
> sata_rcar_init_controller() may accidentally skip some gen2 related
> setup code in case of ES1, maybe this is intentional or perhaps some
> update is needed.

Thanks Magnus,

that is not intentional, its an oversight on my part.

As it does not appear to be difficult to be consistent for the platform
device case I think it would be good to do so.

It seems that the following incremental change would address your concern.

diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
index 61eb6d7..01c8505 100644
--- a/drivers/ata/sata_rcar.c
+++ b/drivers/ata/sata_rcar.c
@@ -792,6 +792,7 @@ static void sata_rcar_init_controller(struct ata_host *host)
 		sata_rcar_gen1_phy_init(priv);
 		break;
 	case RCAR_GEN2_SATA:
+	case RCAR_R8A7790_ES1_SATA:
 		sata_rcar_gen2_phy_init(priv);
 		break;
 	default: