| Message ID | 1393004853-25994-2-git-send-email-lho@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, Loc. On Fri, Feb 21, 2014 at 10:47:32AM -0700, Loc Ho wrote: > +/** > + * Custom Query ID command > + * > + * Due to HW errata, we must stop and re-start the port state machine after > + * read ID command. Also disable support for DEVSLP as hardware don't support > + * it. > + */ Sorry about not being clear before but /** function comment means something like /** * ata_scsi_port_error_handler - recover the port after the commands * @host: SCSI host containing the port * @ap: the ATA port * * Handle the recovery of the port @ap after all the commands have * been recovered. */ > +static int xgene_ahci_do_hardreset(struct ata_link *link, > + unsigned long deadline, bool *online) > +{ > + const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context); > + struct ata_port *ap = link->ap; > + struct ahci_host_priv *hpriv = ap->host->private_data; > + struct xgene_ahci_context *ctx = pdata_to_ctx(hpriv->plat_data); > + struct ahci_port_priv *pp = ap->private_data; > + u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG; > + void __iomem *port_mmio = ahci_port_base(ap); > + struct ata_taskfile tf; > + int first_time = 1; > + int rc; > + u32 val; > + int i; > + > +hardreset_retry: > + /* clear D2H reception area to properly wait for D2H FIS */ > + ata_tf_init(link->device, &tf); > + tf.command = ATA_BUSY; > + ata_tf_to_fis(&tf, 0, 0, d2h_fis); > + rc = sata_link_hardreset(link, timing, deadline, online, > + ahci_check_ready); > + > + if (*online) { > + /* Check to ensure that the disk comes up in matching speed */ > + if (first_time) { > + u32 gen_speed; > + > + first_time = 0; > + sata_scr_read(link, SCR_STATUS, &gen_speed); > + gen_speed = (gen_speed >> 4) & 0xf; > + if (gen_speed == 1 || gen_speed == 2) { > + /* > + * For Gen2/1 and first time, let's check again > + * with Gen2/1 PHY to ensure actual Gen2/1 disk. > + */ Can you please go back two reviews and re-read what I requested? Also, if you're unsure, please don't hesitate to ask back. It's usually a lot easier for both parties than iterating through patchsets without properly understanding each other. Thanks.
Hello, Loc. On Fri, Feb 21, 2014 at 10:43:52AM -0800, Loc Ho wrote: > Do you want this for all functions or only one that with comment? Also, > for both drivers - host and PHY drivres? Oh, no need to do that for every function. It's just customary to use function comment when there's sufficient amount to explain for the whole function. > > Can you please go back two reviews and re-read what I requested? > > Also, if you're unsure, please don't hesitate to ask back. It's > > usually a lot easier for both parties than iterating through patchsets > > without properly understanding each other. > > > > Before posting this morning, I had gone over all response email from you > since we first interacted. I am not quite follow what you want here. Are > you suggesting that I should move this out as an errata patch? Hmmm... maybe I was too ambiguous. Because the behavior is quite unusual and can make the error handling behavior deviate, I think it deserves to explain 1. why such behavior is necessary and 2. what the implications are (e.g. in corner cases, how long it could add to reset timeout) and preferably 3. rationale for choosing this specific approach given #1 and #2, so that when someone else reads the code later on [s]he doesn't have to second-guess the original intention of the workaround. Thanks.
diff --git a/Documentation/devicetree/bindings/ata/apm-xgene.txt b/Documentation/devicetree/bindings/ata/apm-xgene.txt new file mode 100644 index 0000000..633eb3b --- /dev/null +++ b/Documentation/devicetree/bindings/ata/apm-xgene.txt @@ -0,0 +1,70 @@ +* APM X-Gene 6.0 Gb/s SATA host controller nodes + +SATA host controller nodes are defined to describe on-chip Serial ATA +controllers. Each SATA controller (pair of ports) have its own node. + +Required properties: +- compatible : Shall contain: + * "apm,xgene-ahci-sgmii" if mux'ed with SGMII + * "apm,xgene-ahci-pcie" if mux'ed with PCIe +- reg : First memory resource shall be the AHCI memory + resource. + Second memory resource shall be the host controller + memory resource. +- interrupts : Interrupt-specifier for SATA host controller IRQ. +- clocks : Reference to the clock entry. +- phys : A list of phandles + phy-specifiers, one for each + entry in phy-names. +- phy-names : Should contain: + * "sata-6g" for the SATA 6.0Gbps PHY + +Optional properties: +- status : Shall be "ok" if enabled or "disabled" if disabled. + Default is "ok". +- interrupt-parent : Interrupt controller. + +Example: + sataclk: sataclk { + compatible = "fixed-clock"; + #clock-cells = <1>; + clock-frequency = <100000000>; + clock-output-names = "sataclk"; + }; + + phy2: phy@1f22a000 { + compatible = "apm,xgene-phy"; + reg = <0x0 0x1f22a000 0x0 0x100>, + <0x0 0x1f22c000 0x0 0x100>; + #phy-cells = <1>; + }; + + phy3: phy@1f23a000 { + compatible = "apm,xgene-phy"; + reg = <0x0 0x1f23a000 0x0 0x100>, + <0x0 0x1f23c000 0x0 0x100>; + #phy-cells = <1>; + }; + + sata2: sata@1a400000 { + compatible = "apm,xgene-ahci-sgmii"; + reg = <0x0 0x1a400000 0x0 0x1000>, + <0x0 0x1f220000 0x0 0x10000>; + interrupt-parent = <&gic>; + interrupts = <0x0 0x87 0x4>; + status = "ok"; + clocks = <&sataclk 0>; + phys = <&phy2 0>; + phy-names = "sata-6g"; + }; + + sata3: sata@1a800000 { + compatible = "apm,xgene-ahci-pcie"; + reg = <0x0 0x1a800000 0x0 0x1000>, + <0x0 0x1f230000 0x0 0x10000>; + interrupt-parent = <&gic>; + interrupts = <0x0 0x88 0x4>; + status = "ok"; + clocks = <&sataclk 0>; + phys = <&phy3 0>; + phy-names = "sata-6g"; + };