RE: [PATCH V2 RE-SEND 0/7] add new Samsung SXGbE driver
David Miller wrote: > From: Byungho An > Date: Thu, 13 Mar 2014 15:55:28 +0900 > > > This is 2nd posting for Samsung SXGbE driver and just re-sending because > of > > line wrapping in previous posting. > > > > Changes since v1: > > - changed name of driver to SXGbE as per Ben's comment > > - squashed Joe's neatening for many stuff in original patches > > I'm mostly happy with this driver, but all of those module parameters > have to be removed except for 'debug'. OK, I'll remove them. > > They duplicate functionality provided by ethtool. OK. > > Module parameters are strongly, if not completely, discouraged. A > suitable generic configuration mechanism, such as ethtool, should > always be used instead. OK. I'll repost soon. Thanks > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V2 RE-SEND 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver
Francois Romieu > Byungho An : > > From: Siva Reddy > > > > This patch adds support for Samsung 10Gb ethernet driver(sxgbe). > > - sxgbe core initialization > > - Tx and Rx support > > - MDIO support > > - ISRs for Tx and Rx > > - ifconfig support to driver > > You'll find a partial review below. > > [...] > > diff --git a/drivers/net/ethernet/samsung/sxgbe_common.h > b/drivers/net/ethernet/samsung/sxgbe_common.h > > new file mode 100644 > > index 000..3f16220 > > --- /dev/null > > +++ b/drivers/net/ethernet/samsung/sxgbe_common.h > [...] > > +enum dma_irq_status { > > + tx_hard_error = BIT(0), > > + tx_bump_tc = BIT(1), > > + handle_tx = BIT(2), > > + rx_hard_error = BIT(3), > > + rx_bump_tc = BIT(4), > > + handle_rx = BIT(5), > > Please use tabs before "=" to line things up. OK. > > [...] > > +struct sxgbe_hwtimestamp { > > + void (*config_hw_tstamping)(void __iomem *ioaddr, u32 data); > > + void (*config_sub_second_increment)(void __iomem *ioaddr); > > + int (*init_systime)(void __iomem *ioaddr, u32 sec, u32 nsec); > > + int (*config_addend)(void __iomem *ioaddr, u32 addend); > > + int (*adjust_systime)(void __iomem *ioaddr, u32 sec, u32 nsec, > > + int add_sub); > > + u64 (*get_systime)(void __iomem *ioaddr); > > +}; > > None of these method is ever used. OK. Those methods will be posted later. > > Even annotated with __iomem, I'd rather keep the void * to a minimum and > push the device driver pointer through the call chain. Your call. I think either will be fine. > > [...] > > +struct sxgbe_core_ops { > > + /* MAC core initialization */ > > + void (*core_init)(void __iomem *ioaddr); > > + /* Dump MAC registers */ > > + void (*dump_regs)(void __iomem *ioaddr); > > + /* Handle extra events on specific interrupts hw dependent */ > > + int (*host_irq_status)(void __iomem *ioaddr, > > + struct sxgbe_extra_stats *x); > > + /* Set power management mode (e.g. magic frame) */ > > + void (*pmt)(void __iomem *ioaddr, unsigned long mode); > > + /* Set/Get Unicast MAC addresses */ > > + void (*set_umac_addr)(void __iomem *ioaddr, unsigned char *addr, > > + unsigned int reg_n); > > + void (*get_umac_addr)(void __iomem *ioaddr, unsigned char *addr, > > + unsigned int reg_n); > > + void (*enable_rx)(void __iomem *ioaddr, bool enable); > > + void (*enable_tx)(void __iomem *ioaddr, bool enable); > > + > > + /* controller version specific operations */ > > + int (*get_controller_version)(void __iomem *ioaddr); > > + > > + /* If supported then get the optional core features */ > > + unsigned int (*get_hw_feature)(void __iomem *ioaddr, > > + unsigned char feature_index); > > + /* adjust SXGBE speed */ > > + void (*set_speed)(void __iomem *ioaddr, unsigned char speed); > > +}; > > This indirection level is never used. Those are used, can you give more detail? > > > + > > +const struct sxgbe_core_ops *sxgbe_get_core_ops(void); > > + > > +struct sxgbe_ops { > > + const struct sxgbe_core_ops *mac; > > + const struct sxgbe_desc_ops *desc; > > + const struct sxgbe_dma_ops *dma; > > + const struct sxgbe_mtl_ops *mtl; > > Will these indirection levels ever be used ? Those are used, can you give more detail? > > > + const struct sxgbe_hwtimestamp *ptp; > > + struct mii_regs mii;/* MII register Addresses */ > > + struct mac_link link; > > + unsigned int ctrl_uid; > > + unsigned int ctrl_id; > > +}; > > + > > +/* SXGBE private data structures */ > > +struct sxgbe_tx_queue { > > + u8 queue_no; > > + unsigned int irq_no; > > + struct sxgbe_priv_data *priv_ptr; > > + struct sxgbe_tx_norm_desc *dma_tx; > > You may lay things a bit differently. can you give more detail? > > [...] > > +/* SXGBE HW capabilities */ > > +struct sxgbe_hw_features { > > + /** CAP [0] ***/ > > + unsigned int gmii_1000mbps; > > This field is never read. It will be used later and will be removed in the next post. > > > + unsigned int vlan_hfilter; > > This field is never read. Same above. > > > + unsigned int sma_mdio; > > This field is never read. Same above. > > > + unsigned int pmt_remote_wake_up; > > This field *is* read. > > > + unsigned int pmt_magic_frame; > > So is this one. > > > + unsigned int rmon; > > But this one isn't :o/ > > > + unsigned int arp_offload; > > Sic. > > The storage is a bit expensive. You may pack some boolean into a single > unsigned int. It can be packed but not for all. > > [...] > > +struct sxgbe_priv_data { > > + /* DMA descriptos */ > > + struct sxgbe_tx_queue *txq[SXGBE_TX_QUEUES]; > > + struct sxgbe_rx_queue *rxq[SXGBE_RX_QUEUES]; > > + u8 cur_rx_qnum; > > + > > + unsigned int dma_tx_size; > > + unsigned int dma_rx_size; > > + unsigned int dma_buf_sz; > > + u32 rx_riwt; > > + > > + struct napi_struct napi; > > + > > + vo
Re: [PATCH 1/2] watchdog: s3c2410_wdt: Remove unneeded initialization
Hi Sachin, > Initializing clk to NULL as a reset/error condition does not > help as NULL is not an invalid condition w.r.t clk. Remove this > initialization altogether as there is no state retention. > > Signed-off-by: Sachin Kamat > --- > drivers/watchdog/s3c2410_wdt.c |2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index aba6cd46b45b..a0f8f771adec 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -607,7 +607,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > > err_clk: > clk_disable_unprepare(wdt->clock); > - wdt->clock = NULL; > > err: > return ret; > @@ -627,7 +626,6 @@ static int s3c2410wdt_remove(struct platform_device *dev) > s3c2410wdt_cpufreq_deregister(wdt); > > clk_disable_unprepare(wdt->clock); > - wdt->clock = NULL; > > return 0; > } Patch has been added to linux-watchdog-next. Kind regards, Wim. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] watchdog: s3c2410_wdt: Check return value of clk_prepare_enable
Hi Sachin, > clk_prepare_enable can fail. Check its return value. > > Signed-off-by: Sachin Kamat > --- > drivers/watchdog/s3c2410_wdt.c |6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index a0f8f771adec..7c6ccd071baf 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -525,7 +525,11 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > goto err; > } > > - clk_prepare_enable(wdt->clock); > + ret = clk_prepare_enable(wdt->clock); > + if (ret < 0) { > + dev_err(dev, "failed to enable clock\n"); > + return ret; > + } > > ret = s3c2410wdt_cpufreq_register(wdt); > if (ret < 0) { Patch has been added to linux-watchdog-next. Kind regards, Wim. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12
On 15.03.2014 12:36, Kyungmin Park wrote: On Sat, Mar 15, 2014 at 2:35 AM, Tomasz Figa wrote: Hi Chanwoo, Mark, On 14.03.2014 11:56, Chanwoo Choi wrote: Hi Mark, On 03/14/2014 07:35 PM, Mark Rutland wrote: On Fri, Mar 14, 2014 at 07:14:37AM +, Chanwoo Choi wrote: Hi Mark, On 03/14/2014 02:53 AM, Mark Rutland wrote: On Thu, Mar 13, 2014 at 08:17:29AM +, Chanwoo Choi wrote: This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization and then busfreq driver adjusts dynamically the operating frequency/voltage by using DEVFREQ Subsystem. Signed-off-by: Chanwoo Choi --- .../devicetree/bindings/devfreq/exynos4_bus.txt| 49 ++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt new file mode 100644 index 000..2a83fcc --- /dev/null +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt @@ -0,0 +1,49 @@ + +Exynos4210/4x12 busfreq driver +- + +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage +scaling according to PPMU counters of memory controllers + +Required properties: +- compatible : should contain Exynos4 SoC type as follwoing: + - "samsung,exynos4x12-busfreq" for Exynos4x12 + - "samsung,exynos4210-busfreq" for Exynos4210 Is there a device called "busfreq"? What device does this binding describe? I'll add detailed description of busfreq as following: "busfreq(bus frequendcy)" driver means that busfreq driver control dynamically memory bus frequency/voltage by checking memory bus utilization to optimize power-consumption. When checking memeory bus utilization, exynos4_busfreq driver would use PPMU(Performance Profiling Monitoring Units). This still sounds like a description of the _driver_, not the _device_. The binding should describe the hardware, now the high level abstraction that software is going to build atop of it. It sounds like this is a binding for the DMC PPMU? Is the PPMU a component of the DMC, or is it bolted on the side? PPMU(Performance Profiling Monitoring Unit) is to profile performance event of various IP on Exynos4. Each PPMU provide perforamnce event for each IP. We can check various PPMU as following: PPMU_3D PPMU_ACP PPMU_CAMIF PPMU_CPU PPMU_DMC0 PPMU_DMC1 PPMU_FSYS PPMU_IMAGE PPMU_LCD0 PPMU_LCD1 PPMU_MFC_L PPMU_MFC_R PPMU_TV PPMU_LEFT_BUS PPMU_RIGHT_BUS DMC (Dynamic Memory Controller) control the operation of DRAM in Exynos4 SoC. If we need to get memory bust utilization of DMC, we can get memory bus utilization from PPMU_DMC0/PPMU_DMC1. So, Exynos4's busfreq used two(PPMU_DMC0/PPMU_DMC1) among upper various PPMU list. Well, PPMUs and DMCs are separate hardware blocks found inside Exynos SoCs. Busfreq/devfreq is just a Linux-specific abstraction responsible for collecting data using PPMUs and controlling frequencies and voltages of appropriate power planes, vdd_int responsible for powering DMC0 and DMC1 blocks in this case. I'm afraid that the binding you're proposing is unfortunately incorrect, because it represents the software abstraction, not the real hardware. Instead, this should be separated into several independent bindings: - PPMU bindings to list all the PPMU instances present in the SoC and resources they need, - power plane bindings, which define a power plane in which multiple IP blocks might reside, can be monitored by one or more PPMU units and frequency and voltage of which can be configured according to determined performance level. Needed resources will be clocks and regulators to scale and probably also operating points. Then, exynos-busfreq driver should bind to such power planes, parse necessary data from DT (list of PPMUs and IP blocks, clocks, regulators and operating points) and register a devfreq entity. How about to use component DT like DRM? Well, basically this is what I proposed. Each "power plane" would be a "subsystem" built from components - PPMUs, IP blocks, clocks, regulators, etc, specified in DT by existing means, e.g. ppmu_disp: ppmu@1234 { /* Resources of PPMU */ } fimd: fimd@2345 { /* Resources of FIMD */ }; power-plane-display { compatible = "samsung,power-plane"; samsung,ppmus = <&ppmu_disp>, ...; samsung,devices = <&fimd>, ...; clock-names = "aclk_xxx", "sclk_xxx", ...; clocks = ...; vdd_xxx-supply = ...; }; However I'm still wondering whether there shouldn't be a relation between power planes and power domains and simply existing power d
Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12
On Sat, Mar 15, 2014 at 2:35 AM, Tomasz Figa wrote: > Hi Chanwoo, Mark, > > > On 14.03.2014 11:56, Chanwoo Choi wrote: >> >> Hi Mark, >> >> On 03/14/2014 07:35 PM, Mark Rutland wrote: >>> >>> On Fri, Mar 14, 2014 at 07:14:37AM +, Chanwoo Choi wrote: Hi Mark, On 03/14/2014 02:53 AM, Mark Rutland wrote: > > On Thu, Mar 13, 2014 at 08:17:29AM +, Chanwoo Choi wrote: >> >> This patch add busfreq driver for Exynos4210/Exynos4x12 memory >> interface >> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according >> to PPMU >> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 >> SoC provides >> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus >> utilization >> and then busfreq driver adjusts dynamically the operating >> frequency/voltage >> by using DEVFREQ Subsystem. >> >> Signed-off-by: Chanwoo Choi >> --- >> .../devicetree/bindings/devfreq/exynos4_bus.txt| 49 >> ++ >> 1 file changed, 49 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/devfreq/exynos4_bus.txt >> >> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt >> b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt >> new file mode 100644 >> index 000..2a83fcc >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt >> @@ -0,0 +1,49 @@ >> + >> +Exynos4210/4x12 busfreq driver >> +- >> + >> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus >> frequency/voltage >> +scaling according to PPMU counters of memory controllers >> + >> +Required properties: >> +- compatible : should contain Exynos4 SoC type as follwoing: >> + - "samsung,exynos4x12-busfreq" for Exynos4x12 >> + - "samsung,exynos4210-busfreq" for Exynos4210 > > > Is there a device called "busfreq"? What device does this binding > describe? I'll add detailed description of busfreq as following: "busfreq(bus frequendcy)" driver means that busfreq driver control dynamically memory bus frequency/voltage by checking memory bus utilization to optimize power-consumption. When checking memeory bus utilization, exynos4_busfreq driver would use PPMU(Performance Profiling Monitoring Units). >>> >>> >>> This still sounds like a description of the _driver_, not the _device_. >>> The binding should describe the hardware, now the high level abstraction >>> that software is going to build atop of it. >>> >>> It sounds like this is a binding for the DMC PPMU? >>> >>> Is the PPMU a component of the DMC, or is it bolted on the side? >> >> >> PPMU(Performance Profiling Monitoring Unit) is to profile performance >> event of >> various IP on Exynos4. Each PPMU provide perforamnce event for each IP. >> We can check various PPMU as following: >> >> PPMU_3D >> PPMU_ACP >> PPMU_CAMIF >> PPMU_CPU >> PPMU_DMC0 >> PPMU_DMC1 >> PPMU_FSYS >> PPMU_IMAGE >> PPMU_LCD0 >> PPMU_LCD1 >> PPMU_MFC_L >> PPMU_MFC_R >> PPMU_TV >> PPMU_LEFT_BUS >> PPMU_RIGHT_BUS >> >> DMC (Dynamic Memory Controller) control the operation of DRAM in Exynos4 >> SoC. >> If we need to get memory bust utilization of DMC, we can get memory bus >> utilization >> from PPMU_DMC0/PPMU_DMC1. >> >> So, Exynos4's busfreq used two(PPMU_DMC0/PPMU_DMC1) among upper various >> PPMU list. > > > Well, PPMUs and DMCs are separate hardware blocks found inside Exynos SoCs. > Busfreq/devfreq is just a Linux-specific abstraction responsible for > collecting data using PPMUs and controlling frequencies and voltages of > appropriate power planes, vdd_int responsible for powering DMC0 and DMC1 > blocks in this case. > > I'm afraid that the binding you're proposing is unfortunately incorrect, > because it represents the software abstraction, not the real hardware. > > Instead, this should be separated into several independent bindings: > > - PPMU bindings to list all the PPMU instances present in the SoC and > resources they need, > > - power plane bindings, which define a power plane in which multiple IP > blocks might reside, can be monitored by one or more PPMU units and > frequency and voltage of which can be configured according to determined > performance level. Needed resources will be clocks and regulators to scale > and probably also operating points. > > Then, exynos-busfreq driver should bind to such power planes, parse > necessary data from DT (list of PPMUs and IP blocks, clocks, regulators and > operating points) and register a devfreq entity. How about to use component DT like DRM? > > Best regards, > Tomasz > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vge