| Message ID | 1518456406-21564-1-git-send-email-pchandru@nvidia.com |
|---|---|
| Headers | show |
| Series | Refactor and add AHCI support for tegra210 | expand |
I think this now looks good enough to merge - others please take a look as well. Series, Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com> On 12.02.2018 19:26, Preetham Chandru Ramchandra wrote: > From: Preetham Ramchandra <pchandru@nvidia.com> > > 1. ADD AHCI support for tegra210 > 2. Extend the tegra AHCI controller device tree binding with tegra210 > 3. Update initialization sequence > 4. Initialize regulators based on chip > 5. Disable DevSlp > 6. Disable DIPM > --- > Preetham Ramchandra (7): > dt-bindings: ahci-tegra: add binding documentation > arm64: tegra: Enable AHCI on Jetson TX1 > ata: ahci_tegra: Update initialization sequence > ata: ahci_tegra: initialize regulators from soc_data > ata: ahci_tegra: disable devslp for t124 > ata: ahci_tegra: disable DIPM > ata: ahci_tegra: Add AHCI support for Tegra210 > > .../bindings/ata/nvidia,tegra124-ahci.txt | 35 +- > arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi | 6 + > arch/arm64/boot/dts/nvidia/tegra210.dtsi | 16 + > drivers/ata/ahci_tegra.c | 362 ++++++++++++++++----- > 4 files changed, 329 insertions(+), 90 deletions(-) > -- 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 Mon, Feb 12, 2018 at 10:56:41PM +0530, Preetham Chandru Ramchandra wrote: > From: Preetham Ramchandra <pchandru@nvidia.com> > > Add sata node to the Tegra210 device tree, and > enable the device and assign board-specific > properties on Jetson TX1. > Signed-off-by: Preetham Chandru R <pchandru@nvidia.com> Tags such as Signed-off-by: should be in a separate paragraph, so the above is missing a blank line before the Signed-off-by:. > --- > v7: > * Change commit message to reflect accordingly > v4: > * Fixed missing space after 'AUX' > --- > arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi | 6 ++++++ > arch/arm64/boot/dts/nvidia/tegra210.dtsi | 16 ++++++++++++++++ > 2 files changed, 22 insertions(+) I prefer these to be two separate patches, one for the SoC .dtsi and another for the P2597 carrier. > diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi b/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi > index d67ef4319f3b..2aa2979cd6b5 100644 > --- a/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi > +++ b/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi > @@ -1325,6 +1325,12 @@ > status = "okay"; > }; > > + sata@70020000 { > + status = "okay"; > + phys = <&{/padctl@7009f000/pads/sata/lanes/sata-0}>; > + phy-names = "sata-0"; > + }; > + > padctl@7009f000 { > status = "okay"; > > diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi b/arch/arm64/boot/dts/nvidia/tegra210.dtsi > index 9c2402108772..bf72db5386a5 100644 > --- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi > +++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi > @@ -798,6 +798,22 @@ > #iommu-cells = <1>; > }; > > + sata@70020000 { > + compatible = "nvidia,tegra210-ahci"; > + reg = <0x0 0x70027000 0x0 0x2000>, /* AHCI */ > + <0x0 0x70020000 0x0 0x7000>, /* SATA */ > + <0x0 0x70001100 0x0 0x1000>; /* SATA AUX */ > + interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&tegra_car TEGRA210_CLK_SATA>, > + <&tegra_car TEGRA210_CLK_SATA_OOB>; Nit: these are not properly aligned. Thierry
On Mon, Feb 12, 2018 at 10:56:42PM +0530, Preetham Chandru Ramchandra wrote: > From: Preetham Ramchandra <pchandru@nvidia.com> > > Update the controller initialization sequence and move > t124 specifics to tegra124_ahci_init. > > Signed-off-by: Preetham Chandru R <pchandru@nvidia.com> > --- > v7: > * moveed tegra124_ahci_soc_data definition to be > just above the of_device_id table. > --- > drivers/ata/ahci_tegra.c | 288 ++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 223 insertions(+), 65 deletions(-) > > diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c > index 3a62eb246d80..7ffe8a97447a 100644 > --- a/drivers/ata/ahci_tegra.c > +++ b/drivers/ata/ahci_tegra.c [...] > @@ -99,6 +159,14 @@ static const struct sata_pad_calibration tegra124_pad_calibration[] = { > {0x14, 0x0e, 0x1a, 0x0e}, > }; > > +struct tegra_ahci_ops { > + int (*init)(struct ahci_host_priv *hpriv); > +}; > + > +struct tegra_ahci_soc { > + struct tegra_ahci_ops ops; > +}; > + > struct tegra_ahci_priv { > struct platform_device *pdev; > void __iomem *sata_regs; > @@ -108,8 +176,53 @@ struct tegra_ahci_priv { > /* Needs special handling, cannot use ahci_platform */ > struct clk *sata_clk; > struct regulator_bulk_data supplies[5]; > + struct tegra_ahci_soc *soc_data; This should be const. I also think the _data suffix is unnecessary. > @@ -145,7 +258,6 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv) > > disable_regulators: > regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies); > - > return ret; > } Unneeded whitespace change. > @@ -179,78 +290,114 @@ static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv) > return ret; > } > > + /* > + * Program the following SATA IPFS registers > + * to allow SW accesses to SATA's MMIO register range. > + */ You can also use the full width of a line for comments. No need to wrap at arbitrary points. > @@ -285,8 +432,17 @@ static const struct ata_port_info ahci_tegra_port_info = { > .port_ops = &ahci_tegra_port_ops, > }; > > +static const struct tegra_ahci_soc tegra124_ahci_soc_data = { > + .ops = { > + .init = tegra124_ahci_init, > + }, > +}; > + Again, I don't think there's a need for _data. Also, you've made this const, so tegra->soc(_data) should also be const. One rule of thumb is that if you have const in one place, then you need to make it const everywhere. > static const struct of_device_id tegra_ahci_of_match[] = { > - { .compatible = "nvidia,tegra124-ahci" }, > + { > + .compatible = "nvidia,tegra124-ahci", > + .data = &tegra124_ahci_soc_data > + }, > {} > }; of_device_id.data is const as well, which is why this compiles properly. > MODULE_DEVICE_TABLE(of, tegra_ahci_of_match); > @@ -313,6 +469,8 @@ static int tegra_ahci_probe(struct platform_device *pdev) > hpriv->plat_data = tegra; > > tegra->pdev = pdev; > + tegra->soc_data = > + (struct tegra_ahci_soc *)of_device_get_match_data(&pdev->dev); And if you make tegra->soc(_data) const, then there's no need for this cast, you can simply assign directly: tegra->soc = of_device_get_match_data(&pdev->dev); void * will be automatically cast to any other pointer. The only reason why you need the cast above is to avoid the compiler from warning about the const qualifier being cast away. Once const, always const. Thierry
On Mon, Feb 12, 2018 at 10:56:43PM +0530, Preetham Chandru Ramchandra wrote: > From: Preetham Ramchandra <pchandru@nvidia.com> > > Get the regulator names to be initialized from soc_data > and initialize them. > > Signed-off-by: Preetham Chandru R <pchandru@nvidia.com> > --- > v7: > * moved tegra124_supply_names definition to just above > the tegra124_ahci_soc_data structure. > --- > drivers/ata/ahci_tegra.c | 33 +++++++++++++++++++++++---------- > 1 file changed, 23 insertions(+), 10 deletions(-) Acked-by: Thierry Reding <treding@nvidia.com>
On Mon, Feb 12, 2018 at 10:56:44PM +0530, Preetham Chandru Ramchandra wrote: > From: Preetham Ramchandra <pchandru@nvidia.com> > > t124 does not support devslp and it should be > disabled. Please spell out t124 as Tegra124 in the subject and the commit message. > > Signed-off-by: Preetham Chandru R <pchandru@nvidia.com> > --- > drivers/ata/ahci_tegra.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c > index 6aaf8a4571c8..62f2afb54789 100644 > --- a/drivers/ata/ahci_tegra.c > +++ b/drivers/ata/ahci_tegra.c > @@ -145,6 +145,10 @@ > #define FUSE_SATA_CALIB 0x124 > #define FUSE_SATA_CALIB_MASK 0x3 > > +enum { > + NO_DEVSLP = (1 << 0), > +}; > + > struct sata_pad_calibration { > u8 gen1_tx_amp; > u8 gen1_tx_peak; > @@ -166,12 +170,14 @@ struct tegra_ahci_ops { > struct tegra_ahci_soc { > const char *const *supply_names; > u32 num_supplies; > + u32 quirks; > struct tegra_ahci_ops ops; > }; I'd prefer these to be simple booleans, which make the code easier to read, in my opinion. This could be: bool supports_devslp; > > struct tegra_ahci_priv { > struct platform_device *pdev; > void __iomem *sata_regs; > + void __iomem *sata_aux_regs; > struct reset_control *sata_rst; > struct reset_control *sata_oob_rst; > struct reset_control *sata_cold_rst; > @@ -181,6 +187,18 @@ struct tegra_ahci_priv { > struct tegra_ahci_soc *soc_data; > }; > > +static void tegra_ahci_handle_quirks(struct ahci_host_priv *hpriv) > +{ > + struct tegra_ahci_priv *tegra = hpriv->plat_data; > + u32 val; > + > + if (tegra->sata_aux_regs && (tegra->soc_data->quirks & NO_DEVSLP)) { And then this becomes: if (tegra->sata_aux_regs && !tegra->soc->supports_devslp) > + val = readl(tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0); > + val &= ~SATA_AUX_MISC_CNTL_1_0_SDS_SUPPORT; > + writel(val, tegra->sata_aux_regs + SATA_AUX_MISC_CNTL_1_0); > + } > +} > + > static int tegra124_ahci_init(struct ahci_host_priv *hpriv) > { > struct tegra_ahci_priv *tegra = hpriv->plat_data; > @@ -400,6 +418,7 @@ static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv) > val &= ~SATA_CONFIGURATION_0_CLK_OVERRIDE; > writel(val, tegra->sata_regs + SATA_CONFIGURATION_0); > > + tegra_ahci_handle_quirks(hpriv); > > /* Unmask SATA interrupts */ > > @@ -441,6 +460,7 @@ static const char *const tegra124_supply_names[] = { > static const struct tegra_ahci_soc tegra124_ahci_soc_data = { > .supply_names = tegra124_supply_names, > .num_supplies = ARRAY_SIZE(tegra124_supply_names), > + .quirks = NO_DEVSLP, > .ops = { > .init = tegra124_ahci_init, > }, > @@ -485,6 +505,15 @@ static int tegra_ahci_probe(struct platform_device *pdev) > tegra->sata_regs = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(tegra->sata_regs)) > return PTR_ERR(tegra->sata_regs); > + res = platform_get_resource(pdev, IORESOURCE_MEM, 2); > + /* > + * Aux register is optional. > + */ "Aux register" -> "AUX registers". Also, I think it's more idiomatic to place the comment above platform_get_resource() so that the assignment and the check below are not separated. Thierry
On Mon, Feb 12, 2018 at 10:56:45PM +0530, Preetham Chandru Ramchandra wrote: > From: Preetham Ramchandra <pchandru@nvidia.com> > > tegra does not support DIPM and it should be disabled "Tegra" and finish with a '.'. Other than that: Acked-by: Thierry Reding <treding@nvidia.com>
On Mon, Feb 12, 2018 at 10:56:46PM +0530, Preetham Chandru Ramchandra wrote: > From: Preetham Ramchandra <pchandru@nvidia.com> > > Add support for the AHCI-compliant Serial ATA host > controller on the Tegra210 system-on-chip. > > Signed-off-by: Preetham Chandru R <pchandru@nvidia.com> > --- > v7: > * Added commit message > --- > drivers/ata/ahci_tegra.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c > index ffad5c61e991..4524755b9b23 100644 > --- a/drivers/ata/ahci_tegra.c > +++ b/drivers/ata/ahci_tegra.c > @@ -466,11 +466,19 @@ static const struct tegra_ahci_soc tegra124_ahci_soc_data = { > }, > }; > > +static const struct tegra_ahci_soc tegra210_ahci_soc_data = { > + .quirks = NO_DEVSLP, > +}; > + > static const struct of_device_id tegra_ahci_of_match[] = { > { > .compatible = "nvidia,tegra124-ahci", > .data = &tegra124_ahci_soc_data > }, > + { > + .compatible = "nvidia,tegra210-ahci", > + .data = &tegra210_ahci_soc_data > + }, > {} > }; > MODULE_DEVICE_TABLE(of, tegra_ahci_of_match); > @@ -585,5 +593,5 @@ static struct platform_driver tegra_ahci_driver = { > module_platform_driver(tegra_ahci_driver); > > MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>"); > -MODULE_DESCRIPTION("Tegra124 AHCI SATA driver"); > +MODULE_DESCRIPTION("Tegra AHCI SATA driver"); > MODULE_LICENSE("GPL v2"); I think you should also update the Kconfig file to clarify that this is no longer Tegra124-only. With that: Acked-by: Thierry Reding <treding@nvidia.com>
On Mon, Feb 12, 2018 at 10:56:42PM +0530, Preetham Chandru Ramchandra wrote: > From: Preetham Ramchandra <pchandru@nvidia.com> > > Update the controller initialization sequence and move > t124 specifics to tegra124_ahci_init. > > Signed-off-by: Preetham Chandru R <pchandru@nvidia.com> > --- > v7: > * moveed tegra124_ahci_soc_data definition to be > just above the of_device_id table. > --- > drivers/ata/ahci_tegra.c | 288 ++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 223 insertions(+), 65 deletions(-) > > diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c > index 3a62eb246d80..7ffe8a97447a 100644 > --- a/drivers/ata/ahci_tegra.c > +++ b/drivers/ata/ahci_tegra.c [...] > @@ -99,6 +159,14 @@ static const struct sata_pad_calibration tegra124_pad_calibration[] = { > {0x14, 0x0e, 0x1a, 0x0e}, > }; > > +struct tegra_ahci_ops { > + int (*init)(struct ahci_host_priv *hpriv); > +}; > + > +struct tegra_ahci_soc { > + struct tegra_ahci_ops ops; > +}; Just noticed this: it's slightly more customary to make ops a pointer to a function table, like so: struct tegra_ahci_soc { const struct tegra_ahci_ops *ops; }; and then have: static const struct tegra_ahci_ops tegra124_ahci_ops = { .init = tegra124_ahci_init, }; static const struct tegra_ahci_soc tegra124_ahci_soc = { .ops = &tegra124_ahci_ops, }; That's not terribly useful in this case because the ops are not passed around or shared between multiple instances, so there are no benefits. Either way is fine here, I guess. Thierry
From: Preetham Ramchandra <pchandru@nvidia.com> 1. ADD AHCI support for tegra210 2. Extend the tegra AHCI controller device tree binding with tegra210 3. Update initialization sequence 4. Initialize regulators based on chip 5. Disable DevSlp 6. Disable DIPM --- Preetham Ramchandra (7): dt-bindings: ahci-tegra: add binding documentation arm64: tegra: Enable AHCI on Jetson TX1 ata: ahci_tegra: Update initialization sequence ata: ahci_tegra: initialize regulators from soc_data ata: ahci_tegra: disable devslp for t124 ata: ahci_tegra: disable DIPM ata: ahci_tegra: Add AHCI support for Tegra210 .../bindings/ata/nvidia,tegra124-ahci.txt | 35 +- arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi | 6 + arch/arm64/boot/dts/nvidia/tegra210.dtsi | 16 + drivers/ata/ahci_tegra.c | 362 ++++++++++++++++----- 4 files changed, 329 insertions(+), 90 deletions(-)