| Message ID | 1388708539-333-3-git-send-email-lho@apm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Friday 03 January 2014, Loc Ho wrote: > + > +/* Controller who PHY shared with SGMII Ethernet PHY */ > +#define XGENE_AHCI_SGMII_DTS "apm,xgene-ahci-sgmii" > + > +/* Controller who PHY (internal reference clock macro) shared with PCIe */ > +#define XGENE_AHCI_PCIE_DTS "apm,xgene-ahci-pcie" Kill these macros. I've commented on this in the past. Also, there really shouldn't be any difference to the SATA driver based on what PHY is used, so the strings shouldn't be different either. > +/* SATA host controller CSR */ > +#define SLVRDERRATTRIBUTES_ADDR 0x00000000 > +#define SLVWRERRATTRIBUTES_ADDR 0x00000004 > +#define MSTRDERRATTRIBUTES_ADDR 0x00000008 > +#define MSTWRERRATTRIBUTES_ADDR 0x0000000c > +#define BUSCTLREG_ADDR 0x00000014 Are these strings taken literally from the data sheet? You can probably drop the _ADDR part. > +#define MSTAWAUX_COHERENT_BYPASS_SET(dst, src) \ > + (((dst) & ~0x00000002) | (((u32)(src)<<1) & 0x00000002)) > +#define MSTARAUX_COHERENT_BYPASS_SET(dst, src) \ > + (((dst) & ~0x00000001) | (((u32)(src)) & 0x00000001)) The macros are only used once and don't really help readability. Just open-code them. If you have complex macros like these and use them multiple times, it may be better to use an inline function. > +static void xgene_ahci_enable_phy(struct xgene_ahci_context *ctx, > + int channel, int enable) > +{ > + void *mmio = ctx->mmio_base; > + u32 val; > + > + xgene_rd(mmio + PORTCFG_ADDR, &val); > + val = PORTADDR_SET(val, channel == 0 ? 2 : 3); > + xgene_wr_flush(mmio + PORTCFG_ADDR, val); > + xgene_rd(mmio + PORTPHY1CFG_ADDR, &val); > + val = PORTPHY1CFG_FRCPHYRDY_SET(val, enable); > + xgene_wr(mmio + PORTPHY1CFG_ADDR, val); > +} > + > +static void xgene_ahci_set_phy_cfg(struct xgene_ahci_context *ctx, int channel) > +{ > + void *mmio = ctx->mmio_base; > + u32 val; > + > + dev_dbg(ctx->dev, "port configure mmio 0x%p channel %d\n", > + mmio, channel); > + xgene_rd(mmio + PORTCFG_ADDR, &val); > + val = PORTADDR_SET(val, channel == 0 ? 2 : 3); > + xgene_wr_flush(mmio + PORTCFG_ADDR, val); > + /* Disable fix rate */ > + xgene_wr_flush(mmio + PORTPHY1CFG_ADDR, 0x0001fffe); > + xgene_wr_flush(mmio + PORTPHY2CFG_ADDR, 0x5018461c); > + xgene_wr_flush(mmio + PORTPHY3CFG_ADDR, 0x1c081907); > + xgene_wr_flush(mmio + PORTPHY4CFG_ADDR, 0x1c080815); > + xgene_rd(mmio + PORTPHY5CFG_ADDR, &val); > + /* Window negotiation 0x800 to 0x400 */ > + val = PORTPHY5CFG_RTCHG_SET(val, 0x300); > + xgene_wr_flush(mmio + PORTPHY5CFG_ADDR, val); > + xgene_rd(mmio + PORTAXICFG_ADDR, &val); > + val = PORTAXICFG_EN_CONTEXT_SET(val, 0x1); /* enable context mgmt */ > + val = PORTAXICFG_OUTTRANS_SET(val, 0xe); /* Outstanding */ > + xgene_wr_flush(mmio + PORTAXICFG_ADDR, val); > +} This looks like it should be part of the PHY driver? > + /* > + * When both ACPI and DTS are enabled, custom ACPI built-in ACPI > + * table, and booting via DTS, we need to skip the probe of the > + * built-in ACPI table probe. > + */ > + if (!efi_enabled(EFI_BOOT) && dev->of_node == NULL) > + return -ENODEV; I think I've commented on this one before too. The comment talks about ACPI, but the code is about EFI, which is completely unrelated. Neither the code nor the comment seems to make sense here and you wouldn't be in this function in the first place if the device is not defined in ACPI or in DT. > + /* Can't use devm_ioremap_resource due to overlapping region */ > + hpriv->csr_base = devm_ioremap(dev, res->start, resource_size(res)); > + if (!hpriv->csr_base) { > + dev_err(dev, "can't map %pR\n", res); > + return -ENOMEM; > + } Why are the regions overlapping? That should not happen! > + /* Select ATA */ > + if (of_device_is_compatible(pdev->dev.of_node, > + XGENE_AHCI_SGMII_DTS)) { > + if (xgene_ahci_mux_select(hpriv)) { > + dev_err(dev, "SATA mux selection failed\n"); > + return -ENODEV; > + } > + } What kind of mux is this? Why does the SATA driver need to care about it? It sounds like this should be part of the pinctrl driver. > + /* Setup DMA mask */ > + rc = dma_set_mask(dev, DMA_BIT_MASK(64)); > + if (rc) { > + dev_err(dev, "Unable to set dma mask\n"); > + goto error; > + } > + rc = dma_set_coherent_mask(dev, DMA_BIT_MASK(64)); > + if (rc) { > + dev_err(dev, "Unable to set dma coherent mask\n"); > + goto error; > + } dma_set_mask_and_coherent? > + > +static const struct acpi_device_id xgene_ahci_acpi_match[] = { > + {"APMC0D00", 0}, > + {}, > +}; > +MODULE_DEVICE_TABLE(acpi, xgene_ahci_acpi_match); Just drop the ACPI part for now. It's clear that the driver won't work with ACPI in this state, since it would need to handle the PHY and clock management completely differently. It's better to add the code once it has a chance to actually work. 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, >> +/* Controller who PHY shared with SGMII Ethernet PHY */ >> +#define XGENE_AHCI_SGMII_DTS "apm,xgene-ahci-sgmii" >> + >> +/* Controller who PHY (internal reference clock macro) shared with PCIe */ >> +#define XGENE_AHCI_PCIE_DTS "apm,xgene-ahci-pcie" > > Kill these macros. I've commented on this in the past. > > Also, there really shouldn't be any difference to the SATA driver > based on what PHY is used, so the strings shouldn't be different > either. The only need for this is the MUX when the PHY is shared between SGMII and SATA. If we get rip of the MUX code, we can drop this. MUX discussion is below. > >> +/* SATA host controller CSR */ >> +#define SLVRDERRATTRIBUTES_ADDR 0x00000000 >> +#define SLVWRERRATTRIBUTES_ADDR 0x00000004 >> +#define MSTRDERRATTRIBUTES_ADDR 0x00000008 >> +#define MSTWRERRATTRIBUTES_ADDR 0x0000000c >> +#define BUSCTLREG_ADDR 0x00000014 > > Are these strings taken literally from the data sheet? You can probably > drop the _ADDR part. These names are directly from the programming reference manual. I will get rip of the _ADDR for both drivers - host and PHY. > >> +#define MSTAWAUX_COHERENT_BYPASS_SET(dst, src) \ >> + (((dst) & ~0x00000002) | (((u32)(src)<<1) & 0x00000002)) >> +#define MSTARAUX_COHERENT_BYPASS_SET(dst, src) \ >> + (((dst) & ~0x00000001) | (((u32)(src)) & 0x00000001)) > > The macros are only used once and don't really help readability. > Just open-code them. If you have complex macros like these > and use them multiple times, it may be better to use an inline > function. For these particular one, I will get rip of them. > >> +static void xgene_ahci_enable_phy(struct xgene_ahci_context *ctx, >> + int channel, int enable) >> +{ >> + void *mmio = ctx->mmio_base; >> + u32 val; >> + >> + xgene_rd(mmio + PORTCFG_ADDR, &val); >> + val = PORTADDR_SET(val, channel == 0 ? 2 : 3); >> + xgene_wr_flush(mmio + PORTCFG_ADDR, val); >> + xgene_rd(mmio + PORTPHY1CFG_ADDR, &val); >> + val = PORTPHY1CFG_FRCPHYRDY_SET(val, enable); >> + xgene_wr(mmio + PORTPHY1CFG_ADDR, val); >> +} >> + >> +static void xgene_ahci_set_phy_cfg(struct xgene_ahci_context *ctx, int channel) >> +{ >> + void *mmio = ctx->mmio_base; >> + u32 val; >> + >> + dev_dbg(ctx->dev, "port configure mmio 0x%p channel %d\n", >> + mmio, channel); >> + xgene_rd(mmio + PORTCFG_ADDR, &val); >> + val = PORTADDR_SET(val, channel == 0 ? 2 : 3); >> + xgene_wr_flush(mmio + PORTCFG_ADDR, val); >> + /* Disable fix rate */ >> + xgene_wr_flush(mmio + PORTPHY1CFG_ADDR, 0x0001fffe); >> + xgene_wr_flush(mmio + PORTPHY2CFG_ADDR, 0x5018461c); >> + xgene_wr_flush(mmio + PORTPHY3CFG_ADDR, 0x1c081907); >> + xgene_wr_flush(mmio + PORTPHY4CFG_ADDR, 0x1c080815); >> + xgene_rd(mmio + PORTPHY5CFG_ADDR, &val); >> + /* Window negotiation 0x800 to 0x400 */ >> + val = PORTPHY5CFG_RTCHG_SET(val, 0x300); >> + xgene_wr_flush(mmio + PORTPHY5CFG_ADDR, val); >> + xgene_rd(mmio + PORTAXICFG_ADDR, &val); >> + val = PORTAXICFG_EN_CONTEXT_SET(val, 0x1); /* enable context mgmt */ >> + val = PORTAXICFG_OUTTRANS_SET(val, 0xe); /* Outstanding */ >> + xgene_wr_flush(mmio + PORTAXICFG_ADDR, val); >> +} > > This looks like it should be part of the PHY driver? There are the corresponding configure on the host controller side. It is not part of the PHY. Each host such as SATA, SGMII, or PCIe has its own version of these registers. It can be completely different between SATA and PCIe for example. > >> + /* >> + * When both ACPI and DTS are enabled, custom ACPI built-in ACPI >> + * table, and booting via DTS, we need to skip the probe of the >> + * built-in ACPI table probe. >> + */ >> + if (!efi_enabled(EFI_BOOT) && dev->of_node == NULL) >> + return -ENODEV; > > I think I've commented on this one before too. The comment talks > about ACPI, but the code is about EFI, which is completely unrelated. > Neither the code nor the comment seems to make sense here and > you wouldn't be in this function in the first place if the device > is not defined in ACPI or in DT. Let remove them for the time being. Unless one has an custom ACPI table built in the kernel, it isn't a problem. > >> + /* Can't use devm_ioremap_resource due to overlapping region */ >> + hpriv->csr_base = devm_ioremap(dev, res->start, resource_size(res)); >> + if (!hpriv->csr_base) { >> + dev_err(dev, "can't map %pR\n", res); >> + return -ENOMEM; >> + } > > Why are the regions overlapping? That should not happen! Each host controller/PHY includes the following resource: 0xXXXX.0000 for the host core register 0xXXXX.7000 for the mux select if shared with SGMII 0xXXXX.A000 for the PHY indirect access 0xXXXX.C000 for the host/PHY clocks 0xXXXX.D000 for the RAM shutdown removal As I only used one resource register of size 64K, it overlapped with the 0xXXXX.A000 mapped by the PHY already. If you believe it is better that I have two resources to map the host core/mux and the RAM shutdown removal resources, then ok. let me know. > >> + /* Select ATA */ >> + if (of_device_is_compatible(pdev->dev.of_node, >> + XGENE_AHCI_SGMII_DTS)) { >> + if (xgene_ahci_mux_select(hpriv)) { >> + dev_err(dev, "SATA mux selection failed\n"); >> + return -ENODEV; >> + } >> + } > > What kind of mux is this? Why does the SATA driver need to care about > it? It sounds like this should be part of the pinctrl driver. This is an internal mux that select either the SGMII or SATA. The default value is to select SGMII. Therefore, to enable SATA, it musts be cleared to 0. Are you suggesting that I write another driver to clear this register? It is an simple register writes - far too simple to worth an driver itself as it only applies to SATA. > >> + /* Setup DMA mask */ >> + rc = dma_set_mask(dev, DMA_BIT_MASK(64)); >> + if (rc) { >> + dev_err(dev, "Unable to set dma mask\n"); >> + goto error; >> + } >> + rc = dma_set_coherent_mask(dev, DMA_BIT_MASK(64)); >> + if (rc) { >> + dev_err(dev, "Unable to set dma coherent mask\n"); >> + goto error; >> + } > > dma_set_mask_and_coherent? okay. > >> + >> +static const struct acpi_device_id xgene_ahci_acpi_match[] = { >> + {"APMC0D00", 0}, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(acpi, xgene_ahci_acpi_match); > > Just drop the ACPI part for now. It's clear that the driver won't > work with ACPI in this state, since it would need to handle the > PHY and clock management completely differently. It's better to > add the code once it has a chance to actually work. > Okay... I will remove all ACPI stuff for now. -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
diff --git a/Documentation/devicetree/bindings/ata/apm-xgene.txt b/Documentation/devicetree/bindings/ata/apm-xgene.txt new file mode 100644 index 0000000..3d1421a --- /dev/null +++ b/Documentation/devicetree/bindings/ata/apm-xgene.txt @@ -0,0 +1,68 @@ +* 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 be "apm,xgene-ahci-sgmii" if mux'ed with SGMII + or "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 mapping for SATA host controller IRQ. +- clocks : Reference to the clock entry. +- phys : PHY reference with parameter 0. +- phy-names : Name of the PHY. Shall be "sata-6g". + +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-ext"; + reg = <0x0 0x1f23a000 0x0 0x100>, + <0x0 0x1f23c000 0x0 0x100>, + <0x0 0x1f2d0000 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"; + };