[U-Boot] [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
Peng Ma
peng.ma at nxp.com
Wed Apr 17 02:50:13 UTC 2019
>-----Original Message-----
>From: Michal Simek <michal.simek at xilinx.com>
>Sent: 2019年4月16日 18:49
>To: Peng Ma <peng.ma at nxp.com>; Michal Simek <michal.simek at xilinx.com>;
>albert.u.boot at aribaud.net; sjg at chromium.org; Fabio Estevam
><fabio.estevam at nxp.com>; York Sun <york.sun at nxp.com>; Prabhakar
>Kushwaha <prabhakar.kushwaha at nxp.com>
>Cc: Andy Tang <andy.tang at nxp.com>; Yinbo Zhu <yinbo.zhu at nxp.com>;
>u-boot at 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 at xilinx.com>
>>> Sent: 2019年4月16日 18:01
>>> To: Peng Ma <peng.ma at nxp.com>; Michal Simek
>>> <michal.simek at xilinx.com>; albert.u.boot at aribaud.net;
>>> sjg at chromium.org; Fabio Estevam <fabio.estevam at nxp.com>; York Sun
>>> <york.sun at nxp.com>; Prabhakar Kushwaha
><prabhakar.kushwaha at nxp.com>
>>> Cc: Andy Tang <andy.tang at nxp.com>; Yinbo Zhu <yinbo.zhu at nxp.com>;
>>> u-boot at 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 at xilinx.com>
>>>>> Sent: 2019年4月16日 17:31
>>>>> To: Peng Ma <peng.ma at nxp.com>; Michal Simek
>>>>> <michal.simek at xilinx.com>; albert.u.boot at aribaud.net;
>>>>> sjg at chromium.org; Fabio Estevam <fabio.estevam at nxp.com>; York Sun
>>>>> <york.sun at nxp.com>; Prabhakar Kushwaha
>>> <prabhakar.kushwaha at nxp.com>
>>>>> Cc: Andy Tang <andy.tang at nxp.com>; Yinbo Zhu <yinbo.zhu at nxp.com>;
>>>>> u-boot at 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 at xilinx.com>
>>>>>>> Sent: 2019年4月16日 16:04
>>>>>>> To: Peng Ma <peng.ma at nxp.com>; albert.u.boot at aribaud.net;
>>>>>>> sjg at chromium.org; Fabio Estevam <fabio.estevam at nxp.com>; York
>Sun
>>>>>>> <york.sun at nxp.com>; Prabhakar Kushwaha
>>>>> <prabhakar.kushwaha at nxp.com>
>>>>>>> Cc: Andy Tang <andy.tang at nxp.com>; Yinbo Zhu
><yinbo.zhu at nxp.com>;
>>>>>>> michal.simek at xilinx.com; u-boot at 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 at 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
More information about the U-Boot
mailing list