| Message ID | 1384465153-29902-3-git-send-email-lho@apm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thursday 14 November 2013, Loc Ho wrote: > +Required properties: > +- compatible : Shall be "apm,xgene-ahci" > +- reg : First memory resource shall be the AHCI memory > + resource. > + Second memory resource shall be the host controller > + memory resource. > +- id : Controller ID (0 = first, 1 = second, 2 = third) As in the PHY patch, it's probably best to drop the "id" property here and describe the actual differences between the instances directly, either by having distinct "compatible" properties for each model, or by adding binary flags to tell about a capability that only some instances have. Also, I think you should have a separate file for the sata binding, especially since the PHY binding is not actually specific to SATA at all. Aside from these, the binding looks good to me now. 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 Thursday 14 November 2013, Loc Ho wrote: > > +config SATA_XGENE > + tristate "APM X-Gene 6.0Gbps SATA host controller support" > + depends on SATA_AHCI_PLATFORM > + default y if ARM64 > + select SATA_XGENE_PHY I don't think the "default y" is appropriate here, since there are going to be lots of ARM64 platforms in the long run that don't have this. Also, there is a trend towards using CONFIG_COMPILE_TEST to limit the visibility. I'd suggest something like config SATA_XGENE tristate "APM X-Gene 6.0Gbps SATA host controller support" depends on ARM64 || COMPILE_TEST ... > +/* Enable for dumping CSR read/write access */ > +#undef XGENE_DBG_CSR This should be removed now as well, along with the debug macro you already removed. > + > +/* Temporary function until the PHY parameter API */ > +extern void xgene_ahci_phy_set_pq(struct phy *phy, int chan, int data); > +extern void xgene_ahci_phy_set_spd(struct phy *phy, int chan, int gen); Ah, that should have been mentioned in the changeset text. Do you think you can remove these for the final version? > +static void xgene_rd(void *addr, u32 *val) > +{ > + *val = readl(addr); > +#if defined(XGENE_DBG_CSR) > + printk(KERN_DEBUG "SATAPHY CSR RD: 0x%p value: 0x%08x\n", addr, *val); > +#endif > +} The interface you are looking for is pr_debug() or dev_dbg(), which get built-in only if the DEBUG macro is set. In a lot of cases, it's actually best to remove debug output like this from the driver once you have it working -- whoever is debugging problems in the driver next might need completely different debugging information or can add them back easily if needed. It's your choice if you want to use pr_debug() or remove that output entirely. If you remove it, best remove the helper functions entirely and use readl/writel directly. > +/* Flush the IOB to ensure all SATA controller writes completed before > + servicing the completed command. */ > +static int xgene_ahci_iob_flush(struct xgene_ahci_context *ctx) > +{ > + if (ctx->ahbc_io_base == NULL) { > + void *ahbc_base; > + u32 val; > + > + /* The AHBC address is fixed in X-Gene */ > + ahbc_base = devm_ioremap(ctx->dev, 0x1F2A0000, 0x80000); > + if (!ahbc_base) { > + dev_err(ctx->dev, "can't map AHBC resource\n"); > + return -ENODEV; > + } I had an important comment about the misuse of hardcoded I/O addresses here. Please never just post a new version with the same issue. In general, your options are: * rewrite the code in a more appropriate way * reply to the comment, arguing why you think your code is correct * ask for clarification if you don't know how to improve the code * mark the code as TODO and mention in the patch description why you are still working on this and that the current version should not be seen as final. If you don't do any of these, reviewers will get annoyed because you are wasting their time. > + > +static int xgene_ahci_get_u32_param(struct platform_device *pdev, > + const char *of_name, char *acpi_name, > + u32 *param) > +{ > +#ifdef CONFIG_ACPI > + if (efi_enabled(EFI_BOOT)) { > + unsigned long long value; > + acpi_status status; > + if (acpi_name == NULL) > + return -ENODEV; > + status = acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev), > + acpi_name, NULL, &value); > + if (ACPI_FAILURE(status)) > + return -ENODEV; > + *param = value; > + return 0; > + } > +#endif There are more previous comments that you have not addressed yet. > diff --git a/drivers/ata/sata_xgene.h b/drivers/ata/sata_xgene.h > new file mode 100644 > index 0000000..51e73f6 > --- /dev/null > +++ b/drivers/ata/sata_xgene.h This file should probably just be folded into the driver, since it contains no interfaces between modules, just internal definitions. 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, >> +config SATA_XGENE >> + tristate "APM X-Gene 6.0Gbps SATA host controller support" >> + depends on SATA_AHCI_PLATFORM >> + default y if ARM64 >> + select SATA_XGENE_PHY > > I don't think the "default y" is appropriate here, since there are going to > be lots of ARM64 platforms in the long run that don't have this. Also, > there is a trend towards using CONFIG_COMPILE_TEST to limit the > visibility. > > I'd suggest something like > > config SATA_XGENE > tristate "APM X-Gene 6.0Gbps SATA host controller support" > depends on ARM64 || COMPILE_TEST > ... > [Loc Ho] Okay. How about this? depends on ARM64 || COMPILE_TEST select SATA_XGENE_PHY && SATA_AHCI_PLATFORM >> +/* Enable for dumping CSR read/write access */ >> +#undef XGENE_DBG_CSR > > This should be removed now as well, along with the debug macro you already removed. [Loc Ho] This is very useful if one running into problem - one can enable all CSR dump. I would like to keep this around for the time being. > >> + >> +/* Temporary function until the PHY parameter API */ >> +extern void xgene_ahci_phy_set_pq(struct phy *phy, int chan, int data); >> +extern void xgene_ahci_phy_set_spd(struct phy *phy, int chan, int gen); > > Ah, that should have been mentioned in the changeset text. Do you think you > can remove these for the final version? [Loc Ho] Yes... Krishon, can you provide an patch that allows me to accommodate this? Or should I come up one? I will trying to get rip of the first one. But I will need the function to set the PHY speed - for Gen2/Gen1. Let's plan on add these function: a. int (*set_speed)(int gen_speed) where gen_speed is either AUTO, GEN1, GEN2, or GEN3. b. Change the PHY init function to take an optional parameter. This would help my first function in case I can get rip of it. > >> +static void xgene_rd(void *addr, u32 *val) >> +{ >> + *val = readl(addr); >> +#if defined(XGENE_DBG_CSR) >> + printk(KERN_DEBUG "SATAPHY CSR RD: 0x%p value: 0x%08x\n", addr, *val); >> +#endif >> +} > > The interface you are looking for is pr_debug() or dev_dbg(), which get > built-in only if the DEBUG macro is set. > > In a lot of cases, it's actually best to remove debug output like this from > the driver once you have it working -- whoever is debugging problems in > the driver next might need completely different debugging information or > can add them back easily if needed. > > It's your choice if you want to use pr_debug() or remove that output > entirely. If you remove it, best remove the helper functions entirely > and use readl/writel directly. [Loc Ho] I would like to keep this function for now until this driver actual being used in production. I will use pr_debug. > >> +/* Flush the IOB to ensure all SATA controller writes completed before >> + servicing the completed command. */ >> +static int xgene_ahci_iob_flush(struct xgene_ahci_context *ctx) >> +{ >> + if (ctx->ahbc_io_base == NULL) { >> + void *ahbc_base; >> + u32 val; >> + >> + /* The AHBC address is fixed in X-Gene */ >> + ahbc_base = devm_ioremap(ctx->dev, 0x1F2A0000, 0x80000); >> + if (!ahbc_base) { >> + dev_err(ctx->dev, "can't map AHBC resource\n"); >> + return -ENODEV; >> + } > > I had an important comment about the misuse of hardcoded I/O addresses > here. Please never just post a new version with the same issue. In general, > your options are: > > * rewrite the code in a more appropriate way > * reply to the comment, arguing why you think your code is correct > * ask for clarification if you don't know how to improve the code > * mark the code as TODO and mention in the patch description why you > are still working on this and that the current version should not > be seen as final. > > If you don't do any of these, reviewers will get annoyed because you > are wasting their time. [Loc Ho] Sorry about this one. I will add this to the DTS. If this resource isn't available, then it will be nop. >> + >> +static int xgene_ahci_get_u32_param(struct platform_device *pdev, >> + const char *of_name, char *acpi_name, >> + u32 *param) >> +{ >> +#ifdef CONFIG_ACPI >> + if (efi_enabled(EFI_BOOT)) { >> + unsigned long long value; >> + acpi_status status; >> + if (acpi_name == NULL) >> + return -ENODEV; >> + status = acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev), >> + acpi_name, NULL, &value); >> + if (ACPI_FAILURE(status)) >> + return -ENODEV; >> + *param = value; >> + return 0; >> + } >> +#endif > > There are more previous comments that you have not addressed yet. [Loc Ho] Original, I didn't want to get rip of this as ACPI isn't available in the 3.12 kernel yet. I will get rip of this as I don't need the ID any more. Also, I will also get rip of the clock interface. Don't need this as it is handled directly by the driver. This will address for DT as well as ACPI. > >> diff --git a/drivers/ata/sata_xgene.h b/drivers/ata/sata_xgene.h >> new file mode 100644 >> index 0000000..51e73f6 >> --- /dev/null >> +++ b/drivers/ata/sata_xgene.h > > This file should probably just be folded into the driver, since it contains no > interfaces between modules, just internal definitions. [Loc Ho] I can get rip this header file now. But in the future I may need to add this back as I intended this driver to also support running from our co-processor (ARMv7 32-bit). For that, I will need anothe file and other function that will need knowledge of the context structure and etc. -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 Kishon, >>> + >>> +/* Temporary function until the PHY parameter API */ >>> +extern void xgene_ahci_phy_set_pq(struct phy *phy, int chan, int data); >>> +extern void xgene_ahci_phy_set_spd(struct phy *phy, int chan, int gen); >> >> Ah, that should have been mentioned in the changeset text. Do you think you >> can remove these for the final version? > [Loc Ho] > Yes... Krishon, can you provide an patch that allows me to accommodate > this? Or should I come up one? I will trying to get rip of the first > one. But I will need the function to set the PHY speed - for > Gen2/Gen1. Let's plan on add these function: > > a. int (*set_speed)(int gen_speed) where gen_speed is either AUTO, > GEN1, GEN2, or GEN3. > b. Change the PHY init function to take an optional parameter. This > would help my first function in case I can get rip of it. > [Loc Ho] I will provide this patch as part of my next version to include an function as follow: int set_speed(struct *phy, int lane, u64 speed); where lane indicates the lane of the PHY and speed is the PHY speed in hertz. -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 Saturday 16 November 2013, Loc Ho wrote: > > > > config SATA_XGENE > > tristate "APM X-Gene 6.0Gbps SATA host controller support" > > depends on ARM64 || COMPILE_TEST > > ... > > > [Loc Ho] > Okay. How about this? > > depends on ARM64 || COMPILE_TEST > select SATA_XGENE_PHY && SATA_AHCI_PLATFORM I think you already found the mistake withis and corrected this in v4. > > The interface you are looking for is pr_debug() or dev_dbg(), which get > > built-in only if the DEBUG macro is set. > > > > In a lot of cases, it's actually best to remove debug output like this from > > the driver once you have it working -- whoever is debugging problems in > > the driver next might need completely different debugging information or > > can add them back easily if needed. > > > > It's your choice if you want to use pr_debug() or remove that output > > entirely. If you remove it, best remove the helper functions entirely > > and use readl/writel directly. > [Loc Ho] > I would like to keep this function for now until this driver actual > being used in production. I will use pr_debug. Ok. > >> diff --git a/drivers/ata/sata_xgene.h b/drivers/ata/sata_xgene.h > >> new file mode 100644 > >> index 0000000..51e73f6 > >> --- /dev/null > >> +++ b/drivers/ata/sata_xgene.h > > > > This file should probably just be folded into the driver, since it contains no > > interfaces between modules, just internal definitions. > [Loc Ho] > I can get rip this header file now. But in the future I may need to > add this back as I intended this driver to also support running from > our co-processor (ARMv7 32-bit). For that, I will need anothe file and > other function that will need knowledge of the context structure and > etc. Hmm, is the code for that coprocessor compiled from the kernel source? If not, it shouldn't really make a difference as it won't live in a shared code repository. 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, >> >> diff --git a/drivers/ata/sata_xgene.h b/drivers/ata/sata_xgene.h >> >> new file mode 100644 >> >> index 0000000..51e73f6 >> >> --- /dev/null >> >> +++ b/drivers/ata/sata_xgene.h >> > >> > This file should probably just be folded into the driver, since it contains no >> > interfaces between modules, just internal definitions. >> [Loc Ho] >> I can get rip this header file now. But in the future I may need to >> add this back as I intended this driver to also support running from >> our co-processor (ARMv7 32-bit). For that, I will need anothe file and >> other function that will need knowledge of the context structure and >> etc. > > Hmm, is the code for that coprocessor compiled from the kernel source? > If not, it shouldn't really make a difference as it won't live in a shared > code repository. > [Loc Ho] The co-processors are just ARM based core and the same kernel can be build to support it. Let's not worry about this yet. -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 02:52 AM, Loc Ho wrote: > Hi Kishon, > >>>> + >>>> +/* Temporary function until the PHY parameter API */ >>>> +extern void xgene_ahci_phy_set_pq(struct phy *phy, int chan, int data); >>>> +extern void xgene_ahci_phy_set_spd(struct phy *phy, int chan, int gen); >>> >>> Ah, that should have been mentioned in the changeset text. Do you think you >>> can remove these for the final version? >> [Loc Ho] >> Yes... Krishon, can you provide an patch that allows me to accommodate >> this? Or should I come up one? I will trying to get rip of the first >> one. But I will need the function to set the PHY speed - for >> Gen2/Gen1. Let's plan on add these function: >> >> a. int (*set_speed)(int gen_speed) where gen_speed is either AUTO, >> GEN1, GEN2, or GEN3. >> b. Change the PHY init function to take an optional parameter. This >> would help my first function in case I can get rip of it. >> > [Loc Ho] > I will provide this patch as part of my next version to include an > function as follow: sure.. > > int set_speed(struct *phy, int lane, u64 speed); it should be phy_set_speed. > > where lane indicates the lane of the PHY and speed is the PHY speed in hertz. Does lane here means the number of lanes? Is the lane also obtained after the training sequence? Thanks Kishon -- 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, >> int set_speed(struct *phy, int lane, u64 speed); > > it should be phy_set_speed. [Loc Ho] But your other functions don't repeat the prefix "phy"? >> >> where lane indicates the lane of the PHY and speed is the PHY speed in hertz. > > Does lane here means the number of lanes? Is the lane also obtained after the > training sequence? [Loc Ho] Lane here is the lane with you are changing the speed. If there are two lanes, then 0 parameter would change lane 0. 1 would change lane 1. -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 Tuesday 26 November 2013 10:11 PM, Loc Ho wrote: > Hi, > >>> int set_speed(struct *phy, int lane, u64 speed); >> >> it should be phy_set_speed. > [Loc Ho] > But your other functions don't repeat the prefix "phy"? refer include/linux/phy/phy.h.. int phy_init(struct phy *phy); int phy_exit(struct phy *phy); int phy_power_on(struct phy *phy); int phy_power_off(struct phy *phy); ?? > >>> >>> where lane indicates the lane of the PHY and speed is the PHY speed in hertz. >> >> Does lane here means the number of lanes? Is the lane also obtained after the >> training sequence? > [Loc Ho] > Lane here is the lane with you are changing the speed. If there are > two lanes, then 0 parameter would change lane 0. 1 would change lane > 1. can different lanes operate at different speed at the same time? Thanks Kishon -- 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, >>>> int set_speed(struct *phy, int lane, u64 speed); >>> >>> it should be phy_set_speed. >> [Loc Ho] >> But your other functions don't repeat the prefix "phy"? > > refer include/linux/phy/phy.h.. > > int phy_init(struct phy *phy); > int phy_exit(struct phy *phy); > int phy_power_on(struct phy *phy); > int phy_power_off(struct phy *phy); [Loc Ho] For the top level function, it is phy_set_speed. For the operation function pointer, it is set_speed. >>>> >>>> where lane indicates the lane of the PHY and speed is the PHY speed in hertz. >>> >>> Does lane here means the number of lanes? Is the lane also obtained after the >>> training sequence? >> [Loc Ho] >> Lane here is the lane with you are changing the speed. If there are >> two lanes, then 0 parameter would change lane 0. 1 would change lane >> 1. > > can different lanes operate at different speed at the same time? > [Loc Ho] Yes. Each lane is suppose to be independent of each other. -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 index d18db67..0d16bfe 100644 --- a/Documentation/devicetree/bindings/ata/apm-xgene.txt +++ b/Documentation/devicetree/bindings/ata/apm-xgene.txt @@ -1,4 +1,4 @@ -* APM X-Gene 6.0 Gb/s SATA PHY nodes +* APM X-Gene 6.0 Gb/s SATA PHY and controller nodes SATA PHY nodes are defined to describe on-chip Serial ATA PHY. Each SATA PHY (pair of PHY) has its own node. @@ -67,3 +67,63 @@ Example: status = "ok"; }; +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" +- reg : First memory resource shall be the AHCI memory + resource. + Second memory resource shall be the host controller + memory resource. +- id : Controller ID (0 = first, 1 = second, 2 = third) +- interrupt-parent : Interrupt controller +- interrupts : Interrupt mapping for SATA host controller IRQ +- clocks : Reference to the clock entry +- phys : PHY reference +- phy-names : Name of the PHY reference + +Optional properties: +- status : Shall be "ok" if enabled or "na" if disabled. + Default is "ok". + +Example: + sata0: sata@1a000000 { + compatible = "apm,xgene-ahci"; + id = <0>; + reg = <0x0 0x1a000000 0x0 0x100000 + 0x0 0x1f210000 0x0 0x10000>; + interrupt-parent = <&gic>; + interrupts = <0x0 0x86 0x4>; + clocks = <ð01clk 0>; + status = "na"; + phys = <&sataphy0>; + phy-names = "sataphy0"; + }; + + sata1: sata@1a400000 { + compatible = "apm,xgene-ahci"; + id = <1>; + reg = <0x0 0x1a400000 0x0 0x100000 + 0x0 0x1f220000 0x0 0x10000>; + interrupt-parent = <&gic>; + interrupts = <0x0 0x87 0x4>; + clocks = <ð23clk 0>; + status = "ok"; + phys = <&sataphy1>; + phy-names = "sataphy1"; + }; + + sata2: sata@1a800000 { + compatible = "apm,xgene-ahci"; + id = <2>; + reg = <0x0 0x1a800000 0x0 0x100000 + 0x0 0x1f230000 0x0 0x10000 + 0x0 0x1f2d0000 0x0 0x10000 >; + interrupt-parent = <&gic>; + interrupts = <0x0 0x88 0x4>; + clocks = <&sata45clk 0>; + status = "ok"; + phys = <&sataphy2>; + phy-names = "sataphy2"; + };