From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ivan Khoronzhuk Date: Wed, 08 Jul 2015 23:42:04 +0300 Subject: [U-Boot] [PATCH] keystone2: use appropriate HD field for destination port In-Reply-To: <559D7884.2020504@globallogic.com> References: <1436370333-4010-1-git-send-email-vitalya@ti.com> <559D521C.30205@linaro.org> <559D586F.5020702@ti.com> <559D585F.4060009@linaro.org> <559D5D57.2010106@ti.com> <559D62D5.2040507@globallogic.com> <559D6BE5.9010301@ti.com> <559D7884.2020504@globallogic.com> Message-ID: <559D8B1C.40702@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 08.07.15 22:22, ivan.khoronzhuk wrote: > Vitaly, > > On 08.07.15 21:28, Vitaly Andrianov wrote: >> >> >> On 07/08/2015 01:50 PM, ivan.khoronzhuk wrote: >>> Vitaly, >>> >>> On 08.07.15 20:26, Vitaly Andrianov wrote: >>>> >>>> >>>> On 07/08/2015 01:05 PM, Ivan Khoronzhuk wrote: >>>>> Vitaly, >>>>> >>>>> On 08.07.15 20:05, Vitaly Andrianov wrote: >>>>>> >>>>>> >>>>>> On 07/08/2015 12:38 PM, Ivan Khoronzhuk wrote: >>>>>>> Hi, Vitaly >>>>>>> >>>>>>> I suppose it's better to decide in upper driver how to use swinfo >>>>>>> field. >>>>>>> Like in drivers/net/keystone_net.c >>>>>>> >>>>>>> The keystone navigator supposed to be used as a tool for >>>>>>> communicating >>>>>>> between different IPs, and each of them decide how to use swinfo >>>>>>> fields. >>>>>>> It's protocol specific information and should be known only in >>>>>>> sending >>>>>>> parts. What if tomorrow you will decide to send some packet to PA?, >>>>>>> you >>>>>>> will rewrite this function again? >>>>>>> >>>>>>> It's not the place for such kind information. >>>>>>> >>>>>>> Even more, this is the h/w specific decision and no need to check >>>>>>> this >>>>>>> for each sent packet. You better statically assign how to use this >>>>>>> field >>>>>>> depending on h/w revision, using #if. >>>>>>> >>>>>>> On 08.07.15 18:45, Vitaly Andrianov wrote: >>>>>>>> K2L and L2E have different from K2HK EthSS version, which uses >>>>>>>> tag_info >>>>>>>> field for destination slave port. This commit adds the >>>>>>>> dest_port_info >>>>>>>> field >>>>>>>> to the struct pktdma_cfg, to configure which HD filed tag_info or >>>>>>>> pkt_info >>>>>>>> shall be used to configure descriptor. >>>>>>>> >>>>>>>> Before that commit the swinfo[2] was used for that purpose. Even if >>>>>>>> that >>>>>>>> worked on K2HK devices, the correct field for K2HK is the pkt_info. >>>>>>>> >>>>>>>> The netcp_send() configure appropriate HD info field depending on >>>>>>>> the >>>>>>>> direct_info of the currently using netcp. >>>>>>>> >>>>>>>> Signed-off-by: Vitaly Andrianov >>>>>>>> Acked-by: Murali Karicheri >>>>>>>> --- >>>>>>>> arch/arm/include/asm/ti-common/keystone_nav.h | 9 ++++++++- >>>>>>>> drivers/dma/keystone_nav.c | 12 ++++++++++-- >>>>>>>> drivers/net/keystone_net.c | 3 +-- >>>>>>>> 3 files changed, 19 insertions(+), 5 deletions(-) >>>>>>>> >>>>>>>> diff --git a/arch/arm/include/asm/ti-common/keystone_nav.h >>>>>>>> b/arch/arm/include/asm/ti-common/keystone_nav.h >>>>>>>> index 696d8c6..5a0e391 100644 >>>>>>>> --- a/arch/arm/include/asm/ti-common/keystone_nav.h >>>>>>>> +++ b/arch/arm/include/asm/ti-common/keystone_nav.h >>>>>>>> @@ -152,6 +152,11 @@ struct rx_flow_regs { >>>>>>>> u32 thresh[3]; >>>>>>>> }; >>>>>>>> >>>>>>>> +enum dest_port_info { >>>>>>>> + PKT_INFO, >>>>>>>> + TAG_INFO >>>>>>>> +}; >>>>>>>> + >>>>>>>> struct pktdma_cfg { >>>>>>>> struct global_ctl_regs *global; >>>>>>>> struct tx_chan_regs *tx_ch; >>>>>>>> @@ -167,6 +172,7 @@ struct pktdma_cfg { >>>>>>>> u32 tx_snd_q; >>>>>>>> >>>>>>>> u32 rx_flow; /* flow that is used for RX */ >>>>>>>> + enum dest_port_info dest_port_info;/* HD fiels for dest >>>>>>>> port >>>>>>>> bits */ >>>>>>>> }; >>>>>>>> >>>>>>>> extern struct pktdma_cfg netcp_pktdma; >>>>>>>> @@ -184,7 +190,8 @@ struct rx_buff_desc { >>>>>>>> >>>>>>>> int ksnav_close(struct pktdma_cfg *pktdma); >>>>>>>> int ksnav_init(struct pktdma_cfg *pktdma, struct rx_buff_desc >>>>>>>> *rx_buffers); >>>>>>>> -int ksnav_send(struct pktdma_cfg *pktdma, u32 *pkt, int num_bytes, >>>>>>>> u32 swinfo2); >>>>>>>> +int ksnav_send(struct pktdma_cfg *pktdma, u32 *pkt, int num_bytes, >>>>>>>> + u32 dest_port); >>>>>>>> void *ksnav_recv(struct pktdma_cfg *pktdma, u32 **pkt, int >>>>>>>> *num_bytes); >>>>>>>> void ksnav_release_rxhd(struct pktdma_cfg *pktdma, void *hd); >>>>>>>> >>>>>>>> diff --git a/drivers/dma/keystone_nav.c >>>>>>>> b/drivers/dma/keystone_nav.c >>>>>>>> index dfca75a..64b1cee 100644 >>>>>>>> --- a/drivers/dma/keystone_nav.c >>>>>>>> +++ b/drivers/dma/keystone_nav.c >>>>>>>> @@ -278,7 +278,8 @@ int ksnav_close(struct pktdma_cfg *pktdma) >>>>>>>> return QM_OK; >>>>>>>> } >>>>>>>> >>>>>>>> -int ksnav_send(struct pktdma_cfg *pktdma, u32 *pkt, int num_bytes, >>>>>>>> u32 swinfo2) >>>>>>>> +int ksnav_send(struct pktdma_cfg *pktdma, u32 *pkt, int num_bytes, >>>>>>>> + u32 dest_port) >>>>>>>> { >>>>>>>> struct qm_host_desc *hd; >>>>>>>> >>>>>>>> @@ -286,8 +287,15 @@ int ksnav_send(struct pktdma_cfg *pktdma, u32 >>>>>>>> *pkt, int num_bytes, u32 swinfo2) >>>>>>>> if (hd == NULL) >>>>>>>> return QM_ERR; >>>>>>>> >>>>>>>> + dest_port &= 0xf; >>>>>>>> hd->desc_info = num_bytes; >>>>>>>> - hd->swinfo[2] = swinfo2; >>>>>>>> + if (pktdma->dest_port_info == PKT_INFO) { >>>>>>>> + hd->packet_info = qm_cfg->qpool_num | (dest_port << >>>>>>>> 16); >>>>>>>> + } else { >>>>>>>> + hd->packet_info = qm_cfg->qpool_num; >>>>>>>> + hd->tag_info = dest_port; >>>>>>>> + } >>>>>>>> + >>>>>>>> hd->packet_info = qm_cfg->qpool_num; >>>>>>>> >>>>>>>> qm_buff_push(hd, pktdma->tx_snd_q, pkt, num_bytes); >>>>>>>> diff --git a/drivers/net/keystone_net.c >>>>>>>> b/drivers/net/keystone_net.c >>>>>>>> index 0c5fdee..e2adb67 100644 >>>>>>>> --- a/drivers/net/keystone_net.c >>>>>>>> +++ b/drivers/net/keystone_net.c >>>>>>>> @@ -381,8 +381,7 @@ int32_t cpmac_drv_send(u32 *buffer, int >>>>>>>> num_bytes, >>>>>>>> int slave_port_num) >>>>>>>> if (num_bytes < EMAC_MIN_ETHERNET_PKT_SIZE) >>>>>>>> num_bytes = EMAC_MIN_ETHERNET_PKT_SIZE; >>>>>>>> >>>>>>>> - return ksnav_send(&netcp_pktdma, buffer, >>>>>>>> - num_bytes, (slave_port_num) << 16); >>>>>>>> + return ksnav_send(&netcp_pktdma, buffer, num_bytes, >>>>>>>> slave_port_num); >>>>>>>> } >>>>>>>> >>>>>>>> /* Eth device open */ >>>>>>>> >>>>>>> >>>>>> Hi Ivan, >>>>>> >>>>>> I agree with you. And probably we will need to implement your >>>>>> proposal >>>>>> in future commits. This commit is to fix the bug, which is in >>>>>> existing >>>>>> driver. >>>>>> >>>>>> Thanks, >>>>>> Vitaly >>>>> >>>>> Sorry, I supposed that first msg was not sent. >>>>> It's better to fix it in drivers/net/keystone_net.c >>>>> >>>>> >>>> Ivan, >>>> >>>> I don't understand your proposal. The network driver doesn't know >>>> anything about CPPI DMA engine internals. That is the navigator's >>>> job to >>>> fill appropriate packet BD fields and it only takes care about HW >>>> version differences. >>> >>> You have Eth h/w, you have pktDMA engine to communicate through, you >>> have network driver that knows how to use pktDMA to communicate with >>> Ethernet switch. >>> CPII DMA engine can be used to communicate with different h/w, not only >>> with Ethernet switch. This can be used to send packets to PA, to SA, to >>> QMSS PDSPs, to PA PDSPs and others parts, I don't talk even about AIF, >>> SRIO, FFTC and others IPs that have Pktdma on board. >>> >>> Each of them can use swinfo for different purposes. >>> >>> In the chain to communicate directly with Eth you use net driver >>> in the way keystone_net -> keystone_nav -> h/w pktdma -> eht >>> You can use it also like keystone_srio -> keystone_nav -> h/w pktdma -> >>> srio. >>> etc. >>> >>> pktdma engine it's like a bus used to communicate with a h/w. So, it's >>> not correct to fix driver specific issues in the bus s/w, that is used >>> by different drivers. >>> >> OK. What is your proposal? How to fix the issue in the keystone_net.c, >> which doesn't have any idea about structure of buffer descriptors. And >> actually doesn't have to know about it. >> >> The proposed patch fixes the bug of existing upstreamed driver and allow >> K2E and K2L users to work. It is fully tested on all supported devices. >> >> The PA PDSP and SA drivers currently doesn't exist in u-boot and I don't >> have any information whether anybody is going to develop them in the >> nearest future. >> > > It doesn't matter exist those drivers or not. It's a structured model. > If, for instance, you are going to fix some errata in I2C RTC, what > will you do? fix it in I2C driver? But better place is RTC driver. > > It's partly an API issue. It should be allowed to assign protocol > specific flags field in packet_info and allow others to use swinfo in > the same time. > > It be better to add to pktdma_config field like protocol_flags. > > in keystone_nav send function do: > hd->packet_info = qm_cfg->qpool_num | > (qm_cfg->protocol_flags) & 0xf << 16; > > > and in keystone_net, statically: > #if (REVISION == REVISION_WITH_BUG) > qm_cfg->protocol_flags = dest_port; > #else > qm_cfg->protocol_flags = 0; > > But I'm not sure about one thing. Do you have revisions that use swinfo > for port number and cannot use packet_info for this goal? In this case > you should support version when software field is used also. > Or even better to add new structure like pkt_data with vars: struct pkt_data { int protocol_flags; u32 tags; u32 swinfo[2]; char *data; (that is pkt) int num_bytes; }; and int ksnav_send(struct pktdma_cfg *pktdma, struct pkt_data *pkt) { .... hd->packet_info = qm_cfg->qpool_num | ((pkt->protocol_flags) & 0xf) << 16; hd->tag_info = tags; .... } and in keystone_net, statically: #if (REVISION == REVISION_WITH_BUG) pkt->protocol_flags = dest_port; #else pkt->protocol_flags = 0; pkt->tags = dest_port; In this case each driver will be doing what it's allowed to. It can be done with two patches. >>>> >>>> Thanks, >>>> Vitaly >>>> _______________________________________________ >>>> U-Boot mailing list >>>> U-Boot at lists.denx.de >>>> http://lists.denx.de/mailman/listinfo/u-boot >> _______________________________________________ >> U-Boot mailing list >> U-Boot at lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot