Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On 06/17/2014 11:36 AM, Mikko Perttunen wrote:
> On 06/17/2014 08:04 PM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi,
>>
>> On Tuesday, June 17, 2014 10:10:23 AM Stephen Warren wrote:
>>> On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> [...]
>>
> +static struct platform_driver tegra_ahci_driver = {
> +.probe = tegra_ahci_probe,
> +.remove = ata_platform_remove_one,
> +.driver = {
> +.name = "tegra-ahci",
> +.owner = THIS_MODULE,
> +.of_match_table = tegra_ahci_of_match,
> +},
This driver lacks PM suspend/resume support. I assume that
the Tegra124 platform also doesn't support suspend/resume yet
(if so a comment in the driver code about this would be useful),
otherwise the driver should be fixed.
>>>
>>> We do have basic system suspend/resume support. However, I don't think
>>> missing suspend/resume in an individual driver should stop it from being
>>> merged. It can certainly be added later.
>>
>> There should be at least FIXME in the driver explaining the situation and
>> I would really prefer to have PM support added when the driver is still
>> "hot" (meaning there are people actively working on it) instead of
>> possibly
>> having to chase people months/years later when they have already moved on
>> and are working on something else. Please also note that adding PM
>> support
>> should be quite simple if the driver is designed correctly.
>
> AFAIK, the deepest level of suspend currently supported on upstream is
> LP1, for which the driver doesn't need to do anything. Only when we go
> to LP0 the driver will need to save/reload registers and stuff.
Yes, it's generally true that no SoC register state is lost during
suspend, only CPU state.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On 06/17/2014 08:04 PM, Bartlomiej Zolnierkiewicz wrote:
Hi,
On Tuesday, June 17, 2014 10:10:23 AM Stephen Warren wrote:
On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
[...]
+static struct platform_driver tegra_ahci_driver = {
+ .probe = tegra_ahci_probe,
+ .remove = ata_platform_remove_one,
+ .driver = {
+ .name = "tegra-ahci",
+ .owner = THIS_MODULE,
+ .of_match_table = tegra_ahci_of_match,
+ },
This driver lacks PM suspend/resume support. I assume that
the Tegra124 platform also doesn't support suspend/resume yet
(if so a comment in the driver code about this would be useful),
otherwise the driver should be fixed.
We do have basic system suspend/resume support. However, I don't think
missing suspend/resume in an individual driver should stop it from being
merged. It can certainly be added later.
There should be at least FIXME in the driver explaining the situation and
I would really prefer to have PM support added when the driver is still
"hot" (meaning there are people actively working on it) instead of possibly
having to chase people months/years later when they have already moved on
and are working on something else. Please also note that adding PM support
should be quite simple if the driver is designed correctly.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics
AFAIK, the deepest level of suspend currently supported on upstream is
LP1, for which the driver doesn't need to do anything. Only when we go
to LP0 the driver will need to save/reload registers and stuff.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
Hi,
On Tuesday, June 17, 2014 10:10:23 AM Stephen Warren wrote:
> On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
[...]
> >> +static struct platform_driver tegra_ahci_driver = {
> >> + .probe = tegra_ahci_probe,
> >> + .remove = ata_platform_remove_one,
> >> + .driver = {
> >> + .name = "tegra-ahci",
> >> + .owner = THIS_MODULE,
> >> + .of_match_table = tegra_ahci_of_match,
> >> + },
> >
> > This driver lacks PM suspend/resume support. I assume that
> > the Tegra124 platform also doesn't support suspend/resume yet
> > (if so a comment in the driver code about this would be useful),
> > otherwise the driver should be fixed.
>
> We do have basic system suspend/resume support. However, I don't think
> missing suspend/resume in an individual driver should stop it from being
> merged. It can certainly be added later.
There should be at least FIXME in the driver explaining the situation and
I would really prefer to have PM support added when the driver is still
"hot" (meaning there are people actively working on it) instead of possibly
having to chase people months/years later when they have already moved on
and are working on something else. Please also note that adding PM support
should be quite simple if the driver is designed correctly.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On 06/17/2014 10:14 AM, Tejun Heo wrote: > On Tue, Jun 17, 2014 at 12:13:20PM -0400, Tejun Heo wrote: >> On Tue, Jun 17, 2014 at 10:10:23AM -0600, Stephen Warren wrote: sata_writel() and sata_read() static inlines don't seem to add any value. Can they be removed? >>> >>> Such functions are quite idiomatic in drivers, and serve to simplify all >>> the call-sites by removing the need to write out the addition of the >>> base address everywhere. >> >> I think it obfuscates more than helps. If you absoluately have to >> keep it, please at least name it so that it's clear that it's >> something specific to this driver. > > Please also drop inline. It isn't a difficult optimization to perform > for the compiler. There are probably hundreds of drivers marking those functions inline. If they aren't marked inline, it'll just stick out as unusual when people read them all, and then they'll send patches to fix it. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On Tue, Jun 17, 2014 at 10:23:01AM -0600, Stephen Warren wrote: > > Please also drop inline. It isn't a difficult optimization to perform > > for the compiler. > > There are probably hundreds of drivers marking those functions inline. We used to need them. We no longer do. We're in a very long transition phase. > If they aren't marked inline, it'll just stick out as unusual when > people read them all, and then they'll send patches to fix it. Don't worry. They won't be applied. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On Tue, Jun 17, 2014 at 12:13:20PM -0400, Tejun Heo wrote: > On Tue, Jun 17, 2014 at 10:10:23AM -0600, Stephen Warren wrote: > > > sata_writel() and sata_read() static inlines don't seem to add any value. > > > > > > Can they be removed? > > > > Such functions are quite idiomatic in drivers, and serve to simplify all > > the call-sites by removing the need to write out the addition of the > > base address everywhere. > > I think it obfuscates more than helps. If you absoluately have to > keep it, please at least name it so that it's clear that it's > something specific to this driver. Please also drop inline. It isn't a difficult optimization to perform for the compiler. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On Tue, Jun 17, 2014 at 10:10:23AM -0600, Stephen Warren wrote: > > sata_writel() and sata_read() static inlines don't seem to add any value. > > > > Can they be removed? > > Such functions are quite idiomatic in drivers, and serve to simplify all > the call-sites by removing the need to write out the addition of the > base address everywhere. I think it obfuscates more than helps. If you absoluately have to keep it, please at least name it so that it's clear that it's something specific to this driver. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
>
> Hi,
>
> On Wednesday, June 04, 2014 02:32:38 PM Mikko Perttunen wrote:
>> This adds support for the integrated AHCI-compliant Serial ATA
>> controller present on the NVIDIA Tegra124 system-on-chip.
>> +static inline void sata_writel(struct tegra_ahci_priv *tegra, u32 value,
>> + unsigned long offset)
>> +{
>> +writel(value, tegra->sata_regs + offset);
>> +}
>> +
>> +static inline u32 sata_readl(struct tegra_ahci_priv *tegra,
>> +unsigned long offset)
>> +{
>> +return readl(tegra->sata_regs + offset);
>> +}
>
> sata_writel() and sata_read() static inlines don't seem to add any value.
>
> Can they be removed?
Such functions are quite idiomatic in drivers, and serve to simplify all
the call-sites by removing the need to write out the addition of the
base address everywhere.
>> +static struct platform_driver tegra_ahci_driver = {
>> +.probe = tegra_ahci_probe,
>> +.remove = ata_platform_remove_one,
>> +.driver = {
>> +.name = "tegra-ahci",
>> +.owner = THIS_MODULE,
>> +.of_match_table = tegra_ahci_of_match,
>> +},
>
> This driver lacks PM suspend/resume support. I assume that
> the Tegra124 platform also doesn't support suspend/resume yet
> (if so a comment in the driver code about this would be useful),
> otherwise the driver should be fixed.
We do have basic system suspend/resume support. However, I don't think
missing suspend/resume in an individual driver should stop it from being
merged. It can certainly be added later.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
Hi, On Wednesday, June 04, 2014 02:32:38 PM Mikko Perttunen wrote: > This adds support for the integrated AHCI-compliant Serial ATA > controller present on the NVIDIA Tegra124 system-on-chip. > > Signed-off-by: Mikko Perttunen > --- > drivers/ata/Kconfig | 9 ++ > drivers/ata/Makefile | 1 + > drivers/ata/ahci_tegra.c | 386 > +++ > 3 files changed, 396 insertions(+) > create mode 100644 drivers/ata/ahci_tegra.c > > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig > index 7671dba..e65d400 100644 > --- a/drivers/ata/Kconfig > +++ b/drivers/ata/Kconfig > @@ -141,6 +141,15 @@ config AHCI_SUNXI > > If unsure, say N. > > +config AHCI_TEGRA > + tristate "NVIDIA Tegra124 AHCI SATA support" > + depends on ARCH_TEGRA > + help > + This option enables support for the NVIDIA Tegra124 SoC's > + onboard AHCI SATA. > + > + If unsure, say N. > + > config AHCI_XGENE > tristate "APM X-Gene 6.0Gbps AHCI SATA host controller support" > depends on PHY_XGENE > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile > index 5a02aee..ae41107 100644 > --- a/drivers/ata/Makefile > +++ b/drivers/ata/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_AHCI_IMX) += ahci_imx.o libahci.o > libahci_platform.o > obj-$(CONFIG_AHCI_MVEBU) += ahci_mvebu.o libahci.o libahci_platform.o > obj-$(CONFIG_AHCI_SUNXI) += ahci_sunxi.o libahci.o libahci_platform.o > obj-$(CONFIG_AHCI_ST)+= ahci_st.o libahci.o > libahci_platform.o > +obj-$(CONFIG_AHCI_TEGRA) += ahci_tegra.o libahci.o libahci_platform.o > obj-$(CONFIG_AHCI_XGENE) += ahci_xgene.o libahci.o libahci_platform.o > > # SFF w/ custom DMA > diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c > new file mode 100644 > index 000..a10aac0 > --- /dev/null > +++ b/drivers/ata/ahci_tegra.c > @@ -0,0 +1,386 @@ > +/* > + * drivers/ata/ahci_tegra.c > + * > + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. > + * > + * Author: > + * Mikko Perttunen > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "ahci.h" > + > +#define SATA_CONFIGURATION_0 0x180 > +#define SATA_CONFIGURATION_EN_FPCI (1<<0) minor nitpick: BIT() macro can be used here > +#define SCFG_OFFSET 0x1000 > + > +#define T_SATA0_CFG_10x04 > +#define T_SATA0_CFG_1_IO_SPACE (1<<0) > +#define T_SATA0_CFG_1_MEMORY_SPACE (1<<1) > +#define T_SATA0_CFG_1_BUS_MASTER(1<<2) > +#define T_SATA0_CFG_1_SERR (1<<8) ditto > +#define T_SATA0_CFG_90x24 > + > +#define SATA_FPCI_BAR5 0x94 > + > +#define SATA_INTR_MASK 0x188 > +#define SATA_INTR_MASK_IP_INT_MASK (1<<16) ditto > +#define T_SATA0_AHCI_HBA_CAP_BKDR0x300 > + > +#define T_SATA0_BKDOOR_CC0x4a4 > + > +#define T_SATA0_CFG_SATA 0x54c > +#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN(1<<12) ditto > +#define T_SATA0_CFG_MISC 0x550 > + > +#define T_SATA0_INDEX0x680 > + > +#define T_SATA0_CHX_PHY_CTRL1_GEN1 0x690 > +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK 0xff > +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT 0 > +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK (0xff<<8) > +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT 8 > + > +#define T_SATA0_CHX_PHY_CTRL1_GEN2 0x694 > +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK 0xff > +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_SHIFT 0 > +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK (0xff<<12) > +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_SHIFT 12 > + > +#define T_SATA0_CHX_PHY_CTRL20x69c > +#define T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1 0x23 > + > +#define T_SATA0_CHX_PHY_CTRL11 0x6d0 > +#define
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
Hi, On Wednesday, June 04, 2014 02:32:38 PM Mikko Perttunen wrote: This adds support for the integrated AHCI-compliant Serial ATA controller present on the NVIDIA Tegra124 system-on-chip. Signed-off-by: Mikko Perttunen mperttu...@nvidia.com --- drivers/ata/Kconfig | 9 ++ drivers/ata/Makefile | 1 + drivers/ata/ahci_tegra.c | 386 +++ 3 files changed, 396 insertions(+) create mode 100644 drivers/ata/ahci_tegra.c diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index 7671dba..e65d400 100644 --- a/drivers/ata/Kconfig +++ b/drivers/ata/Kconfig @@ -141,6 +141,15 @@ config AHCI_SUNXI If unsure, say N. +config AHCI_TEGRA + tristate NVIDIA Tegra124 AHCI SATA support + depends on ARCH_TEGRA + help + This option enables support for the NVIDIA Tegra124 SoC's + onboard AHCI SATA. + + If unsure, say N. + config AHCI_XGENE tristate APM X-Gene 6.0Gbps AHCI SATA host controller support depends on PHY_XGENE diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile index 5a02aee..ae41107 100644 --- a/drivers/ata/Makefile +++ b/drivers/ata/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_AHCI_IMX) += ahci_imx.o libahci.o libahci_platform.o obj-$(CONFIG_AHCI_MVEBU) += ahci_mvebu.o libahci.o libahci_platform.o obj-$(CONFIG_AHCI_SUNXI) += ahci_sunxi.o libahci.o libahci_platform.o obj-$(CONFIG_AHCI_ST)+= ahci_st.o libahci.o libahci_platform.o +obj-$(CONFIG_AHCI_TEGRA) += ahci_tegra.o libahci.o libahci_platform.o obj-$(CONFIG_AHCI_XGENE) += ahci_xgene.o libahci.o libahci_platform.o # SFF w/ custom DMA diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c new file mode 100644 index 000..a10aac0 --- /dev/null +++ b/drivers/ata/ahci_tegra.c @@ -0,0 +1,386 @@ +/* + * drivers/ata/ahci_tegra.c + * + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved. + * + * Author: + * Mikko Perttunen mperttu...@nvidia.com + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include linux/ahci_platform.h +#include linux/reset.h +#include linux/errno.h +#include linux/kernel.h +#include linux/module.h +#include linux/of_device.h +#include linux/platform_device.h +#include linux/tegra-powergate.h +#include linux/regulator/consumer.h +#include linux/tegra-soc.h +#include ahci.h + +#define SATA_CONFIGURATION_0 0x180 +#define SATA_CONFIGURATION_EN_FPCI (10) minor nitpick: BIT() macro can be used here +#define SCFG_OFFSET 0x1000 + +#define T_SATA0_CFG_10x04 +#define T_SATA0_CFG_1_IO_SPACE (10) +#define T_SATA0_CFG_1_MEMORY_SPACE (11) +#define T_SATA0_CFG_1_BUS_MASTER(12) +#define T_SATA0_CFG_1_SERR (18) ditto +#define T_SATA0_CFG_90x24 + +#define SATA_FPCI_BAR5 0x94 + +#define SATA_INTR_MASK 0x188 +#define SATA_INTR_MASK_IP_INT_MASK (116) ditto +#define T_SATA0_AHCI_HBA_CAP_BKDR0x300 + +#define T_SATA0_BKDOOR_CC0x4a4 + +#define T_SATA0_CFG_SATA 0x54c +#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN(112) ditto +#define T_SATA0_CFG_MISC 0x550 + +#define T_SATA0_INDEX0x680 + +#define T_SATA0_CHX_PHY_CTRL1_GEN1 0x690 +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK 0xff +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT 0 +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK (0xff8) +#define T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT 8 + +#define T_SATA0_CHX_PHY_CTRL1_GEN2 0x694 +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK 0xff +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_SHIFT 0 +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK (0xff12) +#define T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_SHIFT 12 + +#define T_SATA0_CHX_PHY_CTRL20x69c +#define T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1 0x23 + +#define
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
Hi,
On Wednesday, June 04, 2014 02:32:38 PM Mikko Perttunen wrote:
This adds support for the integrated AHCI-compliant Serial ATA
controller present on the NVIDIA Tegra124 system-on-chip.
+static inline void sata_writel(struct tegra_ahci_priv *tegra, u32 value,
+ unsigned long offset)
+{
+writel(value, tegra-sata_regs + offset);
+}
+
+static inline u32 sata_readl(struct tegra_ahci_priv *tegra,
+unsigned long offset)
+{
+return readl(tegra-sata_regs + offset);
+}
sata_writel() and sata_read() static inlines don't seem to add any value.
Can they be removed?
Such functions are quite idiomatic in drivers, and serve to simplify all
the call-sites by removing the need to write out the addition of the
base address everywhere.
+static struct platform_driver tegra_ahci_driver = {
+.probe = tegra_ahci_probe,
+.remove = ata_platform_remove_one,
+.driver = {
+.name = tegra-ahci,
+.owner = THIS_MODULE,
+.of_match_table = tegra_ahci_of_match,
+},
This driver lacks PM suspend/resume support. I assume that
the Tegra124 platform also doesn't support suspend/resume yet
(if so a comment in the driver code about this would be useful),
otherwise the driver should be fixed.
We do have basic system suspend/resume support. However, I don't think
missing suspend/resume in an individual driver should stop it from being
merged. It can certainly be added later.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On Tue, Jun 17, 2014 at 10:10:23AM -0600, Stephen Warren wrote: sata_writel() and sata_read() static inlines don't seem to add any value. Can they be removed? Such functions are quite idiomatic in drivers, and serve to simplify all the call-sites by removing the need to write out the addition of the base address everywhere. I think it obfuscates more than helps. If you absoluately have to keep it, please at least name it so that it's clear that it's something specific to this driver. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On Tue, Jun 17, 2014 at 12:13:20PM -0400, Tejun Heo wrote: On Tue, Jun 17, 2014 at 10:10:23AM -0600, Stephen Warren wrote: sata_writel() and sata_read() static inlines don't seem to add any value. Can they be removed? Such functions are quite idiomatic in drivers, and serve to simplify all the call-sites by removing the need to write out the addition of the base address everywhere. I think it obfuscates more than helps. If you absoluately have to keep it, please at least name it so that it's clear that it's something specific to this driver. Please also drop inline. It isn't a difficult optimization to perform for the compiler. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On Tue, Jun 17, 2014 at 10:23:01AM -0600, Stephen Warren wrote: Please also drop inline. It isn't a difficult optimization to perform for the compiler. There are probably hundreds of drivers marking those functions inline. We used to need them. We no longer do. We're in a very long transition phase. If they aren't marked inline, it'll just stick out as unusual when people read them all, and then they'll send patches to fix it. Don't worry. They won't be applied. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On 06/17/2014 10:14 AM, Tejun Heo wrote: On Tue, Jun 17, 2014 at 12:13:20PM -0400, Tejun Heo wrote: On Tue, Jun 17, 2014 at 10:10:23AM -0600, Stephen Warren wrote: sata_writel() and sata_read() static inlines don't seem to add any value. Can they be removed? Such functions are quite idiomatic in drivers, and serve to simplify all the call-sites by removing the need to write out the addition of the base address everywhere. I think it obfuscates more than helps. If you absoluately have to keep it, please at least name it so that it's clear that it's something specific to this driver. Please also drop inline. It isn't a difficult optimization to perform for the compiler. There are probably hundreds of drivers marking those functions inline. If they aren't marked inline, it'll just stick out as unusual when people read them all, and then they'll send patches to fix it. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
Hi,
On Tuesday, June 17, 2014 10:10:23 AM Stephen Warren wrote:
On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
[...]
+static struct platform_driver tegra_ahci_driver = {
+ .probe = tegra_ahci_probe,
+ .remove = ata_platform_remove_one,
+ .driver = {
+ .name = tegra-ahci,
+ .owner = THIS_MODULE,
+ .of_match_table = tegra_ahci_of_match,
+ },
This driver lacks PM suspend/resume support. I assume that
the Tegra124 platform also doesn't support suspend/resume yet
(if so a comment in the driver code about this would be useful),
otherwise the driver should be fixed.
We do have basic system suspend/resume support. However, I don't think
missing suspend/resume in an individual driver should stop it from being
merged. It can certainly be added later.
There should be at least FIXME in the driver explaining the situation and
I would really prefer to have PM support added when the driver is still
hot (meaning there are people actively working on it) instead of possibly
having to chase people months/years later when they have already moved on
and are working on something else. Please also note that adding PM support
should be quite simple if the driver is designed correctly.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung RD Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On 06/17/2014 08:04 PM, Bartlomiej Zolnierkiewicz wrote:
Hi,
On Tuesday, June 17, 2014 10:10:23 AM Stephen Warren wrote:
On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
[...]
+static struct platform_driver tegra_ahci_driver = {
+ .probe = tegra_ahci_probe,
+ .remove = ata_platform_remove_one,
+ .driver = {
+ .name = tegra-ahci,
+ .owner = THIS_MODULE,
+ .of_match_table = tegra_ahci_of_match,
+ },
This driver lacks PM suspend/resume support. I assume that
the Tegra124 platform also doesn't support suspend/resume yet
(if so a comment in the driver code about this would be useful),
otherwise the driver should be fixed.
We do have basic system suspend/resume support. However, I don't think
missing suspend/resume in an individual driver should stop it from being
merged. It can certainly be added later.
There should be at least FIXME in the driver explaining the situation and
I would really prefer to have PM support added when the driver is still
hot (meaning there are people actively working on it) instead of possibly
having to chase people months/years later when they have already moved on
and are working on something else. Please also note that adding PM support
should be quite simple if the driver is designed correctly.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung RD Institute Poland
Samsung Electronics
AFAIK, the deepest level of suspend currently supported on upstream is
LP1, for which the driver doesn't need to do anything. Only when we go
to LP0 the driver will need to save/reload registers and stuff.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On 06/17/2014 11:36 AM, Mikko Perttunen wrote:
On 06/17/2014 08:04 PM, Bartlomiej Zolnierkiewicz wrote:
Hi,
On Tuesday, June 17, 2014 10:10:23 AM Stephen Warren wrote:
On 06/17/2014 06:14 AM, Bartlomiej Zolnierkiewicz wrote:
[...]
+static struct platform_driver tegra_ahci_driver = {
+.probe = tegra_ahci_probe,
+.remove = ata_platform_remove_one,
+.driver = {
+.name = tegra-ahci,
+.owner = THIS_MODULE,
+.of_match_table = tegra_ahci_of_match,
+},
This driver lacks PM suspend/resume support. I assume that
the Tegra124 platform also doesn't support suspend/resume yet
(if so a comment in the driver code about this would be useful),
otherwise the driver should be fixed.
We do have basic system suspend/resume support. However, I don't think
missing suspend/resume in an individual driver should stop it from being
merged. It can certainly be added later.
There should be at least FIXME in the driver explaining the situation and
I would really prefer to have PM support added when the driver is still
hot (meaning there are people actively working on it) instead of
possibly
having to chase people months/years later when they have already moved on
and are working on something else. Please also note that adding PM
support
should be quite simple if the driver is designed correctly.
AFAIK, the deepest level of suspend currently supported on upstream is
LP1, for which the driver doesn't need to do anything. Only when we go
to LP0 the driver will need to save/reload registers and stuff.
Yes, it's generally true that no SoC register state is lost during
suspend, only CPU state.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This adds support for the integrated AHCI-compliant Serial ATA
> controller present on the NVIDIA Tegra124 system-on-chip.
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> +static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
> + /* Pad calibration */
> +
> + ret = tegra_fuse_readl(0x224, );
> + if (ret) {
> + dev_err(>pdev->dev,
> + "failed to read sata calibration fuse: %d\n", ret);
> + return ret;
> + }
> +
> + calib = tegra124_pad_calibration[val];
Shouldn't val be range-checked before blindly using it?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
This adds support for the integrated AHCI-compliant Serial ATA
controller present on the NVIDIA Tegra124 system-on-chip.
diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
+static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
+ /* Pad calibration */
+
+ ret = tegra_fuse_readl(0x224, val);
+ if (ret) {
+ dev_err(tegra-pdev-dev,
+ failed to read sata calibration fuse: %d\n, ret);
+ return ret;
+ }
+
+ calib = tegra124_pad_calibration[val];
Shouldn't val be range-checked before blindly using it?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
Thanks, will remove.
- Mikko
On 05/06/14 15:18, Rob Herring wrote:
On Wed, Jun 4, 2014 at 6:32 AM, Mikko Perttunen wrote:
This adds support for the integrated AHCI-compliant Serial ATA
controller present on the NVIDIA Tegra124 system-on-chip.
Signed-off-by: Mikko Perttunen
---
[...]
+static int tegra_ahci_probe(struct platform_device *pdev)
+{
+ const struct of_device_id *match;
+ struct ahci_host_priv *hpriv;
+ struct tegra_ahci_priv *tegra;
+ int ret;
+
+ match = of_match_device(tegra_ahci_of_match, >dev);
This is not needed.
+ if (!match)
+ return -EINVAL;
+
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On Wed, Jun 4, 2014 at 6:32 AM, Mikko Perttunen wrote:
> This adds support for the integrated AHCI-compliant Serial ATA
> controller present on the NVIDIA Tegra124 system-on-chip.
>
> Signed-off-by: Mikko Perttunen
> ---
[...]
> +static int tegra_ahci_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *match;
> + struct ahci_host_priv *hpriv;
> + struct tegra_ahci_priv *tegra;
> + int ret;
> +
> + match = of_match_device(tegra_ahci_of_match, >dev);
This is not needed.
> + if (!match)
> + return -EINVAL;
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
On Wed, Jun 4, 2014 at 6:32 AM, Mikko Perttunen mperttu...@nvidia.com wrote:
This adds support for the integrated AHCI-compliant Serial ATA
controller present on the NVIDIA Tegra124 system-on-chip.
Signed-off-by: Mikko Perttunen mperttu...@nvidia.com
---
[...]
+static int tegra_ahci_probe(struct platform_device *pdev)
+{
+ const struct of_device_id *match;
+ struct ahci_host_priv *hpriv;
+ struct tegra_ahci_priv *tegra;
+ int ret;
+
+ match = of_match_device(tegra_ahci_of_match, pdev-dev);
This is not needed.
+ if (!match)
+ return -EINVAL;
+
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/9] ata: Add support for the Tegra124 SATA controller
Thanks, will remove.
- Mikko
On 05/06/14 15:18, Rob Herring wrote:
On Wed, Jun 4, 2014 at 6:32 AM, Mikko Perttunen mperttu...@nvidia.com wrote:
This adds support for the integrated AHCI-compliant Serial ATA
controller present on the NVIDIA Tegra124 system-on-chip.
Signed-off-by: Mikko Perttunen mperttu...@nvidia.com
---
[...]
+static int tegra_ahci_probe(struct platform_device *pdev)
+{
+ const struct of_device_id *match;
+ struct ahci_host_priv *hpriv;
+ struct tegra_ahci_priv *tegra;
+ int ret;
+
+ match = of_match_device(tegra_ahci_of_match, pdev-dev);
This is not needed.
+ if (!match)
+ return -EINVAL;
+
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
