[U-Boot] [EXT] Re: [PATCH 1/2] scsi: ceva: Clean up the driver code
Peng Ma
peng.ma at nxp.com
Wed Apr 17 06:50:07 UTC 2019
>-----Original Message-----
>From: Michal Simek <michal.simek at xilinx.com>
>Sent: 2019年4月17日 13:58
>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 17. 04. 19 4:50, Peng Ma wrote:
>>
>>
>>> -----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%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;
>> + } else
>> + priv->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
More information about the U-Boot
mailing list