| Message ID | 20180830190120.722-1-clabbe.montjoie@gmail.com |
|---|---|
| Headers | show |
| Series | ata: ahci_platform: support allwinner R40 AHCI | expand |
HI, On 30-08-18 21:01, Corentin Labbe wrote: > Since PHY code is now handled by sun4i-a10-sata-phy, the code in > ahci_sunxi is useless, remove it. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > drivers/ata/ahci_sunxi.c | 93 ------------------------------------------------ > 1 file changed, 93 deletions(-) > > diff --git a/drivers/ata/ahci_sunxi.c b/drivers/ata/ahci_sunxi.c > index b8cf3a1be80b..af17f8ce65b2 100644 > --- a/drivers/ata/ahci_sunxi.c > +++ b/drivers/ata/ahci_sunxi.c > @@ -58,15 +58,6 @@ MODULE_PARM_DESC(enable_pmp, > #define AHCI_P0PHYCR 0x0178 > #define AHCI_P0PHYSR 0x017c > > -static void sunxi_clrbits(void __iomem *reg, u32 clr_val) > -{ > - u32 reg_val; > - > - reg_val = readl(reg); > - reg_val &= ~(clr_val); > - writel(reg_val, reg); > -} > - > static void sunxi_setbits(void __iomem *reg, u32 set_val) > { > u32 reg_val; > @@ -86,81 +77,6 @@ static void sunxi_clrsetbits(void __iomem *reg, u32 clr_val, u32 set_val) > writel(reg_val, reg); > } > > -static u32 sunxi_getbits(void __iomem *reg, u8 mask, u8 shift) > -{ > - return (readl(reg) >> shift) & mask; > -} > - > -static int ahci_sunxi_phy_init(struct device *dev, void __iomem *reg_base) > -{ > - u32 reg_val; > - int timeout; > - > - /* > - * When using the new binding, the presence of a sata port node > - * means that PHY is handled by the PHY driver. > - * */ > - if (of_get_child_count(dev->of_node)) { > - dev_info(dev, "Bypassing PHY init\n"); > - return 0; > - } > - > - /* This magic is from the original code */ > - writel(0, reg_base + AHCI_RWCR); > - msleep(5); > - > - sunxi_setbits(reg_base + AHCI_PHYCS1R, BIT(19)); > - sunxi_clrsetbits(reg_base + AHCI_PHYCS0R, > - (0x7 << 24), > - (0x5 << 24) | BIT(23) | BIT(18)); > - sunxi_clrsetbits(reg_base + AHCI_PHYCS1R, > - (0x3 << 16) | (0x1f << 8) | (0x3 << 6), > - (0x2 << 16) | (0x6 << 8) | (0x2 << 6)); > - sunxi_setbits(reg_base + AHCI_PHYCS1R, BIT(28) | BIT(15)); > - sunxi_clrbits(reg_base + AHCI_PHYCS1R, BIT(19)); > - sunxi_clrsetbits(reg_base + AHCI_PHYCS0R, > - (0x7 << 20), (0x3 << 20)); > - sunxi_clrsetbits(reg_base + AHCI_PHYCS2R, > - (0x1f << 5), (0x19 << 5)); > - msleep(5); > - > - sunxi_setbits(reg_base + AHCI_PHYCS0R, (0x1 << 19)); > - > - timeout = 250; /* Power up takes aprox 50 us */ > - do { > - reg_val = sunxi_getbits(reg_base + AHCI_PHYCS0R, 0x7, 28); > - if (reg_val == 0x02) > - break; > - > - if (--timeout == 0) { > - dev_err(dev, "PHY power up failed.\n"); > - return -EIO; > - } > - udelay(1); > - } while (1); > - > - sunxi_setbits(reg_base + AHCI_PHYCS2R, (0x1 << 24)); > - > - timeout = 100; /* Calibration takes aprox 10 us */ > - do { > - reg_val = sunxi_getbits(reg_base + AHCI_PHYCS2R, 0x1, 24); > - if (reg_val == 0x00) > - break; > - > - if (--timeout == 0) { > - dev_err(dev, "PHY calibration failed.\n"); > - return -EIO; > - } > - udelay(1); > - } while (1); > - > - msleep(15); > - > - writel(0x7, reg_base + AHCI_RWCR); > - > - return 0; > -} > - > static void ahci_sunxi_start_engine(struct ata_port *ap) > { > void __iomem *port_mmio = ahci_port_base(ap); > @@ -186,7 +102,6 @@ static struct scsi_host_template ahci_platform_sht = { > > static int ahci_sunxi_probe(struct platform_device *pdev) > { > - struct device *dev = &pdev->dev; > struct ahci_host_priv *hpriv; > int rc; > > @@ -200,10 +115,6 @@ static int ahci_sunxi_probe(struct platform_device *pdev) > if (rc) > return rc; > > - rc = ahci_sunxi_phy_init(dev, hpriv->mmio); > - if (rc) > - goto disable_resources; > - > hpriv->flags = AHCI_HFLAG_32BIT_ONLY | AHCI_HFLAG_NO_MSI | > AHCI_HFLAG_YES_NCQ; > > @@ -238,10 +149,6 @@ static int ahci_sunxi_resume(struct device *dev) > if (rc) > return rc; > > - rc = ahci_sunxi_phy_init(dev, hpriv->mmio); > - if (rc) > - goto disable_resources; > - > rc = ahci_platform_resume_host(dev); > if (rc) > goto disable_resources; > After this change ahci_sunxi_resume() is the same as ahci_platform_resume, so you can drop the entire function and directly refer to ahci_platform_resume in ahci_sunxi_pm_ops. Regards, Hans
Hi, On Fri, Aug 31, 2018 at 4:31 AM Jens Axboe <axboe@kernel.dk> wrote: > > On 8/30/18 1:01 PM, Corentin Labbe wrote: > > Hello > > > > This patchset add support for allwinner R40 AHCI controller. > > > > The whole patchset is tested on sun8i-r40-bananapi-m2-ultra and > > on sun7i-a20-cubieboard2 which doesnt have any of the ressources added > > by this serie, so no regression should come with it. > > > > The last patch(ata: ahci_sunxi: remove PHY code) should not be merged, > > but will be resent for inclustion when all patchs will have hit linus > > tree. > > Applied 1-12 with Hans's blessing, thanks Corentin. Please don't merge device tree ("dts") patches, i.e. patches 9-12. We will merge them through the sunxi / armsoc tree. Having them in separate trees introduces conflicts when we have other stuff going through our tree. Corentin, it's best to lay out the plan to get patches merges in the cover letter, specifically which maintainer should take which patches, or if an immutable tag/branch is preferred, when things can't be separated cleanly. This helps other subsystem maintainers that don't routinely deal with armsoc. Also, we probably can't merge the last patch that removes the PHY code, since we have to support old device trees. Thanks ChenYu
On 8/30/18 8:32 PM, Chen-Yu Tsai wrote: > Hi, > > On Fri, Aug 31, 2018 at 4:31 AM Jens Axboe <axboe@kernel.dk> wrote: >> >> On 8/30/18 1:01 PM, Corentin Labbe wrote: >>> Hello >>> >>> This patchset add support for allwinner R40 AHCI controller. >>> >>> The whole patchset is tested on sun8i-r40-bananapi-m2-ultra and >>> on sun7i-a20-cubieboard2 which doesnt have any of the ressources added >>> by this serie, so no regression should come with it. >>> >>> The last patch(ata: ahci_sunxi: remove PHY code) should not be merged, >>> but will be resent for inclustion when all patchs will have hit linus >>> tree. >> >> Applied 1-12 with Hans's blessing, thanks Corentin. > > Please don't merge device tree ("dts") patches, i.e. patches 9-12. We will > merge them through the sunxi / armsoc tree. Having them in separate trees > introduces conflicts when we have other stuff going through our tree. OK, fair enough, I can drop those. -- Jens Axboe
On Thu, Aug 30, 2018 at 09:01:16PM +0200, Corentin Labbe wrote: > R40 have a sata controller which is the same as A20. > This patch adds a DT node for it. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > --- > arch/arm/boot/dts/sun8i-r40.dtsi | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > index 852c2ccc3268..d6b5820da850 100644 > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > @@ -550,6 +550,29 @@ > #size-cells = <0>; > }; > > + ahci: sata@1c18000 { > + compatible = "allwinner,sun8i-r40-ahci"; > + reg = <0x01c18000 0x1000>; > + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&ccu CLK_BUS_SATA>, <&ccu CLK_SATA>; > + resets = <&ccu RST_BUS_SATA>; > + resets-name = "ahci"; > + #address-cells = <1>; > + #size-cells = <0>; > + status = "disabled"; > + > + sata_port: sata-port@0 { > + reg = <0>; > + phys = <&sata_phy>; > + }; > + }; > + > + sata_phy: sata-phy@1c180c0 { > + compatible = "allwinner,sun8i-r40-sata-phy"; > + reg = <0x1c180c0 0x200>; Overlapping devices in the DTS is not ok. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Jens, On Thu, Aug 30, 2018 at 08:52:52PM -0600, Jens Axboe wrote: > On 8/30/18 8:32 PM, Chen-Yu Tsai wrote: > > Hi, > > > > On Fri, Aug 31, 2018 at 4:31 AM Jens Axboe <axboe@kernel.dk> wrote: > >> > >> On 8/30/18 1:01 PM, Corentin Labbe wrote: > >>> Hello > >>> > >>> This patchset add support for allwinner R40 AHCI controller. > >>> > >>> The whole patchset is tested on sun8i-r40-bananapi-m2-ultra and > >>> on sun7i-a20-cubieboard2 which doesnt have any of the ressources added > >>> by this serie, so no regression should come with it. > >>> > >>> The last patch(ata: ahci_sunxi: remove PHY code) should not be merged, > >>> but will be resent for inclustion when all patchs will have hit linus > >>> tree. > >> > >> Applied 1-12 with Hans's blessing, thanks Corentin. > > > > Please don't merge device tree ("dts") patches, i.e. patches 9-12. We will > > merge them through the sunxi / armsoc tree. Having them in separate trees > > introduces conflicts when we have other stuff going through our tree. > > OK, fair enough, I can drop those. And the DT bits actually have some issues that would need to change the code significantly. Can you drop all of them please? Thanks! Maxmie -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, Aug 31, 2018 at 09:37:24AM +0200, Maxime Ripard wrote: > On Thu, Aug 30, 2018 at 09:01:19PM +0200, Corentin Labbe wrote: > > This patch convert sun4i-a10 sata to the new binding which use a sata > > phy node. > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > arch/arm/boot/dts/sun4i-a10.dtsi | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi > > index 3d62a8950720..52d5c2e79499 100644 > > --- a/arch/arm/boot/dts/sun4i-a10.dtsi > > +++ b/arch/arm/boot/dts/sun4i-a10.dtsi > > @@ -556,7 +556,20 @@ > > reg = <0x01c18000 0x1000>; > > interrupts = <56>; > > clocks = <&ccu CLK_AHB_SATA>, <&ccu CLK_SATA>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > status = "disabled"; > > + > > + sata_port: sata-port@0 { > > + reg = <0>; > > + phys = <&sata_phy>; > > + }; > > + }; > > + > > + sata_phy: sata_phy@1c180c0 { > > + compatible = "allwinner,sun4i-a10-sata-phy"; > > + reg = <0x01c180c0 0x200>; > > + #phy-cells = <0>; > > }; > > This patch, together with the following one, breaks the DT ABI, this > isn't something we can do anymore. > So what can I do ? Keep the DT in place and drop the patch 13 like wens suggested ? Regards
On Fri, Aug 31, 2018 at 09:35:00AM +0200, Maxime Ripard wrote: > On Thu, Aug 30, 2018 at 09:01:16PM +0200, Corentin Labbe wrote: > > R40 have a sata controller which is the same as A20. > > This patch adds a DT node for it. > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > --- > > arch/arm/boot/dts/sun8i-r40.dtsi | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > > index 852c2ccc3268..d6b5820da850 100644 > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > @@ -550,6 +550,29 @@ > > #size-cells = <0>; > > }; > > > > + ahci: sata@1c18000 { > > + compatible = "allwinner,sun8i-r40-ahci"; > > + reg = <0x01c18000 0x1000>; > > + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&ccu CLK_BUS_SATA>, <&ccu CLK_SATA>; > > + resets = <&ccu RST_BUS_SATA>; > > + resets-name = "ahci"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + status = "disabled"; > > + > > + sata_port: sata-port@0 { > > + reg = <0>; > > + phys = <&sata_phy>; > > + }; > > + }; > > + > > + sata_phy: sata-phy@1c180c0 { > > + compatible = "allwinner,sun8i-r40-sata-phy"; > > + reg = <0x1c180c0 0x200>; > > Overlapping devices in the DTS is not ok. > I do the same than arch/arm/boot/dts/berlin2.dtsi (sata@e90000 phy@e900a0) But since it is not a good justification, it seems that regmap is my only solution ? Regards
On Fri, Aug 31, 2018 at 3:56 PM Corentin Labbe <clabbe.montjoie@gmail.com> wrote: > > On Fri, Aug 31, 2018 at 09:35:00AM +0200, Maxime Ripard wrote: > > On Thu, Aug 30, 2018 at 09:01:16PM +0200, Corentin Labbe wrote: > > > R40 have a sata controller which is the same as A20. > > > This patch adds a DT node for it. > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > --- > > > arch/arm/boot/dts/sun8i-r40.dtsi | 23 +++++++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > > > index 852c2ccc3268..d6b5820da850 100644 > > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > > @@ -550,6 +550,29 @@ > > > #size-cells = <0>; > > > }; > > > > > > + ahci: sata@1c18000 { > > > + compatible = "allwinner,sun8i-r40-ahci"; > > > + reg = <0x01c18000 0x1000>; > > > + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; > > > + clocks = <&ccu CLK_BUS_SATA>, <&ccu CLK_SATA>; > > > + resets = <&ccu RST_BUS_SATA>; > > > + resets-name = "ahci"; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + status = "disabled"; > > > + > > > + sata_port: sata-port@0 { > > > + reg = <0>; > > > + phys = <&sata_phy>; > > > + }; > > > + }; > > > + > > > + sata_phy: sata-phy@1c180c0 { > > > + compatible = "allwinner,sun8i-r40-sata-phy"; > > > + reg = <0x1c180c0 0x200>; > > > > Overlapping devices in the DTS is not ok. > > > > I do the same than arch/arm/boot/dts/berlin2.dtsi (sata@e90000 phy@e900a0) > But since it is not a good justification, it seems that regmap is my only solution ? Since you are effectively splitting one device node into two, you should adjust the original (ahci) device nodes register range. ChenYu
On Fri, Aug 31, 2018 at 03:58:34PM +0800, Chen-Yu Tsai wrote: > On Fri, Aug 31, 2018 at 3:56 PM Corentin Labbe > <clabbe.montjoie@gmail.com> wrote: > > > > On Fri, Aug 31, 2018 at 09:35:00AM +0200, Maxime Ripard wrote: > > > On Thu, Aug 30, 2018 at 09:01:16PM +0200, Corentin Labbe wrote: > > > > R40 have a sata controller which is the same as A20. > > > > This patch adds a DT node for it. > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > > --- > > > > arch/arm/boot/dts/sun8i-r40.dtsi | 23 +++++++++++++++++++++++ > > > > 1 file changed, 23 insertions(+) > > > > > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > index 852c2ccc3268..d6b5820da850 100644 > > > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > @@ -550,6 +550,29 @@ > > > > #size-cells = <0>; > > > > }; > > > > > > > > + ahci: sata@1c18000 { > > > > + compatible = "allwinner,sun8i-r40-ahci"; > > > > + reg = <0x01c18000 0x1000>; > > > > + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; > > > > + clocks = <&ccu CLK_BUS_SATA>, <&ccu CLK_SATA>; > > > > + resets = <&ccu RST_BUS_SATA>; > > > > + resets-name = "ahci"; > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + status = "disabled"; > > > > + > > > > + sata_port: sata-port@0 { > > > > + reg = <0>; > > > > + phys = <&sata_phy>; > > > > + }; > > > > + }; > > > > + > > > > + sata_phy: sata-phy@1c180c0 { > > > > + compatible = "allwinner,sun8i-r40-sata-phy"; > > > > + reg = <0x1c180c0 0x200>; > > > > > > Overlapping devices in the DTS is not ok. > > > > > > > I do the same than arch/arm/boot/dts/berlin2.dtsi (sata@e90000 phy@e900a0) > > But since it is not a good justification, it seems that regmap is my only solution ? > > Since you are effectively splitting one device node into two, you should > adjust the original (ahci) device nodes register range. > I cannot do that if I remove patch 13, iow If I keep phy init code in both place as you requested.
On Fri, Aug 31, 2018 at 09:56:31AM +0200, Corentin Labbe wrote: > On Fri, Aug 31, 2018 at 09:35:00AM +0200, Maxime Ripard wrote: > > On Thu, Aug 30, 2018 at 09:01:16PM +0200, Corentin Labbe wrote: > > > R40 have a sata controller which is the same as A20. > > > This patch adds a DT node for it. > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > --- > > > arch/arm/boot/dts/sun8i-r40.dtsi | 23 +++++++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > > > index 852c2ccc3268..d6b5820da850 100644 > > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > > @@ -550,6 +550,29 @@ > > > #size-cells = <0>; > > > }; > > > > > > + ahci: sata@1c18000 { > > > + compatible = "allwinner,sun8i-r40-ahci"; > > > + reg = <0x01c18000 0x1000>; > > > + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; > > > + clocks = <&ccu CLK_BUS_SATA>, <&ccu CLK_SATA>; > > > + resets = <&ccu RST_BUS_SATA>; > > > + resets-name = "ahci"; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + status = "disabled"; > > > + > > > + sata_port: sata-port@0 { > > > + reg = <0>; > > > + phys = <&sata_phy>; > > > + }; > > > + }; > > > + > > > + sata_phy: sata-phy@1c180c0 { > > > + compatible = "allwinner,sun8i-r40-sata-phy"; > > > + reg = <0x1c180c0 0x200>; > > > > Overlapping devices in the DTS is not ok. > > > > I do the same than arch/arm/boot/dts/berlin2.dtsi (sata@e90000 > phy@e900a0) > > But since it is not a good justification, it seems that regmap is my > only solution ? I'm not even sure why you are moving the phy out of its original node (and driver). Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, Aug 31, 2018 at 09:53:50AM +0200, Corentin Labbe wrote: > On Fri, Aug 31, 2018 at 09:37:24AM +0200, Maxime Ripard wrote: > > On Thu, Aug 30, 2018 at 09:01:19PM +0200, Corentin Labbe wrote: > > > This patch convert sun4i-a10 sata to the new binding which use a sata > > > phy node. > > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > --- > > > arch/arm/boot/dts/sun4i-a10.dtsi | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi > > > index 3d62a8950720..52d5c2e79499 100644 > > > --- a/arch/arm/boot/dts/sun4i-a10.dtsi > > > +++ b/arch/arm/boot/dts/sun4i-a10.dtsi > > > @@ -556,7 +556,20 @@ > > > reg = <0x01c18000 0x1000>; > > > interrupts = <56>; > > > clocks = <&ccu CLK_AHB_SATA>, <&ccu CLK_SATA>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > status = "disabled"; > > > + > > > + sata_port: sata-port@0 { > > > + reg = <0>; > > > + phys = <&sata_phy>; > > > + }; > > > + }; > > > + > > > + sata_phy: sata_phy@1c180c0 { > > > + compatible = "allwinner,sun4i-a10-sata-phy"; > > > + reg = <0x01c180c0 0x200>; > > > + #phy-cells = <0>; > > > }; > > > > This patch, together with the following one, breaks the DT ABI, this > > isn't something we can do anymore. > > So what can I do ? Well, you always have the option of not doing anything. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, Aug 31, 2018 at 12:20:21PM +0200, maxime.ripard@bootlin.com wrote: > On Fri, Aug 31, 2018 at 09:56:31AM +0200, Corentin Labbe wrote: > > On Fri, Aug 31, 2018 at 09:35:00AM +0200, Maxime Ripard wrote: > > > On Thu, Aug 30, 2018 at 09:01:16PM +0200, Corentin Labbe wrote: > > > > R40 have a sata controller which is the same as A20. > > > > This patch adds a DT node for it. > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > > --- > > > > arch/arm/boot/dts/sun8i-r40.dtsi | 23 +++++++++++++++++++++++ > > > > 1 file changed, 23 insertions(+) > > > > > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > index 852c2ccc3268..d6b5820da850 100644 > > > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > @@ -550,6 +550,29 @@ > > > > #size-cells = <0>; > > > > }; > > > > > > > > + ahci: sata@1c18000 { > > > > + compatible = "allwinner,sun8i-r40-ahci"; > > > > + reg = <0x01c18000 0x1000>; > > > > + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; > > > > + clocks = <&ccu CLK_BUS_SATA>, <&ccu CLK_SATA>; > > > > + resets = <&ccu RST_BUS_SATA>; > > > > + resets-name = "ahci"; > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + status = "disabled"; > > > > + > > > > + sata_port: sata-port@0 { > > > > + reg = <0>; > > > > + phys = <&sata_phy>; > > > > + }; > > > > + }; > > > > + > > > > + sata_phy: sata-phy@1c180c0 { > > > > + compatible = "allwinner,sun8i-r40-sata-phy"; > > > > + reg = <0x1c180c0 0x200>; > > > > > > Overlapping devices in the DTS is not ok. > > > > > > > I do the same than arch/arm/boot/dts/berlin2.dtsi (sata@e90000 > > phy@e900a0) > > > > But since it is not a good justification, it seems that regmap is my > > only solution ? > > I'm not even sure why you are moving the phy out of its original node > (and driver). > For using the phy-supply already handled by the code. The other choice is to add another xxx-supply to ahci_platform. Or to use hackily port_regulator for this regulator.
On Fri, Aug 31, 2018 at 5:29 PM Corentin Labbe <clabbe.montjoie@gmail.com> wrote: > > On Fri, Aug 31, 2018 at 03:58:34PM +0800, Chen-Yu Tsai wrote: > > On Fri, Aug 31, 2018 at 3:56 PM Corentin Labbe > > <clabbe.montjoie@gmail.com> wrote: > > > > > > On Fri, Aug 31, 2018 at 09:35:00AM +0200, Maxime Ripard wrote: > > > > On Thu, Aug 30, 2018 at 09:01:16PM +0200, Corentin Labbe wrote: > > > > > R40 have a sata controller which is the same as A20. > > > > > This patch adds a DT node for it. > > > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > > > --- > > > > > arch/arm/boot/dts/sun8i-r40.dtsi | 23 +++++++++++++++++++++++ > > > > > 1 file changed, 23 insertions(+) > > > > > > > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > index 852c2ccc3268..d6b5820da850 100644 > > > > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > @@ -550,6 +550,29 @@ > > > > > #size-cells = <0>; > > > > > }; > > > > > > > > > > + ahci: sata@1c18000 { > > > > > + compatible = "allwinner,sun8i-r40-ahci"; > > > > > + reg = <0x01c18000 0x1000>; > > > > > + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&ccu CLK_BUS_SATA>, <&ccu CLK_SATA>; > > > > > + resets = <&ccu RST_BUS_SATA>; > > > > > + resets-name = "ahci"; > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + status = "disabled"; > > > > > + > > > > > + sata_port: sata-port@0 { > > > > > + reg = <0>; > > > > > + phys = <&sata_phy>; > > > > > + }; > > > > > + }; > > > > > + > > > > > + sata_phy: sata-phy@1c180c0 { > > > > > + compatible = "allwinner,sun8i-r40-sata-phy"; > > > > > + reg = <0x1c180c0 0x200>; > > > > > > > > Overlapping devices in the DTS is not ok. > > > > > > > > > > I do the same than arch/arm/boot/dts/berlin2.dtsi (sata@e90000 phy@e900a0) > > > But since it is not a good justification, it seems that regmap is my only solution ? > > > > Since you are effectively splitting one device node into two, you should > > adjust the original (ahci) device nodes register range. > > > > I cannot do that if I remove patch 13, iow If I keep phy init code in both place as you requested. Then you need to split the phy handling between a10 and r40. A10 doesn't need phy-supply at the moment anyway. And migrating A10 to newer binding is only causing you problems anyway. Just drop that part, and handle the R40. ChenYu
On Fri, Aug 31, 2018 at 6:54 PM Corentin Labbe <clabbe.montjoie@gmail.com> wrote: > > On Fri, Aug 31, 2018 at 12:20:21PM +0200, maxime.ripard@bootlin.com wrote: > > On Fri, Aug 31, 2018 at 09:56:31AM +0200, Corentin Labbe wrote: > > > On Fri, Aug 31, 2018 at 09:35:00AM +0200, Maxime Ripard wrote: > > > > On Thu, Aug 30, 2018 at 09:01:16PM +0200, Corentin Labbe wrote: > > > > > R40 have a sata controller which is the same as A20. > > > > > This patch adds a DT node for it. > > > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > > > --- > > > > > arch/arm/boot/dts/sun8i-r40.dtsi | 23 +++++++++++++++++++++++ > > > > > 1 file changed, 23 insertions(+) > > > > > > > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > index 852c2ccc3268..d6b5820da850 100644 > > > > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > @@ -550,6 +550,29 @@ > > > > > #size-cells = <0>; > > > > > }; > > > > > > > > > > + ahci: sata@1c18000 { > > > > > + compatible = "allwinner,sun8i-r40-ahci"; > > > > > + reg = <0x01c18000 0x1000>; > > > > > + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; > > > > > + clocks = <&ccu CLK_BUS_SATA>, <&ccu CLK_SATA>; > > > > > + resets = <&ccu RST_BUS_SATA>; > > > > > + resets-name = "ahci"; > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + status = "disabled"; > > > > > + > > > > > + sata_port: sata-port@0 { > > > > > + reg = <0>; > > > > > + phys = <&sata_phy>; > > > > > + }; > > > > > + }; > > > > > + > > > > > + sata_phy: sata-phy@1c180c0 { > > > > > + compatible = "allwinner,sun8i-r40-sata-phy"; > > > > > + reg = <0x1c180c0 0x200>; > > > > > > > > Overlapping devices in the DTS is not ok. > > > > > > > > > > I do the same than arch/arm/boot/dts/berlin2.dtsi (sata@e90000 > > > phy@e900a0) > > > > > > But since it is not a good justification, it seems that regmap is my > > > only solution ? > > > > I'm not even sure why you are moving the phy out of its original node > > (and driver). > > > > For using the phy-supply already handled by the code. > The other choice is to add another xxx-supply to ahci_platform. > Or to use hackily port_regulator for this regulator. The PHY registers are in the AHCI's "vendor specific registers" region. Following that are the per-port registers, which the ahci driver will need access to. This doesn't look like it should deserve a separate device node. What's wrong with handling the regulator directly in the ahci-sunxi PHY init code? ChenYu
On Fri, Aug 31, 2018 at 07:31:37PM +0800, Chen-Yu Tsai wrote: > On Fri, Aug 31, 2018 at 6:54 PM Corentin Labbe > <clabbe.montjoie@gmail.com> wrote: > > > > On Fri, Aug 31, 2018 at 12:20:21PM +0200, maxime.ripard@bootlin.com wrote: > > > On Fri, Aug 31, 2018 at 09:56:31AM +0200, Corentin Labbe wrote: > > > > On Fri, Aug 31, 2018 at 09:35:00AM +0200, Maxime Ripard wrote: > > > > > On Thu, Aug 30, 2018 at 09:01:16PM +0200, Corentin Labbe wrote: > > > > > > R40 have a sata controller which is the same as A20. > > > > > > This patch adds a DT node for it. > > > > > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > > > > --- > > > > > > arch/arm/boot/dts/sun8i-r40.dtsi | 23 +++++++++++++++++++++++ > > > > > > 1 file changed, 23 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > > index 852c2ccc3268..d6b5820da850 100644 > > > > > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > > @@ -550,6 +550,29 @@ > > > > > > #size-cells = <0>; > > > > > > }; > > > > > > > > > > > > + ahci: sata@1c18000 { > > > > > > + compatible = "allwinner,sun8i-r40-ahci"; > > > > > > + reg = <0x01c18000 0x1000>; > > > > > > + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; > > > > > > + clocks = <&ccu CLK_BUS_SATA>, <&ccu CLK_SATA>; > > > > > > + resets = <&ccu RST_BUS_SATA>; > > > > > > + resets-name = "ahci"; > > > > > > + #address-cells = <1>; > > > > > > + #size-cells = <0>; > > > > > > + status = "disabled"; > > > > > > + > > > > > > + sata_port: sata-port@0 { > > > > > > + reg = <0>; > > > > > > + phys = <&sata_phy>; > > > > > > + }; > > > > > > + }; > > > > > > + > > > > > > + sata_phy: sata-phy@1c180c0 { > > > > > > + compatible = "allwinner,sun8i-r40-sata-phy"; > > > > > > + reg = <0x1c180c0 0x200>; > > > > > > > > > > Overlapping devices in the DTS is not ok. > > > > > > > > > > > > > I do the same than arch/arm/boot/dts/berlin2.dtsi (sata@e90000 > > > > phy@e900a0) > > > > > > > > But since it is not a good justification, it seems that regmap is my > > > > only solution ? > > > > > > I'm not even sure why you are moving the phy out of its original node > > > (and driver). > > > > > > > For using the phy-supply already handled by the code. > > The other choice is to add another xxx-supply to ahci_platform. > > Or to use hackily port_regulator for this regulator. > > The PHY registers are in the AHCI's "vendor specific registers" > region. Following that are the per-port registers, which the ahci > driver will need access to. This doesn't look like it should > deserve a separate device node. > > What's wrong with handling the regulator directly in the ahci-sunxi > PHY init code? > The reason are that I didnt wanted to use the port-regulator, and I didnt want to add new code to ahci_platform. I tried to place a phy-supply in the ahci node, but it doesnt work (with or without a phy subnode). Moving PHy code in a dedicated driver seemed to be good, but with all your comments and Maxime's one, it seems not. I will keep ahci_sunxi as-is and will try to found how to add the needed phy-supply. Regards
在 2018-08-31五的 19:10 +0800，Chen-Yu Tsai写道： > On Fri, Aug 31, 2018 at 5:29 PM Corentin Labbe > <clabbe.montjoie@gmail.com> wrote: > > > > On Fri, Aug 31, 2018 at 03:58:34PM +0800, Chen-Yu Tsai wrote: > > > On Fri, Aug 31, 2018 at 3:56 PM Corentin Labbe > > > <clabbe.montjoie@gmail.com> wrote: > > > > > > > > On Fri, Aug 31, 2018 at 09:35:00AM +0200, Maxime Ripard wrote: > > > > > On Thu, Aug 30, 2018 at 09:01:16PM +0200, Corentin Labbe > > > > > wrote: > > > > > > R40 have a sata controller which is the same as A20. > > > > > > This patch adds a DT node for it. > > > > > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > > > > --- > > > > > > arch/arm/boot/dts/sun8i-r40.dtsi | 23 > > > > > > +++++++++++++++++++++++ > > > > > > 1 file changed, 23 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > > b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > > index 852c2ccc3268..d6b5820da850 100644 > > > > > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > > @@ -550,6 +550,29 @@ > > > > > > #size-cells = <0>; > > > > > > }; > > > > > > > > > > > > + ahci: sata@1c18000 { > > > > > > + compatible = "allwinner,sun8i-r40- > > > > > > ahci"; > > > > > > + reg = <0x01c18000 0x1000>; > > > > > > + interrupts = <GIC_SPI 56 > > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > > + clocks = <&ccu CLK_BUS_SATA>, <&ccu > > > > > > CLK_SATA>; > > > > > > + resets = <&ccu RST_BUS_SATA>; > > > > > > + resets-name = "ahci"; > > > > > > + #address-cells = <1>; > > > > > > + #size-cells = <0>; > > > > > > + status = "disabled"; > > > > > > + > > > > > > + sata_port: sata-port@0 { > > > > > > + reg = <0>; > > > > > > + phys = <&sata_phy>; > > > > > > + }; > > > > > > + }; > > > > > > + > > > > > > + sata_phy: sata-phy@1c180c0 { > > > > > > + compatible = "allwinner,sun8i-r40-sata- > > > > > > phy"; > > > > > > + reg = <0x1c180c0 0x200>; > > > > > > > > > > Overlapping devices in the DTS is not ok. > > > > > > > > > > > > > I do the same than arch/arm/boot/dts/berlin2.dtsi (sata@e90000 > > > > phy@e900a0) > > > > But since it is not a good justification, it seems that regmap > > > > is my only solution ? > > > > > > Since you are effectively splitting one device node into two, you > > > should > > > adjust the original (ahci) device nodes register range. > > > > > > > I cannot do that if I remove patch 13, iow If I keep phy init code > > in both place as you requested. > > Then you need to split the phy handling between a10 and r40. A10 > doesn't > need phy-supply at the moment anyway. And migrating A10 to newer > binding > is only causing you problems anyway. Just drop that part, and handle > the > R40. From the hardware perspective, they're the same. The A10/A20 has also two dedicated VDD pins for SATA, although on boards they're usually always on. > > ChenYu > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
在 2018-08-31五的 19:31 +0800，Chen-Yu Tsai写道： > On Fri, Aug 31, 2018 at 6:54 PM Corentin Labbe > <clabbe.montjoie@gmail.com> wrote: > > > > On Fri, Aug 31, 2018 at 12:20:21PM +0200, maxime.ripard@bootlin.com > > wrote: > > > On Fri, Aug 31, 2018 at 09:56:31AM +0200, Corentin Labbe wrote: > > > > On Fri, Aug 31, 2018 at 09:35:00AM +0200, Maxime Ripard wrote: > > > > > On Thu, Aug 30, 2018 at 09:01:16PM +0200, Corentin Labbe > > > > > wrote: > > > > > > R40 have a sata controller which is the same as A20. > > > > > > This patch adds a DT node for it. > > > > > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> > > > > > > --- > > > > > > arch/arm/boot/dts/sun8i-r40.dtsi | 23 > > > > > > +++++++++++++++++++++++ > > > > > > 1 file changed, 23 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > > b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > > index 852c2ccc3268..d6b5820da850 100644 > > > > > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi > > > > > > @@ -550,6 +550,29 @@ > > > > > > #size-cells = <0>; > > > > > > }; > > > > > > > > > > > > + ahci: sata@1c18000 { > > > > > > + compatible = "allwinner,sun8i-r40- > > > > > > ahci"; > > > > > > + reg = <0x01c18000 0x1000>; > > > > > > + interrupts = <GIC_SPI 56 > > > > > > IRQ_TYPE_LEVEL_HIGH>; > > > > > > + clocks = <&ccu CLK_BUS_SATA>, <&ccu > > > > > > CLK_SATA>; > > > > > > + resets = <&ccu RST_BUS_SATA>; > > > > > > + resets-name = "ahci"; > > > > > > + #address-cells = <1>; > > > > > > + #size-cells = <0>; > > > > > > + status = "disabled"; > > > > > > + > > > > > > + sata_port: sata-port@0 { > > > > > > + reg = <0>; > > > > > > + phys = <&sata_phy>; > > > > > > + }; > > > > > > + }; > > > > > > + > > > > > > + sata_phy: sata-phy@1c180c0 { > > > > > > + compatible = "allwinner,sun8i-r40- > > > > > > sata-phy"; > > > > > > + reg = <0x1c180c0 0x200>; > > > > > > > > > > Overlapping devices in the DTS is not ok. > > > > > > > > > > > > > I do the same than arch/arm/boot/dts/berlin2.dtsi (sata@e90000 > > > > phy@e900a0) > > > > > > > > But since it is not a good justification, it seems that regmap > > > > is my > > > > only solution ? > > > > > > I'm not even sure why you are moving the phy out of its original > > > node > > > (and driver). > > > > > > > For using the phy-supply already handled by the code. > > The other choice is to add another xxx-supply to ahci_platform. > > Or to use hackily port_regulator for this regulator. > > The PHY registers are in the AHCI's "vendor specific registers" > region. Following that are the per-port registers, which the ahci > driver will need access to. This doesn't look like it should > deserve a separate device node. > > What's wrong with handling the regulator directly in the ahci-sunxi > PHY init code? I remember I sent a patch that did this some times ago, and gets rejected. > > ChenYu