[U-Boot] [PATCH 1/2] scsi: ceva: Clean up the driver code
Distinguish the ecc val by chassis version and move the ecc addr to dts. Add ls1028a soc support.
Signed-off-by: Peng Ma peng.ma@nxp.com --- drivers/ata/sata_ceva.c | 43 +++++++++++++++++++++++++------------------ 1 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index 8887be9..d26f712 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -88,20 +88,16 @@ #define LS1021_CEVA_PHY4_CFG 0x064a080b #define LS1021_CEVA_PHY5_CFG 0x2aa86470
-/* for ls1088a */ -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 -#define LS1088_ECC_DIS_VAL_CH2 0x40000000 - -/* ecc addr-val pair */ -#define ECC_DIS_ADDR_CH2 0x20140520 +/* ecc val pair */ +#define ECC_DIS_VAL_CH1 0x00020000 #define ECC_DIS_VAL_CH2 0x80000000 -#define SATA_ECC_REG_ADDR 0x20220520 -#define SATA_ECC_DISABLE 0x00020000 +#define ECC_DIS_VAL_CH3 0x40000000
enum ceva_soc { CEVA_1V84, CEVA_LS1012A, CEVA_LS1021A, + CEVA_LS1028A, CEVA_LS1043A, CEVA_LS1046A, CEVA_LS1088A, @@ -110,12 +106,14 @@ enum ceva_soc {
struct ceva_sata_priv { ulong base; + ulong ecc_base; enum ceva_soc soc; ulong flag; };
static int ceva_init_sata(struct ceva_sata_priv *priv) { + ulong ecc_addr = priv->ecc_base; ulong base = priv->base; ulong tmp;
@@ -132,38 +130,38 @@ static int ceva_init_sata(struct ceva_sata_priv *priv) break;
case CEVA_LS1021A: - writel(SATA_ECC_DISABLE, SATA_ECC_REG_ADDR); + if (ecc_addr) + writel(ECC_DIS_VAL_CH1, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(LS1021_CEVA_PHY2_CFG, base + AHCI_VEND_PP2C); writel(LS1021_CEVA_PHY3_CFG, base + AHCI_VEND_PP3C); writel(LS1021_CEVA_PHY4_CFG, base + AHCI_VEND_PP4C); writel(LS1021_CEVA_PHY5_CFG, base + AHCI_VEND_PP5C); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); - if (priv->flag & FLAG_COHERENT) - writel(CEVA_AXICC_CFG, base + LS1021_AHCI_VEND_AXICC); break;
case CEVA_LS1012A: case CEVA_LS1043A: case CEVA_LS1046A: - writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2); - /* fallthrough */ case CEVA_LS2080A: + if (ecc_addr) + writel(ECC_DIS_VAL_CH2, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); - if (priv->flag & FLAG_COHERENT) - writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC); break;
+ case CEVA_LS1028A: case CEVA_LS1088A: - writel(LS1088_ECC_DIS_VAL_CH2, LS1088_ECC_DIS_ADDR_CH2); + if (ecc_addr) + writel(ECC_DIS_VAL_CH3, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); - if (priv->flag & FLAG_COHERENT) - writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC); break; }
+ if (priv->flag & FLAG_COHERENT) + writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC); + return 0; }
@@ -187,6 +185,7 @@ static const struct udevice_id sata_ceva_ids[] = { { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A }, + { .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A }, { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A }, @@ -205,8 +204,16 @@ static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
+ priv->ecc_base = dev_read_addr_index(dev, 1); + if (priv->ecc_base == FDT_ADDR_T_NONE) + priv->ecc_base = 0; + priv->soc = dev_get_driver_data(dev);
+ debug("ccsr-sata-base %lx\t ecc-base %lx\n", + priv->base, + priv->ecc_base); + return 0; }
Move the ecc addr from driver to dts.
Signed-off-by: Peng Ma peng.ma@nxp.com --- arch/arm/dts/fsl-ls1012a.dtsi | 3 ++- arch/arm/dts/fsl-ls1043a.dtsi | 3 ++- arch/arm/dts/fsl-ls1046a.dtsi | 3 ++- arch/arm/dts/fsl-ls1088a.dtsi | 2 ++ arch/arm/dts/ls1021a.dtsi | 3 ++- 5 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/arch/arm/dts/fsl-ls1012a.dtsi b/arch/arm/dts/fsl-ls1012a.dtsi index f22cbf4..30fe268 100644 --- a/arch/arm/dts/fsl-ls1012a.dtsi +++ b/arch/arm/dts/fsl-ls1012a.dtsi @@ -136,7 +136,8 @@
sata: sata@3200000 { compatible = "fsl,ls1012a-ahci"; - reg = <0x0 0x3200000 0x0 0x10000>; + reg = <0x0 0x3200000 0x0 0x10000 /* ccsr sata base */ + 0x0 0x20140520 0x0 0x4>; /* ecc sata addr*/ interrupts = <0 69 4>; clocks = <&clockgen 4 0>; status = "disabled"; diff --git a/arch/arm/dts/fsl-ls1043a.dtsi b/arch/arm/dts/fsl-ls1043a.dtsi index bb70992..3109765 100644 --- a/arch/arm/dts/fsl-ls1043a.dtsi +++ b/arch/arm/dts/fsl-ls1043a.dtsi @@ -290,7 +290,8 @@
sata: sata@3200000 { compatible = "fsl,ls1043a-ahci"; - reg = <0x0 0x3200000 0x0 0x10000>; + reg = <0x0 0x3200000 0x0 0x10000 /* ccsr sata base */ + 0x0 0x20140520 0x0 0x4>; /* ecc sata addr*/ interrupts = <0 69 4>; clocks = <&clockgen 4 0>; status = "disabled"; diff --git a/arch/arm/dts/fsl-ls1046a.dtsi b/arch/arm/dts/fsl-ls1046a.dtsi index 5ac10e0..e9c8243 100644 --- a/arch/arm/dts/fsl-ls1046a.dtsi +++ b/arch/arm/dts/fsl-ls1046a.dtsi @@ -294,7 +294,8 @@
sata: sata@3200000 { compatible = "fsl,ls1046a-ahci"; - reg = <0x0 0x3200000 0x0 0x10000>; + reg = <0x0 0x3200000 0x0 0x10000 /* ccsr sata base */ + 0x0 0x20140520 0x0 0x4>; /* ecc sata addr*/ interrupts = <0 69 4>; clocks = <&clockgen 4 1>; status = "disabled"; diff --git a/arch/arm/dts/fsl-ls1088a.dtsi b/arch/arm/dts/fsl-ls1088a.dtsi index 9455e03..6a1cd0d 100644 --- a/arch/arm/dts/fsl-ls1088a.dtsi +++ b/arch/arm/dts/fsl-ls1088a.dtsi @@ -154,6 +154,8 @@ sata: sata@3200000 { compatible = "fsl,ls1088a-ahci"; reg = <0x0 0x3200000 0x0 0x10000>; + reg = <0x0 0x3200000 0x0 0x10000 /* ccsr sata base */ + 0x7 0x100520 0x0 0x4>; /* ecc sata addr*/ interrupts = <0 133 4>; status = "disabled"; }; diff --git a/arch/arm/dts/ls1021a.dtsi b/arch/arm/dts/ls1021a.dtsi index 7670a39..b3d1b07 100644 --- a/arch/arm/dts/ls1021a.dtsi +++ b/arch/arm/dts/ls1021a.dtsi @@ -406,7 +406,8 @@
sata: sata@3200000 { compatible = "fsl,ls1021a-ahci"; - reg = <0x3200000 0x10000>; + reg = <0x0 0x3200000 0x0 0x10000 /* ccsr sata base */ + 0x0 0x20220520 0x0 0x4>; /* ecc sata addr*/ interrupts = <0 101 4>; status = "disabled"; };
On 16. 04. 19 9:28, Peng Ma wrote:
Distinguish the ecc val by chassis version and move the ecc addr to dts. Add ls1028a soc support.
Signed-off-by: Peng Ma peng.ma@nxp.com
drivers/ata/sata_ceva.c | 43 +++++++++++++++++++++++++------------------ 1 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index 8887be9..d26f712 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -88,20 +88,16 @@ #define LS1021_CEVA_PHY4_CFG 0x064a080b #define LS1021_CEVA_PHY5_CFG 0x2aa86470
-/* for ls1088a */ -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 -#define LS1088_ECC_DIS_VAL_CH2 0x40000000
-/* ecc addr-val pair */ -#define ECC_DIS_ADDR_CH2 0x20140520 +/* ecc val pair */ +#define ECC_DIS_VAL_CH1 0x00020000 #define ECC_DIS_VAL_CH2 0x80000000 -#define SATA_ECC_REG_ADDR 0x20220520 -#define SATA_ECC_DISABLE 0x00020000 +#define ECC_DIS_VAL_CH3 0x40000000
enum ceva_soc { CEVA_1V84, CEVA_LS1012A, CEVA_LS1021A,
- CEVA_LS1028A, CEVA_LS1043A, CEVA_LS1046A, CEVA_LS1088A,
@@ -110,12 +106,14 @@ enum ceva_soc {
struct ceva_sata_priv { ulong base;
- ulong ecc_base; enum ceva_soc soc; ulong flag;
};
static int ceva_init_sata(struct ceva_sata_priv *priv) {
- ulong ecc_addr = priv->ecc_base; ulong base = priv->base; ulong tmp;
@@ -132,38 +130,38 @@ static int ceva_init_sata(struct ceva_sata_priv *priv) break;
case CEVA_LS1021A:
writel(SATA_ECC_DISABLE, SATA_ECC_REG_ADDR);
if (ecc_addr)writel(ECC_DIS_VAL_CH1, ecc_addr);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base + LS1021_AHCI_VEND_AXICC);break;
case CEVA_LS1012A: case CEVA_LS1043A: case CEVA_LS1046A:
writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2);/* fallthrough */case CEVA_LS2080A:
if (ecc_addr)writel(ECC_DIS_VAL_CH2, ecc_addr);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC);
- case CEVA_LS1028A: case CEVA_LS1088A:
writel(LS1088_ECC_DIS_VAL_CH2, LS1088_ECC_DIS_ADDR_CH2);
if (ecc_addr)writel(ECC_DIS_VAL_CH3, ecc_addr);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC);
- if (priv->flag & FLAG_COHERENT)
writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC);- return 0;
}
@@ -187,6 +185,7 @@ static const struct udevice_id sata_ceva_ids[] = { { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A },
- { .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A }, { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A },
@@ -205,8 +204,16 @@ static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
- priv->ecc_base = dev_read_addr_index(dev, 1);
It would be better to do it via reg-name instead of index. But that's up to your binding doc.
Thanks, Michal
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 16:04 To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; michal.simek@xilinx.com; u-boot@lists.denx.de Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 9:28, Peng Ma wrote:
Distinguish the ecc val by chassis version and move the ecc addr to dts. Add ls1028a soc support.
Signed-off-by: Peng Ma peng.ma@nxp.com
drivers/ata/sata_ceva.c | 43
+++++++++++++++++++++++++------------------
1 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index 8887be9..d26f712 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -88,20 +88,16 @@ #define LS1021_CEVA_PHY4_CFG 0x064a080b #define
LS1021_CEVA_PHY5_CFG
0x2aa86470
-/* for ls1088a */ -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 -#define LS1088_ECC_DIS_VAL_CH2 0x40000000
-/* ecc addr-val pair */ -#define ECC_DIS_ADDR_CH2 0x20140520 +/* ecc val pair */ +#define ECC_DIS_VAL_CH1 0x00020000 #define ECC_DIS_VAL_CH2 0x80000000 -#define SATA_ECC_REG_ADDR 0x20220520 -#define SATA_ECC_DISABLE 0x00020000 +#define ECC_DIS_VAL_CH3 0x40000000
enum ceva_soc { CEVA_1V84, CEVA_LS1012A, CEVA_LS1021A,
CEVA_LS1028A, CEVA_LS1043A, CEVA_LS1046A, CEVA_LS1088A,@@ -110,12 +106,14 @@ enum ceva_soc {
struct ceva_sata_priv { ulong base;
ulong ecc_base; enum ceva_soc soc; ulong flag;};
static int ceva_init_sata(struct ceva_sata_priv *priv) {
ulong ecc_addr = priv->ecc_base; ulong base = priv->base; ulong tmp;@@ -132,38 +130,38 @@ static int ceva_init_sata(struct ceva_sata_priv
*priv)
break; case CEVA_LS1021A:
writel(SATA_ECC_DISABLE, SATA_ECC_REG_ADDR);
if (ecc_addr)writel(ECC_DIS_VAL_CH1, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(LS1021_CEVA_PHY2_CFG, base +AHCI_VEND_PP2C);
writel(LS1021_CEVA_PHY3_CFG, base +AHCI_VEND_PP3C);
writel(LS1021_CEVA_PHY4_CFG, base +AHCI_VEND_PP4C);
writel(LS1021_CEVA_PHY5_CFG, base +AHCI_VEND_PP5C);
writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +LS1021_AHCI_VEND_AXICC);
break; case CEVA_LS1012A: case CEVA_LS1043A: case CEVA_LS1046A:
writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2);/* fallthrough */ case CEVA_LS2080A:
if (ecc_addr)writel(ECC_DIS_VAL_CH2, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +AHCI_VEND_AXICC);
break;
case CEVA_LS1028A: case CEVA_LS1088A:
writel(LS1088_ECC_DIS_VAL_CH2,LS1088_ECC_DIS_ADDR_CH2);
if (ecc_addr)writel(ECC_DIS_VAL_CH3, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +AHCI_VEND_AXICC);
break; }
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC);return 0;}
@@ -187,6 +185,7 @@ static const struct udevice_id sata_ceva_ids[] = { { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A },
{ .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A }, { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A }, @@-205,8 +204,16 @@ static int sata_ceva_ofdata_to_platdata(struct udevice
*dev)
if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);It would be better to do it via reg-name instead of index. But that's up to your binding doc.
[Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file sata node as fallows?
diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi index dfb6ebc..9ad2d84 100644 --- a/arch/arm/dts/zynqmp.dtsi +++ b/arch/arm/dts/zynqmp.dtsi @@ -692,6 +692,7 @@ compatible = "ceva,ahci-1v84"; status = "disabled"; reg = <0x0 0xfd0c0000 0x0 0x2000>; + reg-names = "serdes"; interrupt-parent = <&gic>; interrupts = <0 133 4>; #stream-id-cells = <4>; Best Regards, Peng
Thanks, Michal
On 16. 04. 19 11:22, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 16:04 To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; michal.simek@xilinx.com; u-boot@lists.denx.de Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 9:28, Peng Ma wrote:
Distinguish the ecc val by chassis version and move the ecc addr to dts. Add ls1028a soc support.
Signed-off-by: Peng Ma peng.ma@nxp.com
drivers/ata/sata_ceva.c | 43
+++++++++++++++++++++++++------------------
1 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index 8887be9..d26f712 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -88,20 +88,16 @@ #define LS1021_CEVA_PHY4_CFG 0x064a080b #define
LS1021_CEVA_PHY5_CFG
0x2aa86470
-/* for ls1088a */ -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 -#define LS1088_ECC_DIS_VAL_CH2 0x40000000
-/* ecc addr-val pair */ -#define ECC_DIS_ADDR_CH2 0x20140520 +/* ecc val pair */ +#define ECC_DIS_VAL_CH1 0x00020000 #define ECC_DIS_VAL_CH2 0x80000000 -#define SATA_ECC_REG_ADDR 0x20220520 -#define SATA_ECC_DISABLE 0x00020000 +#define ECC_DIS_VAL_CH3 0x40000000
enum ceva_soc { CEVA_1V84, CEVA_LS1012A, CEVA_LS1021A,
CEVA_LS1028A, CEVA_LS1043A, CEVA_LS1046A, CEVA_LS1088A,@@ -110,12 +106,14 @@ enum ceva_soc {
struct ceva_sata_priv { ulong base;
ulong ecc_base; enum ceva_soc soc; ulong flag;};
static int ceva_init_sata(struct ceva_sata_priv *priv) {
ulong ecc_addr = priv->ecc_base; ulong base = priv->base; ulong tmp;@@ -132,38 +130,38 @@ static int ceva_init_sata(struct ceva_sata_priv
*priv)
break; case CEVA_LS1021A:
writel(SATA_ECC_DISABLE, SATA_ECC_REG_ADDR);
if (ecc_addr)writel(ECC_DIS_VAL_CH1, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(LS1021_CEVA_PHY2_CFG, base +AHCI_VEND_PP2C);
writel(LS1021_CEVA_PHY3_CFG, base +AHCI_VEND_PP3C);
writel(LS1021_CEVA_PHY4_CFG, base +AHCI_VEND_PP4C);
writel(LS1021_CEVA_PHY5_CFG, base +AHCI_VEND_PP5C);
writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +LS1021_AHCI_VEND_AXICC);
break; case CEVA_LS1012A: case CEVA_LS1043A: case CEVA_LS1046A:
writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2);/* fallthrough */ case CEVA_LS2080A:
if (ecc_addr)writel(ECC_DIS_VAL_CH2, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +AHCI_VEND_AXICC);
break;
case CEVA_LS1028A: case CEVA_LS1088A:
writel(LS1088_ECC_DIS_VAL_CH2,LS1088_ECC_DIS_ADDR_CH2);
if (ecc_addr)writel(ECC_DIS_VAL_CH3, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +AHCI_VEND_AXICC);
break; }
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC);return 0;}
@@ -187,6 +185,7 @@ static const struct udevice_id sata_ceva_ids[] = { { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A },
{ .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A }, { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A }, @@-205,8 +204,16 @@ static int sata_ceva_ofdata_to_platdata(struct udevice
*dev)
if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);It would be better to do it via reg-name instead of index. But that's up to your binding doc.
[Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file sata node as fallows?
diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi index dfb6ebc..9ad2d84 100644 --- a/arch/arm/dts/zynqmp.dtsi +++ b/arch/arm/dts/zynqmp.dtsi @@ -692,6 +692,7 @@ compatible = "ceva,ahci-1v84"; status = "disabled"; reg = <0x0 0xfd0c0000 0x0 0x2000>;
reg-names = "serdes"; interrupt-parent = <&gic>; interrupts = <0 133 4>; #stream-id-cells = <4>;
Unfortunately it is not that simple.
First of all you need to reflect this in dt binding doc in the kernel. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...
Did you record your compatible strings anywhere?
Thanks, Michal
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 17:31 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 11:22, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 16:04 To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; michal.simek@xilinx.com; u-boot@lists.denx.de Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 9:28, Peng Ma wrote:
Distinguish the ecc val by chassis version and move the ecc addr to dts. Add ls1028a soc support.
Signed-off-by: Peng Ma peng.ma@nxp.com
drivers/ata/sata_ceva.c | 43
+++++++++++++++++++++++++------------------
1 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index 8887be9..d26f712 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -88,20 +88,16 @@ #define LS1021_CEVA_PHY4_CFG 0x064a080b #define
LS1021_CEVA_PHY5_CFG
0x2aa86470
-/* for ls1088a */ -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 -#define LS1088_ECC_DIS_VAL_CH2 0x40000000
-/* ecc addr-val pair */ -#define ECC_DIS_ADDR_CH2 0x20140520 +/* ecc val pair */ +#define ECC_DIS_VAL_CH1 0x00020000 #define ECC_DIS_VAL_CH2 0x80000000 -#define SATA_ECC_REG_ADDR 0x20220520 -#define SATA_ECC_DISABLE 0x00020000 +#define ECC_DIS_VAL_CH3 0x40000000
enum ceva_soc { CEVA_1V84, CEVA_LS1012A, CEVA_LS1021A,
CEVA_LS1028A, CEVA_LS1043A, CEVA_LS1046A, CEVA_LS1088A,@@ -110,12 +106,14 @@ enum ceva_soc {
struct ceva_sata_priv { ulong base;
ulong ecc_base; enum ceva_soc soc; ulong flag;};
static int ceva_init_sata(struct ceva_sata_priv *priv) {
ulong ecc_addr = priv->ecc_base; ulong base = priv->base; ulong tmp;@@ -132,38 +130,38 @@ static int ceva_init_sata(struct ceva_sata_priv
*priv)
break; case CEVA_LS1021A:
writel(SATA_ECC_DISABLE, SATA_ECC_REG_ADDR);
if (ecc_addr)writel(ECC_DIS_VAL_CH1, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(LS1021_CEVA_PHY2_CFG, base +AHCI_VEND_PP2C);
writel(LS1021_CEVA_PHY3_CFG, base +AHCI_VEND_PP3C);
writel(LS1021_CEVA_PHY4_CFG, base +AHCI_VEND_PP4C);
writel(LS1021_CEVA_PHY5_CFG, base +AHCI_VEND_PP5C);
writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +LS1021_AHCI_VEND_AXICC);
break; case CEVA_LS1012A: case CEVA_LS1043A: case CEVA_LS1046A:
writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2);/* fallthrough */ case CEVA_LS2080A:
if (ecc_addr)writel(ECC_DIS_VAL_CH2, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +AHCI_VEND_AXICC);
break;
case CEVA_LS1028A: case CEVA_LS1088A:
writel(LS1088_ECC_DIS_VAL_CH2,LS1088_ECC_DIS_ADDR_CH2);
if (ecc_addr)writel(ECC_DIS_VAL_CH3, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +AHCI_VEND_AXICC);
break; }
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC);return 0;}
@@ -187,6 +185,7 @@ static const struct udevice_id sata_ceva_ids[] = { { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A },
{ .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A }, { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A }, @@-205,8 +204,16 @@ static int sata_ceva_ofdata_to_platdata(struct udevice
*dev)
if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);It would be better to do it via reg-name instead of index. But that's up to your binding doc.
[Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file sata node as
fallows?
diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi index dfb6ebc..9ad2d84 100644 --- a/arch/arm/dts/zynqmp.dtsi +++ b/arch/arm/dts/zynqmp.dtsi @@ -692,6 +692,7 @@ compatible = "ceva,ahci-1v84"; status = "disabled"; reg = <0x0 0xfd0c0000 0x0 0x2000>;
reg-names = "serdes"; interrupt-parent = <&gic>; interrupts = <0 133 4>; #stream-id-cells = <4>;Unfortunately it is not that simple.
First of all you need to reflect this in dt binding doc in the kernel. https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1 e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C 0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi Ybybu6i1t%2Byzo%3D&reserved=0
Did you record your compatible strings anywhere?
[Peng Ma] Thanks your quick reply. You can find our binding document at: (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...) I think your suggestion is better. But the first register address was got by function dev_read_addr(), to keep consistency, I still prefer to use dev_read_addr_index(). What do you think?
Thanks, Michal
On 16. 04. 19 11:54, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 17:31 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 11:22, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 16:04 To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; michal.simek@xilinx.com; u-boot@lists.denx.de Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 9:28, Peng Ma wrote:
Distinguish the ecc val by chassis version and move the ecc addr to dts. Add ls1028a soc support.
Signed-off-by: Peng Ma peng.ma@nxp.com
drivers/ata/sata_ceva.c | 43
+++++++++++++++++++++++++------------------
1 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index 8887be9..d26f712 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -88,20 +88,16 @@ #define LS1021_CEVA_PHY4_CFG 0x064a080b #define
LS1021_CEVA_PHY5_CFG
0x2aa86470
-/* for ls1088a */ -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 -#define LS1088_ECC_DIS_VAL_CH2 0x40000000
-/* ecc addr-val pair */ -#define ECC_DIS_ADDR_CH2 0x20140520 +/* ecc val pair */ +#define ECC_DIS_VAL_CH1 0x00020000 #define ECC_DIS_VAL_CH2 0x80000000 -#define SATA_ECC_REG_ADDR 0x20220520 -#define SATA_ECC_DISABLE 0x00020000 +#define ECC_DIS_VAL_CH3 0x40000000
enum ceva_soc { CEVA_1V84, CEVA_LS1012A, CEVA_LS1021A,
CEVA_LS1028A, CEVA_LS1043A, CEVA_LS1046A, CEVA_LS1088A,@@ -110,12 +106,14 @@ enum ceva_soc {
struct ceva_sata_priv { ulong base;
ulong ecc_base; enum ceva_soc soc; ulong flag;};
static int ceva_init_sata(struct ceva_sata_priv *priv) {
ulong ecc_addr = priv->ecc_base; ulong base = priv->base; ulong tmp;@@ -132,38 +130,38 @@ static int ceva_init_sata(struct ceva_sata_priv
*priv)
break; case CEVA_LS1021A:
writel(SATA_ECC_DISABLE, SATA_ECC_REG_ADDR);
if (ecc_addr)writel(ECC_DIS_VAL_CH1, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(LS1021_CEVA_PHY2_CFG, base +AHCI_VEND_PP2C);
writel(LS1021_CEVA_PHY3_CFG, base +AHCI_VEND_PP3C);
writel(LS1021_CEVA_PHY4_CFG, base +AHCI_VEND_PP4C);
writel(LS1021_CEVA_PHY5_CFG, base +AHCI_VEND_PP5C);
writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +LS1021_AHCI_VEND_AXICC);
break; case CEVA_LS1012A: case CEVA_LS1043A: case CEVA_LS1046A:
writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2);/* fallthrough */ case CEVA_LS2080A:
if (ecc_addr)writel(ECC_DIS_VAL_CH2, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +AHCI_VEND_AXICC);
break;
case CEVA_LS1028A: case CEVA_LS1088A:
writel(LS1088_ECC_DIS_VAL_CH2,LS1088_ECC_DIS_ADDR_CH2);
if (ecc_addr)writel(ECC_DIS_VAL_CH3, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +AHCI_VEND_AXICC);
break; }
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC);return 0;}
@@ -187,6 +185,7 @@ static const struct udevice_id sata_ceva_ids[] = { { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A },
{ .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A }, { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A }, @@-205,8 +204,16 @@ static int sata_ceva_ofdata_to_platdata(struct udevice
*dev)
if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);It would be better to do it via reg-name instead of index. But that's up to your binding doc.
[Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file sata node as
fallows?
diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi index dfb6ebc..9ad2d84 100644 --- a/arch/arm/dts/zynqmp.dtsi +++ b/arch/arm/dts/zynqmp.dtsi @@ -692,6 +692,7 @@ compatible = "ceva,ahci-1v84"; status = "disabled"; reg = <0x0 0xfd0c0000 0x0 0x2000>;
reg-names = "serdes"; interrupt-parent = <&gic>; interrupts = <0 133 4>; #stream-id-cells = <4>;Unfortunately it is not that simple.
First of all you need to reflect this in dt binding doc in the kernel. https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1 e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C 0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi Ybybu6i1t%2Byzo%3D&reserved=0
Did you record your compatible strings anywhere?
[Peng Ma] Thanks your quick reply. You can find our binding document at: (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...)
Interesting. Why did you create separate binding doc if you target the same ceva sata IP core? Any reason not to merge it together? Anyway I see that this was added in 2015.
I think your suggestion is better. But the first register cd address was got by function dev_read_addr(), to keep consistency, I still prefer to use dev_read_addr_index(). What do you think?
We didn't have a need to have second address range that's why reg-names property wasn't used. Do they have all ecc addresses?
M
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:01 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 11:54, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 17:31 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 11:22, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 16:04 To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; michal.simek@xilinx.com; u-boot@lists.denx.de Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is
safe.
On 16. 04. 19 9:28, Peng Ma wrote:
Distinguish the ecc val by chassis version and move the ecc addr to dts. Add ls1028a soc support.
Signed-off-by: Peng Ma peng.ma@nxp.com
drivers/ata/sata_ceva.c | 43
+++++++++++++++++++++++++------------------
1 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index 8887be9..d26f712 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -88,20 +88,16 @@ #define LS1021_CEVA_PHY4_CFG 0x064a080b #define
LS1021_CEVA_PHY5_CFG
0x2aa86470
-/* for ls1088a */ -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 -#define LS1088_ECC_DIS_VAL_CH2 0x40000000
-/* ecc addr-val pair */ -#define ECC_DIS_ADDR_CH2 0x20140520 +/* ecc val pair */ +#define ECC_DIS_VAL_CH1 0x00020000 #define ECC_DIS_VAL_CH2 0x80000000 -#define SATA_ECC_REG_ADDR 0x20220520 -#define SATA_ECC_DISABLE 0x00020000 +#define ECC_DIS_VAL_CH3 0x40000000
enum ceva_soc { CEVA_1V84, CEVA_LS1012A, CEVA_LS1021A,
CEVA_LS1028A, CEVA_LS1043A, CEVA_LS1046A, CEVA_LS1088A,@@ -110,12 +106,14 @@ enum ceva_soc {
struct ceva_sata_priv { ulong base;
ulong ecc_base; enum ceva_soc soc; ulong flag;};
static int ceva_init_sata(struct ceva_sata_priv *priv) {
ulong ecc_addr = priv->ecc_base; ulong base = priv->base; ulong tmp;@@ -132,38 +130,38 @@ static int ceva_init_sata(struct ceva_sata_priv
*priv)
break; case CEVA_LS1021A:
writel(SATA_ECC_DISABLE, SATA_ECC_REG_ADDR);
if (ecc_addr)writel(ECC_DIS_VAL_CH1, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(LS1021_CEVA_PHY2_CFG, base +AHCI_VEND_PP2C);
writel(LS1021_CEVA_PHY3_CFG, base +AHCI_VEND_PP3C);
writel(LS1021_CEVA_PHY4_CFG, base +AHCI_VEND_PP4C);
writel(LS1021_CEVA_PHY5_CFG, base +AHCI_VEND_PP5C);
writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +LS1021_AHCI_VEND_AXICC);
break; case CEVA_LS1012A: case CEVA_LS1043A: case CEVA_LS1046A:
writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2);/* fallthrough */ case CEVA_LS2080A:
if (ecc_addr)writel(ECC_DIS_VAL_CH2, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +AHCI_VEND_AXICC);
break;
case CEVA_LS1028A: case CEVA_LS1088A:
writel(LS1088_ECC_DIS_VAL_CH2,LS1088_ECC_DIS_ADDR_CH2);
if (ecc_addr)writel(ECC_DIS_VAL_CH3, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC);
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base +AHCI_VEND_AXICC);
break; }
if (priv->flag & FLAG_COHERENT)writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC);return 0;}
@@ -187,6 +185,7 @@ static const struct udevice_id sata_ceva_ids[] =
{
{ .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A },
{ .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A }, { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A },@@ -205,8 +204,16 @@ static int sata_ceva_ofdata_to_platdata(struct udevice
*dev)
if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);It would be better to do it via reg-name instead of index. But that's up to your binding doc.
[Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file sata node as
fallows?
diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi index dfb6ebc..9ad2d84 100644 --- a/arch/arm/dts/zynqmp.dtsi +++ b/arch/arm/dts/zynqmp.dtsi @@ -692,6 +692,7 @@ compatible = "ceva,ahci-1v84"; status = "disabled"; reg = <0x0 0xfd0c0000 0x0 0x2000>;
reg-names = "serdes"; interrupt-parent = <&gic>; interrupts = <0 133 4>; #stream-id-cells = <4>;Unfortunately it is not that simple.
First of all you need to reflect this in dt binding doc in the kernel. https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit .kern
el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr
ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F
h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1
e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi
Ybybu6i1t%2Byzo%3D&reserved=0
Did you record your compatible strings anywhere?
[Peng Ma] Thanks your quick reply. You can find our binding document at: (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git
%
2Ftree%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-fsl-qoriq.
txt%3Fh%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7C977d 7871292f
48332dbb08d6c2526b32%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C 0%7C6369
10056581930082&sdata=KQryE2RvI8jTOAPKGlGnx2NWYdawFUaTURyzd8 oets4%3
D&reserved=0)
Interesting. Why did you create separate binding doc if you target the same ceva sata IP core? Any reason not to merge it together? Anyway I see that this was added in 2015.
[Peng Ma] Although they are both ceva sata, our sata driver is different from yours in kernel to Initializa Vendor-specific registers, you could see our driver at kernel driver/ata/ ahci_qoriq.c.
I think your suggestion is better. But the first register cd address was got by
function dev_read_addr(), to keep consistency, I still prefer to use dev_read_addr_index(). What do you think?
We didn't have a need to have second address range that's why reg-names property wasn't used. Do they have all ecc addresses?
[Peng Ma] Some of our socs need to fixed sata errata. So we should write ecc addr. I didn't add reg-names to keep consistency of Xilinx sata soc.
M
On 16. 04. 19 12:29, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:01 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 11:54, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 17:31 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 11:22, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 16:04 To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; michal.simek@xilinx.com; u-boot@lists.denx.de Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is
safe.
On 16. 04. 19 9:28, Peng Ma wrote: > Distinguish the ecc val by chassis version and move the ecc addr to dts. > Add ls1028a soc support. > > Signed-off-by: Peng Ma peng.ma@nxp.com > --- > drivers/ata/sata_ceva.c | 43 +++++++++++++++++++++++++------------------ > 1 files changed, 25 insertions(+), 18 deletions(-) > > diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c > index > 8887be9..d26f712 100644 > --- a/drivers/ata/sata_ceva.c > +++ b/drivers/ata/sata_ceva.c > @@ -88,20 +88,16 @@ > #define LS1021_CEVA_PHY4_CFG 0x064a080b #define LS1021_CEVA_PHY5_CFG > 0x2aa86470 > > -/* for ls1088a */ > -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 > -#define LS1088_ECC_DIS_VAL_CH2 0x40000000 > - > -/* ecc addr-val pair */ > -#define ECC_DIS_ADDR_CH2 0x20140520 > +/* ecc val pair */ > +#define ECC_DIS_VAL_CH1 0x00020000 > #define ECC_DIS_VAL_CH2 0x80000000 > -#define SATA_ECC_REG_ADDR 0x20220520 > -#define SATA_ECC_DISABLE 0x00020000 > +#define ECC_DIS_VAL_CH3 0x40000000 > > enum ceva_soc { > CEVA_1V84, > CEVA_LS1012A, > CEVA_LS1021A, > + CEVA_LS1028A, > CEVA_LS1043A, > CEVA_LS1046A, > CEVA_LS1088A, > @@ -110,12 +106,14 @@ enum ceva_soc { > > struct ceva_sata_priv { > ulong base; > + ulong ecc_base; > enum ceva_soc soc; > ulong flag; > }; > > static int ceva_init_sata(struct ceva_sata_priv *priv) { > + ulong ecc_addr = priv->ecc_base; > ulong base = priv->base; > ulong tmp; > > @@ -132,38 +130,38 @@ static int ceva_init_sata(struct > ceva_sata_priv *priv) > break; > > case CEVA_LS1021A: > - writel(SATA_ECC_DISABLE, SATA_ECC_REG_ADDR); > + if (ecc_addr) > + writel(ECC_DIS_VAL_CH1, ecc_addr); > writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); > writel(LS1021_CEVA_PHY2_CFG, base + AHCI_VEND_PP2C); > writel(LS1021_CEVA_PHY3_CFG, base + AHCI_VEND_PP3C); > writel(LS1021_CEVA_PHY4_CFG, base + AHCI_VEND_PP4C); > writel(LS1021_CEVA_PHY5_CFG, base + AHCI_VEND_PP5C); > writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); > - if (priv->flag & FLAG_COHERENT) > - writel(CEVA_AXICC_CFG, base + LS1021_AHCI_VEND_AXICC); > break; > > case CEVA_LS1012A: > case CEVA_LS1043A: > case CEVA_LS1046A: > - writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2); > - /* fallthrough */ > case CEVA_LS2080A: > + if (ecc_addr) > + writel(ECC_DIS_VAL_CH2, ecc_addr); > writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); > writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); > - if (priv->flag & FLAG_COHERENT) > - writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC); > break; > > + case CEVA_LS1028A: > case CEVA_LS1088A: > - writel(LS1088_ECC_DIS_VAL_CH2, LS1088_ECC_DIS_ADDR_CH2); > + if (ecc_addr) > + writel(ECC_DIS_VAL_CH3, ecc_addr); > writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); > writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); > - if (priv->flag & FLAG_COHERENT) > - writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC); > break; > } > > + if (priv->flag & FLAG_COHERENT) > + writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC); > + > return 0; > } > > @@ -187,6 +185,7 @@ static const struct udevice_id sata_ceva_ids[] =
{
> { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, > { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, > { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A }, > + { .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A }, > { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, > { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, > { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A }, > @@ > -205,8 +204,16 @@ static int sata_ceva_ofdata_to_platdata(struct > udevice *dev) > if (priv->base == FDT_ADDR_T_NONE) > return -EINVAL; > > + priv->ecc_base = dev_read_addr_index(dev, 1);
It would be better to do it via reg-name instead of index. But that's up to your binding doc.
[Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file sata node as
fallows?
diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi index dfb6ebc..9ad2d84 100644 --- a/arch/arm/dts/zynqmp.dtsi +++ b/arch/arm/dts/zynqmp.dtsi @@ -692,6 +692,7 @@ compatible = "ceva,ahci-1v84"; status = "disabled"; reg = <0x0 0xfd0c0000 0x0 0x2000>;
reg-names = "serdes"; interrupt-parent = <&gic>; interrupts = <0 133 4>; #stream-id-cells = <4>;Unfortunately it is not that simple.
First of all you need to reflect this in dt binding doc in the kernel. https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit .kern
el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr
ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F
h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1
e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi
Ybybu6i1t%2Byzo%3D&reserved=0
Did you record your compatible strings anywhere?
[Peng Ma] Thanks your quick reply. You can find our binding document at: (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git
%
2Ftree%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-fsl-qoriq.
txt%3Fh%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7C977d 7871292f
48332dbb08d6c2526b32%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C 0%7C6369
10056581930082&sdata=KQryE2RvI8jTOAPKGlGnx2NWYdawFUaTURyzd8 oets4%3
D&reserved=0)
Interesting. Why did you create separate binding doc if you target the same ceva sata IP core? Any reason not to merge it together? Anyway I see that this was added in 2015.
[Peng Ma] Although they are both ceva sata, our sata driver is different from yours in kernel to Initializa Vendor-specific registers, you could see our driver at kernel driver/ata/ ahci_qoriq.c.
I have briefly looked at it. They should be merged together. It happened to us too that we send driver out but didn't know who was the vendor in past and then we found out.
I think your suggestion is better. But the first register cd address was got by
function dev_read_addr(), to keep consistency, I still prefer to use dev_read_addr_index(). What do you think?
We didn't have a need to have second address range that's why reg-names property wasn't used. Do they have all ecc addresses?
[Peng Ma] Some of our socs need to fixed sata errata. So we should write ecc addr. I didn't add reg-names to keep consistency of Xilinx sata soc.
Ok. I think that in this case you should pass different .data to driver.
struct platform_data { enum ceva_soc; bool ecc_present; }
It means have current enum followed by bool with "true" for specific IPs which needs to have also ECC range. Then you can better check that binding is correct.
And use _index there.
Thanks, Michal
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:49 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 12:29, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:01 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 11:54, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 17:31 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is
safe.
On 16. 04. 19 11:22, Peng Ma wrote:
> -----Original Message----- > From: Michal Simek michal.simek@xilinx.com > Sent: 2019年4月16日 16:04 > To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; > sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
> york.sun@nxp.com; Prabhakar Kushwaha
> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
> michal.simek@xilinx.com; u-boot@lists.denx.de > Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver > code > > WARNING: This email was created outside of NXP. DO NOT CLICK > links or attachments unless you recognize the sender and know the > content is
safe.
> > > > On 16. 04. 19 9:28, Peng Ma wrote: >> Distinguish the ecc val by chassis version and move the ecc addr to
dts.
>> Add ls1028a soc support. >> >> Signed-off-by: Peng Ma peng.ma@nxp.com >> --- >> drivers/ata/sata_ceva.c | 43 > +++++++++++++++++++++++++------------------ >> 1 files changed, 25 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c >> index >> 8887be9..d26f712 100644 >> --- a/drivers/ata/sata_ceva.c >> +++ b/drivers/ata/sata_ceva.c >> @@ -88,20 +88,16 @@ >> #define LS1021_CEVA_PHY4_CFG 0x064a080b #define > LS1021_CEVA_PHY5_CFG >> 0x2aa86470 >> >> -/* for ls1088a */ >> -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 >> -#define LS1088_ECC_DIS_VAL_CH2 0x40000000 >> - >> -/* ecc addr-val pair */ >> -#define ECC_DIS_ADDR_CH2 0x20140520 >> +/* ecc val pair */ >> +#define ECC_DIS_VAL_CH1 0x00020000 >> #define ECC_DIS_VAL_CH2 0x80000000 >> -#define SATA_ECC_REG_ADDR 0x20220520 >> -#define SATA_ECC_DISABLE 0x00020000 >> +#define ECC_DIS_VAL_CH3 0x40000000 >> >> enum ceva_soc { >> CEVA_1V84, >> CEVA_LS1012A, >> CEVA_LS1021A, >> + CEVA_LS1028A, >> CEVA_LS1043A, >> CEVA_LS1046A, >> CEVA_LS1088A, >> @@ -110,12 +106,14 @@ enum ceva_soc { >> >> struct ceva_sata_priv { >> ulong base; >> + ulong ecc_base; >> enum ceva_soc soc; >> ulong flag; >> }; >> >> static int ceva_init_sata(struct ceva_sata_priv *priv) { >> + ulong ecc_addr = priv->ecc_base; >> ulong base = priv->base; >> ulong tmp; >> >> @@ -132,38 +130,38 @@ static int ceva_init_sata(struct >> ceva_sata_priv > *priv) >> break; >> >> case CEVA_LS1021A: >> - writel(SATA_ECC_DISABLE, SATA_ECC_REG_ADDR); >> + if (ecc_addr) >> + writel(ECC_DIS_VAL_CH1, ecc_addr); >> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>> writel(LS1021_CEVA_PHY2_CFG, base + > AHCI_VEND_PP2C); >> writel(LS1021_CEVA_PHY3_CFG, base + > AHCI_VEND_PP3C); >> writel(LS1021_CEVA_PHY4_CFG, base + > AHCI_VEND_PP4C); >> writel(LS1021_CEVA_PHY5_CFG, base + > AHCI_VEND_PP5C); >> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>> - if (priv->flag & FLAG_COHERENT) >> - writel(CEVA_AXICC_CFG, base + > LS1021_AHCI_VEND_AXICC); >> break; >> >> case CEVA_LS1012A: >> case CEVA_LS1043A: >> case CEVA_LS1046A: >> - writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2); >> - /* fallthrough */ >> case CEVA_LS2080A: >> + if (ecc_addr) >> + writel(ECC_DIS_VAL_CH2, ecc_addr); >> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>> - if (priv->flag & FLAG_COHERENT) >> - writel(CEVA_AXICC_CFG, base + > AHCI_VEND_AXICC); >> break; >> >> + case CEVA_LS1028A: >> case CEVA_LS1088A: >> - writel(LS1088_ECC_DIS_VAL_CH2, > LS1088_ECC_DIS_ADDR_CH2); >> + if (ecc_addr) >> + writel(ECC_DIS_VAL_CH3, ecc_addr); >> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>> - if (priv->flag & FLAG_COHERENT) >> - writel(CEVA_AXICC_CFG, base + > AHCI_VEND_AXICC); >> break; >> } >> >> + if (priv->flag & FLAG_COHERENT) >> + writel(CEVA_AXICC_CFG, base +
AHCI_VEND_AXICC);
>> + >> return 0; >> } >> >> @@ -187,6 +185,7 @@ static const struct udevice_id >> sata_ceva_ids[] =
{
>> { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, >> { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, >> { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A >> }, >> + { .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A >> + }, >> { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, >> { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, >> { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A >> }, @@ >> -205,8 +204,16 @@ static int sata_ceva_ofdata_to_platdata(struct >> udevice > *dev) >> if (priv->base == FDT_ADDR_T_NONE) >> return -EINVAL; >> >> + priv->ecc_base = dev_read_addr_index(dev, 1); > > It would be better to do it via reg-name instead of index. But > that's up to your binding doc. > [Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file sata node as
fallows?
diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi index dfb6ebc..9ad2d84 100644 --- a/arch/arm/dts/zynqmp.dtsi +++ b/arch/arm/dts/zynqmp.dtsi @@ -692,6 +692,7 @@ compatible = "ceva,ahci-1v84"; status = "disabled"; reg = <0x0 0xfd0c0000 0x0 0x2000>;
reg-names = "serdes"; interrupt-parent = <&gic>; interrupts = <0 133 4>; #stream-id-cells = <4>;Unfortunately it is not that simple.
First of all you need to reflect this in dt binding doc in the kernel. https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg it .kern
el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr
ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F
h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1
e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi
Ybybu6i1t%2Byzo%3D&reserved=0
Did you record your compatible strings anywhere?
[Peng Ma] Thanks your quick reply. You can find our binding document at: (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg it .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.
gi
t
%
2Ftree%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-fsl-qoriq.
txt%3Fh%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7C977d
7871292f
48332dbb08d6c2526b32%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
0%7C6369
10056581930082&sdata=KQryE2RvI8jTOAPKGlGnx2NWYdawFUaTURyzd8
oets4%3
D&reserved=0)
Interesting. Why did you create separate binding doc if you target the same ceva sata IP core? Any reason not to merge it together? Anyway I see that this was added in 2015.
[Peng Ma] Although they are both ceva sata, our sata driver is different from yours in kernel to Initializa Vendor-specific registers, you could
see our driver at kernel driver/ata/ ahci_qoriq.c.
I have briefly looked at it. They should be merged together. It happened to us too that we send driver out but didn't know who was the vendor in past and then we found out.
I think your suggestion is better. But the first register cd address was got by
function dev_read_addr(), to keep consistency, I still prefer to use dev_read_addr_index(). What do you think?
We didn't have a need to have second address range that's why reg-names property wasn't used. Do they have all ecc addresses?
[Peng Ma] Some of our socs need to fixed sata errata. So we should write ecc
addr.
I didn't add reg-names to keep consistency of Xilinx sata soc.
Ok. I think that in this case you should pass different .data to driver.
struct platform_data { enum ceva_soc; bool ecc_present; }
It means have current enum followed by bool with "true" for specific IPs which needs to have also ECC range. Then you can better check that binding is correct.
And use _index there.
[Peng Ma] As far as I am concerned, It is not necessary to do this, I would rather lean on the former way as fallows: diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..a1d452a 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -196,6 +196,10 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev); + const void *blob = gd->fdt_blob; + int node = dev_of_offset(dev); + struct fdt_resource res_regs; + int ret;
if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT; @@ -204,9 +208,13 @@ static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
- priv->ecc_base = dev_read_addr_index(dev, 1); - if (priv->ecc_base == FDT_ADDR_T_NONE) + ret = fdt_get_named_resource(blob, node, "reg", "reg-names", + "ecc-addr", &res_regs); + if (ret) { + debug("Error: can't get ecc addresses(ret = %d)!\n", ret); priv->ecc_base = 0; + } else + priv->ecc_base = res_regs.start;
priv->soc = dev_get_driver_data(dev); What do you think?
Best Regards, Peng
Thanks, Michal
On 17. 04. 19 4:50, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:49 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 12:29, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:01 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 11:54, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 17:31 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is
safe.
On 16. 04. 19 11:22, Peng Ma wrote: > > >> -----Original Message----- >> From: Michal Simek michal.simek@xilinx.com >> Sent: 2019年4月16日 16:04 >> To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; >> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
>> york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com >> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
>> michal.simek@xilinx.com; u-boot@lists.denx.de >> Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver >> code >> >> WARNING: This email was created outside of NXP. DO NOT CLICK >> links or attachments unless you recognize the sender and know the >> content is
safe.
>> >> >> >> On 16. 04. 19 9:28, Peng Ma wrote: >>> Distinguish the ecc val by chassis version and move the ecc addr to
dts.
>>> Add ls1028a soc support. >>> >>> Signed-off-by: Peng Ma peng.ma@nxp.com >>> --- >>> drivers/ata/sata_ceva.c | 43 >> +++++++++++++++++++++++++------------------ >>> 1 files changed, 25 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c >>> index >>> 8887be9..d26f712 100644 >>> --- a/drivers/ata/sata_ceva.c >>> +++ b/drivers/ata/sata_ceva.c >>> @@ -88,20 +88,16 @@ >>> #define LS1021_CEVA_PHY4_CFG 0x064a080b #define >> LS1021_CEVA_PHY5_CFG >>> 0x2aa86470 >>> >>> -/* for ls1088a */ >>> -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 >>> -#define LS1088_ECC_DIS_VAL_CH2 0x40000000 >>> - >>> -/* ecc addr-val pair */ >>> -#define ECC_DIS_ADDR_CH2 0x20140520 >>> +/* ecc val pair */ >>> +#define ECC_DIS_VAL_CH1 0x00020000 >>> #define ECC_DIS_VAL_CH2 0x80000000 >>> -#define SATA_ECC_REG_ADDR 0x20220520 >>> -#define SATA_ECC_DISABLE 0x00020000 >>> +#define ECC_DIS_VAL_CH3 0x40000000 >>> >>> enum ceva_soc { >>> CEVA_1V84, >>> CEVA_LS1012A, >>> CEVA_LS1021A, >>> + CEVA_LS1028A, >>> CEVA_LS1043A, >>> CEVA_LS1046A, >>> CEVA_LS1088A, >>> @@ -110,12 +106,14 @@ enum ceva_soc { >>> >>> struct ceva_sata_priv { >>> ulong base; >>> + ulong ecc_base; >>> enum ceva_soc soc; >>> ulong flag; >>> }; >>> >>> static int ceva_init_sata(struct ceva_sata_priv *priv) { >>> + ulong ecc_addr = priv->ecc_base; >>> ulong base = priv->base; >>> ulong tmp; >>> >>> @@ -132,38 +130,38 @@ static int ceva_init_sata(struct >>> ceva_sata_priv >> *priv) >>> break; >>> >>> case CEVA_LS1021A: >>> - writel(SATA_ECC_DISABLE, SATA_ECC_REG_ADDR); >>> + if (ecc_addr) >>> + writel(ECC_DIS_VAL_CH1, ecc_addr); >>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>> writel(LS1021_CEVA_PHY2_CFG, base + >> AHCI_VEND_PP2C); >>> writel(LS1021_CEVA_PHY3_CFG, base + >> AHCI_VEND_PP3C); >>> writel(LS1021_CEVA_PHY4_CFG, base + >> AHCI_VEND_PP4C); >>> writel(LS1021_CEVA_PHY5_CFG, base + >> AHCI_VEND_PP5C); >>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>> - if (priv->flag & FLAG_COHERENT) >>> - writel(CEVA_AXICC_CFG, base + >> LS1021_AHCI_VEND_AXICC); >>> break; >>> >>> case CEVA_LS1012A: >>> case CEVA_LS1043A: >>> case CEVA_LS1046A: >>> - writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2); >>> - /* fallthrough */ >>> case CEVA_LS2080A: >>> + if (ecc_addr) >>> + writel(ECC_DIS_VAL_CH2, ecc_addr); >>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>> - if (priv->flag & FLAG_COHERENT) >>> - writel(CEVA_AXICC_CFG, base + >> AHCI_VEND_AXICC); >>> break; >>> >>> + case CEVA_LS1028A: >>> case CEVA_LS1088A: >>> - writel(LS1088_ECC_DIS_VAL_CH2, >> LS1088_ECC_DIS_ADDR_CH2); >>> + if (ecc_addr) >>> + writel(ECC_DIS_VAL_CH3, ecc_addr); >>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>> - if (priv->flag & FLAG_COHERENT) >>> - writel(CEVA_AXICC_CFG, base + >> AHCI_VEND_AXICC); >>> break; >>> } >>> >>> + if (priv->flag & FLAG_COHERENT) >>> + writel(CEVA_AXICC_CFG, base +
AHCI_VEND_AXICC);
>>> + >>> return 0; >>> } >>> >>> @@ -187,6 +185,7 @@ static const struct udevice_id >>> sata_ceva_ids[] =
{
>>> { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, >>> { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, >>> { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A >>> }, >>> + { .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A >>> + }, >>> { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, >>> { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, >>> { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A >>> }, @@ >>> -205,8 +204,16 @@ static int sata_ceva_ofdata_to_platdata(struct >>> udevice >> *dev) >>> if (priv->base == FDT_ADDR_T_NONE) >>> return -EINVAL; >>> >>> + priv->ecc_base = dev_read_addr_index(dev, 1); >> >> It would be better to do it via reg-name instead of index. But >> that's up to your binding doc. >> > [Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file sata > node as fallows? > > diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi > index > dfb6ebc..9ad2d84 100644 > --- a/arch/arm/dts/zynqmp.dtsi > +++ b/arch/arm/dts/zynqmp.dtsi > @@ -692,6 +692,7 @@ > compatible = "ceva,ahci-1v84"; > status = "disabled"; > reg = <0x0 0xfd0c0000 0x0 0x2000>; > + reg-names = "serdes"; > interrupt-parent = <&gic>; > interrupts = <0 133 4>; > #stream-id-cells = <4>;
Unfortunately it is not that simple.
First of all you need to reflect this in dt binding doc in the kernel. https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg it .kern
el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr
ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F
h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1
e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi
Ybybu6i1t%2Byzo%3D&reserved=0
Did you record your compatible strings anywhere?
[Peng Ma] Thanks your quick reply. You can find our binding document at: (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg it .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.
gi
t
%
2Ftree%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-fsl-qoriq.
txt%3Fh%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7C977d
7871292f
48332dbb08d6c2526b32%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
0%7C6369
10056581930082&sdata=KQryE2RvI8jTOAPKGlGnx2NWYdawFUaTURyzd8
oets4%3
D&reserved=0)
Interesting. Why did you create separate binding doc if you target the same ceva sata IP core? Any reason not to merge it together? Anyway I see that this was added in 2015.
[Peng Ma] Although they are both ceva sata, our sata driver is different from yours in kernel to Initializa Vendor-specific registers, you could
see our driver at kernel driver/ata/ ahci_qoriq.c.
I have briefly looked at it. They should be merged together. It happened to us too that we send driver out but didn't know who was the vendor in past and then we found out.
I think your suggestion is better. But the first register cd address was got by
function dev_read_addr(), to keep consistency, I still prefer to use dev_read_addr_index(). What do you think?
We didn't have a need to have second address range that's why reg-names property wasn't used. Do they have all ecc addresses?
[Peng Ma] Some of our socs need to fixed sata errata. So we should write ecc
addr.
I didn't add reg-names to keep consistency of Xilinx sata soc.
Ok. I think that in this case you should pass different .data to driver.
struct platform_data { enum ceva_soc; bool ecc_present; }
It means have current enum followed by bool with "true" for specific IPs which needs to have also ECC range. Then you can better check that binding is correct.
And use _index there.
[Peng Ma] As far as I am concerned, It is not necessary to do this, I would rather lean on the former way as fallows: diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..a1d452a 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -196,6 +196,10 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev);
const void *blob = gd->fdt_blob;
gd is suggesting that you use incorrect functions. We are trying to get rid of gd from device drivers. Please take a look at live tree functions if there is any alternative there.
int node = dev_of_offset(dev);struct fdt_resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT;@@ -204,9 +208,13 @@ static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = fdt_get_named_resource(blob, node, "reg", "reg-names","ecc-addr", &res_regs);if (ret) {debug("Error: can't get ecc addresses(ret = %d)!\n", ret); priv->ecc_base = 0;} elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev);What do you think?
One thing I would like to avoid is that we will get any error even in debug for IPs which don't have this ecc space. And also that we will get this errors for IPs which should have this ecc space.
Thanks, Michal
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月17日 13:58 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 17. 04. 19 4:50, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:49 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 12:29, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:01 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is
safe.
On 16. 04. 19 11:54, Peng Ma wrote:
> -----Original Message----- > From: Michal Simek michal.simek@xilinx.com > Sent: 2019年4月16日 17:31 > To: Peng Ma peng.ma@nxp.com; Michal Simek > michal.simek@xilinx.com; albert.u.boot@aribaud.net; > sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
> york.sun@nxp.com; Prabhakar Kushwaha
> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
> u-boot@lists.denx.de > Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the > driver code > > WARNING: This email was created outside of NXP. DO NOT CLICK > links or attachments unless you recognize the sender and know the > content is
safe.
> > > > On 16. 04. 19 11:22, Peng Ma wrote: >> >> >>> -----Original Message----- >>> From: Michal Simek michal.simek@xilinx.com >>> Sent: 2019年4月16日 16:04 >>> To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; >>> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
>>> york.sun@nxp.com; Prabhakar Kushwaha > prabhakar.kushwaha@nxp.com >>> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
>>> michal.simek@xilinx.com; u-boot@lists.denx.de >>> Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver >>> code >>> >>> WARNING: This email was created outside of NXP. DO NOT CLICK >>> links or attachments unless you recognize the sender and know >>> the content is
safe.
>>> >>> >>> >>> On 16. 04. 19 9:28, Peng Ma wrote: >>>> Distinguish the ecc val by chassis version and move the ecc >>>> addr to
dts.
>>>> Add ls1028a soc support. >>>> >>>> Signed-off-by: Peng Ma peng.ma@nxp.com >>>> --- >>>> drivers/ata/sata_ceva.c | 43 >>> +++++++++++++++++++++++++------------------ >>>> 1 files changed, 25 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c >>>> index >>>> 8887be9..d26f712 100644 >>>> --- a/drivers/ata/sata_ceva.c >>>> +++ b/drivers/ata/sata_ceva.c >>>> @@ -88,20 +88,16 @@ >>>> #define LS1021_CEVA_PHY4_CFG 0x064a080b #define >>> LS1021_CEVA_PHY5_CFG >>>> 0x2aa86470 >>>> >>>> -/* for ls1088a */ >>>> -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 >>>> -#define LS1088_ECC_DIS_VAL_CH2 0x40000000 >>>> - >>>> -/* ecc addr-val pair */ >>>> -#define ECC_DIS_ADDR_CH2 0x20140520 >>>> +/* ecc val pair */ >>>> +#define ECC_DIS_VAL_CH1 0x00020000 >>>> #define ECC_DIS_VAL_CH2 0x80000000 >>>> -#define SATA_ECC_REG_ADDR 0x20220520 >>>> -#define SATA_ECC_DISABLE 0x00020000 >>>> +#define ECC_DIS_VAL_CH3 0x40000000 >>>> >>>> enum ceva_soc { >>>> CEVA_1V84, >>>> CEVA_LS1012A, >>>> CEVA_LS1021A, >>>> + CEVA_LS1028A, >>>> CEVA_LS1043A, >>>> CEVA_LS1046A, >>>> CEVA_LS1088A, >>>> @@ -110,12 +106,14 @@ enum ceva_soc { >>>> >>>> struct ceva_sata_priv { >>>> ulong base; >>>> + ulong ecc_base; >>>> enum ceva_soc soc; >>>> ulong flag; >>>> }; >>>> >>>> static int ceva_init_sata(struct ceva_sata_priv *priv) { >>>> + ulong ecc_addr = priv->ecc_base; >>>> ulong base = priv->base; >>>> ulong tmp; >>>> >>>> @@ -132,38 +130,38 @@ static int ceva_init_sata(struct >>>> ceva_sata_priv >>> *priv) >>>> break; >>>> >>>> case CEVA_LS1021A: >>>> - writel(SATA_ECC_DISABLE,
SATA_ECC_REG_ADDR);
>>>> + if (ecc_addr) >>>> + writel(ECC_DIS_VAL_CH1, ecc_addr); >>>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>>> writel(LS1021_CEVA_PHY2_CFG, base + >>> AHCI_VEND_PP2C); >>>> writel(LS1021_CEVA_PHY3_CFG, base + >>> AHCI_VEND_PP3C); >>>> writel(LS1021_CEVA_PHY4_CFG, base + >>> AHCI_VEND_PP4C); >>>> writel(LS1021_CEVA_PHY5_CFG, base + >>> AHCI_VEND_PP5C); >>>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>>> - if (priv->flag & FLAG_COHERENT) >>>> - writel(CEVA_AXICC_CFG, base + >>> LS1021_AHCI_VEND_AXICC); >>>> break; >>>> >>>> case CEVA_LS1012A: >>>> case CEVA_LS1043A: >>>> case CEVA_LS1046A: >>>> - writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2); >>>> - /* fallthrough */ >>>> case CEVA_LS2080A: >>>> + if (ecc_addr) >>>> + writel(ECC_DIS_VAL_CH2, ecc_addr); >>>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>>> - if (priv->flag & FLAG_COHERENT) >>>> - writel(CEVA_AXICC_CFG, base + >>> AHCI_VEND_AXICC); >>>> break; >>>> >>>> + case CEVA_LS1028A: >>>> case CEVA_LS1088A: >>>> - writel(LS1088_ECC_DIS_VAL_CH2, >>> LS1088_ECC_DIS_ADDR_CH2); >>>> + if (ecc_addr) >>>> + writel(ECC_DIS_VAL_CH3, ecc_addr); >>>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>>> - if (priv->flag & FLAG_COHERENT) >>>> - writel(CEVA_AXICC_CFG, base + >>> AHCI_VEND_AXICC); >>>> break; >>>> } >>>> >>>> + if (priv->flag & FLAG_COHERENT) >>>> + writel(CEVA_AXICC_CFG, base +
AHCI_VEND_AXICC);
>>>> + >>>> return 0; >>>> } >>>> >>>> @@ -187,6 +185,7 @@ static const struct udevice_id >>>> sata_ceva_ids[] =
{
>>>> { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, >>>> { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, >>>> { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A >>>> }, >>>> + { .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A >>>> + }, >>>> { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, >>>> { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, >>>> { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A >>>> }, @@ >>>> -205,8 +204,16 @@ static int >>>> sata_ceva_ofdata_to_platdata(struct >>>> udevice >>> *dev) >>>> if (priv->base == FDT_ADDR_T_NONE) >>>> return -EINVAL; >>>> >>>> + priv->ecc_base = dev_read_addr_index(dev, 1); >>> >>> It would be better to do it via reg-name instead of index. But >>> that's up to your binding doc. >>> >> [Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file sata >> node as > fallows? >> >> diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi >> index >> dfb6ebc..9ad2d84 100644 >> --- a/arch/arm/dts/zynqmp.dtsi >> +++ b/arch/arm/dts/zynqmp.dtsi >> @@ -692,6 +692,7 @@ >> compatible = "ceva,ahci-1v84"; >> status = "disabled"; >> reg = <0x0 0xfd0c0000 0x0 0x2000>; >> + reg-names = "serdes"; >> interrupt-parent = <&gic>; >> interrupts = <0 133 4>; >> #stream-id-cells = <4>; > > Unfortunately it is not that simple. > > First of all you need to reflect this in dt binding doc in the kernel. > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2 > Fg > it > .kern >
el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr
>
ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F
>
h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1
>
e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>
0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi
> Ybybu6i1t%2Byzo%3D&reserved=0 > > Did you record your compatible strings anywhere? > [Peng Ma] Thanks your quick reply. You can find our binding document at: (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2 Fg it .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flin
ux.
gi
t
%
2Ftree%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-fsl-qoriq.
txt%3Fh%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7C977d
7871292f
48332dbb08d6c2526b32%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
0%7C6369
10056581930082&sdata=KQryE2RvI8jTOAPKGlGnx2NWYdawFUaTURyzd8
oets4%3
D&reserved=0)
Interesting. Why did you create separate binding doc if you target the same ceva sata IP core? Any reason not to merge it together? Anyway I see that this was added in 2015.
[Peng Ma] Although they are both ceva sata, our sata driver is different from yours in kernel to Initializa Vendor-specific registers, you could
see our driver at kernel driver/ata/ ahci_qoriq.c.
I have briefly looked at it. They should be merged together. It happened to us too that we send driver out but didn't know who was the vendor in past and then we found out.
I think your suggestion is better. But the first register cd address was got by
function dev_read_addr(), to keep consistency, I still prefer to use dev_read_addr_index(). What do you think?
We didn't have a need to have second address range that's why reg-names property wasn't used. Do they have all ecc addresses?
[Peng Ma] Some of our socs need to fixed sata errata. So we should write ecc
addr.
I didn't add reg-names to keep consistency of Xilinx sata soc.
Ok. I think that in this case you should pass different .data to driver.
struct platform_data { enum ceva_soc; bool ecc_present; }
It means have current enum followed by bool with "true" for specific IPs which needs to have also ECC range. Then you can better check that binding is correct.
And use _index there.
[Peng Ma] As far as I am concerned, It is not necessary to do this, I would rather lean on the former way as fallows: diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..a1d452a 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -196,6 +196,10 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev);
const void *blob = gd->fdt_blob;gd is suggesting that you use incorrect functions. We are trying to get rid of gd from device drivers. Please take a look at live tree functions if there is any alternative there.
int node = dev_of_offset(dev);struct fdt_resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT;@@ -204,9 +208,13 @@ static int sata_ceva_ofdata_to_platdata(struct
udevice *dev)
if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = fdt_get_named_resource(blob, node, "reg", "reg-names","ecc-addr", &res_regs);if (ret) {debug("Error: can't get ecc addresses(ret = %d)!\n", ret); priv->ecc_base = 0;} elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev);What do you think?
One thing I would like to avoid is that we will get any error even in debug for IPs which don't have this ecc space. And also that we will get this errors for IPs which should have this ecc space.
[Peng Ma] OK，changed as fallows:
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..bd98d85 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -8,6 +8,7 @@ #include <ahci.h> #include <scsi.h> #include <asm/io.h> +#include <linux/ioport.h>
/* Vendor Specific Register Offsets */ #define AHCI_VEND_PCFG 0xA4 @@ -196,6 +197,8 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev); + struct resource res_regs; + int ret;
if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT; @@ -204,9 +207,11 @@ static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
- priv->ecc_base = dev_read_addr_index(dev, 1); - if (priv->ecc_base == FDT_ADDR_T_NONE) + ret = dev_read_resource_byname(dev, "ecc-addr", &res_regs); + if (ret) priv->ecc_base = 0; + else + priv->ecc_base = res_regs.start;
priv->soc = dev_get_driver_data(dev);
Best Regards, Peng
Thanks, Michal
On 17. 04. 19 8:50, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月17日 13:58 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 17. 04. 19 4:50, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:49 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 16. 04. 19 12:29, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:01 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is
safe.
On 16. 04. 19 11:54, Peng Ma wrote: > > >> -----Original Message----- >> From: Michal Simek michal.simek@xilinx.com >> Sent: 2019年4月16日 17:31 >> To: Peng Ma peng.ma@nxp.com; Michal Simek >> michal.simek@xilinx.com; albert.u.boot@aribaud.net; >> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
>> york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com >> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
>> u-boot@lists.denx.de >> Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the >> driver code >> >> WARNING: This email was created outside of NXP. DO NOT CLICK >> links or attachments unless you recognize the sender and know the >> content is
safe.
>> >> >> >> On 16. 04. 19 11:22, Peng Ma wrote: >>> >>> >>>> -----Original Message----- >>>> From: Michal Simek michal.simek@xilinx.com >>>> Sent: 2019年4月16日 16:04 >>>> To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; >>>> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
>>>> york.sun@nxp.com; Prabhakar Kushwaha >> prabhakar.kushwaha@nxp.com >>>> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
>>>> michal.simek@xilinx.com; u-boot@lists.denx.de >>>> Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver >>>> code >>>> >>>> WARNING: This email was created outside of NXP. DO NOT CLICK >>>> links or attachments unless you recognize the sender and know >>>> the content is safe. >>>> >>>> >>>> >>>> On 16. 04. 19 9:28, Peng Ma wrote: >>>>> Distinguish the ecc val by chassis version and move the ecc >>>>> addr to
dts.
>>>>> Add ls1028a soc support. >>>>> >>>>> Signed-off-by: Peng Ma peng.ma@nxp.com >>>>> --- >>>>> drivers/ata/sata_ceva.c | 43 >>>> +++++++++++++++++++++++++------------------ >>>>> 1 files changed, 25 insertions(+), 18 deletions(-) >>>>> >>>>> diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c >>>>> index >>>>> 8887be9..d26f712 100644 >>>>> --- a/drivers/ata/sata_ceva.c >>>>> +++ b/drivers/ata/sata_ceva.c >>>>> @@ -88,20 +88,16 @@ >>>>> #define LS1021_CEVA_PHY4_CFG 0x064a080b #define >>>> LS1021_CEVA_PHY5_CFG >>>>> 0x2aa86470 >>>>> >>>>> -/* for ls1088a */ >>>>> -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 >>>>> -#define LS1088_ECC_DIS_VAL_CH2 0x40000000 >>>>> - >>>>> -/* ecc addr-val pair */ >>>>> -#define ECC_DIS_ADDR_CH2 0x20140520 >>>>> +/* ecc val pair */ >>>>> +#define ECC_DIS_VAL_CH1 0x00020000 >>>>> #define ECC_DIS_VAL_CH2 0x80000000 >>>>> -#define SATA_ECC_REG_ADDR 0x20220520 >>>>> -#define SATA_ECC_DISABLE 0x00020000 >>>>> +#define ECC_DIS_VAL_CH3 0x40000000 >>>>> >>>>> enum ceva_soc { >>>>> CEVA_1V84, >>>>> CEVA_LS1012A, >>>>> CEVA_LS1021A, >>>>> + CEVA_LS1028A, >>>>> CEVA_LS1043A, >>>>> CEVA_LS1046A, >>>>> CEVA_LS1088A, >>>>> @@ -110,12 +106,14 @@ enum ceva_soc { >>>>> >>>>> struct ceva_sata_priv { >>>>> ulong base; >>>>> + ulong ecc_base; >>>>> enum ceva_soc soc; >>>>> ulong flag; >>>>> }; >>>>> >>>>> static int ceva_init_sata(struct ceva_sata_priv *priv) { >>>>> + ulong ecc_addr = priv->ecc_base; >>>>> ulong base = priv->base; >>>>> ulong tmp; >>>>> >>>>> @@ -132,38 +130,38 @@ static int ceva_init_sata(struct >>>>> ceva_sata_priv >>>> *priv) >>>>> break; >>>>> >>>>> case CEVA_LS1021A: >>>>> - writel(SATA_ECC_DISABLE,
SATA_ECC_REG_ADDR);
>>>>> + if (ecc_addr) >>>>> + writel(ECC_DIS_VAL_CH1, ecc_addr); >>>>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>>>> writel(LS1021_CEVA_PHY2_CFG, base + >>>> AHCI_VEND_PP2C); >>>>> writel(LS1021_CEVA_PHY3_CFG, base + >>>> AHCI_VEND_PP3C); >>>>> writel(LS1021_CEVA_PHY4_CFG, base + >>>> AHCI_VEND_PP4C); >>>>> writel(LS1021_CEVA_PHY5_CFG, base + >>>> AHCI_VEND_PP5C); >>>>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>>>> - if (priv->flag & FLAG_COHERENT) >>>>> - writel(CEVA_AXICC_CFG, base + >>>> LS1021_AHCI_VEND_AXICC); >>>>> break; >>>>> >>>>> case CEVA_LS1012A: >>>>> case CEVA_LS1043A: >>>>> case CEVA_LS1046A: >>>>> - writel(ECC_DIS_VAL_CH2, ECC_DIS_ADDR_CH2); >>>>> - /* fallthrough */ >>>>> case CEVA_LS2080A: >>>>> + if (ecc_addr) >>>>> + writel(ECC_DIS_VAL_CH2, ecc_addr); >>>>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>>>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>>>> - if (priv->flag & FLAG_COHERENT) >>>>> - writel(CEVA_AXICC_CFG, base + >>>> AHCI_VEND_AXICC); >>>>> break; >>>>> >>>>> + case CEVA_LS1028A: >>>>> case CEVA_LS1088A: >>>>> - writel(LS1088_ECC_DIS_VAL_CH2, >>>> LS1088_ECC_DIS_ADDR_CH2); >>>>> + if (ecc_addr) >>>>> + writel(ECC_DIS_VAL_CH3, ecc_addr); >>>>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>>>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>>>> - if (priv->flag & FLAG_COHERENT) >>>>> - writel(CEVA_AXICC_CFG, base + >>>> AHCI_VEND_AXICC); >>>>> break; >>>>> } >>>>> >>>>> + if (priv->flag & FLAG_COHERENT) >>>>> + writel(CEVA_AXICC_CFG, base +
AHCI_VEND_AXICC);
>>>>> + >>>>> return 0; >>>>> } >>>>> >>>>> @@ -187,6 +185,7 @@ static const struct udevice_id >>>>> sata_ceva_ids[] = { >>>>> { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, >>>>> { .compatible = "fsl,ls1012a-ahci", .data = CEVA_LS1012A }, >>>>> { .compatible = "fsl,ls1021a-ahci", .data = CEVA_LS1021A >>>>> }, >>>>> + { .compatible = "fsl,ls1028a-ahci", .data = CEVA_LS1028A >>>>> + }, >>>>> { .compatible = "fsl,ls1043a-ahci", .data = CEVA_LS1043A }, >>>>> { .compatible = "fsl,ls1046a-ahci", .data = CEVA_LS1046A }, >>>>> { .compatible = "fsl,ls1088a-ahci", .data = CEVA_LS1088A >>>>> }, @@ >>>>> -205,8 +204,16 @@ static int >>>>> sata_ceva_ofdata_to_platdata(struct >>>>> udevice >>>> *dev) >>>>> if (priv->base == FDT_ADDR_T_NONE) >>>>> return -EINVAL; >>>>> >>>>> + priv->ecc_base = dev_read_addr_index(dev, 1); >>>> >>>> It would be better to do it via reg-name instead of index. But >>>> that's up to your binding doc. >>>> >>> [Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file sata >>> node as >> fallows? >>> >>> diff --git a/arch/arm/dts/zynqmp.dtsi b/arch/arm/dts/zynqmp.dtsi >>> index >>> dfb6ebc..9ad2d84 100644 >>> --- a/arch/arm/dts/zynqmp.dtsi >>> +++ b/arch/arm/dts/zynqmp.dtsi >>> @@ -692,6 +692,7 @@ >>> compatible = "ceva,ahci-1v84"; >>> status = "disabled"; >>> reg = <0x0 0xfd0c0000 0x0 0x2000>; >>> + reg-names = "serdes"; >>> interrupt-parent = <&gic>; >>> interrupts = <0 133 4>; >>> #stream-id-cells = <4>; >> >> Unfortunately it is not that simple. >> >> First of all you need to reflect this in dt binding doc in the kernel. >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2 >> Fg >> it >> .kern >>
el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr
>>
ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F
>>
h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1
>>
e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>>
0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi
>> Ybybu6i1t%2Byzo%3D&reserved=0 >> >> Did you record your compatible strings anywhere? >> > [Peng Ma] Thanks your quick reply. > You can find our binding document at: > (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2 > Fg > it > .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flin
ux.
gi
> t % >
2Ftree%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-fsl-qoriq.
>
txt%3Fh%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7C977d
7871292f >
48332dbb08d6c2526b32%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
0%7C6369 >
10056581930082&sdata=KQryE2RvI8jTOAPKGlGnx2NWYdawFUaTURyzd8
oets4%3 > D&reserved=0)
Interesting. Why did you create separate binding doc if you target the same ceva sata IP core? Any reason not to merge it together? Anyway I see that this was added in 2015.
[Peng Ma] Although they are both ceva sata, our sata driver is different from yours in kernel to Initializa Vendor-specific registers, you could
see our driver at kernel driver/ata/ ahci_qoriq.c.
I have briefly looked at it. They should be merged together. It happened to us too that we send driver out but didn't know who was the vendor in past and then we found out.
> I think your suggestion is better. But the first register cd > address was got by function dev_read_addr(), to keep consistency, I still prefer to use dev_read_addr_index(). What do you think?
We didn't have a need to have second address range that's why reg-names property wasn't used. Do they have all ecc addresses?
[Peng Ma] Some of our socs need to fixed sata errata. So we should write ecc
addr.
I didn't add reg-names to keep consistency of Xilinx sata soc.
Ok. I think that in this case you should pass different .data to driver.
struct platform_data { enum ceva_soc; bool ecc_present; }
It means have current enum followed by bool with "true" for specific IPs which needs to have also ECC range. Then you can better check that binding is correct.
And use _index there.
[Peng Ma] As far as I am concerned, It is not necessary to do this, I would rather lean on the former way as fallows: diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..a1d452a 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -196,6 +196,10 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev);
const void *blob = gd->fdt_blob;gd is suggesting that you use incorrect functions. We are trying to get rid of gd from device drivers. Please take a look at live tree functions if there is any alternative there.
int node = dev_of_offset(dev);struct fdt_resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT;@@ -204,9 +208,13 @@ static int sata_ceva_ofdata_to_platdata(struct
udevice *dev)
if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = fdt_get_named_resource(blob, node, "reg", "reg-names","ecc-addr", &res_regs);if (ret) {debug("Error: can't get ecc addresses(ret = %d)!\n", ret); priv->ecc_base = 0;} elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev);What do you think?
One thing I would like to avoid is that we will get any error even in debug for IPs which don't have this ecc space. And also that we will get this errors for IPs which should have this ecc space.
[Peng Ma] OK，changed as fallows:
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..bd98d85 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -8,6 +8,7 @@ #include <ahci.h> #include <scsi.h> #include <asm/io.h> +#include <linux/ioport.h>
/* Vendor Specific Register Offsets */ #define AHCI_VEND_PCFG 0xA4 @@ -196,6 +197,8 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev);
struct resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT;@@ -204,9 +207,11 @@ static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = dev_read_resource_byname(dev, "ecc-addr", &res_regs);if (ret) priv->ecc_base = 0;elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev);
This looks good but still missing information that in your case there are some versions where this ecc-addr is required. It means if it is required you should error it out when it is not present in DT.
Thanks, Michal
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月17日 14:58 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 17. 04. 19 8:50, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月17日 13:58 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 17. 04. 19 4:50, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:49 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is
safe.
On 16. 04. 19 12:29, Peng Ma wrote:
> -----Original Message----- > From: Michal Simek michal.simek@xilinx.com > Sent: 2019年4月16日 18:01 > To: Peng Ma peng.ma@nxp.com; Michal Simek > michal.simek@xilinx.com; albert.u.boot@aribaud.net; > sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
> york.sun@nxp.com; Prabhakar Kushwaha
> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
> u-boot@lists.denx.de > Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the > driver code > > WARNING: This email was created outside of NXP. DO NOT CLICK > links or attachments unless you recognize the sender and know the > content is
safe.
> > > > On 16. 04. 19 11:54, Peng Ma wrote: >> >> >>> -----Original Message----- >>> From: Michal Simek michal.simek@xilinx.com >>> Sent: 2019年4月16日 17:31 >>> To: Peng Ma peng.ma@nxp.com; Michal Simek >>> michal.simek@xilinx.com; albert.u.boot@aribaud.net; >>> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
>>> york.sun@nxp.com; Prabhakar Kushwaha > prabhakar.kushwaha@nxp.com >>> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
>>> u-boot@lists.denx.de >>> Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the >>> driver code >>> >>> WARNING: This email was created outside of NXP. DO NOT CLICK >>> links or attachments unless you recognize the sender and know >>> the content is
safe.
>>> >>> >>> >>> On 16. 04. 19 11:22, Peng Ma wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Michal Simek michal.simek@xilinx.com >>>>> Sent: 2019年4月16日 16:04 >>>>> To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; >>>>> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com;
York
Sun
>>>>> york.sun@nxp.com; Prabhakar Kushwaha >>> prabhakar.kushwaha@nxp.com >>>>> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
>>>>> michal.simek@xilinx.com; u-boot@lists.denx.de >>>>> Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the >>>>> driver code >>>>> >>>>> WARNING: This email was created outside of NXP. DO NOT CLICK >>>>> links or attachments unless you recognize the sender and know >>>>> the content is > safe. >>>>> >>>>> >>>>> >>>>> On 16. 04. 19 9:28, Peng Ma wrote: >>>>>> Distinguish the ecc val by chassis version and move the ecc >>>>>> addr to
dts.
>>>>>> Add ls1028a soc support. >>>>>> >>>>>> Signed-off-by: Peng Ma peng.ma@nxp.com >>>>>> --- >>>>>> drivers/ata/sata_ceva.c | 43 >>>>> +++++++++++++++++++++++++------------------ >>>>>> 1 files changed, 25 insertions(+), 18 deletions(-) >>>>>> >>>>>> diff --git a/drivers/ata/sata_ceva.c >>>>>> b/drivers/ata/sata_ceva.c index >>>>>> 8887be9..d26f712 100644 >>>>>> --- a/drivers/ata/sata_ceva.c >>>>>> +++ b/drivers/ata/sata_ceva.c >>>>>> @@ -88,20 +88,16 @@ >>>>>> #define LS1021_CEVA_PHY4_CFG 0x064a080b #define >>>>> LS1021_CEVA_PHY5_CFG >>>>>> 0x2aa86470 >>>>>> >>>>>> -/* for ls1088a */ >>>>>> -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 >>>>>> -#define LS1088_ECC_DIS_VAL_CH2 0x40000000 >>>>>> - >>>>>> -/* ecc addr-val pair */ >>>>>> -#define ECC_DIS_ADDR_CH2 0x20140520 >>>>>> +/* ecc val pair */ >>>>>> +#define ECC_DIS_VAL_CH1 0x00020000 >>>>>> #define ECC_DIS_VAL_CH2 0x80000000 >>>>>> -#define SATA_ECC_REG_ADDR 0x20220520 >>>>>> -#define SATA_ECC_DISABLE 0x00020000 >>>>>> +#define ECC_DIS_VAL_CH3 0x40000000 >>>>>> >>>>>> enum ceva_soc { >>>>>> CEVA_1V84, >>>>>> CEVA_LS1012A, >>>>>> CEVA_LS1021A, >>>>>> + CEVA_LS1028A, >>>>>> CEVA_LS1043A, >>>>>> CEVA_LS1046A, >>>>>> CEVA_LS1088A, >>>>>> @@ -110,12 +106,14 @@ enum ceva_soc { >>>>>> >>>>>> struct ceva_sata_priv { >>>>>> ulong base; >>>>>> + ulong ecc_base; >>>>>> enum ceva_soc soc; >>>>>> ulong flag; >>>>>> }; >>>>>> >>>>>> static int ceva_init_sata(struct ceva_sata_priv *priv) { >>>>>> + ulong ecc_addr = priv->ecc_base; >>>>>> ulong base = priv->base; >>>>>> ulong tmp; >>>>>> >>>>>> @@ -132,38 +130,38 @@ static int ceva_init_sata(struct >>>>>> ceva_sata_priv >>>>> *priv) >>>>>> break; >>>>>> >>>>>> case CEVA_LS1021A: >>>>>> - writel(SATA_ECC_DISABLE,
SATA_ECC_REG_ADDR);
>>>>>> + if (ecc_addr) >>>>>> + writel(ECC_DIS_VAL_CH1,
ecc_addr);
>>>>>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>>>>> writel(LS1021_CEVA_PHY2_CFG, base + >>>>> AHCI_VEND_PP2C); >>>>>> writel(LS1021_CEVA_PHY3_CFG, base + >>>>> AHCI_VEND_PP3C); >>>>>> writel(LS1021_CEVA_PHY4_CFG, base + >>>>> AHCI_VEND_PP4C); >>>>>> writel(LS1021_CEVA_PHY5_CFG, base + >>>>> AHCI_VEND_PP5C); >>>>>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>>>>> - if (priv->flag & FLAG_COHERENT) >>>>>> - writel(CEVA_AXICC_CFG, base + >>>>> LS1021_AHCI_VEND_AXICC); >>>>>> break; >>>>>> >>>>>> case CEVA_LS1012A: >>>>>> case CEVA_LS1043A: >>>>>> case CEVA_LS1046A: >>>>>> - writel(ECC_DIS_VAL_CH2,
ECC_DIS_ADDR_CH2);
>>>>>> - /* fallthrough */ >>>>>> case CEVA_LS2080A: >>>>>> + if (ecc_addr) >>>>>> + writel(ECC_DIS_VAL_CH2,
ecc_addr);
>>>>>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>>>>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>>>>> - if (priv->flag & FLAG_COHERENT) >>>>>> - writel(CEVA_AXICC_CFG, base + >>>>> AHCI_VEND_AXICC); >>>>>> break; >>>>>> >>>>>> + case CEVA_LS1028A: >>>>>> case CEVA_LS1088A: >>>>>> - writel(LS1088_ECC_DIS_VAL_CH2, >>>>> LS1088_ECC_DIS_ADDR_CH2); >>>>>> + if (ecc_addr) >>>>>> + writel(ECC_DIS_VAL_CH3,
ecc_addr);
>>>>>> writel(CEVA_PHY1_CFG, base +
AHCI_VEND_PPCFG);
>>>>>> writel(CEVA_TRANS_CFG, base +
AHCI_VEND_PTC);
>>>>>> - if (priv->flag & FLAG_COHERENT) >>>>>> - writel(CEVA_AXICC_CFG, base + >>>>> AHCI_VEND_AXICC); >>>>>> break; >>>>>> } >>>>>> >>>>>> + if (priv->flag & FLAG_COHERENT) >>>>>> + writel(CEVA_AXICC_CFG, base +
AHCI_VEND_AXICC);
>>>>>> + >>>>>> return 0; >>>>>> } >>>>>> >>>>>> @@ -187,6 +185,7 @@ static const struct udevice_id >>>>>> sata_ceva_ids[] = > { >>>>>> { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, >>>>>> { .compatible = "fsl,ls1012a-ahci", .data =
CEVA_LS1012A },
>>>>>> { .compatible = "fsl,ls1021a-ahci", .data = >>>>>> CEVA_LS1021A }, >>>>>> + { .compatible = "fsl,ls1028a-ahci", .data = >>>>>> + CEVA_LS1028A }, >>>>>> { .compatible = "fsl,ls1043a-ahci", .data =
CEVA_LS1043A },
>>>>>> { .compatible = "fsl,ls1046a-ahci", .data =
CEVA_LS1046A },
>>>>>> { .compatible = "fsl,ls1088a-ahci", .data = >>>>>> CEVA_LS1088A }, @@ >>>>>> -205,8 +204,16 @@ static int >>>>>> sata_ceva_ofdata_to_platdata(struct >>>>>> udevice >>>>> *dev) >>>>>> if (priv->base == FDT_ADDR_T_NONE) >>>>>> return -EINVAL; >>>>>> >>>>>> + priv->ecc_base = dev_read_addr_index(dev, 1); >>>>> >>>>> It would be better to do it via reg-name instead of index. >>>>> But that's up to your binding doc. >>>>> >>>> [Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file >>>> sata node as >>> fallows? >>>> >>>> diff --git a/arch/arm/dts/zynqmp.dtsi >>>> b/arch/arm/dts/zynqmp.dtsi index >>>> dfb6ebc..9ad2d84 100644 >>>> --- a/arch/arm/dts/zynqmp.dtsi >>>> +++ b/arch/arm/dts/zynqmp.dtsi >>>> @@ -692,6 +692,7 @@ >>>> compatible = "ceva,ahci-1v84"; >>>> status = "disabled"; >>>> reg = <0x0 0xfd0c0000 0x0
0x2000>;
>>>> + reg-names = "serdes"; >>>> interrupt-parent = <&gic>; >>>> interrupts = <0 133 4>; >>>> #stream-id-cells = <4>; >>> >>> Unfortunately it is not that simple. >>> >>> First of all you need to reflect this in dt binding doc in the kernel. >>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F >>> %2 >>> Fg >>> it >>> .kern >>> >
el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr
>>> >
ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F
>>> >
h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1
>>> >
e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>>> >
0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi
>>> Ybybu6i1t%2Byzo%3D&reserved=0 >>> >>> Did you record your compatible strings anywhere? >>> >> [Peng Ma] Thanks your quick reply. >> You can find our binding document at: >> (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F >> %2 >> Fg >> it >> .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2F
lin
ux.
gi
>> t > % >> >
2Ftree%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-fsl-qoriq.
>> >
txt%3Fh%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7C977d
> 7871292f >> >
48332dbb08d6c2526b32%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C6369 >> >
10056581930082&sdata=KQryE2RvI8jTOAPKGlGnx2NWYdawFUaTURyzd8
> oets4%3 >> D&reserved=0) > > Interesting. Why did you create separate binding doc if you > target the same ceva sata IP core? Any reason not to merge it
together?
> Anyway I see that this was added in 2015. > [Peng Ma] Although they are both ceva sata, our sata driver is different from yours in kernel to Initializa Vendor-specific registers, you could
see our driver at kernel driver/ata/ ahci_qoriq.c.
I have briefly looked at it. They should be merged together. It happened to us too that we send driver out but didn't know who was the vendor in past and then we found out.
>> I think your suggestion is better. But the first register cd >> address was got by > function dev_read_addr(), to keep consistency, I still prefer to > use dev_read_addr_index(). What do you think? > > We didn't have a need to have second address range that's why > reg-names property wasn't used. Do they have all ecc addresses? > [Peng Ma] Some of our socs need to fixed sata errata. So we should write ecc
addr.
I didn't add reg-names to keep consistency of Xilinx sata soc.
Ok. I think that in this case you should pass different .data to driver.
struct platform_data { enum ceva_soc; bool ecc_present; }
It means have current enum followed by bool with "true" for specific IPs which needs to have also ECC range. Then you can better check that binding is correct.
And use _index there.
[Peng Ma] As far as I am concerned, It is not necessary to do this, I would rather lean on the former way as fallows: diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..a1d452a 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -196,6 +196,10 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev);
const void *blob = gd->fdt_blob;gd is suggesting that you use incorrect functions. We are trying to get rid of gd from device drivers. Please take a look at live tree functions if there is any alternative there.
int node = dev_of_offset(dev);struct fdt_resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT; @@ -204,9 +208,13@@
static int sata_ceva_ofdata_to_platdata(struct
udevice *dev)
if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = fdt_get_named_resource(blob, node, "reg", "reg-names","ecc-addr", &res_regs);if (ret) {debug("Error: can't get ecc addresses(ret = %d)!\n",ret); priv->ecc_base = 0;
} elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev); What do you think?One thing I would like to avoid is that we will get any error even in debug for IPs which don't have this ecc space. And also that we will get this errors for IPs which should have this ecc space.
[Peng Ma] OK，changed as fallows:
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..bd98d85 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -8,6 +8,7 @@ #include <ahci.h> #include <scsi.h> #include <asm/io.h> +#include <linux/ioport.h>
/* Vendor Specific Register Offsets */ #define AHCI_VEND_PCFG 0xA4 @@ -196,6 +197,8 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev);
struct resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT; @@ -204,9 +207,11@@
static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = dev_read_resource_byname(dev, "ecc-addr", &res_regs);if (ret) priv->ecc_base = 0;elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev);This looks good but still missing information that in your case there are some versions where this ecc-addr is required. It means if it is required you should error it out when it is not present in DT.
[Peng Ma] Ok, It will return error when some socs need ecc address with the regs is not Present in DT.
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..6e28a38 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -8,6 +8,7 @@ #include <ahci.h> #include <scsi.h> #include <asm/io.h> +#include <linux/ioport.h>
/* Vendor Specific Register Offsets */ #define AHCI_VEND_PCFG 0xA4 @@ -130,8 +131,9 @@ static int ceva_init_sata(struct ceva_sata_priv *priv) break;
case CEVA_LS1021A: - if (ecc_addr) - writel(ECC_DIS_VAL_CH1, ecc_addr); + if (!ecc_addr) + return -EINVAL; + writel(ECC_DIS_VAL_CH1, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(LS1021_CEVA_PHY2_CFG, base + AHCI_VEND_PP2C); writel(LS1021_CEVA_PHY3_CFG, base + AHCI_VEND_PP3C); @@ -143,17 +145,19 @@ static int ceva_init_sata(struct ceva_sata_priv *priv) case CEVA_LS1012A: case CEVA_LS1043A: case CEVA_LS1046A: + if (!ecc_addr) + return -EINVAL; + writel(ECC_DIS_VAL_CH2, ecc_addr); case CEVA_LS2080A: - if (ecc_addr) - writel(ECC_DIS_VAL_CH2, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); break;
case CEVA_LS1028A: case CEVA_LS1088A: - if (ecc_addr) - writel(ECC_DIS_VAL_CH3, ecc_addr); + if (!ecc_addr) + return -EINVAL; + writel(ECC_DIS_VAL_CH3, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); break; @@ -196,6 +200,8 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev); + struct resource res_regs; + int ret;
if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT; @@ -204,9 +210,11 @@ static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
- priv->ecc_base = dev_read_addr_index(dev, 1); - if (priv->ecc_base == FDT_ADDR_T_NONE) + ret = dev_read_resource_byname(dev, "ecc-addr", &res_regs); + if (ret) priv->ecc_base = 0; + else + priv->ecc_base = res_regs.start;
priv->soc = dev_get_driver_data(dev);
Best Regards, peng
Thanks, Michal
On 17. 04. 19 9:27, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月17日 14:58 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 17. 04. 19 8:50, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月17日 13:58 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 17. 04. 19 4:50, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月16日 18:49 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is
safe.
On 16. 04. 19 12:29, Peng Ma wrote: > > >> -----Original Message----- >> From: Michal Simek michal.simek@xilinx.com >> Sent: 2019年4月16日 18:01 >> To: Peng Ma peng.ma@nxp.com; Michal Simek >> michal.simek@xilinx.com; albert.u.boot@aribaud.net; >> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
>> york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com >> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
>> u-boot@lists.denx.de >> Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the >> driver code >> >> WARNING: This email was created outside of NXP. DO NOT CLICK >> links or attachments unless you recognize the sender and know the >> content is
safe.
>> >> >> >> On 16. 04. 19 11:54, Peng Ma wrote: >>> >>> >>>> -----Original Message----- >>>> From: Michal Simek michal.simek@xilinx.com >>>> Sent: 2019年4月16日 17:31 >>>> To: Peng Ma peng.ma@nxp.com; Michal Simek >>>> michal.simek@xilinx.com; albert.u.boot@aribaud.net; >>>> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
>>>> york.sun@nxp.com; Prabhakar Kushwaha >> prabhakar.kushwaha@nxp.com >>>> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
>>>> u-boot@lists.denx.de >>>> Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the >>>> driver code >>>> >>>> WARNING: This email was created outside of NXP. DO NOT CLICK >>>> links or attachments unless you recognize the sender and know >>>> the content is safe. >>>> >>>> >>>> >>>> On 16. 04. 19 11:22, Peng Ma wrote: >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Michal Simek michal.simek@xilinx.com >>>>>> Sent: 2019年4月16日 16:04 >>>>>> To: Peng Ma peng.ma@nxp.com; albert.u.boot@aribaud.net; >>>>>> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com;
York
Sun >>>>>> york.sun@nxp.com; Prabhakar Kushwaha >>>> prabhakar.kushwaha@nxp.com >>>>>> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; >>>>>> michal.simek@xilinx.com; u-boot@lists.denx.de >>>>>> Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the >>>>>> driver code >>>>>> >>>>>> WARNING: This email was created outside of NXP. DO NOT CLICK >>>>>> links or attachments unless you recognize the sender and know >>>>>> the content is >> safe. >>>>>> >>>>>> >>>>>> >>>>>> On 16. 04. 19 9:28, Peng Ma wrote: >>>>>>> Distinguish the ecc val by chassis version and move the ecc >>>>>>> addr to dts. >>>>>>> Add ls1028a soc support. >>>>>>> >>>>>>> Signed-off-by: Peng Ma peng.ma@nxp.com >>>>>>> --- >>>>>>> drivers/ata/sata_ceva.c | 43 >>>>>> +++++++++++++++++++++++++------------------ >>>>>>> 1 files changed, 25 insertions(+), 18 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/ata/sata_ceva.c >>>>>>> b/drivers/ata/sata_ceva.c index >>>>>>> 8887be9..d26f712 100644 >>>>>>> --- a/drivers/ata/sata_ceva.c >>>>>>> +++ b/drivers/ata/sata_ceva.c >>>>>>> @@ -88,20 +88,16 @@ >>>>>>> #define LS1021_CEVA_PHY4_CFG 0x064a080b #define >>>>>> LS1021_CEVA_PHY5_CFG >>>>>>> 0x2aa86470 >>>>>>> >>>>>>> -/* for ls1088a */ >>>>>>> -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 >>>>>>> -#define LS1088_ECC_DIS_VAL_CH2 0x40000000 >>>>>>> - >>>>>>> -/* ecc addr-val pair */ >>>>>>> -#define ECC_DIS_ADDR_CH2 0x20140520 >>>>>>> +/* ecc val pair */ >>>>>>> +#define ECC_DIS_VAL_CH1 0x00020000 >>>>>>> #define ECC_DIS_VAL_CH2 0x80000000 >>>>>>> -#define SATA_ECC_REG_ADDR 0x20220520 >>>>>>> -#define SATA_ECC_DISABLE 0x00020000 >>>>>>> +#define ECC_DIS_VAL_CH3 0x40000000 >>>>>>> >>>>>>> enum ceva_soc { >>>>>>> CEVA_1V84, >>>>>>> CEVA_LS1012A, >>>>>>> CEVA_LS1021A, >>>>>>> + CEVA_LS1028A, >>>>>>> CEVA_LS1043A, >>>>>>> CEVA_LS1046A, >>>>>>> CEVA_LS1088A, >>>>>>> @@ -110,12 +106,14 @@ enum ceva_soc { >>>>>>> >>>>>>> struct ceva_sata_priv { >>>>>>> ulong base; >>>>>>> + ulong ecc_base; >>>>>>> enum ceva_soc soc; >>>>>>> ulong flag; >>>>>>> }; >>>>>>> >>>>>>> static int ceva_init_sata(struct ceva_sata_priv *priv) { >>>>>>> + ulong ecc_addr = priv->ecc_base; >>>>>>> ulong base = priv->base; >>>>>>> ulong tmp; >>>>>>> >>>>>>> @@ -132,38 +130,38 @@ static int ceva_init_sata(struct >>>>>>> ceva_sata_priv >>>>>> *priv) >>>>>>> break; >>>>>>> >>>>>>> case CEVA_LS1021A: >>>>>>> - writel(SATA_ECC_DISABLE,
SATA_ECC_REG_ADDR);
>>>>>>> + if (ecc_addr) >>>>>>> + writel(ECC_DIS_VAL_CH1,
ecc_addr);
>>>>>>> writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); >>>>>>> writel(LS1021_CEVA_PHY2_CFG, base + >>>>>> AHCI_VEND_PP2C); >>>>>>> writel(LS1021_CEVA_PHY3_CFG, base + >>>>>> AHCI_VEND_PP3C); >>>>>>> writel(LS1021_CEVA_PHY4_CFG, base + >>>>>> AHCI_VEND_PP4C); >>>>>>> writel(LS1021_CEVA_PHY5_CFG, base + >>>>>> AHCI_VEND_PP5C); >>>>>>> writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); >>>>>>> - if (priv->flag & FLAG_COHERENT) >>>>>>> - writel(CEVA_AXICC_CFG, base + >>>>>> LS1021_AHCI_VEND_AXICC); >>>>>>> break; >>>>>>> >>>>>>> case CEVA_LS1012A: >>>>>>> case CEVA_LS1043A: >>>>>>> case CEVA_LS1046A: >>>>>>> - writel(ECC_DIS_VAL_CH2,
ECC_DIS_ADDR_CH2);
>>>>>>> - /* fallthrough */ >>>>>>> case CEVA_LS2080A: >>>>>>> + if (ecc_addr) >>>>>>> + writel(ECC_DIS_VAL_CH2,
ecc_addr);
>>>>>>> writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); >>>>>>> writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); >>>>>>> - if (priv->flag & FLAG_COHERENT) >>>>>>> - writel(CEVA_AXICC_CFG, base + >>>>>> AHCI_VEND_AXICC); >>>>>>> break; >>>>>>> >>>>>>> + case CEVA_LS1028A: >>>>>>> case CEVA_LS1088A: >>>>>>> - writel(LS1088_ECC_DIS_VAL_CH2, >>>>>> LS1088_ECC_DIS_ADDR_CH2); >>>>>>> + if (ecc_addr) >>>>>>> + writel(ECC_DIS_VAL_CH3,
ecc_addr);
>>>>>>> writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); >>>>>>> writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); >>>>>>> - if (priv->flag & FLAG_COHERENT) >>>>>>> - writel(CEVA_AXICC_CFG, base + >>>>>> AHCI_VEND_AXICC); >>>>>>> break; >>>>>>> } >>>>>>> >>>>>>> + if (priv->flag & FLAG_COHERENT) >>>>>>> + writel(CEVA_AXICC_CFG, base + AHCI_VEND_AXICC); >>>>>>> + >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> @@ -187,6 +185,7 @@ static const struct udevice_id >>>>>>> sata_ceva_ids[] = >> { >>>>>>> { .compatible = "ceva,ahci-1v84", .data = CEVA_1V84 }, >>>>>>> { .compatible = "fsl,ls1012a-ahci", .data =
CEVA_LS1012A },
>>>>>>> { .compatible = "fsl,ls1021a-ahci", .data = >>>>>>> CEVA_LS1021A }, >>>>>>> + { .compatible = "fsl,ls1028a-ahci", .data = >>>>>>> + CEVA_LS1028A }, >>>>>>> { .compatible = "fsl,ls1043a-ahci", .data =
CEVA_LS1043A },
>>>>>>> { .compatible = "fsl,ls1046a-ahci", .data =
CEVA_LS1046A },
>>>>>>> { .compatible = "fsl,ls1088a-ahci", .data = >>>>>>> CEVA_LS1088A }, @@ >>>>>>> -205,8 +204,16 @@ static int >>>>>>> sata_ceva_ofdata_to_platdata(struct >>>>>>> udevice >>>>>> *dev) >>>>>>> if (priv->base == FDT_ADDR_T_NONE) >>>>>>> return -EINVAL; >>>>>>> >>>>>>> + priv->ecc_base = dev_read_addr_index(dev, 1); >>>>>> >>>>>> It would be better to do it via reg-name instead of index. >>>>>> But that's up to your binding doc. >>>>>> >>>>> [Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file >>>>> sata node as >>>> fallows? >>>>> >>>>> diff --git a/arch/arm/dts/zynqmp.dtsi >>>>> b/arch/arm/dts/zynqmp.dtsi index >>>>> dfb6ebc..9ad2d84 100644 >>>>> --- a/arch/arm/dts/zynqmp.dtsi >>>>> +++ b/arch/arm/dts/zynqmp.dtsi >>>>> @@ -692,6 +692,7 @@ >>>>> compatible = "ceva,ahci-1v84"; >>>>> status = "disabled"; >>>>> reg = <0x0 0xfd0c0000 0x0
0x2000>;
>>>>> + reg-names = "serdes"; >>>>> interrupt-parent = <&gic>; >>>>> interrupts = <0 133 4>; >>>>> #stream-id-cells = <4>; >>>> >>>> Unfortunately it is not that simple. >>>> >>>> First of all you need to reflect this in dt binding doc in the kernel. >>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F >>>> %2 >>>> Fg >>>> it >>>> .kern >>>> >>
el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr
>>>> >>
ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F
>>>> >>
h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1
>>>> >>
e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>>>> >>
0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi
>>>> Ybybu6i1t%2Byzo%3D&reserved=0 >>>> >>>> Did you record your compatible strings anywhere? >>>> >>> [Peng Ma] Thanks your quick reply. >>> You can find our binding document at: >>> (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F >>> %2 >>> Fg >>> it >>> .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2F
lin
ux.
gi >>> t >> % >>> >>
2Ftree%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-fsl-qoriq.
>>> >>
txt%3Fh%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7C977d
>> 7871292f >>> >>
48332dbb08d6c2526b32%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>> 0%7C6369 >>> >>
10056581930082&sdata=KQryE2RvI8jTOAPKGlGnx2NWYdawFUaTURyzd8
>> oets4%3 >>> D&reserved=0) >> >> Interesting. Why did you create separate binding doc if you >> target the same ceva sata IP core? Any reason not to merge it
together?
>> Anyway I see that this was added in 2015. >> > [Peng Ma] Although they are both ceva sata, our sata driver is > different from yours in kernel to Initializa Vendor-specific > registers, you could see our driver at kernel driver/ata/ ahci_qoriq.c.
I have briefly looked at it. They should be merged together. It happened to us too that we send driver out but didn't know who was the vendor in past and then we found out.
>>> I think your suggestion is better. But the first register cd >>> address was got by >> function dev_read_addr(), to keep consistency, I still prefer to >> use dev_read_addr_index(). What do you think? >> >> We didn't have a need to have second address range that's why >> reg-names property wasn't used. Do they have all ecc addresses? >> > [Peng Ma] Some of our socs need to fixed sata errata. So we should > write ecc addr. > I didn't add reg-names to keep consistency of Xilinx sata soc.
Ok. I think that in this case you should pass different .data to driver.
struct platform_data { enum ceva_soc; bool ecc_present; }
It means have current enum followed by bool with "true" for specific IPs which needs to have also ECC range. Then you can better check that binding is correct.
And use _index there.
[Peng Ma] As far as I am concerned, It is not necessary to do this, I would rather lean on the former way as fallows: diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..a1d452a 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -196,6 +196,10 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev);
const void *blob = gd->fdt_blob;gd is suggesting that you use incorrect functions. We are trying to get rid of gd from device drivers. Please take a look at live tree functions if there is any alternative there.
int node = dev_of_offset(dev);struct fdt_resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT; @@ -204,9 +208,13@@
static int sata_ceva_ofdata_to_platdata(struct
udevice *dev)
if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = fdt_get_named_resource(blob, node, "reg", "reg-names","ecc-addr", &res_regs);if (ret) {debug("Error: can't get ecc addresses(ret = %d)!\n",ret); priv->ecc_base = 0;
} elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev); What do you think?One thing I would like to avoid is that we will get any error even in debug for IPs which don't have this ecc space. And also that we will get this errors for IPs which should have this ecc space.
[Peng Ma] OK，changed as fallows:
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..bd98d85 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -8,6 +8,7 @@ #include <ahci.h> #include <scsi.h> #include <asm/io.h> +#include <linux/ioport.h>
/* Vendor Specific Register Offsets */ #define AHCI_VEND_PCFG 0xA4 @@ -196,6 +197,8 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev);
struct resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT; @@ -204,9 +207,11@@
static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = dev_read_resource_byname(dev, "ecc-addr", &res_regs);if (ret) priv->ecc_base = 0;elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev);This looks good but still missing information that in your case there are some versions where this ecc-addr is required. It means if it is required you should error it out when it is not present in DT.
[Peng Ma] Ok, It will return error when some socs need ecc address with the regs is not Present in DT.
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..6e28a38 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -8,6 +8,7 @@ #include <ahci.h> #include <scsi.h> #include <asm/io.h> +#include <linux/ioport.h>
/* Vendor Specific Register Offsets */ #define AHCI_VEND_PCFG 0xA4 @@ -130,8 +131,9 @@ static int ceva_init_sata(struct ceva_sata_priv *priv) break;
case CEVA_LS1021A:
if (ecc_addr)writel(ECC_DIS_VAL_CH1, ecc_addr);
if (!ecc_addr)return -EINVAL;writel(ECC_DIS_VAL_CH1, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(LS1021_CEVA_PHY2_CFG, base + AHCI_VEND_PP2C); writel(LS1021_CEVA_PHY3_CFG, base + AHCI_VEND_PP3C);@@ -143,17 +145,19 @@ static int ceva_init_sata(struct ceva_sata_priv *priv) case CEVA_LS1012A: case CEVA_LS1043A: case CEVA_LS1046A:
if (!ecc_addr)return -EINVAL;writel(ECC_DIS_VAL_CH2, ecc_addr); case CEVA_LS2080A:
if (ecc_addr)writel(ECC_DIS_VAL_CH2, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); break; case CEVA_LS1028A: case CEVA_LS1088A:if (ecc_addr)writel(ECC_DIS_VAL_CH3, ecc_addr);
if (!ecc_addr)return -EINVAL;writel(ECC_DIS_VAL_CH3, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); break;@@ -196,6 +200,8 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev);
struct resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT;@@ -204,9 +210,11 @@ static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = dev_read_resource_byname(dev, "ecc-addr", &res_regs);if (ret) priv->ecc_base = 0;elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev);
ok. Looks good.
M
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月17日 15:38 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 17. 04. 19 9:27, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月17日 14:58 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe.
On 17. 04. 19 8:50, Peng Ma wrote:
-----Original Message----- From: Michal Simek michal.simek@xilinx.com Sent: 2019年4月17日 13:58 To: Peng Ma peng.ma@nxp.com; Michal Simek michal.simek@xilinx.com; albert.u.boot@aribaud.net; sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York Sun york.sun@nxp.com; Prabhakar Kushwaha
Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu yinbo.zhu@nxp.com; u-boot@lists.denx.de Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is
safe.
On 17. 04. 19 4:50, Peng Ma wrote:
> -----Original Message----- > From: Michal Simek michal.simek@xilinx.com > Sent: 2019年4月16日 18:49 > To: Peng Ma peng.ma@nxp.com; Michal Simek > michal.simek@xilinx.com; albert.u.boot@aribaud.net; > sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
> york.sun@nxp.com; Prabhakar Kushwaha
> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
> u-boot@lists.denx.de > Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the > driver code > > WARNING: This email was created outside of NXP. DO NOT CLICK > links or attachments unless you recognize the sender and know the > content is
safe.
> > > > On 16. 04. 19 12:29, Peng Ma wrote: >> >> >>> -----Original Message----- >>> From: Michal Simek michal.simek@xilinx.com >>> Sent: 2019年4月16日 18:01 >>> To: Peng Ma peng.ma@nxp.com; Michal Simek >>> michal.simek@xilinx.com; albert.u.boot@aribaud.net; >>> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com; York
Sun
>>> york.sun@nxp.com; Prabhakar Kushwaha > prabhakar.kushwaha@nxp.com >>> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
>>> u-boot@lists.denx.de >>> Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the >>> driver code >>> >>> WARNING: This email was created outside of NXP. DO NOT CLICK >>> links or attachments unless you recognize the sender and know >>> the content is
safe.
>>> >>> >>> >>> On 16. 04. 19 11:54, Peng Ma wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Michal Simek michal.simek@xilinx.com >>>>> Sent: 2019年4月16日 17:31 >>>>> To: Peng Ma peng.ma@nxp.com; Michal Simek >>>>> michal.simek@xilinx.com; albert.u.boot@aribaud.net; >>>>> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com;
York
Sun
>>>>> york.sun@nxp.com; Prabhakar Kushwaha >>> prabhakar.kushwaha@nxp.com >>>>> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu
>>>>> u-boot@lists.denx.de >>>>> Subject: Re: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the >>>>> driver code >>>>> >>>>> WARNING: This email was created outside of NXP. DO NOT CLICK >>>>> links or attachments unless you recognize the sender and know >>>>> the content is > safe. >>>>> >>>>> >>>>> >>>>> On 16. 04. 19 11:22, Peng Ma wrote: >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Michal Simek michal.simek@xilinx.com >>>>>>> Sent: 2019年4月16日 16:04 >>>>>>> To: Peng Ma peng.ma@nxp.com;
albert.u.boot@aribaud.net;
>>>>>>> sjg@chromium.org; Fabio Estevam fabio.estevam@nxp.com;
York
> Sun >>>>>>> york.sun@nxp.com; Prabhakar Kushwaha >>>>> prabhakar.kushwaha@nxp.com >>>>>>> Cc: Andy Tang andy.tang@nxp.com; Yinbo Zhu > yinbo.zhu@nxp.com; >>>>>>> michal.simek@xilinx.com; u-boot@lists.denx.de >>>>>>> Subject: [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the >>>>>>> driver code >>>>>>> >>>>>>> WARNING: This email was created outside of NXP. DO NOT >>>>>>> CLICK links or attachments unless you recognize the sender >>>>>>> and know the content is >>> safe. >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 16. 04. 19 9:28, Peng Ma wrote: >>>>>>>> Distinguish the ecc val by chassis version and move the >>>>>>>> ecc addr to > dts. >>>>>>>> Add ls1028a soc support. >>>>>>>> >>>>>>>> Signed-off-by: Peng Ma peng.ma@nxp.com >>>>>>>> --- >>>>>>>> drivers/ata/sata_ceva.c | 43 >>>>>>> +++++++++++++++++++++++++------------------ >>>>>>>> 1 files changed, 25 insertions(+), 18 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/ata/sata_ceva.c >>>>>>>> b/drivers/ata/sata_ceva.c index >>>>>>>> 8887be9..d26f712 100644 >>>>>>>> --- a/drivers/ata/sata_ceva.c >>>>>>>> +++ b/drivers/ata/sata_ceva.c >>>>>>>> @@ -88,20 +88,16 @@ >>>>>>>> #define LS1021_CEVA_PHY4_CFG 0x064a080b #define >>>>>>> LS1021_CEVA_PHY5_CFG >>>>>>>> 0x2aa86470 >>>>>>>> >>>>>>>> -/* for ls1088a */ >>>>>>>> -#define LS1088_ECC_DIS_ADDR_CH2 0x100520 >>>>>>>> -#define LS1088_ECC_DIS_VAL_CH2 0x40000000 >>>>>>>> - >>>>>>>> -/* ecc addr-val pair */ >>>>>>>> -#define ECC_DIS_ADDR_CH2 0x20140520 >>>>>>>> +/* ecc val pair */ >>>>>>>> +#define ECC_DIS_VAL_CH1 0x00020000 >>>>>>>> #define ECC_DIS_VAL_CH2 0x80000000 >>>>>>>> -#define SATA_ECC_REG_ADDR 0x20220520 >>>>>>>> -#define SATA_ECC_DISABLE 0x00020000 >>>>>>>> +#define ECC_DIS_VAL_CH3 0x40000000 >>>>>>>> >>>>>>>> enum ceva_soc { >>>>>>>> CEVA_1V84, >>>>>>>> CEVA_LS1012A, >>>>>>>> CEVA_LS1021A, >>>>>>>> + CEVA_LS1028A, >>>>>>>> CEVA_LS1043A, >>>>>>>> CEVA_LS1046A, >>>>>>>> CEVA_LS1088A, >>>>>>>> @@ -110,12 +106,14 @@ enum ceva_soc { >>>>>>>> >>>>>>>> struct ceva_sata_priv { >>>>>>>> ulong base; >>>>>>>> + ulong ecc_base; >>>>>>>> enum ceva_soc soc; >>>>>>>> ulong flag; >>>>>>>> }; >>>>>>>> >>>>>>>> static int ceva_init_sata(struct ceva_sata_priv *priv) { >>>>>>>> + ulong ecc_addr = priv->ecc_base; >>>>>>>> ulong base = priv->base; >>>>>>>> ulong tmp; >>>>>>>> >>>>>>>> @@ -132,38 +130,38 @@ static int ceva_init_sata(struct >>>>>>>> ceva_sata_priv >>>>>>> *priv) >>>>>>>> break; >>>>>>>> >>>>>>>> case CEVA_LS1021A: >>>>>>>> - writel(SATA_ECC_DISABLE,
SATA_ECC_REG_ADDR);
>>>>>>>> + if (ecc_addr) >>>>>>>> + writel(ECC_DIS_VAL_CH1,
ecc_addr);
>>>>>>>> writel(CEVA_PHY1_CFG, base + > AHCI_VEND_PPCFG); >>>>>>>> writel(LS1021_CEVA_PHY2_CFG, base + >>>>>>> AHCI_VEND_PP2C); >>>>>>>> writel(LS1021_CEVA_PHY3_CFG, base + >>>>>>> AHCI_VEND_PP3C); >>>>>>>> writel(LS1021_CEVA_PHY4_CFG, base + >>>>>>> AHCI_VEND_PP4C); >>>>>>>> writel(LS1021_CEVA_PHY5_CFG, base + >>>>>>> AHCI_VEND_PP5C); >>>>>>>> writel(CEVA_TRANS_CFG, base + > AHCI_VEND_PTC); >>>>>>>> - if (priv->flag & FLAG_COHERENT) >>>>>>>> - writel(CEVA_AXICC_CFG, base + >>>>>>> LS1021_AHCI_VEND_AXICC); >>>>>>>> break; >>>>>>>> >>>>>>>> case CEVA_LS1012A: >>>>>>>> case CEVA_LS1043A: >>>>>>>> case CEVA_LS1046A: >>>>>>>> - writel(ECC_DIS_VAL_CH2,
ECC_DIS_ADDR_CH2);
>>>>>>>> - /* fallthrough */ >>>>>>>> case CEVA_LS2080A: >>>>>>>> + if (ecc_addr) >>>>>>>> + writel(ECC_DIS_VAL_CH2,
ecc_addr);
>>>>>>>> writel(CEVA_PHY1_CFG, base + > AHCI_VEND_PPCFG); >>>>>>>> writel(CEVA_TRANS_CFG, base + > AHCI_VEND_PTC); >>>>>>>> - if (priv->flag & FLAG_COHERENT) >>>>>>>> - writel(CEVA_AXICC_CFG, base + >>>>>>> AHCI_VEND_AXICC); >>>>>>>> break; >>>>>>>> >>>>>>>> + case CEVA_LS1028A: >>>>>>>> case CEVA_LS1088A: >>>>>>>> - writel(LS1088_ECC_DIS_VAL_CH2, >>>>>>> LS1088_ECC_DIS_ADDR_CH2); >>>>>>>> + if (ecc_addr) >>>>>>>> + writel(ECC_DIS_VAL_CH3,
ecc_addr);
>>>>>>>> writel(CEVA_PHY1_CFG, base + > AHCI_VEND_PPCFG); >>>>>>>> writel(CEVA_TRANS_CFG, base + > AHCI_VEND_PTC); >>>>>>>> - if (priv->flag & FLAG_COHERENT) >>>>>>>> - writel(CEVA_AXICC_CFG, base + >>>>>>> AHCI_VEND_AXICC); >>>>>>>> break; >>>>>>>> } >>>>>>>> >>>>>>>> + if (priv->flag & FLAG_COHERENT) >>>>>>>> + writel(CEVA_AXICC_CFG, base + > AHCI_VEND_AXICC); >>>>>>>> + >>>>>>>> return 0; >>>>>>>> } >>>>>>>> >>>>>>>> @@ -187,6 +185,7 @@ static const struct udevice_id >>>>>>>> sata_ceva_ids[] = >>> { >>>>>>>> { .compatible = "ceva,ahci-1v84", .data =
CEVA_1V84 },
>>>>>>>> { .compatible = "fsl,ls1012a-ahci", .data =
CEVA_LS1012A },
>>>>>>>> { .compatible = "fsl,ls1021a-ahci", .data = >>>>>>>> CEVA_LS1021A }, >>>>>>>> + { .compatible = "fsl,ls1028a-ahci", .data = >>>>>>>> + CEVA_LS1028A }, >>>>>>>> { .compatible = "fsl,ls1043a-ahci", .data =
CEVA_LS1043A },
>>>>>>>> { .compatible = "fsl,ls1046a-ahci", .data =
CEVA_LS1046A },
>>>>>>>> { .compatible = "fsl,ls1088a-ahci", .data = >>>>>>>> CEVA_LS1088A }, @@ >>>>>>>> -205,8 +204,16 @@ static int >>>>>>>> sata_ceva_ofdata_to_platdata(struct >>>>>>>> udevice >>>>>>> *dev) >>>>>>>> if (priv->base == FDT_ADDR_T_NONE) >>>>>>>> return -EINVAL; >>>>>>>> >>>>>>>> + priv->ecc_base = dev_read_addr_index(dev, 1); >>>>>>> >>>>>>> It would be better to do it via reg-name instead of index. >>>>>>> But that's up to your binding doc. >>>>>>> >>>>>> [Peng Ma] OK, could I change arch/arm/dts/zynqmp.dtsi file >>>>>> sata node as >>>>> fallows? >>>>>> >>>>>> diff --git a/arch/arm/dts/zynqmp.dtsi >>>>>> b/arch/arm/dts/zynqmp.dtsi index >>>>>> dfb6ebc..9ad2d84 100644 >>>>>> --- a/arch/arm/dts/zynqmp.dtsi >>>>>> +++ b/arch/arm/dts/zynqmp.dtsi >>>>>> @@ -692,6 +692,7 @@ >>>>>> compatible = "ceva,ahci-1v84"; >>>>>> status = "disabled"; >>>>>> reg = <0x0 0xfd0c0000 0x0
0x2000>;
>>>>>> + reg-names = "serdes"; >>>>>> interrupt-parent = <&gic>; >>>>>> interrupts = <0 133 4>; >>>>>> #stream-id-cells = <4>; >>>>> >>>>> Unfortunately it is not that simple. >>>>> >>>>> First of all you need to reflect this in dt binding doc in the kernel. >>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A% >>>>> 2F >>>>> %2 >>>>> Fg >>>>> it >>>>> .kern >>>>> >>> >
el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftr
>>>>> >>> >
ee%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-ceva.txt%3F
>>>>> >>> >
h%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7Cc588ddff1b1
>>>>> >>> >
e471f645b08d6c24e54d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>>>>> >>> >
0%7C636910039050031150&sdata=0Gsuo2uI3pot4VAhBr7SQUnCRYT7Xi
>>>>> Ybybu6i1t%2Byzo%3D&reserved=0 >>>>> >>>>> Did you record your compatible strings anywhere? >>>>> >>>> [Peng Ma] Thanks your quick reply. >>>> You can find our binding document at: >>>> (https://eur01.safelinks.protection.outlook.com/?url=https%3A% >>>> 2F >>>> %2 >>>> Fg >>>> it >>>> .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%
2F
lin
ux.
> gi >>>> t >>> % >>>> >>> >
2Ftree%2FDocumentation%2Fdevicetree%2Fbindings%2Fata%2Fahci-fsl-qoriq.
>>>> >>> >
txt%3Fh%3Dv5.1-rc5&data=02%7C01%7Cpeng.ma%40nxp.com%7C977d
>>> 7871292f >>>> >>> >
48332dbb08d6c2526b32%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
>>> 0%7C6369 >>>> >>> >
10056581930082&sdata=KQryE2RvI8jTOAPKGlGnx2NWYdawFUaTURyzd8
>>> oets4%3 >>>> D&reserved=0) >>> >>> Interesting. Why did you create separate binding doc if you >>> target the same ceva sata IP core? Any reason not to merge it
together?
>>> Anyway I see that this was added in 2015. >>> >> [Peng Ma] Although they are both ceva sata, our sata driver is >> different from yours in kernel to Initializa Vendor-specific >> registers, you could > see our driver at kernel driver/ata/ ahci_qoriq.c. > > I have briefly looked at it. They should be merged together. It > happened to us too that we send driver out but didn't know who > was the vendor in past and then we found out. > >>>> I think your suggestion is better. But the first register cd >>>> address was got by >>> function dev_read_addr(), to keep consistency, I still prefer >>> to use dev_read_addr_index(). What do you think? >>> >>> We didn't have a need to have second address range that's why >>> reg-names property wasn't used. Do they have all ecc addresses? >>> >> [Peng Ma] Some of our socs need to fixed sata errata. So we >> should write ecc > addr. >> I didn't add reg-names to keep consistency of Xilinx sata soc. > > Ok. I think that in this case you should pass different .data to driver. > > struct platform_data { > enum ceva_soc; > bool ecc_present; > } > > It means have current enum followed by bool with "true" for > specific IPs which needs to have also ECC range. > Then you can better check that binding is correct. > > And use _index there. [Peng Ma] As far as I am concerned, It is not necessary to do this, I would rather lean on the former way as fallows: diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..a1d452a 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -196,6 +196,10 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct
udevice *dev) {
struct ceva_sata_priv *priv = dev_get_priv(dev);
const void *blob = gd->fdt_blob;gd is suggesting that you use incorrect functions. We are trying to get rid of gd from device drivers. Please take a look at live tree functions if there is any alternative there.
int node = dev_of_offset(dev);struct fdt_resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT; @@ -204,9+208,13
@@
static int sata_ceva_ofdata_to_platdata(struct
udevice *dev)
if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = fdt_get_named_resource(blob, node, "reg","reg-names",
"ecc-addr", &res_regs);if (ret) {debug("Error: can't get ecc addresses(ret =%d)!\n", ret); priv->ecc_base = 0;
} elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev); What do you think?One thing I would like to avoid is that we will get any error even in debug for IPs which don't have this ecc space. And also that we will get this errors for IPs which should have this ecc space.
[Peng Ma] OK，changed as fallows:
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..bd98d85 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -8,6 +8,7 @@ #include <ahci.h> #include <scsi.h> #include <asm/io.h> +#include <linux/ioport.h>
/* Vendor Specific Register Offsets */ #define AHCI_VEND_PCFG 0xA4 @@ -196,6 +197,8 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice
*dev) {
struct ceva_sata_priv *priv = dev_get_priv(dev);
struct resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT; @@ -204,9 +207,11@@
static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = dev_read_resource_byname(dev, "ecc-addr", &res_regs);if (ret) priv->ecc_base = 0;elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev);This looks good but still missing information that in your case there are some versions where this ecc-addr is required. It means if it is required you should error it out when it is not present in DT.
[Peng Ma] Ok, It will return error when some socs need ecc address with the regs is not Present in DT.
diff --git a/drivers/ata/sata_ceva.c b/drivers/ata/sata_ceva.c index d26f712..6e28a38 100644 --- a/drivers/ata/sata_ceva.c +++ b/drivers/ata/sata_ceva.c @@ -8,6 +8,7 @@ #include <ahci.h> #include <scsi.h> #include <asm/io.h> +#include <linux/ioport.h>
/* Vendor Specific Register Offsets */ #define AHCI_VEND_PCFG 0xA4 @@ -130,8 +131,9 @@ static int ceva_init_sata(struct ceva_sata_priv *priv) break;
case CEVA_LS1021A:
if (ecc_addr)writel(ECC_DIS_VAL_CH1, ecc_addr);
if (!ecc_addr)return -EINVAL;writel(ECC_DIS_VAL_CH1, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(LS1021_CEVA_PHY2_CFG, base +AHCI_VEND_PP2C);
writel(LS1021_CEVA_PHY3_CFG, base +AHCI_VEND_PP3C);
@@ -143,17 +145,19 @@ static int ceva_init_sata(struct ceva_sata_priv
*priv)
case CEVA_LS1012A: case CEVA_LS1043A: case CEVA_LS1046A:
if (!ecc_addr)return -EINVAL;writel(ECC_DIS_VAL_CH2, ecc_addr); case CEVA_LS2080A:
if (ecc_addr)writel(ECC_DIS_VAL_CH2, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); break; case CEVA_LS1028A: case CEVA_LS1088A:if (ecc_addr)writel(ECC_DIS_VAL_CH3, ecc_addr);
if (!ecc_addr)return -EINVAL;writel(ECC_DIS_VAL_CH3, ecc_addr); writel(CEVA_PHY1_CFG, base + AHCI_VEND_PPCFG); writel(CEVA_TRANS_CFG, base + AHCI_VEND_PTC); break;@@ -196,6 +200,8 @@ static const struct udevice_id sata_ceva_ids[] = { static int sata_ceva_ofdata_to_platdata(struct udevice *dev) { struct ceva_sata_priv *priv = dev_get_priv(dev);
struct resource res_regs;int ret; if (dev_read_bool(dev, "dma-coherent")) priv->flag |= FLAG_COHERENT; @@ -204,9 +210,11@@
static int sata_ceva_ofdata_to_platdata(struct udevice *dev) if (priv->base == FDT_ADDR_T_NONE) return -EINVAL;
priv->ecc_base = dev_read_addr_index(dev, 1);if (priv->ecc_base == FDT_ADDR_T_NONE)
ret = dev_read_resource_byname(dev, "ecc-addr", &res_regs);if (ret) priv->ecc_base = 0;elsepriv->ecc_base = res_regs.start; priv->soc = dev_get_driver_data(dev);ok. Looks good.
[Peng Ma] Thanks very much for your patient guidance.
Best Regards, Peng
M
participants (2)
-
Michal Simek -
Peng Ma