Re: [PATCH 2/5] arm: shmobile: r8a7790: Add SATA clock

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

Hi Valentine,

On Thursday 19 December 2013 05:00:03 Valentine wrote:
> On 12/18/2013 06:19 PM, Laurent Pinchart wrote:
> > Hi Valentine,
> 
> Hi Laurent, Morimoto-san,
> 
> > Thank you for the patch.
> > 
> > On Wednesday 18 December 2013 16:44:17 Valentine Barshak wrote:
> >> This adds SATA 0/1 clock support. External 100MHz SATA 0/1
> >> 
> >> reference clock is supposed to be applied to the following pins:
> >>    CICREFP0_SATA/CICREFP1_SATA;
> >>    CICREFN0_SATA/CICREFN1_SATA.
> >> 
> >> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> >> ---
> >> 
> >>   arch/arm/mach-shmobile/clock-r8a7790.c | 19 +++++++++++++++++++
> >>   1 file changed, 19 insertions(+)
> >> 
> >> diff --git a/arch/arm/mach-shmobile/clock-r8a7790.c
> >> b/arch/arm/mach-shmobile/clock-r8a7790.c index 3e27200..c4b567b 100644
> >> --- a/arch/arm/mach-shmobile/clock-r8a7790.c
> >> +++ b/arch/arm/mach-shmobile/clock-r8a7790.c
> >> @@ -73,6 +73,18 @@ static struct clk extal_clk = {
> >>   	.mapping	= &cpg_mapping,
> >>   };
> >> 
> >> +/* External SATA0 reference clock: 100MHz fixed */
> >> +static struct clk sata0_clk = {
> >> +	.rate		= 100000000,
> >> +	.mapping	= &cpg_mapping,
> >> +};
> >> +
> >> +/* External SATA1 reference clock: 100MHz fixed */
> >> +static struct clk sata1_clk = {
> >> +	.rate		= 100000000,
> >> +	.mapping	= &cpg_mapping,
> >> +};
> >> +
> >>   static struct sh_clk_ops followparent_clk_ops = {
> >>   	.recalc	= followparent_recalc,
> >>   };
> >> @@ -140,6 +152,8 @@ static struct clk *main_clks[] = {
> >>   	&ddr_clk,
> >>   	&mp_clk,
> >>   	&cp_clk,
> >> +	&sata0_clk,
> >> +	&sata1_clk,
> >>   };
> >>   
> >>   /* SDHI (DIV4) clock */
> >> @@ -187,6 +201,7 @@ enum {
> >>   	MSTP1009, MSTP1008, MSTP1007, MSTP1006, MSTP1005,
> >>   	MSTP931, MSTP930, MSTP929, MSTP928,
> >>   	MSTP917,
> >> +	MSTP815, MSTP814,
> >>   	MSTP813,
> >>   	MSTP726, MSTP725, MSTP724, MSTP723, MSTP722, MSTP721, MSTP720,
> >>   	MSTP717, MSTP716,
> >> @@ -215,6 +230,8 @@ static struct clk mstp_clks[MSTP_NR] = {
> >>   	[MSTP929] = SH_CLK_MSTP32(&p_clk, SMSTPCR9, 29, 0), /* I2C2 */
> >>   	[MSTP928] = SH_CLK_MSTP32(&p_clk, SMSTPCR9, 28, 0), /* I2C3 */
> >>   	[MSTP917] = SH_CLK_MSTP32(&qspi_clk, SMSTPCR9, 17, 0), /* QSPI */
> >> +	[MSTP815] = SH_CLK_MSTP32(&sata0_clk, SMSTPCR8, 15, 0), /* SATA0 */
> >> +	[MSTP814] = SH_CLK_MSTP32(&sata1_clk, SMSTPCR8, 14, 0), /* SATA1 */
> > 
> > Are those two clocks really children of the external reference clocks ? I
> > got the impression that the external reference clocks would be used by
> > the PHY only and that the functional clocks for the SATA controllers
> > would be children of an internal clock generated by the CPG.
> > Morimoto-san, do you have any information about this ?
> 
> If understand the h/w manual correctly, the external clock is connected
> directly to the SATA module:
> 
> "Pin Name: CICREFP0_SATA CICREFN0_SATA CICREFP1_SATA CICREFN1_SATA
> Description: Reference clock input to the PLL circuit in the Serial-ATA
> module (differential input). Apply a 100-MHz clock."

That's my understanding as well, but I suspect that clock to be the PHY clock 
only, not the SATA module functional clock.

> Morimoto-san, could you confirm, please?
> 
> >>   	[MSTP813] = SH_CLK_MSTP32(&p_clk, SMSTPCR8, 13, 0), /* Ether */
> >>   	[MSTP726] = SH_CLK_MSTP32(&zx_clk, SMSTPCR7, 26, 0), /* LVDS0 */
> >>   	[MSTP725] = SH_CLK_MSTP32(&zx_clk, SMSTPCR7, 25, 0), /* LVDS1 */
> >> @@ -321,6 +338,8 @@ static struct clk_lookup lookups[] = {
> >>   	CLKDEV_DEV_ID("pci-rcar-gen2.0", &mstp_clks[MSTP703]),
> >>   	CLKDEV_DEV_ID("pci-rcar-gen2.1", &mstp_clks[MSTP703]),
> >>   	CLKDEV_DEV_ID("pci-rcar-gen2.2", &mstp_clks[MSTP703]),
> >> +	CLKDEV_DEV_ID("sata-r8a7790.0", &mstp_clks[MSTP815]),
> >> +	CLKDEV_DEV_ID("sata-r8a7790.1", &mstp_clks[MSTP814]),
> >> 
> >>   	/* ICK */
> >>   	CLKDEV_ICK_ID("usbhs", "usb_phy_rcar_gen2", &mstp_clks[MSTP704]),

-- 
Regards,

Laurent Pinchart