| Message ID | 1384905197-3566-3-git-send-email-lho@apm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi, > + txbooster = <0x2 0x2 0x2 0x2 0x2 0x2>; > + txskew = <0xa 0xa 0xa 0xa 0xa 0xa>; [Loc Ho] These isn't correct. They should be txboostgain and txeyetuning. I will fix these in the next version. -Loc -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 20, 2013 at 7:53 AM, Loc Ho <lho@apm.com> wrote: > + > + sprintf(name, "xgene-ahci-phy-%08X", (u32) ctx->csr_phys); > + ctx->phy = devm_phy_create(ctx->dev, (u32) ctx->csr_phys, > + &xgene_phy_ops, name); The devm_phy_create() API has changed, so suggest you make your patch against -next tree. Thanks, -- Ming Lei
On Tue, Nov 19, 2013 at 11:53:15PM +0000, Loc Ho wrote: > Documentation: Add APM X-Gene SoC 6.0Gbps SATA PHY driver binding documentation > > Document the DTS binding for the X-Gene SoC SATA PHY driver. > > Signed-off-by: Loc Ho <lho@apm.com> > Signed-off-by: Tuan Phan <tphan@apm.com> > Signed-off-by: Suman Tripathi <stripathi@apm.com> > --- > .../devicetree/bindings/ata/apm-xgene-phy.txt | 61 ++++++++++++++++++++ > 1 files changed, 61 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/ata/apm-xgene-phy.txt > > diff --git a/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt b/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt > new file mode 100644 > index 0000000..bbae164 > --- /dev/null > +++ b/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt > @@ -0,0 +1,61 @@ > +* APM X-Gene 6.0 Gb/s SATA PHY nodes > + > +SATA PHY nodes are defined to describe on-chip Serial ATA PHY. Each SATA PHY > +(pair of PHY) has its own node. > + > +Required properties: > +- compatible : Shall be "apm,xgene-ahci-phy" or > + "apm,xgene-ahci-phy2". The "apm,xgene-ahci-phy" > + describes an port shared with SGMII Ethernet port. > + The "apm,xgene-ahci-phy2" describes an port not > + shared with SGMII and the PLL located at another > + memory resource region. Is there not a better name available than "apm,xgene-ahci-phy2"? > +- reg : First PHY memory resource > + Second separate PHY PLL clock memory resource if > + type "apm,xgene-ahci-phy2" Is ths PLL actually part of the PHY, or does it just feed the PHY? > +- #phy-cells : Shall be 0 > + > +Optional properties: > +- status : Shall be "ok" if enabled or "na" if disabled. Default > + is "ok". Do not use "na", use the standard "disabled". > +- txeyetuning : Manual control to fine tune the capture of the serial > + bit lines from the automatic calibrated position. > + Two set of 3-tuple setting for Gen1, Gen2, and Gen3. > + Range from 0 to 0x7f. Default is 0xa. If you have a property for which the name consists of multiple words, split the words with '-'. What does this actually mean? What units are these values in? What effect do those values have? > +- txeyedirection : Eye tuning manual control direction. 0 means sample > + data earlier than the nominal sampling point. 1 means > + sample data later than the nominal sampling point. > + Two set of 3-tuple setting for Gen1, Gen2, and Gen3. > + Default is 0x0. Likewise, use '-'. s/than/then/ ? > +- txboostgain : Frequency boost and DC gain control. Two set of > + 3-tuple setting for Gen1, Gen2, and Gen3. Range is > + between 0 to 0x1f. Default is 0x3. Use '-'. What units are these values in? > +- txspeed : Tx operating speed. Two set of 3-tuple for > + Gen1 (0x1), Gen2 (0x3), and Gen3 (0x7). Default is > + 0x7. Units? > + > +NOTE: PHY override parameters are board specific setting. > + > +Example: > + sataphy0: sataphy@1f210000 { > + compatible = "apm,xgene-ahci-phy"; > + reg = <0x0 0x1f210000 0x0 0x10000>; > + #phy-cells = <0>; > + status = "na"; Please eradicate all instances of "na". > + }; > + > + sataphy1: sataphy@1f220000 { > + compatible = "apm,xgene-ahci-phy"; > + reg = <0x0 0x1f220000 0x0 0x10000>; > + #phy-cells = <0>; > + status = "ok"; > + }; > + > + sataphy2: sataphy@1f230000 { > + compatible = "apm,xgene-ahci-phy2"; > + reg = <0x0 0x1f230000 0x0 0x10000 > + 0x0 0x1f2d0000 0x0 0x10000 >; Nit: bracket list entries individually, and don't apply random spacing: reg = <0x0 0x1f230000 0x0 0x10000>, <0x0 0x1f2d0000 0x0 0x10000>; Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Nov 19, 2013 at 11:53:16PM +0000, Loc Ho wrote: > This patch adds support for APM X-Gene SoC 6.0Gbps SATA PHY. This is the > physical layer interface for the corresponding SATA host controller. This > driver uses the new PHY generic framework posted by Kishon Vijay Abrahm. > > Signed-off-by: Loc Ho <lho@apm.com> > Signed-off-by: Tuan Phan <tphan@apm.com> > Signed-off-by: Suman Tripathi <stripathi@apm.com> > --- As a general note, this patch is very difficult to review. There are a large number of macros for accessing bits of registers, vast swathes of register reads and writes which all look very similar, and while I suspect could be factored out further into more descriptive helpers I don't know enough about the hardware to even figure out if any of it is correct as it stands. Many of the comments provide little to no information, and comments next to variables seem to be just the variable name with additional spaces. There are a few inconsistencies in style which are simply jarring (e.g. randomly jumping between lower case and upper case in prints, using different loop structures when repeatedly performing an action for calibration), and preferably would be more uniform. [...] > +struct xgene_ahci_phy_ctx { > + struct device *dev; > + struct phy *phy; > + u64 csr_phys; /* Physical address of PHY CSR base addr */ > + void *csr_base; /* PHY CSR base addr */ > + void *pcie_base; /* PHY CSR base addr with PCIE clock macro */ Should these not have an __iomem annotation? Elsewhere too... > + > + /* Override Serdes parameters */ > + u32 speed[MAX_CHANNEL]; /* Index for override parameter per channel */ > + u32 txspeed[3]; /* Tx speed */ > + u32 txboostgain[MAX_CHANNEL*3]; /* Tx freq boost and gain control */ > + u32 txeyetuning[MAX_CHANNEL*3]; /* Tx eye tuning */ > + u32 txeyedirection[MAX_CHANNEL*3]; /* Tx eye tuning direction */ > +}; > + > +/* Manual calibration is required for chip that is earlier than A3. > + To enable, pass boot argument sata_xgene_phy.manual=1 */ > +static int enable_manual_cal; > +MODULE_PARM_DESC(manual, "Enable manual calibration (1=enable 0=disable)"); > +module_param_named(manual, enable_manual_cal, int, 0444); > + > +static void phy_rd(void *addr, u32 *val) > +{ > + *val = readl(addr); > +#if defined(XGENE_DBG1_CSR) > + pr_debug("SATAPHY CSR RD: 0x%p value: 0x%08x\n", addr, *val); Can you not use dev_dbg here as you do elsehere? Either that or make your own debug print function, with the ifdefs hidden in its definition. [...] > +static void phy_wr_flush(void *addr, u32 val) > +{ > + writel(val, addr); > +#if defined(XGENE_DBG1_CSR) > + pr_debug("SATAPHY CSR WR: 0x%p value: 0x%08x\n", addr, val); > +#endif > + val = readl(addr); /* Force an barrier */ Nit: s/an barrier/a barrier/g > +} > + > +static void serdes_wr(void *csr_base, u32 indirect_cmd_reg, > + u32 indirect_data_reg, u32 addr, u32 data) > +{ > + u32 val; > + u32 cmd; > + > + cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK; > + cmd = (addr << 4) | cmd; > + phy_wr(csr_base + indirect_data_reg, data); > + phy_rd(csr_base + indirect_data_reg, &val); /* Force a barrier */ > + phy_wr(csr_base + indirect_cmd_reg, cmd); > + phy_rd(csr_base + indirect_cmd_reg, &val); /* Force a barrier */ > + /* Add an barrier - very important please don't remove */ > + mb(); Why? Either justify this or remove it. [...] > +static void serdes_rd(void *csr_base, u32 indirect_cmd_reg, > + u32 indirect_data_reg, u32 addr, u32 *data) > +{ > + u32 val; > + u32 cmd; > + > + cmd = CFG_IND_RD_CMD_MASK | CFG_IND_CMD_DONE_MASK; > + cmd = (addr << 4) | cmd; > + phy_wr(csr_base + indirect_cmd_reg, cmd); > + phy_rd(csr_base + indirect_cmd_reg, &val); /* Force a barrier */ > + /* Add an barrier - very important please don't remove */ > + mb(); Why? Please justify why I shouldn't just remove it. > + /* Ignore one read */ > + phy_rd(csr_base + indirect_cmd_reg, &val); Why? [...] > +static int xgene_phy_cal_rdy_check(void *csr_serdes, > + void (*serdes_rd) (void *, u32, u32 *), > + void (*serdes_wr) (void *, u32, u32), > + void (*serdes_toggle1to0) (void *, u32, u32)) > +{ > + int loopcount; > + u32 val; > + > + if (!enable_manual_cal) > + goto skip_manual_cal; > + > + /* TERM CALIBRATION CH0 */ > + serdes_rd(csr_serdes, KC_CLKMACRO_CMU_REGS_CMU_REG17_ADDR, &val); > + val = CMU_REG17_PVT_CODE_R2A_SET(val, 0x12); > + val = CMU_REG17_RESERVED_7_SET(val, 0x0); > + serdes_wr(csr_serdes, KC_CLKMACRO_CMU_REGS_CMU_REG17_ADDR, val); > + serdes_toggle1to0(csr_serdes, KC_CLKMACRO_CMU_REGS_CMU_REG17_ADDR, > + CMU_REG17_PVT_TERM_MAN_ENA_MASK); > + /* DOWN CALIBRATION CH0 */ > + serdes_rd(csr_serdes, KC_CLKMACRO_CMU_REGS_CMU_REG17_ADDR, &val); > + val = CMU_REG17_PVT_CODE_R2A_SET(val, 0x26); > + val = CMU_REG17_RESERVED_7_SET(val, 0x0); > + serdes_wr(csr_serdes, KC_CLKMACRO_CMU_REGS_CMU_REG17_ADDR, val); > + serdes_toggle1to0(csr_serdes, KC_CLKMACRO_CMU_REGS_CMU_REG16_ADDR, > + CMU_REG16_PVT_DN_MAN_ENA_MASK); > + /* UP CALIBRATION CH0 */ > + serdes_rd(csr_serdes, KC_CLKMACRO_CMU_REGS_CMU_REG17_ADDR, &val); > + val = CMU_REG17_PVT_CODE_R2A_SET(val, 0x28); > + val = CMU_REG17_RESERVED_7_SET(val, 0x0); > + serdes_wr(csr_serdes, KC_CLKMACRO_CMU_REGS_CMU_REG17_ADDR, val); > + serdes_toggle1to0(csr_serdes, KC_CLKMACRO_CMU_REGS_CMU_REG16_ADDR, > + CMU_REG16_PVT_UP_MAN_ENA_MASK); > + > +skip_manual_cal: > + /* Check PLL calibration for 10ms */ > + loopcount = 50; > + do { > + serdes_rd(csr_serdes, KC_CLKMACRO_CMU_REGS_CMU_REG7_ADDR, &val); > + if (CMU_REG7_PLL_CALIB_DONE_RD(val)) > + return 0; > + udelay(200); /* No need to poll faster than 200 us */ Why? Does the hardware take that long to respond? If so, mention that. > + } while (!CMU_REG7_PLL_CALIB_DONE_RD(val) && --loopcount > 0); > + > + return -1; The return value never seems to be used by callers. If this fails, how bad is it to continue as if this had succeeded? [...] > +/* SATA port macro configuration */ > +static int xgene_phy_sata_macro_cfg(struct xgene_ahci_phy_ctx *ctx) > +{ > + void *csr_serdes = ctx->csr_base + SATA_SERDES_OFFSET; > + int calib_loop_count = 0; > + u32 val; > + > + phy_rd(csr_serdes + SATA_ENET_CLK_MACRO_REG_ADDR, &val); > + val = I_RESET_B_SET(val, 0x0); > + val = I_PLL_FBDIV_SET(val, 0x27); > + val = I_CUSTOMEROV_SET(val, 0x0); > + phy_wr(csr_serdes + SATA_ENET_CLK_MACRO_REG_ADDR, val); > + > + xgene_phy_macro_cfg(csr_serdes, sds_rd, sds_wr, 1 /* 100MHz ref */); > + > + phy_rd(csr_serdes + SATA_ENET_CLK_MACRO_REG_ADDR, &val); > + val = I_RESET_B_SET(val, 0x1); > + val = I_CUSTOMEROV_SET(val, 0x0); > + phy_wr(csr_serdes + SATA_ENET_CLK_MACRO_REG_ADDR, val); > + > + mb(); Please comment all barriers with a description of why they are necessary. > + > + while (++calib_loop_count <= 5) { > + if (xgene_phy_macro_cal_rdy_chk(ctx) == 0) > + break; > + xgene_phy_macro_pdwn_force_vco(ctx); > + } Why 5 times? Does the hardware take some time to respond? I didn't spot any explicitly delays in the callees. > + if (calib_loop_count > 5) { > + dev_err(ctx->dev, "Clock macro PLL not ready...\n"); > + return -1; > + } > + phy_rd(csr_serdes + SATA_ENET_CLK_MACRO_REG_ADDR, &val); > + dev_dbg(ctx->dev, "Clock macro PLL %sLOOKED...\n", Perhaps "locked"? > + O_PLL_LOCK_RD(val) ? "" : "UN"); > + dev_dbg(ctx->dev, "Clock macro PLL %sREADY...\n", > + O_PLL_READY_RD(val) ? "" : "NOT"); Why is part of this in UPPERCASE? [...] > + xgene_phy_macro_cfg(pcie_base, sds_pcie_rd, sds_pcie_wr, > + 0 /* 50Mhz clock */); The fact that you have a comment on each call to xgene_phy_macro_cfg shows that the prototype can be improved. Take a flags parameter instead of the ref_100MHz parameter, and have some defines like PHY_CLK_50MHZ and PHY_CLK_100MHZ. That will make things much clearer. > + > + phy_rd(pcie_base + SM_PCIE_X8_SDS_CSR_REGS_PCIE_CLK_MACRO_REG_ADDR, > + &val); > + val = PCIE_CLK_MACRO_REG_I_RESET_B_SET(val, 0x1); > + val = PCIE_CLK_MACRO_REG_I_CUSTOMEROV_SET(val, 0x0); > + phy_wr(pcie_base + SM_PCIE_X8_SDS_CSR_REGS_PCIE_CLK_MACRO_REG_ADDR, > + val); > + > + mb(); Why? > + > + /* Check for 5 times */ > + calib_loop_count = 5; > + do { > + if (xgene_phy_pcie_macro_cal_rdy_chk(ctx) == 0) > + break; > + xgene_phy_pcie_macro_pdwn_force_vco(ctx); > + } while (--calib_loop_count > 0); Why 5 times? > + if (calib_loop_count <= 0) { > + dev_err(ctx->dev, "Clock macro PLL failed\n"); > + return -1; > + } > + phy_rd(pcie_base + SM_PCIE_X8_SDS_CSR_REGS_PCIE_CLK_MACRO_REG_ADDR, > + &val); > + dev_dbg(ctx->dev, "Clock macro PLL %sLOOKED...\n", > + PCIE_CLK_MACRO_REG_O_PLL_LOCK_RD(val) ? "" : "UN"); > + dev_dbg(ctx->dev, "Clock macro PLL %sREADY...\n", > + PCIE_CLK_MACRO_REG_O_PLL_READY_RD(val) ? "" : "NOT "); Again, why does the sentence suddenly switch to UPPERCASE? [...] > +static void xgene_phy_force_lat_summer_cal(struct xgene_ahci_phy_ctx *ctx, > + int channel) > +{ > + void *csr_base = ctx->csr_base + SATA_SERDES_OFFSET; > + u32 os = channel * 0x200; > + int i; > + struct { > + u32 reg; > + u32 val; > + } serdes_reg[] = { > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG38_ADDR, 0x0}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG39_ADDR, 0xff00}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG40_ADDR, 0xffff}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG41_ADDR, 0xffff}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG42_ADDR, 0xffff}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG43_ADDR, 0xffff}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG44_ADDR, 0xffff}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG45_ADDR, 0xffff}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG46_ADDR, 0xffff}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG47_ADDR, 0xfffc}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG48_ADDR, 0x0}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG49_ADDR, 0x0}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG50_ADDR, 0x0}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG51_ADDR, 0x0}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG52_ADDR, 0x0}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG53_ADDR, 0x0}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG54_ADDR, 0x0}, > + {KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG55_ADDR, 0x0}, > + {0, 0x0}, > + }; > + > + /* SUMMER CALIBRATION CH0/CH1 */ > + /* SUMMER calib toggle CHX */ > + sds_toggle1to0(csr_base, > + KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG127_ADDR + os, > + CH0_RXTX_REG127_FORCE_SUM_CAL_START_MASK); > + /* latch calib toggle CHX */ > + sds_toggle1to0(csr_base, > + KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG127_ADDR + os, > + CH0_RXTX_REG127_FORCE_LAT_CAL_START_MASK); > + /* CHX */ > + sds_wr(csr_base, KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG28_ADDR + os, 0x7); > + sds_wr(csr_base, KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG31_ADDR + os, > + 0x7e00); > + > + sds_clrbits(csr_base, KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG4_ADDR + os, > + CH0_RXTX_REG4_TX_LOOPBACK_BUF_EN_MASK); > + sds_clrbits(csr_base, KC_SERDES_X2_RXTX_REGS_CH0_RXTX_REG7_ADDR + os, > + CH0_RXTX_REG7_LOOP_BACK_ENA_CTLE_MASK); > + > + /* RXTX_REG38-55 */ > + for (i = 0; serdes_reg[i].reg != 0; i++) > + sds_wr(csr_base, serdes_reg[i].reg + os, serdes_reg[i].val); As the size is well known in advance, why not use ARRAY_SIZE rather than having a terminating element: for (i = 0; i < ARRAY_SIZE(serdes_reg); i++) sds_wr(...); [...] > +static void xgene_phy_gen_avg_val(struct xgene_ahci_phy_ctx *ctx, int channel) > +{ > + void *csr_serdes_base = ctx->csr_base + SATA_SERDES_OFFSET; > + int avg_loop = 10; What is value meant to mean? How was it derived? > + int MAX_LOOP = 10; Why UPPERCASE? That violates the principle of least surprise, I'd expect a name in uppercase to be a #defined value. [...] > + dev_dbg(ctx->dev, "Interation Value: %d\n", avg_loop); s/Interation/Iteration/g Even then, why not just say something like "took %d loops to ${DO THING}"? [...] > +skip_manual_cal: > + /* Check for 10 ms */ > + loopcount = 50; > + do { > + sds_rd(csr_serdes, KC_SERDES_CMU_REGS_CMU_REG7_ADDR, &val); > + if (CMU_REG7_PLL_CALIB_DONE_RD(val)) > + break; > + udelay(200); /* No need to poll more than 200 us */ Please comment _why_ there's no need. > + } while (--loopcount > 0); > + > + sds_rd(csr_serdes, KC_SERDES_CMU_REGS_CMU_REG7_ADDR, &val); > + if (CMU_REG7_PLL_CALIB_DONE_RD(val) == 1) > + dev_dbg(ctx->dev, "PLL calibration done\n"); > + if (CMU_REG7_VCO_CAL_FAIL_RD(val) == 0x0) { > + dev_dbg(ctx->dev, "PLL calibration successful\n"); > + } else { > + /* Assert SDS reset and recall calib function */ > + dev_err(ctx->dev, > + "PLL calibration failed due to VCO failure\n"); > + return -1; > + } > + > + dev_dbg(ctx->dev, "Checking TX ready\n"); > + sds_rd(csr_serdes, KC_SERDES_CMU_REGS_CMU_REG15_ADDR, &val); > + dev_dbg(ctx->dev, "PHY TX is %sready\n", val & 0x300 ? "" : "NOT "); Now we have MIXED case; why? [...] > + phy_rd(csr_serdes_base + SATA_ENET_SDS_PCS_CTL0_ADDR, &val); > + val = REGSPEC_CFG_I_RX_WORDMODE0_SET(val, 0x3); > + val = REGSPEC_CFG_I_TX_WORDMODE0_SET(val, 0x3); > + phy_wr(csr_serdes_base + SATA_ENET_SDS_PCS_CTL0_ADDR, val); > + > + mb(); Why? > + > + calib_loop_count = 10; > + do { > + rc = xgene_phy_cal_rdy_chk(ctx); > + if (rc == 0) > + break; > + xgene_phy_pdwn_force_vco(ctx); > + } while (++calib_loop_count > 0); > + if (calib_loop_count <= 0) { > + dev_err(ctx->dev, "PLL calibration failed\n"); > + return -ENODEV; > + } This loop and check are broken. Because calib_loop_count is signed, overflow is undefined behaviour. As calib_loop_count initialised to 10, and then only incremented, the only was it could become less than zero is upon overflow. Perhaps this should be calib_loop_count--, and then a test for calib_loop_count > 0. > +static int xgene_ahci_phy_hw_init(struct phy *phy) > +{ > + struct xgene_ahci_phy_ctx *ctx = phy_get_drvdata(phy); > + int rc; > + > + rc = xgene_phy_init(ctx, SATA_CLK_EXT_DIFF, 0 /* SSC */); It would be far nicer to make the call tell you everything you need to know about the arguments, rather than adding comments inside the call. > +static int xgene_ahci_phy_power_on(struct phy *phy) > +{ > + return 0; > +} > + > +static int xgene_ahci_phy_power_off(struct phy *phy) > +{ > + return 0; > +} I'm not entirely familiar with the PHY framework, but it seems odd to have these if they do nothing. Are there no regulators / resets / gpios to poke, registers to reset to sane values? > +static void xgene_ahci_phy_get_param(struct platform_device *pdev, > + const char *name, u32 *buffer, int count, u32 default_val) > +{ > + int rc; > + int i; > + rc = of_property_read_u32_array(pdev->dev.of_node, name, buffer, > + count); Why not return here if !rc? Then you don't need to indent the next statement. > + if (rc) { > + for (i = 0; i < count; i++) > + buffer[i] = default_val; > + } > +} > + > +static int xgene_ahci_phy_probe(struct platform_device *pdev) > +{ > + struct phy_provider *phy_provider; > + struct xgene_ahci_phy_ctx *ctx; > + struct resource *res; > + char name[80]; > + int rc = 0; > + > + if (!of_device_is_available(pdev->dev.of_node)) > + return -ENODEV; I was under the impression the standard bus types already checked the device was available, and would not allocate a device for those which were not. From a glance at drivers/of/platform.c, it seems this line is unnecessary. > + /* Load override paramaters */ > + xgene_ahci_phy_get_param(pdev, "txeyetuning", ctx->txeyetuning, 6, > + DEFAULT_TXEYETUNING); > + xgene_ahci_phy_get_param(pdev, "txeyedirection", ctx->txeyedirection, > + 6, DEFAULT_TXEYEDIRECTION); > + xgene_ahci_phy_get_param(pdev, "txboostgain", ctx->txboostgain, 6, > + DEFAULT_TXBOOST_GAIN); > + xgene_ahci_phy_get_param(pdev, "txspeed", ctx->txspeed, 3, > + DEFAULT_SPD_SEL); Further to my comments on the binding, these should probably also be prefixed with "apm,". > + ctx->speed[0] = 2; /* Default to Gen3 for channel 0 */ > + ctx->speed[1] = 2; /* Default to Gen3 for channel 1 */ > + > + ctx->dev = &pdev->dev; > + platform_set_drvdata(pdev, ctx); > + > + phy_provider = devm_of_phy_provider_register(ctx->dev, > + xgene_ahci_phy_xlate); > + if (IS_ERR(phy_provider)) { > + rc = PTR_ERR(phy_provider); > + goto error; > + } > + > + sprintf(name, "xgene-ahci-phy-%08X", (u32) ctx->csr_phys); Why cap this to 32 bits if this going to appear on a 64-bit system? > + ctx->phy = devm_phy_create(ctx->dev, (u32) ctx->csr_phys, > + &xgene_phy_ops, name); Likewise, this looks odd. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 19, 2013 at 11:53:17PM +0000, Loc Ho wrote: > arm64: Add APM X-Gene SoC 6.0Gbps SATA PHY DTS entries > > This patch adds the DTS entries for the APM X-Gene SoC 6.0Gbps SATA PHY > driver. The PHY for controller 0 and 1 are enabled by default. > > Signed-off-by: Loc Ho <lho@apm.com> > Signed-off-by: Tuan Phan <tphan@apm.com> > Signed-off-by: Suman Tripathi <stripathi@apm.com> > --- > arch/arm64/boot/dts/apm-storm.dtsi | 28 ++++++++++++++++++++++++++++ > 1 files changed, 28 insertions(+), 0 deletions(-) > > diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi > index 359d7b6..1d1c8bc 100644 > --- a/arch/arm64/boot/dts/apm-storm.dtsi > +++ b/arch/arm64/boot/dts/apm-storm.dtsi > @@ -193,5 +193,33 @@ > reg = <0x0 0x17000014 0x0 0x100>; > mask = <0x1>; > }; > + > + sataphy0: sataphy@1f210000 { > + compatible = "apm,xgene-ahci-phy"; > + reg = <0x0 0x1f210000 0x0 0x10000>; > + #phy-cells = <0>; > + status = "na"; s/"na"/"disabled"/g > + txbooster = <0x2 0x2 0x2 0x2 0x2 0x2>; > + txskew = <0xa 0xa 0xa 0xa 0xa 0xa>; > + }; > + > + sataphy1: sataphy@1f220000 { > + compatible = "apm,xgene-ahci-phy"; > + reg = <0x0 0x1f220000 0x0 0x10000>; > + #phy-cells = <0>; > + status = "ok"; > + txbooster = <0x2 0x2 0x2 0x2 0x2 0x2>; > + txskew = <0xa 0xa 0xa 0x5 0x5 0x5>; I don't recall seeing txskew in the binding document or code, but I see you've noticed too. > + }; > + > + sataphy2: sataphy@1f230000 { > + compatible = "apm,xgene-ahci-phy2"; > + reg = <0x0 0x1f230000 0x0 0x10000 > + 0x0 0x1f2d0000 0x0 0x10000>; Nit: please bracket entries individually: reg = <0x0 0x1f230000 0x0 0x10000>, <0x0 0x1f2d0000 0x0 0x10000>; Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 20 November 2013 11:37:05 Mark Rutland wrote: > > +static void phy_rd(void *addr, u32 *val) > > +{ > > + *val = readl(addr); > > +#if defined(XGENE_DBG1_CSR) > > + pr_debug("SATAPHY CSR RD: 0x%p value: 0x%08x\n", addr, *val); > > Can you not use dev_dbg here as you do elsehere? > > Either that or make your own debug print function, with the ifdefs > hidden in its definition. > pr_debug() is already conditional on #ifdef DEBUG, the extra #if can just get removed here. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey, guys. On Tue, Nov 19, 2013 at 04:53:16PM -0700, Loc Ho wrote: > This patch adds support for APM X-Gene SoC 6.0Gbps SATA PHY. This is the > physical layer interface for the corresponding SATA host controller. This > driver uses the new PHY generic framework posted by Kishon Vijay Abrahm. Hmm... this is related to ahci but isn't even a libata driver and is the first such driver proposed. Are we sure drivers/ata is the best home for this? Shouldn't we create a separate directory somewhere? It's confusing, at least to me, to have non-libata driver under drivers/ata. Thanks.
On Wednesday 20 November 2013, Tejun Heo wrote: > On Tue, Nov 19, 2013 at 04:53:16PM -0700, Loc Ho wrote: > > This patch adds support for APM X-Gene SoC 6.0Gbps SATA PHY. This is the > > physical layer interface for the corresponding SATA host controller. This > > driver uses the new PHY generic framework posted by Kishon Vijay Abrahm. > > Hmm... this is related to ahci but isn't even a libata driver and is > the first such driver proposed. Are we sure drivers/ata is the best > home for this? Shouldn't we create a separate directory somewhere? > It's confusing, at least to me, to have non-libata driver under > drivers/ata. It needs to be in drivers/phy, which is currently being prepared for the next merge window and which contains the generic interface used in this driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Arnd. On Wed, Nov 20, 2013 at 10:41 AM, Arnd Bergmann <arnd@arndb.de> wrote: > It needs to be in drivers/phy, which is currently being prepared for > the next merge window and which contains the generic interface used > in this driver. Ah, cool, so I don't need to worry about this one, right? Thanks a lot. :)
Hello. On 20-11-2013 3:53, Loc Ho wrote: > arm64: Add APM X-Gene SoC 6.0Gbps SATA PHY DTS entries > This patch adds the DTS entries for the APM X-Gene SoC 6.0Gbps SATA PHY > driver. The PHY for controller 0 and 1 are enabled by default. > Signed-off-by: Loc Ho <lho@apm.com> > Signed-off-by: Tuan Phan <tphan@apm.com> > Signed-off-by: Suman Tripathi <stripathi@apm.com> > --- > arch/arm64/boot/dts/apm-storm.dtsi | 28 ++++++++++++++++++++++++++++ > 1 files changed, 28 insertions(+), 0 deletions(-) > diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi > index 359d7b6..1d1c8bc 100644 > --- a/arch/arm64/boot/dts/apm-storm.dtsi > +++ b/arch/arm64/boot/dts/apm-storm.dtsi > @@ -193,5 +193,33 @@ > reg = <0x0 0x17000014 0x0 0x100>; > mask = <0x1>; > }; > + > + sataphy0: sataphy@1f210000 { The nodes should better be named like "sata-phy@1f210000" to better comply with the "ethernet-phy" name we see in the ePAPR spec. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 20 November 2013, Tejun Heo wrote: > On Wed, Nov 20, 2013 at 10:41 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > It needs to be in drivers/phy, which is currently being prepared for > > the next merge window and which contains the generic interface used > > in this driver. > > Ah, cool, so I don't need to worry about this one, right? Right, we just need to make sure the patches are staged the right way when they go through different subsystem maintainers. There should not be any build-time dependencies, so I think it won't be an issue. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 20 November 2013, Sergei Shtylyov wrote: > > --- a/arch/arm64/boot/dts/apm-storm.dtsi > > +++ b/arch/arm64/boot/dts/apm-storm.dtsi > > @@ -193,5 +193,33 @@ > > reg = <0x0 0x17000014 0x0 0x100>; > > mask = <0x1>; > > }; > > + > > + sataphy0: sataphy@1f210000 { > > The nodes should better be named like "sata-phy@1f210000" to better comply > with the "ethernet-phy" name we see in the ePAPR spec. Well, since the phy can be used for other devices, I'd rather name it just "phy", or possibly "serdes". The fact that it happens to be used for sata here should not influence the naming when both the phy driver and the hardware are completely generic. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, I will move to the official GIT for 3.12.00 instead rc7. It is the same as Linux-next GIT. -Loc On Wed, Nov 20, 2013 at 12:57 AM, Ming Lei <tom.leiming@gmail.com> wrote: > On Wed, Nov 20, 2013 at 7:53 AM, Loc Ho <lho@apm.com> wrote: >> + >> + sprintf(name, "xgene-ahci-phy-%08X", (u32) ctx->csr_phys); >> + ctx->phy = devm_phy_create(ctx->dev, (u32) ctx->csr_phys, >> + &xgene_phy_ops, name); > > The devm_phy_create() API has changed, so suggest you make your > patch against -next tree. > > Thanks, > -- > Ming Lei > > -- > Ming Lei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, >> +- compatible : Shall be "apm,xgene-ahci-phy" or >> + "apm,xgene-ahci-phy2". The "apm,xgene-ahci-phy" >> + describes an port shared with SGMII Ethernet port. >> + The "apm,xgene-ahci-phy2" describes an port not >> + shared with SGMII and the PLL located at another >> + memory resource region. > > Is there not a better name available than "apm,xgene-ahci-phy2"? [Loc Ho] I will name it apm,xgene-ahci-phyx. >> +- reg : First PHY memory resource >> + Second separate PHY PLL clock memory resource if >> + type "apm,xgene-ahci-phy2" > > Is ths PLL actually part of the PHY, or does it just feed the PHY? [Loc Ho] This is always the PLL clock macro in each IP. For the 3rd controller, we require an extra clock macro and can NOT use the original clock macro as it is not mux'ed with the SGMII port. > >> +- txeyetuning : Manual control to fine tune the capture of the serial >> + bit lines from the automatic calibrated position. >> + Two set of 3-tuple setting for Gen1, Gen2, and Gen3. >> + Range from 0 to 0x7f. Default is 0xa. > > If you have a property for which the name consists of multiple words, > split the words with '-'. > > What does this actually mean? > > What units are these values in? > > What effect do those values have? [Loc Ho] Will provide in next version. > >> +- txeyedirection : Eye tuning manual control direction. 0 means sample >> + data earlier than the nominal sampling point. 1 means >> + sample data later than the nominal sampling point. >> + Two set of 3-tuple setting for Gen1, Gen2, and Gen3. >> + Default is 0x0. > > Likewise, use '-'. > > s/than/then/ ? [Loc Ho] I believe that than is appropriate here a we are doing an comparison. -Loc -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > On Wednesday 20 November 2013, Tejun Heo wrote: >> On Tue, Nov 19, 2013 at 04:53:16PM -0700, Loc Ho wrote: >> > This patch adds support for APM X-Gene SoC 6.0Gbps SATA PHY. This is the >> > physical layer interface for the corresponding SATA host controller. This >> > driver uses the new PHY generic framework posted by Kishon Vijay Abrahm. >> >> Hmm... this is related to ahci but isn't even a libata driver and is >> the first such driver proposed. Are we sure drivers/ata is the best >> home for this? Shouldn't we create a separate directory somewhere? >> It's confusing, at least to me, to have non-libata driver under >> drivers/ata. > > It needs to be in drivers/phy, which is currently being prepared for > the next merge window and which contains the generic interface used > in this driver. [Loc Ho] I will move it to drivers/phy. The PHY framework did make it to the final release of 3.12.00. -Loc -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 20, 2013 at 08:07:45PM +0000, Loc Ho wrote: > Hi, > > >> +- compatible : Shall be "apm,xgene-ahci-phy" or > >> + "apm,xgene-ahci-phy2". The "apm,xgene-ahci-phy" > >> + describes an port shared with SGMII Ethernet port. > >> + The "apm,xgene-ahci-phy2" describes an port not > >> + shared with SGMII and the PLL located at another > >> + memory resource region. > > > > Is there not a better name available than "apm,xgene-ahci-phy2"? > [Loc Ho] > I will name it apm,xgene-ahci-phyx. > > >> +- reg : First PHY memory resource > >> + Second separate PHY PLL clock memory resource if > >> + type "apm,xgene-ahci-phy2" > > > > Is ths PLL actually part of the PHY, or does it just feed the PHY? > [Loc Ho] > This is always the PLL clock macro in each IP. For the 3rd controller, > we require an extra clock macro and can NOT use the original clock > macro as it is not mux'ed with the SGMII port. Ok. Is the reference clock into the PLL always on, or might we need to describe it? > > > > >> +- txeyetuning : Manual control to fine tune the capture of the serial > >> + bit lines from the automatic calibrated position. > >> + Two set of 3-tuple setting for Gen1, Gen2, and Gen3. > >> + Range from 0 to 0x7f. Default is 0xa. > > > > If you have a property for which the name consists of multiple words, > > split the words with '-'. > > > > What does this actually mean? > > > > What units are these values in? > > > > What effect do those values have? > [Loc Ho] > Will provide in next version. Cheers. > > > > >> +- txeyedirection : Eye tuning manual control direction. 0 means sample > >> + data earlier than the nominal sampling point. 1 means > >> + sample data later than the nominal sampling point. > >> + Two set of 3-tuple setting for Gen1, Gen2, and Gen3. > >> + Default is 0x0. > > > > Likewise, use '-'. > > > > s/than/then/ ? > [Loc Ho] > I believe that than is appropriate here a we are doing an comparison. Apologies, you are corerct; I completely misread this. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt b/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt new file mode 100644 index 0000000..bbae164 --- /dev/null +++ b/Documentation/devicetree/bindings/ata/apm-xgene-phy.txt @@ -0,0 +1,61 @@ +* APM X-Gene 6.0 Gb/s SATA PHY nodes + +SATA PHY nodes are defined to describe on-chip Serial ATA PHY. Each SATA PHY +(pair of PHY) has its own node. + +Required properties: +- compatible : Shall be "apm,xgene-ahci-phy" or + "apm,xgene-ahci-phy2". The "apm,xgene-ahci-phy" + describes an port shared with SGMII Ethernet port. + The "apm,xgene-ahci-phy2" describes an port not + shared with SGMII and the PLL located at another + memory resource region. +- reg : First PHY memory resource + Second separate PHY PLL clock memory resource if + type "apm,xgene-ahci-phy2" +- #phy-cells : Shall be 0 + +Optional properties: +- status : Shall be "ok" if enabled or "na" if disabled. Default + is "ok". +- txeyetuning : Manual control to fine tune the capture of the serial + bit lines from the automatic calibrated position. + Two set of 3-tuple setting for Gen1, Gen2, and Gen3. + Range from 0 to 0x7f. Default is 0xa. +- txeyedirection : Eye tuning manual control direction. 0 means sample + data earlier than the nominal sampling point. 1 means + sample data later than the nominal sampling point. + Two set of 3-tuple setting for Gen1, Gen2, and Gen3. + Default is 0x0. +- txboostgain : Frequency boost and DC gain control. Two set of + 3-tuple setting for Gen1, Gen2, and Gen3. Range is + between 0 to 0x1f. Default is 0x3. +- txspeed : Tx operating speed. Two set of 3-tuple for + Gen1 (0x1), Gen2 (0x3), and Gen3 (0x7). Default is + 0x7. + +NOTE: PHY override parameters are board specific setting. + +Example: + sataphy0: sataphy@1f210000 { + compatible = "apm,xgene-ahci-phy"; + reg = <0x0 0x1f210000 0x0 0x10000>; + #phy-cells = <0>; + status = "na"; + }; + + sataphy1: sataphy@1f220000 { + compatible = "apm,xgene-ahci-phy"; + reg = <0x0 0x1f220000 0x0 0x10000>; + #phy-cells = <0>; + status = "ok"; + }; + + sataphy2: sataphy@1f230000 { + compatible = "apm,xgene-ahci-phy2"; + reg = <0x0 0x1f230000 0x0 0x10000 + 0x0 0x1f2d0000 0x0 0x10000 >; + #phy-cells = <0>; + status = "ok"; + }; +